add an option to specify ssl version

[guanqun]

Fixes #6684

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[guanqun]

@alexcrichton :)

[alexcrichton]

Thanks for the PR!

I've got some questions about this though before we might want to merge this:

• I don't personally understand the rationale for selecting TLS versions, but most libraries seem to have something like "here's the min" and "here's the min and max". This only exports the functionality of "here's the exact version", so I think there may be a functionality gap we want to fix? (being able to specify lower/upper bounds)
• Do we really need to export SSL2 and SSL3? I thought those were horribly broken and basically irresponsible to use at this point?

[guanqun]

• The personal reason for me to select TLS version is that I'm trying to use cargo behind our company and we're still using only TLS1.2 compatible ciphers. From API level, that's right most libraries export both "set min" and "set min and max" version, but as cargo is intended for end-users, and I'm following the flag used in openssl https://github.com/openssl/openssl/blob/master/apps/s_client.c#L1304 This flag -2 sets the exact version. We could possibly solve this by following two approaches:
  1. make ssl-version accept an OR string, e.g. "tlsv1.2 | tlsv1.3"
  2. create another flag in the config. so that we have both ssl-min-version and ssl-max-version. but it's a bit tedious.

which one do you prefer? other ideas?

• I'm including SSL2 and SSL3 for the sake of completeness, I agree with you we should remove this for the safety reasons.

[alexcrichton]

Perhaps something like ssl-version = "tlsv1.2" means "use that one version" but ssl-version = ["tlsv1.2", "tlsv1.3"] means "use that range of versions"?

That seems reasonable to drop the ssl variants for now, and we can just have the tls ones

[guanqun]

"sslv2" and "sslv3" methods are removed.

For the syntax: ssl-version = ["tlsv1.2", "tlsv1.3"] is not quite ideal, as the array gives the impression that it could have multiple versions there, but actually we're accepting only two.

how about we leverage the dotted key, thus we have ssl.min_version = "tlsv1.2" and another key as ssl.max_version = "tlsv1.3"? or ssl-version.min and ssl-version.max (naming is hard...)

[alexcrichton]

sounds reasonable to me!

[alexcrichton]

I like:

[http]
ssl-version = "tlsv1.2" # use this exact version

# specify min/max
ssl-version.min = "tlsv1.2" 
ssl-version.max = "tlsv1.3"

[bors]

☔ The latest upstream changes (presumably #7422) made this pull request unmergeable. Please resolve the merge conflicts.

[guanqun]

Sorry, I missed this one, will resolve the conflicts and add the new feature in a few days.

[guanqun]

an attempted fix, haven't tested yet, will let you know the testing result.

[alexcrichton]

Thanks! I think that we'll probably want to add a small tests for this as well, I think the usage of get_string with ? will cause problems here using both forms, but you should also be able to use get with a helper like so:

#[derive(Deserialize)]
#[serde(untagged)]
enum SslVersion {
    Exactly(String),
    Range(SslVersionRange),
}

#[derive(Deserialize)]
struct SslVersionRange {
    min: Option<String>,
    max: Option<String>,
}

[guanqun]

I added the unit test to cover several different cases. One thing that's not quite perfect is that if I'm using config.get::<Option<SslVersionConfig>>, then even if we have the both forms, it would simply return a Result<None>, it doesn't show up the proper errors. But anyway, having both forms is a wrong format in TOML format itself, any other usage of config.get_string() would trigger an error. so probably that's covered...

Another thing is the semantic update for single version and min/max version:

1. for single version, I'm calling handle.ssl_version()
2. for min/max, I'm calling handle.ssl_min_max_version().

I'll do some integration tests tomorrow and report back.

[alexcrichton]

This all sounds reasonable to me! Looks basically good to go to me when you're ok with it as well

[guanqun]

I tested this change behind our firm's firewall, and can confirm that if I'm just using ssl-version = "tlsv1.2", it would pickup the "tlsv1.3" because it means "tlsv1.2" and above. In this case the handshake would fail due to our self-assigned certificate.

Then I switch to ssl-version.min = "tlsv1.2" and ssl-version.max= "tlsv1.2", it's pinned down to this specific version and our self-assigned certificate works well for this protocol. It can download the proper crates and start building with cargo!

In a word, with this fix, cargo would work well with firms that have a mis-configured/not-well-supported self-assigned certificate. Hopefully this would encourge more companies to try out rust. :)

[alexcrichton]

@bors: r+

Looks fantastic to me, thanks again @guanqun!

[bors]

📌 Commit 852cf05 has been approved by alexcrichton

[bors]

⌛ Testing commit 852cf05 with merge 8ae8b5e...

[bors]

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 8ae8b5e to master...