TLSv1.3 Add external PSK support #3670

mattcaswell · 2017-06-13T14:01:40Z

This PR adds support for external PSKs with TLSv1.3.

PSKs work differently in TLSv1.3 and there is a new API for them.

I have mostly reused the existing s_client/s_server options for PSKs. So "-psk" can be used to set a hex PSK key (which must be 32 or 48 bytes long to be usable by any TLSv1.3 ciphersuite), and "-psk_identity" is used to set the identity associated with the PSK. The server side "-psk_hint" is not relevant for TLSv1.3. I have also introduced a new option "-psk_session" which only works for TLSv1.3 and can be used instead of "-psk". The argument is a session file which is used as the basis for the PSK (i.e. the key is loaded from the session file). This enables other settings, such as ALPN, to be associated with the PSK too.

Marked as WIP because this still needs tests and documentation.

Note: this is based on PR#3623 so there are a number of commits from that PR showing here.

Checklist

documentation is added or updated
tests are added or updated

mattcaswell · 2017-06-16T11:07:33Z

Rebased now that #3623 has gone in. I've also added some documentation. Still WIP because there is more documentation to do and tests to add.

Broken through previous PSK related commits

Ensure that we properly distinguish between successful return (PSK provided), successful return (no PSK provided) and failure.

mattcaswell · 2017-06-19T14:54:53Z

Rebased again and more documentation added. Just tests to add now.

Found while developing the PSK tests

mattcaswell · 2017-06-20T13:28:14Z

I've now added some tests for this too. This is now out of WIP and ready for review. Anyone?

richsalz · 2017-06-20T13:31:57Z

apps/apps.h

@@ -515,9 +515,9 @@ int do_X509_REQ_sign(X509_REQ *x, EVP_PKEY *pkey, const EVP_MD *md,
                     STACK_OF(OPENSSL_STRING) *sigopts);
 int do_X509_CRL_sign(X509_CRL *x, EVP_PKEY *pkey, const EVP_MD *md,
                     STACK_OF(OPENSSL_STRING) *sigopts);
-# ifndef OPENSSL_NO_PSK
+


I am in favor of fewer config things. So TLS 1.3 turns PSK on. Can TLS 1.3 be disabled at config time? Should there be a warning if you do enable-tls13 and disable-psk? Should we really remove all no-psk guards?

No, TLS1.3 does not turn PSK on. OPENSSL_NO_PSK only applies to TLSv1.2 and does not impact the TLSv1.3 PSK implementation. In the apps there are a few options that are re-used between the TLSv1.2 and TLSv1.3 implementations which is why there are a few changes to the guards in this area.

richsalz · 2017-06-20T13:33:54Z

apps/s_client.c

@@ -197,6 +198,77 @@ static unsigned int psk_client_cb(SSL *ssl, const char *hint, char *identity,
 }
 #endif

+#define TLS13_AES_128_GCM_SHA256_BYTES  ((const unsigned char *)"\x13\x01")


Is this better?

static const unsigned char tls13_aesgcm256_id[] = { 0x13, 0x01 };

? I think so ...

richsalz · 2017-06-20T13:35:35Z

apps/s_client.c

+            *sess = NULL;
+            return 0;
+        }
+        usesess = SSL_SESSION_new();


I keep reading this variable as "useless" Maybe just "sess" or "newsess" Or leave it as-is.

I think that's just you ;-). I left it.

richsalz · 2017-06-20T13:35:43Z

apps/s_client.c

+    }
+
+    cipher = SSL_SESSION_get0_cipher(usesess);
+


remove this blank line.

richsalz · 2017-06-20T13:36:19Z

apps/s_client.c

    OPT_PSK_IDENTITY, OPT_PSK,
-#endif
+    OPT_PSK_SESS,


wrap this on the previous line.

richsalz · 2017-06-20T13:56:17Z

ssl/ssl_lib.c

+
+    memcpy(sess->master_key, in, len);
+    sess->master_key_length = len;
+


remove this blank line.

richsalz · 2017-06-20T13:58:23Z

ssl/statem/extensions_clnt.c

-        return EXT_RETURN_NOT_SENT;
-    }
+    if (s->session->ext.ticklen != 0) {
+        if (s->session->cipher == NULL) {


I'd like to see a short comment here, if possible.

richsalz · 2017-06-20T14:03:27Z

ssl/statem/extensions_clnt.c

+        }
+
+        pskhashsize = EVP_MD_size(mdpsk);
+    }

    /* Create the extension, but skip over the binder for now */


is this skip binder part still right?

Yes, the binder generation isn't done until the tls_psk_do_binder() calls below.

richsalz · 2017-06-20T14:04:23Z

ssl/statem/extensions_clnt.c

@@ -893,16 +956,30 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,

    msgstart = WPACKET_get_curr(pkt) - msglen;

