From 446e6f3444e8cf9099f69763aa59c5b262fb2e17 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 16 Oct 2026 19:48:32 -0700 Subject: [PATCH] s3tables: scope OIDC-derived account IDs to the token issuer (#11774) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * s3tables: scope OIDC-derived account IDs to the token issuer Table ownership keyed off the bare sub claim, so two identity providers issuing the same sub collided on one account - a caller could act on tables owned by a different provider's user. OIDC bearer tokens now carry the same issuer-namespaced (sub, iss) hash STS sessions already expose as aws:userid, and getAccountID prefers it over the raw claims. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3tables: keep legacy owners working and scrub client-subject claims - OIDC bearer tokens without a subject are rejected, and a token-supplied aws:userid claim is always overwritten with the verified (sub, iss) hash, so a forged claim can no longer stand in for the principal. - Owners written before issuer scoping hold a bare subject; permission checks now re-anchor that subject under the caller's issuer, so the same (sub, iss) pair keeps its ownership while scoped owners remain issuer-isolated. The token issuer is asserted into claims alongside sub and carried through PolicyContext. * s3tables: anchor legacy subjects to the configured issuer set * s3: enumerate trusted issuers from registered providers too * s3: fail closed when the OIDC provider store cannot be listed trustedOIDCIssuers kept the registered issuers when ListOIDCProviders failed, presenting a partial set as complete. A deployment trusting one registered issuer A plus a stored issuer B would, on a listing error, report only A — the single-issuer shape under which a legacy bare subject re-anchors — letting a caller from A claim a stored subject that belongs to B and gain owner access. With no provider store the registered providers are provably complete, so keep returning them; with a store, a failed listing means the real set is unknown, so return none. --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/s3api/s3_iam_bearer_trust_policy_test.go | 158 ++++++++++++++++++ weed/s3api/s3_iam_middleware.go | 72 ++++++++ weed/s3api/s3tables/handler.go | 60 +++++-- weed/s3api/s3tables/handler_bucket_create.go | 1 + .../handler_bucket_get_list_delete.go | 3 + weed/s3api/s3tables/handler_identity_test.go | 15 ++ weed/s3api/s3tables/handler_maintenance.go | 2 + weed/s3api/s3tables/handler_namespace.go | 5 + weed/s3api/s3tables/handler_policy.go | 9 + weed/s3api/s3tables/handler_table.go | 15 ++ weed/s3api/s3tables/handler_view.go | 1 + weed/s3api/s3tables/permissions.go | 36 +++- weed/s3api/s3tables/permissions_test.go | 33 ++++ 13 files changed, 393 insertions(+), 17 deletions(-) diff --git a/weed/s3api/s3_iam_bearer_trust_policy_test.go b/weed/s3api/s3_iam_bearer_trust_policy_test.go index 017e95f44..51209a450 100644 --- a/weed/s3api/s3_iam_bearer_trust_policy_test.go +++ b/weed/s3api/s3_iam_bearer_trust_policy_test.go @@ -6,8 +6,10 @@ import ( "crypto/rsa" "encoding/base64" "encoding/json" + "fmt" "net/http" "net/http/httptest" + "sync/atomic" "testing" "time" @@ -148,3 +150,159 @@ func TestOIDCBearerEnforcesRoleTrustPolicy(t *testing.T) { require.Equal(t, s3err.ErrNone, errCode) assert.Equal(t, "arn:aws:iam::role/BearerTrustedRole", identity.Principal) } + +// An OIDC bearer token's aws:userid claim is server-asserted from the +// verified (sub, iss) pair: a token-supplied value is overwritten, and a +// token without a subject is rejected outright. +func TestOIDCBearerUserIDIsIssuerScoped(t *testing.T) { + ctx := context.Background() + + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + jwks := map[string]interface{}{ + "keys": []map[string]interface{}{{ + "kty": "RSA", + "kid": "bearer-test-key", + "use": "sig", + "alg": "RS256", + "n": base64.RawURLEncoding.EncodeToString(key.PublicKey.N.Bytes()), + "e": "AQAB", + }}, + } + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/jwks" { + json.NewEncoder(w).Encode(jwks) + } + })) + defer server.Close() + + provider := oidc.NewOIDCProvider("test-oidc") + require.NoError(t, provider.Initialize(&oidc.OIDCConfig{ + Issuer: server.URL, + ClientID: "test-client", + JWKSUri: server.URL + "/jwks", + RoleMapping: &providers.RoleMapping{ + Rules: []providers.MappingRule{ + {Claim: "groups", Value: "trusted-group", Role: "arn:aws:iam::role/BearerTrustedRole"}, + }, + }, + })) + + iamManager := integration.NewIAMManager() + require.NoError(t, iamManager.Initialize(&integration.IAMConfig{ + STS: &sts.STSConfig{ + TokenDuration: sts.FlexibleDuration{Duration: time.Hour}, + MaxSessionLength: sts.FlexibleDuration{Duration: 12 * time.Hour}, + Issuer: "test-sts", + SigningKey: []byte("test-signing-key-32-characters-long"), + }, + Policy: &policy.PolicyEngineConfig{DefaultEffect: "Deny", StoreType: "memory"}, + Roles: &integration.RoleStoreConfig{StoreType: "memory"}, + }, func() string { return "localhost:8888" })) + require.NoError(t, iamManager.RegisterIdentityProvider(provider)) + require.NoError(t, iamManager.CreatePolicy(ctx, "", "BearerTestPolicy", &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{{ + Effect: "Allow", + Action: []string{"s3:*"}, + Resource: []string{"arn:aws:s3:::*", "arn:aws:s3:::*/*"}, + }}, + })) + require.NoError(t, iamManager.CreateRole(ctx, "", "BearerTrustedRole", &integration.RoleDefinition{ + RoleName: "BearerTrustedRole", + TrustPolicy: &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{{ + Effect: "Allow", + Principal: map[string]interface{}{"Federated": "test-oidc"}, + Action: []string{"sts:AssumeRoleWithWebIdentity"}, + }}, + }, + AttachedPolicies: []string{"BearerTestPolicy"}, + })) + + sign := func(extra map[string]interface{}) string { + claims := jwt.MapClaims{ + "iss": server.URL, + "aud": "test-client", + "exp": time.Now().Add(time.Hour).Unix(), + "iat": time.Now().Unix(), + "groups": []string{"trusted-group"}, + } + for k, v := range extra { + claims[k] = v + } + token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) + token.Header["kid"] = "bearer-test-key" + signed, err := token.SignedString(key) + require.NoError(t, err) + return signed + } + + s3iam := NewS3IAMIntegration(iamManager, "localhost:8888") + authenticate := func(token string) (*IAMIdentity, s3err.ErrorCode) { + req := httptest.NewRequest("GET", "/test-bucket/test-object", http.NoBody) + req.Header.Set("Authorization", "Bearer "+token) + return s3iam.AuthenticateJWT(ctx, req) + } + + identity, errCode := authenticate(sign(map[string]interface{}{ + "sub": "bearer-user", + "aws:userid": "admin", + })) + require.Equal(t, s3err.ErrNone, errCode) + assert.Equal(t, + sts.ComputeParentUser("bearer-user", server.URL), + identity.Claims["aws:userid"], + "the token-supplied aws:userid must be overwritten with the verified (sub, iss) hash") + assert.Equal(t, "bearer-user", identity.Claims["sub"]) + assert.Equal(t, server.URL, identity.Claims["iss"]) + assert.Equal(t, server.URL, identity.Claims["seaweed:oidc-issuers"], + "the registered provider's issuer must be asserted as trusted for legacy-owner anchoring") + + _, errCode = authenticate(sign(map[string]interface{}{"sub": ""})) + assert.Equal(t, s3err.ErrAccessDenied, errCode, "a token with no subject must not authenticate") +} + +// flakyListOIDCStore fails ListProviders on demand, standing in for a +// filer-backed provider store that cannot be read. +type flakyListOIDCStore struct { + integration.OIDCProviderStore + fail atomic.Bool +} + +func (s *flakyListOIDCStore) ListProviders(ctx context.Context, filerAddress string) ([]*integration.OIDCProviderRecord, error) { + if s.fail.Load() { + return nil, fmt.Errorf("list OIDC providers: simulated outage") + } + return s.OIDCProviderStore.ListProviders(ctx, filerAddress) +} + +// A failed store listing must not pass the registered providers off as the +// complete trusted set: hidden issuers could shrink the reported set to one +// entry, the only shape under which a legacy bare subject re-anchors. +func TestTrustedOIDCIssuersFailsClosedOnStoreError(t *testing.T) { + ctx := context.Background() + + iamManager := integration.NewIAMManager() + s3iam := NewS3IAMIntegration(iamManager, "") + + // No provider store at all: registered providers are the complete set — + // there is nowhere an issuer could hide behind a listing failure. + assert.Empty(t, s3iam.trustedOIDCIssuers(ctx)) + + mem := integration.NewMemoryOIDCProviderStore() + require.NoError(t, mem.StoreProvider(ctx, "", &integration.OIDCProviderRecord{ + ARN: "arn:aws:iam::111122223333:oidc-provider/idp.example.com", + URL: "https://idp.example.com", + })) + store := &flakyListOIDCStore{OIDCProviderStore: mem} + iamManager.SetOIDCProviderStore(store) + + assert.Equal(t, []string{"https://idp.example.com"}, s3iam.trustedOIDCIssuers(ctx)) + + store.fail.Store(true) + assert.Empty(t, s3iam.trustedOIDCIssuers(ctx), + "a failed listing must report no trusted issuers, not the partial registered set") +} diff --git a/weed/s3api/s3_iam_middleware.go b/weed/s3api/s3_iam_middleware.go index 86851b5b2..6861f48bf 100644 --- a/weed/s3api/s3_iam_middleware.go +++ b/weed/s3api/s3_iam_middleware.go @@ -126,6 +126,12 @@ func (s3iam *S3IAMIntegration) AuthenticateJWT(ctx context.Context, r *http.Requ return nil, s3err.ErrAccessDenied } + // A token without a subject has no issuer-scoped principal; letting it + // through would make the client-supplied claims below stand in for one. + if identity.UserID == "" { + return nil, s3err.ErrAccessDenied + } + // Extract role from OIDC identity if identity.RoleArn == "" { return nil, s3err.ErrAccessDenied @@ -162,6 +168,22 @@ func (s3iam *S3IAMIntegration) AuthenticateJWT(ctx context.Context, r *http.Requ // Set critical claims explicitly, overwriting any from attributes to ensure correctness. claims["sub"] = identity.UserID claims["role"] = identity.RoleArn + claims["iss"] = identity.Issuer + + // The bare sub collides across issuers; aws:userid carries the same + // (sub, iss) hash STS sessions already expose, so downstream ownership + // keys stay namespaced per identity provider. A claim the token itself + // supplies is always scrubbed, never adopted. + if parentUser := sts.ComputeParentUser(identity.UserID, identity.Issuer); parentUser != "" { + claims["aws:userid"] = parentUser + } else { + delete(claims, "aws:userid") + } + if issuers := s3iam.trustedOIDCIssuers(ctx); len(issuers) > 0 { + claims["seaweed:oidc-issuers"] = strings.Join(issuers, " ") + } else { + delete(claims, "seaweed:oidc-issuers") + } // Use real email address if available emailAddress := identity.UserID + "@oidc.local" @@ -425,6 +447,7 @@ type OIDCIdentity struct { UserID string RoleArn string Provider string + Issuer string Email string DisplayName string Groups []string @@ -488,6 +511,7 @@ func (s3iam *S3IAMIntegration) validateExternalOIDCToken(ctx context.Context, to UserID: externalIdentity.UserID, RoleArn: roleArn, Provider: fmt.Sprintf("%T", provider), // Use provider type as identifier + Issuer: externalIdentity.Issuer, Email: externalIdentity.Email, DisplayName: externalIdentity.DisplayName, Groups: externalIdentity.Groups, @@ -495,6 +519,54 @@ func (s3iam *S3IAMIntegration) validateExternalOIDCToken(ctx context.Context, to }, nil } +// trustedOIDCIssuers returns the issuer URLs of the OIDC providers the +// deployment is configured to trust. It is what a legacy (pre-scoping) bare +// subject can be re-anchored to: exactly one issuer makes the mapping +// unambiguous; anything else leaves it unknown, which callers must treat as +// fail-closed rather than guessing from the caller's issuer. +// +// The list is only safe while complete. Without a provider store the +// registered providers are the whole set — there is nowhere an issuer could +// hide. With a store, a failed listing leaves the real set unknown, and +// passing the registered subset off as complete could collapse a +// multi-issuer deployment to a single issuer: the one shape a bare subject +// re-anchors under, so a caller from that issuer could claim a stored +// subject that belongs to a provider hidden behind the error. Fail closed. +func (s3iam *S3IAMIntegration) trustedOIDCIssuers(ctx context.Context) []string { + if s3iam.iamManager == nil { + return nil + } + seen := make(map[string]bool) + var issuers []string + // Configured identity providers registered with STS cover deployments + // whose OIDC setup lives in config rather than the provider store. + if stsService := s3iam.iamManager.GetSTSService(); stsService != nil { + for _, p := range stsService.GetProviders() { + if issuer, ok := p.(interface{ GetIssuer() string }); ok { + if iss := issuer.GetIssuer(); iss != "" && !seen[iss] { + seen[iss] = true + issuers = append(issuers, iss) + } + } + } + } + if s3iam.iamManager.GetOIDCProviderStore() == nil { + return issuers + } + records, err := s3iam.iamManager.ListOIDCProviders(ctx) + if err != nil { + glog.V(2).Infof("trustedOIDCIssuers: %v", err) + return nil + } + for _, rec := range records { + if rec.URL != "" && !seen[rec.URL] { + seen[rec.URL] = true + issuers = append(issuers, rec.URL) + } + } + return issuers +} + // selectPrimaryRole simply picks the first role from the list // The OIDC provider should return roles in priority order (most important first) func (s3iam *S3IAMIntegration) selectPrimaryRole(roles []string, externalIdentity *providers.ExternalIdentity) string { diff --git a/weed/s3api/s3tables/handler.go b/weed/s3api/s3tables/handler.go index 3f2fedc32..73eb2381b 100644 --- a/weed/s3api/s3tables/handler.go +++ b/weed/s3api/s3tables/handler.go @@ -238,20 +238,12 @@ func (h *S3TablesHandler) getAccountID(r *http.Request) string { // Prefer stable claims from JWT/STS identities. Only "sub" is guaranteed durable per OIDC; // preferred_username is ergonomic but can rotate and may orphan ownership data, while email // is explicitly excluded to avoid storing PII in metadata. - claimsField := val.FieldByName("Claims") - if claimsField.IsValid() && claimsField.Kind() == reflect.Map && !claimsField.IsNil() && claimsField.Type().Key().Kind() == reflect.String { - for _, claimKey := range []string{"sub", "preferred_username"} { - claimVal := claimsField.MapIndex(reflect.ValueOf(claimKey)) - if !claimVal.IsValid() { - continue - } - if claimVal.Kind() == reflect.Interface && !claimVal.IsNil() { - claimVal = claimVal.Elem() - } - if claimVal.Kind() == reflect.String { - if principal := normalizePrincipalID(claimVal.String()); principal != "" { - return principal - } + if claims := identityClaimsMap(val); claims != nil { + // aws:userid is the issuer-namespaced (sub, iss) hash; a bare + // sub or preferred_username collides across providers. + for _, claimKey := range []string{"aws:userid", "sub", "preferred_username"} { + if principal := normalizePrincipalID(claimString(claims, claimKey)); principal != "" { + return principal } } } @@ -291,6 +283,46 @@ func (h *S3TablesHandler) getAccountID(r *http.Request) string { return h.accountID } +// identityClaimsMap extracts the Claims map from an identity value, using +// reflection to avoid an import cycle with the s3api identity type. +func identityClaimsMap(val reflect.Value) map[string]interface{} { + claimsField := val.FieldByName("Claims") + if !claimsField.IsValid() || claimsField.Kind() != reflect.Map || claimsField.IsNil() || claimsField.Type().Key().Kind() != reflect.String { + return nil + } + claims, ok := claimsField.Interface().(map[string]interface{}) + if !ok { + return nil + } + return claims +} + +func claimString(claims map[string]interface{}, key string) string { + v, ok := claims[key] + if !ok || v == nil { + return "" + } + str, _ := v.(string) + return str +} + +// trustedIssuers returns the OIDC issuers the auth layer asserted into the +// identity's claims; they re-anchor legacy owner checks in checkPermission. +func (h *S3TablesHandler) trustedIssuers(r *http.Request) []string { + identityRaw := s3_constants.GetIdentityFromContext(r) + if identityRaw == nil { + return nil + } + val := reflect.ValueOf(identityRaw) + if val.Kind() == reflect.Ptr { + val = val.Elem() + } + if val.Kind() != reflect.Struct { + return nil + } + return strings.Fields(claimString(identityClaimsMap(val), "seaweed:oidc-issuers")) +} + // normalizePrincipalID collapses ARN and identity strings to a key that is stable within a single account. // WARNING: this assumes identity names are unique per account; distinct principals such as // arn:aws:iam::111:user/alice and arn:aws:iam::222:user/alice will both normalize to "alice". diff --git a/weed/s3api/s3tables/handler_bucket_create.go b/weed/s3api/s3tables/handler_bucket_create.go index fd42c2ede..5191db730 100644 --- a/weed/s3api/s3tables/handler_bucket_create.go +++ b/weed/s3api/s3tables/handler_bucket_create.go @@ -65,6 +65,7 @@ func (h *S3TablesHandler) handleCreateTableBucket(w http.ResponseWriter, r *http owner = DefaultAccountID } if !CheckPermissionWithContext("CreateTableBucket", principal, owner, "", "", &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), }) { diff --git a/weed/s3api/s3tables/handler_bucket_get_list_delete.go b/weed/s3api/s3tables/handler_bucket_get_list_delete.go index a85e151dc..57fb01cf0 100644 --- a/weed/s3api/s3tables/handler_bucket_get_list_delete.go +++ b/weed/s3api/s3tables/handler_bucket_get_list_delete.go @@ -70,6 +70,7 @@ func (h *S3TablesHandler) handleGetTableBucket(w http.ResponseWriter, r *http.Re principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("GetTableBucket", principal, metadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), @@ -192,6 +193,7 @@ func (h *S3TablesHandler) handleListTableBuckets(w http.ResponseWriter, r *http. bucketARN := h.generateTableBucketARN(metadata.OwnerAccountID, entry.Entry.Name) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("GetTableBucket", accountID, metadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: entry.Entry.Name, IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), @@ -296,6 +298,7 @@ func (h *S3TablesHandler) handleDeleteTableBucket(w http.ResponseWriter, r *http principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("DeleteTableBucket", principal, metadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), diff --git a/weed/s3api/s3tables/handler_identity_test.go b/weed/s3api/s3tables/handler_identity_test.go index 12122a81e..b4ab8d9a0 100644 --- a/weed/s3api/s3tables/handler_identity_test.go +++ b/weed/s3api/s3tables/handler_identity_test.go @@ -171,3 +171,18 @@ func TestGetAccountIDNormalizesAccountIDARN(t *testing.T) { assert.Equal(t, "bob", h.getAccountID(req), "expected ARN account ID to be normalized to the suffix") } + +func TestGetAccountIDPrefersIssuerScopedUserID(t *testing.T) { + h := NewS3TablesHandler() + id := &testIdentity{ + Claims: map[string]interface{}{ + "aws:userid": "issuerA-hash", + "sub": "alice-sub", + "preferred_username": "alice", + }, + } + req := httptest.NewRequest(http.MethodGet, "/", nil) + req = req.WithContext(s3_constants.SetIdentityInContext(req.Context(), id)) + + assert.Equal(t, "issuerA-hash", h.getAccountID(req), "aws:userid should win over a bare sub") +} diff --git a/weed/s3api/s3tables/handler_maintenance.go b/weed/s3api/s3tables/handler_maintenance.go index f80f2ba09..1bbe57b94 100644 --- a/weed/s3api/s3tables/handler_maintenance.go +++ b/weed/s3api/s3tables/handler_maintenance.go @@ -349,6 +349,7 @@ func (h *S3TablesHandler) authorizeMaintenanceBucket(r *http.Request, filerClien bucketARN := h.generateTableBucketARN(bucketMetadata.OwnerAccountID, bucketName) principal := h.getAccountID(r) if !CheckPermissionWithContext(operation, principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, IdentityActions: getIdentityActions(r), DefaultAllow: h.defaultAllowFor(r), @@ -385,6 +386,7 @@ func (h *S3TablesHandler) authorizeMaintenanceTable(r *http.Request, filerClient tableARN := h.generateTableARN(metadata.OwnerAccountID, bucketName, namespaceName+"/"+tableName) principal := h.getAccountID(r) if !CheckPermissionWithContext(operation, principal, metadata.OwnerAccountID, bucketPolicy, tableARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableName: tableName, diff --git a/weed/s3api/s3tables/handler_namespace.go b/weed/s3api/s3tables/handler_namespace.go index 33213ab2d..ea54c2ede 100644 --- a/weed/s3api/s3tables/handler_namespace.go +++ b/weed/s3api/s3tables/handler_namespace.go @@ -114,6 +114,7 @@ func (h *S3TablesHandler) handleCreateNamespace(w http.ResponseWriter, r *http.R identityActions := getIdentityActions(r) glog.Infof("S3Tables: CreateNamespace permission check - principal=%s, owner=%s, actions=%v", principal, bucketMetadata.OwnerAccountID, identityActions) if !CheckPermissionWithContext("CreateNamespace", principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableBucketTags: bucketTags, @@ -255,6 +256,7 @@ func (h *S3TablesHandler) handleGetNamespace(w http.ResponseWriter, r *http.Requ principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("GetNamespace", principal, metadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableBucketTags: bucketTags, @@ -345,6 +347,7 @@ func (h *S3TablesHandler) handleUpdateNamespace(w http.ResponseWriter, r *http.R principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("UpdateNamespace", principal, metadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableBucketTags: bucketTags, @@ -447,6 +450,7 @@ func (h *S3TablesHandler) handleListNamespaces(w http.ResponseWriter, r *http.Re principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("ListNamespaces", principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, TableBucketTags: bucketTags, IdentityActions: identityActions, @@ -631,6 +635,7 @@ func (h *S3TablesHandler) handleDeleteNamespace(w http.ResponseWriter, r *http.R principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("DeleteNamespace", principal, metadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableBucketTags: bucketTags, diff --git a/weed/s3api/s3tables/handler_policy.go b/weed/s3api/s3tables/handler_policy.go index c749412ef..0f1aed19d 100644 --- a/weed/s3api/s3tables/handler_policy.go +++ b/weed/s3api/s3tables/handler_policy.go @@ -92,6 +92,7 @@ func (h *S3TablesHandler) handlePutTableBucketPolicy(w http.ResponseWriter, r *h principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("PutTableBucketPolicy", principal, bucketMetadata.OwnerAccountID, "", bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), @@ -170,6 +171,7 @@ func (h *S3TablesHandler) handleGetTableBucketPolicy(w http.ResponseWriter, r *h principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("GetTableBucketPolicy", principal, bucketMetadata.OwnerAccountID, string(policy), bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), @@ -246,6 +248,7 @@ func (h *S3TablesHandler) handleDeleteTableBucketPolicy(w http.ResponseWriter, r principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("DeleteTableBucketPolicy", principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, IdentityActions: identityActions, DefaultAllow: h.defaultAllowFor(r), @@ -345,6 +348,7 @@ func (h *S3TablesHandler) handlePutTablePolicy(w http.ResponseWriter, r *http.Re principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("PutTablePolicy", principal, metadata.OwnerAccountID, bucketPolicy, tableARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableName: tableName, @@ -453,6 +457,7 @@ func (h *S3TablesHandler) handleGetTablePolicy(w http.ResponseWriter, r *http.Re principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("GetTablePolicy", principal, metadata.OwnerAccountID, bucketPolicy, tableARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableName: tableName, @@ -543,6 +548,7 @@ func (h *S3TablesHandler) handleDeleteTablePolicy(w http.ResponseWriter, r *http principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("DeleteTablePolicy", principal, metadata.OwnerAccountID, bucketPolicy, tableARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableName: tableName, @@ -640,6 +646,7 @@ func (h *S3TablesHandler) handleTagResource(w http.ResponseWriter, r *http.Reque principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("TagResource", principal, ownerAccountID, bucketPolicy, resourceARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, TableBucketTags: bucketTags, RequestTags: req.Tags, @@ -760,6 +767,7 @@ func (h *S3TablesHandler) handleListTagsForResource(w http.ResponseWriter, r *ht principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("ListTagsForResource", principal, ownerAccountID, bucketPolicy, resourceARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, TableBucketTags: bucketTags, ResourceTags: tags, @@ -867,6 +875,7 @@ func (h *S3TablesHandler) handleUntagResource(w http.ResponseWriter, r *http.Req principal := h.getAccountID(r) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("UntagResource", principal, ownerAccountID, bucketPolicy, resourceARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, TableBucketTags: bucketTags, TagKeys: req.TagKeys, diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index 31fdc77fe..f76ed8879 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -420,6 +420,7 @@ func (h *S3TablesHandler) handleGetTable(w http.ResponseWriter, r *http.Request, bucketARN := h.generateTableBucketARN(bucketMetadata.OwnerAccountID, bucketName) identityActions := getIdentityActions(r) tableAllowed := CheckPermissionWithContext("GetTable", accountID, metadata.OwnerAccountID, tablePolicy, tableARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespace, TableName: tableName, @@ -429,6 +430,7 @@ func (h *S3TablesHandler) handleGetTable(w http.ResponseWriter, r *http.Request, DefaultAllow: h.defaultAllowFor(r), }) bucketAllowed := CheckPermissionWithContext("GetTable", accountID, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespace, TableName: tableName, @@ -561,6 +563,7 @@ func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Reques bucketARN := h.generateTableBucketARN(bucketMeta.OwnerAccountID, bucketName) identityActions := getIdentityActions(r) nsAllowed := CheckPermissionWithContext("ListTables", accountID, nsMeta.OwnerAccountID, namespacePolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableBucketTags: bucketTags, @@ -568,6 +571,7 @@ func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Reques DefaultAllow: h.defaultAllowFor(r), }) bucketAllowed := CheckPermissionWithContext("ListTables", accountID, bucketMeta.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableBucketTags: bucketTags, @@ -614,6 +618,7 @@ func (h *S3TablesHandler) handleListTables(w http.ResponseWriter, r *http.Reques bucketARN := h.generateTableBucketARN(bucketMeta.OwnerAccountID, bucketName) identityActions := getIdentityActions(r) if !CheckPermissionWithContext("ListTables", accountID, bucketMeta.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, TableBucketTags: bucketTags, IdentityActions: identityActions, @@ -951,6 +956,7 @@ func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Reque principal := h.getAccountID(r) identityActions := getIdentityActions(r) tableAllowed := CheckPermissionWithContext("DeleteTable", principal, metadata.OwnerAccountID, tablePolicy, tableARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableName: tableName, @@ -960,6 +966,7 @@ func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Reque DefaultAllow: h.defaultAllowFor(r), }) bucketAllowed := CheckPermissionWithContext("DeleteTable", principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableName: tableName, @@ -1131,6 +1138,7 @@ func (h *S3TablesHandler) readTableMetadata(ctx context.Context, client filer_pb // cannot see the entry keeps the not-found veil. func (h *S3TablesHandler) canReadEntry(readOp, principal, tableOwner, bucketOwner, tablePolicy, bucketPolicy, tableARN, bucketARN, bucketName, namespace, name string, bucketTags, tableTags map[string]string, identityActions []string, r *http.Request) bool { ctx := &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespace, TableName: name, @@ -1312,6 +1320,7 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ principal := h.getAccountID(r) identityActions := getIdentityActions(r) tableAllowed := CheckPermissionWithContext(kind.renameOp, principal, metadata.OwnerAccountID, tablePolicy, tableARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: srcNamespace, TableName: srcName, @@ -1321,6 +1330,7 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ DefaultAllow: h.defaultAllowFor(r), }) bucketAllowed := CheckPermissionWithContext(kind.renameOp, principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: srcNamespace, TableName: srcName, @@ -1385,6 +1395,7 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ // caller move tables into namespaces they don't control). Denials report the // same not-found as a missing destination namespace. destNamespaceAllowed := CheckPermissionWithContext(kind.createOp, principal, destNamespaceMetadata.OwnerAccountID, destNamespacePolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: destNamespace, TableName: destName, @@ -1393,6 +1404,7 @@ func (h *S3TablesHandler) renameCatalogEntry(w http.ResponseWriter, r *http.Requ DefaultAllow: h.defaultAllowFor(r), }) destBucketAllowed := CheckPermissionWithContext(kind.createOp, principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: destNamespace, TableName: destName, @@ -1596,6 +1608,7 @@ func (h *S3TablesHandler) handleUpdateTable(w http.ResponseWriter, r *http.Reque identityActions := getIdentityActions(r) tableAllowed := CheckPermissionWithContext("UpdateTable", principal, metadata.OwnerAccountID, tablePolicy, tableARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableName: tableName, @@ -1605,6 +1618,7 @@ func (h *S3TablesHandler) handleUpdateTable(w http.ResponseWriter, r *http.Reque DefaultAllow: h.defaultAllowFor(r), }) bucketAllowed := CheckPermissionWithContext("UpdateTable", principal, bucketMetadata.OwnerAccountID, bucketPolicy, bucketARN, &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableName: tableName, @@ -1809,6 +1823,7 @@ func (h *S3TablesHandler) authorizeCreateTable(w http.ResponseWriter, r *http.Re bucketARN := h.generateTableBucketARN(bucketMetadata.OwnerAccountID, bucketName) identityActions := getIdentityActions(r) policyContext := &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespaceName, TableName: tableName, diff --git a/weed/s3api/s3tables/handler_view.go b/weed/s3api/s3tables/handler_view.go index cd8144a0a..70cb37d37 100644 --- a/weed/s3api/s3tables/handler_view.go +++ b/weed/s3api/s3tables/handler_view.go @@ -635,6 +635,7 @@ func (h *S3TablesHandler) loadAuthContext(r *http.Request, filerClient FilerClie func (h *S3TablesHandler) authorizeViewOp(r *http.Request, operation, principal, resourceOwner, bucketOwner, resourcePolicy, bucketPolicy, resourceARN, bucketName, namespace, viewName string, bucketTags map[string]string) bool { identityActions := getIdentityActions(r) ctx := &PolicyContext{ + TrustedIssuers: h.trustedIssuers(r), TableBucketName: bucketName, Namespace: namespace, TableName: viewName, diff --git a/weed/s3api/s3tables/permissions.go b/weed/s3api/s3tables/permissions.go index 08a666c83..f00e5ce2c 100644 --- a/weed/s3api/s3tables/permissions.go +++ b/weed/s3api/s3tables/permissions.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/seaweedfs/seaweedfs/weed/glog" + "github.com/seaweedfs/seaweedfs/weed/iam/sts" "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/seaweedfs/seaweedfs/weed/util/wildcard" @@ -88,6 +89,11 @@ type PolicyContext struct { KMSKeyArn string StorageClass string DefaultAllow bool + // TrustedIssuers lists the OIDC issuers the deployment is configured to + // trust, used to re-anchor owners/principals written before issuer-scoped + // account IDs. A bare subject is only resolvable when exactly one issuer + // is configured; more than one leaves it ambiguous, so it must not match. + TrustedIssuers []string } // CheckPermissionWithContext checks permission with optional resource and condition context. @@ -109,8 +115,8 @@ func CheckPermissionWithContext(operation, principal, owner, resourcePolicy, res func checkPermission(operation, principal, owner, resourcePolicy, resourceARN string, ctx *PolicyContext) bool { - // Owner always has permission - if principal == owner { + // Owner always has permission. + if principal == owner || matchesLegacySubject(owner, principal, ctx) { return true } @@ -145,7 +151,7 @@ func checkPermission(operation, principal, owner, resourcePolicy, resourceARN st for _, stmt := range policy.Statement { // Check if principal matches - if !matchesPrincipal(stmt.Principal, principal) { + if !matchesCaller(stmt.Principal, principal, ctx) { continue } @@ -225,6 +231,30 @@ func hasAdminAction(actions []string) bool { return false } +// matchesCaller matches a statement's Principal against the caller's scoped +// principal or, for policies written before issuer scoping, a bare subject +// that resolves to it under the deployment's single trusted issuer. +func matchesCaller(principalSpec interface{}, principal string, ctx *PolicyContext) bool { + if matchesPrincipal(principalSpec, principal) { + return true + } + legacy, ok := principalSpec.(string) + return ok && matchesLegacySubject(legacy, principal, ctx) +} + +// matchesLegacySubject reports whether a bare subject stored before issuer +// scoping belongs to the caller. Re-anchoring is only sound when exactly one +// OIDC issuer is configured: the stored subject is hashed under that issuer +// and compared to the caller's scoped principal. With multiple issuers a bare +// subject is ambiguous — matching it under the caller's own issuer would let +// a same-sub identity from any configured issuer claim it. +func matchesLegacySubject(legacy, principal string, ctx *PolicyContext) bool { + if ctx == nil || len(ctx.TrustedIssuers) != 1 { + return false + } + return sts.ComputeParentUser(legacy, ctx.TrustedIssuers[0]) == principal +} + // matchesPrincipal checks if the principal matches the statement's principal func matchesPrincipal(principalSpec interface{}, principal string) bool { if principalSpec == nil { diff --git a/weed/s3api/s3tables/permissions_test.go b/weed/s3api/s3tables/permissions_test.go index 05233f56f..d19712164 100644 --- a/weed/s3api/s3tables/permissions_test.go +++ b/weed/s3api/s3tables/permissions_test.go @@ -3,6 +3,8 @@ package s3tables import ( "encoding/json" "testing" + + "github.com/seaweedfs/seaweedfs/weed/iam/sts" ) func TestMatchesActionPattern(t *testing.T) { @@ -251,3 +253,34 @@ func TestCheckPermissionWithDefaultAllow(t *testing.T) { }) } } + +// Owners written before issuer scoping hold a bare subject. Re-anchoring it +// is only unambiguous when exactly one OIDC issuer is configured; under two +// or more issuers a bare subject could belong to either, so it must not +// match anyone. +func TestCheckPermissionLegacyOwnerScopedToIssuer(t *testing.T) { + owner := "alice-sub" + principal := sts.ComputeParentUser(owner, "corp-idp") + + if !CheckPermissionWithContext("GetTable", principal, owner, "", "arn", &PolicyContext{ + TrustedIssuers: []string{"corp-idp"}, + }) { + t.Error("single-issuer deployments must keep recognizing the legacy owner") + } + + if CheckPermissionWithContext("GetTable", principal, owner, "", "arn", &PolicyContext{ + TrustedIssuers: []string{"corp-idp", "evil-idp"}, + }) { + t.Error("a bare subject is ambiguous across multiple issuers and must not match") + } + + if CheckPermissionWithContext("GetTable", sts.ComputeParentUser(owner, "evil-idp"), owner, "", "arn", &PolicyContext{ + TrustedIssuers: []string{"corp-idp", "evil-idp"}, + }) { + t.Error("a same-sub caller from another issuer must not claim the legacy owner") + } + + if CheckPermissionWithContext("GetTable", principal, owner, "", "arn", &PolicyContext{}) { + t.Error("legacy owner matching must not engage without a trusted issuer") + } +}