Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (553f400) 95.90% compared to head (e5a2a2a) 95.90%.

@@           Coverage Diff           @@
##             main    #1651   +/-   ##
=======================================
  Coverage   95.90%   95.90%           
=======================================
  Files          78       78           
  Lines       16083    16083           
=======================================
  Hits        15424    15424           
  Misses        659      659           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Looks great! I've done a pass through the commits and spotted a couple tweaks:

These defaults are exposed as (eg) WebPkiServerVerifier::default_verify_tls13_signature which you can call to restore the old behaviour.

This bit fell out of date as of d963be3 - the default_xxx fns are removed, instead callers should use rustls::crypto::verify_tls12_signature, rustls::crypto::verify_tls13_signature and $PROVIDER.signature_verification_algorithms.supported_schemes() with the crypto provider of their choice.

Some other breaking changes to mention:

• RootCertStore::add_server_trust_anchors became RootCertStore::add_trust_anchors (6978464)
• The deprecated ConfigBuilder<ClientConfig, WantsClientCert>::with_single_cert fn was removed in favour of with_client_auth_cert (42cda46)

What do you think of adding the full list of items that have been moved / renamed / added / removed, like I summarized at #1435 (comment) ? I suspect most people will upgrade by building with the new version, seeing what breaks, then checking the changelog for the affected symbols. That will be be made easier if they are all named. Note my list only contains structs/enums/traits/functions. If it's useful I can add methods to the mix as well.

Also: can we add a stub CHANGELOG.md that points people to the GitHub Releases list? And/or link to the GitHub Releases list from lib.rs / README.md?

spotted a couple tweaks:

Edited these into the above.

What do you think of adding the full list of items that have been moved / renamed / added / removed

Have expanded/added more of the moved items, but ran out of steam for today. Feels like this is something that is best automated from the cargo doc json / cargo-public-api.

And/or link to the GitHub Releases list from lib.rs / README.md?

We actually have that -- https://github.com/rustls/rustls#release-history

Have expanded/added more of the moved items, but ran out of steam for today. Feels like this is something that is best automated from the cargo doc json / cargo-public-api.

I provided a list in the linked comment that I think is more comprehensive and easier to read than the old/new table: #1435 (comment). I think it should be possible to just copy-paste that in place of the old/new table.

Agreed that this is best automated (with some human editing on top to help it all make sense). I did try using cargo-public-api to generate the list, but it produced excessive diffs; I think it may be having trouble with rustdoc JSON generated by different versions of rustdoc.

And/or link to the GitHub Releases list from lib.rs / README.md?

We actually have that -- https://github.com/rustls/rustls#release-history

Huh, I went looking several times and didn't notice that link. I think that's because "release history can be found on GitHub" sounds like a tautology to me, since all releases are published on GitHub. I was searching for "changes" or "changelog" to find the human-curated description of the changes in each release. I'll submit a separate PR for this.

I feel like crypto providers should be at top of the list, before CRL support, since I suspect it will have impact for more of our audience. Otherwise, this looks good!

I provided a list in the linked comment that I think is more comprehensive and easier to read than the old/new table: #1435 (comment). I think it should be possible to just copy-paste that in place of the old/new table.

I see what you're saying now. The release note item with the table was really intended to record types that moved specifically for the crypto provider work. Not a list of absolutely everything moved/removed/added (some of which are better covered in other release note items, eg Certificate and PrivateKey). I have added the full list at the end, and updated to cover changes made since.

I feel like we should consider addressing #1628 (review) before we release.

I feel like we should consider addressing #1628 (review) before we release.

I think this is much too fundamental to be changing at this point.

I feel like crypto providers should be at top of the list, before CRL support,

Agreed, maybe for similar reasons we should put the aws-lc-rs support after the crypto provider and before the CRL bits?

I feel like we should consider addressing #1628 (review) [specifying TLS 1.2 and 1.3 ciphersuites separately] before we release.

I think this is much too fundamental to be changing at this point.

This is mainly a user interface change. Under the hood nothing would change.

