Hi Christer, Christer Holmberg via Datatracker <noreply@xxxxxxxx> writes: > Reviewer: Christer Holmberg > Review result: Ready with Nits > > 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. Okay. > For more information, please see the FAQ at > > https://trac.ietf.org/trac/gen/wiki/GenArtfaq > > Document: draft-ietf-curdle-ssh-curves-09 > Reviewer: Christer Holmberg > Review Date: 2019-08-22 > IETF LC End Date: 2019-08-26 > IESG Telechat date: Not scheduled for a telechat > > Summary: > > The document is almost ready for publication. I have no technical issues, but > there are some issues in Section 1 that I'd like to authors to address. > > Major issues: N/A > > Minor issues: N/A > > Nits/editorial comments: > > General: > --------- > > QGEN_1: > > - The document uses “as discussed in”, “as defined in”, “as described” > in terminology. It might be justified to use different terminology in > some cases, but in general I suggest trying to use consistent > terminology. I understand. Only occurance of "as discussed in" is found in this text: "Alternative implementations of these functions SHOULD abort when either input forces the shared secret to one of a small set of values, as discussed in Section 7 of [RFC7748]." Section 7 of RFC7748 is the Security Considerations section and is a discussion of the elements which are potentially a security issue during implementation. This draft provides a SHOULD recommendation to abort the connection when one of these raised security considerations is found. That said, I am okay with changing the text to "Alternative implementations of these functions SHOULD abort when either input forces the shared secret to one of a small set of values, as described in Section 7 of [RFC7748]." The only other time the word discussed is used is in No further validation is required beyond what is discussed in <xref target="RFC7748"/>. which may be replaced with No further validation is required beyond what is described in <xref target="RFC7748"/>. I was unable to find any "as defined in" statements. I found declarative statements "is defined in" "are defined in" "is defined as" which were references to normative protocol elements. The use of "as described in" is found in the standard boilerplate copyright notice and the boilerplate Requirements language sections. So, I have migrated from "discussed" to described" for consistentcy. > Section 1: > ---------- > > Q1_1: > > The text says: > > ”[RFC5656] describes how elliptic curves are > integrated in SSH, and this document reuses those protocol messages.” > > …and: > > “This document describes how to implement key exchange based on > Curve25519 and Curve448 [RFC7748] in SSH.” > > - It is unclear to me what “integrated in SSH” means. The title of RFC 5656 is "Elliptic Curve Algorithm Integration in the Secure Shell Transport Layer" and this Draft intents to augment the Elliptic Curve methods defined there. > Does it mean that RFC 5656 describes the generic procedures for > performing SSH key exchanges using elliptic curves, or does it also > cover other things? RFC5656 covers three specific constructions: a) Elliptic Curve Diffie-Hellman (ECDH), b) Elliptic Curve Menezes-Qu-Vanstone (ECMQV) key agreement, and c) Elliptic Curve Digital Signature Algorithm (ECDSA). This draft does not cover the use of a digital signature algoirthm or apply the Curve25519 or Curve448 constructions to the use of ECMQV and focuses entirely on ECDH key exchange extensions for a different construction of elliptic curves. > - I think the “this document reuses those protocol messages” sounds a > little confusing because I don't know what “those protocol message” > refers to. Perhaps say something like “reuses the protocol messages > defined in that specification”. This draft resuses the SSH_MSG_KEX_ECDH_INIT and SSH_MSG_KEX_ECDH_REPLY messages which apply to the ECDH construction as in in section 4 of this draft. Does this paragraph work as a better replacement for the first paragraph of the introduction? Or, is it too detailed for a summary introduction? Secure Shell (SSH) <xref target="RFC4251"/> is a secure remote login protocol. The key exchange protocol described in <xref target="RFC4253"/> supports an extensible set of methods. <xref target="RFC5656"/> defines how elliptic curves are integrated into this extensible SSH framework, and this document reuses the Elliptic Curve Diffie-Hellman (ECDH) key exchange protocol messages defined in section 7.1 "ECDH Message Numbers" <xref target="RFC5656"/>. It seems a bit detailed to me for an introduction. Let me know if you have any suggestions on a revision. > Q1_2: > > - I don't think you should use “we” terminology (“we describe”, “we chose”, > etc..). Please talk about the document, and if you want to refer to a choice > made by the WG please indicate that. Fixed. > Q1_3: > > - Instead of “currently”, I suggest to say something like “at the time of > publication”. Because, the meaning of “currently” changes every second :) Fixed. > Q1_4: > > The text says: > > “The Curve448 key exchange method is novel but similar in spirit,” > > - I don't know what this means, since there is now further explanation. The paragraph now reads: This document describes how to implement key exchange based on Curve25519 and Curve448 <xref target="RFC7748"/> in SSH. For Curve25519 with SHA-256 <xref target="RFC6234"/>, the algorithm described is equivalent to the privately defined algorithm "curve25519-sha256@xxxxxxxxxx", which at the time of publication was implemented and widely deployed in libssh and OpenSSH. The Curve448 key exchange method is similar but uses SHA-512 <xref target="RFC6234"/> to further separate it from the Curve25519 alternative. Would you rather that I use 'This document defines ...' here and in the abstract rather than 'This document describes ...' ? Please advise. > Q1_5: > > The text says: > > “This document provide Curve25519 as the preferred choice, but > suggests that the fall back option Curve448 is implemented to provide > an hedge against unforeseen analytical advances against Curve25519 > and SHA-256.” > > - Is the only reason why one should implement Curve448 that something > MAY happen to Curve25519 in the future? No, the Curve448 also has a stronger cryptographic security strength. If it becomes a requirement to use a minimum of 128 bits of security strength, then Curve25519 may be rejected by some and thus the need to provide for something stronger. > - Also, is there anything preventing unforeseen analytical advances > against Curve448? There is nothing preventing any unforeseen analytical advances against any of the current public key implementations. This is why Post-Quantum-Cryptographic (PQC) is such a host topic today. However, the same tools which may be able to exploit EcDSA may not be as quickly pointed at these curves. Cryptographic agility is the key here. Let me know if you which to have me remove the entire paragraph or not. This document provide Curve25519 as the preferred choice, but suggests that the fall back option Curve448 is implemented to provide an hedge against unforeseen analytical advances against Curve25519 and SHA-256. Due to different implementation status of these two curves (high-quality free implementations of Curve25519 has been in deployed use for several years, while Curve448 implementations are slowly appearing), it is accepted that adoption of Curve448 will be slower. Should I upload my updated draft-ietf-curdle-ssh-curves-10.xml or do you have additional suggestions? Thank you, -- Mark
