Make ring optional, and demonstrate how alternatives would be integrated #1405

ctz · 2023-08-17T11:24:00Z

This builds on #1401 to make ring and webpki dependencies optional. Then there is a demonstration of custom crypto using various crypto crates from the ecosystem.

TODOs:

we should run cargo run --example client in the connect-tests job
~~this PR seems like a good venue to introduce an optional aws-lc-rs dependency, and arrange to run our tests against it~~
run cargo hack --feature-powerset --no-dev-deps build and cargo hack --feature-powerset test to make sure all combinations of features at least compile and pass tests
seems like the is some utility in making the provider example introduced here more reusable: Check for UB using krabcake rustls-ffi#342 (comment)
- this will be follow-on work, see discussion on add an example tls server with provider #1440

codecov · 2023-08-17T11:29:10Z

Codecov Report

Merging #1405 (78295cf) into main (3fc1c93) will decrease coverage by 0.13%.
The diff coverage is 94.59%.

@@            Coverage Diff             @@
##             main    #1405      +/-   ##
==========================================
- Coverage   96.55%   96.43%   -0.13%     
==========================================
  Files          71       72       +1     
  Lines       15123    15163      +40     
==========================================
+ Hits        14602    14622      +20     
- Misses        521      541      +20

Files Changed	Coverage Δ
rustls/src/builder.rs	`98.95% <ø> (ø)`
rustls/src/client/builder.rs	`88.23% <ø> (ø)`
rustls/src/crypto/mod.rs	`0.00% <ø> (ø)`
rustls/src/crypto/ring/mod.rs	`94.25% <ø> (ø)`
rustls/src/hash_hs.rs	`100.00% <ø> (ø)`
rustls/src/hkdf.rs	`100.00% <ø> (ø)`
rustls/src/server/server_conn.rs	`87.41% <ø> (ø)`
rustls/src/suites.rs	`99.24% <ø> (ø)`
rustls/src/tls12/mod.rs	`97.82% <ø> (ø)`
rustls/src/tls12/prf.rs	`100.00% <ø> (ø)`
... and 18 more

... and 2 files with indirect coverage changes

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

djc · 2023-08-17T11:30:47Z

I think we should postpone adding aws-lc-rs until a future PR -- this one is already large enough as it is.

djc

Early feedback, hope it's useful.

rustls/src/signer.rs

rustls/src/webpki/verify.rs

rustls/Cargo.toml

provider-example/src/aead.rs

ctz · 2023-08-31T13:22:14Z

Quick note for bisecting icount changes:

