This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new d1800ed88 fix(rest): layer vended credentials onto the base config
everywhere (#2417)
d1800ed88 is described below
commit d1800ed88e213e44b4023ef65de82319442d61d8
Author: Revanth <[email protected]>
AuthorDate: Wed Nov 4 11:17:02 2026 -0600
fix(rest): layer vended credentials onto the base config everywhere (#2417)
---
catalog/rest/refresh_table_credentials_test.go | 28 +++++++++
catalog/rest/vended_creds.go | 69 +++++++--------------
catalog/rest/vended_creds_test.go | 83 +++++++++++++++++++++++++-
3 files changed, 130 insertions(+), 50 deletions(-)
diff --git a/catalog/rest/refresh_table_credentials_test.go
b/catalog/rest/refresh_table_credentials_test.go
index 7dc37b9be..2692011ee 100644
--- a/catalog/rest/refresh_table_credentials_test.go
+++ b/catalog/rest/refresh_table_credentials_test.go
@@ -832,3 +832,31 @@ func TestTableResponseLayersPartialS3Credential(t
*testing.T) {
assert.Equal(t, "vended-token", props[iceio.S3SessionToken],
name)
}
}
+
+// TestTableResponseDropsStaleS3SessionTokenForVendedKeyPair covers the
+// table-wide merge sites: a session token from table config only works with
+// the key pair it was issued with, so a vended key pair without one drops it.
+func TestTableResponseDropsStaleS3SessionTokenForVendedKeyPair(t *testing.T) {
+ const scheme = "restcreds-table-s3-stale"
+
+ expiresAt := strconv.FormatInt(time.Now().Add(time.Hour).UnixMilli(),
10)
+ got := tableResponseFSProps(t, scheme,
+
scheme+"://warehouse/database/table/metadata/00000-a.metadata.json",
+ map[string]string{
+ iceio.S3AccessKeyID: "config-key",
+ iceio.S3SecretAccessKey: "config-secret",
+ iceio.S3SessionToken: "config-token",
+ "s3.session-token-expires-at-ms": expiresAt,
+ },
+ map[string]string{
+ iceio.S3AccessKeyID: "vended-key",
+ iceio.S3SecretAccessKey: "vended-secret",
+ })
+
+ for name, props := range got {
+ assert.Equal(t, "vended-key", props[iceio.S3AccessKeyID], name)
+ assert.Equal(t, "vended-secret",
props[iceio.S3SecretAccessKey], name)
+ assert.NotContains(t, props, iceio.S3SessionToken, name)
+ assert.NotContains(t, props, "s3.session-token-expires-at-ms",
name)
+ }
+}
diff --git a/catalog/rest/vended_creds.go b/catalog/rest/vended_creds.go
index 81e190f02..d3b4a8229 100644
--- a/catalog/rest/vended_creds.go
+++ b/catalog/rest/vended_creds.go
@@ -452,9 +452,7 @@ 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)
- mergeVendedCredentials(props, credentialConfig, name)
+ mergeVendedCredentials(props,
p.credentials[credentialIndex].Config, name)
}
return props
@@ -462,19 +460,28 @@ func (p *prefixScopedIO) propertiesForLocation(name
string) iceberg.Properties {
// mergeVendedCredentials layers vended storage credential properties over
// config, the properties an IO for location is built from. Every site that
-// merges storage credentials goes through here.
+// merges storage credentials goes through here. Properties the credential
+// does not vend are kept, so user and catalog config fill in around it.
//
-// A nonempty adls.token is the one case that also removes properties:
-// credentials the Azure FileIO selects ahead of the token would shadow it and
-// the read would 403; see adlsauth.ClearTokenShadowingCredentials. config is
-// always a copy built for one IO, and the Azure FileIO serves only location's
-// storage account, so the removals cannot reach another account's IO.
+// Two cases also remove properties:
//
-// Unlike the S3 and GCS clearing in clearOverriddenCredentialProperties, which
-// keys on property presence, this keys on a nonempty value: the Azure FileIO
-// skips an empty adls.token, so clearing for one would discard working
-// credentials.
+// - A vended S3 access key drops the base session token and its expiry. An
+// STS session token only works with the key pair it was issued with, so a
+// token left from the base config would get the read a 403. A token and
+// expiry vended with the key are copied back in below.
+// - A nonempty adls.token drops the credentials the Azure FileIO selects
+// ahead of the token, which would shadow it and get the read a 403; see
+// adlsauth.ClearTokenShadowingCredentials. config is always a copy built
+// for one IO, and the Azure FileIO serves only location's storage account,
+// so the removals cannot reach another account's IO. This keys on a
+// nonempty value rather than presence: the Azure FileIO skips an empty
+// adls.token, so clearing for one would discard working credentials.
func mergeVendedCredentials(config, credentialConfig iceberg.Properties,
location string) {
+ if _, ok := credentialConfig[iceio.S3AccessKeyID]; ok {
+ delete(config, iceio.S3SessionToken)
+ delete(config, keyS3TokenExpiresAtMs)
+ }
+
if credentialConfig[iceio.ADLSToken] != "" {
if acct, ok := adlsauth.AccountFor(location); ok {
adlsauth.ClearTokenShadowingCredentials(config, acct)
@@ -484,42 +491,6 @@ func mergeVendedCredentials(config, credentialConfig
iceberg.Properties, locatio
maps.Copy(config, credentialConfig)
}
-// clearOverriddenCredentialProperties prevents a matched credential from being
-// combined with optional fields from another credential. In particular, an S3
-// access/secret pair without a session token must not inherit a stale token
-// from the table or catalog configuration.
-func clearOverriddenCredentialProperties(props, credentialConfig
iceberg.Properties) {
- _, hasS3AccessKey := credentialConfig[iceio.S3AccessKeyID]
- _, hasS3SecretKey := credentialConfig[iceio.S3SecretAccessKey]
- _, hasS3SessionToken := credentialConfig[iceio.S3SessionToken]
- if hasS3AccessKey || hasS3SecretKey || hasS3SessionToken {
- delete(props, iceio.S3AccessKeyID)
- delete(props, iceio.S3SecretAccessKey)
- delete(props, iceio.S3SessionToken)
- delete(props, keyS3TokenExpiresAtMs)
- }
-
- _, hasGCSOAuthToken := credentialConfig[iceio.GCSOAuthToken]
- _, hasGCSOAuthExpiry := credentialConfig[iceio.GCSOAuthExpiresAt]
- if hasGCSOAuthToken || hasGCSOAuthExpiry {
- delete(props, iceio.GCSOAuthToken)
- delete(props, iceio.GCSOAuthExpiresAt)
- }
-
- for key := range credentialConfig {
- switch {
- case strings.HasPrefix(key, iceio.ADLSSasTokenPrefix):
- suffix := strings.TrimPrefix(key,
iceio.ADLSSasTokenPrefix)
- delete(props, iceio.ADLSSasTokenPrefix+suffix)
- delete(props, keyAdlsSasExpiresAtMs+"."+suffix)
- case strings.HasPrefix(key, keyAdlsSasExpiresAtMs+"."):
- suffix := strings.TrimPrefix(key,
keyAdlsSasExpiresAtMs+".")
- delete(props, iceio.ADLSSasTokenPrefix+suffix)
- delete(props, keyAdlsSasExpiresAtMs+"."+suffix)
- }
- }
-}
-
func scopedFilesystemKey(credentialIndex int, location string) string {
parsed, err := url.Parse(location)
if err != nil {
diff --git a/catalog/rest/vended_creds_test.go
b/catalog/rest/vended_creds_test.go
index d19173d10..61f014b11 100644
--- a/catalog/rest/vended_creds_test.go
+++ b/catalog/rest/vended_creds_test.go
@@ -725,7 +725,7 @@ func TestPrefixScopedIOUsesLongestCredentialPerLocation(t
*testing.T) {
assert.Equal(t, "yes",
p.propertiesForLocation("s3://other/table/file.parquet")["base"])
}
-func TestPrefixScopedIOReplacesS3CredentialAtomically(t *testing.T) {
+func TestPrefixScopedIODropsStaleS3SessionTokenForVendedKeyPair(t *testing.T) {
t.Parallel()
p := newPrefixScopedIO(context.Background(), iceberg.Properties{
@@ -750,6 +750,87 @@ func TestPrefixScopedIOReplacesS3CredentialAtomically(t
*testing.T) {
assert.NotContains(t, props, keyS3TokenExpiresAtMs)
}
+// TestPrefixScopedIOLayersPartialS3Credential matches the table-wide sites
+// (TestTableResponseLayersPartialS3Credential): a credential that vends only
+// a session token keeps the base key pair.
+func TestPrefixScopedIOLayersPartialS3Credential(t *testing.T) {
+ t.Parallel()
+
+ p := newPrefixScopedIO(context.Background(), iceberg.Properties{
+ iceio.S3AccessKeyID: "static-key",
+ iceio.S3SecretAccessKey: "static-secret",
+ }, []StorageCredential{{
+ Prefix: "s3://bucket/data/",
+ Config: iceberg.Properties{iceio.S3SessionToken:
"vended-token"},
+ }})
+
+ props := p.propertiesForLocation("s3://bucket/data/file.parquet")
+ assert.Equal(t, "static-key", props[iceio.S3AccessKeyID])
+ assert.Equal(t, "static-secret", props[iceio.S3SecretAccessKey])
+ assert.Equal(t, "vended-token", props[iceio.S3SessionToken])
+}
+
+func TestMergeVendedCredentialsS3SessionToken(t *testing.T) {
+ t.Parallel()
+
+ base := iceberg.Properties{
+ iceio.S3AccessKeyID: "base-key",
+ iceio.S3SecretAccessKey: "base-secret",
+ iceio.S3SessionToken: "base-token",
+ keyS3TokenExpiresAtMs: "1000",
+ }
+
+ tests := []struct {
+ name string
+ vended iceberg.Properties
+ want iceberg.Properties
+ }{
+ {
+ name: "key pair without token drops base token and
expiry",
+ vended: iceberg.Properties{iceio.S3AccessKeyID: "key",
iceio.S3SecretAccessKey: "secret"},
+ want: iceberg.Properties{iceio.S3AccessKeyID: "key",
iceio.S3SecretAccessKey: "secret"},
+ },
+ {
+ name: "key pair with token drops base expiry",
+ vended: iceberg.Properties{
+ iceio.S3AccessKeyID: "key",
iceio.S3SecretAccessKey: "secret", iceio.S3SessionToken: "token",
+ },
+ want: iceberg.Properties{
+ iceio.S3AccessKeyID: "key",
iceio.S3SecretAccessKey: "secret", iceio.S3SessionToken: "token",
+ },
+ },
+ {
+ name: "key pair with token and expiry replaces both",
+ vended: iceberg.Properties{
+ iceio.S3AccessKeyID: "key",
iceio.S3SecretAccessKey: "secret",
+ iceio.S3SessionToken: "token",
keyS3TokenExpiresAtMs: "2000",
+ },
+ want: iceberg.Properties{
+ iceio.S3AccessKeyID: "key",
iceio.S3SecretAccessKey: "secret",
+ iceio.S3SessionToken: "token",
keyS3TokenExpiresAtMs: "2000",
+ },
+ },
+ {
+ name: "token only layers onto base key pair",
+ vended: iceberg.Properties{iceio.S3SessionToken:
"token"},
+ want: iceberg.Properties{
+ iceio.S3AccessKeyID: "base-key",
iceio.S3SecretAccessKey: "base-secret",
+ iceio.S3SessionToken: "token",
keyS3TokenExpiresAtMs: "1000",
+ },
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ t.Parallel()
+
+ config := maps.Clone(base)
+ mergeVendedCredentials(config, tt.vended,
"s3://bucket/table/metadata.json")
+ assert.Equal(t, tt.want, config)
+ })
+ }
+}
+
func TestPrefixScopedIOClearsStaleADLSAuthForVendedToken(t *testing.T) {
t.Parallel()
