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

Added flags CURL_SSLVERSION_OR_UP_TO_* to CURLOPT_SSLVERSION #1166

Closed
wants to merge 25 commits into from
Closed

Added flags CURL_SSLVERSION_OR_UP_TO_* to CURLOPT_SSLVERSION #1166

wants to merge 25 commits into from

Conversation

@jkralik
Copy link

@jkralik jkralik commented Dec 17, 2016

By this patch user/developer can setup range of TLS versions, that
he want to support by curl/libcurl.

curl: curl --tlsv1.1 --up-to-tls-default https://www.google.com
libcurl: curl_easy_setopt(curl, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1.1 | CURL_SSLVERSION_OR_UP_TO_DEFAULT);

@mention-bot
Copy link

@mention-bot mention-bot commented Dec 17, 2016

@jkralik, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jay, @kdudka and @bagder to be potential reviewers.

Loading

@jay
Copy link
Member

@jay jay commented Dec 17, 2016

OR_UP_TO doesn't seem too intuitive, why not something like --tlsmin 1.0 --tlsmax 1.2

Loading

@bagder
Copy link
Member

@bagder bagder commented Dec 18, 2016

Yeah, max sounds better to my ears as well rather than "or_up_to". I think that if --tlsmax 1.2 is used, the regular --tlsv* option would be treated as the minimum version. Right?

Loading

@jkralik
Copy link
Author

@jkralik jkralik commented Dec 18, 2016

Right. --tlsv* is treated as minimum. For tool I will rewrite args "up-to-tlsv1.2,..." as --tlsmax 1.2. Do you want to refactor name of flags too? Like CURL_SSLVERSION_OR_UP_TO_TLSv1.2 =>CURL_SSLVERSION_MAX_TLSv1.2

Loading

@kdudka
Copy link
Collaborator

@kdudka kdudka commented Dec 19, 2016

Yes, CURL_SSLVERSION_MAX_TLSv1.2 sounds less cryptic to me.

Loading

Use up to TLSv1.2 . The supported minimum is tlsv1.0 or tlsv1.1.
.IP "1.3"
Use up to TLSv1.3 . The supported minimum is tlsv1.0 or tlsv1.1 or tlsv1.2.
.RE
Copy link
Collaborator

@kdudka kdudka Dec 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand it correctly that minimum and maximum cannot be set to the same value? Why?

Loading

Copy link
Author

@jkralik jkralik Dec 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is supported. But it is wired to tls-max with tlsvX.Y when you want to just tlsvX.Y. If you wish I can add this option to documentation.

Loading

Copy link
Collaborator

@kdudka kdudka Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop all the "The supported minimum is ..." sentences from the particular options. The statement above says it clearly enough in a generic way. The fact that the minimum enabled TLS version cannot be higher than the maximum enabled TLS version is obvious. Also the space before comma appears disruptive to me.

Loading

Copy link
Author

@jkralik jkralik Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - removed sentences.

Loading

