Repository navigation
camel-crypto: resolve secureRandomName without needing a keystore - #29707
Conversation
The five reference-name setters of DigitalSignatureConfiguration look their
object up in the registry and apply it when the lookup found something. Four
of them guard on the value they just looked up:
KeyStore keystore = context.getRegistry().lookupByNameAndType(keystoreName, KeyStore.class);
if (keystore != null) {
setKeystore(keystore);
}
setSecureRandomName copied that guard but named its local `random`, so
`keystore` there is the *field*:
SecureRandom random = context.getRegistry().lookupByNameAndType(randomName, SecureRandom.class);
if (keystore != null) { // the field, unrelated to the lookup
setSecureRandom(random);
}
So secureRandomName only takes effect when a KeyStore happens to be set on
the configuration already - which signing or verifying with a plain key never
does - and the resolved SecureRandom is dropped on the floor. When a keystore
is set and the name resolves to nothing, the opposite happens and
setSecureRandom(null) wipes a SecureRandom that was configured directly.
The guard now tests `random`, like its four siblings.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. The one-character fix (keystore → random in the guard) is correct — setSecureRandomName was the only one of the five reference-name setters that guarded on the wrong variable, and the three tests cover exactly the three failure modes described in the PR description.
Pre-fix validation: static trace confirms all three tests would have been failing on main before the fix:
secureRandomNameShouldResolveWithoutAKeystore—keystore == nullin a keystore-less config, sosetSecureRandom(random)was never called →assertSamefails ✅secureRandomNameShouldResolveTheSameWayAsTheOtherReferenceNames— same path insetCamelContextre-runs →assertSamefails ✅anUnknownSecureRandomNameShouldNotClearTheConfiguredOne—random == nullafter a failed lookup butkeystore != nullguard would have passed and calledsetSecureRandom(null), wiping the directly configured instance →assertSamefails ✅
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: LGTM, a precise one-word fix with good tests.
setSecureRandomName was the only one of the five reference-name setters that guarded on the field keystore rather than the value it had just looked up. Guarding on random brings it in line with setKeystoreName/setPublicKeyName/setPrivateKeyName/setCertificateName, and since setCamelContext re-runs all five, the named SecureRandom now reaches SigningProcessor with or without a keystore.
One optional improvement: anUnknownSecureRandomNameShouldNotClearTheConfiguredOne passes on main as well, since no keystore is configured. If you set an empty keystore first (KeyStore ks = KeyStore.getInstance(KeyStore.getDefaultType()); ks.load(null, null); configuration.setKeystore(ks);), it would fail on main (the old guard calls setSecureRandom(null)) and pin the second half of the bug.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 10 of 723 tested, 27 compile-only — current: 10 all testedMaveniverse Scalpel detected 10 affected modules (current approach: 10). Skip-tests mode would test 10 modules (1 direct + 9 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (10)
Modules with tests skipped (27)
All tested modules (37 modules, 6m 35s total)Total reactor time: 6m 35s
Top 20 slowest modules:
|
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Confirmed the bug and the fix, and checked the sibling setters.
setSecureRandomName looked up the SecureRandom by name but then applied it under if (keystore != null) rather than if (random != null). Two consequences: a secureRandomName was silently ignored whenever no keystore was configured (the common case for a detached-signature route that only needs a custom RNG), and when a keystore was present but the name resolved to nothing it would push a null SecureRandom over whatever was already set. Guarding on random is the right fix.
I checked the other four reference-name setters to be sure this was the only one off: setKeystoreName guards on keystore, setPublicKeyName and setPrivateKeyName on their pk, and setCertificateName on certificate, each on the value it just looked up. setSecureRandomName was the only one keyed on a different variable, so this brings it in line with the rest.
The three tests pin the behavior well: resolution without any keystore, resolution deferred until setCamelContext re-applies the names (so the order the configurer sets options in does not matter), and an unknown name leaving an already-configured SecureRandom untouched rather than clearing it. Good.
Claude Code on behalf of oscerd
Problem
DigitalSignatureConfigurationhas five "reference name" options that look an object up in the registry and apply it if the lookup found one. Four of them guard on the value they just looked up:setPublicKeyName,setPrivateKeyNameandsetCertificateNameare the same shape, each guarding on its own local (pk,pk,certificate).setSecureRandomNamecopied that guard, but its local is calledrandom— sokeystorein the condition is the field, which has nothing to do with the lookup:Two consequences:
secureRandomNameis silently ignored unless aKeyStoreobject is already set on the configuration. Signing or verifying with a plainprivateKey/publicKey, which is what the component's own tests do, never sets one, so theSecureRandomthe user named is resolved and then dropped, and theSignatureis initialised without it.setSecureRandom(null)wipes aSecureRandomthat was configured directly throughsecureRandom.setCamelContextre-runs all five setters once the context is known, so the bug does not depend on the order the options are set in — it is not reachable at all without a keystore.Fix
Guard on
random, like the four siblings.Tests
New
DigitalSignatureConfigurationTest:secureRandomNameShouldResolveWithoutAKeystore— the namedSecureRandomis used when no keystore is configured. Fails onmain.secureRandomNameShouldResolveTheSameWayAsTheOtherReferenceNames— the name set before the context is known is resolved bysetCamelContext, as the other reference names are. Fails onmain.anUnknownSecureRandomNameShouldNotClearTheConfiguredOne— a name that resolves to nothing leaves a directly configuredSecureRandomalone. This is the second half of the bug; it passes either way without a keystore, and pins the behaviour.