feat(core)!: conform hybrid PQ/T key formats to IETF drafts#3563
Open
dmihalcik-virtru wants to merge 12 commits into
Open
feat(core)!: conform hybrid PQ/T key formats to IETF drafts#3563dmihalcik-virtru wants to merge 12 commits into
dmihalcik-virtru wants to merge 12 commits into
Conversation
github-actions
Bot
added
comp:sdk
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request brings the hybrid post-quantum key wrapping schemes (X-Wing and NIST composite KEMs) into full compliance with the latest IETF drafts. By transitioning to standard ASN.1 envelopes and adopting the specified combiners, the implementation ensures interoperability with external PQ-aware tooling. The refactor introduces an OID-based dispatcher for key parsing and simplifies the public API by removing legacy parameters, while maintaining backward compatibility for the TDF-level DEK wrapping container. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. In lattice lands where secrets hide, / The quantum keys now align with pride. / From custom blocks to standard form, / The IETF drafts become the norm. Footnotes
|
|
Warning Review limit reached
More reviews will be available in 34 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR refactors hybrid post-quantum key wrapping formats (NIST EC+ML-KEM composites and X-Wing) to conform to IETF draft specifications. Changes include new ASN.1 serialization helpers, OID-based dispatcher routing, updated hybrid core implementations with SHA3-256 key derivation and byte-ordering adjustments, conformance tests, and cascading integration updates across SDK and service layers. ChangesHybrid PQ Key Format Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant HybridWrap as HybridNISTWrapDEK
participant ECDH
participant MLKEM
participant Combiner as SHA3-256
participant AES as AES-256-GCM
Client->>HybridWrap: DEK + hybrid SPKI public key
HybridWrap->>HybridWrap: parse SPKI -> split mlkemPK || ecPoint
HybridWrap->>ECDH: ECDH with ephemeral EC key / tradPK -> tradSS, tradCT
HybridWrap->>MLKEM: encapsulate mlkemPK -> mlkemSS, mlkemCT
HybridWrap->>Combiner: derive SHA3-256(mlkemSS||tradSS||tradCT||tradPK||Label)
Combiner-->>HybridWrap: wrapKey
HybridWrap->>AES: AES-GCM encrypt DEK with wrapKey
AES-->>HybridWrap: encryptedDEK (nonce||ciphertext||tag)
HybridWrap->>Client: HybridNISTWrappedKey ASN.1 (hybridCiphertext = mlkemCT||ephemeralECPub, encryptedDEK)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request aligns the hybrid post-quantum/traditional (PQ/T) key formats with the IETF drafts, specifically draft-ietf-lamps-pq-composite-kem-14 for NIST composite-KEM (P-256/P-384 + ML-KEM) and draft-connolly-cfrg-xwing-kem-10 for X-Wing. It replaces custom PEM block headers with standard PUBLIC KEY and PRIVATE KEY blocks wrapped in SPKI and PKCS#8 envelopes, routing decryption and encryption via AlgorithmIdentifier OIDs. Additionally, it updates the NIST hybrid combiner to use the draft-specified SHA3-256 scheme with domain-separation labels and identity binding, removing the previous HKDF-SHA256 concatenation. All tests, benchmarks, and external call sites have been updated to support these conformant formats. There are no review comments on this pull request, so no further feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/experimental/tdf/writer_test.go (1)
897-910:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the exact DEK in the hybrid unwrap helper.
assert.NotEmpty(dek)will still pass if the dispatcher returns the wrong bytes, so these new hybrid-path tests no longer prove unwrap correctness. Pass the expected DEK intohybridUnwrapForTestand compare it byte-for-byte.Suggested tightening
-func hybridUnwrapForTest(t *testing.T, ktype ocrypto.KeyType, privatePEM, wrappedKeyB64 string) { +func hybridUnwrapForTest(t *testing.T, ktype ocrypto.KeyType, privatePEM, wrappedKeyB64 string, expectedDEK []byte) { t.Helper() wrappedDER, err := ocrypto.Base64Decode([]byte(wrappedKeyB64)) require.NoError(t, err, "Base64Decode wrapped key") dec, err := ocrypto.FromPrivatePEM(privatePEM) require.NoError(t, err, "FromPrivatePEM") dek, err := dec.Decrypt(wrappedDER) require.NoError(t, err, "hybrid Decrypt") - assert.NotEmpty(t, dek, "%s recovered DEK", ktype) + assert.Equal(t, expectedDEK, dek, "%s recovered DEK", ktype) }Update the hybrid call sites to pass a copy of the writer DEK they expect to recover.
As per coding guidelines, "Run
go test ./...ormake testand ensure all existing tests pass; add tests for new functionality".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/experimental/tdf/writer_test.go` around lines 897 - 910, hybridUnwrapForTest currently only asserts dek is non-empty; change its signature (hybridUnwrapForTest) to accept an expectedDEK []byte and after Decrypt compare the recovered dek to expectedDEK byte-for-byte (use the existing test assertion helpers, e.g., require.Equal/ assert.Equal) with a helpful message; update all call sites to pass a copy of the writer DEK they expect to recover (not the original slice if it may be mutated) so the tests validate exact DEK equality.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ocrypto/hybrid_common.go`:
- Around line 8-20: HybridWrapDEK must reject non-hybrid key types before using
the dispatcher: in HybridWrapDEK (which calls FromPublicPEM and later
enc.Encrypt), first ensure the requested ktype is a hybrid kind (or explicitly
check that enc.Type() == Hybrid after FromPublicPEM) and return an error if not;
do not rely solely on enc.KeyType() matching ktype because FromPublicPEM may
return RSA/EC encryptors—add a guard that verifies ktype is hybrid (or asserts
enc.Type() == Hybrid) and only then call enc.Encrypt(dek).
In `@lib/ocrypto/hybrid_conformance_test.go`:
- Around line 45-123: Add explicit assertions that the PKCS#8 private-key
payload and the wrapped ciphertext follow the pinned concatenation layouts:
after generating a keypair with NewP256MLKEM768KeyPair (and the P384 analog),
call PrivateKeyInPemFormat/FromPrivatePEM and parse the PKCS#8 payload to assert
it is mlkemSeed || ECPrivateKey(DER) (use the MLKEM seed size constant like
P256MLKEM768MLKEMSeedSize and the EC private-key DER size constant) and likewise
after wrapping with FromPublicPEM/Encrypt, parse the wrapped blob to assert it
is mlkemCT || ephemeralECPub (use the MLKEM ciphertext size constant and the EC
public-key size constant) before calling Decrypt; add equivalent checks for the
P384/MLKEM1024/ephemeral curve symbols to ensure both private and wrapped
layouts are validated.
In `@lib/ocrypto/HYBRID_NIST_KEY_WRAPPING.md`:
- Around line 33-37: The fenced code blocks in this doc (e.g., the block
starting with "SubjectPublicKeyInfo { AlgorithmIdentifier ... }" and the other
fenced sections called out in the review) need explicit language tags to satisfy
markdownlint MD040; update each triple-backtick fence to include an appropriate
identifier (asn1, text, json, etc.) so the blocks render consistently and
lint-clean. Locate each fenced block (examples include the SubjectPublicKeyInfo
block and the other blocks referenced in the review) and add the correct
language token immediately after the opening ``` for that block.
In `@lib/ocrypto/pq_asn1.go`:
- Around line 52-64: The parseHybridSPKI function currently ignores
AlgorithmIdentifier.Parameters; update parseHybridSPKI to reject any SPKI whose
Algorithm.Parameters is present or non-empty (i.e., ensure
spki.Algorithm.Parameters is absent/zero-length) and return an error like "parse
SPKI: unexpected algorithm parameters" when parameters exist; apply the same
check to the corresponding PKCS#8 parser (the private-key parser handling
AlgorithmIdentifier.Parameters around the other block mentioned) so both
subjectPublicKeyInfo/parseHybridSPKI and the PKCS#8 parsing path enforce "OID
with no parameters" by validating Algorithm.Parameters before accepting the key
bytes.
In `@service/internal/security/standard_crypto.go`:
- Around line 523-529: The code decrypts with
ocrypto.FromPrivatePEM(key.hybridPrivateKeyPem) but never verifies that the
parsed private key actually implements the hybrid algorithm declared in
key.Algorithm; add a validation step after dec is obtained that compares the
parsed key's algorithm identifier to key.Algorithm (e.g. use dec.Algorithm /
dec.Scheme or call a provided Algorithm() / Type() accessor on the returned dec
object, or inspect its concrete params) and return an error if they differ, so
misconfigured keys are rejected up front instead of being used for decryption.
---
Outside diff comments:
In `@sdk/experimental/tdf/writer_test.go`:
- Around line 897-910: hybridUnwrapForTest currently only asserts dek is
non-empty; change its signature (hybridUnwrapForTest) to accept an expectedDEK
[]byte and after Decrypt compare the recovered dek to expectedDEK byte-for-byte
(use the existing test assertion helpers, e.g., require.Equal/ assert.Equal)
with a helpful message; update all call sites to pass a copy of the writer DEK
they expect to recover (not the original slice if it may be mutated) so the
tests validate exact DEK equality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4b1c244-8169-4928-8305-1253209a2dbe
📒 Files selected for processing (21)
lib/ocrypto/HYBRID_NIST_KEY_WRAPPING.mdlib/ocrypto/asym_decryption.golib/ocrypto/asym_encryption.golib/ocrypto/benchmark_test.golib/ocrypto/hybrid_common.golib/ocrypto/hybrid_conformance_test.golib/ocrypto/hybrid_nist.golib/ocrypto/hybrid_nist_test.golib/ocrypto/pem_blocks.golib/ocrypto/pq_asn1.golib/ocrypto/pq_asn1_test.golib/ocrypto/pq_oids.golib/ocrypto/xwing.golib/ocrypto/xwing_test.gosdk/experimental/tdf/key_access_test.gosdk/experimental/tdf/writer_test.gosdk/tdf_hybrid_test.gosdk/tdf_test.goservice/internal/security/basic_manager.goservice/internal/security/standard_crypto.gospec/DSPX-3396.md
Contributor
dmihalcik-virtru
added a commit
that referenced
this pull request
Jun 4, 2026
PR #3563 review surfaced gaps at the edges of the IETF-draft conformance work. Tightens dispatch, adds spec-anchored KATs, and corrects citations that had drifted between draft revisions. - Dispatcher: distinguish unknown-hybrid-OID from non-envelope so unrecognised hybrid OIDs no longer silently retry as RSA/EC; reject CERTIFICATE PEM blocks carrying a hybrid SPKI. - KAS decrypt sites cross-check the OID-routed decryptor's KeyType against keyDetails.Algorithm before trusting it. - Combiner asserts input lengths and is anchored byte-for-byte to the lamps-wg/draft-composite-kem reference KATs for both NIST hybrids. - HybridNISTDecryptor parses and validates the EC DER tail at construction, mirroring the encryptor's strictness. - Six-way cross-scheme dispatch rejection test (table-driven). - pq_asn1: reject non-absent AlgorithmIdentifier.Parameters per draft-14 §6 / draft-10 §5.8. - X-Wing: inline TODO(DSPX-TBD) noting the draft-05 primitive vs. draft-10 wire-format split until cloudflare/circl ships draft-10. - Doc/spec citation sweep across hybrid_nist.go, pq_oids.go, hybrid_common.go, HYBRID_NIST_KEY_WRAPPING.md; trim stale BenchmarkHybridSubOps references from BENCHMARK_REPORT.md. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
conformant hybrid pqt
Rewrite the spec body from the Jira ticket as the source of truth so tables, headings, and bullet lists render correctly. The original import had collapsed all table rows into single lines and dropped the section headings. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Brings the three hybrid PQ/T KEMs (X-Wing, P-256+ML-KEM-768, P-384+ML-KEM-1024) into interop with draft-ietf-lamps-pq-composite-kem-14 and draft-connolly-cfrg-xwing-kem-10 so we can honestly advertise the registered AlgorithmIdentifier OIDs. - Public/private keys now use standard `PUBLIC KEY` / `PRIVATE KEY` PEM blocks wrapped in SPKI / PKCS#8; the OID inside the AlgorithmIdentifier selects the scheme (no more custom block names). - NIST hybrids flip to draft-14 byte order: `mlkemPK || ecPoint` for public keys, `mlkemSeed || ECPrivateKey(RFC 5915 DER)` for private keys, `mlkemCT || ephemeralECPoint` for hybrid ciphertext. - NIST combiner is now `SHA3-256(mlkemSS || tradSS || tradCT || tradPK || Label)` per draft-14 §4.3 — the old HKDF-with-TDF-salt path is removed and `salt`/`info` parameters are dropped from the NIST public API. X-Wing's combiner is unchanged (delegated to circl). - `FromPublicPEM` / `FromPrivatePEM` dispatch hybrids by OID and fall through to the stdlib x509 path for RSA/EC. This is a wire-format break for hybrid keys. No on-disk material in the old format was deployed, so no migration tooling is needed. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
PR #3563 review surfaced gaps at the edges of the IETF-draft conformance work. Tightens dispatch, adds spec-anchored KATs, and corrects citations that had drifted between draft revisions. - Dispatcher: distinguish unknown-hybrid-OID from non-envelope so unrecognised hybrid OIDs no longer silently retry as RSA/EC; reject CERTIFICATE PEM blocks carrying a hybrid SPKI. - KAS decrypt sites cross-check the OID-routed decryptor's KeyType against keyDetails.Algorithm before trusting it. - Combiner asserts input lengths and is anchored byte-for-byte to the lamps-wg/draft-composite-kem reference KATs for both NIST hybrids. - HybridNISTDecryptor parses and validates the EC DER tail at construction, mirroring the encryptor's strictness. - Six-way cross-scheme dispatch rejection test (table-driven). - pq_asn1: reject non-absent AlgorithmIdentifier.Parameters per draft-14 §6 / draft-10 §5.8. - X-Wing: inline TODO(DSPX-TBD) noting the draft-05 primitive vs. draft-10 wire-format split until cloudflare/circl ships draft-10. - Doc/spec citation sweep across hybrid_nist.go, pq_oids.go, hybrid_common.go, HYBRID_NIST_KEY_WRAPPING.md; trim stale BenchmarkHybridSubOps references from BENCHMARK_REPORT.md. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-3396-conformant-hybrid-pqt
branch
from
b0e0034 to
6058b41
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/experimental/tdf/writer_test.go (1)
897-910:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen
hybridUnwrapForTestto assert DEK equality (not just non-empty).The helper only checks
assert.NotEmpty(t, dek, ...)afterdec.Decrypt(...). A regression that decrypts to the wrong (but non-empty) DEK would still pass; the hybrid wrap/unwrapped key material should be byte-for-byte compared (callers can pass the writer’s DEK).Suggested tightening
-func hybridUnwrapForTest(t *testing.T, ktype ocrypto.KeyType, privatePEM, wrappedKeyB64 string) { +func hybridUnwrapForTest(t *testing.T, ktype ocrypto.KeyType, privatePEM, wrappedKeyB64 string, expectedDEK []byte) { t.Helper() wrappedDER, err := ocrypto.Base64Decode([]byte(wrappedKeyB64)) require.NoError(t, err, "Base64Decode wrapped key") dec, err := ocrypto.FromPrivatePEM(privatePEM) require.NoError(t, err, "FromPrivatePEM") dek, err := dec.Decrypt(wrappedDER) require.NoError(t, err, "hybrid Decrypt") - assert.NotEmpty(t, dek, "%s recovered DEK", ktype) + assert.Equal(t, expectedDEK, dek, "%s recovered DEK", ktype) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/experimental/tdf/writer_test.go` around lines 897 - 910, hybridUnwrapForTest currently only asserts the decrypted DEK is non-empty; change it to accept an expected DEK parameter and assert byte-for-byte equality against the decrypted value. Specifically, update the function signature hybridUnwrapForTest(t *testing.T, ktype ocrypto.KeyType, privatePEM, wrappedKeyB64 string, expectedDEK []byte), keep Base64Decode and FromPrivatePEM/dec.Decrypt as-is, replace assert.NotEmpty(t, dek, ...) with assert.Equal(t, expectedDEK, dek, "%s recovered DEK matches expected", ktype), and update all callers to pass the writer’s original DEK into the new expectedDEK parameter.
♻️ Duplicate comments (1)
service/internal/security/standard_crypto.go (1)
509-553:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject mislabeled hybrid/X-Wing keys during load, not on first decrypt.
assertDecryptorAlgorithmcloses the hole at decrypt time, butloadKeystill buckets these PEMs underkeysByAlg[k.Algorithm]without validating the routedKeyType()up front. A misconfigured key can still be advertised under the wrong algorithm and only fail once traffic reachesDecrypt(). Parse and compare the private PEM inloadKeyso bad config is rejected at startup instead of surfacing as an interoperability break later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/internal/security/standard_crypto.go` around lines 509 - 553, loadKey currently buckets PEMs into keysByAlg without validating the routed decryptor type, so mislabelled hybrid/X-Wing keys only fail at first Decrypt; fix by parsing the private PEM in loadKey with ocrypto.FromPrivatePEM, cast to the interface{ KeyType() ocrypto.KeyType } (or simply call assertDecryptorAlgorithm(dec, key.Algorithm, kid)) and compare kt.KeyType() to key.Algorithm, and if mismatched return an error and refuse to load the key (ensuring keysByAlg only stores validated keys); apply this check for the hybrid/X-Wing branches before inserting into keysByAlg.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ocrypto/BENCHMARK_REPORT.md`:
- Around line 95-100: The benchmark text incorrectly claims "HKDF, AES-GCM, and
ASN.1 marshaling are all sub-microsecond" for "all hybrid schemes" even though
the NIST composite KEMs now follow the new combiner path (no
HKDF-with-TDF-salt), conflating X-Wing and NIST composites; update the paragraph
in BENCHMARK_REPORT.md to either (a) use scheme-neutral wording that refers to
"combine/hash operations" or "post-encapsulation hashing/combining" instead of
naming HKDF for all schemes, and call out that X-Wing uses HKDF-related work
while NIST composites follow the new combiner path, or (b) split the note into
two sentences that separately report X-Wing behavior (mention HKDF cost) and
NIST composite behavior (mention combiner/hash cost), preserving the per-scheme
latency comparisons (P-256 vs P-384 and ML-KEM-768/1024) and advising
re-introduction of per-op pprof breakdown if needed.
In `@sdk/tdf_test.go`:
- Around line 3103-3106: In the "hybrid-wrapped" fake KAS path in
sdk/tdf_test.go, after creating the decryptor with
ocrypto.FromPrivatePEM(kasPrivateKey) and before calling
dec.Decrypt(wrappedKey), assert the decryptor's KeyType() is present and equals
f.Algorithm (use f.s.Require().NotNil/Require().NotEmpty on dec.KeyType() and
f.s.Require().Equal to compare to f.Algorithm), and fail the test if the types
mismatch, mirroring the production checks in
basic_manager.go/standard_crypto.go.
In `@test/start-up-with-containers/action.yaml`:
- Around line 174-177: The if condition currently uses the raw input expression
"if: ${{ inputs.pqc-enabled }}" which treats any non-empty string (including
"false") as truthy; change the gate to explicitly compare the input string to
'true' (for example using "if: ${{ inputs.pqc-enabled == 'true' }}" or the
equals() expression) so the PQC step only runs when inputs.pqc-enabled is
exactly the string 'true'.
---
Outside diff comments:
In `@sdk/experimental/tdf/writer_test.go`:
- Around line 897-910: hybridUnwrapForTest currently only asserts the decrypted
DEK is non-empty; change it to accept an expected DEK parameter and assert
byte-for-byte equality against the decrypted value. Specifically, update the
function signature hybridUnwrapForTest(t *testing.T, ktype ocrypto.KeyType,
privatePEM, wrappedKeyB64 string, expectedDEK []byte), keep Base64Decode and
FromPrivatePEM/dec.Decrypt as-is, replace assert.NotEmpty(t, dek, ...) with
assert.Equal(t, expectedDEK, dek, "%s recovered DEK matches expected", ktype),
and update all callers to pass the writer’s original DEK into the new
expectedDEK parameter.
---
Duplicate comments:
In `@service/internal/security/standard_crypto.go`:
- Around line 509-553: loadKey currently buckets PEMs into keysByAlg without
validating the routed decryptor type, so mislabelled hybrid/X-Wing keys only
fail at first Decrypt; fix by parsing the private PEM in loadKey with
ocrypto.FromPrivatePEM, cast to the interface{ KeyType() ocrypto.KeyType } (or
simply call assertDecryptorAlgorithm(dec, key.Algorithm, kid)) and compare
kt.KeyType() to key.Algorithm, and if mismatched return an error and refuse to
load the key (ensuring keysByAlg only stores validated keys); apply this check
for the hybrid/X-Wing branches before inserting into keysByAlg.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 71c8b994-4efb-4b9f-bfc8-46912561e136
📒 Files selected for processing (24)
lib/ocrypto/BENCHMARK_REPORT.mdlib/ocrypto/HYBRID_NIST_KEY_WRAPPING.mdlib/ocrypto/asym_decryption.golib/ocrypto/asym_encryption.golib/ocrypto/benchmark_test.golib/ocrypto/hybrid_common.golib/ocrypto/hybrid_conformance_test.golib/ocrypto/hybrid_nist.golib/ocrypto/hybrid_nist_test.golib/ocrypto/pem_blocks.golib/ocrypto/pq_asn1.golib/ocrypto/pq_asn1_test.golib/ocrypto/pq_oids.golib/ocrypto/xwing.golib/ocrypto/xwing_test.gosdk/experimental/tdf/key_access_test.gosdk/experimental/tdf/writer_test.gosdk/tdf_hybrid_test.gosdk/tdf_test.goservice/internal/security/basic_manager.goservice/internal/security/standard_crypto.gospec/DSPX-3396.mdtest/start-additional-kas/action.yamltest/start-up-with-containers/action.yaml
Member
Author
|
Addressed the outside-diff writer test note in 2399243: Validation run locally:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Member
Author
|
Addressed the latest CodeRabbit review-summary items in 492a55d:
Validation:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
sujankota
previously approved these changes
Jun 5, 2026
The watch-sh-fix tag predates PQC support in init-temp-keys.sh, so kas-xwing-private.pem and related files were never generated. The pqc-enabled tag points to main HEAD which already runs `go run ./service/cmd/keygen` to produce the PQC key pairs needed by start-additional-kas when pqc-enabled is true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
sujankota
approved these changes
Jun 5, 2026
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
2 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
Brings the three hybrid PQ/T KEMs (X-Wing, P-256+ML-KEM-768, P-384+ML-KEM-1024) into interop with
draft-ietf-lamps-pq-composite-kem-14anddraft-connolly-cfrg-xwing-kem-10so we can honestly advertise the registered AlgorithmIdentifier OIDs.PUBLIC KEY/PRIVATE KEYblocks wrapped in SPKI / PKCS#8. The OID inside the AlgorithmIdentifier (1.3.6.1.5.5.7.6.59,1.3.6.1.5.5.7.6.63,1.3.6.1.4.1.62253.25722) selects the scheme; custom block names (SECP256R1 MLKEM768 PUBLIC KEY,XWING PUBLIC KEY, etc.) are gone.mlkemPK || ecPointfor public keys,mlkemSeed || ECPrivateKey(RFC 5915 DER)for private keys,mlkemCT || ephemeralECPointfor hybrid ciphertext.SHA3-256(mlkemSS || tradSS || tradCT || tradPK || Label)per draft-14 §4.3 — the old HKDF-with-TDF-salt path is removed andsalt/infoparameters are dropped from the NIST public API. X-Wing's combiner is unchanged (delegated to circl).FromPublicPEM/FromPrivatePEMroute hybrids by OID and fall through to the stdlib x509 path for RSA/EC. All*PrivateKeyFromPem/*UnwrapDEKper-scheme helpers are removed; call sites insdk/,service/, andotdfctl/migrated to the dispatcher.The
HybridNISTWrappedKey/XWingWrappedKeyASN.1 envelopes used for the TDF DEK wrap are unchanged — the IETF drafts cover only the KEM.Benchmark notes
The current benchmark suite measures end-to-end key generation, wrap, and unwrap
paths. It no longer includes the removed sub-operation benchmark, so percentages
such as "KEM encapsulation is 93-97% of wrap time" should not be reported from
these tables alone. Use
pprofor reintroduce a dedicated sub-operationbenchmark if a granular breakdown is needed.
The aggregate results still show the main performance shape clearly: RSA-2048
key generation and unwrap remain dominated by RSA private-key work; EC P-384 is
much slower than EC P-256; and the P384+ML-KEM-1024 hybrid is materially slower
than P256+ML-KEM-768 because it combines the larger curve and larger ML-KEM
parameter set. X-Wing and P256+ML-KEM-768 stay in the same broad latency range
for the end-to-end wrap path, while the NIST composites use the draft-14
SHA3-256 combiner instead of HKDF.
Test Plan
go test ./...inlib/ocrypto,sdk,service,otdfctlTestREADMECodeBlocksinsdkgolangci-lint run --new-from-rev=mainreports 0 new issues across all three modulesgofumpt -wapplied to all modified Go filesotdfctlwithhpqt:secp256r1-mlkem768andhpqt:xwingkeys🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation