ffi: rnp_key_protect_ex() with Argon2 + AEAD, and PQC/Argon2 negative tests - #2431
Open
ronaldtse wants to merge 8 commits into
Open
ffi: rnp_key_protect_ex() with Argon2 + AEAD, and PQC/Argon2 negative tests#2431ronaldtse wants to merge 8 commits into
ronaldtse wants to merge 8 commits into
Conversation
3 tasks
ronaldtse
force-pushed
the
pqc-v6-argon2-tests
branch
from
August 10, 2026 02:09
db2a3a6 to
acd2667
Compare
ronaldtse
force-pushed
the
pqc-v6-argon2-tests
branch
from
August 20, 2026 10:11
acd2667 to
18dbead
Compare
ronaldtse
changed the title
ronaldtse
force-pushed
the
pqc-v6-argon2-tests
branch
2 times, most recently
from
August 22, 2026 05:38
1f50b28 to
09bf221
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2431 +/- ##
==========================================
- Coverage 85.46% 85.20% -0.26%
==========================================
Files 125 125
Lines 22962 23032 +70
==========================================
Hits 19625 19625
- Misses 3337 3407 +70 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ronaldtse
force-pushed
the
pqc-v6-argon2-tests
branch
3 times, most recently
from
August 23, 2026 10:43
09bf221 to
6147a5f
Compare
ni4
approved these changes
Aug 23, 2026
ni4
left a comment
ni4
left a comment
Contributor
There was a problem hiding this comment.
LGTM, but CodeQL complains about unused variables. @ronaldtse should we handle this, or it is false-positive?
ronaldtse
force-pushed
the
pqc-v6-argon2-tests
branch
from
August 29, 2026 01:43
4c7dc4a to
3e147cc
Compare
7 tasks
…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.
Adds a new public FFI function rnp_key_protect_ex() that lets callers opt into Argon2 S2K and AEAD encryption of secret-key material. The existing rnp_key_protect() function exposes only the legacy iterated-and-salted + CFB path; the Argon2 + AEAD path was reachable only via JSON keygen, which is awkward for non-JSON callers. New public types: - rnp_protection_params_t with fields for cipher, cipher_mode, hash, iterations, s2k_type, aead_alg, argon2_t/p/m_kib. Behaviour: - s2k_type = "Argon2" forces AEAD usage (per RFC 9580 crypto refresh) and requires ENABLE_CRYPTO_REFRESH at build time. Without it, the function returns RNP_ERROR_NOT_SUPPORTED. - aead_alg can be specified independently for non-Argon2 AEAD. - All string fields accept NULL for sensible defaults (mirrors rnp_key_protect()). - The Argon2 memory parameter is given in KiB and converted to the on-wire log2(bytes) representation. Test: test_ffi_key_protect_ex_argon2_roundtrip generates a fresh RSA key, protects it with explicit Argon2 parameters (small m/t/p for CI speed), unlocks with wrong (fails) and correct (succeeds) passwords, and confirms the key is still usable for signing after the round-trip. Standalone PQC keygen (ML-DSA, ML-KEM standalone; SLH-DSA 192f/192s/ 256f) is intentionally NOT in this PR. Adding it requires: - Allocating new PGP_PKA_* codepoints per draft-ietf-openpgp-pqc - Verifying wire-format details against the latest draft - Material parsing/serialisation in key_material.cpp - New fixtures from the draft test vector appendix The Botan backend already supports these parameter sets (SLHDSA192Small/Fast, SLHDSA256Fast); only the OpenPGP wire layer is missing. Tracked as a follow-up.
The Argon2 wrong-password test insertion dropped the
'#if defined(ENABLE_PQC) && defined(ENABLE_CRYPTO_REFRESH)' guard and
the TEST_F(rnp_tests, test_ffi_verify_v2_seipd_test_vector) declaration,
leaving an orphaned function body ('expected unqualified-id') and an
unbalanced #endif. Restore both, matching main, and re-guard the PQC
default-subkey / v6-key tests that sat outside their guards.
The test sat outside the #if defined(ENABLE_PQC) block, so it compiled (and failed importing the PQC secret key) on every CI leg where PQC is off by default. Also apply clang-format to two over-long call sites.
Three bugs in rnp_key_protect_ex and its test: - argon2_m_kib was converted as log2(bytes), but the on-wire field is log2 of the memory in KiB (Botan's Argon2 from_params unit), so 8 MiB became 8 GiB and the derivation took ~9s instead of milliseconds. - protect_ex called encrypt_secret_key directly, leaving the Key's raw packet (which unlock, export and store writes all read) stale -- for a freshly generated key it still held the unprotected packet, so rnp_key_unlock() succeeded with ANY password. Route through Key::write_sec_rawpkt like Key::protect() does. - the test used invented API names (rnp_op_generate_set_rsa_bits, RNP_KEY_SECRET_SUBKEY) and passed PUBLIC|SECRET to rnp_key_export, which the API rejects; use the real names and SECRET-only export.
Their only reads are on crypto-refresh-only paths; without the feature enabled CodeQL flags all four as unused variables.
ronaldtse
force-pushed
the
pqc-v6-argon2-tests
branch
from
August 30, 2026 20:37
3e147cc to
5c2372e
Compare
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.
Summary
Two related changes, combined for one review (formerly #2431 + #2432):
rnp_key_protect_ex()— protects a secret key with explicit S2K parameters, supporting Argon2 (crypto-refresh) as the KDF and AEAD encryption of protected key material.rnp_key_protect_ex()path (malformed Argon2 parameters, AEAD tampering).Test plan