sunchao commented on code in PR #6059:
URL: https://github.com/apache/datafusion-comet/pull/6059#discussion_r4057444900
##########
native/core/src/parquet/objectstore/azure.rs:
##########
@@ -118,12 +190,426 @@ pub fn create_store(
.map(|(k, _)| k.as_ref())
.collect::<Vec<_>>()
);
+
+ let env: Vec<(String, String)> = env.collect();
+ validate_translated(
+ configs,
+ &translated,
+ account.as_deref(),
+ container.as_deref(),
+ env_token_file(&env).is_some(),
+ )?;
+ let store = build_builder(
+ url,
+ configs,
+ account.as_deref(),
+ container.as_deref(),
+ &translated,
+ env.into_iter(),
+ )
+ .build()?;
+ Ok((Box::new(store), path))
+}
+
+fn config_error(message: String) -> object_store::Error {
+ object_store::Error::Generic {
+ store: "MicrosoftAzure",
+ source: message.into(),
+ }
+}
+
+/// Reject a Hadoop configuration that `object_store` would silently build a
different
+/// identity from: a blank credential, an auth type or mechanism the native
scan cannot
+/// build, a named principal with no token file in Hadoop or the environment,
or a client
+/// secret or token file without the client id and tenant that complete it.
+/// `has_env_token_file` says whether `AZURE_FEDERATED_TOKEN_FILE` is set.
+fn validate_translated(
+ configs: &HashMap<String, String>,
+ translated: &[(AzureConfigKey, String)],
+ account: Option<&str>,
+ container: Option<&str>,
+ has_env_token_file: bool,
+) -> Result<(), object_store::Error> {
+ let account_name = account.unwrap_or("<unknown>");
+ let fail = |reason: String| {
+ Err(config_error(format!(
+ "Hadoop configuration for account {account_name}: {reason}"
+ )))
+ };
+ if let Some(reason) = hadoop_problem(configs, account, container,
translated) {
+ return fail(reason);
+ }
+ let has = |wanted: AzureConfigKey| translated.iter().any(|(key, _)| *key
== wanted);
+ let borrows_env_token_file = env_policy(configs, account, container,
translated)
+ == EnvPolicy::TokenFileOnly
+ && !has(AzureConfigKey::FederatedTokenFile);
+ if borrows_env_token_file && !has_env_token_file {
+ return fail(format!(
+ "the principal named by the Hadoop keys needs a token file from \
+ `{HADOOP_WI_TOKEN_FILE}` or `{ENV_FEDERATED_TOKEN_FILE}`"
+ ));
+ }
+ let mechanism = if has(AzureConfigKey::ClientSecret) {
+ HADOOP_OAUTH_CLIENT_SECRET
+ } else if has(AzureConfigKey::FederatedTokenFile) {
+ HADOOP_WI_TOKEN_FILE
Review Comment:
### Correctness
[P2] Validate only the authentication mechanism selected for this account
This now rejects a valid SharedKey configuration when inactive global OAuth
settings remain in the Hadoop configuration. For example:
```properties
fs.azure.account.auth.type=OAuth
fs.azure.account.auth.type.myacct.dfs.core.windows.net=SharedKey
fs.azure.account.key.myacct.dfs.core.windows.net=<valid account key>
fs.azure.account.oauth.provider.type=org.apache.hadoop.fs.azurebfs.oauth2.ClientCredsTokenProvider
fs.azure.account.oauth2.client.secret=<unused OAuth secret>
```
Hadoop's `getAuthType` selects the account-scoped SharedKey override, and
`AzureBlobFileSystemStore.initializeClient` calls only `getStorageAccountKey`,
so the missing OAuth client ID/endpoint are irrelevant to this account.
`NativeConfig` nevertheless forwards the global secret. The old native builder
uses the supplied account key; here, after SharedKey validation succeeds, the
presence of that inactive secret makes `validate_translated` require OAuth's
client ID and tenant and fail before constructing the store.
I reproduced this with the exact old/new translation and validation helpers
and a synthetic valid-base64 account key: old selection is AccessKey, while the
new validator returns the missing client-ID/tenant error. Could we resolve the
effective auth mechanism first and validate only its applicable credential
fields/provider? The same rule should cover the earlier
blank/unsupported-provider checks, while preserving rejection of incomplete
**active** OAuth credentials.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
