Ed25519 TLS support #3585

snhenson · 2017-05-30T19:48:31Z

Checklist

tests are added or updated

This PR adds support for Ed25519 in TLS 1.3 and TLS 1.2 for OpenSSL. The code can connect to itself and can also interop with BoringSSL.

davidben · 2017-05-30T21:57:50Z

The code can connect to itself but has not been tested against any other implementations: anyone know of any?

BoringSSL has an implementation, though it's not exposed in Chrome yet. If using the command-line tool as a client, you'll need to pass the -ed25519 flag to enable it. As a server, just configure an Ed25519 cert and key. We also have a Go implementation in our test suite you can test against if you update the revision.

tmshort · 2017-06-06T18:21:43Z

"openssl speed ecdsa" gives 0.0 sign/s and verify/s for X25519, not sure if it needs more hooking into that app

mattcaswell · 2017-06-08T19:20:36Z

ssl/t1_lib.c

@@ -788,7 +792,8 @@ static const uint16_t tls_default_sigalg[] = {
    TLSEXT_SIGALG_ecdsa_sha1, /* SSL_PKEY_ECC */
    TLSEXT_SIGALG_gostr34102001_gostr3411, /* SSL_PKEY_GOST01 */
    TLSEXT_SIGALG_gostr34102012_256_gostr34112012_256, /* SSL_PKEY_GOST12_256 */
-    TLSEXT_SIGALG_gostr34102012_512_gostr34112012_512 /* SSL_PKEY_GOST12_512 */
+    TLSEXT_SIGALG_gostr34102012_512_gostr34112012_512, /* SSL_PKEY_GOST12_512 */
+    0 /* SSL_PKEY_ED25519 */


Not sure I understand why this is 0 here.

That table is the default signature algorithm associated with the certificate in the absence of a signature algorithms extension. That applies to TLS < 1.2 or TLS 1.2 if signature algorithms is not present only. It doesn't apply to TLS 1.3 because signature algorithms is mandatory.

Since Ed25519 can't be used with TLS < 1.3 it is zero.

draft-ietf-tls-rfc4492bis-17 specifies it for TLS 1.2 as well, and is what we (BoringSSL) have implemented.

Hmm... that makes it a bit more interesting. Will need to use the handshake record cache for Ed25519 and TLS 1.2. Looking into it.

It's allowed with TLS 1.0 and 1.1 too.

It's not only in the "signature_algorithms" extension but also the "Supported Elliptic Curves" extension. All previous 25 curves did not require distinct "signature_algorithms" entries after all.

I find nothing in the document that would restrict EdDSA to TLS >= 1.2. Otherwise I guess the introduction would not put ECDSA and EdDSA at the same level.

And EdDSA is explicitely mentioned as applicable before TLS 1.2 in section 5.10:

[...] Such standardization never took place, and as a result, SHA-1 is used in
TLS 1.1 and earlier (except for EdDSA, which uses identity function).

The previous ECC curves could all be used for both ECDH and ECDSA. Ed25519 can be used for signing only and X25519 for ECDH: though there are similarities they're treated as distinct algorithms. The supported curves extension has an entry for x25519 but not Ed25519.

I'm not sure why that paragraph mentions TLS 1.1 and EdDSA. While it could theoretically be used there is no signalling mechanism for a client to indicate it supports Ed25519.

Yes the curve assignments are named after the D-H functions and I agree that using them as a signal for signature support is my interpretation trying to make some sense from all of this.

I don't want to slow you down unnecessarily. I'll be happy to test EdDSA with TLS 1.2 in OpenSSL.

The curve values have nothing to do with Ed25519 support. Ed25519 follows the TLS 1.3 negotiation mechanism where signing is purely determined by signature algorithms and curves/groups only affect (EC)DH. Plenty of clients today support X25519 without Ed25519 (including OpenSSL 1.1.0).

Re TLS 1.1 and below, we don't allow Ed25519 at those versions either for the same reason. It's rather silly without a signaling mechanism, and this is one less combination to worry about when thinking about downgrade possibilities, especially since you need TLS 1.2 to get a strong transcript hash.

Anyway back to the original query. My comment still applies: the entry is 0 in tls_default_sigalg because there is no default signature algorithm for Ed25519: that is because we only use Ed25519 if the client explicitly indicates support for it.

snhenson · 2017-06-14T23:54:53Z

Update: rebased against master, fixed signature algorithm print and setting, added a test.

snhenson · 2017-06-18T11:26:55Z

Update: rebased against master, experimental TLS 1.2 support, verified against BoringSSL. Updated tests.

Note: to support the oneshot operation the data being signed/verified is first collected into a single malloced buffer. To simplify the code the oneshot API is always used though it is only needed with Ed25519.

TODO: client auth currently untested.

ocheron · 2017-06-18T18:44:41Z

Thank you all for your explanations, I get it now.
Backporting TLS 1.3 extension handling makes a lot of sense too.
The assignments are perfectly in line with that. Text in the draft, much less.

My manual tests run OK with both OpenSSL and BoringSSL. TLS 1.0 and 1.1 are rejected as expected.
And openssl s_server accepts an Ed25519 certificate sent by my client (TLS 1.2).

snhenson · 2017-06-19T13:49:35Z

Update: added client auth tests. Rebased against master. Taking out of "WIP".

mattcaswell · 2017-06-20T13:59:51Z

crypto/x509/x509type.c

@@ -41,6 +41,9 @@ int X509_certificate_type(const X509 *x, const EVP_PKEY *pkey)
    case EVP_PKEY_EC:
        ret = EVP_PK_EC | EVP_PKT_SIGN | EVP_PKT_EXCH;
        break;
+    case NID_ED25519:
+        ret = EVP_PKT_SIGN;


Why no specific certificate type for Ed25519, e.g. EVP_PK_ED25519

We don't have one for X25519 either. Should I add a type for both?

For completeness, I guess we should, although I can't imagine there are ever going to be too many X25519 certificates???

OK added in separate commit. We might see X25519 certificates for CMS but I think they'll be pretty rare.

mattcaswell · 2017-06-20T14:08:49Z

ssl/statem/statem_clnt.c

-            && (exp_idx != SSL_PKEY_GOST_EC ||
-                (i != SSL_PKEY_GOST12_512 && i != SSL_PKEY_GOST12_256
-                 && i != SSL_PKEY_GOST01))) {
+                && (exp_idx != SSL_PKEY_ECC && i != SSL_PKEY_ED25519)


This doesn't look right to me. Should this be:

(exp_idx != SSL_PKEY_ECC || i != SSL_PKEY_ED25519)

Yes it should. Fix coming up

Approved by Oracle. Reviewed-by: Bernd Edlinger <[email protected]> (Merged from #3585)

richsalz · 2017-06-20T15:21:13Z

Just for the record, the Oracle/Sun copyright change points here, and it should be #3685

mattcaswell · 2017-06-21T08:54:31Z

crypto/x509/x509type.c

@@ -41,6 +41,9 @@ int X509_certificate_type(const X509 *x, const EVP_PKEY *pkey)
    case EVP_PKEY_EC:
        ret = EVP_PK_EC | EVP_PKT_SIGN | EVP_PKT_EXCH;
        break;
+    case EVP_PKEY_ED25519:
+        ret = EVP_PKT_SIGN;


This still looks odd to me. So we've changed the NID_X25519 -> EVP_PKEY_ED25519, but should the return flags see a similar change? e.g. ret = EVP_PK_ED25519 | EVP_PKT_SIGN

X509_certificate_type() is an ancient function. The only reason Ed25519 has an entry there is because some code in tls_process_cert_verify() needs it to function. The only other recent addition (GOST) only has the capabilities bits set and no algorithm bits. Also I just checked EVP_PK_EC is not referenced anywhere in OpenSSL.

TBH while looking through this it was clear that adding a new certificate type was far more complex than it should be. There are functions all over the place that need changing and obscre additions to if/switch statements which it is too easy to get wrong. The only way to find them all is to test it and see what breaks and then fix it hoping you catch everything.

I think the whole thing can be handled with a single table of certificate capabilities. I'd planned a follow up PR to do exactly that. I could instead do that first and convert this PR to use the new scheme.

No, a follow on PR will be fine.

TBH while looking through this it was clear that adding a new certificate type was far more complex than it should be. There are functions all over the place that need changing and obscre additions to if/switch statements which it is too easy to get wrong. The only way to find them all is to test it and see what breaks and then fix it hoping you catch everything.

+1

snhenson · 2017-06-21T13:13:40Z

Pushed, thanks everyone.

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

This imports the following changes from OpenSSL: * openssl/openssl@aa8f3d7 * openssl/openssl@5aba2b6 * openssl/openssl@c80149d See the following links for some related discussion in OpenSSL's repository: * openssl/openssl#3663 * openssl/openssl#3684 * openssl/openssl#3585 (comment) * openssl/openssl#3685 The copyright_summary script may be used to compare this CL. Note there is one change to ecdsa_test.cc to align with OpenSSL. See go/openssl-copyright-consolidation-comparison (internal-only). Bug: 364634028 Change-Id: I987c4e145d2ccd0c32bbf9e7bb2cc69e89019d35 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/74712 Reviewed-by: Adam Langley <[email protected]> Commit-Queue: David Benjamin <[email protected]>

snhenson force-pushed the ed25519-tls branch from 2d4f2ae to 6957275 Compare May 30, 2017 21:48

mattcaswell requested changes Jun 8, 2017

View reviewed changes

snhenson force-pushed the ed25519-tls branch from 6957275 to 63f41e4 Compare June 14, 2017 23:52

snhenson force-pushed the ed25519-tls branch from 63f41e4 to 34e4941 Compare June 17, 2017 23:41

snhenson force-pushed the ed25519-tls branch from 34e4941 to ee75ceb Compare June 19, 2017 13:47

snhenson changed the title ~~WIP: Ed25519 TLS support~~ Ed25519 TLS support Jun 19, 2017

snhenson added 9 commits June 20, 2017 14:39

Recognise Ed25519 in X509_certificate_type

27a7ea8

Use X509_get_signature_info to get signature strength.

eab8bdd

Add index for ED25519

fbe2bdf

Add Ed25519 to signature algorithm table

dd1afa8

Add Ed25519 signature algorithm

e30991f

Handle signature algorithms with no associated digest

0f65056

Add Ed25519 to trace output

602d3b1

Print Ed25519 in s_client/s_server

d790b21

Convert key exchange to one shot call

6e0b130

mattcaswell requested changes Jun 20, 2017

View reviewed changes

levitte pushed a commit that referenced this pull request Jun 20, 2017

Modify Sun copyright to follow OpenSSL style

aa8f3d7

Approved by Oracle. Reviewed-by: Bernd Edlinger <[email protected]> (Merged from #3585)

snhenson added 5 commits June 20, 2017 16:17

Allow Ed25519 in TLS 1.2

15aa15b

Ed25519 support for mkcert.sh

e9d042e

Add Ed25519 EE certificates

0d2854b

Add Ed25519 TLS 1.3 and 1.2 tests

1ff0b1e

Update test config file

9bc71a8

Use EVP_PKEY_X25519, EVP_PKEY_ED25519 instead of NIDs where appropriate.

4969147

snhenson force-pushed the ed25519-tls branch from ee75ceb to 4969147 Compare June 20, 2017 15:39

mattcaswell reviewed Jun 21, 2017

View reviewed changes

mattcaswell approved these changes Jun 21, 2017

View reviewed changes

mattcaswell added the approval: done This pull request has the required number of approvals label Jun 21, 2017

snhenson closed this Jun 21, 2017

levitte pushed a commit that referenced this pull request Jun 21, 2017

Recognise Ed25519 in X509_certificate_type

d3c094c

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Use X509_get_signature_info to get signature strength.

65e8973

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Add index for ED25519

07afa3d

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Add Ed25519 to signature algorithm table

b04d4e3

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Add Ed25519 signature algorithm

3d234c9

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Handle signature algorithms with no associated digest

168067b

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Add Ed25519 to trace output

881d2c5

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Print Ed25519 in s_client/s_server

03327c8

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Convert key exchange to one shot call

72ceb6a

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Allow Ed25519 in TLS 1.2

b202155

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Ed25519 support for mkcert.sh

bc88fc7

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Add Ed25519 EE certificates

60bbed3

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Add Ed25519 TLS 1.3 and 1.2 tests

fbe1af9

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Update test config file

29b0cab

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

levitte pushed a commit that referenced this pull request Jun 21, 2017

Use EVP_PKEY_X25519, EVP_PKEY_ED25519 instead of NIDs where appropriate.

d2916a5

Reviewed-by: Matt Caswell <[email protected]> (Merged from #3585)

This was referenced Jun 21, 2017

ED25519 Support pyca/cryptography#3509

Closed

Support for EdDSA (Ed25519 & Ed448) #487

Closed

anarcat mentioned this pull request Nov 7, 2017

ed25519 support SecurityInnovation/PGPy#221

Closed

kaduk mentioned this pull request Jan 9, 2018

Support for Ed448 #5049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ed25519 TLS support #3585

Ed25519 TLS support #3585

snhenson commented May 30, 2017 •

edited

Loading

davidben commented May 30, 2017

tmshort commented Jun 6, 2017

mattcaswell Jun 8, 2017

snhenson Jun 13, 2017

davidben Jun 15, 2017

snhenson Jun 15, 2017

ocheron Jun 16, 2017

ocheron Jun 16, 2017

snhenson Jun 16, 2017

ocheron Jun 17, 2017

davidben Jun 17, 2017

snhenson Jun 20, 2017

snhenson commented Jun 14, 2017

snhenson commented Jun 18, 2017

ocheron commented Jun 18, 2017

snhenson commented Jun 19, 2017

mattcaswell Jun 20, 2017

snhenson Jun 20, 2017

mattcaswell Jun 20, 2017

snhenson Jun 20, 2017

mattcaswell Jun 20, 2017

snhenson Jun 20, 2017

richsalz commented Jun 20, 2017

mattcaswell Jun 21, 2017

snhenson Jun 21, 2017

mattcaswell Jun 21, 2017

tmshort Jun 21, 2017

snhenson commented Jun 21, 2017

Ed25519 TLS support #3585

Ed25519 TLS support #3585

Conversation

snhenson commented May 30, 2017 • edited Loading

Checklist

davidben commented May 30, 2017

tmshort commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snhenson commented Jun 14, 2017

snhenson commented Jun 18, 2017

ocheron commented Jun 18, 2017

snhenson commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richsalz commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snhenson commented Jun 21, 2017

snhenson commented May 30, 2017 •

edited

Loading