Skip to content

Session key size follows the encryption algorithm (AES-256 used a 128-bit key) - #401

Merged
phax merged 2 commits into
phax:masterfrom
Dinpo:fix/session-key-matches-algorithm
Sep 30, 2026
Merged

phax merged 2 commits into
phax:masterfrom
Dinpo:fix/session-key-matches-algorithm

Conversation

@Dinpo

@Dinpo Dinpo commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Problem

AS4CryptParams uses DEFAULT_SESSION_KEY_PROVIDER = INSTANCE_RANDOM_AES_128 regardless of the configured encryption algorithm. Selecting e.g. AES_256_GCM or AES_256_CBC via setAlgorithmCrypt(...) or a P-Mode therefore still produces a 128-bit session key, which is then used under the aes256-* algorithm URI. Nothing fails: the sender emits a message labelled AES-256 but encrypted with a 128-bit key, and a phase4 receiver decrypts it without complaint (JCE's AES accepts any valid key length). A stricter receiver may reject it; either way the effective strength is not what was configured.

We ran into this while implementing the ENTSOG-style profile of the Danish gas DSO (AES-256) with Phase4ENTSOGSender: a loopback test between two phase4 instances showed 128-bit keys going out under http://www.w3.org/2001/04/xmlenc#aes256-cbc. The workaround today is to remember to call setSessionKeyProvider(ICryptoSessionKeyProvider.INSTANCE_RANDOM_AES_256) whenever an AES-256 algorithm is chosen.

Change

  • ECryptoAlgorithmCrypt#getKeySizeBits(): the key size each algorithm requires (3DES 192, AES 128/192/256).
  • ICryptoSessionKeyProvider.createRandomFor(ECryptoAlgorithmCrypt): random keys of exactly that size, using plain JCE so it also works before WSS4J/XMLSec is initialised.
  • AS4CryptParams#getSessionKeyProvider(): if no provider was set explicitly, it now returns a provider that matches the configured algorithm. With no algorithm set it still returns DEFAULT_SESSION_KEY_PROVIDER, and an explicitly set provider always wins, so existing explicit configurations are unchanged. hasExplicitSessionKeyProvider() is added; cloneTo keeps the "derived" state.
  • AS4Encryptor: before encrypting, the session key size is checked against the algorithm and a mismatch fails with a clear IllegalStateException instead of silently producing an under-strength message.

For the default algorithm (AES_128_GCM, used by Peppol, eDelivery etc.) the behaviour is identical to today.

Tests

  • AS4CryptParamsTest: derived key size for every ECryptoAlgorithmCrypt, default without algorithm, explicit provider wins, clone keeps the derived provider.
  • AS4EncryptorTest (new): matching key accepted, 128-bit key for aes256-gcm rejected.
  • mvn -pl phase4-lib test: 167 tests, 0 failures. mvn -pl phase4-test test (end-to-end client/server incl. encryption): 205 tests, 0 failures, 9 skipped (existing @Ignore).

Happy to adjust naming or split the guard into a separate PR if you prefer.

🤖 Generated with Claude Code

AS4CryptParams used a 128-bit session key by default even when an AES-256
algorithm was configured, so messages labelled aes256-cbc/aes256-gcm were
encrypted with 128-bit keys. The default provider is now derived from the
configured algorithm (an explicitly set provider still wins), and
AS4Encryptor rejects a session key whose size does not match the algorithm.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
.setEncryptSymmetricSessionKey (m_bEncryptSymmetricSessionKey)
.setWSSecEncryptCustomizer (m_aWSSecEncryptCustomizer);
// Copy the field directly, so that "derived from the algorithm" survives the clone
aTarget.m_aSessionKeyProvider = m_aSessionKeyProvider;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I don't fully like - the "setSessionKeyProvider" should handle that case correctly

AES_192_GCM ("aes192-gcm", "2.16.840.1.101.3.4.1.26", WSS4JConstants.AES_192_GCM),
AES_256_CBC ("aes256-cbc", "2.16.840.1.101.3.4.1.42", WSS4JConstants.AES_256),
AES_256_GCM ("aes256-gcm", "2.16.840.1.101.3.4.1.46", WSS4JConstants.AES_256_GCM);
CRYPT_3DES ("3des", "1.2.840.113549.3.7", WSS4JConstants.TRIPLE_DES, 192),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if instead of provide an int nKeySizeBits if we are providing a Supplier<KeyGenerator> instead or on top of it? That would avoid the special handling for 3DES below

@phax

phax commented Sep 30, 2026

Copy link
Copy Markdown
Owner

Thanks for that - it looks good to me

- ECryptoAlgorithmCrypt gets a KeyGenerator supplier per constant and
  createKeyGenerator(), so createRandomFor needs no special case for 3DES
- AS4CryptParams.setSessionKeyProvider accepts null (derive from the
  encryption algorithm), so cloneTo goes through the setter again
- @SInCE 4.8.0

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Dinpo

Dinpo commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the quick review, and for merging #402 and #403. Both points are addressed in 4e9b9f7:

  • cloneTo: setSessionKeyProvider now accepts null, meaning "derive the session key from the encryption algorithm" (the default). cloneTo goes through the setter again, the direct field copy is gone.
  • Supplier<KeyGenerator>: each ECryptoAlgorithmCrypt constant now carries its own key generator supplier, exposed as createKeyGenerator (). ICryptoSessionKeyProvider.createRandomFor is reduced to eAlgorithmCrypt.createKeyGenerator ().generateKey (), so the 3DES special case is gone. I kept getKeySizeBits () on top of it, because AS4Encryptor uses it to reject a key of the wrong size from a custom provider.

Also changed the @since tags to 4.8.0 and added tests for the reset to null, the clone of an explicit provider, and that every algorithm's key generator creates a key of getKeySizeBits (). phase4-lib and phase4-test run green here.

@phax

phax commented Sep 30, 2026

Copy link
Copy Markdown
Owner

Thanks for the swift changes - I am awaiting feedback from somebody else, before merging - so please have some patience. Thanks

@Dinpo

Dinpo commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

No hurry, thanks. One note on the red CI run for 4e9b9f7: the only failure is AS4ClientUserMessageSoapFaultTest.testPermanentFaultStopsRetriesInLegacyMode:318 in phase4-test (expected:<0> but was:<1> on the retry counter). That test sends an unencrypted message and does not touch the code changed here, it passes on master, and the full phase4-lib + phase4-test run passed for me locally on the same commit, so it looks like a timing-dependent retry rather than something from this PR. A re-run of the job should tell; I cannot trigger it from the fork.

@phax

phax commented Sep 30, 2026

Copy link
Copy Markdown
Owner

Agree, the bug is timing related and has nothing to do with your changes. Will merge now

@phax
phax merged commit 22a2a85 into phax:master Sep 30, 2026
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants