fix: harden credential redaction in logged SQL - #1610
fix: harden credential redaction in logged SQL#1610SreeramaYeshwanthGowd wants to merge 6 commits into
Conversation
Signed-off-by: Sreerama Yeshwanth Gowd <yeshwanthgowdsreerama@gmail.com>
Signed-off-by: Sreerama Yeshwanth Gowd <yeshwanthgowdsreerama@gmail.com>
Signed-off-by: Sreerama Yeshwanth Gowd <yeshwanthgowdsreerama@gmail.com>
|
Flagging that I have three small fixes open: #1613, #1610 and #1612. Could someone approve the fork workflow runs on #1610 and #1612 so CI can report? #1613 is already green. If it helps with review order, #1613 is the smallest. #1612 is the one that would benefit most from an |
…-redaction # Conflicts: # CHANGELOG.md
|
@sd-db @jprakash-db Merged main and cleared the changelog conflict, so this is ready again. No rush, just flagging in case it helps. |
|
Thanks for working on this. I found one remaining redaction edge case: an escaped apostrophe immediately before I prepared a narrow follow-up in [602987d](602987d). It keeps redaction best-effort and fail-open. Could you use or cherry-pick this commit into the PR? |
Resolves #1609
Description
redact_credentialsis applied to every logged statement (connections.py:244and:342), but theCOPY INTOpattern behind it misses several shapes, so secrets can reachlogs/dbt.logverbatim: anuppercase
CREDENTIAL (...)clause, theencryption (...)clause thatdatabricks_copy_intoemitsitself, a dotted key such as
'fs.azure.account.key', a value containing a newline, and every clauseafter the first.
The same function has two problems that are not leaks. Splitting the clause body on
,raisesValueError: not enough values to unpackwhen a value contains a comma, and because the call sits online 244 before the
try:on line 246, the exception escapes while the context manager is beingentered, so the statement never runs. Separately,
'.*?'inside a repeated group backtracksexponentially on an unterminated clause.
Fix: match
credentialandencryptioncase-insensitively, recover the keys with a regex insteadof splitting on
,, andsubevery clause. A quoted value is described as'(?:[^']|'(?!\s*[,)]))*', which treats a quote followed by a delimiter as the closing quote and anyother quote as part of the value. That keeps the match linear and still redacts values containing a
quote. A cheap substring test runs first so the common no-clause path does not pay for the
case-insensitive scan; a 36 KB statement with no clause ends up faster than before, and the
pathological input drops from roughly 24 s to sub-millisecond.
The keyword deliberately has no
\bprefix. The current lookbehind has no boundary requirement, sostorage_credential (...)is redacted today, and adding one would silently narrow coverage. Key namesare preserved and every value in a matched clause is redacted, including a non-secret one such as
'TYPE' = 'AWS_SSE_C', which matches the existing behaviour forcredential (...).Scope note: this repairs the existing
COPY INTOredaction only. Other syntax that can carrysecrets, such as the
OPTIONS (...)clause, is out of scope here.Testing:
1321 passed, 5 skippedon the unit suite. The three existing redaction tests areunchanged and still pass byte for byte, which confirms no behaviour regression. Ten cases are added,
one per shape above plus negative cases proving that
credential_id = 1and a clause with no'key' = 'value'options are left alone. No functional test is included because log redaction is notobservable against a warehouse.
Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.