IETF-SSH archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Core IDs - last review



Chris Lonvick writes:
> I've placed a set of diffs from what the RFC Editor had previously 
> proposed to what is currently being proposed here:
>   http://www.employees.org/~lonvick/secsh-wg/2005-nov-30/4250diff.html
>   http://www.employees.org/~lonvick/secsh-wg/2005-nov-30/4251diff.html
>   http://www.employees.org/~lonvick/secsh-wg/2005-nov-30/4252diff.html
>   http://www.employees.org/~lonvick/secsh-wg/2005-nov-30/4253diff.html
>   http://www.employees.org/~lonvick/secsh-wg/2005-nov-30/4254diff.html

Following is a straight comparison of the change requests you posted to
the WG mailing list and the edits actually made by RFC-Editor, based on
the diffs at employees.org. (This assumes that what was posted here is
what was sent to RFC-Editor, modulo the corrections also posted here.)
I've also highlighted a few nits with the original change requests.

Executive summary: everything I've picked up is a tedious little nit
which won't affect understanding (with the possible exception of RFC4252
comment 11). Not worth the documents languishing for another month,
anyway.

RFC4250 (assignednumbers): pathetically tiny nits:
> 4)
> Section 4.5.1, first paragraph
> - replace the paragraph
> OLD:
>     Protocol packets containing the SSH_MSG_CHANNEL_REQUEST message with
>     a "pty-req" string MUST contain "encoded terminal modes" with an
>     opcode of 1 byte.  The opcode values are in the range of 1 to 255.
>     Opcodes 1 to 159 have a single uint32 argument.  Opcodes 160 to 255
>     are not yet defined.
> NEW:
>     Protocol packets containing the SSH_MSG_CHANNEL_REQUEST message with
>     a "pty-req" string MUST contain a 'encoded terminal modes' value.
>     The opcode values consist of a single byte and are in the range of 1
>     to 255.  Opcodes 1 to 159 have a uint32 argument.  Opcodes 160 to
>     255 are not yet defined.

Nit: was edited as requested, but >>a 'encoded terminal modes' value<<
should be >>an 'encoded terminal modes' value<< in the new text
(2nd line).

> 8)
> Section 4.6.1, first paragraph
> - add a clause
> OLD:
>        at-sign ("@"), a comma (","), whitespace, or control characters
>                                                  ^^^
>        (ASCII codes 32 or less).  Names are case-sensitive, and MUST NOT
> NEW:
>        at-sign ("@"), a comma (","), whitespace, control characters
>        (ASCII codes 32 or less), or the ASCII code 127 (DEL).  Names are
>                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>        case-sensitive, and MUST NOT ...

Nit: old text did not say >>a comma (",")<<, it said just
>>comma (",")<< (1st line). RFC-Editor has inserted "a". This should be
reverted.

RFC4251 (architecture): pathetically tiny nits:
> 3)
> Section 4.1, last paragraph
> -  restore the previous wording
> OLD:
>     Thus, providing the option to not check the server host key is
>                                ^^^^^^
> NEW:
>     Thus, providing the option not to check the server host key is
>                                ^^^^^^

The second-to-last paragraph, which contained a similar phrase, was
edited. The last paragraph was not.

> 4)
> Section 4.3, third bullet in the first paragraph
> - modify a line
> OLD:
>     depend on the location from which the user is trying to log in.
> NEW:
>     depend on the location from where the client is trying to gain
>     access.

Only 'which' has been changed to 'where'. (To be honest, I'm not too
bothered about the missing changes.)

> 6)
> Section 4.5, next to last paragraph
> - reword the last line
> OLD:
>     names.  Straight, bit-wise, binary comparison is RECOMMENDED.
>                     ^         ^
> NEW:
>     names.  Straight bit-wise binary comparison is RECOMMENDED.
> 
> 7)
> Section 5.4, third paragraph
> - remove a comma
> OLD:
>    names.  Straight, bit-wise, binary comparison is RECOMMENDED.
>                    ^
> NEW:
>    names.  Straight bit-wise, binary comparison is RECOMMENDED.

There is no section 5.4, and this phrase appears only once in the
document. RFC-Editor has implemented the second edit to section 4.5.

> 8)
> Section 6, first bullet in second paragraph
> - add a clause
> OLD:
>        at-sign ("@"), a comma (","), whitespace, or control characters
>                                                  ^^^
>        (ASCII codes 32 or less).  Names are case-sensitive, and MUST NOT
> NEW:
>        at-sign ("@"), a comma (","), whitespace, control characters
>        (ASCII codes 32 or less), or the ASCII code 127 (DEL).  Names are
>                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>        case-sensitive, and MUST NOT

Same comment as assignednumbers re >>a comma<<, except that it does
appear thus in the original version of this document.

