Subject: Genart last call review of draft-ietf-curdle-ssh-curves-09

From: Christer Holmberg via Datatracker <[email-masked]>
Date: Thu, 22 Aug 2019 05:00:38 -0700

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.

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.

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. 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? -       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”.

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.

Q1_3:

- Instead of “currently”, I suggest to say something like “at the time of
publication”. Because, the meaning of “currently” changes every second :)

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.

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?

- Also, is there anything preventing unforeseen analytical advances against
Curve448?

---

From: "Mark D. Baushke" <[email-masked]>
Date: Mon, 26 Aug 2019 09:28:42 -0700

Hi Christer,

> like any other last call comments.

Okay.

> https://trac.ietf.org/trac/gen/wiki/GenArtfaq
>
> - 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.

> -       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”,

Fixed.

> publication”. Because, the meaning of “currently” changes every second :)

Fixed.

> - 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
        "[email-masked]", 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.

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

---

From: Christer Holmberg <[email-masked]>
Date: Mon, 26 Aug 2019 17:37:16 +0000

Hi Mark,

Please see inline. I have removed the comments where your suggested solution is fine and I have nothing further to say.

Nits/editorial comments:

General:
---------

QGEN_1:

>> some cases, but in general I suggest trying to use consistent terminology.

…

>So, I have migrated from "discussed" to described" for consistentcy.

That's good.

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.”
>>
> The title of RFC 5656 is "Elliptic Curve Algorithm Integration in the Secure Shell Transport Layer" and this Draft intents to augment the
>> 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?
>
>   a) Elliptic Curve Diffie-Hellman (ECDH),>
> 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.

Would it be good to indicate that in a note?

> messages which apply to the ECDH construction as in in section 4 of this draft.
>

I think the text looks good, and it is for sure a good clarification for a non-security person like myself :)

---

> target="RFC6234"/> to further separate it from the Curve25519

Looks good.

> abstract rather than 'This document describes ...' ? Please advise.

Personally I would use "describe", but my main issue is being consistent - no matter what word is used :)

---

> provide for something stronger.

Wouldn't it be enough to say that, instead of talking about unforeseen analytical advances etc? I noted that the sec-dir reviewer had some comments on that too.

Please see below for suggested modified text.

> Let me know if you which to have me remove the entire paragraph or not.

I think you could keep the paragraph, but instead say something like:

          “This document provide Curve25519 as the preferred choice, but
           suggests that the Curve448 is implemented in order to provide
           128 bits of security strength, should that become a requirement.

           At the time of writing this specification high-quality free implementations
           of Curve25519 had been in deployed use for several years, while Curve448
           implementations were slowly appearing, so it was accepted that adoption 
           of Curve448 would be slower."  

> do you have additional suggestions?

I haven't seen the update draft yet, but if you are ok with my suggestions you can go ahead to upload. If you are not ok with my suggestions, then let me know :)

Regards,

Christer

---

From: "Mark D. Baushke" <[email-masked]>
Date: Mon, 26 Aug 2019 11:23:16 -0700

Christer Holmberg <[email-masked]> writes:

> Hi Mark,
> 
> Please see inline. I have removed the comments where your suggested
> solution is fine and I have nothing further to say.

I have done likewise.

> Section 1:
...elided...
> > 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.
> 

Possibly. I could add this as the final sentence to the first paragraph
of the introduction:

	Other parts of <xref target="RFC5656"/>, such as Elliptic Curve
        Menezes-Qu-Vanstone (ECMQV) key agreement, and Elliptic Curve
        Digital Signature Algorithm (ECDSA) are not considered in this
        document.

...elided...

> > It seems a bit detailed to me for an introduction. Let me know if
> > you have any suggestions on a revision.
> 

Okay, I will keep at as revised with the only other question being the
addition of the sentence noted previously.

...elided...

> Personally I would use "describe", but my main issue is being
> consistent - no matter what word is used :)

I will retain the 'This document defines...' text.

I believe that there is a difference between defines and describes. The
former is normative text and the latter is more informative.

> Wouldn't it be enough to say that, instead of talking about unforeseen
> analytical advances etc? I noted that the sec-dir reviewer had some
> comments on that too.
> 
>            At the time of writing this specification high-quality free
>            implementations of Curve25519 had been in deployed use for
>            several years, while Curve448 implementations were slowly
>            appearing, so it was accepted that adoption of Curve448
>            would be slower."
> 

I have adopted your two paragraphs to replace the one paragraph that was
previously present.

> I haven't seen the update draft yet, but if you are ok with my
> suggestions you can go ahead to upload. If you are not ok with my
> suggestions, then let me know :)

Only one questions remains as the last sentence of the first paragarph
of the introduction.

	-- Mark

---

From: Christer Holmberg <[email-masked]>
Date: Mon, 26 Aug 2019 18:34:06 +0000

Hi Mark,

Section 1:
-----------

...elided...

Looks good.

...elided...

> addition of the sentence noted previously.

Ok.

...elided...

> former is normative text and the latter is more informative.

Sure, and if you think there are situations where you want to use both, that's fine. But, often people just use the word that comes to their mind when writing (or adding text written by someone else), and that causes the inconsistence in terminology.

---

> Only one questions remains as the last sentence of the first paragarph

The text you suggested looks good.

So, as far as I'm concerned, go ahead and make the change and submit the -10 version 🙂

Regards,

Christer

---

From: "Mark D. Baushke" <[email-masked]>
Date: Mon, 26 Aug 2019 12:08:19 -0700

I have uploaded draft-ietf-curdle-ssh-curves-10.xml

  https://datatracker.ietf.org/doc/draft-ietf-curdle-ssh-curves/

with all of your suggested changes.

This version addresses all of the review comments I have received to
date.

	-- Mark

---

From: Christer Holmberg <[email-masked]>
Date: Wed, 28 Aug 2019 05:18:43 +0000

Hi,

My gen-art issues have been properly addressed.

Thanks!

Regards,

Christer

-----Alkuperäinen viesti-----
Lähettäjä: Mark D. Baushke <[email-masked]> 
Lähetetty: maanantai 26. elokuuta 2019 22.08
Vastaanottaja: Christer Holmberg <[email-masked]>
Kopio: [email-masked]; [email-masked]; [email-masked]; [email-masked]
Aihe: Re: Genart last call review of draft-ietf-curdle-ssh-curves-09

This version addresses all of the review comments I have received to date.

	-- Mark