[cryptor] support AES and SM4 encryption strategies

[im47cn]

Motivation

The cryptor plugin ships only RSA out of the box. Users needing symmetric ciphers (AES, SM4) have no built-in strategy today; SM4 is also required for Chinese national-standard (GM) compliance.

Modifications

• New AesStrategy and Sm4Strategy (@Join) implementing the CryptorStrategy SPI, registered as aes and sm4.
• Shared AbstractCbcCryptorStrategy encapsulates CBC wiring: key parsing, SecretKeySpec/IvParameterSpec init and BouncyCastle provider registration; subclasses only declare transformation + algorithm.
• Key convention base64(secret):base64(iv), fixed AES|SM4/CBC/PKCS7Padding.
• Added bcprov-jdk18on to the cryptor module.

No breaking changes: the CryptorStrategy interface, CryptorRuleHandler, admin and existing RSA rules are untouched.

Rule config example

strategyName=sm4, key=<base64-secret>:<base64-iv>

Tests

• Parameterized round-trip (ASCII/CJK/JSON) and invalid-key-format cases for both strategies.
• New strategy classes at 100% instruction coverage (Jacoco); existing RSA + plugin tests still pass (33 total).

[Aias00]

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.

[im47cn]

Thanks @Aias00 for the thorough review. All items addressed in the latest push:

Should-fix #1 (IV reuse Javadoc): Added security warning to AbstractCbcCryptorStrategy class-level Javadoc documenting that the IV is fixed per rule, the CBC reuse risk, and operator guidance to regenerate IVs per deployment/rule.

Should-fix #2 (SPI-wiring regression test): Added CryptorStrategyFactorySpiTest that loads strategies via CryptorStrategyFactory.newInstance("aes") / newInstance("sm4") — exercising the real ExtensionLoader.getJoin + META-INF SPI path, not direct instantiation. If the SPI file or @Join annotation is dropped, these tests fail.

Nit #3 (key format divergence): Added cross-reference in the same Javadoc pointing to AesUtils and warning against interchanging secrets.

Nit #4 (negative test coverage): Added 3 new negative tests: AES 15-byte key (wrong length), SM4 18-byte key (wrong length), and non-base64 key content.

[Aias00]

Strong test coverage (parameterized round-trip incl. CJK/JSON, wrong key-length, non-base64, SPI loadability). The SPI registration and the BouncyCastle provider guard are correct. One security design issue I think should be addressed before merge:

CBC with a fixed IV — IV reused across all messages. AbstractCbcCryptorStrategy.buildCipher parses the IV from the rule's key string (base64(secret):base64(iv)) and uses the same (key, IV) pair for every message encrypted under that rule. A gateway encrypts many requests/responses under one configured rule, so the same IV is reused per-message. CBC IV reuse leaks first-block plaintext equality and opens chosen-plaintext attack paths (CWE-329/CWE-1204) — and gateway request bodies are often partially attacker-controlled, which makes the CPA path reachable. The Javadoc acknowledges this ("Security note on IV reuse") and mentions a random-IV-per-message variant as a future enhancement, but I think it should be the default design rather than a TODO. Suggested fix: generate a fresh IV via SecureRandom per encrypt, prepend it to the ciphertext (base64(iv || ciphertext)), and parse it on decrypt — or switch to AES/GCM/NoPadding, which also gives authenticated encryption and resolves the malleability concern below.

CBC without authentication (malleable). AES/CBC/PKCS7Padding with no MAC means an attacker who can modify ciphertext in transit can flip bits to affect the decrypted plaintext (CBC bit-flipping). AES-GCM (above) would solve this; alternatively CBC+HMAC. The existing RSA strategy is also encryption-only, so this isn't a regression, but for new strategies it's worth flagging.