switch(ssl_version) {
case CURL_SSLVERSION_TLSv1_0:
return set_ssl_version_up_to(conn, sockindex, ssl_version,
CURL_SSLVERSION_OR_UP_TO_TLSv1_0);
Copy link
Collaborator

@kdudka kdudka Dec 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you recursively call the function? Would not it be enough to (re)assign ssl_version_up_to and continue the execution?

Loading

Copy link
Author

@jkralik jkralik Dec 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I will rewrite it as you suggest.

Loading

@bagder bagder added the TLS label Dec 19, 2016
@jkralik
Copy link
Author

@jkralik jkralik commented Jan 16, 2017

Hi Kamil.
Please, would you look at the latest changes?
Is it ok, now?
Thx

Loading

@kdudka
Copy link
Collaborator

@kdudka kdudka commented Jan 19, 2017

Loading

docs/curl.1 Outdated
Use up to TLSv1.3 . The supported minimum is tlsv1.0 or tlsv1.1 or tlsv1.2.
.RE

See also \fI--tlsv1.0\fP and \fI--tlsv1.1\fP and \fI--tlsv1.2\fP. \fI--tls-max\fP requires that the underlying libcurl was built to support TLS. Added in 7.53.0.
Copy link
Collaborator

@kdudka kdudka Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please break the line such that it does not exceed 79 columns, according to:
https://github.com/curl/curl/blob/master/docs/CODE_STYLE.md

Loading

Copy link
Author

@jkralik jkralik Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs/curl.1 is generated document by command: docs/cmdline-opts/gen.pl mainpage

Loading

@@ -50,6 +50,18 @@ TLSv1.1 (Added in 7.34.0)
TLSv1.2 (Added in 7.34.0)
.IP CURL_SSLVERSION_TLSv1_3
TLSv1.3 (Added in 7.52.0)
.IP CURL_SSLVERSION_MAX_DEFAULT
Use as flag with CURL_SSLVERSION_TLSv1_x and behavior is same as
CURL_SSLVERSION_MAX_TLSv1_2.
Copy link
Collaborator

@kdudka kdudka Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is CURL_SSLVERSION_MAX_DEFAULT same as CURL_SSLVERSION_MAX_TLSv1_2? How would one ask libcurl to set the maximum TLS version to the TLS library default?

Loading

Copy link
Author

@jkralik jkralik Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I look to libraries and they are three kinds:

  1. supports to get/set default maximum: nss
  2. maximum is not determined: openssl, gnutls, mbedtls
  3. doesn't support: darwinSSL, polarssl, gskit

For 1. default is determined by library.
For 2. what is default? eg: library supports TLSv3, but TLSv2 in this time is recommended.
For 3. default must be determined by us.

My suggestion:
For 1. Function <Curl_ssl_evaluate_version_max> (renamed retrieve_ssl_version_max) calls function <Curl_ssl_get_default_max_version> that returns CURL_SSLVERSION_MAX_TLSvX for certain ssl library.
For 2. I suggest use behavior as in point 3.
For 3. <Curl_ssl_get_default_max_version> returns CURL_SSLVERSION_MAX_TLSv1_2.

What do you think?

Loading

Copy link
Collaborator

@kdudka kdudka Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if NSS was the only library that currently allows one to set sslver.max to TLS library default, it is still worth to have this feature exposed via libcurl API. We can hard-code it to TLSv1.2 for other libraries now and eventually implement propagation of the flag once other libraries introduce a similar API.

However, it needs to be hard-coded in libcurl implementation, not in its API/ABI. If both CURL_SSLVERSION_MAX_DEFAULT and CURL_SSLVERSION_MAX_TLSv1_2 were mapped to the same constant in libcurl API/ABI, it would be nearly impossible to change the default later on.

Loading

Copy link
Author

@jkralik jkralik Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, CURL_SSLVERSION_MAX_DEFAULT is as separeted value and every library CURL_SSLVERSION_MAX_DEFAULT must serve self (NSS via SSL_VersionRangeGetDefault and others as CURL_SSLVERSION_MAX_TLSv1.2).

Loading

#define GET_CURL_SSLVERSION(x) (x & 0xffff)
#define GET_CURL_SSLVERSION_MAX(x) (x & 0xffff0000)
#define SET_CURL_SSLVERSION_MAX(x, val) \
CURL_SSLVERSION_MAX_##x = (val << 16)
Copy link
Collaborator

@kdudka kdudka Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are changing a public header file. All the defines you introduce need to start with the CURL prefix.

Loading

Copy link
Author

@jkralik jkralik Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Loading

SET_CURL_SSLVERSION_MAX(TLSv1_3, 4),

SET_CURL_SSLVERSION_MAX(LAST, 5), /* never use, keep last */
SET_CURL_SSLVERSION_MAX(DEFAULT, 3) /* = ..._OR_UP_TO_TLSv1_2 */
Copy link
Collaborator

@kdudka kdudka Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks wrong to me. If we changed the default in libcurl later on, all dependent software would need to be recompiled for the change to take effect. You need to assign a separate value for CURL_SSLVERSION_MAX_DEFAULT so that a rebuilt of (dynamically linked) libcurl is sufficient to change the default.

Loading

}
}
return ssl_version_max_default;
}
Copy link
Collaborator

