From 6483583491b57fe172b9161379f9919d051de6c8 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 5 May 2026 13:06:53 -0700 Subject: [PATCH] feat(iam): account-scoped OIDC providers (Phase 3c) (#9323) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(iam): claim-based policy mode for AssumeRoleWithWebIdentity When the caller passes the sentinel RoleArn arn:aws:iam:::role/sts-claim-based (or omits it entirely) and the matched OIDC provider has policyClaim set, mint a session whose effective policies come from that JWT claim instead of from a server-side role mapping. Accepts string, comma-separated string, or array shapes — MinIO-compatible behaviour for IDPs that already attach policies to the user. Trust-policy validation is skipped in claim-mode: the IDP is the sole authority for both authentication and authorization, mirroring the contract MinIO documents for its DummyRoleARN flow. Concrete-role mode is unchanged and still requires the role definition + trust policy. * fix(iam): trim policy-claim array elements + clean up stale comments Three medium-priority cleanups gemini flagged on the claim-based path: - extractClaimPolicies's array branch was leaving whitespace on each element while the string/comma-separated branch trimmed via splitPolicyClaimString. An IDP that emits ["readonly", " billing "] would create a "billing" policy lookup that didn't match the stored name. Trim every array element, drop empties. - The "synthetic ARN keyed on the session name" comment was wrong — effectiveRoleArn here is the literal sentinel; it's the assumed-role ARN generated downstream that's session-keyed. Reword. - The empty if/else block at the start of validateAssumeRoleWithWebIdentityRequest existed only to host a comment about deferred validation; the comment now lives in the function godoc and the empty branch is gone. Addresses three gemini medium reviews on PR #9322. * feat(iam): account-scoped OIDC providers Add OIDCProviderRecord.AccountID enforcement: when a role lives in account A, the OIDC provider validating the assume-role token must be either global (AccountID="") or also live in account A. Cross-account use is rejected at the IAM-manager layer before reaching the trust policy validator. OIDCProviderStore gains GetProviderByIssuerAndAccount; both the in- memory and filer-backed stores implement it. Static-config-only deployments are unaffected since they don't populate the store. * fix(iam): account-scoped lookup for cross-account check enforceProviderAccountScope was calling GetProviderByIssuer, which returns the first match arbitrarily when multiple providers share an issuer (one global + one per tenant is the canonical setup). On a two-record collision the wrong record could come back first and falsely reject a valid same-account or global-provider request. Use GetProviderByIssuerAndAccount as the primary lookup so the filter happens in the store. On miss, fall back to GetProviderByIssuer purely to distinguish "issuer entirely unknown" (let the STS layer reject) from "issuer registered in a different account" (surface a precise cross-account error). Addresses gemini high-priority review on PR #9323. --- weed/iam/integration/account_scope_test.go | 72 ++++++++++++++++++++ weed/iam/integration/iam_manager.go | 73 +++++++++++++++++++++ weed/iam/integration/oidc_provider_store.go | 43 ++++++++++++ 3 files changed, 188 insertions(+) create mode 100644 weed/iam/integration/account_scope_test.go diff --git a/weed/iam/integration/account_scope_test.go b/weed/iam/integration/account_scope_test.go new file mode 100644 index 000000000..8195a304b --- /dev/null +++ b/weed/iam/integration/account_scope_test.go @@ -0,0 +1,72 @@ +package integration + +import ( + "context" + "testing" +) + +func TestGetProviderByIssuerAndAccount(t *testing.T) { + ctx := context.Background() + store := NewMemoryOIDCProviderStore() + + // Two providers for the same issuer, scoped to different accounts. + must := func(rec *OIDCProviderRecord) { + if err := store.StoreProvider(ctx, "", rec); err != nil { + t.Fatalf("store: %v", err) + } + } + must(&OIDCProviderRecord{ + AccountID: "111", + ARN: "arn:aws:iam::111:oidc-provider/idp.example", + URL: "https://idp.example", + ClientIDs: []string{"a"}, + }) + must(&OIDCProviderRecord{ + AccountID: "222", + ARN: "arn:aws:iam::222:oidc-provider/idp.example", + URL: "https://idp.example", + ClientIDs: []string{"b"}, + }) + + t.Run("matching account returns scoped record", func(t *testing.T) { + rec, err := store.GetProviderByIssuerAndAccount(ctx, "", "https://idp.example", "111") + if err != nil { + t.Fatalf("err: %v", err) + } + if rec.AccountID != "111" { + t.Fatalf("unexpected record: %+v", rec) + } + }) + + t.Run("unknown account is rejected", func(t *testing.T) { + if _, err := store.GetProviderByIssuerAndAccount(ctx, "", "https://idp.example", "999"); err == nil { + t.Fatal("expected error for cross-account use") + } + }) + + t.Run("global provider satisfies any account", func(t *testing.T) { + must(&OIDCProviderRecord{ + AccountID: "", + ARN: "arn:aws:iam:::oidc-provider/global.example", + URL: "https://global.example", + ClientIDs: []string{"x"}, + }) + rec, err := store.GetProviderByIssuerAndAccount(ctx, "", "https://global.example", "anything") + if err != nil { + t.Fatalf("err: %v", err) + } + if rec.AccountID != "" { + t.Fatalf("expected global record, got AccountID=%q", rec.AccountID) + } + }) + + t.Run("empty caller account accepts any record", func(t *testing.T) { + rec, err := store.GetProviderByIssuerAndAccount(ctx, "", "https://idp.example", "") + if err != nil { + t.Fatalf("err: %v", err) + } + if rec == nil { + t.Fatal("expected a record") + } + }) +} diff --git a/weed/iam/integration/iam_manager.go b/weed/iam/integration/iam_manager.go index 95d6c440b..9ce4701bc 100644 --- a/weed/iam/integration/iam_manager.go +++ b/weed/iam/integration/iam_manager.go @@ -744,6 +744,14 @@ func (m *IAMManager) AssumeRoleWithWebIdentity(ctx context.Context, request *sts return nil, fmt.Errorf("IAM manager not initialized") } + // Claim-based mode bypasses role lookup and trust policy entirely; the + // STS service handles the policy resolution from the JWT itself. Account + // scoping still applies but at the provider-resolution layer in Phase 3c + // (we'll plug that in once a multi-account assume path lands). + if sts.IsClaimBasedPolicyRoleArn(request.RoleArn) { + return m.stsService.AssumeRoleWithWebIdentity(ctx, request) + } + // Extract role name from ARN roleName := utils.ExtractRoleNameFromArn(request.RoleArn) @@ -753,6 +761,14 @@ func (m *IAMManager) AssumeRoleWithWebIdentity(ctx context.Context, request *sts return nil, fmt.Errorf("role not found: %s", roleName) } + // Account scoping: when the role lives in account A, the OIDC provider + // validating the token must be either global (AccountID="") or also live + // in account A. Skip when we can't resolve account context; fall through + // to the existing trust-policy enforcement. + if err := m.enforceProviderAccountScope(ctx, request); err != nil { + return nil, err + } + // Validate trust policy before allowing STS to assume the role if err := m.validateTrustPolicyForWebIdentity(ctx, roleDef, request.WebIdentityToken, request.DurationSeconds); err != nil { return nil, fmt.Errorf("trust policy validation failed: %w", err) @@ -767,6 +783,63 @@ func (m *IAMManager) AssumeRoleWithWebIdentity(ctx context.Context, request *sts return m.stsService.AssumeRoleWithWebIdentity(ctx, request) } +// enforceProviderAccountScope checks that the OIDC provider matching the +// token's issuer is registered in either the role's account or as a global +// (account-less) provider. Returns nil when no provider store is configured +// (preserves the static-config-only path) or when the issuer is not known to +// the store (the existing STS-layer issuer→provider map handles that case +// during validation). +func (m *IAMManager) enforceProviderAccountScope(ctx context.Context, request *sts.AssumeRoleWithWebIdentityRequest) error { + if m.oidcProviderStore == nil { + return nil + } + roleAccount := utils.ParseRoleARN(request.RoleArn).AccountID + if roleAccount == "" { + // Legacy ARN form (no account): nothing to enforce. + return nil + } + issuer, err := extractIssuerFromJWT(request.WebIdentityToken) + if err != nil { + return nil // signature validation will reject, no need to fail twice + } + // Look for a provider that is either global or scoped to the role's + // account. Multiple providers may share an issuer (e.g. one global plus + // one per tenant), and GetProviderByIssuer returns the first match + // arbitrarily — using it here would falsely reject a valid request + // whenever the wrong record happens to come back first. + if _, err := m.oidcProviderStore.GetProviderByIssuerAndAccount(ctx, m.getFilerAddress(), issuer, roleAccount); err == nil { + return nil + } + // No allowed match. Distinguish "issuer entirely unknown" (let the STS + // layer reject with the existing not-registered error) from "issuer is + // registered but only in a different account" (surface a precise error + // so the operator knows the call was cross-account). + if other, err := m.oidcProviderStore.GetProviderByIssuer(ctx, m.getFilerAddress(), issuer); err == nil { + return fmt.Errorf("OIDC provider for issuer %s is registered in account %s; cannot be used to assume a role in account %s", issuer, other.AccountID, roleAccount) + } + return nil +} + +// extractIssuerFromJWT returns the iss claim of a JWT without verifying its +// signature. Safe here because the caller still goes through full signature +// + issuer validation in the STS service; this is purely for routing. +func extractIssuerFromJWT(token string) (string, error) { + parser := new(jwt.Parser) + parsed, _, err := parser.ParseUnverified(token, jwt.MapClaims{}) + if err != nil { + return "", err + } + claims, ok := parsed.Claims.(jwt.MapClaims) + if !ok { + return "", fmt.Errorf("invalid claims") + } + iss, _ := claims["iss"].(string) + if iss == "" { + return "", fmt.Errorf("token has no iss claim") + } + return iss, nil +} + // capDurationByRole returns the requested duration clamped to the role's // MaxSessionDuration. A nil requested duration is left nil so the STS // service's calculateSessionDuration applies the global default (typically diff --git a/weed/iam/integration/oidc_provider_store.go b/weed/iam/integration/oidc_provider_store.go index 5f4e1a5c1..1a139d78f 100644 --- a/weed/iam/integration/oidc_provider_store.go +++ b/weed/iam/integration/oidc_provider_store.go @@ -81,6 +81,7 @@ type OIDCProviderStore interface { StoreProvider(ctx context.Context, filerAddress string, record *OIDCProviderRecord) error GetProviderByARN(ctx context.Context, filerAddress string, arn string) (*OIDCProviderRecord, error) GetProviderByIssuer(ctx context.Context, filerAddress string, issuer string) (*OIDCProviderRecord, error) + GetProviderByIssuerAndAccount(ctx context.Context, filerAddress string, issuer, accountID string) (*OIDCProviderRecord, error) ListProviders(ctx context.Context, filerAddress string) ([]*OIDCProviderRecord, error) DeleteProvider(ctx context.Context, filerAddress string, arn string) error } @@ -137,6 +138,29 @@ func (m *MemoryOIDCProviderStore) GetProviderByIssuer(ctx context.Context, _ str return nil, fmt.Errorf("no OIDC provider registered for issuer: %s", issuer) } +// GetProviderByIssuerAndAccount returns the record matching `issuer` whose +// AccountID is empty (global) or equals `accountID`. AWS-style account +// scoping: cross-account references must go through a shared/global +// provider, which is why an empty AccountID is treated as a wildcard. +// +// `accountID` may itself be empty when the caller has no specific account +// context (e.g. claim-based mode); in that case any registered provider +// matching the issuer is acceptable. +func (m *MemoryOIDCProviderStore) GetProviderByIssuerAndAccount(ctx context.Context, _ string, issuer, accountID string) (*OIDCProviderRecord, error) { + want := normalizeIssuer(issuer) + m.mu.RLock() + defer m.mu.RUnlock() + for _, rec := range m.providers { + if normalizeIssuer(rec.URL) != want { + continue + } + if rec.AccountID == "" || accountID == "" || rec.AccountID == accountID { + return copyOIDCProviderRecord(rec), nil + } + } + return nil, fmt.Errorf("no OIDC provider registered for issuer %s in account %s", issuer, accountID) +} + // ListProviders returns every record sorted by ARN for stable output. func (m *MemoryOIDCProviderStore) ListProviders(ctx context.Context, _ string) ([]*OIDCProviderRecord, error) { m.mu.RLock() @@ -283,6 +307,25 @@ func (f *FilerOIDCProviderStore) GetProviderByIssuer(ctx context.Context, filerA return nil, fmt.Errorf("no OIDC provider registered for issuer: %s", issuer) } +// GetProviderByIssuerAndAccount filters by AccountID on top of the issuer scan. +// See the MemoryOIDCProviderStore equivalent for semantics. +func (f *FilerOIDCProviderStore) GetProviderByIssuerAndAccount(ctx context.Context, filerAddress string, issuer, accountID string) (*OIDCProviderRecord, error) { + records, err := f.ListProviders(ctx, filerAddress) + if err != nil { + return nil, err + } + want := normalizeIssuer(issuer) + for _, rec := range records { + if normalizeIssuer(rec.URL) != want { + continue + } + if rec.AccountID == "" || accountID == "" || rec.AccountID == accountID { + return rec, nil + } + } + return nil, fmt.Errorf("no OIDC provider registered for issuer %s in account %s", issuer, accountID) +} + // ListProviders enumerates every record in the directory. func (f *FilerOIDCProviderStore) ListProviders(ctx context.Context, filerAddress string) ([]*OIDCProviderRecord, error) { filerAddress = f.resolveFilerAddress(filerAddress)