Minor: key material (base64(secret):base64(iv)) lives in the rule config and flows through admin/data-sync — consistent with RSA, so not a regression, but there's no KMS/vault integration for the new symmetric secrets. Also a small nit: encrypt uses Base64.getEncoder() (standard) while key/ciphertext parsing uses getMimeDecoder() (lenient, ignores line separators) — getDecoder() would be stricter and symmetric.

[im47cn]

Thanks @Aias00 — your points on IV reuse and missing authentication were spot on. Rather than documenting the risk, this push closes it. All three items addressed:

1. CBC IV reuse (CWE-329) → resolved by switching to GCM.
AES and SM4 now use AES/GCM/NoPadding / SM4/GCM/NoPadding with a fresh 96-bit SecureRandom nonce per message and a 128-bit tag. Output is base64(nonce ‖ ciphertext ‖ tag), so the nonce is unique per message — no fixed IV to reuse. The base64(secret):base64(iv) key form is gone (GCM must never reuse a fixed IV); the key is now just base64(secret), and the legacy secret:iv form is explicitly rejected with a clear error.

2. CBC malleability (CWE-1204) → resolved by GCM's authentication tag.
Any in-transit modification now fails decryption (AEADBadTagException) instead of silently producing corrupted plaintext. New tests assert both properties: shouldEmitDifferentCiphertextForRepeatedEncrypts (nonce freshness) and shouldFailAuthenticationWhenCiphertextIsTampered (integrity).

3. Base64 encoder/decoder asymmetry → fixed. Unified to getEncoder()/getDecoder() throughout; decrypt now decodes UTF-8 explicitly.

RSA — PKCS#1 v1.5 → OAEP, with a PKCS#1 fallback for your non-regression concern.
To address the padding-oracle weakness without breaking external peers that only speak PKCS#1 v1.5:

• The default rsa strategy is now RSA/ECB/OAEPWithSHA-256AndMGF1Padding, with an explicit OAEPParameterSpec (MGF1 also SHA-256) so the transformation is identical across JDKs/providers.
• A new rsa-pkcs1 strategy keeps PKCS#1 v1.5 — any rule interoperating with a legacy/external PKCS#1 system can opt in by strategy name, with no code change and no surprise breakage.

Shared RSA logic is factored into AbstractRsaStrategy; AbstractCbcCryptorStrategy is removed.

Note: RSA test fixtures moved from 512-bit to 2048-bit — OAEP/SHA-256 physically cannot encrypt any plaintext under a 512-bit key (keyBytes − 2·32 − 2 < 0), so the old fixture is no longer usable. AES/SM4 round-trip tests cover CJK + JSON payloads.

The full cryptor module suite (40 tests) is green locally (main project install + integrated-test test-compile both pass), including the SPI-wiring path you flagged earlier. Would appreciate another look when you have time.

[Aias00]

Review: add AES/SM4 symmetric ciphers + RSA OAEP hardening

Verdict: ✅ Approved (with two important notes the maintainer should consciously sign off on).

This is a high-quality, security-focused PR. The crypto design is sound and the tests are meaningful. Two points need addressing — both about communication/documentation, not code correctness — because the PR's own "no breaking changes" claim is inaccurate.

What's done well

• Secure AEAD design. AbstractAeadCryptorStrategy draws a fresh SecureRandom 96-bit nonce per message, prepends it to ciphertext||tag, Base64-wraps the lot (base64(nonce || ct || tag)). This is the correct pattern and defeats GCM IV-reuse (CWE-329).
• parseSecret rejects the legacy base64(secret):base64(iv) form for GCM (lines 209-216). Good defensive choice — it stops an operator from accidentally configuring a fixed IV, which would catastrophically leak the secret under GCM.
• Tamper detection is real and tested. GCM's auth tag makes the ciphertext non-malleable; the tests shouldFailAuthenticationWhenCiphertextIsTampered / shouldFailWhenCiphertextIsTampered flip a bit and expect failure. Same for OAEP (RSAStrategyTest.shouldFailWhenCiphertextIsTampered).
• RSA hardening. RsaStrategy now pins OAEPParameterSpec("SHA-256","MGF1",SHA256,PSpecified.DEFAULT), avoiding the JDK's default that pairs a SHA-256 label with a SHA-1 MGF1. OAEP is semantically secure vs the old PKCS#1 v1.5.
• Clean SPI refactor. AbstractRsaStrategy + RsaPkcs1Strategy share wiring; rsa-pkcs1 is provided as a compatibility shim. SPI file + @Join wiring is correct, and CryptorStrategyFactorySpiTest verifies resolution via the factory (not just direct instantiation).
• BouncyCastle dependency is version-managed (bcprov-jdk18on.version=1.84 in root pom dependencyManagement), so the unversioned <dependency> resolves and compiles. No build risk.