RFC4252 (userauth): mostly nits, one borderline-semantic comment:
> 2)
> Section 5, packet description after the first paragrah
> - the packet syntax is not consistent with the format used in RFC 4254.
> OLD:
>        byte      SSH_MSG_USERAUTH_REQUEST
>        string    user name in ISO-10646 UTF-8 encoding [RFC3629]
>        string    service name in US-ASCII
>        string    method name in US-ASCII
> 
>        The rest of the packet is method-specific.
> NEW:
>        byte      SSH_MSG_USERAUTH_REQUEST
>        string    user name in ISO-10646 UTF-8 encoding [RFC3629]
>        string    service name in US-ASCII
>        string    method name in US-ASCII
>        ....      method specific fields follow

RFC-Editor has put just >>method specific fields<<. (*shrug*.)

> 4)
> Section 5.2, second paragraph
> - replace the paragraph
> OLD:
>     If no authentication is needed for the user, the server MUST return
>     SSH_MSG_USERAUTH_SUCCESS.  Otherwise, the server MUST return
>     SSH_MSG_USERAUTH_FAILURE and MAY return it with a list of
>     authentication 'method name' values.
> NEW:
>     If no authentication is needed for the user, the server MUST return
>     SSH_MSG_USERAUTH_SUCCESS.  Otherwise, the server MUST return
>     SSH_MSG_USERAUTH_FAILURE and MAY return it a list of methods that
>     may continue in its 'authentications that can continue' value.

RFC-Editor's edit matches this, but on the third line, I'd
s/return it a list/return with it a list/.

> 8)
> Section 7, Second paragraph
> - replace the paragraph
> OLD:
>     With this method, the possession of a private key serves as
>     authentication.  This method works by sending a 'signature' created
>     with a private key of the user.  The server MUST check that the key
>     is a valid authenticator for the user, and MUST check that the
>     'signature' is valid.  If both hold, the authentication request MUST
>     be accepted; otherwise, it MUST be rejected.  (Note that the server
>     MAY require additional authentications after successful
>     authentication.)
> NEW:
>     With this method, the possession of a private key serves as
>     authentication.  This method works by sending a signature created
>     with a private key of the user.  The server MUST check that the key
>     is a valid authenticator for the user, and MUST check that the
>     signature is valid.  If both hold, the authentication request MUST
>     be accepted; otherwise, it MUST be rejected.  (Note that the server
>     MAY require additional authentications after successful
>     authentication.)

Parentheses on the final sentence seem to have been removed.
(*shrug*.)

Extra edits have appeared in sections 7, 8, and 9 in packet
descriptions: s/service/service name/ in a couple of places.
However, there is still one place in section 8 (packet description after
1st paragraph) where it just says 'service'.

> 11)
> Section 8, second paragraph
> -  change the wording
> OLD:
>     added to the database, or compare them (with or without hashing) to
>                            ^^^^^^^^^^^^
> NEW:
>     added to the database, or compared (with or without hashing) to

RFC-Editor hasn't removed the "them". The resulting sentence doesn't
make sense. IIRC the consensus from previous discussion was that the
original edit approached a semantic change, so it would be good to have
this clear.

> 13)
> Section 9, in the fourth paragraph
> - add a reference
> OLD:
>     key' are defined in the transport layer specification.  The 'public
> NEW:
>     key' are defined in the transport layer specification [RFC4253].  The 'public

Actually edited to [SSH-TRANS] (which I prefer, and is consistent with
references elsewhere).

RFC4253 (transport): missing edits, but nits really:
> 5)
> Section 6.1, first paragraph
> - revise a line
> OLD:
>     larger packets MAY be sent if the version string indicates that the
>                                       ^^^^^^^
> NEW:
>     larger packets MAY be sent if the identification string indicates that 
> the
>                                       ^^^^^^^^^^^^^^

RFC-Editor's version has lost the final "the" (possibly due to confusing
wrapping as above). Text now reads "[...] indicates that other party is
able [...]".

An extra, but welcome, edit has appeared in 6.3 third paragraph.

> 13)
> Section 7.1, seventh paragraph
> - replace the paragraph
> OLD:
>     Note, however, that during a key re-exchange, after sending a KEXINIT
>     message, each party MUST be prepared to process an arbitrary number
>     of messages that may be in-flight before receiving a KEXINIT from the
>     other party.
> NEW:
>     Note, however, that during a key re-exchange, after sending a
>     SSH_MSG_KEXINIT message, each party MUST be prepared to process an
>     arbitrary number of messages that may be in-flight before receiving a
>     SSH_MSG_KEXINIT messages from the other party.

RFC-Editor's text matches this, but on the last line, "messages" should
be just "message".

