Implement RFC 9180 HPKE provider backed by aws-lc-rs #1963
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1963 +/- ##
==========================================
- Coverage 95.54% 93.94% -1.60%
==========================================
Files 86 95 +9
Lines 18728 19681 +953
==========================================
+ Hits 17893 18489 +596
- Misses 835 1192 +357 ☔ View full report in Codecov by Sentry. |
ctz
reviewed
May 21, 2024
cpu
commented
May 21, 2024
|
cpu
commented
May 22, 2024
cpu
commented
May 22, 2024
|
I’ve been interested in a HPKE implementation that is backed by aws-lc-rs. Will these structs and implementation be publicly accessible and usable even if I’m not interested in any other functionality in rustls? I’ve been using hpke-rs with their RustCrypto provider but would be interested in switching. |
Yes, they're in the public API and should work in isolation with the caveat that only base mode is offered. If you need more modes/functionality writing an aws-lc-rs backend for hpke-rs is probably a better option. |
|
Base mode is all I’m interested in so this sounds great. Thank you! |
TLS1.3 requires HKDF, and HPKE/ECH requires TLS1.3, so I think it's reasonable if HKDF lives in the |
cpu
force-pushed
the
cpu-aws-lc-rs-hpke
branch
2 times, most recently
from
2b1af46
to
cde04d3
Compare
I find this a reasonable line of thinking and don't feel strongly. Leaving things as-is works for me.
The wonderful folks upstream cut a aws-lc-rs 1.7.2 release that pulls in a better fix for the HKDF info limit we were bumping into. I reworked this branch to take that update and simplified the testing workarounds I was using before. |
I was able to knock out the one TODO I left myself to try and make the labelled expand step infallible. I think this is ready for a first review pass now. The solution I arrived at for the above notches up the code complexity enough that I kept it in a separate commit at the end in case I'm on the wrong track. I think this is the first time I've used const generics in earnest and so expect there might be further tweaks needed. |
ctz
approved these changes
May 24, 2024
Benchmark resultsInstruction countsSignificant differencesClick to expand
Other differencesClick to expand
Wall-timeSignificant differencesClick to expand
Other differencesClick to expand
Additional informationCheckout details:
|
|
Thank you both for the reviews! I will start working through the feedback. |
I believe I've addressed all the feedback. Thank you, the current state is a nice improvement over my first draft. @ctz I'm going to dismiss your review since there's been substantial change. If you could re- 🔍 I'd appreciate it.
Since it seems like everyone liked the const-ified version I squashed it down into the initial implementation commit.
|
cpu
commented
May 24, 2024
Done. I tried a few different approaches before settling on the one I pushed in 81ae6ae - I think it's the least invasive solution but would be open to alternatives if anyone has smarter ideas. Some notes about the new test:
Done: cpu-aws-lc-rs-hpke-client-ech - I haven't pushed an update to the PR branch yet (need to sort out BOGO w/ ring skipping ECH tests) but it works in the aws-lc-rs case (meaning we have HPKE interop with a 3rd Go based impl) and against some of the test servers (themselves using a 4th HPKE impl). Going to flip this out of draft status. I think it's ready to merge as soon as reviewers are happy. |
djc
approved these changes
May 30, 2024
|
Great work! |
cpu
force-pushed
the
cpu-aws-lc-rs-hpke
branch
2 times, most recently
from
6d1401c
to
d2a0b08
Compare
This will make it easier to drop the `HpkeProvider` trait. The only implementation, in the `provider-example` crate, is reworked to support the new trait fn. We can make this breaking change without worrying about semver because the `crypto::hpke` module is docs-hidden and has been considered an internal unstable API. This change also allows deriving `Debug` for the provider example `HpkeRs` struct.
Requiring this was a quirk of the hpke-rs backed impl in the provider crate. With the latest rework to store just the `HpkeSuite` this isn't required anymore. State isn't introduced until a `Sealer`/`Opener` are created. Similarly, our forthcoming `aws-lc-rs` HPKE impl requires no mutability for these trait fns. Like before we can make breaking changes here because the `crypto::hpke` module is docs-hidden.
It's useful to be able to check these for equality, and all of the field members are themselves `Eq` and `PartialEq`, making this easy to achieve.
With more experience we've discovered the `HpkeProvider` trait isn't required. We can instead get away with having a slice of supported `&dyn Hpke` instances. To facilitate finding a supported suite from that slice a newtype wrapper (`HpkeSuites`) is offered with a `supported()` fn to handle this task. The existing `provider-example` implementation and tests are updated accordingly.
Adds a `fips()` fn to the `Hpke` trait, and a default implementation that returns false.
The existing `Hkdf` trait is well suited to the TLS 1.3 use-case where all pseudo-random keys (PRKs) extracted using HKDF are only used for an as-is expansion. In this context the PRK is encapsulated in an `HkdfExpander`, and not accessible directly to callers (helping to avoid misuse). In other contexts, notably HPKE, we need to use an HKDF to expand two PRKs that are combined with some other metadata before being used for an expansion. This requires direct access to the PRK bytes, and so can't be done with the existing traits. This commit adds a new `HkdfPrkExpand` trait that uses the existing `Hkdf` trait as a super-trait. It adds one new fn, `extract_prk_from_secret`, that returns the raw PRK. We implement this trait for the `HkdfExpanderUsingHmac` type (and can reuse the code in the existing `Hkdf::extract_from_zero_ikm` and `Hkdf::extract_from_secret` fns that need a PRK. We can't implement this trait for the upstream Ring/aws-lc-rs `RingHkdf` types, as these use an opaque upstream `Prk` type tailored to the extract-and-then-expand usecase. Since a HMAC based HKDF is sufficient for the current needs and implementing the new trait requires no upstream changes we prefer this approach.
This allows users of aws-lc-rs to build HMAC HKDF constructions using the ring-like HMAC algorithms.
Now that we'll be using these HMAC algorithms from the aws-lc-rs HPKE implementation they should be made available to reference without needing the `tls12` feature enabled. Similarly the `HMAC_SHA512` algorithm should now be usable outside of test contexts.
This commit introduces a minimal RFC 9180 "Hybrid Public Key Encryption" (HPKE) implementation using crypto primitives (ECDH, HKDF, AEAD) from the `aws-lc-rs` crate. Our implementation is tailored towards the needs of Encrypted Client Hello (ECH) and so: 1. We only implement "base mode" HPKE. 2. We don't implement the secret export support. Care is taken to ensure the implementation is "linker friendly" - unreferenced algorithms will be discarded by the linker.
Now that we have an "in-house" implementation of the HPKE traits (via `crypto::aws_lc_rs::hpke` it feels like time to un-hide this module.
As an initial starting point, test that the aws-lc-rs HPKE suites from the main rustls crate and the hpke-rs HPKE suites from the provider-example crate can interoperate. This process is slower (~30s) so having the tests in a separate crate is helpful. It also allows us to take heavier deps (like the provider-example crate, hpke-rs and rust crypto).
This commit implements a better testing strategy using the RFC 9180 test vectors. Unlike the `rustls-provider-test` test, this version matches the produced `EncapsulatedSecret` and ciphertext values from a `Seal` operation against the expected values from the RFC. We don't do this in the integration test because it requires overriding the DH KEM so that it doesn't produce a randomly generated ephemeral private key during the `Encap` operation. This is obviously not something we want to support in normal usage and so requires some care. To achieve this we introduce a test-only constructor for the aws-lc-rs based `Sealer` that can pass through the pre-specified `skE` to a special test-only DH KEM `Encap` that will use it instead of generating one. These test only functions are not exported, and so we make use of them from a unit test in the same module. This results in some mild duplication of the test vector supporting code, but seems like the best trade-off. We continue to ignore the test vector `shared_secret`, `key_schedule_context`, `secret`, `key`, and `base_nonce` values: exposing these for test interrogation will require even more gross test-only contortions and if we've gotten these intermediate values wrong, the `enc` and `ct` values we _are_ matching would be off, or the unseal would break.
|
@ctz If you have a chance I'd love another review pass on this branch. I dismissed your +1 from earlier because of the changes that landed since. |
ctz
approved these changes
May 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to support Encrypted Client Hello (ECH) we need a Hybrid Public Key Encryption (HPKE) provider (see RFC 9180). Previously we landed the traits required to abstract over an HPKE provider, and a proof-of-concept provider that used hpke-rs and Rust Crypto. In order to support HPKE in the base Rustls crate we need an HPKE provider backed by either
aws-lc-rs, or*ring*.Since static ECDH is not supported by
*ring*(briansmith/ring#331), but is supported byaws-lc-rs(aws/aws-lc-rs#302) this branch implements an HPKE provider backed byaws-lc-rs. The existing test harness is moved to a newrustls-provider-testcrate and lightly updated for pairwise interop testing betweenaws-lc-rsand thehpke-rsbasedprovider-exampleimpl.Following the precedent set by the HPKE traits we landed, this is focused on only what is required for ECH. We don't implement any HPKE modes other than "base mode" (see RFC 9180 §5) , and don't offer export secrets.
The HPKE RFC requires some HKDF operations that previously weren't needed for TLS 1.3. Most notably, we need to be able to extract pseudo-random keys (PRKs) without immediately expanding them. The existing
Hkdftrait, and the upstream implementations from aws-lc-rs and*ring*both encapsulate the PRK to prevent misuse, and so can't be used for this purpose. We work around this by adding a newHkdfPrkExtracttrait that is a superset ofHkdf, offering a newextract_prk_from_secret()fn. We then implement this for the HMAC based HKDF impl from thetls13package. This means the aws-lc-rs impl is tied to the HMAC based HKDF, but this doesn't seem to have any disadvantages in practice and the non-HMAC options can't support this operation without upstream changes.Related to #1718