You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by "cloud-fan (via GitHub)" <gi...@apache.org> on 2026/06/20 06:48:56 UTC

[PR] [SPARK-57580][SQL] Redact credentials embedded in JDBC URLs [spark]

cloud-fan opened a new pull request, #56630:
URL: https://github.com/apache/spark/pull/56630

   ### What changes were proposed in this pull request?
   
   `JDBCOptions.getRedactUrl()` is used to surface the JDBC URL in logs and in the
   `FAILED_JDBC.*` error messages (e.g. `FAILED_JDBC.CONNECTION`). Today it is implemented as:
   
   ```scala
   def getRedactUrl(): String = Utils.redact(SQLConf.get.stringRedactionPattern, url)
   ```
   
   `SQLConf.get.stringRedactionPattern` (`spark.sql.redaction.string.regex`) is **unset by
   default**, so `Utils.redact(None, url)` returns the URL unchanged. JDBC URLs routinely embed
   credentials — either as userinfo in the authority (`jdbc:mysql://user:password@host/db`) or as
   connection properties (`...?password=secret`, `...;token=abc`) — so by default those secrets are
   printed verbatim in error messages and logs.
   
   This PR makes `getRedactUrl()` redact credentials unconditionally, independent of the optional
   `spark.sql.redaction.string.regex`:
   - A new `JDBCOptions.redactUrl(url, regex)` helper redacts the password in the authority's userinfo
     (keeping the username) and the values of sensitive connection properties (`password`, `pwd`,
     `token`, `secret`, `accessKey`, `credential`, …), then still applies the user-configured
     `regex` on top, preserving existing behavior.
   - `getRedactUrl()` now delegates to this helper.
   
   Non-sensitive parts of the URL (scheme, host, port, database, and non-secret properties) are kept,
   so error messages remain useful for debugging while no longer leaking credentials.
   
   ### Why are the changes needed?
   
   `FAILED_JDBC.*` errors and connection logs can currently leak JDBC credentials to anyone who can
   read query error messages or driver logs, because the default redaction is a no-op. Redacting at
   the single `getRedactUrl()` chokepoint fixes every `FAILED_JDBC.*` call site at once.
   
   ### Does this PR introduce _any_ user-facing change?
   
   Yes. When a JDBC URL contains embedded credentials, `FAILED_JDBC.*` error messages (and any log
   line built from `getRedactUrl()`) now show the credentials replaced with `*********(redacted)`
   instead of in clear text. URLs without embedded credentials are unchanged.
   
   ### How was this patch tested?
   
   New unit test `redactUrl redacts credentials embedded in a JDBC URL` in `JdbcUtilsSuite`, covering:
   credential-free URLs (passthrough), userinfo password redaction (username kept), bare-username
   passthrough, sensitive connection properties across `&`/`;` separators and mixed case,
   non-sensitive properties kept, combined userinfo + property redaction, and null/empty inputs.
   
   ### Was this patch authored or co-authored using generative AI tooling?
   
   Generated-by: Claude Opus 4.8
   


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-57580][SQL] Redact credentials embedded in JDBC URLs [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.

cloud-fan commented on PR #56630:
URL: https://github.com/apache/spark/pull/56630#issuecomment-4763225958

   thanks for the review, merging to master/4.x/4.2


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-57580][SQL] Redact credentials embedded in JDBC URLs [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.

cloud-fan closed pull request #56630: [SPARK-57580][SQL] Redact credentials embedded in JDBC URLs
URL: https://github.com/apache/spark/pull/56630


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-57580][SQL] Redact credentials embedded in JDBC URLs [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.

cloud-fan commented on PR #56630:
URL: https://github.com/apache/spark/pull/56630#issuecomment-4761241943

Heads-up that the latest commit is a **design pivot**, not just a fix, so it's worth a fresh look rather than a line-diff review.

The original commit tried to *find and strip* each place a credential could hide (userinfo authority + `?`/`;` properties). As @MaxGekk's review showed, that's a denylist that has to chase every driver's URL grammar (Oracle Thin being the first miss). The rework inverts it to an **allowlist**: keep only the `jdbc:<subprotocol>:` prefix — which can't contain credentials — and redact the whole subname.

This resolves the review thread wholesale:
- **Oracle Thin (blocking):** redacted, no `//`-authority assumption anywhere.
- **`?`/`#`-in-password partial leak:** gone; no userinfo parsing.
- **`regex`-on-top coverage:** added a `Some(regex)` test.
- **Doc accuracy:** scaladoc now states only the driver type survives.

Trade-off, called out in the PR description: messages now show only the engine type (e.g. `jdbc:mysql:*********(redacted)`), not host/database. That's intentional, on the "worst case is showing no URL at all" principle — `FAILED_JDBC.*` still carries the driver exception/SQLState for triage. Happy to dial this back to also surface the host if reviewers feel the lost signal outweighs the simplicity.

--
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-57580][SQL] Redact credentials embedded in JDBC URLs [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.

MaxGekk commented on code in PR #56630:
URL: https://github.com/apache/spark/pull/56630#discussion_r3447465703


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala:
##########
@@ -302,6 +304,41 @@ object JDBCOptions {
     name
   }
 
+  // The userinfo of a URL authority (everything between "//" and the authority's last "@") may
+  // carry credentials, e.g. the "user:password" in "//user:password@host". "[^/?#]*" extends
+  // greedily to the last "@" before the authority ends (at the first "/", "?" or "#"), so an "@"
+  // embedded in the password is covered as well; the whole userinfo is redacted.
+  private val URL_USER_INFO_PATTERN = "(//)[^/?#]*(@)".r

Review Comment:
   Blocking: this misses Oracle Thin's standard credential syntax, where the goal of redacting credentials "unconditionally" is the whole point. Credentials before an `@` that isn't reached via a `//...@` authority survive in clear text — I confirmed all three forms:
   - `jdbc:oracle:thin:scott/tiger@host:1521/service` → unchanged (no `//`, no `?`/`;`)
   - `jdbc:oracle:thin:scott/tiger@//host:1521/service` → unchanged (the `//` sits *after* the `@`, so `(//)[^/?#]*(@)` never matches)
   - `…scott/tiger@//host:1521/service?x=1` → only the `?x=1` tail is redacted; `scott/tiger` survives
   
   Oracle is a mainstream driver and this is its primary embedded-credential form, so a feature that announces unconditional redaction while silently exempting it is a false-confidence trap. Resolve either by broadening redaction to also catch a `…@host` credential form not preceded by `//` (carefully, so an `@` legitimately appearing in a path/database segment isn't over-matched), or by scoping the "unconditionally" wording and documenting Oracle Thin inline credentials as unhandled.



##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala:
##########
@@ -302,6 +304,41 @@ object JDBCOptions {
     name
   }
 
+  // The userinfo of a URL authority (everything between "//" and the authority's last "@") may
+  // carry credentials, e.g. the "user:password" in "//user:password@host". "[^/?#]*" extends

Review Comment:
   Edge-case partial leak: because `[^/?#]*` stops at `?`, a password containing `?` (or `#`) prevents the userinfo `@` from being reached, and the property pass only redacts from the `?` onward. So `jdbc:mysql://user:pa?ss@host/db` → `jdbc:mysql://user:pa?*********(redacted)`, leaking `user:pa`. Raw `?`/`#` in userinfo is non-RFC-compliant (should be percent-encoded), so this is low-likelihood — noting it for completeness. (`;` in a password is fine: it's not in the excluded class, so the `@` is still reached.)



##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala:
##########
@@ -302,6 +304,41 @@ object JDBCOptions {
     name
   }
 
+  // The userinfo of a URL authority (everything between "//" and the authority's last "@") may
+  // carry credentials, e.g. the "user:password" in "//user:password@host". "[^/?#]*" extends
+  // greedily to the last "@" before the authority ends (at the first "/", "?" or "#"), so an "@"
+  // embedded in the password is covered as well; the whole userinfo is redacted.
+  private val URL_USER_INFO_PATTERN = "(//)[^/?#]*(@)".r
+
+  // The query / connection-property portion of a JDBC URL (everything from the first "?" or ";")
+  // can carry credentials as key=value pairs (e.g. "password=...", "token=..."), and the set of
+  // credential-bearing property names is driver-specific and open-ended. Rather than enumerate
+  // them, redact the whole portion, keeping only the structural "scheme://host:port/database".
+  private val URL_PARAMS_PATTERN = "([?;]).*".r
+
+  /**
+   * Redacts credentials that may be embedded in a JDBC URL so it is safe to surface in logs and
+   * error messages. Only the structural prefix (scheme, host, port, database/path) is preserved;

Review Comment:
   Minor: the doc says only the "structural prefix (scheme, host, port, database/path)" is preserved, but for `;`/`?`-property URLs the database name is carried as a property and is redacted — the test asserts `jdbc:sqlserver://localhost:1433;*********(redacted)` for `;databaseName=testdb`. Worth noting the db name is preserved only when it's part of the `/`-path, not when carried as a property.



##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtilsSuite.scala:
##########
@@ -69,4 +70,48 @@ class JdbcUtilsSuite extends SparkFunSuite {
       condition = "PARSE_SYNTAX_ERROR",
       parameters = Map("error" -> "'.'", "hint" -> ""))
   }
+
+  test("redactUrl redacts credentials embedded in a JDBC URL") {

Review Comment:
   Every assertion passes `regex = None`, so the "user-configured `spark.sql.redaction.string.regex` is still applied on top" behavior — the one piece of prior behavior this PR preserves — has no coverage. Consider a `Some(regex)` case asserting it redacts an additional substring of the structural prefix (e.g. the host).



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-57580][SQL] Redact credentials embedded in JDBC URLs [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.

cloud-fan commented on code in PR #56630:
URL: https://github.com/apache/spark/pull/56630#discussion_r3448061148


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala:
##########
@@ -302,6 +304,41 @@ object JDBCOptions {
     name
   }
 
+  // The userinfo of a URL authority (everything between "//" and the authority's last "@") may
+  // carry credentials, e.g. the "user:password" in "//user:password@host". "[^/?#]*" extends
+  // greedily to the last "@" before the authority ends (at the first "/", "?" or "#"), so an "@"
+  // embedded in the password is covered as well; the whole userinfo is redacted.
+  private val URL_USER_INFO_PATTERN = "(//)[^/?#]*(@)".r

Review Comment:
   Good catch — thanks for digging into the Oracle Thin forms. Rather than extend the matching to chase this (and the next driver's syntax after it), I've reworked the redaction from a denylist into an allowlist: keep only the credential-free `jdbc:<subprotocol>:` prefix and redact the entire driver-specific subname.
   
   - `jdbc:oracle:thin:scott/tiger@host:1521/svc` -> `jdbc:oracle:*********(redacted)`
   - `jdbc:oracle:thin:scott/tiger@//host:1521/svc?x=1` -> `jdbc:oracle:*********(redacted)`
   
   This is safe by construction regardless of the driver's URL grammar, so it also subsumes the `?`/`#`-in-password partial-leak edge case you flagged below (no userinfo parsing happens at all). The trade-off is that we no longer surface host/port/database — only the engine type — which matches the "worst case is showing no URL at all" bar. Updated in the follow-up commit; the doc and tests now reflect this.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org