laskoviymishka commented on code in PR #2163:
URL: https://github.com/apache/iceberg-go/pull/2163#discussion_r4234099472
##########
catalog/rest/rest.go:
##########
@@ -1566,7 +1566,7 @@ func (r *Catalog) CreateTable(ctx context.Context,
identifier table.Identifier,
maps.Copy(saved, ret.Config)
scanPlanningConfig := maps.Clone(config)
credsVended := len(ret.StorageCredentials) > 0
- maps.Copy(config, resolveStorageCredentials(ret.StorageCredentials,
ret.MetadataLoc))
+ mergeVendedCredentials(config,
resolveStorageCredentials(ret.StorageCredentials, ret.MetadataLoc))
Review Comment:
Worth being explicit that this is a behavior change for S3/GCS users, not
just plumbing for the new token. The clear-then-copy used to run only inside
`propertiesForLocation`, where a credential's Config is a complete bundle; here
`config` is the table-wide map including `ret.Config`, so a server that returns
a static access/secret pair in config and vends only `s3.session-token` now has
that pair wiped and is left with just the token.
I'd either split the helper plus the five site conversions into their own PR
and describe it as a semantics change, or at minimum call it out in the
description and decide explicitly whether a partial vended credential should
replace or layer onto the base config.
##########
catalog/rest/refresh_table_credentials_test.go:
##########
@@ -466,3 +466,63 @@ func TestRefreshTableCredentialsNotFound(t *testing.T) {
assert.Nil(t, refreshed)
assert.ErrorIs(t, err, catalog.ErrNoSuchTable)
}
+
+// TestRefreshTableCredentialsDropsStaleCredentials checks that refreshed
+// credentials replace, rather than sit beside, credentials of the same kind
left
+// in the table's saved config. A stale higher-precedence credential would
+// otherwise shadow the freshly vended one.
+func TestRefreshTableCredentialsDropsStaleCredentials(t *testing.T) {
Review Comment:
I'd extend coverage past RefreshTableCredentials — the three new merge sites
(CreateTable/RegisterTable/loadTableWithMode) go through the same helper but
have no test, and those are exactly where `config` is the table-wide map rather
than a per-location copy, so the cross-account and partial-credential cases
below actually bite there.
A table test through the real Create/Register/Load handlers with (a) a
second-account SAS that must survive a vended token and (b) a partial vended S3
credential layered over a full key pair in the server config block would pin
both blockers.
##########
catalog/rest/vended_creds.go:
##########
@@ -474,6 +486,26 @@ func clearOverriddenCredentialProperties(props,
credentialConfig iceberg.Propert
delete(props, keyAdlsSasExpiresAtMs+"."+suffix)
}
}
+
+ if credentialConfig[iceio.ADLSToken] != "" {
+ // adls.token is evaluated after shared-key, SAS-token, and
connection-string
+ // auth, so any of those left over from the table or catalog
configuration
+ // would shadow a freshly vended token and the read would 403.
props is a
+ // per-location copy, so clearing them cannot affect another
location's IO.
+ //
+ // The value must be nonempty: the Azure bucket factory skips
an empty
+ // adls.token, so clearing on mere key presence would discard
working
+ // credentials and fall through to a weaker auth method.
+ delete(props, iceio.ADLSSharedKeyAccountName)
+ delete(props, iceio.ADLSSharedKeyAccountKey)
+ for key := range props {
+ if strings.HasPrefix(key, iceio.ADLSSasTokenPrefix) ||
+ strings.HasPrefix(key,
keyAdlsSasExpiresAtMs+".") ||
Review Comment:
This only clears the per-host `adls.sas-token-expires-at-ms.<host>` form,
but `parseCredentialExpiry` also reads the bare `keyAdlsSasExpiresAtMs` — so a
bare expiry key would survive the SAS wipe and dangle. Worth a `delete(props,
keyAdlsSasExpiresAtMs)` alongside the loop.
##########
catalog/rest/vended_creds_test.go:
##########
@@ -692,6 +692,96 @@ func TestPrefixScopedIOReplacesS3CredentialAtomically(t
*testing.T) {
assert.NotContains(t, props, keyS3TokenExpiresAtMs)
}
+func TestPrefixScopedIOClearsStaleADLSAuthForVendedToken(t *testing.T) {
+ t.Parallel()
+
+ const host = "acct.dfs.core.windows.net"
+
+ p := newPrefixScopedIO(context.Background(), iceberg.Properties{
+ iceio.ADLSEndpoint:
"dfs.core.windows.net",
+ iceio.ADLSSharedKeyAccountName: "acct",
+ iceio.ADLSSharedKeyAccountKey: "stale-key",
+ iceio.ADLSSasTokenPrefix + host: "stale-sas",
+ keyAdlsSasExpiresAtMs + "." + host: "1000",
+ iceio.ADLSConnectionStringPrefix + "acct":
"stale-connection-string",
+ }, []StorageCredential{{
+ Prefix: "abfss://container@" + host + "/",
+ Config: iceberg.Properties{iceio.ADLSToken: "vended-token"},
+ }})
+
+ props := p.propertiesForLocation("abfss://container@" + host +
"/table/metadata.json")
+
+ // The vended token and unrelated tuning properties survive.
+ assert.Equal(t, "vended-token", props[iceio.ADLSToken])
+ assert.Equal(t, "dfs.core.windows.net", props[iceio.ADLSEndpoint])
+
+ // Everything that outranks adls.token in the Azure auth chain is gone,
so the
+ // vended token is what actually authenticates the read.
+ assert.NotContains(t, props, iceio.ADLSSharedKeyAccountName)
+ assert.NotContains(t, props, iceio.ADLSSharedKeyAccountKey)
+ assert.NotContains(t, props, iceio.ADLSSasTokenPrefix+host)
+ assert.NotContains(t, props, keyAdlsSasExpiresAtMs+"."+host)
+ assert.NotContains(t, props, iceio.ADLSConnectionStringPrefix+"acct")
+}
+
+func TestPrefixScopedIOKeepsCredentialsWhenVendedADLSTokenIsEmpty(t
*testing.T) {
+ t.Parallel()
+
+ const host = "acct.dfs.core.windows.net"
+
+ p := newPrefixScopedIO(context.Background(), iceberg.Properties{
+ iceio.ADLSSharedKeyAccountName: "acct",
+ iceio.ADLSSharedKeyAccountKey: "working-key",
+ iceio.ADLSSasTokenPrefix + host: "working-sas",
+ }, []StorageCredential{{
+ Prefix: "abfss://container@" + host + "/",
+ Config: iceberg.Properties{iceio.ADLSToken: ""},
+ }})
+
+ props := p.propertiesForLocation("abfss://container@" + host +
"/table/metadata.json")
+
+ // The Azure bucket factory skips an empty adls.token, so clearing on
key
+ // presence alone would discard these working credentials and fall
through to
+ // a weaker auth method.
+ assert.Equal(t, "acct", props[iceio.ADLSSharedKeyAccountName])
+ assert.Equal(t, "working-key", props[iceio.ADLSSharedKeyAccountKey])
+ assert.Equal(t, "working-sas", props[iceio.ADLSSasTokenPrefix+host])
+}
+
+func TestVendedCredsRefreshClearsStaleADLSAuth(t *testing.T) {
+ const host = "acct.dfs.core.windows.net"
+
+ var captured iceberg.Properties
+ iceio.Register("vended-adls-refresh-test", func(_ context.Context, _
*url.URL, props map[string]string) (iceio.IO, error) {
Review Comment:
This one registers into the global scheme registry with no cleanup and
captures props in a closure, while the sibling refresh tests go through the
`ioPropsRecorder` helper that unregisters for you. I'd reuse `rec.register(t,
scheme)` here too so a `-count=N` run or a future parallel sibling doesn't trip
a re-register or a race.
##########
website/src/configuration.md:
##########
@@ -227,15 +227,20 @@ Tuning properties:
### Azure Data Lake Storage / Blob
-Authentication is selected in the following order
(`io/gocloud/azure/azure.go`); the first applicable option wins, and if none of
1-4 apply the default credential chain (5) is used:
+Authentication is selected in the following order
(`io/gocloud/azure/azure.go`); the first applicable option wins, and if none of
1-5 apply the default credential chain (6) is used:
1. Shared key: a nonempty `adls.auth.shared-key.account.name` selects this
path and requires a nonempty `adls.auth.shared-key.account.key`.
2. Per-host SAS token: `adls.sas-token.<hostname>`, where `<hostname>` exactly
matches the storage account hostname (for example,
`myaccount.dfs.core.windows.net`).
3. Per-account connection string: `adls.connection-string.<account-name>` (for
example, `adls.connection-string.myaccount`).
-4. Managed identity: `adls.auth.managed-identity.enabled` set to exactly
`"true"` selects this path and uses `ManagedIdentityCredential` directly. Set
`adls.client-id` to select a user-assigned managed identity; otherwise, the
system-assigned managed identity is used.
-5. Default credential chain: `DefaultAzureCredential`, which includes managed
identity among its credential sources.
+4. Static access token: a nonempty `adls.token`, supplied without the `Bearer
` prefix.
+5. Managed identity: `adls.auth.managed-identity.enabled` set to exactly
`"true"` selects this path and uses `ManagedIdentityCredential` directly. Set
`adls.client-id` to select a user-assigned managed identity; otherwise, the
system-assigned managed identity is used.
+6. Default credential chain: `DefaultAzureCredential`, which includes managed
identity among its credential sources.
-> **Note on cross-client parity.** The two properties in step 4 diverge from
the
+`adls.token` accepts an OAuth2 access token for Azure Storage, including a
token supplied by a catalog. An empty value is ignored. A rejected or expired
token does not fall back to managed identity or the default credential chain.
+
+The token is static and is not refreshed. As in
[Java](https://github.com/apache/iceberg/blob/main/azure/src/main/java/org/apache/iceberg/azure/AzureProperties.java)
and
[PyIceberg](https://github.com/apache/iceberg-python/blob/main/pyiceberg/io/fsspec.py),
the credential reports a one-hour cache window to the Azure SDK each time the
SDK requests the token. This does not extend the token's actual validity: Azure
enforces its expiration. Callers must obtain a new token and recreate the
FileIO when it expires; changing the original properties map does not update an
existing FileIO.
Review Comment:
Could we note this caveat for the catalog-vended path specifically? The "not
refreshed" line is right for a FileIO built from static props, but a REST
catalog can vend `adls.token` via storage-credentials, and the refresher has no
expiry key for it — so a vended token only gets the generic TTL fallback (or
never refreshes without `fetchCreds`), and a token whose real lifetime is
shorter than the fallback will 401 silently.
It matches Java (the spec defines no ADLS token expiry key), so documenting
the limitation may be enough, but honoring `expiration-time` for the token
would be the stronger fix. wdyt?
##########
catalog/rest/vended_creds.go:
##########
@@ -474,6 +486,26 @@ func clearOverriddenCredentialProperties(props,
credentialConfig iceberg.Propert
delete(props, keyAdlsSasExpiresAtMs+"."+suffix)
}
}
+
+ if credentialConfig[iceio.ADLSToken] != "" {
Review Comment:
This clears credentials for every account, not just the one the vended token
covers — the loop drops all `adls.sas-token.*` and `adls.connection-string.*`
plus the shared key regardless of host. The comment justifies that as safe
because props is a per-location copy, which only holds inside
`propertiesForLocation`; at the three table-wide sites and
RefreshTableCredentials, `config` is the whole table/catalog map.
Since `resolveStorageCredentials` matches one credential by longest
metadata-location prefix and data files can sit under a different account, a
vended token for account A silently clears a working SAS or connection string
for account B, and that read falls through to the default chain or 403s.
I'd scope the deletes to the matched credential's account/host, or keep this
wipe confined to the per-location path, and correct the comment either way.
##########
catalog/rest/vended_creds.go:
##########
@@ -432,14 +432,26 @@ func (p *prefixScopedIO) propertiesForLocation(name
string) iceberg.Properties {
props := make(iceberg.Properties, len(p.baseProps))
maps.Copy(props, p.baseProps)
if credentialIndex >= 0 {
- credentialConfig := p.credentials[credentialIndex].Config
- clearOverriddenCredentialProperties(props, credentialConfig)
- maps.Copy(props, credentialConfig)
+ mergeVendedCredentials(props,
p.credentials[credentialIndex].Config)
}
return props
}
+// mergeVendedCredentials applies a matched storage credential over config,
+// first dropping any inherited property that the credential is meant to
+// replace. Every site that merges vended credentials must go through here:
+// merging with maps.Copy alone leaves stale higher-precedence credentials in
+// place, which then shadow the freshly vended ones.
+func mergeVendedCredentials(config, credentialConfig iceberg.Properties) {
Review Comment:
Routing every merge through one chokepoint is the right call. The thing I'd
shore up is that the shadow-key list `clearOverriddenCredentialProperties`
deletes is really a mirror of the Azure factory's precedence order, but it
lives a file away from that factory with nothing tying them together — if the
factory order ever changes, this breaks silently with no compile error.
I'd either colocate that list with the auth code or add a test that pins the
two, and leave a one-line note that S3/GCS clear on key presence while ADLS
clears on a nonempty value, since that asymmetry isn't obvious to the next
reader.
--
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]
