Skip to content

[Improve][Core] Warn that the base64 config shade does not encrypt anything - #12073

Open
SEZ9 wants to merge 1 commit into
apache:devfrom
SEZ9:improve-config-shade-base64-warning
Open

[Improve][Core] Warn that the base64 config shade does not encrypt anything#12073
SEZ9 wants to merge 1 commit into
apache:devfrom
SEZ9:improve-config-shade-base64-warning

Conversation

@SEZ9

@SEZ9 SEZ9 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Close #12069

Purpose of this pull request

base64 is the built-in ConfigShade and the one most users reach for first, because it needs no setup. Its name gives no hint that the result is reversible by anyone who can read the config file, and a config that has been through POST /encrypt-config looks protected. The docs already say it is not recommended for production, but nothing says so at the point where it is actually used — so configs get committed to Git and shipped in images under the belief that the credentials in them are encrypted.

This logs a warning when a config is decrypted with the base64 shade.

Does this PR introduce any user-facing change?

No behaviour change. One new WARN line when shade.identifier = "base64":

Config shade 'base64' encodes sensitive options, it does not encrypt them: anyone who reads the config can decode the values.

Configs using any other identifier, including the default, are unaffected.

How was this patch tested?

No new test. The change is a single log statement guarded by an equality check on the shade identifier, with no effect on the returned config, so the existing ConfigShadeTest coverage of the base64 path applies unchanged.

To see it, run any job whose config sets shade.identifier = "base64"; the warning is emitted once as the config is decrypted. A config with no shade.identifier does not emit it.

Check list

  • If any new Jar binary package adding in your PR, please add License Notice according
    New License Guide
  • If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
  • If necessary, please update incompatible-changes.md to describe the incompatibility caused by this PR.

…ything

`base64` is the built-in ConfigShade that most users reach for first, and its
name gives no hint that it is reversible by anyone who can read the config
file. The docs already say it is not recommended for production, but nothing
says so at the point where it is actually used.

Log a warning when a config is decrypted with the `base64` shade, so an
operator who picked it by accident finds out from the job log instead of from
an incident.
@github-actions github-actions Bot added the core SeaTunnel core module label Sep 3, 2026

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Problem Does This PR Solve?

base64 is SeaTunnel's built-in, zero-setup ConfigShade and the one users reach for first, but its name gives no hint that the result is trivially reversible by anyone who can read the config file. A config that has been through POST /encrypt-config with shade.identifier = "base64" looks protected, even though the docs already say it is not recommended for production — nothing said so at the point where it is actually used, so such configs get committed to Git or shipped in images under the mistaken belief that the embedded credentials are encrypted. This PR adds a single WARN log line whenever a config is processed with the base64 shade.

1. Code Change Review

1.1 Core Logic Analysis

seatunnel-core/seatunnel-core-starter/.../utils/ConfigShadeUtils.java, method processConfig (private, shared by both encryptConfig and decryptConfig):

ConfigShade configShade = CONFIG_SHADES.getOrDefault(identifier, DEFAULT_SHADE);
if (Base64ConfigShade.IDENTIFIER.equals(identifier)) {
    log.warn(
            "Config shade '{}' encodes sensitive options, it does not encrypt them:"
                    + " anyone who reads the config can decode the values.",
            Base64ConfigShade.IDENTIFIER);
}
// call open method before the encrypt/decrypt
configShade.open(props);

Key findings:

  1. Base64ConfigShade is a public static nested class inside ConfigShadeUtils itself (ConfigShadeUtils$Base64ConfigShade, registered via META-INF/services/org.apache.seatunnel.api.configuration.ConfigShade), so the unqualified reference Base64ConfigShade.IDENTIFIER resolves within the same top-level compilation unit without any new import — this compiles cleanly and IDENTIFIER correctly equals "base64".
  2. Because the check lives inside the shared processConfig helper rather than in decryptConfig specifically, the warning actually fires on both the encrypt path (POST /encrypt-config) and the decrypt path (job submission), not only on decrypt as the PR description states ("This logs a warning when a config is decrypted..."). This is a documentation-of-intent inaccuracy in the PR description, not a code defect — firing on encrypt as well is arguably more useful, since it warns the user at the point they believe they are protecting their config, not only later when it is read back.
  3. processConfig is invoked once per job submission / once per /encrypt-config call, not in any per-record or per-checkpoint hot path, so the added equals check and conditional log call have no measurable performance cost.
  4. The check runs before configShade.open(props) and does not alter configShade, props, or the returned Config in any way — this is a pure, side-effect-free (aside from logging) addition; the encryption/decryption result for base64 configs is provably unchanged.
  5. I verified the existing ConfigShadeTest (not touched by this PR) already exercises the base64 decrypt path (env.put("shade.identifier", "base64") at line 93, plus multiple ConfigShadeUtils.encryptOption("base64", ...) / decryptOption("base64", ...) calls) — these continue to pass unchanged since encryptOption/decryptOption are separate, simpler methods that do not go through processConfig at all, while the processConfig-based tests exercise the new warning branch without any behavioral change to assert against beyond "still returns the same decrypted config."