> 15)
> Section 8, second paragraph with bullets
> - replace the paragraph with bullets
> - do not break the bulleted sections across page boundaries
> OLD:
>     The following steps are used to exchange a key.  In this, C is the
>     client; S is the server; p is a large safe prime; g is a generator
>     for a subgroup of GF(p); q is the order of the subgroup; V_S is S's
>     version string; V_C is C's version string; K_S is S's public host
>     key; I_C is C's KEXINIT message and I_S is S's KEXINIT message that
>     have been exchanged before this part begins.
> 
>     1. C generates a random number x (1 < x < q) and computes e = g^x mod
>        p.  C sends "e" to S.
> 
>     2. S generates a random number y (0 < y < q) and computes f = g^y mod
>        p.  S receives "e".  It computes K = e^y mod p, H = hash(V_C ||
>        V_S || I_C || I_S || K_S || e || f || K) (these elements are
>        encoded according to their types; see below), and signature s on H
>        with its private host key.  S sends "K_S || f || s" to C. The
>        signing operation may involve a second hashing operation.
> 
>     3. C verifies that K_S really is the host key for S (e.g., using
>        certificates or a local database).  C is also allowed to accept
>        the key without verification; however, doing so will render the
>        protocol insecure against active attacks (but may be desirable for
>        practical reasons in the short term in many environments).  C then
>        computes K = f^x mod p, H = hash(V_C || V_S || I_C || I_S || K_S
>        || e || f || K), and verifies the signature s on H.
> NEW:
>     The following steps are used to exchange a key.  In this, C is the
>     client; S is the server; p is a large safe prime; g is a generator
>     for a subgroup of GF(p); q is the order of the subgroup; V_S is S's
>     identification string; V_C is C's identification string; K_S is S's
>     public host key; I_C is C's SSH_MSG_KEXINIT message and I_S is S's
>     SSH_MSG_KEXINIT message that have been exchanged before this part
>     begins.
> 
>     1. C generates a random number x (1 < x < q) and computes
>        'e' = g^x mod p.  C sends 'e' to S.
> 
>     2. S generates a random number y (0 < y < q) and computes
>        'f' = g^y mod p.  S receives 'e'.  It computes K = e^y mod p,
>        H = hash(V_C || V_S || I_C || I_S || K_S || e || f || K)
>        (these elements are encoded according to their types; see below),
>        and s ('signature of H') with its private host key.  S sends
>        "K_S || f || s" to C. The signing operation may involve a
>        second hashing operation.
> 
>     3. C verifies that K_S really is the host key for S (e.g., using
>        certificates or a local database).  C is also allowed to accept
>        the key without verification; however, doing so will render the
>        protocol insecure against active attacks (but may be desirable for
>        practical reasons in the short term in many environments).  C then
>        computes K = f^x mod p, H = hash(V_C || V_S || I_C || I_S || K_S
>        || e || f || K), and verifies the 'signature of H' (s).

The text change requested here has not been performed.
(The requested changes boil down to:
  s/version/identification/;
  s/KEXINIT/SSH_MSG_KEXINIT/;
  s/signature s on H/s ('signature of H')/ and similar;
  quote character changes and quote mark additions)

(It's not clear to me what you meant by your request "replace the
paragraph with bullets". Nothing seems to have happened, anyway.)

RFC4254 (connection):
> 7)
> Section 5.2, fourth paragraph
> - replace the paragraph
> OLD:
>     The maximum amount of data allowed is the current window size.  The
>     window size is decremented by the amount of data sent.  Both parties
>     MAY ignore all extra data sent after the allowed window is empty.
> NEW:
>     The maximum amount of data allowed is determined by the maximum
>     packet size for the channel, and the current window size, whichever
>     is smaller.  The window size is decremented by the amount of data
>     sent.  Both parties MAY ignore all extra data sent after the allowed
>     window is empty.
> 
>     Implementations are expected to have some limit on the ssh
>     transport layer packet size (any limit for rececived packets MUST
>     be 32768 bytes or larger, as described in [SSH-TRANS]). The
>     implementation of the SSH connection layer
> 
>     o   MUST NOT advertise a maximum packet size that would result in
>         transport packets larger than its transport layer is willing to
>         receive.
> 
>     o   MUST NOT generate data packets larger that its transport layer is
>         willing to send, even if the remote end would be willing to
>         accept very large packets.

RFC-Editor's changes match the request (modulo the typo 'rececived'
above). A couple of further nits:
  s/ssh transport layer/SSH transport layer/ in the second paragraph;
  s/larger that/larger than/ in the second bullet.

An extra edit was made in section 6.10 first paragraph: remove "after
this message".

> 14)
> Section 6.10, fourth paragraph
> - do not break a hyphen segmented name
> OLD:
>     Additional 'signal name' values MAY be sent in the format "sig-
>     name@xyz", where "sig-name" and "xyz" may be anything a particular
> NEW:
>     Additional 'signal name' values MAY be sent in the format
>     "sig-name@xyz", where "sig-name" and "xyz" may be anything a particular

This edit has not been made.



Home | Main Index | Thread Index | Old Index