$ mkdir ci-bench/commits
$ git rebase -i main --exec 'cd ci-bench && cargo run --release run-all > commits/$(git rev-parse HEAD).csv'
(...)
$ grep handshake_tickets_1.2_rsa_aes_client ci-bench/commits/*.csv
ci-bench/commits/0438e888d1617a32d244eabdf2b8c7232ea469c9.csv:handshake_tickets_1.2_rsa_aes_client,4777225
ci-bench/commits/39ab4ea727a2b7b96cec021c6e8876a2c4695b5d.csv:handshake_tickets_1.2_rsa_aes_client,4774559
ci-bench/commits/4b9efe02185956838fcb8c3bc97324847d07543e.csv:handshake_tickets_1.2_rsa_aes_client,4771167
ci-bench/commits/6d0896cd768220c68eb5f36f640023636da994c8.csv:handshake_tickets_1.2_rsa_aes_client,4769570
ci-bench/commits/9383141f6d44cccfc65a35e4e0080d77873d4a9a.csv:handshake_tickets_1.2_rsa_aes_client,4775916
ci-bench/commits/ff29c8a92082faa99aaee375dde120e169deedf7.csv:handshake_tickets_1.2_rsa_aes_client,4774989

Which doesn't point to a specific commit. Hmm!

@aochagavia any ideas?

djc · 2023-08-31T13:47:23Z

What doesn't point to a specific commit? I'm guessing if main changed underneath you the commit hashes might, too?

aochagavia · 2023-08-31T13:53:18Z

There is a very high chance the "noteworthy" diffs you are seeing are just noise (it involves only 2 resumed handshakes, which are known to be noisy, and the diffs are close to the noise threshold). I thought I'd managed to solve the noise problem, but it looks like I'll have to tweak things some more.

ctz · 2023-08-31T14:05:18Z

What doesn't point to a specific commit?

Sorry, I mean the performance regression doesn't seem to be attributable to any one commit in this PR. But now I think that's because I can't see the regression locally.

There is a very high chance the "noteworthy" diffs you are seeing are just noise

Ah, understood. Is there much that can be done for that?

I saw your comment about HashMap usage which makes sense for servers, but not clients (because they're only talking to one server ever, they would end up with their cache HashMap only ever containing one item, indexed by "localhost"?)

aochagavia · 2023-08-31T14:18:42Z

Hmmm I just checked out the code locally and maybe it's not noise after all. Do you mind if I have a look at this next week?

ctz · 2023-08-31T14:19:33Z

Sure, yes!

aochagavia · 2023-09-04T08:56:40Z

After some more investigation, I discovered the stdio IPC introduces a low amount of noise that affects the resumed handshakes. I'll be figuring out how to get rid from it, and afterwards compare again between main and this PR.

Update: I can confirm this is noise, though interestingly the handshake_session_id_1.2_rsa_aes_client has reduced its instruction count with 0.40%. Below is the reduction's details, as reported by cg_diff + cg_annotate. It looks like we now allocate less, the encoding of messages requires less instructions, and supported_verify_schemes requires less instructions too. Does that make sense?

-12,401 (65.99%)  ./malloc/./malloc/malloc.c:_int_malloc
 -4,516 (24.03%)  ./malloc/./malloc/malloc.c:unlink_chunk.constprop.0
 -3,764 (20.03%)  ???:rustls::msgs::message::MessagePayload::encode
  2,542 (-13.5%)  ???:<rustls::msgs::message::PlainMessage as core::convert::From<rustls::msgs::message::Message>>::from
  1,984 (-10.6%)  ???:<rustls::webpki::verify::WebPkiServerVerifier as rustls::verify::ServerCertVerifier>::supported_verify_schemes
   -825 ( 4.39%)  ./malloc/./malloc/malloc.c:malloc_consolidate
   -708 ( 3.77%)  ./malloc/./malloc/malloc.c:_int_free
   -608 ( 3.24%)  ./elf/../sysdeps/x86_64/dl-machine.h:_dl_relocate_object
    406 (-2.16%)  ./string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:__memcpy_avx_unaligned_erms
   -384 ( 2.04%)  ./malloc/./malloc/malloc.c:alloc_perturb
   -380 ( 2.02%)  ./elf/./elf/do-rel.h:_dl_relocate_object
   -192 ( 1.02%)  ???:rustls::msgs::message::MessagePayload::new
    138 (-0.73%)  ./string/../sysdeps/x86_64/multiarch/memcmp-avx2-movbe.S:__memcmp_avx2_movbe
   -126 ( 0.67%)  ???:rustls::tls12::prf::prf
     96 (-0.51%)  ???:rustls::webpki::verify::verify_signed_struct
    -80 ( 0.43%)  ???:rustls::webpki::verify::WebPkiServerVerifier::default_verify_tls12_signature
    -33 ( 0.18%)  /home/aochagavia/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/x86_64-mont-elf.S:bn_sqr8x_mont
     26 (-0.14%)  ???:rustls::msgs::handshake::HasServerExtensions::has_duplicate_extension

ctz · 2023-09-04T12:02:20Z

and supported_verify_schemes requires less instructions too

that's very interesting, because on main supported_verify_schemes is simpler. on main it is:

162     pub fn default_supported_verify_schemes() -> Vec<SignatureScheme> {
163         vec![
164             SignatureScheme::ECDSA_NISTP384_SHA384,
165             SignatureScheme::ECDSA_NISTP256_SHA256,
166             SignatureScheme::ED25519,
167             SignatureScheme::RSA_PSS_SHA512,
168             SignatureScheme::RSA_PSS_SHA384,
169             SignatureScheme::RSA_PSS_SHA256,
170             SignatureScheme::RSA_PKCS1_SHA512,
171             SignatureScheme::RSA_PKCS1_SHA384,
172             SignatureScheme::RSA_PKCS1_SHA256,
173         ]
174     }

on this branch it is:

520     fn supported_schemes(&self) -> Vec<SignatureScheme> {
521         self.mapping
522             .iter()
523             .map(|item| item.0)
524             .collect()
525     }

which feels like it should take more instructions. (there are some indirections, but ones i'm assuming will be inlined away in release mode.)

aochagavia · 2023-09-04T13:38:18Z

Oops, it looks like I didn't look carefully enough at the + and - signs in the report. supported_verify_schemes has in fact 10% more instructions.

ctz · 2023-09-04T15:30:15Z

Aha! That makes a lot more sense!

rustls/src/msgs/handshake_test.rs

djc · 2023-09-12T12:38:58Z

rustls/src/webpki/verify.rs

+    /// Used for verifying certificate chains.
+    ///
+    /// The order of this list is not significant.
+    pub all: &'static [&'static dyn SignatureVerificationAlgorithm],


Okay, but so SignatureVerificationAlgorithm is in pki-types now, so why do we need to wrap over this? Sorry to keep harping at this, something keeps nagging at me about this types which feels unidiomatic and I can't quite grasp why it's necessary.

rustls/src/webpki/verify.rs

djc · 2023-09-12T12:43:01Z

rustls/src/webpki/verify.rs

+    ///
+    /// The supported schemes in this mapping is communicated to the peer and the order is significant.
+    /// The first mapping is our highest preference.
+    pub mapping: &'static [(


Why don't we want to this to just be a top-level static?

not quite sure what you mean. it needs to be configurable rather than static so the WebPkiServerVerifier (etc.) can be reused without ring. there's a separate constructor of this type in ba26a6d#diff-8749c6ccb1619fb20bb8abc0a3651cc58eaf05ff79a0c395c7b9ef76047cea05R11-R17 backed by https://crates.io/crates/rsa if that makes the goal any clearer?

Okay, so this is really a way for the crypto provider to provide it's signature verification algorithms, and a mapping from SignatureScheme to them? And you used the WebPki prefix since this is specifically used for the webpki-backed verifiers?

correct, yes.

I don't know, it feels more like a crypto provider concern that happens to be used by the verifiers, not all that webpki-specific.

Any thoughts on a way forward on this?

I think I'm convinced that this a decent design. Should we be worried about the link-time inclusion of all signature verification algorithms by default?

I think that is the status quo. Though I guess this does now give people the opportunity to avoid that if desired.

rustls/src/msgs/message.rs

rustls/src/crypto/cipher.rs

provider-example/src/lib.rs

cpu

nb. since the above reviews the webpki-is-optional commit has been dropped, and there are two net-new refactoring commits to OpaqueMessage which would benefit from a look.

New changes look good to me 👍

tarcieri · 2023-09-13T14:39:06Z

FYI, the @RustCrypto project is going to try to put together a provider based on the example.

You can follow along at https://github.com/RustCrypto/rustls-rustcrypto (currently an empty repo)

djc · 2023-09-13T14:47:52Z

@tarcieri maybe collaborate with @stevefan1999-personal, they started on https://github.com/stevefan1999-personal/rustls-provider-rustcrypto already? Feel free to ask questions via issues or our Discord.

tarcieri · 2023-09-13T14:52:47Z

Wasn't aware @stevefan1999-personal already started on one but I think it would make sense for this provider to live under the @RustCrypto organization

stevefan1999-personal · 2023-09-13T15:09:37Z

@tarcieri Yep. This should be a community collaboration effort. And not only RustCrypto, we also have https://github.com/franziskuskiefer/evercrypt-rust/ too, which is a formally verified TLS library from F*, but ported to Rust.

tarcieri · 2023-09-13T15:10:53Z

@stevefan1999-personal would you want to upstream your current work to our repo? We can give you code review in the process.

Using the crate without this feature means something external needs to provide all the cryptography, and (eg) convenient integrated key loading APIs disappear.

…re used The prior arrangements are still available (and the default), if the crate is built with the *ring* feature. `WebPkiSupportedAlgorithms` is a new structure (designed for static construction, and direct use in webpki calls) that links `pki_types::SignatureVerificationAlgorithm`s to their corresponding TLS `SignatureScheme`. This replaces the hardcoded mappings in `fn convert_scheme` etc.

This removes a further need for `Payload` to be understood outside this crate. `payload()` allows immutable access as a slice, `payload_mut()` allows mutable access to the underlying vec (such as needed to decrypt the message without a copy).

This is an example that builds a mostly-unchanged rustls example (simpleclient), but only using crypto from the rust-crypto project and elsewhere. This is intended to be minimalistic, and not a complete replacement for *ring*. It implements: - TLS1.3 TLS13_CHACHA20_POLY1305_SHA256 cipher suite. - TLS1.2 TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 cipher suite. - X25519 key exchange. - RSA-PSS-SHA256 and RSA-PKCS1-SHA256 signature verification for verifying the server, integrated into the webpki crate. - random generation using `rand_core`. This means it can fetch www.rust-lang.org. TLS1.2 is not strictly necessary for this server, but serves to demonstrate that part of the API.

stevefan1999-personal · 2023-09-13T15:56:19Z

@stevefan1999-personal would you want to upstream your current work to our repo? We can give you code review in the process.

Sure thing! The server example should be good for review, but I will need to make some adjustment first. I will head back to it later tomorrow.

ctz changed the title ~~Jbp generalise crypto usage pt3~~ Make *ring* optional, and demonstrate how alternatives would be integrated Aug 17, 2023

ctz force-pushed the jbp-generalise-crypto-usage-pt3 branch 3 times, most recently from 6921995 to 2743ccc Compare August 24, 2023 11:48

ctz changed the base branch from main to jbp-generalise-crypto-usage-pt2 August 24, 2023 11:48

ctz force-pushed the jbp-generalise-crypto-usage-pt2 branch 2 times, most recently from 1458d75 to 05aa07b Compare August 25, 2023 13:54

Base automatically changed from jbp-generalise-crypto-usage-pt2 to main August 25, 2023 14:06

ctz force-pushed the jbp-generalise-crypto-usage-pt3 branch from 2743ccc to 09d71b4 Compare August 29, 2023 14:10

djc reviewed Aug 29, 2023

View reviewed changes

ctz force-pushed the jbp-generalise-crypto-usage-pt3 branch 4 times, most recently from 5b8a7e5 to ff29c8a Compare August 31, 2023 13:04

ctz marked this pull request as ready for review August 31, 2023 13:36

djc mentioned this pull request Aug 31, 2023

0.22 release planning #1435

Closed

15 tasks

ctz force-pushed the jbp-generalise-crypto-usage-pt3 branch from ff29c8a to 38a471d Compare September 1, 2023 12:10

stevefan1999-personal mentioned this pull request Sep 3, 2023

Crypto provider support rustls/tokio-rustls#15

Closed

ctz requested a review from cpu September 12, 2023 12:11

djc reviewed Sep 12, 2023

View reviewed changes

cpu approved these changes Sep 12, 2023

View reviewed changes

ctz force-pushed the jbp-generalise-crypto-usage-pt3 branch from 18737c7 to 937bb0c Compare September 12, 2023 16:43

djc approved these changes Sep 13, 2023

View reviewed changes

Move crate::sign to crate::crypto::signer

e7b1a36

ctz added 8 commits September 13, 2023 16:13

Move ring-backed signature keys to crypto::ring

eeee58b

Remove ring use in handshake_test.rs

db7aebc

Make *ring* an optional dependency

6ed9920

Using the crate without this feature means something external needs to provide all the cryptography, and (eg) convenient integrated key loading APIs disappear.

OpaqueMessage: allow construction without exposing Payload

52ff7a6

OpaqueMessage: privatize payload type

ea5fa26

This removes a further need for `Payload` to be understood outside this crate. `payload()` allows immutable access as a slice, `payload_mut()` allows mutable access to the underlying vec (such as needed to decrypt the message without a copy).

crypto::Iv::copy: allow use outside crate

e72e155

ctz force-pushed the jbp-generalise-crypto-usage-pt3 branch from 937bb0c to 78295cf Compare September 13, 2023 15:16

ctz added this pull request to the merge queue Sep 13, 2023

Merged via the queue into main with commit a1950e8 Sep 13, 2023
38 of 40 checks passed

ctz deleted the jbp-generalise-crypto-usage-pt3 branch September 13, 2023 15:40

djc mentioned this pull request Sep 14, 2023

Crypto provider feature guards are not additive #1472

Closed

stevefan1999-personal mentioned this pull request Sep 14, 2023

add an example tls server with provider (again) #1474

Closed

thomaseizinger mentioned this pull request Sep 25, 2023

feat(quic): support stateless resets libp2p/rust-libp2p#4554

Merged

4 tasks

japaric mentioned this pull request Oct 12, 2023

[3/3] no-std support phase I #1502

Merged

1 task

aochagavia mentioned this pull request Oct 24, 2023

v0.21.8 preparation #1525

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ring optional, and demonstrate how alternatives would be integrated #1405

Make ring optional, and demonstrate how alternatives would be integrated #1405

ctz commented Aug 17, 2023 •

edited

codecov bot commented Aug 17, 2023 •

edited

djc commented Aug 17, 2023 •

edited

djc left a comment

ctz commented Aug 31, 2023 •

edited

djc commented Aug 31, 2023

aochagavia commented Aug 31, 2023 •

edited

ctz commented Aug 31, 2023

aochagavia commented Aug 31, 2023

ctz commented Aug 31, 2023

aochagavia commented Sep 4, 2023 •

edited

ctz commented Sep 4, 2023

aochagavia commented Sep 4, 2023

ctz commented Sep 4, 2023

djc Sep 12, 2023

djc Sep 12, 2023

ctz Sep 12, 2023

djc Sep 12, 2023

ctz Sep 12, 2023

djc Sep 12, 2023

ctz Sep 13, 2023

djc Sep 13, 2023

ctz Sep 13, 2023

cpu left a comment

tarcieri commented Sep 13, 2023

djc commented Sep 13, 2023 •

edited

tarcieri commented Sep 13, 2023

stevefan1999-personal commented Sep 13, 2023

tarcieri commented Sep 13, 2023

stevefan1999-personal commented Sep 13, 2023 •

edited

Make *ring* optional, and demonstrate how alternatives would be integrated #1405

Make *ring* optional, and demonstrate how alternatives would be integrated #1405

Conversation

ctz commented Aug 17, 2023 • edited

codecov bot commented Aug 17, 2023 • edited

Codecov Report

djc commented Aug 17, 2023 • edited

djc left a comment

Choose a reason for hiding this comment

ctz commented Aug 31, 2023 • edited

djc commented Aug 31, 2023

aochagavia commented Aug 31, 2023 • edited

ctz commented Aug 31, 2023

aochagavia commented Aug 31, 2023

ctz commented Aug 31, 2023

aochagavia commented Sep 4, 2023 • edited

ctz commented Sep 4, 2023

aochagavia commented Sep 4, 2023

ctz commented Sep 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu left a comment

Choose a reason for hiding this comment

tarcieri commented Sep 13, 2023

djc commented Sep 13, 2023 • edited

tarcieri commented Sep 13, 2023

stevefan1999-personal commented Sep 13, 2023

tarcieri commented Sep 13, 2023

stevefan1999-personal commented Sep 13, 2023 • edited

Make ring optional, and demonstrate how alternatives would be integrated #1405

Make ring optional, and demonstrate how alternatives would be integrated #1405

ctz commented Aug 17, 2023 •

edited

codecov bot commented Aug 17, 2023 •

edited

djc commented Aug 17, 2023 •

edited

ctz commented Aug 31, 2023 •

edited

aochagavia commented Aug 31, 2023 •

edited

aochagavia commented Sep 4, 2023 •

edited

djc commented Sep 13, 2023 •

edited

stevefan1999-personal commented Sep 13, 2023 •

edited