wombatu-kun commented on code in PR #16541:
URL: https://github.com/apache/iceberg/pull/16541#discussion_r3478731142
##########
azure/src/main/java/org/apache/iceberg/azure/AzureProperties.java:
##########
@@ -212,4 +202,38 @@ public String keyWrapAlgorithm() {
public Optional<String> keyVaultUrl() {
return Optional.ofNullable(this.keyVaultUrl);
}
+
+ /**
+ * Returns the credential to use when authenticating to Azure Key Vault,
derived only from
+ * credentials explicitly supplied through configuration: a catalog-vended
{@link #ADLS_TOKEN}
+ * bearer token, or an explicitly configured {@link
#ADLS_TOKEN_CREDENTIAL_PROVIDER}.
+ *
+ * <p>Ambient credentials such as {@link
com.azure.identity.DefaultAzureCredential} are
+ * intentionally not used for Key Vault: combined with a misconfigured or
malicious vault URL they
+ * could be used to exfiltrate the client's ambient identity. The supplied
token must be scoped
+ * for Key Vault (for the public cloud, {@code https://vault.azure.net}).
+ *
+ * @return the configured Key Vault token credential, or empty if none was
supplied
+ */
+ public Optional<TokenCredential> keyVaultTokenCredential() {
+ if (token != null && !token.isEmpty()) {
+ return Optional.of(tokenCredential(token));
+ }
+
+ if (allProperties.containsKey(ADLS_TOKEN_CREDENTIAL_PROVIDER)) {
+ return
Optional.of(AdlsTokenCredentialProviders.from(allProperties).credential());
Review Comment:
Separating the two is reasonable, but the managed-identity-for-Key-Vault
half is intentionally out of scope: removing ambient/`DefaultAzureCredential`
auth for Key Vault is exactly the fix for #16465. The real remaining gap is
that `adls.token` is storage-scoped by convention while Key Vault needs a
vault-scoped token (`https://vault.azure.net`), so one value can't serve both.
I reused the ADLS properties to keep this security fix minimal and flagged a
dedicated `azure.keyvault.token` as a follow-up in the description - happy to
add `azure.keyvault.token` (+ `azure.keyvault.token-credential-provider`) in
this PR instead if you'd prefer the decoupling now. Note the ambient default
inside `AdlsTokenCredentialProviders` is never reached on this path: it only
runs when `adls.token-credential-provider` is explicitly set.
--
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]
