Repository navigation
Conversation
randombit
requested review from
falko-strenzke and
reneme
and
a balanced review from Copilot
October 2, 2026 23:04
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The large cryptographic API requires human review, and its key-validation documentation currently overstates the implemented guarantees.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds an RFC 9180 HPKE implementation with dedicated key, suite, KEM, and context APIs.
Changes:
- Implements DHKEM, key scheduling, encryption, decryption, and secret export.
- Adds RFC vectors and behavioral tests.
- Adds API documentation and an executable example.
| File | Description |
|---|---|
src/lib/pubkey/hpke/info.txt |
Defines the HPKE module and dependencies. |
src/lib/pubkey/hpke/hpke.h |
Declares the public HPKE API. |
src/lib/pubkey/hpke/hpke.cpp |
Implements suites, keys, and contexts. |
src/lib/pubkey/hpke/hpke_kem.h |
Declares internal KEM operations. |
src/lib/pubkey/hpke/hpke_kem.cpp |
Implements RFC 9180 DHKEMs. |
src/tests/test_hpke.cpp |
Tests vectors, validation, and context behavior. |
src/tests/data/pubkey/hpke.vec |
Provides RFC 9180 known-answer vectors. |
src/examples/hpke.cpp |
Demonstrates base-mode HPKE usage. |
doc/api_ref/hpke.rst |
Documents the new API. |
doc/api_ref/contents.rst |
Adds HPKE to the API reference. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+159
to
+163
| HPKE keys are represented by dedicated types, unrelated by inheritance to | ||
| each other and to ``Botan::Public_Key``/``Botan::Private_Key``. A key is | ||
| validated for use with a specific KEM when it is created, so holding an | ||
| ``HPKE::Public_Key`` or ``HPKE::Private_Key`` guarantees the key is usable | ||
| for HPKE with its KEM. Both are cheaply copyable value types. |
Collaborator
There was a problem hiding this comment.
That claim from Copilot seems to be valid and still unaddressed.
Comment on lines
+269
to
+271
| * This is a value type (cheap to copy) wrapping an asymmetric key that | ||
| * has been validated for use with a particular HPKE KEM. It has no | ||
| * inheritance relationship with Botan::Public_Key or with HPKE::Private_Key. |
falko-strenzke
requested changes
Oct 6, 2026
falko-strenzke
left a comment
falko-strenzke
left a comment
Collaborator
There was a problem hiding this comment.
All in all looks good to me. Just some minor to medium points which should be easy to fix.
|
|
||
| /// Bytes added to each plaintext when sealed (the AEAD tag length) | ||
| /// See RFC 9180 Section 5.2 | ||
| size_t ciphertext_overhead() const { return m_aead.tag_length(); } |
Collaborator
There was a problem hiding this comment.
Header and RST doc should say that this one throws on an export-only suites.
| return std::vector<uint8_t>(s.begin(), s.end()); | ||
| } | ||
|
|
||
| class HPKE_KAT_Tests final : public Text_Based_Test { |
Collaborator
There was a problem hiding this comment.
AI review gave me a list of things not tested:
- Malformed inputs on the NIST curves: enc or a public key of wrong length, with a compressed or hybrid prefix, or a point off the curve. The only malformed-input tests are the low-order Montgomery points; check_public_encoding() and the Decoding_Error path of deserialize_public() are never exercised.
- Mismatched context inputs: no test opens a message with a different info, a different PSK or PSK identity, or a different sender identity key. The existing "wrong aad" test covers the AEAD's associated data, not the key schedule inputs, so the authentication guarantee of the PSK and Auth modes is only shown for matching inputs.
- Private_Key::deserialize with a wrong length or an out-of-range scalar on the NIST curves.
- is_known() returning true for a defined code point (only the GREASE case is tested).
I have skimmed through the tests and not spotted these, but I can't claim to fully substiante these points. Simplest will be to let AI generate these few test cases I guess.
Comment on lines
+159
to
+163
| HPKE keys are represented by dedicated types, unrelated by inheritance to | ||
| each other and to ``Botan::Public_Key``/``Botan::Private_Key``. A key is | ||
| validated for use with a specific KEM when it is created, so holding an | ||
| ``HPKE::Public_Key`` or ``HPKE::Private_Key`` guarantees the key is usable | ||
| for HPKE with its KEM. Both are cheaply copyable value types. |
Collaborator
There was a problem hiding this comment.
That claim from Copilot seems to be valid and still unaddressed.
|
|
||
| Both values must be non-empty, and (following the recommendation of | ||
| RFC 9180 section 9.5) the PSK must be at least 32 bytes; violating | ||
| either throws ``Invalid_Argument``. Note that the PSK mechanism is |
Collaborator
There was a problem hiding this comment.
Suggested change
| either throws ``Invalid_Argument``. Note that the PSK mechanism is | |
| either throws ``Invalid_Argument``. But longer values of Nh are recommended by RFC 9180 in section 7.2 depending on the KDF variant. Note that the PSK mechanism is |
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
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.
Probably the most unusual design decision here was that HPKE public and private keys are represented by dedicated HPKE types, rather than working directly with the normal PK hierarchy (there are of course conversions in both directions). This seems cleaner to me overall in that it avoids many redundant checks that the key is of the correct type, and also more directly supports HPKE's rules regarding key serialization, derivation, etc. In general the goal is that for each type in the RFC there is a 1:1 type in the C++ API that directly enforces the rules from the RFC.
HPKE doesn't specify any mandatory to implement schemes. This implementation requires ECDH and the NIST curves, leaving X25519/X448 optional, because making ECDH optional causes quite a mess of ifdefs. Also, somewhat, a product of my general anti-cofactor bias.
Pretty huge PR but no way to break it up, and over half of it is test data, so hopefully this is tractable to review.
There is a draft for ML-KEM support but things still seem to be in significant flux there so this is just plain EC with PQ left for it reaching a final RFC.