Subject: RE: publickey subsystem

From: "Richard Whalen" <[email-masked]>
Date: Wed, 10 Aug 2005 08:56:09 -0400

I've implemented both versions.  I've used the version 1 implementation with the VanDyke software pieces, but I have not done any interoperability testing with the current version.

I have no issues with the current draft, and I think that it is ready for WG Last Call.

----------------------
Richard Whalen
Process Software

---

From: Bill Sommerfeld <[email-masked]>
Date: Wed, 10 Aug 2005 13:27:49 -0400

This message marks the start of a Working Group Last Call on
draft-ietf-secsh-publickey-subsystem-02, for publication as a Proposed
Standard.  This last call period expires on August 24th, 2005.

During this Last Call period, comments supporting publication are
encouraged as are comments pointing out problems or suggesting changes
to the spec.  

Reports of successful implementation of this draft are encouraged.  As a
reminder, tests of interoperability are *not* a prerequisite for
publication as Proposed Standard but any obstacles to implementation or
spec ambiguities should be corrected before publication.

Please send comments to the WG list as a whole.  Reports of minor
problems (typos) will not delay advancement of this document.

                                                - Bill

---

From: der Mouse <[email-masked]>
Date: Wed, 10 Aug 2005 18:07:58 -0400 (EDT)

> draft-ietf-secsh-publickey-subsystem-02, for publication as a
> Proposed Standard.

> Please send comments to the WG list as a whole.

Here are mine.  I'll leave it up to the WG and/or the document
author/editor which of these are worth doing; I'm just listing
everything I notice - put it this way: if I had sole responsibility for
the document, these are changes I'd make.

For changes where I think something should be changed but I give no
specific wording, I'll be happy to have a stab at writing up wording if
anyone wants.

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.

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/

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.

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/

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'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?

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.

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.

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).

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.

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.)

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.

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.)

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.)

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.

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.  There also
is no provision for cases such as imposing certain hosts' presence or
absence in (say) the "agent" 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).
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.

/~\ The ASCII				der Mouse
\ / Ribbon Campaign
 X  Against HTML	       [email-masked]
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

---

From: Jon Bright <[email-masked]>
Date: Tue, 23 Aug 2005 17:38:48 +0200

Hi,

>    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.

>    Client implementations SHOULD reject this request; it is normally

Changed.

>    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).

> 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.

> 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.)

> lot of duplicated, or nearly duplicated, language?

I've collapsed the two sections together.

> 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 as it currently stands.

I've moved it to section 2.

> 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.

>    "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".

> error condition, and GENERAL_FAILURE is notably unhelpful.)

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

> Based on the context, I believe s/more than /&one / is needed here.

Done.

> 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.

> 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.

> request but choose to deny it for the client in question.  I'd prefer

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.

> 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

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.

> 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).

> 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

---

From: der Mouse <[email-masked]>
Date: Sun, 28 Aug 2005 16:32:56 -0400 (EDT)

>> [various minor criticisms of publickey-subsystem-02]
> [replies]

Looks good; see my comments on publickey-subsystem-03 for more.

>> 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?

I actually meant the agent list, but it was just an example; similar
remarks apply to the from list.

> For presence, I again don't see the case where an admin wants this.

Neither do I, but I'm not confident there are none.

I suppose, though, that anyone who has such desires will probably be
ready to ignore anything in the spec that seems to forbid that.

>> 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).

By "private name" I meant a domain-localized name.  The longest name I
have in use is [email-masked], which is
44 characters long (43 was probably a mistake on my part).

64 is probably safe for the foreseeable future, though I am put in mind
of etdomenenavnkanmaksimaltinneholdesekstitrebokstaversliksomdette.com
(which is already 67 characters long).  While its holder(s) probably
aren't working on ssh, and obviously chose the name for its length
(given the meaning), it's still already over the limit.

/~\ The ASCII				der Mouse
\ / Ribbon Campaign