Add TLS support for X448 and Ed448 #5470

mattcaswell · 2018-02-27T17:41:33Z

This PR adds support for X448 and Ed448 into libssl.

Note I'm planning to get all of this merged before beta1 (in 2 weeks!) if at all possible!!

kroeckx · 2018-02-27T17:58:11Z

Do you know about an other implementation that supports them and have you tried talking to it?

mattcaswell · 2018-02-27T17:59:07Z

No, I haven't tried. I'll look into it.

kroeckx · 2018-02-28T20:36:32Z

https://gitlab.com/ilari_l/btls claims to support X448 and experimental Ed448, it's the only thing I could find.

kroeckx · 2018-02-28T21:14:04Z

test/clienthellotest.c

+         * Otherwise we need to make sure we have a small enough message to
+         * not need padding.
+         */
+        else if (!TEST_true(SSL_CTX_set_cipher_list(ctx,


Does that else belong to just the "currtest == TEST_ADD_PADDING"?

Yes, good catch. Fixed.

kroeckx · 2018-02-28T21:18:12Z

test/ecdsatest.c

@@ -239,7 +239,7 @@ static int test_builtin(void)
        unsigned char dirt, offset;

        nid = curves[n].nid;
-        if (nid == NID_ipsec4 || nid == NID_X25519)
+        if (nid == NID_ipsec4)


Should NID_ipsec3 get excluded here too?

It actually does get excluded a few lines below in the EC_GROUP_get_degree() check.

kroeckx · 2018-02-28T21:31:56Z

test/ssl-tests/20-cert-select.conf.in

+            "ExpectedServerCertType" =>, "Ed448",
+            "ExpectedServerSignType" =>, "Ed448",
+            # Note: certificate_authorities not sent for TLS < 1.3
+            "ExpectedServerCANames" =>, "empty",


Should we have a TLS 1.3 check with it?

Do you mean should we have a check with Ed448 signing in TLSv1.3? If so there is one a bit later on (in the TLSv1.3 group of tests).

mattcaswell · 2018-03-02T10:57:07Z

I rebased this now that #5481 is merged. Also made one change as per the comments above.

mattcaswell · 2018-03-02T10:57:19Z

Oh...and I took this out of WIP.

richsalz

I'd wait for @kroeckx, but I'm okay with this after the nits.

richsalz · 2018-03-02T13:09:28Z

CHANGES

@@ -9,8 +9,8 @@

 Changes between 1.1.0g and 1.1.1 [xx XXX xxxx]

-  *) Added support for X448 and Ed448. Currently this is only supported in
-     libcrypto (not libssl). Heavily based on original work by Mike Hamburg.
+  *) Added support for X448 and Ed448 to both libcrypto and libssl. Heavily


get rid of "to both librypto and libssl"

richsalz · 2018-03-02T13:09:50Z

apps/s_cb.c

@@ -231,6 +231,9 @@ static const char *get_sigtype(int nid)
     case NID_ED25519:
        return "Ed25519";

+     case NID_ED448:
+        return "Ed448";


I thought we call it X448 ?

In this context we are specifically referring to it as a signature algorithm. Which means Ed448. We only use X448 if referring to it in the context of a Diffie-Hellman key exchange.

richsalz · 2018-03-02T13:12:34Z

test/ossl_shim/ossl_shim.cc

@@ -968,7 +968,7 @@ static bool DoExchange(bssl::UniquePtr<SSL_SESSION> *out_session,
  }
  if (config->enable_all_curves) {
    static const int kAllCurves[] = {
-      NID_X9_62_prime256v1, NID_secp384r1, NID_secp521r1, NID_X25519,
+      NID_X9_62_prime256v1, NID_secp384r1, NID_secp521r1, NID_X25519, NID_X448


If only to avoid questions like this one, should this be in the same order as eccurves_default?

richsalz · 2018-03-02T13:12:59Z

test/ssl-tests/14-curves.conf.in

@@ -17,7 +17,7 @@ my @curves = ("sect163k1", "sect163r1", "sect163r2", "sect193r1",
              "secp160r2", "secp192k1", "prime192v1", "secp224k1",
              "secp224r1", "secp256k1", "prime256v1", "secp384r1",
              "secp521r1", "brainpoolP256r1", "brainpoolP384r1",
-              "brainpoolP512r1", "X25519");
+              "brainpoolP512r1", "X25519", "X448");


Same ordering question.

This one is slightly different. It is a complete list of all the curves (not just default ones). Therefore it seems to make more sense to me to be in the order of the full nid_list (just above eccurves_default in t1_lib.c). This list is in that order.

mattcaswell · 2018-03-02T13:54:16Z

https://gitlab.com/ilari_l/btls claims to support X448 and experimental Ed448

I tested against that. Note this only supports TLSv1.2 (although it does seem to have some TLSv1.3 awareness as it validates the key_share extension - even though it doesn't use it (see below)).

After much frustration (there are no instructions) I successfully created a connection using both X448 and Ed448 blts-clienttest -> s_server. Connections using Ed448 s_client -> btls-servertest also fine. Initially connections using X448 s_client -> btls-servertest failed. This seems to be due to a bug in btls. I forced X448 on s_client by using "-groups X448". This sends a supported_groups extension containing exactly one group. However the btls server always seems to fail if the key_share uses a group which is the last one listed in supported_groups. Looking at the btls source code I was able to find the erroneous code. Sending more than one group solves the issues and X448 s_client -> btls-servertest works.

mattcaswell · 2018-03-02T13:57:02Z

tested against that. Note this only supports TLSv1.2

Ah. Although it only claims TLSv1.2 in the README, looking around the source code it does seem to know about TLSv1.3, but I think it is for a different draft version. Checking that out...

mattcaswell · 2018-03-02T14:56:39Z

Ah. Although it only claims TLSv1.2 in the README, looking around the source code it does seem to know about TLSv1.3, but I think it is for a different draft version. Checking that out...

It does support draft23/draft24. After hacking the source files to turn this on (surely there must be an easier way?) I encountered an OpenSSL bug that prevented a handshake from completing. This was nothing to do with X448/Ed448 - it was in our extension processing code (PR coming up). After fixing that I was able to confirm inter-operability for X448/Ed448 in both directions in TLSv1.3.

mattcaswell · 2018-03-02T16:00:26Z

I have also tested haskell client to s_server using X448. Also X448 in both directions for mint based on this PR:

bifurcation/mint#179

Note, in spite of the title of the PR, it only adds X448 support not Ed448.

All of the above tests passed.

mattcaswell · 2018-03-02T16:30:24Z

Fix up commits pushed addressing @richsalz's nits (where appropriate).

richsalz · 2018-03-02T16:34:44Z

Your changes and comments make sense; reconfirmed.

mattcaswell · 2018-03-05T11:48:32Z

Pushed. Thanks.

Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from openssl/openssl#5470)

This was referenced Feb 28, 2018

Add support for X448 and Ed448 #5419

Closed

Add support for X448 and Ed448 (take 2) #5481

Closed

Darkspirit mentioned this pull request Feb 28, 2018

X448 support briansmith/ring#464

Open

kroeckx reviewed Feb 28, 2018

View reviewed changes

mattcaswell added 3 commits March 2, 2018 10:30

Add X448/Ed448 support to libssl

f789ca3

Update tests for TLS Ed448

b731dc1

Update CHANGES for X448/Ed448 support in libssl

cfdbc04

mattcaswell force-pushed the curve448-tls branch from 9fe3208 to cfdbc04 Compare March 2, 2018 10:55

mattcaswell changed the title ~~WIP: Add TLS support for X448 and Ed448~~ Add TLS support for X448 and Ed448 Mar 2, 2018

richsalz approved these changes Mar 2, 2018

View reviewed changes

mattcaswell added 2 commits March 2, 2018 16:05

fixup! Update CHANGES for X448/Ed448 support in libssl

e127aa6

fixup! Update tests for TLS Ed448

5d618df

richsalz closed this Mar 2, 2018

richsalz reopened this Mar 2, 2018

kroeckx approved these changes Mar 2, 2018

View reviewed changes

mattcaswell closed this Mar 5, 2018

rhuijben pushed a commit to rhuijben/openssl that referenced this pull request Mar 5, 2018

Add X448/Ed448 support to libssl

0e1d6ec

Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from openssl/openssl#5470)

rhuijben pushed a commit to rhuijben/openssl that referenced this pull request Mar 5, 2018

Update tests for TLS Ed448

fe93b01

Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from openssl/openssl#5470)

rhuijben pushed a commit to rhuijben/openssl that referenced this pull request Mar 5, 2018

Update CHANGES for X448/Ed448 support in libssl

c04c602

Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from openssl/openssl#5470)

mspncp mentioned this pull request Mar 8, 2018

MAX_CURVELIST too small #5232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TLS support for X448 and Ed448 #5470

Add TLS support for X448 and Ed448 #5470

mattcaswell commented Feb 27, 2018 •

edited

Loading

kroeckx commented Feb 27, 2018

mattcaswell commented Feb 27, 2018

kroeckx commented Feb 28, 2018 •

edited

Loading

kroeckx Feb 28, 2018

mattcaswell Mar 2, 2018

kroeckx Feb 28, 2018

mattcaswell Mar 2, 2018

kroeckx Feb 28, 2018

mattcaswell Mar 2, 2018

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

richsalz left a comment

richsalz Mar 2, 2018

mattcaswell Mar 2, 2018

richsalz Mar 2, 2018

mattcaswell Mar 2, 2018

richsalz Mar 2, 2018

mattcaswell Mar 2, 2018

richsalz Mar 2, 2018

mattcaswell Mar 2, 2018

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

richsalz commented Mar 2, 2018

mattcaswell commented Mar 5, 2018

Add TLS support for X448 and Ed448 #5470

Add TLS support for X448 and Ed448 #5470

Conversation

mattcaswell commented Feb 27, 2018 • edited Loading

kroeckx commented Feb 27, 2018

mattcaswell commented Feb 27, 2018

kroeckx commented Feb 28, 2018 • edited Loading

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

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

richsalz left a 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

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

mattcaswell commented Mar 2, 2018

richsalz commented Mar 2, 2018

mattcaswell commented Mar 5, 2018

mattcaswell commented Feb 27, 2018 •

edited

Loading

kroeckx commented Feb 28, 2018 •

edited

Loading