Subject: [Gen-art] Genart last call review of draft-ietf-curdle-ssh-kex-sha2-14

From: Roni Even via Datatracker <[email-masked]>
Date: Mon, 15 Feb 2021 02:15:21 -0800

Reviewer: Roni Even
Review result: Almost Ready

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-curdle-ssh-kex-sha2-??
Reviewer: Roni Even
Review Date: 2021-02-15
IETF LC End Date: 2021-02-24
IESG Telechat date: Not scheduled for a telechat

Summary:
The document is almost ready for publication as standard track RFC

Major issues:

Minor issues:
It is not clear what is the backward interoperability is, I noticed that only
rsa1024-sha1 is deprecated.  It would be good to add some text maybe  in
section 4 that will explain it and maybe have some recommendations for client
and server side.

Nits/editorial comments:

---

From: "Mark D. Baushke" <[email-masked]>
Date: Mon, 15 Feb 2021 10:54:39 -0800

Hi Roni,

> Minor issues:
> It is not clear what is the backward interoperability is, I noticed
> that only rsa1024-sha1 is deprecated. It would be good to add some
> text maybe in section 4 that will explain it and maybe have some
> recommendations for client and server side.

The simple answer is that because a server and client always present a
list of possible key exchanges to be negotiated. The loss of one or two
should not cause any interoperability issues.

The inclusion of the original diffie-hellman-group14-sha1 key exchange
name somewhere in the list or the new "MUST"
diffie-hellman-group14-sha256 key exchange should guarantee that
interoperability will be maintained.

Background for the reviewer:

    The only Mandatory-to-Implement (MTI) Key Exchanges in any of the
    current set of standards are specified in RFC 4253 Section 6.5 are
    these:
    
       diffie-hellman-group1-sha1 REQUIRED
       diffie-hellman-group14-sha1 REQUIRED
    
    Additional key exchange methods may be added using the methods
    provided in RFC 4250.
    
    All of the other key exchange algorithms are provided in other RFCs
    are optionally added to the pool of possible key exchanges to be
    implemented.
    
    The existence of rsa1024-sha1 in RFC 4432 was not uniformly adopted
    by implementations of the secure shell and the mitigation has always
    been that the client and the server would offer the algorithms that
    they are willing to accept. In fact, there are only a handful of
    implementations supporting this key exchange.
    
    This RFC deprecates the original two MTI key exchanges by moving
    them from the "MUST" be implmeneted to "SHOULD NOT" and "MAY"
    respectively and promoting diffie-hellman-group14-sha256 to the only
    mandatory to implement algorithm.

Suggestion to the reviewer of replacement paragraphs before the table in
section 4 "Summary Guidance for Key Exchange Method Names Implementations"

    This RFC provides guidance to users and implementors as to which key
    exchanges are mandatory-to-implmement (MTI) -- MUST be implemented,
    as well as what current cryptographic key exchange methods SHOULD be
    implemented and MAY be implemented.

    The ordering of the default negotiation list is at the discretion of
    the implementor with the ability of the local adminitrator to
    configure the server in accordance with local policies and the users
    to configure the client with key exchanges that meet their needs.

    It is suggested that the MUST and SHOULD key exchange method names
    come in the preference list before the SHOULD NOT and MUST NOT
    names.

    It is suggested that SHOULD NOT and MUST NOT key exchange method
    names not be in the default negotation list provided by
    implementors, or that they be provided only at the very end of the
    negotation list.
    
    It is suggested that the MUST NOT key exchange method code be
    removed from the any implementations using them.

    The Implement column is the current recommendations of this RFC. Key
    Exchange Method Names are listed alphabetically. This is ordering is
    not intended to be the order used in either the server or client
    negotiation lists.

Please let me know if this addresses the issues raised. I wrote the
above rather quickly, so an editors touch to make it easier to read may
be desirable.

	Be safe, stay healthy,
	-- Mark

---

From: Ron Frederick <[email-masked]>
Date: Mon, 15 Feb 2021 12:34:34 -0800

Hi Mark,

A couple of minor issues with the recommended text (grammatical, not technical):

On Feb 15, 2021, at 10:54 AM, Mark D. Baushke <mdb=[email-masked]> wrote:
>     [snip]

This should be “from any” instead of “from the any”.

>    not intended to be the order used in either the server or client

This should be “This ordering is” instead of “This is ordering is”.
-- 
Ron Frederick
[email-masked]

---

From: "Mark D. Baushke" <[email-masked]>
Date: Mon, 15 Feb 2021 13:06:58 -0800

> A couple of minor issues with the recommended text (grammatical, not

As always, any feedback greatfully received.

> This should be “from any” instead of “from the any”.

Hmmm... I seem to have managed to lose what I intended to write. I will
suggest this as a replacement:

    It is suggested that the code which implements a "MUST NOT" key
    exchange method have that implementation code be removed.

> This should be “This ordering is” instead of “This is ordering is”.

Yes.

   The Implement column is the current recommendations of this RFC. Key
   Exchange Method Names are listed alphabetically. This ordering is not
   intended to be the order used in either the server or client
   negotiation lists.

Thank you very much for your review of the text I wrote.

	Be safe, stay healthy,
	-- Mark

---

From: Ron Frederick <[email-masked]>
Date: Mon, 15 Feb 2021 13:16:48 -0800

Hi Mark,

>    exchange method have that implementation code be removed.

There’s some redundancy with “code” here twice, and “have..be” still doesn’t read right. I’d suggest:

   It is suggested that the code which implements a "MUST NOT" key
   exchange method be removed from implementations that contain it.

> Thank you very much for your review of the text I wrote.

No problem!
-- 
Ron Frederick
[email-masked]

---

From: "Mark D. Baushke" <[email-masked]>
Date: Mon, 15 Feb 2021 13:22:47 -0800

>    exchange method be removed from implementations that contain it.

+1 I like it. Thank you!

	Be safe, stay healthy,
	-- Mark