-    if (tls_psk_do_binder(s, md, msgstart, binderoffset, NULL, binder,
-                          s->session, 1) != 1) {
+    if (dores && tls_psk_do_binder(s, mdres, msgstart, binderoffset, NULL,


break the line before the "&&" and rewrap the args. here and below. it will be logically cleaner.

richsalz · 2017-06-20T14:07:23Z

test/sslapitest.c

+static int use_session_cb(SSL *ssl, const EVP_MD *md, const unsigned char **id,
+                          size_t *idlen, SSL_SESSION **sess)
+{
+    use_session_cb_cnt++;


perhaps switch (++use_session_cb_cnt) is more clear here.

mattcaswell · 2017-06-21T11:25:47Z

New commits pushed addressing review comments.

mattcaswell · 2017-06-21T13:50:54Z

Pushed. Thanks.

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

Broken through previous PSK related commits Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

Ensure that we properly distinguish between successful return (PSK provided), successful return (no PSK provided) and failure. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

Found while developing the PSK tests Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

mattcaswell mentioned this pull request Jun 14, 2017

TLSv1.3 Exporters #3680

Closed

mattcaswell force-pushed the tls1_3-psk branch from 2d11d50 to 6d76f21 Compare June 16, 2017 11:05

mattcaswell added 19 commits June 19, 2017 15:06

Add TLSv1.3 server side external PSK support

6750061

Add TLSv1.3 client side external PSK support

9a79216

Add public API functions for setting TLSv1.3 PSK callbacks

b2393a5

Add a function to get the handshake digest for an SSL_CIPHER

aea4524

Add the ability to use a client side TLSv1.3 external PSK in s_client

499d30f

Add the ability to use a server side TLSv1.3 external PSK in s_server

4aba5be

Add SSL_SESSION_set1_master_key()

af8579c

Add SSL_SESSION_set_cipher()

6ae55c8

Add SSL_SESSION_set_protocol_version()

9b3c05e

Add the ability to set a TLSv1.3 PSK via just the key bytes

a3e44f9

Fix no-psk

a4a53b2

Broken through previous PSK related commits

Add documentation for SSL_SESSION_set_cipher()

5ad953a

Document SSL_SESSION_set1_master_key()

e8f5210

Document SSL_SESSION_set_protocol_version()

97b037c

Document SSL_CIPHER_get_handshake_digest()

bcb86aa

Add documentation for SSL_CTX_set_psk_use_session_callback()

ab4691f

Tweak the client side PSK callback

135dcc0

Ensure that we properly distinguish between successful return (PSK provided), successful return (no PSK provided) and failure.

Document SSL_set_psk_use_session_callback() and SSL_CTX equivalent

64d4da4

Document SSL_set_psk_find_session_callback() and SSL_CTX equivalent

f559ff3

mattcaswell force-pushed the tls1_3-psk branch from 6d76f21 to f559ff3 Compare June 19, 2017 14:54

mattcaswell added 3 commits June 20, 2017 14:24

Make the input parameters for SSL_SESSION_set1_master_key const

e1ecebd

Fix some bugs in the TLSv1.3 PSK code

6fc748f

Found while developing the PSK tests

Add some tests for the new TLSv1.3 PSK code

e9376b9

mattcaswell changed the title ~~WIP: TLSv1.3 Add external PSK support~~ TLSv1.3 Add external PSK support Jun 20, 2017

richsalz suggested changes Jun 20, 2017

View reviewed changes

mattcaswell added 2 commits June 21, 2017 11:58

Use constants rather than macros for the cipher bytes in the apps

ba8659d

PSK related tweaks based on review feedback

c1c1bf4

richsalz approved these changes Jun 21, 2017

View reviewed changes

mattcaswell closed this Jun 21, 2017

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

Add TLSv1.3 server side external PSK support

3a7c56b

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add TLSv1.3 client side external PSK support

9368f86

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add public API functions for setting TLSv1.3 PSK callbacks

f46184b

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add a function to get the handshake digest for an SSL_CIPHER

ba4df68

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add the ability to use a client side TLSv1.3 external PSK in s_client

e261bdd

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add the ability to use a server side TLSv1.3 external PSK in s_server

df89494

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add SSL_SESSION_set1_master_key()

911d63f

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add SSL_SESSION_set_cipher()

1a993d1

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add SSL_SESSION_set_protocol_version()

5a43d51

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add the ability to set a TLSv1.3 PSK via just the key bytes

5ffff59

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Fix no-psk

14e3535

Broken through previous PSK related commits Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add documentation for SSL_SESSION_set_cipher()

7721978

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Document SSL_SESSION_set1_master_key()

5eb7273

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Document SSL_SESSION_set_protocol_version()

267869d

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Document SSL_CIPHER_get_handshake_digest()

9c39fa1

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add documentation for SSL_CTX_set_psk_use_session_callback()

801d9fb

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Document SSL_set_psk_use_session_callback() and SSL_CTX equivalent

93a048a

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Document SSL_set_psk_find_session_callback() and SSL_CTX equivalent

8ead615

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Make the input parameters for SSL_SESSION_set1_master_key const

725b0f1

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Fix some bugs in the TLSv1.3 PSK code

011d768

Found while developing the PSK tests Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Add some tests for the new TLSv1.3 PSK code

ca8c71b

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

Use constants rather than macros for the cipher bytes in the apps

adfc378

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

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

PSK related tweaks based on review feedback

7225720

Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3670)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLSv1.3 Add external PSK support #3670

TLSv1.3 Add external PSK support #3670

mattcaswell commented Jun 13, 2017

mattcaswell commented Jun 16, 2017

mattcaswell commented Jun 19, 2017

mattcaswell commented Jun 20, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

richsalz Jun 20, 2017

mattcaswell Jun 21, 2017

mattcaswell commented Jun 21, 2017

mattcaswell commented Jun 21, 2017


		memcpy(sess->master_key, in, len);
		sess->master_key_length = len;

TLSv1.3 Add external PSK support #3670

TLSv1.3 Add external PSK support #3670

Conversation

mattcaswell commented Jun 13, 2017

Checklist

mattcaswell commented Jun 16, 2017

mattcaswell commented Jun 19, 2017

mattcaswell 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

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

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 Jun 21, 2017

mattcaswell commented Jun 21, 2017