From 1207c3754213ed2a16abbb461e33345e1ae9c50a Mon Sep 17 00:00:00 2001 From: Khris Richardson Date: Mon, 28 Sep 2026 07:07:38 -0700 Subject: [PATCH] s3/iam: persist IAM-managed OIDC providers in the filer, and trust them after a restart The S3 server's IAM config loader never read the documented `oidcProviderStore` key, so the OIDC provider store was always in memory: a provider created with CreateOpenIDConnectProvider lived in one gateway's process, was lost on restart, and was never seen by peers. The /etc/iam/oidc-providers metadata subscription refreshed from that empty in-memory store. - Read `oidcProviderStore` and pass it to the IAM manager. With an IAM config file the default stays memory. With no config file (zero-config IAM, as `weed filer -s3` and operator-managed clusters run) it defaults to the filer: there is nothing static to shadow, and providers created at runtime otherwise vanish on restart. - With a store that outlives the process, load the STS runtime view from it at startup, so providers created on an earlier boot or on a peer are trusted without waiting for the next mutation. - If the store cannot be read at startup (a filer not up yet), the load is retried in the background with backoff until it succeeds: the metadata subscription reports only later changes, so providers already stored would otherwise stay unknown to STS until one of them changed. - Mark records mirrored from STS.Providers as `source: static-config`, and at startup delete such records whose provider has left the config, so removing a provider from the config file still revokes it. Records created through the IAM API are never pruned. - The filer store reported every failed lookup, an unreachable filer included, as ErrOIDCProviderNotFound, which CreateOIDCProvider reads as "free to create". Only a confirmed absence is now not-found. Co-Authored-By: Claude Opus 5.5 (1M context) --- weed/iam/integration/iam_manager.go | 66 ++++++ .../integration/oidc_provider_persist_test.go | 209 ++++++++++++++++++ weed/iam/integration/oidc_provider_store.go | 21 +- weed/s3api/iam_defaults_test.go | 29 +++ weed/s3api/s3api_server.go | 15 +- 5 files changed, 336 insertions(+), 4 deletions(-) create mode 100644 weed/iam/integration/oidc_provider_persist_test.go diff --git a/weed/iam/integration/iam_manager.go b/weed/iam/integration/iam_manager.go index bff5a9c1c..dd825db67 100644 --- a/weed/iam/integration/iam_manager.go +++ b/weed/iam/integration/iam_manager.go @@ -559,6 +559,8 @@ func (m *IAMManager) initOIDCProviderStore(config *IAMConfig) error { } m.oidcProviderStore = store + mirrored := map[string]bool{} + defer m.pruneAndHydrateOIDCProviders(context.Background(), mirrored) if config.STS == nil { return nil } @@ -598,16 +600,80 @@ func (m *IAMManager) initOIDCProviderStore(config *IAMConfig) error { Thumbprints: extractStringList(pc.Config, "thumbprints"), AllowedPrincipalTagKeys: extractStringList(pc.Config, "allowedPrincipalTagKeys"), PolicyClaim: extractString(pc.Config, "policyClaim"), + Source: OIDCProviderSourceStaticConfig, CreatedAt: createdAt, UpdatedAt: now, } if err := store.StoreProvider(ctx, m.getFilerAddress(), rec); err != nil { glog.Warningf("mirror static OIDC provider %s into store: %v", pc.Name, err) } + mirrored[arn] = true } return nil } +// pruneAndHydrateOIDCProviders runs after the static mirror when the store +// outlives the process. A record the static config seeded on an earlier boot +// and no longer lists is deleted, so removing a provider from the config file +// still revokes it. The runtime STS view is then loaded from the store, since +// providers created through the IAM API on an earlier boot, or on a peer, are +// otherwise unknown until the next mutation. An in-memory store holds nothing +// from before this boot, so it needs neither. +func (m *IAMManager) pruneAndHydrateOIDCProviders(ctx context.Context, mirrored map[string]bool) { + if _, inMemory := m.oidcProviderStore.(*MemoryOIDCProviderStore); inMemory { + return + } + if err := m.pruneAndHydrateOnce(ctx, mirrored); err != nil { + // The metadata subscription only reports changes made from now on, so + // providers already in the store would stay unknown until one changes. + glog.Warningf("load OIDC providers from the store at startup: %v; retrying in the background", err) + keep := make(map[string]bool, len(mirrored)) + for arn := range mirrored { + keep[arn] = true + } + go m.retryPruneAndHydrate(keep) + } +} + +// oidcHydrateRetry bounds the backoff between startup load attempts. +var oidcHydrateRetry = struct{ initial, max time.Duration }{initial: time.Second, max: 30 * time.Second} + +func (m *IAMManager) retryPruneAndHydrate(mirrored map[string]bool) { + delay := oidcHydrateRetry.initial + for { + time.Sleep(delay) + err := m.pruneAndHydrateOnce(context.Background(), mirrored) + if err == nil { + glog.V(0).Infof("loaded OIDC providers from the store after retrying") + return + } + glog.V(1).Infof("load OIDC providers from the store: %v; retrying in %v", err, delay) + if delay *= 2; delay > oidcHydrateRetry.max { + delay = oidcHydrateRetry.max + } + } +} + +// pruneAndHydrateOnce deletes static-config records the config no longer +// lists, then loads the store into STS. It fails when the store cannot be read. +func (m *IAMManager) pruneAndHydrateOnce(ctx context.Context, mirrored map[string]bool) error { + records, err := m.oidcProviderStore.ListProviders(ctx, m.getFilerAddress()) + if err != nil { + return fmt.Errorf("list OIDC providers: %w", err) + } + for _, rec := range records { + if rec == nil || rec.Source != OIDCProviderSourceStaticConfig || mirrored[rec.ARN] { + continue + } + if err := m.oidcProviderStore.DeleteProvider(ctx, m.getFilerAddress(), rec.ARN); err != nil { + glog.Warningf("prune OIDC provider %s removed from static config: %v", rec.ARN, err) + continue + } + glog.V(1).Infof("pruned OIDC provider %s: no longer in static config", rec.ARN) + } + return m.RefreshOIDCProvidersFromStore(ctx) +} + // refreshOIDCProvidersBestEffort calls RefreshOIDCProvidersFromStore and // logs a warning on failure. The IAM API call has already succeeded by the // time we get here, so a refresh failure must not surface to the caller — diff --git a/weed/iam/integration/oidc_provider_persist_test.go b/weed/iam/integration/oidc_provider_persist_test.go new file mode 100644 index 000000000..5011b03a7 --- /dev/null +++ b/weed/iam/integration/oidc_provider_persist_test.go @@ -0,0 +1,209 @@ +package integration + +import ( + "context" + "errors" + "strings" + "sync" + "testing" + "time" + + "github.com/golang-jwt/jwt/v5" + "github.com/seaweedfs/seaweedfs/weed/iam/policy" + "github.com/seaweedfs/seaweedfs/weed/iam/sts" + "github.com/stretchr/testify/assert" +) + +// persistentTestStore stands in for a store that outlives the process (the +// filer store): anything but *MemoryOIDCProviderStore is treated as persistent. +type persistentTestStore struct{ *MemoryOIDCProviderStore } + +const ( + persistTestCurrentIssuer = "https://current.example" + persistTestStaleIssuer = "https://stale.example" + persistTestAPIIssuer = "https://api.example" +) + +func newPersistTestManager(t *testing.T) *IAMManager { + t.Helper() + mgr := NewIAMManager() + cfg := &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"), + Providers: []*sts.ProviderConfig{{ + Name: "current", + Type: sts.ProviderTypeOIDC, + Enabled: true, + Config: map[string]interface{}{"issuer": persistTestCurrentIssuer, "clientId": "aud"}, + }}, + }, + Policy: &policy.PolicyEngineConfig{DefaultEffect: "Deny", StoreType: "memory"}, + Roles: &RoleStoreConfig{StoreType: "memory"}, + } + if err := mgr.Initialize(cfg, func() string { return "localhost:8888" }); err != nil { + t.Fatalf("Initialize: %v", err) + } + return mgr +} + +// seedEarlierBoot fills a store as an earlier boot would have left it: the +// provider the config still lists, one it has since dropped, and one created +// through the IAM API. +func seedEarlierBoot(t *testing.T, store OIDCProviderStore) (current, stale, api string) { + t.Helper() + ctx := context.Background() + for _, r := range []struct{ issuer, source string }{ + {persistTestCurrentIssuer, OIDCProviderSourceStaticConfig}, + {persistTestStaleIssuer, OIDCProviderSourceStaticConfig}, + {persistTestAPIIssuer, ""}, + } { + arn, err := DeriveOIDCProviderARN("", r.issuer) + if err != nil { + t.Fatalf("derive ARN: %v", err) + } + rec := &OIDCProviderRecord{ARN: arn, URL: r.issuer, ClientIDs: []string{"aud"}, Source: r.source} + if err := store.StoreProvider(ctx, "", rec); err != nil { + t.Fatalf("seed %s: %v", r.issuer, err) + } + } + current, _ = DeriveOIDCProviderARN("", persistTestCurrentIssuer) + stale, _ = DeriveOIDCProviderARN("", persistTestStaleIssuer) + api, _ = DeriveOIDCProviderARN("", persistTestAPIIssuer) + return current, stale, api +} + +func storedARNs(t *testing.T, mgr *IAMManager) map[string]bool { + t.Helper() + recs, err := mgr.ListOIDCProviders(context.Background()) + if err != nil { + t.Fatalf("ListOIDCProviders: %v", err) + } + out := map[string]bool{} + for _, r := range recs { + out[r.ARN] = true + } + return out +} + +// stsKnowsIssuer reports whether STS resolves a provider for the issuer. The +// token is not validly signed; the only question is which error comes back. +func stsKnowsIssuer(t *testing.T, mgr *IAMManager, issuer string) bool { + t.Helper() + tok, err := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{ + "iss": issuer, "sub": "probe", "aud": "aud", "exp": time.Now().Add(time.Hour).Unix(), + }).SignedString([]byte("not-the-providers-key")) + if err != nil { + t.Fatalf("mint token: %v", err) + } + _, _, err = mgr.GetSTSService().ValidateWebIdentityToken(context.Background(), tok) + if err == nil { + t.Fatalf("an unsigned-by-provider token for %s was accepted", issuer) + } + return !strings.Contains(err.Error(), "no identity provider registered") +} + +func TestPersistentStorePrunesDroppedStaticProvidersAndHydratesSTS(t *testing.T) { + mgr := newPersistTestManager(t) + store := &persistentTestStore{NewMemoryOIDCProviderStore()} + current, stale, api := seedEarlierBoot(t, store) + mgr.SetOIDCProviderStore(store) + + if stsKnowsIssuer(t, mgr, persistTestAPIIssuer) { + t.Fatal("precondition: STS already knows the API-created issuer before hydration") + } + + mgr.pruneAndHydrateOIDCProviders(context.Background(), map[string]bool{current: true}) + + got := storedARNs(t, mgr) + if got[stale] { + t.Errorf("provider dropped from static config is still stored: %s", stale) + } + if !got[current] { + t.Errorf("provider still in static config was pruned: %s", current) + } + if !got[api] { + t.Errorf("provider created through the IAM API was pruned: %s", api) + } + if !stsKnowsIssuer(t, mgr, persistTestAPIIssuer) { + t.Error("STS does not trust the API-created provider found in the store at startup") + } + if stsKnowsIssuer(t, mgr, persistTestStaleIssuer) { + t.Error("STS still trusts the provider dropped from static config") + } +} + +func TestInMemoryStoreIsNeitherPrunedNorHydrated(t *testing.T) { + mgr := newPersistTestManager(t) + store := NewMemoryOIDCProviderStore() + current, stale, _ := seedEarlierBoot(t, store) + mgr.SetOIDCProviderStore(store) + + mgr.pruneAndHydrateOIDCProviders(context.Background(), map[string]bool{current: true}) + + if !storedARNs(t, mgr)[stale] { + t.Error("an in-memory store was pruned; it holds nothing from an earlier boot") + } + if stsKnowsIssuer(t, mgr, persistTestAPIIssuer) { + t.Error("an in-memory store was hydrated into STS at startup") + } +} + +func TestStaticMirrorMarksItsRecords(t *testing.T) { + mgr := newPersistTestManager(t) + recs, err := mgr.ListOIDCProviders(context.Background()) + if err != nil { + t.Fatalf("ListOIDCProviders: %v", err) + } + if len(recs) != 1 || recs[0].Source != OIDCProviderSourceStaticConfig { + t.Fatalf("static mirror did not mark its record: %+v", recs) + } +} + +// unreachableThenReadyStore fails its first reads, as a filer that is not up +// yet when the S3 server starts. +type unreachableThenReadyStore struct { + *MemoryOIDCProviderStore + mu sync.Mutex + failsLeft int +} + +func (s *unreachableThenReadyStore) ListProviders(ctx context.Context, addr string) ([]*OIDCProviderRecord, error) { + s.mu.Lock() + if s.failsLeft > 0 { + s.failsLeft-- + s.mu.Unlock() + return nil, errors.New("filer unavailable") + } + s.mu.Unlock() + return s.MemoryOIDCProviderStore.ListProviders(ctx, addr) +} + +// A store that cannot be read at startup is retried: the metadata +// subscription only reports later changes, so without a retry the providers +// already stored would stay unknown to STS. +func TestStartupLoadRetriesUntilTheStoreIsReadable(t *testing.T) { + saved := oidcHydrateRetry + oidcHydrateRetry.initial, oidcHydrateRetry.max = time.Millisecond, 5*time.Millisecond + t.Cleanup(func() { oidcHydrateRetry = saved }) + + mgr := newPersistTestManager(t) + store := &unreachableThenReadyStore{MemoryOIDCProviderStore: NewMemoryOIDCProviderStore(), failsLeft: 3} + current, stale, api := seedEarlierBoot(t, store) + mgr.SetOIDCProviderStore(store) + + mgr.pruneAndHydrateOIDCProviders(context.Background(), map[string]bool{current: true}) + + deadline := time.Now().Add(2 * time.Second) + for !stsKnowsIssuer(t, mgr, persistTestAPIIssuer) { + if time.Now().After(deadline) { + t.Fatal("STS never learned the stored provider after the store became readable") + } + time.Sleep(5 * time.Millisecond) + } + got := storedARNs(t, mgr) + assert.False(t, got[stale], "the provider dropped from static config was not pruned on retry") + assert.True(t, got[api]) +} diff --git a/weed/iam/integration/oidc_provider_store.go b/weed/iam/integration/oidc_provider_store.go index 1a139d78f..3b23b40a5 100644 --- a/weed/iam/integration/oidc_provider_store.go +++ b/weed/iam/integration/oidc_provider_store.go @@ -71,10 +71,20 @@ type OIDCProviderRecord struct { // (audit/inventory metadata, not propagated into sessions). Tags map[string]string `json:"tags,omitempty"` + // Source records where the entry came from. OIDCProviderSourceStaticConfig + // marks a record mirrored from STS.Providers at boot; empty means it was + // created through the IAM API. Only static-config records are pruned when + // their provider leaves the configuration. + Source string `json:"source,omitempty"` + CreatedAt time.Time `json:"createdAt"` UpdatedAt time.Time `json:"updatedAt"` } +// OIDCProviderSourceStaticConfig is the Source of a record mirrored from the +// static STS provider configuration. +const OIDCProviderSourceStaticConfig = "static-config" + // OIDCProviderStore stores OIDCProviderRecord entries. Implementations are // expected to be safe for concurrent use. type OIDCProviderStore interface { @@ -267,15 +277,20 @@ func (f *FilerOIDCProviderStore) GetProviderByARN(ctx context.Context, filerAddr var data []byte err := f.withFilerClient(filerAddress, func(client filer_pb.SeaweedFilerClient) error { - resp, err := client.LookupDirectoryEntry(ctx, &filer_pb.LookupDirectoryEntryRequest{ + resp, err := filer_pb.LookupEntry(ctx, client, &filer_pb.LookupDirectoryEntryRequest{ Directory: f.basePath, Name: f.fileName(arn), }) + // Only a confirmed absence is ErrOIDCProviderNotFound: callers create + // on it, so an unreachable filer must not read as "no such provider". + if errors.Is(err, filer_pb.ErrNotFound) { + return fmt.Errorf("%w: %s", ErrOIDCProviderNotFound, arn) + } if err != nil { - return fmt.Errorf("%w: %v", ErrOIDCProviderNotFound, err) + return fmt.Errorf("lookup OIDC provider %s: %w", arn, err) } if resp.Entry == nil { - return fmt.Errorf("OIDC provider not found: %s", arn) + return fmt.Errorf("%w: %s", ErrOIDCProviderNotFound, arn) } data = resp.Entry.Content return nil diff --git a/weed/s3api/iam_defaults_test.go b/weed/s3api/iam_defaults_test.go index 37aeeb482..8ce1febf9 100644 --- a/weed/s3api/iam_defaults_test.go +++ b/weed/s3api/iam_defaults_test.go @@ -277,3 +277,32 @@ func TestLoadIAMManagerFromConfig_ExplicitFileEnforcesUserScopedPolicy(t *testin assert.NoError(t, err) assert.True(t, allowed, "user-scoped bucket creation should be allowed") } + +func TestLoadIAMManagerFromConfig_HonorsOIDCProviderStore(t *testing.T) { + // The documented oidcProviderStore key must reach the IAM manager; without + // it, providers created through the IAM API live in one gateway's memory. + cases := []struct { + name string + store string + filer bool + }{ + {"absent keeps memory", ``, false}, + {"filer persists", `,"oidcProviderStore":{"storeType":"filer"}`, true}, + {"no config file persists", "no-file", true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + configPath := "" + if tc.store != "no-file" { + configPath = filepath.Join(t.TempDir(), "iam_config.json") + configContent := `{"sts":{"providers":[]},"policy":{"storeType":"memory","defaultEffect":"Deny"}` + tc.store + `}` + assert.NoError(t, os.WriteFile(configPath, []byte(configContent), 0644)) + } + + manager, err := loadIAMManagerFromConfig(configPath, func() string { return "localhost:8888" }, func() string { return "oidc-store-signing-key" }) + assert.NoError(t, err) + _, isFiler := manager.GetOIDCProviderStore().(*integration.FilerOIDCProviderStore) + assert.Equal(t, tc.filer, isFiler) + }) + } +} diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index b9c6205ee..e54d2ebfa 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -1168,7 +1168,10 @@ func loadIAMManagerFromConfig(configPath string, filerAddressProvider func() str Policy *policy.PolicyEngineConfig `json:"policy"` Providers []map[string]interface{} `json:"providers"` Roles []*integration.RoleDefinition `json:"roles"` - Policies []struct { + // OIDCProviderStore selects where IAM-managed OIDC providers persist. + // Absent, they live in memory and are lost on restart. + OIDCProviderStore *integration.OIDCProviderStoreConfig `json:"oidcProviderStore"` + Policies []struct { Name string `json:"name"` Document *policy.PolicyDocument `json:"document"` } `json:"policies"` @@ -1213,6 +1216,15 @@ func loadIAMManagerFromConfig(configPath string, filerAddressProvider func() str glog.V(1).Infof("Using policy defaults: DefaultEffect=%s, StoreType=%s", configRoot.Policy.DefaultEffect, configRoot.Policy.StoreType) } + // With no IAM config file there is nothing static for a persisted + // provider to shadow or outlive, so providers created at runtime default + // to the filer, where restarts and peer S3 servers see them. A config + // file keeps the in-memory default unless it sets oidcProviderStore. + oidcProviderStore := configRoot.OIDCProviderStore + if oidcProviderStore == nil && configPath == "" && filerAddressProvider != nil { + oidcProviderStore = &integration.OIDCProviderStoreConfig{StoreType: "filer"} + } + // Create IAM configuration iamConfig := &integration.IAMConfig{ STS: configRoot.STS, @@ -1220,6 +1232,7 @@ func loadIAMManagerFromConfig(configPath string, filerAddressProvider func() str Roles: &integration.RoleStoreConfig{ StoreType: sts.StoreTypeMemory, // Use memory store for JSON config-based setup }, + OIDCProviders: oidcProviderStore, } // Apply default signing key if not present in config