chovy-3012 commented on PR #12403:
URL: https://github.com/apache/seatunnel/pull/12403#issuecomment-5870091204
> Thanks for the very thorough turnaround, @chovy-3012, and thanks for the
ping. I re-read the complete current diff at head `d79b98c382b9` from scratch
(not just the incremental diff since my last review), and I want to say up
front: every one of the 10 points from my previous review has been genuinely
fixed, not just papered over, and each fix now comes with a dedicated
regression test that pins the exact behavior. That is a rare and excellent
level of rigor for a first pass. While tracing the new code end to end I did
find one new, real problem that the new changes introduced (a broken doc link
that is currently failing CI), plus one small non-blocking design note; neither
of these existed in the version I reviewed before, so there is nothing here I
should apologize for missing earlier.
>
> # What Problem Does This PR Solve?
> * User pain point: the Zeta SQL transform has no built-in AES function, so
users had to write a `ZetaUDF` or use the separate `FieldEncrypt` transform.
> * Fix approach: add `AES_ENCRYPT(value, key[, iv])` and
`AES_DECRYPT(value, key[, iv])` (AES/CBC/PKCS5Padding, Base64 output) as
built-in functions, registered in `ZetaSQLFunction`/`ZetaSQLType`, implemented
in `CryptoFunction`, documented in `docs/{en,zh}/transforms/sql-functions.md`
and `docs/{en,zh}/introduction/concepts/incompatible-changes.md`.
> * One-sentence summary: built-in, per-row AES-CBC encryption/decryption in
SQL, now with safe error messages, a bounded key-derivation cache, explicit
compatibility documentation, and both unit- and SQL/E2E-level test coverage.
>
> # 1. Code Change Review
> ## 1.1 Core Logic Analysis
> What changed since my last review (`fb7324064a12` -> `d79b98c382b9`):
>
> * `CryptoFunction.java` grew from 205 to 288 lines. Every exception path
now passes a fixed, non-sensitive placeholder (`"key"`, `"iv"`, `"value"`, an
argument count, or a byte length) as the `argument` parameter of
`CommonError.illegalArgument`, never the actual key/plaintext/ciphertext/IV.
> * A new `rejectNonScalar` helper rejects array/`byte[]`/`Map` values
before they reach `toString()`.
> * `resolveIv` now treats an explicitly-supplied `NULL` IV as an error
instead of silently falling back to a random IV.
> * A bounded `ConcurrentHashMap<String, SecretKeySpec> KEY_CACHE` (cap 64,
full-clear on overflow) now caches the derived key so a constant key is
hashed/decoded once instead of on every row.
> * A class-level Javadoc documents the wire format, the `base64:` vs.
passphrase key conventions, the KDF's weakness, and the unauthenticated-CBC
caveat.
> * New: `docs/{en,zh}/introduction/concepts/incompatible-changes.md` gets a
full entry describing the UDF-shadowing behavior and the migration path.
> * New: `SQLCryptoFunctionsTest.java` (150 lines) drives the functions
through `SQLTransform`/`ZetaSQLFunction` dispatch and `ZetaSQLType` inference,
not just direct `CryptoFunction` calls.
> * `CryptoFunctionTest.java` grew from 29 to ~40 tests, adding (among
others) `testErrorMessageDoesNotContainSecrets`,
`testInteropWithAesCbcEncryptor` (round trip against the real `AesCbcEncryptor`
in both directions), `testPassphraseKeyDerivationKnownAnswer` (independently
re-derives the SHA-256 key and AES ciphertext with raw `javax.crypto` and
asserts an exact match), `testEncryptByteArrayInputThrows`,
`testExplicitNullIvThrows`.
> * `func_string.conf` (E2E) now includes
`aes_decrypt(aes_encrypt(name,'mySecretPass'),'mySecretPass')` and the
explicit-IV variant, with matching sink assertions.
>
> I re-verified each of the 10 previously-raised issues against this head:
>
> # Previous issue Verified fixed? How
> 1 Secrets in exception messages Yes Every
`CommonError.illegalArgument` call now passes only a fixed label/count/length;
`testErrorMessageDoesNotContainSecrets` asserts this directly for 5 different
error paths.
> 2 UDF shadowing undocumented Yes `incompatible-changes.md`
(en+zh) now has a full entry with impact and migration guidance, matching
Option A from my last review.
> 3 Key-semantics/interop with `FieldEncrypt` undocumented and untested
Yes Class Javadoc + user docs spell it out;
`testInteropWithAesCbcEncryptor` proves both directions against the real
`AesCbcEncryptor`.
> 4 Unauthenticated CBC silent-garbage undocumented Yes Documented in
the class Javadoc and in both language docs ("Do not rely on a decryption error
to detect a wrong key").
> 5 No SQL-level/E2E test of registration+dispatch+typing Yes
`SQLCryptoFunctionsTest` exercises the real `ZetaSQLFunction`/`ZetaSQLType`
path; `func_string.conf` E2E now covers both the random-IV and explicit-IV
round trip.
> 6 `byte[]` silently encrypts array identity string Yes
`rejectNonScalar` throws for arrays/`byte[]`/`Map`;
`testEncryptByteArrayInputThrows` and `testEncryptArrayAndMapInputThrows` cover
it.
> 7 Per-row key derivation/cipher lookup (perf) Yes (with a caveat, see
1.3) `KEY_CACHE` caches the derived `SecretKeySpec`.
> 8 NULL IV silently switches ciphertext layout Yes `resolveIv` now
throws for an explicit `null` IV; `testExplicitNullIvThrows` covers both
encrypt and decrypt.
> 9 Docs used `CALL`, no full example, no security notes Yes Docs
now use bare-call style, include a full `SELECT ... FROM dual` example, and add
the CBC/determinism notes.
> 10 Error wording, missing class Javadoc, weak-assertion tests Yes
Decrypt failure message now says "decryption failed (wrong key, wrong IV, or
corrupted ciphertext)"; class Javadoc added;
`testPassphraseKeyDerivationKnownAnswer` replaces the old vacuous assertion
with an actual known-answer check.
> Runtime path (unchanged from before, re-verified against the current
source):
>
> ```
> SQL "select aes_encrypt(col,'key')" (JSqlParser, at open())
> -> ZetaSQLType.getExpressionType(function) : STRING
(ZetaSQLType.java:360-361)
> -> per row: ZetaSQLFunction.executeFunctionExpr : switch(name) ->
CryptoFunction.aesEncrypt (ZetaSQLFunction.java:540-543)
> default branch (ZetaSQLFunction.java:~684) is where user ZetaUDFs
are looked up -> never reached for AES_* names
> -> per row inside CryptoFunction: cachedKey (SHA-256 or Base64 decode,
now cached), Cipher.getInstance, SecureRandom IV, doFinal
> ```
>
> Key findings (this round):
>
> * **New real bug (not in the previous diff at all): a broken relative link
in the newly-added `incompatible-changes.md` entry currently fails CI.** Both
`docs/en/introduction/concepts/incompatible-changes.md` and
`docs/zh/introduction/concepts/incompatible-changes.md` link to `[SQL
Functions](transforms/sql-functions.md)`. That file lives at
`docs/{en,zh}/transforms/sql-functions.md`, but `incompatible-changes.md` lives
two directories deeper at `docs/{en,zh}/introduction/concepts/`. Every other
cross-directory link already in that file uses `../../` (e.g.
`docs/en/introduction/concepts/incompatible-changes.md:184` links to
`../../connectors/source/GoogleBigtable.md#...`), so this one needs
`../../transforms/sql-functions.md` in both languages. This is exactly what the
fork's "Run / Dead links" job caught: `transforms/sql-functions.md -> Status:
400` (marked as a dead link), `ERROR: 1 dead links found!` (see CI section
below for the exact log line). This must be fixed in this PR
. See Issue 1 in Section 1.4.
> * The `KEY_CACHE` fix for the old performance note (Issue 7) is a good,
simple improvement, but as written it is a single JVM-wide static map with no
notion of job/pipeline boundaries: derived key material for every job that has
ever called `AES_ENCRYPT`/`AES_DECRYPT` in this worker process stays resident
until either the process restarts or the 65th distinct key triggers a full
`clear()`. A workload that legitimately uses more than 64 distinct keys (e.g. a
per-tenant or per-row key column) will cause the cache to be cleared and
rebuilt on every insert past the cap, and — because the map is `static`, not
per-transform-instance — that churn is shared across unrelated concurrent jobs
in the same worker. Not a correctness bug (a cache miss just re-derives the key
correctly), but worth a look. See Issue 2 below (Low, non-blocking).
> * I could not find any other correctness, compatibility, or
resource-handling regression introduced by this round's changes. The
dispatch/typing wiring (`ZetaSQLFunction.java:540-543`,
`ZetaSQLType.java:360-361`) is byte-for-byte identical to what I reviewed
before.
>
> ## 1.2 Compatibility Impact
> **Partially incompatible**, same as before, and now properly documented.
Jobs that do not use these function names are unaffected; no config option,
default, or serialization format changes. The UDF-shadowing behavior is real
and is now explicitly called out in `incompatible-changes.md` with a migration
guide, which is exactly what was missing last round.
>
> ## 1.3 Performance / Side-Effect Analysis
> * The key-derivation caching addresses the old per-row
`SHA-256`/Base64-decode/`Cipher.getInstance` concern for the common case of a
constant key.
> * New note: the static, process-wide, full-clear-on-overflow cache (see
Key Findings above) trades one perf problem for a smaller one. I would not
block on this, but a per-`SQLTransform`-instance cache (or a simple LRU instead
of a full clear) would remove the cross-job interference entirely and also
shrink the residency window of derived key material. Non-blocking (Issue 2,
Low).
> * No I/O, threads, or locks are introduced; `SecureRandom` usage is
unchanged and fine.
>
> ## 1.4 Error Handling and Logging
> **Issue 1: Broken relative link in `incompatible-changes.md` fails the
"Dead links" CI job**
>
> * Location: `docs/en/introduction/concepts/incompatible-changes.md` and
`docs/zh/introduction/concepts/incompatible-changes.md`, the new "Zeta SQL
Transform: built-in AES_ENCRYPT / AES_DECRYPT" section - both language versions
link to `transforms/sql-functions.md` using the same relative path form.
> * Problem: the link is relative to `docs/{en,zh}/introduction/concepts/`,
where `transforms/sql-functions.md` does not exist; the real target is two
levels up. Every other existing cross-directory link in this same file already
uses the `../../` form.
> * Risk: this is not a hypothetical - it is the actual cause of the current
CI failure. The fork's "Run / Dead links" job output is: `FILE:
./docs/en/introduction/concepts/incompatible-changes.md` /
`transforms/sql-functions.md` (marked as a dead link) /
`transforms/sql-functions.md -> Status: 400` (marked as a dead link) / `ERROR:
1 dead links found!`. (The `zh` file has the identical mistake but is not
covered by this particular lint job, so it doesn't show up as a separate CI
failure - please fix both for correctness.)
> * Best improvement: change both links to
`../../transforms/sql-functions.md`.
> * Severity: High (this is a required, currently-red CI check, directly
caused by this PR's new content, and the fix is a one-line-per-file change).
> * Raised by another reviewer: No.
>
> **Issue 2: Static, unbounded-lifetime `KEY_CACHE` with a "clear
everything" eviction policy**
>
> * Location: `CryptoFunction.java` (`KEY_CACHE`, `cachedKey`).
> * Problem: see Key Findings / 1.3 above - the cache is process-wide, not
scoped to a job or transform instance, and a full `clear()` fires whenever more
than 64 distinct keys have been seen, which can thrash under a
per-row/per-tenant key workload and affects unrelated concurrent jobs sharing
the worker JVM.
> * Potential risk: reduced or negated caching benefit under multi-key
workloads, and derived key material sitting in static memory for longer than
any single job's lifetime.
> * Best improvement: scope the cache per `SQLTransform` instance (it is
already effectively singleton per compiled query), or swap the full-clear for a
bounded LRU (e.g. `LinkedHashMap` with `removeEldestEntry`, guarded by a lock,
or a small `Caffeine`-style cache if that dependency is already available). Not
required for this PR.
> * Severity: Low
> * Raised by another reviewer: No
>
> No other issues from the previous review remain open; see the table in 1.1.
>
> # 2. Code Quality Assessment
> ## 2.1 Coding Standards
> * ASF license headers present on all new/modified files, no wildcard
imports, `spotless` conventions followed.
> * `CryptoFunction` now has a proper class-level Javadoc in addition to the
existing method Javadocs, closing the old gap. `KEY_CACHE`,
`KEY_CACHE_MAX_SIZE`, and the new helper methods (`rejectNonScalar`,
`cachedKey`, `resolveIv`) are all either self-explanatory or have a short
comment explaining intent (e.g. the "Bounded cache..." comment above
`KEY_CACHE`). No missing-documentation issue this round.
>
> ## 2.2 Test Coverage and Test Stability
> * Coverage is now excellent and specifically targets the
previously-identified gaps: SQL-level dispatch/typing
(`SQLCryptoFunctionsTest`), E2E round trip (`func_string.conf`), interop with
the sibling `AesCbcEncryptor`, a known-answer vector, `byte[]`/array/map
rejection, explicit-NULL-IV rejection, and a dedicated "no secrets in exception
messages" assertion across 5 different failure paths.
> * Stability rating: **Stable.** All tests are pure in-JVM/in-process calls
with no sleeps, no fixed ports, no shared mutable statics being asserted on for
order (the new `KEY_CACHE` is populated as a side effect but no test depends on
its state or eviction timing), and no external resources. The two tests whose
outcome depends on the random IV
(`testDecryptWithWrongKeyDoesNotLeakPlaintext`,
`testExplicitIvCiphertextCannotBeDecryptedWithoutIv`) still correctly accept
both a thrown exception and a garbage-but-different result, which is the only
sound way to assert around CBC's ~1/256 accidental-padding-pass. No flaky-test
anti-patterns found.
>
> ## 2.3 Documentation Updates
> * `docs/en` and `docs/zh` sql-functions.md are fully parallel and
consistent with the code (argument order, defaults, NULL handling, security
notes). `incompatible-changes.md` is now updated in both languages with a
correct impact/migration description - the only defect is the broken relative
link noted in Issue 1.
>
> # 3. Architectural Soundness
> ## 3.1 Elegance of the Solution
> Still a small, contained, **long-term** addition, not a workaround.
Placement in the shared `ZetaSQLFunction` switch (rather than a `ZetaUDF`) is a
deliberate, now-documented trade-off, consistent with what I recommended last
round.
>
> ## 3.2 Maintainability
> Good. The duplicated key-parsing logic relative to
`AbstractAesEncryptor.buildAesKey` still exists (this was Issue 3 last round,
now closed via documentation + interop test rather than code reuse), which is
an acceptable, explicitly-tested trade-off rather than a latent bug.
>
> ## 3.3 Extensibility
> Unchanged from my last review: the `(value, key[, iv])` argument shape
leaves room for a future mode argument (e.g. GCM) without a breaking change.
>
> ## 3.4 Historical-Version Compatibility
> No config options, defaults, serialization, or checkpoint state are
touched. The only historical-version-relevant behavior is the UDF-name
shadowing, which is now properly documented in `incompatible-changes.md`. No
incompatible impact on the `2.6-release -> 3.0` style upgrade path beyond that.
>
> # 4. Issue Summary
> # Issue Location Severity
> 1 Broken relative link to `sql-functions.md` in `incompatible-changes.md`
(en+zh); currently fails the "Dead links" CI job
`docs/en/introduction/concepts/incompatible-changes.md`,
`docs/zh/introduction/concepts/incompatible-changes.md` High
> 2 Static, process-wide `KEY_CACHE` with full-clear eviction can thrash
under many distinct keys and affects unrelated concurrent jobs
`CryptoFunction.java` (`KEY_CACHE`, `cachedKey`) Low
> (All 10 issues from the previous review round are closed; see the
verification table in 1.1.)
>
> # 5. Merge Recommendation
> ### Conclusion: Ready to merge after fixes
> 1. Blockers - must be fixed
>
> * Issue 1: fix the two broken relative links
(`transforms/sql-functions.md` -> `../../transforms/sql-functions.md`) in both
the en and zh `incompatible-changes.md` files, then re-run CI. This is the only
concrete blocker I found, and it is a one-line-per-file fix.
> * Separately, the PR is currently `diverged` from `dev` (behind by a
number of commits) and `mergeable_state` is `dirty` because another PR has
since added a different section to the same `incompatible-changes.md` files in
both languages. Please merge/rebase `dev` to resolve the textual conflict; it
is a simple "both sections stay" resolution, not a real conflict of substance.
> 2. Recommended fixes - non-blocking
>
> * Issue 2: consider scoping `KEY_CACHE` per transform instance or
switching to a bounded LRU instead of a full clear, to avoid cross-job cache
interference. Fine to defer to a follow-up.
>
> Overall assessment: this is an exemplary second pass - every
previously-raised point was fixed with real code changes backed by targeted
regression tests, including the two things I'd have called must-fix
(secret-free error messages, UDF-shadowing documentation). The only new problem
is a small, mechanical doc-link bug that is already visible as a red CI check,
plus a `dev`-sync needed for the same file. Once the link is fixed and the
branch is synced with `dev`, I have no reservations about merging this.
>
> Thanks again for the careful work and for engaging with every point from
the first review - much appreciated.
Thanks for the thorough second round. Both items are fixed in this commit.
--
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]