The reason I think it's useful to squeeze in is that #1628 wound up introducing {Server,Client}Config::builder_with_protocols, which, while it's nice for reducing errors, is confusing to compare with ConfigBuilder::with_protocol_versions.

By making that proposed change we can (a) get rid of an unwrap during config building and (b) have exactly one place to specify the protocol versions (by specifying a CryptoProvider with an empty list of TLS 1.2 ciphersuites).

{Server,Client}Config::builder_with_protocols, which, while it's nice for reducing errors, is confusing to compare with ConfigBuilder::with_protocol_versions.

I'm not sure I understand what's confusing about it. Can you expand on your thought process?

Sure! If I'm a user looking at the docs and I know I want to turn off TLS 1.2, I might find ConfigBuilder::with_protocol_versions. So I write:

    let config = rustls::ClientConfig::builder()
    .with_protocol_versions(&[&rustls::version::TLS13])
    .unwrap()
    .with_root_certificates(root_store)
    .with_no_client_auth();

And I get the error:

error[E0599]: no method named `with_protocol_versions` found for struct `ConfigBuilder<ClientConfig, WantsVerifier>` in the current scope
  --> examples/src/bin/limitedclient.rs:19:6
   |
18 |       let config = rustls::ClientConfig::builder()
   |  __________________-
19 | |     .with_protocol_versions(&[&rustls::version::TLS13])
   | |     -^^^^^^^^^^^^^^^^^^^^^^ method not found in `ConfigBuilder<ClientConfig, WantsVerifier>`
   | |_____|
   | 
   |
   = note: the method was found for
           - `ConfigBuilder<S, WantsVersions>`

That error is a bit confusing if you haven't internalized the state machine of ConfigBuilder- that is, if you aren't paying close attention to the type parameters.

If I look in the docs for ConfigBuilder I would see that the way to get ConfigBuilder<S, WantsVersions> is to call ClientConfig::builder_with_provider. So I might go down the (wrong) avenue of trying to specify a provider.

Or I might notice at the same time that there is also ClientConfig::builder_with_protocols. In that case I would wonder "Why can protocols be set either before getting a builder, or while mutating a builder? Does it do different things if I choose different methods?"

@jsha Ahh! yes ok, I understand now. Thank you, that's helpful.

Agreed, maybe for similar reasons we should put the aws-lc-rs support after the crypto provider and before the CRL bits?

Have done that reordering.

This is mainly a user interface change. Under the hood nothing would change.

Not disagreeing, but the constraint we have here is to release 0.22 really very soon (today), to clear to board for 0.23 PRs to start being landed. Those need to be landed by EOY for rather dry contractual reasons.

Fair point. And with the additional discussion on #1628 (comment) I'm convinced it's not practical to land the change to configuring protocol versions in time for 0.22. Thanks for considering. 👍🏻

FYI made some edits to the top post to mention that OwnedTrustAnchor and ServerName were moved to rustls_pki_types.

If I find more things that should be in the changelog after the release is published, do you mind if I go ahead and edit them into the release notes, or would you like a code review process for changelog updates?

Also I think the changelog could use a section on related crates, saying for instance:

You will probably need to add a dependency on rustls-pki-types:

rustls-pki-types = { version = "1" }

Also, if you have dependencies on these related crates you will need to upgrade to these versions:

webpki-roots = { version = "0.26" }
rustls-webpki = { version = "0.102" }
rustls-pemfile = { version = "2" }
rustls-native-certs = ??

One question I've had is if we should be re-exporting the pki-types, maybe as rustls::types? I think that would make life a bit easier? We could do the same across some of the other crates, I suppose.

Bump on this:

do you mind if I go ahead and edit them into the release notes, or would you like a code review process for changelog updates?

There are some items I've noticed as I upgrade ureq that people are very likely to run into (like removing with_safe_defaults from ConfigBuilder). I'm going to go ahead and edit these into the release notes, but let me know if you'd like me to stop.

Yes, please do improve the release notes as you see fit.