Repository navigation
fix: keep URL passwords and query strings out of object store errors - #7556
dwsmith1983 wants to merge 37 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.
object_store's builders and its HTTP store quote the URL they are given in their errors, and several Comet paths printed the URL themselves, so a password in the userinfo or a SAS token or signature in the query ended up in task failures and logs. The fallthrough to object_store's parse_url, the S3 store and its template now work from a URL without userinfo or query; an az URL keeps its username, which object_store reads as the container. The S3 "not S3" error, the blob alias parse and missing bucket errors, and the hostless HDFS error name only the scheme. None of these stores reads the password or the query, so only the messages change.
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks for splitting #7128 up. I checked the HTTP fallthrough: object_store 0.13.2 sends its requests through reqwest::Request::new and Client::execute, which never turn URL userinfo into a Basic Authorization header, so stripping it there changes only the messages, as the description says. The comments below are about where the stripping happens and test overlap.
| ) -> Result<(Box<dyn ObjectStore>, Path), object_store::Error> { | ||
| let (scheme, path) = ObjectStoreScheme::parse(url)?; | ||
| // The parser quotes the URL it rejects. | ||
| let (scheme, path) = ObjectStoreScheme::parse(&without_credentials(url))?; |
There was a problem hiding this comment.
create_store strips the URL here for the parser and then passes the original url to S3StoreTemplate::new (both call sites below), which strips it again at line 186. Could create_store start with let url = &without_credentials(url); and use it throughout, with S3StoreTemplate::new storing url.to_string() as before? host_str() and path() are unchanged by the stripping, and create_store is the only non-test caller of S3StoreTemplate::new, so one strip would cover the parser, the error and the template.
There was a problem hiding this comment.
Done in b749d68. create_store strips the URL once at the top and S3StoreTemplate::new stores url.to_string() again. The template test went away with the second strip, so create_store_errors_do_not_echo_userinfo_or_query now also takes an https://...amazonaws.com URL that the parser accepts and the S3 builder rejects; the builder's error quotes the URL the template holds.
| fn promote_errors_do_not_echo_the_url() { | ||
| let err = | ||
| promote_hostless_alias_url("blob://alice:SECRET@bucket:notaport/x?sig=SAS-MARKER") | ||
| .unwrap_err() | ||
| .to_string(); | ||
| for leaked in ["alice", "SECRET", "SAS-MARKER"] { | ||
| assert!(!err.contains(leaked), "{leaked} echoed: {err}"); | ||
| } | ||
| assert!( | ||
| err.contains("Error parsing blob URL: invalid port number"), | ||
| "{err}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
This is the same check as the existing parse_errors_name_only_a_valid_scheme above, with the same bucket:notaport URL shape, only going through promote_hostless_alias_url. Could it be one more case in that test instead of a new one?
There was a problem hiding this comment.
Moved it into parse_errors_name_only_a_valid_scheme in b749d68.
create_store now strips credentials from the URL at the top and hands the stripped URL to both the parser and S3StoreTemplate, so the template no longer strips it a second time. The template test goes with it; a create_store case whose URL the S3 builder rejects checks that the builder never sees the credentials. The promote_hostless_alias_url error case moves into parse_errors_name_only_a_valid_scheme.
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
The PR backlog is huge and the maintainers don't have the bandwidth to review them all, so here is an automated review. Feel free to ignore it.
I found no blocking concerns. The change is a tidy follow-up to the redaction helpers and the tests exercise each leak path with a password, a username and a signature query.
I checked the claim that stripping userinfo and query from the parse_url fallthrough in prepare_object_store_with_config_hash changes only messages. In object_store 0.13.2 the HTTP store passes &url[..Position::BeforePath] to HttpBuilder, so the query was already dropped there. Its requests go through reqwest::Request::new and Client::execute, which do not run reqwest's extract_authority, so userinfo never became a Basic Authorization header. For Azure, MicrosoftAzureBuilder::parse_url reads the username only for az, abfs and abfss. abfs and abfss already go through azure::create_store, and az keeps its username through without_secrets, so the container is preserved. The adl and azure schemes read only the host, so stripping the username is safe for them. The returned path comes from url.path(), which the stripping does not touch.
For the comparison against the latest release, I found no change in results, fallbacks, configs or memory. The only differences are error text and the get_name_node_uri error variant moving from InvalidPath to Generic. Nothing in native/core or the Scala sources matches on that variant or on the old message text.
I have no further concerns beyond the earlier thread, which the second commit addressed by stripping once at the top of create_store and folding the hostless alias case into parse_errors_name_only_a_valid_scheme.
Which issue does this PR close?
Part of #7128.
This is the first of four pull requests for #7128, each covering one area. They use the URL redaction helpers from #7127, which has merged. Suggested order:
Rationale for this change
Several native object store paths print the whole URL in an error message or keep it in a stored value. A URL can carry a password in its userinfo or a SAS token or signature in its query string, so those errors put secrets in task failures and logs. object_store's own errors quote whatever URL they are handed, so passing it the raw URL is enough to leak.
What changes are included in this PR?
parse_urlhands it the URL without userinfo or query. AnazURL keeps its username, which object_store reads as the container.blobstyle aliases no longer quote the URL.hdfsURL with no host reports its scheme instead of the whole URL.None of these stores reads the password or the query from the URL, so only the messages change.
How are these changes tested?
Native tests build each URL with a password, a username and a signature query, then check that none of them appears in the error or stored URL while the host or scheme does. One covers the HTTP store's request error, and one checks that an
azURL keeps its container. Each test fails on the base branch.