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

Disable middlebox for TLS 1.3 #3221

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@technoLord
Copy link

technoLord commented Nov 2, 2018

Work done during IETF 103 Hackathon.

@danielgustafsson
Copy link
Member

danielgustafsson left a comment

Seems a good addition on a quick skim. Do you know if there are other TLS libraries that support this?

Help: Disable middlebox option for tls 1.3
Added: 7.25.0
---
This option tells curl to not use middlebox for tls 1.3.

This comment has been minimized.

@danielgustafsson

danielgustafsson Nov 2, 2018

Member

This should mention that it only applies to the OpenSSL backend.

This comment has been minimized.

@technoLord
@@ -2396,6 +2396,8 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)
ctx_options |= SSL_OP_NO_TLSv1_2;
#ifdef TLS1_3_VERSION
ctx_options |= SSL_OP_NO_TLSv1_3;
if (SSL_SET_OPTION(disable_tls13_middlebox))
SSL_CTX_clear_options(BACKEND->ctx, SSL_OP_ENABLE_MIDDLEBOX_COMPAT);

This comment has been minimized.

@danielgustafsson

danielgustafsson Nov 2, 2018

Member

Seems to have incorrect indentation, you can test by running make checksrc

This comment has been minimized.

@technoLord

technoLord Nov 3, 2018

Correct indentation used.

This comment has been minimized.

@vszakats

vszakats Nov 15, 2018

Member

Above indentation is still wrong and will result in a warning with certain C compilers.

@@ -0,0 +1,5 @@
Long: ssl-disable-tls13-middlebox
Help: Disable middlebox option for tls 1.3
Added: 7.25.0

This comment has been minimized.

@bagder

bagder Nov 2, 2018

Member

Ideally, this will go into 7.63.0...

This comment has been minimized.

@technoLord
@@ -795,6 +795,9 @@ typedef enum {
servers, a user can this way allow the vulnerability back. */
#define CURLSSLOPT_ALLOW_BEAST (1<<0)

/* - TLSv1.3 MIDDLEBOX mode */
#define CURLSSLOPT_TLS13_MIDDLEBOX (1<<0)

This comment has been minimized.

@bagder

bagder Nov 2, 2018

Member

Surely this shouldn't use the same bit as "allow beast" ?

This comment has been minimized.

@technoLord

technoLord Nov 3, 2018

New bit used for disable tls13 middlebox.

@mkauf

This comment has been minimized.

Copy link
Contributor

mkauf commented Nov 3, 2018

The feature should probably be called "disable middlebox compatibility mode". The name "disable middlebox" is misleading.

@@ -1473,6 +1473,7 @@ static CURLcode operate_do(struct GlobalConfig *global,
/* new in 7.25.0 and 7.44.0 */
{
long mask = (config->ssl_allow_beast ? CURLSSLOPT_ALLOW_BEAST : 0) |
(config->ssl_disable_tls13_middlebox ? CURLSSLOPT_TLS13_MIDDLEBOX: 0) |

This comment has been minimized.

@vszakats

vszakats Nov 4, 2018

Member

Indention is off by two spaces to the left here.

This comment has been minimized.

@technoLord

technoLord Nov 4, 2018

Indentation fixed

technoLord added some commits Nov 4, 2018

@danielgustafsson
Copy link
Member

danielgustafsson left a comment

Let's try to get this in shape for the next release window, there doesn't seem to be much left.

There are many red builds in Travis due to the symbol table being out of sync, the following diff should fix that:

diff --git a/docs/libcurl/symbols-in-versions b/docs/libcurl/symbols-in-versions
index 6c17c384c..449a32a2b 100644
--- a/docs/libcurl/symbols-in-versions
+++ b/docs/libcurl/symbols-in-versions
@@ -719,6 +719,7 @@ CURLSSLBACKEND_SCHANNEL         7.34.0
 CURLSSLBACKEND_WOLFSSL          7.49.0
 CURLSSLOPT_ALLOW_BEAST          7.25.0
 CURLSSLOPT_NO_REVOKE            7.44.0
+CURLSSLOPT_TLS13_MIDDLEBOX      7.64.0
 CURLSSLSET_NO_BACKENDS          7.56.0
 CURLSSLSET_OK                   7.56.0
 CURLSSLSET_TOO_LATE             7.56.0
@@ -229,6 +229,8 @@ struct OperationConfig {
bool xattr; /* store metadata in extended attributes */
long gssapi_delegation;
bool ssl_allow_beast; /* allow this SSL vulnerability */
bool ssl_disable_tls13_middlebox; /* disable tls 1.3 middlebox */
bool proxy_ssl_disable_tls13_middlebox; /* disable tls 1.3 middlebox for proxy */

This comment has been minimized.

@danielgustafsson

danielgustafsson Dec 10, 2018

Member

This variable seems unused?

@danielgustafsson

This comment has been minimized.

Copy link
Member

danielgustafsson commented Dec 24, 2018

Ping @technoLord. Are you still interested in getting this PR in, or would you like some help with the last few things?

@technoLord

This comment has been minimized.

Copy link

technoLord commented Dec 25, 2018

Hello @danielgustafsson . I need guidance regarding the libcurl part.

@danielgustafsson

This comment has been minimized.

Copy link
Member

danielgustafsson commented Dec 28, 2018

Hello @danielgustafsson . I need guidance regarding the libcurl part.

No worries, I've cleaned up a version of this patch with the libcurl parts fixed and will post a version for review soon.

danielgustafsson added a commit to danielgustafsson/curl that referenced this pull request Dec 30, 2018

openssl: TLS 1.3 middlebox compatibility mode option
This adds support for enabling or disabling the TLS 1.3 middlebox
compatibility mode when using the OpenSSL tls backend. The new
option is named --tls13-middlebox and takes a boolean. It is on
by default.

Closes curl#3221

danielgustafsson added a commit to danielgustafsson/curl that referenced this pull request Jan 1, 2019

openssl: TLS 1.3 middlebox compatibility mode option
This adds support for enabling or disabling the TLS 1.3 middlebox
compatibility mode when using the OpenSSL tls backend. The new
option is named --tls13-middlebox and takes a boolean. It is on
by default.

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