Re: Last call comments for draft-ietf-secsh-publickey-subsystem-07

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

Hi,

Jacob Nevins wrote:
> (Apologies for the lateness of these comments.)

Apologies for the delay in replying.


>    In the following, "public-key blob" has the format specified
>    in section 6.6, "Public Key Algorithms" of the SSH Transport
>    Protocol document [2]:
>
>          string    certificate or public key format identifier
>          byte[n]   key/certificate data
>
> (Although in fact I'd rather we avoided the term "blob" entirely, as
> in publickeyfile, due to the conflict with 4253.)

The suggestion seems OK.  I'm not sure what word other than "blob" we 
could use.  "public key object"?  "public key representation"?  Since 
we're in IESG evaluation now, I'm not sure what the process is for 
integrating such a change.  If I can issue another rev, I will (unless 
someone speaks up against it) include this change.

> By way of example, I believe this is a well-formed "publickey"
> response according to this interpretation (although I haven't tested
> it):

I agree that it's well formed.

> (Yes, the algorithm name does appear twice on the wire.)

Confused me when implementing 4253, too.

> Other, minor comments:
>
> Section 4 "Requests and responses":
>
> I don't understand what the last paragraph means.
> Is it meant to imply that a version of other than 2 could change the
> semantics of any and all "name" and data parts?

Assuming you meant the last paragraph of 3.2: No, it's just meant to 
clarify that we're not going to write out length every time.

> 4.1: attributes:
>
> "x11", "shell", "exec" etc: "The attribute-value field SHOULD be
> empty": why not MUST? (Nit, as this doesn't create any actual 
> ambiguity.)

It's not necessary for interoperability (at least not with peers 
implementing version 2 between one another).  It's not an absolute 
requirement that the attribute-value be empty, the presence or absence 
of the attribute is decisive.  As such, I believe SHOULD to be the 
correct 2119ish.

> "from" (etc): is it worth spelling out what a "host" is in the
> "comma-separated list of hosts"? (I presume FQDNs, and IP addresses --
> IPv4, IPv6, whatever. No wildcards, suffix matching etc.)

IESG review has turned up a similar comment.  I guess a clarification 
that the from list are FQDNs or IP addresses would work.  Again, if 
another rev happens, it'll include this.

> 6.2.1 "Conventions for names":
> Nit: To reduce confusion, it might be worth changing the example of a
> locally defined name from "ourcipher-cbc%example.com@localhost" to something
> more appropriate to the context, say "ourattr%example.com@localhost".  

Agreed.  Ditto another-rev comment from above.

> Status codes duplicated in 3.3.1 and 6.6.2 (consistently).   
> I reckon 3.3.1 should instead refer to 6.6.2.

Hm, I think I disagree.  3.3.1 is defining stuff for the purposes of the 
protocol.  Unless you happen to be IANA, I'm guessing you could pretty 
much cut off section 6...

> Nit: 6.6.3 says "message numbers" twice where it should say "status codes".

True, will change if possible.

--
Jon Bright