@kdudka kdudka Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you introducing a new module for the helper function? It could be added to the vtls module, which all the crypto backends use already. To be honest, I am not convinced that such a helper function simplifies the code anyhow. At least in the nss backend it does not seem to be the case.

Loading

lib/vtls/nss.c Outdated
}
return CURLE_OK;
}

Copy link
Collaborator

@kdudka kdudka Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drastically increases the code complexity, which is unnecessary IMO. I will propose a better approach once the API issues (namely CURL_SSLVERSION_MAX_DEFAULT) are resolved.

Loading

Copy link
Author

@jkralik jkralik Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CURL_SSLVERSION_MAX_DEFAULT is now as separated value and for NSS it uses maximum version from SSL_VersionRangeGetDefault. For other libraries it is CURL_SSLVERSION_MAX_TLSv1.2.

Loading

@jkralik
Copy link
Author

@jkralik jkralik commented Feb 3, 2017

Hello folks. I drop Curl_ssl_retrieve_version_max and simplify the code similarly as Kamil for others backends.

Loading

value from SSL library. Only library NSS currently allows to get
maximum supported TLS version.
(Added in 7.53.0)
.IP CURL_SSLVERSION_MAX_TLSv1_1
Copy link
Collaborator

@kdudka kdudka Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation for CURL_SSLVERSION_MAX_TLSv1_0 is missing.

Loading

Copy link
Author

@jkralik jkralik Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Loading

value from SSL library. Only library NSS currently allows to get
maximum supported TLS version.
(Added in 7.53.0)
.IP CURL_SSLVERSION_MAX_TLSv1_1
Copy link
Collaborator

@kdudka kdudka Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Documentation for CURL_SSLVERSION_MAX_TLSv1_0 is missing.

Loading

Copy link
Author

@jkralik jkralik Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Loading

size_t i = 0;
if(!str)
return PARAM_REQUIRES_PARAMETER;
for(i = 0; i < sizeof(tls_max_array)/sizeof(tls_max_array[i]); i++) {
Copy link
Collaborator

@kdudka kdudka Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizeof(tls_max_array)/sizeof(tls_max_array[0])

Loading

Copy link
Author

@jkralik jkralik Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point :)

Loading

if(ssl_version_max == CURL_SSLVERSION_MAX_NONE) {
ssl_version_max = ssl_version;
}

Copy link
Collaborator

@kdudka kdudka Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not we handle CURL_SSLVERSION_MAX_DEFAULT at this point?

Loading

Copy link
Author

@jkralik jkralik Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Loading

@iboukris
Copy link
Contributor

@iboukris iboukris commented Feb 3, 2017

This is interesting work, btw why not make the tls version a bit-mask instead of min-max, so it would be possible to do things like '--tlsv1.0 --tlsv1.2' for example.
Is that the control that the tls api / protocol allows?

Loading

lib/vtls/gtls.c Outdated
@@ -375,6 +375,98 @@ static gnutls_x509_crt_fmt_t do_file_type(const char *type)
return -1;
}

#ifndef USE_GNUTLS_PRIORITY_SET_DIRECT
static CURLcode
set_ssl_version_min_max(int *protocol_priority, struct connectdata *conn)
Copy link
Collaborator

@kdudka kdudka Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can lead to stack overflow if new TLS versions were added later on but size of the array not updated. Please make set_ssl_version_min_max() take size of the array as an argument and let it check the boundaries before writing to that array.

Loading

