New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TLS 1.3 support for GnuTLS #2971

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@loganaden
Contributor

loganaden commented Sep 10, 2018

Test against tls 1.3 servers (without any arguments):

./src/curl -v https://tls13.crypto.mozilla.org/

  • Trying 52.32.149.186...
  • TCP_NODELAY set
  • Connected to tls13.crypto.mozilla.org (52.32.149.186) port 443 (#0)
  • found 151 certificates in /etc/ssl/certs/ca-certificates.crt
  • This GnuTLS does not support SRP
  • ALPN, offering h2
  • ALPN, offering http/1.1
  • SSL connection using TLS1.3 / ECDHE_RSA_AES_128_GCM_SHA256
  • server certificate verification OK
  • server certificate status verification SKIPPED
  • common name: tls13.crypto.mozilla.org (matched)
  • server certificate expiration date OK
  • server certificate activation date OK
  • certificate public key: RSA
  • certificate version: #3
  • subject: CN=tls13.crypto.mozilla.org
  • start date: Wed, 20 Jun 2018 09:14:50 GMT
  • expire date: Tue, 18 Sep 2018 09:14:50 GMT
  • issuer: C=US,O=Let's Encrypt,CN=Let's Encrypt Authority X3
  • compression: NULL

Test against TLS 1.3 server with explicit TLS 1.3:
./src/curl -v --tlsv1.3 https://tls13.crypto.mozilla.org/

  • Trying 52.32.149.186...
  • TCP_NODELAY set
  • Connected to tls13.crypto.mozilla.org (52.32.149.186) port 443 (#0)
  • found 151 certificates in /etc/ssl/certs/ca-certificates.crt
  • This GnuTLS does not support SRP
  • ALPN, offering h2
  • ALPN, offering http/1.1
  • SSL connection using TLS1.3 / ECDHE_RSA_AES_128_GCM_SHA256
  • server certificate verification OK
  • server certificate status verification SKIPPED
  • common name: tls13.crypto.mozilla.org (matched)
  • server certificate expiration date OK
  • server certificate activation date OK
  • certificate public key: RSA
  • certificate version: #3
  • subject: CN=tls13.crypto.mozilla.org
  • start date: Wed, 20 Jun 2018 09:14:50 GMT
  • expire date: Tue, 18 Sep 2018 09:14:50 GMT
  • issuer: C=US,O=Let's Encrypt,CN=Let's Encrypt Authority X3

Test with TLS 1.2 server without any argument (to check for regressions):
./src/curl -v https://www.google.mu/

  • Trying 216.58.223.67...
  • TCP_NODELAY set
  • Connected to www.google.mu (216.58.223.67) port 443 (#0)
  • found 151 certificates in /etc/ssl/certs/ca-certificates.crt
  • This GnuTLS does not support SRP
  • ALPN, offering h2
  • ALPN, offering http/1.1
  • SSL connection using TLS1.2 / ECDHE_RSA_CHACHA20_POLY1305
  • server certificate verification OK
  • server certificate status verification SKIPPED
  • common name: *.google.mu (matched)
  • server certificate expiration date OK
  • server certificate activation date OK
  • certificate public key: RSA
  • certificate version: #3
  • subject: C=US,ST=California,L=Mountain View,O=Google LLC,CN=*.google.mu
  • start date: Tue, 21 Aug 2018 08:04:00 GMT
Loganaden Velvindron
@jay

This comment has been minimized.

Member

jay commented Sep 10, 2018

You will have to add all the new combinations because if TLS 1.3 is the max then we have that many more, so for example case CURL_SSLVERSION_TLSv1_2 | CURL_SSLVERSION_MAX_DEFAULT it's going to be +VERS-TLS1.2:+VERS-TLS1.3: now if I understand this correctly.

@loganaden

This comment has been minimized.

Contributor

loganaden commented Sep 10, 2018

Thanks. You're right @jay. Updating the patch right away. However, this will include more ifdef to check for the correct version of gnutls.

@jay

This comment has been minimized.

Member

jay commented Sep 10, 2018

It can't be done that way, what I'm saying is for example

    case CURL_SSLVERSION_TLSv1_0 | CURL_SSLVERSION_MAX_TLSv1_2:
    case CURL_SSLVERSION_TLSv1_0 | CURL_SSLVERSION_MAX_DEFAULT:

those things now are different when max default is 1.3. my suggestion is break out those max defaults separately

at the beginning of the file

#if GNUTLS_VERSION_NUMBER >= 0x030603
#define HAS_TLS13
#endif

and in that function handle max defaults separately

    case CURL_SSLVERSION_TLSv1_0 | CURL_SSLVERSION_MAX_DEFAULT:
      *prioritylist = GNUTLS_CIPHERS ":-VERS-SSL3.0:-VERS-TLS-ALL:"
                      "+VERS-TLS1.0:+VERS-TLS1.1:+VERS-TLS1.2:"
#ifdef HAS_TLS13
                      "+VERS-TLS1.3:"
#endif
                      GNUTLS_SRP;

loganaden added some commits Sep 11, 2018

@loganaden

This comment has been minimized.

Contributor

loganaden commented Sep 11, 2018

@jay Latest commit follows your suggestion.

@jay

This comment has been minimized.

Member

jay commented Sep 11, 2018

It looks better but is still wrong. The CURL_SSLVERSION_MAX_TLSvxxx is different from CURL_SSLVERSION_MAX_DEFAULT. The former shouldn't change (eg CURL_SSLVERSION_MAX_TLSv1_2 is always TLS 1.2) but the latter should if TLS 1.3 is enabled. That is why I suggest handle the MAX_DEFAULT cases separately after the MAX_TLS, it should be easiest to read, for example

case CURL_SSLVERSION_TLSv1_2 | CURL_SSLVERSION_MAX_TLSv1_2:
...
case CURL_SSLVERSION_TLSv1_3 | CURL_SSLVERSION_MAX_TLSv1_3:
...
case CURL_SSLVERSION_TLSv1_2 | CURL_SSLVERSION_MAX_DEFAULT:
...
case CURL_SSLVERSION_TLSv1_3 | CURL_SSLVERSION_MAX_DEFAULT:
...

@loganaden

This comment has been minimized.

Contributor

loganaden commented Sep 11, 2018

@jay Now I get what you mean. PR updated.

@bagder

This comment has been minimized.

Member

bagder commented Sep 21, 2018

Thanks!

@bagder bagder closed this in 9bdadbb Sep 21, 2018

@nmav

This comment has been minimized.

Contributor

nmav commented Sep 21, 2018

Thank you for that. Note that this does not enable post-handshake authentication, and thus re-authentication will fail under HTTPS (I'm not sure if PHA is relevant for other protocols). That's a tricky use-case used by apache mostly. Tried something quick and dirty (listed below) and made my mind in simplifying that re-authentication support in gnutls (https://gitlab.com/gnutls/gnutls/issues/571 )

diff --git a/lib/vtls/gtls.c b/lib/vtls/gtls.c
index 8b8b64bde..16f6ef8bf 100644
--- a/lib/vtls/gtls.c
+++ b/lib/vtls/gtls.c
@@ -664,6 +664,10 @@ gtls_connect_step1(struct connectdata *conn,
 
   /* Initialize TLS session as a client */
   init_flags = GNUTLS_CLIENT;
+#ifdef HAS_TLS13
+  /* enable post-handshake authentication */
+  init_flags |= GNUTLS_POST_HANDSHAKE_AUTH;
+#endif
 
 #if defined(GNUTLS_NO_TICKETS)
   /* Disable TLS session tickets */
@@ -1720,6 +1724,14 @@ static ssize_t gtls_recv(struct connectdata *conn, /* connection data */
     return -1;
   }
 
+  if (ret == GNUTLS_E_REAUTH_REQUEST) {
+    /* BLOCKING call, this is bad but a work-around for now. Fixing this "the
+       proper way" takes a whole lot of work. */
+    do {
+      ret = gnutls_reauth(BACKEND->session, 0);
+    } while(ret == GNUTLS_E_INTERRUPTED || GNUTLS_E_AGAIN);
+  }
+
   if(ret == GNUTLS_E_REHANDSHAKE) {
     /* BLOCKING call, this is bad but a work-around for now. Fixing this "the
        proper way" takes a whole lot of work. */
@bagder

This comment has been minimized.

Member

bagder commented Sep 21, 2018

Thanks @nmav! As we already merged this #2971, would you mind providing your patch as a separate follow-up PR?

@loganaden

This comment has been minimized.

Contributor

loganaden commented Sep 21, 2018

@bagder & @nmav thanks. I am going to test the patch on my system by setting up an apache server.

@nmav

This comment has been minimized.

Contributor

nmav commented Sep 23, 2018

It is really a quick hack (seeing the complexity in handshake()). Feel free to use it for a better version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment