Repository navigation
Update PQC Draft to Version 12 - #2355
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2355 +/- ##
=======================================
Coverage 85.46% 85.46%
=======================================
Files 126 126
Lines 22711 22732 +21
=======================================
+ Hits 19409 19428 +19
- Misses 3302 3304 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@ni4 3 checks fail due to the Botan version (3.6.0 required now). I suppose the images can easily be changed (or alternatively RFC95080/PQC disabled) in the corresponding yml files. I'm not familiar with your CI/CD setup, therefore I think someone else should do the necessary changes. |
121d8b8 to
e1af516
Compare
|
Hi Johannes, would it be easy for you to rebase your PR? Nickolay said the CI issues should be fixed, so maybe merging will be possible soon. If you don't have time, Nickolay said he could replicate your PR. |
... for rnp_generate_key_ex add roundtrip test for PQC certs clang-format
require Botan 3.6.0 for PQC switch to final NIST PQC standards update KMAC Key Combiner
fail gracefully on parsing v6 cleartext sigs
|
Hi Kai, since GitHub says there are no conflicts, it is easy for me, yes :) |
|
Great, thanks a lot! |
|
It seems like there are still some issues either with the CI or with my code. Unfortunately I don't have a lot of time right now to take care of this. FYI, I have set "Allow edits and access to secrets by maintainers" if Nickolay finds the time. |
|
I get a build warning when building against Botan 3.11.1 [edited: warning, not error] In file kyber_ecdh_composite.h a forward declaration is added "struct pgp_kyber_ecdh_composite_public_key_t;", |
|
The CI runs that uses the openssl backend fails because it is not yet available for pqc in this PR. The two CI runs that use Botan 3.3.0 fail because that workflow enables PQC, but that configuration requires at least Botan 3.6.0 The fuzzing build has the same issue, but it requires a patch for oss-fuzz. The issues in windows did NOT happen in the latest CI runs in my PR #2392. In the other CI runs for my recent pull requests, I see a new failure appearing, I would like to suggest that you add workflows that use Botan 3.11.1 or 3.12 |
ni4
left a comment
There was a problem hiding this comment.
While there are some CI failures they don't seem relevant and we may fix those later. LGTM!
…ak-hash binding) Audit-driven polish on top of PR #2355 to close the remaining RFC 9980 compliance gaps. Three changes, one story: make rnp's PQC implementation fully RFC 9980-conformant on the receiver and keygen sides. G1 — Preferred AEAD Ciphersuites for PQ keys (RFC 9980 §7.1) UserPrefs::check_defaults() now populates aead_prefs from symm_algs not only for v6 keys (the prior behaviour) but also for any key whose public-key algorithm is PQ. This ensures v4 ML-KEM-768+X25519 keys also advertise AES-256+OCB, satisfying the RFC's SHOULD for all PQ certs rather than just v6. G3 — Reject weak-hash subkey binding signatures over PQ subkeys (RFC 9980 §7.2) Key::validate_binding() now rejects MD5, SHA-1, and RIPEMD-160 in subkey binding signatures (type 0x18) when the subkey's algorithm is one of the PQ(/T) algorithms. The RFC says a receiving implementation MUST treat such signatures as invalid; rnp now does. G4 — Canonical algorithm name strings (RFC 9980 §2.1) rnp_keygen_alg_map in src/lib/keygen.cpp used hardcoded strings with underscore separators ("ML-KEM-768_X25519") and two outdated names that don't match RFC 9980 at all ("Kyber-X448" for alg ID 36 and "Dilithium-ED448" for alg ID 31). Now uses the RNP_ALGNAME_* macros from include/rnp/rnp.h, which already use the canonical RFC 9980 "+" form. This makes both internal maps consistent and matches the wire-spec naming. G2 (deferred): RFC 9980 §7.1 also specifies that a receiver should implicitly append AES-256 to a PQ recipient's preferences if it's missing. rnp does not have a recipient-pref-intersection code path today (callers choose the cipher via rnp_op_encrypt_set_cipher), so there's nothing to add AES-256 to. This item is moot until rnp grows recipient-driven cipher selection.
…follow-ups) The merged PRs #2355 (PQC draft 12 + v6 salt + Ed448/X448) and #2296 (Argon2 S2K + AEAD secret-key encryption) shipped with happy-path coverage only — import sample, verify, decrypt with correct password. This PR adds the missing negative coverage: - test_ffi_argon2_locked_seckey_wrong_password: wrong password must fail cleanly and not leave the key in a partial-unlock state; subsequent correct password still works after multiple failures. - test_ffi_decrypt_argon2_skesk_wrong_password: SKESK decryption fails without leaking plaintext when the derived AEAD key is wrong. - test_ffi_decrypt_pqc_pkesk_corrupted: a single byte flip near the end of a valid PQC-encrypted message must cause decryption to fail (exercises the AEAD tag verification path on the PQC PKESK). These close the "happy-path only" codecov gap for the AEAD-tag verification paths in stream-parse.cpp (PR #2422's earlier fix extends to PQC ciphertexts too) and for the Argon2 S2K derivation failure path in stream-key.cpp. Note: locally the build is blocked by a pre-existing Botan 3.12 API incompatibility in src/lib/crypto/ec.cpp (uses Botan::EC_Group members that became opaque in 3.12). CI runs against the Botan version pinned in the centos-and-fedora workflow (3.6 / 3.12 from source) where this is not an issue.
…ak-hash binding) Audit-driven polish on top of PR #2355 to close the remaining RFC 9980 compliance gaps. Three changes, one story: make rnp's PQC implementation fully RFC 9980-conformant on the receiver and keygen sides. G1 — Preferred AEAD Ciphersuites for PQ keys (RFC 9980 §7.1) UserPrefs::check_defaults() now populates aead_prefs from symm_algs not only for v6 keys (the prior behaviour) but also for any key whose public-key algorithm is PQ. This ensures v4 ML-KEM-768+X25519 keys also advertise AES-256+OCB, satisfying the RFC's SHOULD for all PQ certs rather than just v6. G3 — Reject weak-hash subkey binding signatures over PQ subkeys (RFC 9980 §7.2) Key::validate_binding() now rejects MD5, SHA-1, and RIPEMD-160 in subkey binding signatures (type 0x18) when the subkey's algorithm is one of the PQ(/T) algorithms. The RFC says a receiving implementation MUST treat such signatures as invalid; rnp now does. G4 — Canonical algorithm name strings (RFC 9980 §2.1) rnp_keygen_alg_map in src/lib/keygen.cpp used hardcoded strings with underscore separators ("ML-KEM-768_X25519") and two outdated names that don't match RFC 9980 at all ("Kyber-X448" for alg ID 36 and "Dilithium-ED448" for alg ID 31). Now uses the RNP_ALGNAME_* macros from include/rnp/rnp.h, which already use the canonical RFC 9980 "+" form. This makes both internal maps consistent and matches the wire-spec naming. G2 (deferred): RFC 9980 §7.1 also specifies that a receiver should implicitly append AES-256 to a PQ recipient's preferences if it's missing. rnp does not have a recipient-pref-intersection code path today (callers choose the cipher via rnp_op_encrypt_set_cipher), so there's nothing to add AES-256 to. This item is moot until rnp grows recipient-driven cipher selection.
…ak-hash binding) Audit-driven polish on top of PR #2355 to close the remaining RFC 9980 compliance gaps. Three changes, one story: make rnp's PQC implementation fully RFC 9980-conformant on the receiver and keygen sides. G1 — Preferred AEAD Ciphersuites for PQ keys (RFC 9980 §7.1) UserPrefs::check_defaults() now populates aead_prefs from symm_algs not only for v6 keys (the prior behaviour) but also for any key whose public-key algorithm is PQ. This ensures v4 ML-KEM-768+X25519 keys also advertise AES-256+OCB, satisfying the RFC's SHOULD for all PQ certs rather than just v6. G3 — Reject weak-hash subkey binding signatures over PQ subkeys (RFC 9980 §7.2) Key::validate_binding() now rejects MD5, SHA-1, and RIPEMD-160 in subkey binding signatures (type 0x18) when the subkey's algorithm is one of the PQ(/T) algorithms. The RFC says a receiving implementation MUST treat such signatures as invalid; rnp now does. G4 — Canonical algorithm name strings (RFC 9980 §2.1) rnp_keygen_alg_map in src/lib/keygen.cpp used hardcoded strings with underscore separators ("ML-KEM-768_X25519") and two outdated names that don't match RFC 9980 at all ("Kyber-X448" for alg ID 36 and "Dilithium-ED448" for alg ID 31). Now uses the RNP_ALGNAME_* macros from include/rnp/rnp.h, which already use the canonical RFC 9980 "+" form. This makes both internal maps consistent and matches the wire-spec naming. G2 (deferred): RFC 9980 §7.1 also specifies that a receiver should implicitly append AES-256 to a PQ recipient's preferences if it's missing. rnp does not have a recipient-pref-intersection code path today (callers choose the cipher via rnp_op_encrypt_set_cipher), so there's nothing to add AES-256 to. This item is moot until rnp grows recipient-driven cipher selection.
…follow-ups) The merged PRs #2355 (PQC draft 12 + v6 salt + Ed448/X448) and #2296 (Argon2 S2K + AEAD secret-key encryption) shipped with happy-path coverage only — import sample, verify, decrypt with correct password. This PR adds the missing negative coverage: - test_ffi_argon2_locked_seckey_wrong_password: wrong password must fail cleanly and not leave the key in a partial-unlock state; subsequent correct password still works after multiple failures. - test_ffi_decrypt_argon2_skesk_wrong_password: SKESK decryption fails without leaking plaintext when the derived AEAD key is wrong. - test_ffi_decrypt_pqc_pkesk_corrupted: a single byte flip near the end of a valid PQC-encrypted message must cause decryption to fail (exercises the AEAD tag verification path on the PQC PKESK). These close the "happy-path only" codecov gap for the AEAD-tag verification paths in stream-parse.cpp (PR #2422's earlier fix extends to PQC ciphertexts too) and for the Argon2 S2K derivation failure path in stream-key.cpp. Note: locally the build is blocked by a pre-existing Botan 3.12 API incompatibility in src/lib/crypto/ec.cpp (uses Botan::EC_Group members that became opaque in 3.12). CI runs against the Botan version pinned in the centos-and-fedora workflow (3.6 / 3.12 from source) where this is not an issue.
…ak-hash binding) Audit-driven polish on top of PR #2355 to close the remaining RFC 9980 compliance gaps. Three changes, one story: make rnp's PQC implementation fully RFC 9980-conformant on the receiver and keygen sides. G1 — Preferred AEAD Ciphersuites for PQ keys (RFC 9980 §7.1) UserPrefs::check_defaults() now populates aead_prefs from symm_algs not only for v6 keys (the prior behaviour) but also for any key whose public-key algorithm is PQ. This ensures v4 ML-KEM-768+X25519 keys also advertise AES-256+OCB, satisfying the RFC's SHOULD for all PQ certs rather than just v6. G3 — Reject weak-hash subkey binding signatures over PQ subkeys (RFC 9980 §7.2) Key::validate_binding() now rejects MD5, SHA-1, and RIPEMD-160 in subkey binding signatures (type 0x18) when the subkey's algorithm is one of the PQ(/T) algorithms. The RFC says a receiving implementation MUST treat such signatures as invalid; rnp now does. G4 — Canonical algorithm name strings (RFC 9980 §2.1) rnp_keygen_alg_map in src/lib/keygen.cpp used hardcoded strings with underscore separators ("ML-KEM-768_X25519") and two outdated names that don't match RFC 9980 at all ("Kyber-X448" for alg ID 36 and "Dilithium-ED448" for alg ID 31). Now uses the RNP_ALGNAME_* macros from include/rnp/rnp.h, which already use the canonical RFC 9980 "+" form. This makes both internal maps consistent and matches the wire-spec naming. G2 (deferred): RFC 9980 §7.1 also specifies that a receiver should implicitly append AES-256 to a PQ recipient's preferences if it's missing. rnp does not have a recipient-pref-intersection code path today (callers choose the cipher via rnp_op_encrypt_set_cipher), so there's nothing to add AES-256 to. This item is moot until rnp grows recipient-driven cipher selection.
…follow-ups) The merged PRs #2355 (PQC draft 12 + v6 salt + Ed448/X448) and #2296 (Argon2 S2K + AEAD secret-key encryption) shipped with happy-path coverage only — import sample, verify, decrypt with correct password. This PR adds the missing negative coverage: - test_ffi_argon2_locked_seckey_wrong_password: wrong password must fail cleanly and not leave the key in a partial-unlock state; subsequent correct password still works after multiple failures. - test_ffi_decrypt_argon2_skesk_wrong_password: SKESK decryption fails without leaking plaintext when the derived AEAD key is wrong. - test_ffi_decrypt_pqc_pkesk_corrupted: a single byte flip near the end of a valid PQC-encrypted message must cause decryption to fail (exercises the AEAD tag verification path on the PQC PKESK). These close the "happy-path only" codecov gap for the AEAD-tag verification paths in stream-parse.cpp (PR #2422's earlier fix extends to PQC ciphertexts too) and for the Argon2 S2K derivation failure path in stream-key.cpp. Note: locally the build is blocked by a pre-existing Botan 3.12 API incompatibility in src/lib/crypto/ec.cpp (uses Botan::EC_Group members that became opaque in 3.12). CI runs against the Botan version pinned in the centos-and-fedora workflow (3.6 / 3.12 from source) where this is not an issue.
…follow-ups) The merged PRs #2355 (PQC draft 12 + v6 salt + Ed448/X448) and #2296 (Argon2 S2K + AEAD secret-key encryption) shipped with happy-path coverage only — import sample, verify, decrypt with correct password. This PR adds the missing negative coverage: - test_ffi_argon2_locked_seckey_wrong_password: wrong password must fail cleanly and not leave the key in a partial-unlock state; subsequent correct password still works after multiple failures. - test_ffi_decrypt_argon2_skesk_wrong_password: SKESK decryption fails without leaking plaintext when the derived AEAD key is wrong. - test_ffi_decrypt_pqc_pkesk_corrupted: a single byte flip near the end of a valid PQC-encrypted message must cause decryption to fail (exercises the AEAD tag verification path on the PQC PKESK). These close the "happy-path only" codecov gap for the AEAD-tag verification paths in stream-parse.cpp (PR #2422's earlier fix extends to PQC ciphertexts too) and for the Argon2 S2K derivation failure path in stream-key.cpp. Note: locally the build is blocked by a pre-existing Botan 3.12 API incompatibility in src/lib/crypto/ec.cpp (uses Botan::EC_Group members that became opaque in 3.12). CI runs against the Botan version pinned in the centos-and-fedora workflow (3.6 / 3.12 from source) where this is not an issue.
…follow-ups) The merged PRs #2355 (PQC draft 12 + v6 salt + Ed448/X448) and #2296 (Argon2 S2K + AEAD secret-key encryption) shipped with happy-path coverage only — import sample, verify, decrypt with correct password. This PR adds the missing negative coverage: - test_ffi_argon2_locked_seckey_wrong_password: wrong password must fail cleanly and not leave the key in a partial-unlock state; subsequent correct password still works after multiple failures. - test_ffi_decrypt_argon2_skesk_wrong_password: SKESK decryption fails without leaking plaintext when the derived AEAD key is wrong. - test_ffi_decrypt_pqc_pkesk_corrupted: a single byte flip near the end of a valid PQC-encrypted message must cause decryption to fail (exercises the AEAD tag verification path on the PQC PKESK). These close the "happy-path only" codecov gap for the AEAD-tag verification paths in stream-parse.cpp (PR #2422's earlier fix extends to PQC ciphertexts too) and for the Argon2 S2K derivation failure path in stream-key.cpp. Note: locally the build is blocked by a pre-existing Botan 3.12 API incompatibility in src/lib/crypto/ec.cpp (uses Botan::EC_Group members that became opaque in 3.12). CI runs against the Botan version pinned in the centos-and-fedora workflow (3.6 / 3.12 from source) where this is not an issue.
…ak-hash binding) Audit-driven polish on top of PR #2355 to close the remaining RFC 9980 compliance gaps. Three changes, one story: make rnp's PQC implementation fully RFC 9980-conformant on the receiver and keygen sides. G1 — Preferred AEAD Ciphersuites for PQ keys (RFC 9980 §7.1) UserPrefs::check_defaults() now populates aead_prefs from symm_algs not only for v6 keys (the prior behaviour) but also for any key whose public-key algorithm is PQ. This ensures v4 ML-KEM-768+X25519 keys also advertise AES-256+OCB, satisfying the RFC's SHOULD for all PQ certs rather than just v6. G3 — Reject weak-hash subkey binding signatures over PQ subkeys (RFC 9980 §7.2) Key::validate_binding() now rejects MD5, SHA-1, and RIPEMD-160 in subkey binding signatures (type 0x18) when the subkey's algorithm is one of the PQ(/T) algorithms. The RFC says a receiving implementation MUST treat such signatures as invalid; rnp now does. G4 — Canonical algorithm name strings (RFC 9980 §2.1) rnp_keygen_alg_map in src/lib/keygen.cpp used hardcoded strings with underscore separators ("ML-KEM-768_X25519") and two outdated names that don't match RFC 9980 at all ("Kyber-X448" for alg ID 36 and "Dilithium-ED448" for alg ID 31). Now uses the RNP_ALGNAME_* macros from include/rnp/rnp.h, which already use the canonical RFC 9980 "+" form. This makes both internal maps consistent and matches the wire-spec naming. G2 (deferred): RFC 9980 §7.1 also specifies that a receiver should implicitly append AES-256 to a PQ recipient's preferences if it's missing. rnp does not have a recipient-pref-intersection code path today (callers choose the cipher via rnp_op_encrypt_set_cipher), so there's nothing to add AES-256 to. This item is moot until rnp grows recipient-driven cipher selection.
…follow-ups) The merged PRs #2355 (PQC draft 12 + v6 salt + Ed448/X448) and #2296 (Argon2 S2K + AEAD secret-key encryption) shipped with happy-path coverage only — import sample, verify, decrypt with correct password. This PR adds the missing negative coverage: - test_ffi_argon2_locked_seckey_wrong_password: wrong password must fail cleanly and not leave the key in a partial-unlock state; subsequent correct password still works after multiple failures. - test_ffi_decrypt_argon2_skesk_wrong_password: SKESK decryption fails without leaking plaintext when the derived AEAD key is wrong. - test_ffi_decrypt_pqc_pkesk_corrupted: a single byte flip near the end of a valid PQC-encrypted message must cause decryption to fail (exercises the AEAD tag verification path on the PQC PKESK). These close the "happy-path only" codecov gap for the AEAD-tag verification paths in stream-parse.cpp (PR #2422's earlier fix extends to PQC ciphertexts too) and for the Argon2 S2K derivation failure path in stream-key.cpp. Note: locally the build is blocked by a pre-existing Botan 3.12 API incompatibility in src/lib/crypto/ec.cpp (uses Botan::EC_Group members that became opaque in 3.12). CI runs against the Botan version pinned in the centos-and-fedora workflow (3.6 / 3.12 from source) where this is not an issue.
…ak-hash binding) Audit-driven polish on top of PR #2355 to close the remaining RFC 9980 compliance gaps. Three changes, one story: make rnp's PQC implementation fully RFC 9980-conformant on the receiver and keygen sides. G1 — Preferred AEAD Ciphersuites for PQ keys (RFC 9980 §7.1) UserPrefs::check_defaults() now populates aead_prefs from symm_algs not only for v6 keys (the prior behaviour) but also for any key whose public-key algorithm is PQ. This ensures v4 ML-KEM-768+X25519 keys also advertise AES-256+OCB, satisfying the RFC's SHOULD for all PQ certs rather than just v6. G3 — Reject weak-hash subkey binding signatures over PQ subkeys (RFC 9980 §7.2) Key::validate_binding() now rejects MD5, SHA-1, and RIPEMD-160 in subkey binding signatures (type 0x18) when the subkey's algorithm is one of the PQ(/T) algorithms. The RFC says a receiving implementation MUST treat such signatures as invalid; rnp now does. G4 — Canonical algorithm name strings (RFC 9980 §2.1) rnp_keygen_alg_map in src/lib/keygen.cpp used hardcoded strings with underscore separators ("ML-KEM-768_X25519") and two outdated names that don't match RFC 9980 at all ("Kyber-X448" for alg ID 36 and "Dilithium-ED448" for alg ID 31). Now uses the RNP_ALGNAME_* macros from include/rnp/rnp.h, which already use the canonical RFC 9980 "+" form. This makes both internal maps consistent and matches the wire-spec naming. G2 (deferred): RFC 9980 §7.1 also specifies that a receiver should implicitly append AES-256 to a PQ recipient's preferences if it's missing. rnp does not have a recipient-pref-intersection code path today (callers choose the cipher via rnp_op_encrypt_set_cipher), so there's nothing to add AES-256 to. This item is moot until rnp grows recipient-driven cipher selection.
This PR updates to the newest PQC draft version, and adds/fixes some RFC 9580 functionality. The PR replaces #2287. The PQC draft can be seen as stable now since it has passed Working Group Last Call recently.
The most prominent changes are:
V6 / RFC 9580
PQC
Further Code Changes
PQC code is not independent from Crypto Refresh / RFC9580 any more and thusThis is changed again in newer commits to allow MLKEM768+X25519 for v4 keys without compiling the crypto refresh code.ENABLE_CRYPTO_REFRESHis required forENABLE_PQCENABLE_CRYPTO_REFRESHandENABLE_PQCnow requires Botan 3.6.CRYPTO_REFRESH_ENABLEDis true.@ni4 since I had to rebase a lot and fixed some stuff only at the end of the rebasing, the history is not perfectly intact. Please tell me if you prefer to keep the commits anyway or whether I should squash them into a single commit. I hope I did not mess anything up when rebasing.
As next steps I would like to rebase the other PRs #2296 and #2207 (that is considerably less code than in this PR).