Fix the build on modern Rust and stop advertising unimplementable AES-GCM suites - #8
Open
DanGould wants to merge 4 commits into
Open
Fix the build on modern Rust and stop advertising unimplementable AES-GCM suites#8DanGould wants to merge 4 commits into
DanGould wants to merge 4 commits into
Conversation
DanGould
force-pushed
the
fix-build-and-aes-gcm-advertisement
branch
from
August 19, 2026 10:58
e02c1bb to
50d7ce2
Compare
DanGould
force-pushed
the
fix-build-and-aes-gcm-advertisement
branch
from
September 7, 2026 11:45
50d7ce2 to
99f24f5
Compare
Run the rust-hpke backend on stable Rust and nothing else, so a red board means a real regression. The nss legs could never pass: NSS has no secp256k1 DHKEM, and the backend has not compiled since the Kem enum was pruned to K256Sha256 in bac8f2c. Whether to delete the backend is a separate decision; until then it stays out of the matrix. The 1.63.0 legs die during dependency resolution, before any code in this repository is compiled, because that cargo cannot parse modern dependency manifests; the MSRV leg returns once martinthomson#6 settles what the MSRV is. The NSS build prerequisites and clone steps go with the legs that used them, as does the llvm-tools-preview component nothing here consumes.
`PrivateKey`'s Debug impl still bound `b` from `key_data()` after the body was changed to print "[REDACTED]", leaving it unused. With `#![deny(warnings)]` that is a hard error on current rustc, so the crate does not build on the CI matrix's `stable` leg. `PublicKey`'s Debug still uses `b` and is unchanged.
The AES-GCM symmetric suites were advertised but could never be used. bitcoin-hpke removed its AES-GCM schemes in 0.13.0, and `dispatch_hpkes_new!` only ever mapped ChaCha20Poly1305, so a peer that honoured the advertisement and selected AES-GCM got `Err(InvalidKeyType)`. `Config::supported()` returned true for it anyway, and the test constants listed it first, which is why 11 of this crate's own tests failed. Correcting `supported()` makes `strip_unsupported` prune the advertised KeyConfig automatically. The `Aead` enum keeps its GCM variants so other peers' configs still parse. `decode` built its probe config with `Aes128Gcm` on the grounds that "the KDF and AEAD doesn't matter here"; it does now, since the probe is checked against `supported()`, so it uses ChaCha20Poly1305. `decode` also rejects a config whose suite list is empty after pruning, with `Error::Unsupported`. Without that, a client handed a GCM-only config reached `ClientRequest::from_config`, which selects `symmetric[0]`, and panicked on the empty list where `main` returned `InvalidKeyType`. `decode_list` already skips `Unsupported` entries, so a list containing such a config decodes to the usable remainder. A test covers both paths. The example server no longer requests AES-128-GCM, which it would only have had stripped. Two tests were stale from before the secp256k1 port and never passed: `derive_key_pair`'s expected config encoded a 32-byte X25519 key under KEM 0x0020, regenerated here for KEM 0x0016 with a 65-byte key; and `truncate_kdf_aead_list` hard-coded an offset that assumed the X25519 key size, so it now derives the offset from the encoding. Also fixes two lints current clippy rejects under this crate's `deny(warnings, clippy::pedantic)`: a redundant `continue` and non-inlined format args.
DanGould
force-pushed
the
fix-build-and-aes-gcm-advertisement
branch
from
September 7, 2026 12:03
99f24f5 to
57b6619
Compare
DanGould
marked this pull request as ready for review
September 7, 2026 12:15
benalleng
approved these changes
Sep 9, 2026
Member
Author
|
FTR I did test this locally including stacked onto rust-payjoin with a local bitcoin-hpke 0.20 release candidate built on payjoin/bitcoin-hpke#13 and made sure those tests continued to pass. I also have done differential and Wycheproof testing on that no. 13 pr. A review pass before un-drafting found that pruning the advertised suites could leave an empty list and panic in from_config on a GCM-only config, so decode now returns Unsupported there and a test covers both the pruned and empty cases. Security review after found nothing further. This is core dependency stuff, so it wouldn't be right to merge it untested. Seeking a touch more reassurance to understand the level of your qa confidence in this PR before merge @benalleng |
Collaborator
|
Ok, I will test in earnest today |
benalleng
approved these changes
Sep 10, 2026
benalleng
left a comment
benalleng
left a comment
Collaborator
There was a problem hiding this comment.
ReACK 57b6619
Tested by adding the rev = "57b66191de124d61440438450deb4320d4bf6290" from this branch into rust-payjoin and also built this branch with to confirm
cargo +1.63.0 build --no-default-features --features rust-hpke,server
and clippy with
cargo +1.63.0 clippy --no-default-features --features rust-hpke,server -- -D warnings
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
maindoes not build on current Rust, and once it does, 11 of thebitcoin-ohttp's 23 tests fail. CI had not run here which is how the fails survived. Three causes of failure get one commit each to fix + a CI change. Nothing here needs a newbitcoin-hpkerelease, so this can land now.ohttpcompile`[REDACTED]change in e275713 got fixedbitcoin-hpkedropts its GCM schemes in 0.13.0 and the dispatch macro only ever pointed to ChaCha20Poly1305, so that'd always throwInvalidKeyType. Supported now wasys so which is enough for it to get pruned from the advertised config.decodenow returnsUnsupportedfor an unusable config instead of letting aClientRequest::from_configget built and then throwing later.CI
1.63 was red because it needed dep resolution #6, nss was referencing an already-removed
Kemvariant from bac8f2c and can't work without a secp256k1 DHKEM in NSS. nss should be removed in its own follow up imo.Disclosure: co-authored by Claude Code.