-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
SSL version can be specified more precisely. #79
Conversation
CURL_SSLVERSION_TLSv1_0, CURL_SSLVERSION_TLSv1_1, CURL_SSLVERSION_TLSv1_2 enum values are added to force exact TLS version (CURL_SSLVERSION_TLSv1 means TLS 1.x). axTLS: axTLS only supports TLS 1.0 and 1.1 but it cannot be set that only one of these should be used, so we don't allow the new enum values. darwinssl: Added support for the new enum values. SChannel: Added support for the new enum values. CyaSSL: Added support for the new enum values. Bug: The original CURL_SSLVERSION_TLSv1 value enables only TLS 1.0 (it did the same before this commit), because CyaSSL cannot be configured to use TLS 1.0-1.2. GSKit: GSKit doesn't seem to support TLS 1.1 and TLS 1.2, so we do not allow those values. Bugfix: There was a typo that caused wrong SSL versions to be passed to GSKit. NSS: TLS minor version cannot be set, so we don't allow the new enum values. QsoSSL: TLS minor version cannot be set, so we don't allow the new enum values. OpenSSL: Added support for the new enum values. Bugfix: The original CURL_SSLVERSION_TLSv1 value enabled only TLS 1.0, now it enables 1.0-1.2. Command-line tool: Added command line options for the new values.
Is this really necessary? It seems to me that if the user wants a TLSv1 session, then we should always be requesting a 1.2 session if both the SSL library and the other server supports it, and downgrade from there if they don't. Otherwise, I can't think of why the user would want to specifically request a 1.1 session when both the client and the server can use 1.2. But I could be wrong... |
The released 7.32.0 version has a bug, that causes the value CURL_SSLVERSION_TLSv1 to enforce TLSv1.0 only (and not 1.1, 1.2) with OpenSSL backend. Pls see my original bug report at: Our client wants to connect to different servers, some of those support TLSv1.2, and some that only support TLSv1.0. We exactly know which server supports which version, and we want to enforce the highest version everywhere possible, to avoid version rollback attacks. That's why I added these new values instead of just fixing the bug. |
Then it seems to me that the OpenSSL code needs to be modified so that, by default and when TLSv1 is requested, then it allows up to 1.2 (if OpenSSL 1.0.1 or later is installed) with the option to fall back to 1.0 if 1.2 or 1.1 is not supported server-side. This is how some of the other back-ends, like the native Mac and Windows back-ends, work, and that's how the OpenSSL back-end also ought to work. So I don't see a reason to request a specific version when we should be asking for the highest supported version possible and then automatically falling back when that's not supported. But Daniel will have the final say here... |
I think we're talking about two different things here. First, I agree with Nick that when asking libcurl for "TLSv1" it doesn't mean 1.0, it means "1.0+" and really as high as possible but no less than 1.0. If I understand things correctly, ngg's patch makes this true. Then there's the second part where a user could be interested in specifying a least accepted TLS version to be 1.1 or 1.2 which then the existing libcurl options won't allow and as I read it that's also something that ngg's patch brings to the OpenSSL backend. I can't say that I know exactly when this second scenario will be interesting to users but I also can't see any obvious downside with allowing this - apart from the fact that it will become another differentiator between the different SSL backends libcurl supports. Is there really a reason to deny this second part? After all that's what we've offered before for SSLv2, SSLv3 and TLSv1... |
I'm happy with this patch (apart from the two lines that are 80+ columns) and I intend to merge it after the release when the feature window opens again. I'll leave this pull request open until I do that. There's talk in the IETF about an upcoming TLS version 1.3 but I can't see any reason why we would benefit from adding such a symbol already now when there's no actual such protocol version made yet. |
This has now been merged, thanks a lot! |
CURL_SSLVERSION_TLSv1_0, CURL_SSLVERSION_TLSv1_1,
CURL_SSLVERSION_TLSv1_2 enum values are added to force exact TLS version
(CURL_SSLVERSION_TLSv1 means TLS 1.x).