Re: WG Last Call on draft-ietf-secsh-publickey-subsystem-02

[Jon Bright <jon%siliconcircus.com@localhost>]

Hi,

der Mouse wrote:
> In 2.1:
>    The public-key subsystem is opened when the clients sends a
>    SSH_MSG_CHANNEL_REQUEST over an existing session.
> This has so many minor mistakes that rather than try to fix each one
> individually, I'll just propose a rewrite:
>    The public-key subsystem is opened by a client sending a
>    SSH_MSG_CHANNEL_REQUEST over an existing session's channel.

I've changed this to:
    The public-key subsystem is started by a client sending an
    SSH_MSG_CHANNEL_REQUEST over an existing session's channel.

> Fourteen lines further down in 2.1:
>    Client implementations SHOULD reject this request; it is normally
>    only sent by the client.
> s/only sent/sent only/

Changed.

> Nine lines further down:
>    authenticated with a restricted public key that does not allow access
>    to the publickey subsystem.
> Proposed rewrite:
>    authenticated by a means (such as a restricted public key) that does
>    not allow access to the publickey subsystem.

I've written this so (since I figure it's not necessarily just dependent 
on authentication method):

    The server SHOULD respond with SSH_MSG_CHANNEL_FAILURE if the
    user is not allowed access to the publickey subsystem (for example
    because the user authenticated with a restricted public key).

> In 2.2:
>    The length field describes the length of the request-name field and
>    the request-specific data, but not of the length field itself.  The
> s/but not of/but does not include/

I've done s/but not of/but does not include the length of/.  I'm not 
sure this makes things clearer, but it's difficult to come up with 
something that's both grammatically and technically clear.

> A few lines further down, I see "the...'data' portion of the packet",
> but there is no field called "data":
>         uint32    length
>         string    request-name
>         ... request specific data follows
> My preferred fix here is to s/'data'/the data/.

I now have "...of the 'name' field and the data part of the packet.". 
(It's no longer 'request-name' since it now covers responses too, see 
below.)

> I'd like to see something explicit in 2.3 stating that the length works
> the same way it does in 2.2 - perhaps 2.2 and the non-subsectioned
> portion of 2.3 could be collapsed, since they otherwise will include a
> lot of duplicated, or nearly duplicated, language?

I've collapsed the two sections together.

> In 2.3.1.1, I see no description of what the intended semantics of the
> return codes are, except for the description implicit in their names.
> Do we have consensus that this is sufficient?  While I think it
> probably is for native anglophones, I don't have any other perspective
> on it myself - and even if it is sufficient, I'd prefer to see a short
> statement to that effect.

I've added:

    If a request completed successfully, the server MUST send the
    status code SSH_PUBLICKEY_SUCCESS.  The meaning of the failure
    codes is as implied by their names.

Also, I've added this clarification to the description of the version 
packet:

    The SSH_PUBLICKEY_VERSION_NOT_SUPPORTED status code must not be sent
    in response to any other request.
	
> Section 3 ("Public-Key Subsystem Operations") starts by saing that
> "[t]he public-key subsystem currently defines four operations: add,
> remove, list, and listattributes", and then proceeds to section 3.1,
> which describes something (version negotiation) that does not appear to
> be any of those four and therefore I would argue does not belong in
> section 3 as it currently stands.

I've moved it to section 2.

> In 3.1, in case of an unsupported version, "[b]efore closing the
> subsystem, a status message with the status
> SSH_PUBLICKEY_VERSION_NOT_SUPPORTED SHOULD be sent".  But 2.3.1, which
> describes status messages, writes of them only as responses to
> requests, and the version packet is not described as a request (though
> it fits the format of requests).

It says "A request is acknowledged by sending a status packet.", without 
excluding the possibility that the status packet might be used for other 
things, such as here.

> I'm not sure this is worth fixing, though I'd prefer to see a brief
> note recognizing this slight mismatch, such as
>    ....  Before closing the subsystem, a status message with the status
>    SSH_PUBLICKEY_VERSION_NOT_SUPPORTED SHOULD be sent, even though the
>    "version" packet is not a normal request.