Note 1 — the default rsa strategy is now wire-incompatible (contradicts "no breaking changes")

The original RsaStrategy called Cipher.getInstance("RSA") → RSA/ECB/PKCS1Padding (PKCS#1 v1.5). This PR changes the rsa SPI name to OAEP (RSA/ECB/OAEPWithSHA-256AndMGF1Padding). That is a wire-format breaking change: any existing peer/client that encrypts with the old PKCS#1 v1.5 scheme can no longer interoperate with the upgraded gateway (and vice-versa).

The class javadoc correctly admits this ("Not wire-compatible with PKCS#1 v1.5") and the rsa-pkcs1 shim mitigates it — but the PR description still claims "No breaking changes: … existing RSA rules are untouched." The rule config is untouched, yet the on-the-wire format of the rsa strategy changed. This must be called out in the release notes, and the description's claim should be corrected. OAEP is the right default (strict security improvement) and a compatibility path exists, so this isn't a blocker — but it needs explicit maintainer acknowledgement, not a silent upgrade.

Note 2 — PR description is stale vs the implementation (CBC → GCM)

The "Modifications" section describes a different (earlier) design:

Shared AbstractCbcCryptorStrategy encapsulates CBC wiring … Key convention base64(secret):base64(iv), fixed AES|SM4/CBC/PKCS7Padding.

The shipped code is AbstractAeadCryptorStrategy using GCM (AES/GCM/NoPadding, SM4/GCM/NoPadding) with key base64(secret) (no IV; random per-message nonce). The GCM design in the code is actually better and safer than the CBC design the text describes — but a reviewer reading the description would be misled (and a fixed IV under CBC would itself be a security smell). Please update the description to match the actual GCM implementation before merge.

Minor / non-blocking

• Security.addProvider(new BouncyCastleProvider()) (static block, line 153-157) registers BC globally at the lowest preference. It's idempotent-guarded and Shenyu already uses BC elsewhere, so this is acceptable — just noting it's a global side effect.
• AbstractRsaStrategy relies on built-in SunJCE (Cipher.getInstance(transformation()) with no provider arg) for RSA, which is fine.

Bottom line

Code is correct, secure, and well-tested. Please (a) correct the "no breaking changes" claim and add a release note about the rsa PKCS#1 v1.5 → OAEP wire change (use rsa-pkcs1 to keep old peers working), and (b) update the description's CBC/base64(secret):base64(iv) text to the real GCM/base64(secret) design. After those, this is good to merge.

[Aias00]

hi, pls add my wechat: aias00

[Aias00]

@im47cn hi some security issue should be handled

[Aias00]

Heads-up (PMC Aias00): your PR's CI failures are in shared infrastructure checks (k8s-examples-http, e2e-case-logging-rocketmq, build-docker-images) that are currently failing across many unrelated PRs. This looks like a CI-environment issue, not a problem with your change. Please re-run the failed checks; the code change itself appears fine. No code changes needed for these specific failures.

[Aias00]

The existing rsa SPI name is changed from PKCS#1 v1.5 to OAEP while the default admin configuration still uses strategyName=rsa. This breaks wire compatibility for existing peers. Please preserve rsa as the legacy strategy and add a new rsa-oaep name, or provide an explicit compatible migration path.