fix(xtest): correct X-Wing KAO size assertions for ASN.1 wrappedKey format#461
fix(xtest): correct X-Wing KAO size assertions for ASN.1 wrappedKey format#461sujankota wants to merge 1 commit into
Conversation
…ormat Signed-off-by: sujan kota <sujankota@gmail.com>
📝 WalkthroughWalkthroughThis PR updates test assertions in ChangesX-Wing KAO hybrid assertions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Code Review
This pull request updates the assert_xwing_kao_sizes function in xtest/test_pqc.py to reflect changes in the X-Wing Key Access Object (KAO) structure. The assertions now verify that the wrappedKey is larger than the raw ciphertext due to ASN.1 DER framing and that the ephemeralPublicKey is absent. Feedback suggests improving the clarity of the XWING_CIPHERTEXT_SIZE constant's definition and refining the error message for better descriptive consistency.
| assert wrapped_len > XWING_CIPHERTEXT_SIZE, ( | ||
| f"X-Wing wrappedKey should be > {XWING_CIPHERTEXT_SIZE} bytes, got {wrapped_len}" | ||
| ) |
There was a problem hiding this comment.
The constant XWING_CIPHERTEXT_SIZE is used here as a lower bound for the wrappedKey size. However, its definition on line 20 includes a comment stating it is the (wrappedKey) size, which is now misleading since the wrappedKey in the new ASN.1 format is significantly larger (~1190 bytes). Consider updating the comment on line 20 to clarify that it refers to the raw KEM ciphertext size. Additionally, the error message can be made more descriptive and consistent with other tests in this file (e.g., lines 221 and 279).
| assert wrapped_len > XWING_CIPHERTEXT_SIZE, ( | |
| f"X-Wing wrappedKey should be > {XWING_CIPHERTEXT_SIZE} bytes, got {wrapped_len}" | |
| ) | |
| assert wrapped_len > XWING_CIPHERTEXT_SIZE, ( | |
| f"X-Wing wrappedKey should be larger than raw ciphertext ({XWING_CIPHERTEXT_SIZE} bytes), got {wrapped_len}" | |
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
xtest/test_pqc.py (2)
275-285: ⚡ Quick winConsider adding ephemeralPublicKey assertion for consistency.
The
assert_xwing_kao_sizesfunction now validates thatephemeralPublicKey is Nonefor hybrid-wrapped KAOs. If secpmlkem KAOs follow the same hybrid-wrapped pattern, this test should also verifykao.ephemeralPublicKey is Noneto ensure structural consistency and catch potential regressions.🔍 Proposed consistency check
# Verify NIST curve compatible MLKEM hybrid sizes in the KAO and registered public key kao = manifest.encryptionInformation.keyAccess[0] wrapped_len = _b64_decoded_len(kao.wrappedKey) assert wrapped_len > XWING_CIPHERTEXT_SIZE, ( f"wrappedKey should be larger than {XWING_CIPHERTEXT_SIZE} bytes, got {wrapped_len}" ) +assert kao.ephemeralPublicKey is None, ( + "hybrid-wrapped secpmlkem-5 KAO should not have ephemeralPublicKey" +) pem = key_secpmlkem_5.key.public_key_ctx.pem🤖 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 `@xtest/test_pqc.py` around lines 275 - 285, The test verifies sizes for hybrid-wrapped KAOs but misses the structural check; add an assertion that the KAO's ephemeralPublicKey is None to mirror assert_xwing_kao_sizes behavior. Locate the block using variables kao and wrapped_len (and the surrounding secpmlkem key checks) and insert a simple assert kao.ephemeralPublicKey is None with an explanatory message to ensure secpmlkem hybrid-wrapped KAOs maintain the same ephemeralPublicKey discipline.
217-227: ⚡ Quick winConsider adding ephemeralPublicKey assertion for consistency.
The
assert_xwing_kao_sizesfunction now validates thatephemeralPublicKey is Nonefor hybrid-wrapped KAOs. If secpmlkem KAOs follow the same hybrid-wrapped pattern, this test should also verifykao.ephemeralPublicKey is Noneto ensure structural consistency and catch potential regressions.🔍 Proposed consistency check
# Verify NIST curve compatible MLKEM hybrid sizes in the KAO and registered public key kao = manifest.encryptionInformation.keyAccess[0] wrapped_len = _b64_decoded_len(kao.wrappedKey) assert wrapped_len > XWING_CIPHERTEXT_SIZE, ( f"wrappedKey should be larger than {XWING_CIPHERTEXT_SIZE} bytes, got {wrapped_len}" ) +assert kao.ephemeralPublicKey is None, ( + "hybrid-wrapped secpmlkem-3 KAO should not have ephemeralPublicKey" +) pem = key_secpmlkem_3.key.public_key_ctx.pem🤖 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 `@xtest/test_pqc.py` around lines 217 - 227, The test should assert that the hybrid-wrapped secpmlkem KAO has no ephemeral public key for consistency; in the test (inside assert_xwing_kao_sizes / the block using kao and key_secpmlkem_3) add an assertion that kao.ephemeralPublicKey is None (placed near the wrapped_len check) so the test verifies the same ephemeralPublicKey == None invariant as other hybrid-wrapped KAOs.
🤖 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.
Nitpick comments:
In `@xtest/test_pqc.py`:
- Around line 275-285: The test verifies sizes for hybrid-wrapped KAOs but
misses the structural check; add an assertion that the KAO's ephemeralPublicKey
is None to mirror assert_xwing_kao_sizes behavior. Locate the block using
variables kao and wrapped_len (and the surrounding secpmlkem key checks) and
insert a simple assert kao.ephemeralPublicKey is None with an explanatory
message to ensure secpmlkem hybrid-wrapped KAOs maintain the same
ephemeralPublicKey discipline.
- Around line 217-227: The test should assert that the hybrid-wrapped secpmlkem
KAO has no ephemeral public key for consistency; in the test (inside
assert_xwing_kao_sizes / the block using kao and key_secpmlkem_3) add an
assertion that kao.ephemeralPublicKey is None (placed near the wrapped_len
check) so the test verifies the same ephemeralPublicKey == None invariant as
other hybrid-wrapped KAOs.
2 participants
assert_xwing_kao_sizesintest_pqc.pyto match the actual ASN.1 DER wrappedKey formatTest plan
> XWING_CIPHERTEXT_SIZE)Summary by CodeRabbit