I've added this note instead:

    Note that normally, status messages are only sent by the server in
    response to requests from the client.  This is the only occasion on
    which the client sends a status message.

I've also changed the title "The Status Response" to "The Status Message".

> In 3.2, I see
>    algorithm names in [1].  If the server does not implement a mandatory
>    attribute, it MUST fail the add.
> but, in contrast to the error conditions described in the previous
> paragraph, I see no indication of what error code should be used.  (I
> also don't see any error code whose name makes it look suited to this
> error condition, and GENERAL_FAILURE is notably unhelpful.)

I've added an SSH_PUBLICKEY_ATTRIBUTE_NOT_SUPPORTED status code (9) and 
referenced it here.

> In 3.2, describing the "comment-language" attribute,
>    [5].  The client MAY specify more than comment if it additionally
>    specifies a different language for each of those comments.  The
> Based on the context, I believe s/more than /&one / is needed here.

Done.

> In 3.2, describing the "subsystem" attribute, on first sight, it
> appears that there is no way to specify a key that permits any
> subsystem.  (Presumably the way is to not specify the attribute at all.
> But this is not obvious at first sight; indeed, it wasn't until I was
> writing the first version of this paragraph that I realized it.)

I've added
   If the "subsystem" attribute is not specified, no restrictions
   are placed on which subsystems may be started when authenticated
   using this key.

> In 3.3, no indication is given of what to do if the removal succeeds,
> or for that matter if it fails.  (I assume SUCCESS and KEY_NOT_FOUND
> are the principal return codes, with a few others like ACCESS_DENIED
> possible, but I'd still rather see it explicit.)

I think success is now covered by the text added in the status codes 
section.  We could explicitly cover KEY_NOT_FOUND (maybe with a SHOULD). 
 I wouldn't want to get into discussing when ACCESS_DENIED should be 
sent, I think.

> In 3.4, no explicit provision is made for servers that support the
> request but choose to deny it for the client in question.  I'd prefer
> to see this changed.

I could add (indeed, did add, but took it out again):

    An implementation MAY also choose to respond with
    SSH_PUBLICKEY_ACCESS_DENIED if it does not wish to provide the list
    for a given client.

I'm dubious - implementations may choose to do that for any of the other 
requests.  Unless I've missed something, there's nothing stopping them, 
which would imply adding similar text for each request, which seems like 
overkill.

> In 3.2 and 3.5, no provision is made for a server that supports a
> specific attribute but is administratively set to impose a lack of that
> attribute - only for imposing the presence of an attribute.  

The attributes are all negative, they disallow or restrict things.  I 
can't see when the server administrator would want to insist on someone 
absolutely, positively having to have (say) X11 forwarding allowed for 
every key they define.

> There also
> is no provision for cases such as imposing certain hosts' presence or
> absence in (say) the "agent" list.

I presume you mean the "from" list?  For presence, I again don't see the 
case where an admin wants this.  For absence, I've added the following 
text to the "from" description:

    The server MAY provide a method for administrators to disallow the
    appearance of a host in this list.

> In 5.2.1, there is a restriction in that the length limit applies even
> to domain-localized names.  This seems semi-broken, since FQDNs can be
> relatively long (though 64 seems generous now, I have little confidence
> it will remain so - I already have a private name 43 characters long).

You also have one 22 characters long, though (rodents.montreal.qc.ca).

> Unless the "names" in "[n]ames are case-sensitive, and MUST NOT be
> longer than 64 characters" refers to just the part before the @, in
> which case I think the language needs clarification - this section uses
> "name" to refer to both the whole string and just the part before the
> @, which is confusing; consider "Names with the at-sign in them will
> have the format of "name@domainname" (without the double quotes) where
> the part preceeding the at-sign is the name.", which uses the word in
> both senses in the same sentence.

This text (and the length) is directly taken from the core numbers 
draft.  I figure the potential of people not being able to come up with 
64-character names is not worth the inconsistency with that - though 
I'll happily change the publickey subsystem's text if the text in the 
numbers draft changes...

I'm just about to respin the draft based on these changes (and because 
the current version is due to expire in September).

--
Jon Bright
Silicon Circus Ltd.
http://www.siliconcircus.com