Repository navigation
fix: keep URL passwords and query strings out of Parquet writer output - #7561
Draft
dwsmith1983 wants to merge 29 commits into
Draft
dwsmith1983 wants to merge 29 commits into
dwsmith1983 wants to merge 29 commits into
Conversation
… logs The native Azure store printed whole URLs in its errors and debug lines, and handed them to object_store, whose own errors quote the URL. A password in the userinfo or a SAS token in the query could end up in task errors and logs. object_store now gets a copy without the password, query and fragment, keeping the container it reads from the username. Messages name the URL by scheme, container and host only.
… driver messages object_store_authority kept an ABFS URL's password, so it reached the store cache key, the registration URL, debug lines and the deregister_store error. object_store_backend parsed the raw URL and normalize_object_store_url echoed input it could not parse. On the driver, AbfsAuthResolver put the URI into forwarded errors and a warning, and the mixed-store decline reason and objectStoreKey kept the raw authority. All of these now name a URL by scheme, container and host only. The URL helpers move to objectstore/url_redaction.rs, NativeConfig gains a matching displayAuthority, objectStoreKey matches the native key again, and the resolver scrubs the URI's password and query from Hadoop messages it forwards. Credentials only come from configuration, so URLs that differ only by password share a store.
The message no longer needs the host, so the redacted helper that built it is removed.
Drop comments that restate the code or a test's name, and shorten the rest to why each part of a URL is kept or removed.
Native now builds the key callback path from the authority without its password, so the retriever registered under the listed path was never found. Register and look up retrievers by one key: S3 aliases folded to s3a, and for abfs/abfss the scheme lowercased and the password dropped by the same rule as native. The missing-retriever error prints that key.
Secrets were replaced one after another, so a URI password inside a configured credential was masked first and the rest of the credential stayed in the message. Match every secret in the same text and replace each run of masked characters with one marker.
The object store key is built per file during fallback checks and again per split in partition packing. Return the raw authority as is when it has no `@`, or no `:` before the last `@`, instead of rebuilding the same string.
…layAuthority nativeFallbackReason stripped the URL password with its own copy of the logic in NativeConfig.authorityWithoutPassword. Use the shared helpers so the resolver renders a location one way in every message: lower-case scheme, no password, path or query.
The ABFS resolver skipped secrets under eight characters, so a short password in the URI reached the driver warning when Hadoop named the container-scoped keystore alias it could not recover. URI secrets are now hidden at any length; the floor applies only to configured values. The password is read from the raw authority, split as Hadoop splits the container, so a registry-based authority (an underscore in the host) no longer loses it.
The native Parquet writer printed its output path in its URL errors, its plan line and the debug line it logs for each file, so a password in the userinfo or a SAS token or signature in the query ended up in task failures and logs. Those lines now show the path without its password, query or fragment, and outside ABFS without its username; ABFS keeps the username, which names the container. A path with nothing to hide is shown exactly as Spark passed it, and one that does not parse shows only its scheme.
github-actions
Bot
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #7128.
This is the second of four pull requests for #7128, each covering one area. They build on #7127, which adds the URL redaction helpers they use, so #7127 merges first. Suggested order after it:
They target
main, so until #7127 merges each one also shows its commits.Rationale for this change
The native Parquet writer prints its output path in its URL errors, its plan line and the debug line it logs for each file. A path can carry a password in its userinfo or a SAS token or signature in its query string, so those lines put secrets in task failures and logs.
What changes are included in this PR?
The HDFS store setup and operator messages also print the error they wrap; for a URL with no host that error quotes the URL until #7556 lands.
How are these changes tested?
Native tests run the writer's error paths, its plan display and its debug log with a path holding a password, a username, a signature query and a fragment, and check that none of them appears while the scheme, host and path do. A table test covers each part that gets removed, the ABFS container, unparseable paths, and local paths that must stay as they are. Each test fails on the base branch. The two HDFS messages need a running name node, so no unit test reaches them.