1.2 Compatibility Impact

Fully compatible. No config schema, API, default value, or return value changes — this is a pure logging addition guarded by an equality check on identifier. Existing jobs using shade.identifier = "base64" behave identically except for the new WARN line.

1.3 Performance / Side-Effect Analysis

Negligible. A single String.equals check plus, in the base64 case only, one log.warn call per job-submission or per encrypt-config call — not a hot path, no new allocations of consequence, no locking, threading, or retry behavior introduced.

1.4 Error Handling and Logging

This is the entire purpose of the change and it is correctly scoped: it does not throw, does not block, and does not alter behavior for any other shade identifier (including the case-sensitive exact match against "base64" — a shade identifier of, say, "Base64" or "BASE64" would not trigger the warning, which is consistent with how CONFIG_SHADES.getOrDefault(identifier, ...) already does an exact, case-sensitive lookup elsewhere in this same class, so there is no new inconsistency here).

No blocking issues found in this section.

2. Code Quality Assessment

2.1 Coding Standards

The added block is short and self-explanatory (a warning message describing exactly what a user needs to know); no new core method or nontrivial field is introduced that would require additional Javadoc. The existing // call open method before the encrypt/decrypt comment immediately below is left intact and still accurate.

2.2 Test Coverage and Test Stability

Stable. No new test was added, but the PR description's reasoning holds up: the change is a single log statement with no branch affecting the return value, and the existing ConfigShadeTest base64-identifier coverage (both the env-block decrypt path and the direct encryptOption/decryptOption calls) continues to exercise this code without any risk of a new failure mode. There is no timing, concurrency, or Thread.sleep-style flakiness surface introduced.

2.3 Documentation Updates

No update strictly required for docs/en/docs/zh — this is a runtime log message, not a config option, default, or API contract. I did check whether the shade/encryption docs already caution against base64 for production use (per the PR description, "the docs already say it is not recommended for production") and did not find an obvious opportunity to cross-link the new warning from user-facing docs, but this is a nice-to-have, not a gap introduced by this PR.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. It is the smallest possible change that closes the actual gap (silence at the point of use), and it reuses the existing Base64ConfigShade.IDENTIFIER constant rather than hardcoding the string "base64" a second time.

3.2 Maintainability

High. The check is co-located with the configShade resolution it depends on, so a future reader immediately sees both the shade lookup and the special-case warning together.

3.3 Extensibility

Not directly relevant; this does not change how new ConfigShade implementations are registered or consumed. If a future shade also warranted a similar caveat, the same pattern (an identifier-specific warning in processConfig) could be reused, though a small registry of "shades that warrant a security caveat" would scale better than a second hardcoded if — not a concern at the current scale of one such shade.

3.4 Historical-Version Compatibility

No serialization, protocol, or state-recovery surface touched; fully compatible with prior versions and with configs already encrypted/decrypted via the base64 shade.

4. Issue Summary

No blocking issues found.

Number Issue Location Severity
1 PR description says the warning fires "when a config is decrypted," but the check is in the shared processConfig helper and also fires on the encrypt path (POST /encrypt-config); this is a description-accuracy note only, and the actual (broader) behavior is arguably preferable seatunnel-core/seatunnel-core-starter/.../utils/ConfigShadeUtils.java (processConfig) Low

5. Merge Recommendation

Conclusion: Ready to merge

  1. Blockers — none.
  2. Recommended fixes — non-blocking: none required; optionally correct the PR description's "on decrypt" phrasing since the log fires on both encrypt and decrypt.

Overall assessment: a minimal, well-targeted, zero-risk change that closes a real gap between what the docs say about base64 and what a user actually sees at the point of use. No better alternative implementation is warranted for a change this size. Note: the Build CI check was still in_progress at review time; this recommendation assumes it completes green, consistent with the local static analysis above which found no compile or logic issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core SeaTunnel core module reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature][Core] Warn that the base64 config shade does not encrypt anything

2 participants