Rework KDF interface by ctz · Pull Request #1551 · rustls/rustls

ctz · 2023-10-24T16:20:42Z

Previously we implemented the TLS1.2 PRF and HKDF ourselves given something that impl rustls::crypto::hmac::Hmac. As outlined in #1540 (comment) this isn't the right approach for FIPS.

This puts both those dependencies behind new traits, referenced from the cipher suite types. The existing implementations we have are recast to be in terms of these traits, and continue to take an dyn hmac::Hmac. That means writing a crypto provider continues to be as easy as it was previously.

codecov · 2023-10-24T16:24:51Z

Codecov Report

Merging #1551 (1fd7697) into main (60420c5) will decrease coverage by 0.18%.
The diff coverage is 94.05%.

@@            Coverage Diff             @@
##             main    #1551      +/-   ##
==========================================
- Coverage   96.51%   96.34%   -0.18%     
==========================================
  Files          75       75              
  Lines       15355    15521     +166     
==========================================
+ Hits        14820    14953     +133     
- Misses        535      568      +33

Files	Coverage Δ
rustls/src/client/tls13.rs	`97.05% <100.00%> (-0.01%)`	⬇️
rustls/src/crypto/mod.rs	`100.00% <ø> (ø)`
rustls/src/crypto/ring/quic.rs	`99.68% <100.00%> (+<0.01%)`	⬆️
rustls/src/crypto/ring/tls12.rs	`97.75% <ø> (ø)`
rustls/src/crypto/ring/tls13.rs	`100.00% <100.00%> (ø)`
rustls/src/crypto/tls12.rs	`100.00% <100.00%> (ø)`
rustls/src/quic.rs	`77.74% <100.00%> (+1.09%)`	⬆️
rustls/src/server/tls13.rs	`96.97% <100.00%> (-0.01%)`	⬇️
rustls/src/tls12/mod.rs	`97.73% <100.00%> (-0.13%)`	⬇️
rustls/src/tls13/key_schedule.rs	`99.74% <100.00%> (+0.01%)`	⬆️
... and 2 more

... and 1 file with indirect coverage changes

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

ctz · 2023-10-25T11:38:16Z

 }

+#[test]
+fn test_tls13_exporter_maximum_output_length() {


nb. this test uncovers a panic against rel-0.21. Since nobody has reported it, and I think it's highly unlikely to be reachable except deliberately writing this code (in which case, why not write panic!() and save some effort) I don't think it's a pressing concern. However this test could be a backport candidate if we choose to fix that.

djc

Good stuff!

djc · 2023-10-25T12:14:22Z

    pub hmac_provider: &'static dyn crypto::hmac::Hmac,

+    /// How to complete HKDF with the suite's hash function.
+    pub hkdf_provider: &'static dyn crypto::tls13::Hkdf,


It looks like hmac_provider has only one user left after this. Is there a way to get rid of it here?

It would be a slight confusion of the tls13::Hkdf interface to add it there, but on the other hand it would be a much simpler result than requiring the whole of hmac::Hmac when only a tiny proportion of it is actually used. Hmm.

Have added a final commit which tries this. Seems a bit simpler, but I think it might need a bit of renaming of trait Hkdf to look sensible?

Yeah, I like the changes but can't think of a better name either -- I guess maybe something like HmacOperations or HashOps? Just leave it for now?

cpu

Nice work!

It comes at the expense of trait Hkdf having a weird bit bolted on. Maybe we rename it?

It doesn't feel too out of place to me, and I'm not sure I can think of a better name 🤷‍♂️

This replaces the HMAC trait in Tls12CipherSuite (there were no other uses of HMAC). Provide an implementation of the new PRF trait in terms of HMAC, for convenience of providers that have a HMAC (common) but not a separate TLS1.2 PRF (relatively uncommon). The *ring* and `provider-example/` providers use this.

Test data from https://mailarchive.ietf.org/arch/msg/tls/fzVCzk-z3FShgGJ6DOXqM1ydxms/

Have an impl of this for hmac::Hmac

This means `Hkdf` covers the entire use of TLS1.3 for HMAC/HKDF, and that avoids having to implement the HMAC traits just for this.

djc · 2023-10-26T09:20:20Z

Consistent but relatively small perf regression for handshake latency. @aochagavia should maybe get a run for wall clock benchmarks?

ctz · 2023-10-26T10:28:52Z

Consistent but relatively small perf regression for handshake latency

I think this was due to the extra Box allocations introduced in the additional indirection. I've pushed an additional commit (sorry!) that goes back to using the ring hkdf API; this turns the ~0.3% regression into a ~0.6% improvement compared to main.

djc

Awesome!

Now we no longer have a depedency on hmac, we can avoid that and save some heap allocations. This marginally improves TLS1.3 handshake performance.

aochagavia · 2023-10-26T13:18:11Z

Great to see the icount stuff is paying off :)

ctz force-pushed the jbp-kdf-interface branch from 27667ef to a5b4b8b Compare October 24, 2023 16:25

cpu reviewed Oct 24, 2023

View reviewed changes

ctz force-pushed the jbp-kdf-interface branch from a5b4b8b to 0e5a64f Compare October 25, 2023 10:53

ctz commented Oct 25, 2023

View reviewed changes

djc reviewed Oct 25, 2023

View reviewed changes

ctz force-pushed the jbp-kdf-interface branch 2 times, most recently from 7d4a0d8 to e8997f9 Compare October 25, 2023 16:37

cpu approved these changes Oct 25, 2023

View reviewed changes

ctz force-pushed the jbp-kdf-interface branch from 5ec056f to ed5b577 Compare October 26, 2023 09:07

ctz added 8 commits October 26, 2023 10:08

Move tls12::prf to crypto::tls12

73b9f20

Add test for TLS1.2 PRF using HMAC-SHA384

b63725b

Test data from https://mailarchive.ietf.org/arch/msg/tls/fzVCzk-z3FShgGJ6DOXqM1ydxms/

Put HKDF use behind trait

029bf28

Have an impl of this for hmac::Hmac

Delete unused hkdf.rs: migrate tests to crypto::tls13

2dc4548

Arrange for key exchange to be completed in Hkdf trait

fd18d3b

Test excess secret exporting, to address TODO

3e2c9cd

Bolt hmac_sign onto Hkdf trait

82f1ccc

This means `Hkdf` covers the entire use of TLS1.3 for HMAC/HKDF, and that avoids having to implement the HMAC traits just for this.

ctz force-pushed the jbp-kdf-interface branch from ed5b577 to 82f1ccc Compare October 26, 2023 09:08

djc approved these changes Oct 26, 2023

View reviewed changes

Comment thread rustls/src/crypto/ring/tls13.rs Outdated

Return to using ring hkdf API

1fd7697

Now we no longer have a depedency on hmac, we can avoid that and save some heap allocations. This marginally improves TLS1.3 handshake performance.

ctz force-pushed the jbp-kdf-interface branch from 57ade5b to 1fd7697 Compare October 26, 2023 11:03

ctz enabled auto-merge October 26, 2023 11:03

ctz added this pull request to the merge queue Oct 26, 2023

Merged via the queue into main with commit 0013a08 Oct 26, 2023

ctz deleted the jbp-kdf-interface branch October 26, 2023 11:19

Uh oh!

Conversation

ctz commented Oct 24, 2023

Uh oh!

codecov Bot commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ctz Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

ctz Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

ctz Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

djc Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

djc commented Oct 26, 2023

Uh oh!

ctz commented Oct 26, 2023

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aochagavia commented Oct 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Oct 24, 2023 •

edited

Loading

djc Oct 26, 2023 •

edited

Loading