-
Notifications
You must be signed in to change notification settings - Fork 504
Fix: CLI's "--debug" mode leaks credentials to stderr (sanitizer misses camelCase keys on the wire) #5289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix: CLI's "--debug" mode leaks credentials to stderr (sanitizer misses camelCase keys on the wire) #5289
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,21 +32,35 @@ | |
| OAUTH_TOKEN_BODY_REDACTED = "<redacted sensitive authentication payload>" | ||
| SANITIZE_FAILURE_MESSAGE = "<redacted: unable to sanitize payload>" | ||
|
|
||
| SENSITIVE_BODY_KEYS = frozenset({"client_secret", "access_token", "refresh_token"}) | ||
| # The Polaris management API serializes bodies by alias (camelCase — e.g. | ||
| # ``clientSecret``, ``bearerToken``), while the OAuth token endpoint uses | ||
| # snake_case. Matching on the normalized key covers both. | ||
| SENSITIVE_BODY_KEYS = frozenset( | ||
| { | ||
| "clientsecret", | ||
| "accesstoken", | ||
| "refreshtoken", | ||
| "bearertoken", | ||
| "token", | ||
| "password", | ||
| "secret", | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| def _is_sensitive_key(key: Any) -> bool: | ||
| return ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this, we will need to change to following to captured known keys as well as adls specific where they can put adls.sas-token as prefix (reference: https://github.com/apache/polaris/blob/main/site/content/in-dev/unreleased/configuration/config-sections/storage-azure.md?plain=1): |
||
| isinstance(key, str) | ||
| and key.replace("_", "").replace("-", "").lower() in SENSITIVE_BODY_KEYS | ||
| ) | ||
|
|
||
|
|
||
| def sanitize_data(data: Any) -> Any: | ||
| if isinstance(data, dict): | ||
| sanitized: dict[Any, Any] = {} | ||
| for key, value in data.items(): | ||
| if key in SENSITIVE_BODY_KEYS: | ||
| if isinstance(value, (dict, list, tuple)): | ||
| sanitized[key] = sanitize_data(value) | ||
| else: | ||
| sanitized[key] = REDACTED | ||
| else: | ||
| sanitized[key] = sanitize_data(value) | ||
| return sanitized | ||
| return { | ||
| key: REDACTED if _is_sensitive_key(key) else sanitize_data(value) | ||
| for key, value in data.items() | ||
| } | ||
| if isinstance(data, list): | ||
| return [sanitize_data(item) for item in data] | ||
| if isinstance(data, tuple): | ||
|
|
@@ -69,7 +83,7 @@ def is_oauth_token_endpoint(url: str) -> bool: | |
|
|
||
| def _sanitize_form_body(body: str) -> str: | ||
| sanitized_pairs = [ | ||
| (key, REDACTED if key in SENSITIVE_BODY_KEYS else value) | ||
| (key, REDACTED if _is_sensitive_key(key) else value) | ||
| for key, value in parse_qsl(body, keep_blank_values=True) | ||
| ] | ||
| return urlencode(sanitized_pairs, safe="*") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would preferred we be more specific on this as all sensitive keys should be known. Also, this doesn't cover case for s3, gcs, and adls as they can be present in the body during debug mode as well. I would change to following instead: