Re: [PR] Add ML-KEM (FIPS 203) post-quantum key transpor t support [santuario-xml-security-java]

Arpan0995 commented on PR #652:
URL: https://github.com/apache/santuario-xml-security-java/pull/652#issuecomment-5374860629

   While looking further at the StAX side after the rebase, I noticed a coverage seam in the negative tests, including the two I just parameterized, and verified the actual inbound behavior locally on this branch.
   
   **The StAX negative tests do not exercise the StAX inbound handler.** `testMLKEMStaxWrongRecipientPrivateKeyFailsCleanly` and `testMLKEMStaxTruncatedEncapsulationRejected` both decrypt through the `decryptUsingDOM` helper, so the failure path they pin down is the DOM `XMLCipher` one (`decryptKey` throwing `XMLEncryptionException`). The new inbound branch in `XMLEncryptedKeyInputHandler#getGenericHybridSecret` is only reached by the happy-path `testMLKEMStaxEncryptStaxDecrypt`.
   
   **The two paths reject differently.** I ran both failure cases through the real inbound path (`InboundXMLSec#processInMessage`) on this branch:
   
   - Wrong recipient private key: no error at the key-transport layer. Decryption proceeds with the substituted random CEK from the fake-key fallback, and the failure surfaces later as an `XMLStreamException` whose cause chain ends in `AEADBadTagException: mac check in GCM failed`.
   - Truncated `CipherValue` (shorter than `encapsulationSize()`): same outcome. The length check in `KeyUtils#kemDecapsulate` does throw, but the surrounding `catch (Exception e)` in `getGenericHybridSecret` converts that into the fake random key too, so the structural error is also deferred to the GCM tag check.
   
   I assume the fake-key fallback is deliberate, since it mirrors the existing RSA key-transport behavior in the same handler and gives uniform timing. Two thoughts on it:
   
   1. It means the StAX path's rejection of a bad `EncryptedKey` relies on the content cipher being authenticated. With AES-GCM that holds. It may be worth a short code comment stating that assumption, since with a non-AEAD content algorithm rejection would depend on padding failures or on garbage plaintext failing XML parsing rather than on an authenticated integrity check.
   2. For the truncation case specifically, the fallback may be broader than intended: a ciphertext shorter than `encapsulationSize()` is structurally invalid rather than a wrong-key guess, and the DOM path rejects it cleanly with `XMLEncryptionException`. Rethrowing the `XMLSecurityException` from `kemDecapsulate` instead of falling through to the fake key would align the two paths there, without affecting the timing story for well-formed ciphertexts.
   
   Either way, it seems worth pinning the inbound behavior with negative tests so a future refactor cannot silently turn "wrong key" into "successful decrypt". Happy to contribute those in the same parameterized shape as the merged ones: wrong-key and truncated-ciphertext driven through `processInMessage`, asserting the failure.
   
   The signature split has the same seam (its StAX tests verify through the DOM engine rather than `processInMessage`), and driving the inbound path there surfaced an actual round-trip issue with the KeyValue key identifier; I have written that up separately on #651.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]