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

[Jacob Nevins <jacobn+secsh%chiark.greenend.org.uk@localhost>]

(Apologies for the lateness of these comments.)

My main comment:

3: "Public key blob" specification is still too vague. It became clear
when we were finishing off publickeyfile that the language that
escaped to publication in 4253 is less than perfect, and has led to
confusion in practice.

The term "blob" is not explicitly defined in the reference, but where
it is mentioned it seems to refer to something other than that which
some people expect it to mean.

Therefore, I think we should explicitly define what's required, as in
publickeyfile-13. Assuming the same interpretation is required, the
existing paragraph should be replaced with:

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

I believe that this interpretation matches at least one implementation
(VanDyke's patch for OpenSSH) -- that implementation is rather old (it
seems to correspond to the text circa
draft-galb-secsh-publickey-subsystem-01), but this part of the
protocol hasn't obviously changed.

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

00000000   00 00 00 B0 00 00 00 09 70 75 62 6C 69 63 6B 65   ........publicke
00000010   79 00 00 00 07 73 73 68 2D 64 73 73 00 00 00 90   y....ssh-dss....
00000020   00 00 00 07 73 73 68 2D 64 73 73 00 00 00 20 73   ....ssh-dss... s
00000030   F9 F2 9C 4E 7A DD DF E0 17 49 92 73 75 1B 7C 0B   ...Nz....I.su.|.
00000040   28 77 A2 CB 12 AA 03 97 8C AA B8 9A FB D6 D3 00   (w..............
00000050   00 00 15 00 86 6E 41 6C 0B 64 99 87 F5 8D A8 DD   .....nAl.d......
00000060   05 E7 E1 4D 85 A3 49 5F 00 00 00 20 2B 22 75 CF   ...M..I_... +"u.
00000070   04 3E F0 28 69 8B 82 D1 ED 65 F5 23 D3 EE 8F 9C   .>.(i....e.#....
00000080   FC 64 41 B3 C2 D2 C5 25 2F 4C 9C 45 00 00 00 20   .dA....%/L.E...
00000090   07 67 23 4D A8 C0 3F 7F F2 50 1A C0 5F CF D1 A2   .g#M..?..P.._...
000000A0   4F 65 64 07 2A A7 CF 3C 8B 39 7A 38 59 B5 A2 F3   Oed.*..<.9z8Y...
000000B0   00 00 00 00                                       ....

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

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?

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

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

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

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

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