Copy link
Author

@jkralik jkralik Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Loading

lib/vtls/gtls.c Outdated
case CURL_SSLVERSION_TLSv1_2 | CURL_SSLVERSION_MAX_DEFAULT:
*prioritylist = GNUTLS_CIPHERS ":-VERS-SSL3.0:-VERS-TLS-ALL:"
"+VERS-TLS1.2:" GNUTLS_SRP;
break;
Copy link
Collaborator

@kdudka kdudka Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should fail by default instead of pretending success.

Loading

Copy link
Author

@jkralik jkralik Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default returns func CURLE_SSL_CONNECT_ERROR

Loading

CURLcode result = CURLE_OK;
long ssl_version = SSL_CONN_CONFIG(version);
long ssl_version_max = SSL_CONN_CONFIG(version_max) >> 16;
if(ssl_version_max == CURL_SSLVERSION_MAX_NONE) {
Copy link
Collaborator

@kdudka kdudka Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is not going to work as expected because ssl_version_max is already shifted. You might also want to handle CURL_SSLVERSION_MAX_DEFAULT at this point.

Loading

Copy link
Author

@jkralik jkralik Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Loading

@kdudka
Copy link
Collaborator

@kdudka kdudka commented Feb 3, 2017

Loading

@iboukris
Copy link
Contributor

@iboukris iboukris commented Feb 3, 2017

Loading

@kdudka
Copy link
Collaborator

@kdudka kdudka commented Feb 6, 2017

Looks good to me. Thanks for all the fixes! As it is two weeks after devel freeze now, I propose to wait till curl-7.53.0 is out, then rebase on top of upstream and push. Additional review comments are welcome!

Loading

@kdudka
Copy link
Collaborator

@kdudka kdudka commented Feb 27, 2017

I have squashed the commits and rebased against latest upstream. The result is available in the tls-max branch of my git repo: kdudka/curl@master...tls-max

Are we ready to merge this upstream?

Loading

@jkralik
Copy link
Author

@jkralik jkralik commented Feb 27, 2017

Hi Kamil. Do we want to set version "7.53.0" in documentation (and not "7.54.0") ?

Loading

@kdudka
Copy link
Collaborator

@kdudka kdudka commented Feb 27, 2017

Good point. 7.53.0 needs to be substituted by 7.54.0 everywhere in the patch.

Loading

@jkralik
Copy link
Author

@jkralik jkralik commented Feb 28, 2017

I create a patch (attached) to fix it for https://github.com/kdudka/curl/tree/tls-max. and I agree with merge.

0001-fixup-docs-set-right-version-for-specify-range-of-en.txt

Loading

kdudka added a commit to kdudka/curl that referenced this issue Mar 1, 2017
This commit introduces the CURL_SSLVERSION_MAX_* constants as well as
the --tls-max option of the curl tool.

Closes curl#1166
@kdudka
Copy link
Collaborator

@kdudka kdudka commented Mar 1, 2017

Thanks for the update! I have squashed all the fixup commits:
kdudka/curl@master...tls-max

@bagder are you fine with merging it upstream as it is?

Loading

kdudka added a commit to kdudka/curl that referenced this issue Mar 8, 2017
This commit introduces the CURL_SSLVERSION_MAX_* constants as well as
the --tls-max option of the curl tool.

Closes curl#1166
@kdudka
Copy link
Collaborator

@kdudka kdudka commented Mar 8, 2017

Rebased on top of upstream.

Loading

@bagder
Copy link
Member

@bagder bagder commented Mar 8, 2017

LGTM, ready to merge. When merged, this will make the next release truly become "7.54.0"

Loading

@kdudka
Copy link
Collaborator

@kdudka kdudka commented Mar 8, 2017

I have appended one more fixup to sync packages/OS400/curl.inc.in: kdudka@e8ef23d

Loading

@kdudka kdudka closed this in 6448f98 Mar 8, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants