gnodet-bot commented on code in PR #26725:
URL: https://github.com/apache/camel/pull/26725#discussion_r4091464336
##########
components/camel-crypto/src/main/java/org/apache/camel/converter/crypto/CryptoDataFormat.java:
##########
@@ -247,6 +293,31 @@ public byte[] getCalculatedMac() {
};
}
+ /**
+ * A fresh initialization vector, sized to the cipher's block length. Only
used when the vector is inlined into the
+ * message, so the reader takes it from the stream and nothing needs to be
shared out of band.
+ */
+ private byte[] generateInitializationVector() throws Exception {
+ byte[] iv = new byte[getCipherBlockSize()];
+ SECURE_RANDOM.nextBytes(iv);
+ return iv;
+ }
+
+ private int getCipherBlockSize() throws Exception {
+ int blockSize = cachedBlockSize;
+ if (blockSize == 0) {
+ Cipher cipher
+ = cryptoProvider == null ? Cipher.getInstance(algorithm) :
Cipher.getInstance(algorithm, cryptoProvider);
+ blockSize = cipher.getBlockSize();
+ if (blockSize <= 0) {
+ // A stream cipher reports no block size; 16 bytes is the
usual nonce length
+ blockSize = 16;
+ }
+ cachedBlockSize = blockSize;
Review Comment:
🔵 **`cachedBlockSize` goes stale if `setAlgorithm` or `setCryptoProvider` is
called after the first marshal.**
`setAlgorithm(String)` does not reset `cachedBlockSize`. If a route changes
the algorithm from `DES/CBC/PKCS5Padding` (block size 8) to
`AES/CBC/PKCS5Padding` (block size 16) after the first marshal, the cached
value stays 8 and subsequent marshals generate 8-byte IVs for AES →
`InvalidAlgorithmParameterException: wrong IV length`.
Davsclaus flagged this as 🔵. Fix: reset `cachedBlockSize = 0` in both
`setAlgorithm` and `setCryptoProvider` after the field assignment. The
lazy-init pattern here is correct otherwise — it just needs cache invalidation
on reconfiguration.
##########
components/camel-crypto/src/main/java/org/apache/camel/converter/crypto/CryptoDataFormat.java:
##########
@@ -124,6 +136,20 @@ private Cipher initializeCipher(int mode, Key key, byte[]
iv) throws Exception {
@Override
public void marshal(Exchange exchange, Object graph, OutputStream
outputStream) throws Exception {
byte[] iv = getInitializationVector(exchange);
+ if (iv == null && inline) {
+ if (algorithmParameterSpec != null) {
+ // initializeCipher gives algorithmParameterSpec precedence
over the IV, so a generated vector would be
+ // written into the message but never used - every message
would encrypt identically behind a vector
+ // that only looks per-message. Keep failing loudly, as this
configuration did before.
+ throw new IllegalStateException(
+ "Inlining cannot be performed when an
algorithmParameterSpec is configured, as the spec is"
+ + " used instead of the
initialization vector");
+ }
+ // The whole point of inlining is that the IV travels with the
message, so there is no reason to make
+ // the caller supply a fixed one - and requiring it is what used
to push users into reusing a single IV
+ // across every message.
+ iv = generateInitializationVector();
Review Comment:
🔴 **ECB + `inline` (davsclaus item 3) — still broken.**
For `AES/ECB/PKCS5Padding` + `inline`, no static IV, no
`algorithmParameterSpec`:
1. `algorithmParameterSpec != null` is false → falls through to
`generateInitializationVector()`
2. `getCipherBlockSize()` returns 16 (ECB block cipher has a block size)
3. A 16-byte IV is generated
4. `initializeCipher` calls `cipher.init(mode, key, new
IvParameterSpec(iv))` for ECB mode
5. JCE throws `InvalidAlgorithmParameterException: ECB mode cannot use IV`
Before the PR the message was `"Inlining cannot be performed, as no
initialization vector was specified"` — honest. The fix must detect IV-less
modes. ECB's `Cipher.getBlockSize()` returns 16, so block size alone cannot
distinguish it from CBC. The reliable probe: after `cipher.init(mode, key)`
with no IV, `cipher.getParameters()` returns null for ECB — that null means the
mode does not use an IV:
```suggestion
if (iv == null && inline) {
if (algorithmParameterSpec != null) {
// initializeCipher gives algorithmParameterSpec precedence
over the IV, so a generated vector would be
// written into the message but never used - every message
would encrypt identically behind a vector
// that only looks per-message. Keep failing loudly, as this
configuration did before.
throw new IllegalStateException(
"Inlining cannot be performed when an
algorithmParameterSpec is configured, as the spec is"
+ " used instead of the
initialization vector");
}
// Detect IV-less modes (e.g. ECB): after a no-IV init,
getParameters() is null.
Cipher probe = cryptoProvider == null
? Cipher.getInstance(algorithm)
: Cipher.getInstance(algorithm, cryptoProvider);
try {
probe.init(Cipher.ENCRYPT_MODE, key);
} catch (Exception ignored) {
// probe only; ignore failures here
}
if (probe.getParameters() == null) {
throw new IllegalStateException(
"Inlining cannot be performed with a mode that does
not use an initialization vector (e.g. ECB)."
+ " Use a mode that supports an IV (e.g.
AES/CBC/PKCS5Padding) or supply a static vector.");
}
// The whole point of inlining is that the IV travels with the
message, so there is no reason to make
// the caller supply a fixed one - and requiring it is what used
to push routes into reusing a single IV
// across every message.
iv = generateInitializationVector();
}
```
--
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]
