Aias00 commented on PR #6531:
URL: https://github.com/apache/shenyu/pull/6531#issuecomment-5156775206
Reviewed #6531. No blockers. Two should-fix items and a few nits.
**Should fix**
1. **Static IV in CBC (security).** `AbstractCbcCryptorStrategy` reuses the
configured IV for every call (`buildCipher`, lines 62-71), and the documented
key format is `base64(secret):base64(iv)` — i.e. one fixed (key, IV) pair per
rule. Reusing a (key, IV) in CBC leaks first-block plaintext equality
(identical JSON prefixes → identical first ciphertext blocks across requests)
and opens chosen-plaintext paths. I know this matches the existing
`shenyu-common/AesUtils` posture, but for a *security* plugin it's worth not
silently propagating. At minimum, add a Javadoc warning that the IV must be
unique per deployment/rule; ideally add a random-IV-per-message variant that
prepends the IV to the ciphertext.
2. **No SPI-wiring regression test.** Both new tests instantiate the
strategy directly (`new AesStrategy()` / `new Sm4Strategy()`), so the META-INF
SPI registration that the plugin actually depends on
(`CryptorStrategyFactory.newInstance("aes")` → `ExtensionLoader.getJoin`) is
never exercised. If the `aes=`/`sm4=` line or the `@Join` annotation is
dropped, these tests still pass and the plugin fails at runtime (silently — the
factory catches and returns `null`). Please add a test that loads via
`CryptorStrategyFactory.newInstance("aes")`/`"sm4"` and round-trips.
**Nits**
3. Key-format divergence from `AesUtils` (raw UTF-8 string key/iv vs
`base64(secret):base64(iv)`) is unmentioned and will confuse operators using
the same secret across `shenyu.aes.secret.*` and the cryptor `aes` strategy. A
Javadoc cross-reference would help.
4. Negative tests cover separator/format errors only — no non-base64 content
or wrong byte-length (15-byte AES secret, 17-byte SM4 key) cases.
5. `AesStrategyTest`/`Sm4StrategyTest` vs the existing `RSAStrategyTest`
naming — trivial style divergence.
The crypto wiring itself (explicit BC provider via
`Cipher.getInstance(transformation, "BC")`, PKCS7Padding for AES/SM4, per-call
`Cipher`, standard Base64 output decoded by the factory's MIME decoder) is
correct, and the `@Join`/SPI file additions are right. The static-block
provider registration with a `getProvider` null-guard is actually cleaner than
`AesUtils`'s unconditional `addProvider` per call.
--
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]
