Skip to content
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 from
Closed

TLS 1.3 support for GnuTLS #2971

wants to merge 7 commits into from

Conversation

loganaden
Copy link
Contributor

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: use fsetattr instead of setattr #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: use fsetattr instead of setattr #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: use fsetattr instead of setattr #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

@jay
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

@jay Latest commit follows your suggestion.

@jay
Copy link
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
Copy link
Contributor Author

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

@bagder
Copy link
Member

bagder commented Sep 21, 2018

Thanks!

@bagder bagder closed this in 9bdadbb Sep 21, 2018
@nmav
Copy link
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
Copy link
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
Copy link
Contributor Author

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

@nmav
Copy link
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.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants