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

Use dynamic dispatch for CryptoProvider #1448

Merged
merged 6 commits into from Sep 19, 2023
Merged

Conversation

ctz
Copy link
Member

@ctz ctz commented Sep 6, 2023

The ClientConfig::builder() API is restored and suitable for most uses, but now is conditional on the ring crate feature.

ClientConfig::builder_with_provider(&'static dyn CryptoProvider) is unconditionally provided for being explicit or use with custom providers.

Consumers can make their choice explicit:

rustls::ClientConfig::builder_with_provider(rustls::crypto::ring::RING)
    .with_safe_defaults()

(this is durable to changes in our defaults changing the meaning of ClientConfig::builder().)

fixes #1409

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Merging #1448 (a0dec93) into main (cb9884d) will increase coverage by 0.03%.
Report is 4 commits behind head on main.
The diff coverage is 96.89%.

❗ Current head a0dec93 differs from pull request most recent head a5cc25b. Consider uploading reports for the commit a5cc25b to get more accurate results

@@            Coverage Diff             @@
##             main    #1448      +/-   ##
==========================================
+ Coverage   96.43%   96.46%   +0.03%     
==========================================
  Files          72       71       -1     
  Lines       15161    15232      +71     
==========================================
+ Hits        14620    14694      +74     
+ Misses        541      538       -3     
Files Changed Coverage Δ
rustls/src/client/builder.rs 88.57% <83.33%> (+0.33%) ⬆️
rustls/src/client/client_conn.rs 86.10% <90.90%> (+0.10%) ⬆️
rustls/src/client/tls13.rs 96.90% <92.00%> (-0.16%) ⬇️
rustls/src/server/tls13.rs 96.81% <94.87%> (-0.17%) ⬇️
rustls/src/crypto/ring/mod.rs 95.95% <97.50%> (+1.70%) ⬆️
rustls/src/builder.rs 99.00% <100.00%> (+0.05%) ⬆️
rustls/src/client/hs.rs 97.23% <100.00%> (+0.17%) ⬆️
rustls/src/client/tls12.rs 98.54% <100.00%> (+0.15%) ⬆️
rustls/src/msgs/handshake.rs 98.10% <100.00%> (ø)
rustls/src/quic.rs 76.65% <100.00%> (ø)
... and 6 more

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ctz ctz force-pushed the jbp-generalise-crypto-usage-pt3 branch 6 times, most recently from 847d74c to d204c06 Compare September 8, 2023 10:27
@ctz ctz force-pushed the jbp-generalise-crypto-usage-pt4 branch 5 times, most recently from 94d79cf to 941b132 Compare September 11, 2023 14:51
@ctz ctz force-pushed the jbp-generalise-crypto-usage-pt3 branch 5 times, most recently from 937bb0c to 78295cf Compare September 13, 2023 15:16
Base automatically changed from jbp-generalise-crypto-usage-pt3 to main September 13, 2023 15:40
@ctz ctz force-pushed the jbp-generalise-crypto-usage-pt4 branch 2 times, most recently from 069057b to f5ca822 Compare September 13, 2023 15:43
@stevefan1999-personal
Copy link
Contributor

stevefan1999-personal commented Sep 13, 2023

This is pretty good. I already hacked around hyper-rustls and tokio-rustls to target this patchset as a proof-of-concept to demonstrate how this would impact existing code, and how to port them.

@ctz ctz marked this pull request as ready for review September 13, 2023 16:39
rustls/src/crypto/mod.rs Outdated Show resolved Hide resolved
rustls/src/crypto/mod.rs Outdated Show resolved Hide resolved
rustls/src/client/hs.rs Outdated Show resolved Hide resolved
rustls/src/server/tls12.rs Outdated Show resolved Hide resolved
rustls/src/crypto/ring/mod.rs Outdated Show resolved Hide resolved
rustls/src/server/server_conn.rs Outdated Show resolved Hide resolved
ci-bench/src/main.rs Show resolved Hide resolved
provider-example/src/lib.rs Show resolved Hide resolved
rustls/src/crypto/mod.rs Outdated Show resolved Hide resolved
rustls/src/client/builder.rs Outdated Show resolved Hide resolved
rustls/src/crypto/ring/mod.rs Outdated Show resolved Hide resolved
provider-example/src/lib.rs Outdated Show resolved Hide resolved
provider-example/src/lib.rs Show resolved Hide resolved
rustls/src/crypto/mod.rs Show resolved Hide resolved
@djc djc mentioned this pull request Sep 14, 2023
15 tasks
@ctz ctz force-pushed the jbp-generalise-crypto-usage-pt4 branch from a0dec93 to 11fd16d Compare September 18, 2023 13:28
provider-example/src/kx.rs Show resolved Hide resolved
provider-example/src/kx.rs Outdated Show resolved Hide resolved
provider-example/src/kx.rs Outdated Show resolved Hide resolved
provider-example/src/lib.rs Outdated Show resolved Hide resolved
rustls/src/crypto/mod.rs Outdated Show resolved Hide resolved
@ctz ctz force-pushed the jbp-generalise-crypto-usage-pt4 branch from 0a02600 to cd84676 Compare September 19, 2023 10:36
This turns `SupportedKxGroup` into a trait, which can tell you
which `NamedGroup` it is, and `start()` an `ActiveKeyExchange`.

An `ActiveKeyExchange` represents the need for the peer's public key
which can be passed to `ActiveKeyExchange::complete`.

Unfortunately we can't be generic at compile-time over the various uses
of the resulting shared secret, so define a further type
which encapsulates the resulting shared secret.

Predefined key exchange algorithms (eg `rustls::kx_group::X25519`)
are now `&'static dyn rustls::SupportedKxGroup`.

The remainder of this commit is noise as much code ceased needing
to be generic of CryptoProvider (for its `KeyExchange` associated type).
Instead of the type `rustls::crypto::ring::Ring`, the value
`rustls::crypto::ring::RING` implements this, and is more
entertaining to write.

`ServerConfig::builder()` references this by default, and
is equivalent to `ServerConfig::builder_with_provider(crypto::ring::RING)`.
@ctz ctz force-pushed the jbp-generalise-crypto-usage-pt4 branch from cd84676 to a5cc25b Compare September 19, 2023 10:40
@ctz ctz added this pull request to the merge queue Sep 19, 2023
Merged via the queue into main with commit 30412d0 Sep 19, 2023
35 of 38 checks passed
@ctz ctz deleted the jbp-generalise-crypto-usage-pt4 branch September 19, 2023 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConfigBuilder: default crypto provider
4 participants