Make HttpClient on Unix treat SslProtocols.None as meaning system defaults #13527
Conversation
…aults This way, as libcurl and OpenSSL are updated to support newer protocols and disable older ones, we inherit the latest settings.
| @@ -95,7 +95,12 @@ internal static void SetSslOptions(EasyRequest easy, ClientCertificateOption cli | |||
| private static void SetSslVersion(EasyRequest easy, IntPtr sslCtx = default(IntPtr)) | |||
| { | |||
| // Get the requested protocols. | |||
| System.Security.Authentication.SslProtocols protocols = easy._handler.ActualSslProtocols; | |||
| SslProtocols protocols = easy._handler.SslProtocols; | |||
There was a problem hiding this comment.
Nit: this looks odd - why is the code accessing private members?
There was a problem hiding this comment.
I don't understand the question. Are you asking how it's accessing easy._handler? _handler is internal, not private.
There was a problem hiding this comment.
Looks odd as it has the _ which, I thought, means private. If it's internal shouldn't it be capitalized and, even better, converted into a property to preserve encapsulation and the open-close principle? (easy.Handler instead of easy._handler)
There was a problem hiding this comment.
If it's internal shouldn't it be capitalized
Our guidelines are that all non-public instance fields are prefixed with an underscore, regardless of private or internal.
and, even better, converted into a property to preserve encapsulation and the open-close principle? (easy.Handler instead of easy._handler)
My care level on that, for an internal implementation detail where this is essentially just a bag of state shared between different pieces of the same type, is basically zero. If you'd like to submit a PR to make the change, I'd be fine with it. Regardless, that naming exists well earlier than this change.
| if (protocols == SslProtocols.None) | ||
| { | ||
| // Let libcurl use its defaults if None is set. | ||
| return; |
There was a problem hiding this comment.
Do we know that Curl will use TLS1.2 by default? (Is there a test?)
There was a problem hiding this comment.
Do we know that Curl will use TLS1.2 by default? (Is there a test?)
There was a problem hiding this comment.
(Note that newer versions of libcurl support the draft 1.3 as well.)
|
@dotnet-bot test Outerloop Ubuntu14.04 Debug please |
|
@dotnet-bot test Outerloop OSX Debug please (package restore failure) |
|
@CIPop, @bartonjs, this change broke a few of our outerloop tests on CentOS and RedHat. The purpose of this change was to use whatever the underlying platform defaults to, but libcurl prior to 7.39.0 included SSLv3 as an allowed protocol, and the version of libcurl used in CI on RedHat and CentOS is older than that. I see three options:
I'm leaning towards (3). Thoughts? |
|
If (3) is easy, I'd vote (3). If it's not, I'd vote (1). (If we had already had SystemDefault we probably wouldn't be looking at this at all, which is my basis for (1). Like, I don't expect us to necessarily revisit this when we start ensuring no one is hard-coding 1.0 as allowed...) |
|
In NetFX we leave the decision to the OS. Windows includes SSL3 within the defaults for versions < Win10 1607: https://support.microsoft.com/en-us/kb/3154518 (see the table under More Information). Windows APIs don't have the ability to remove protocols (we're discussing options with Schannel). If CURL has this option already, we should always remove unwanted protocols (SSL2, 3) regardless of the version while ensuring that new protocols are allowed (TLS1.3, future non-TLS protocols, etc). |
OpenSSL has that ability, and if you're using libcurl with an OpenSSL backend, we already do so (as part of this change). The issue is with libcurl with some other backend (which is the case in our CI environment with RedHat and CentOS). We don't have the ability to tell libcurl itself to remove protocols, so the best we could do is say to use TLS 1.x; that would remove SSLv3, but it would also prevent future non-TLS protocols. Thanks both for the feedback. Sounds like I should just leave the implementation as-is and fix the tests accordingly. |
Per discussion at #13075 (comment). Prior to this change, specifying SslProtocols.None for HttpClientHandler.SslProtocols meant that the default hardcoded into System.Net.Http should be used. This changes it to mean the underlying system's default, i.e. that of libcurl and OpenSSL on Unix.
cc: @bartonjs, @CIPop, @davidsh