diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index aff58e713..bb34f0bd4 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -101,6 +101,12 @@ type IdentityAccessManagement struct { // These identities are immutable and cannot be updated by dynamic configuration staticIdentityNames map[string]bool + // fileIdentityNames tracks the identity names the current static config file + // declares, as opposed to staticIdentityNames, which also carries identities + // added outside the file (the AWS environment credentials). A reload reads the + // file as the full set of its own identities, so this is the set it may revoke + fileIdentityNames map[string]bool + // staticPolicyNames tracks policy names loaded from the static config file // so full-state reconciliation does not drop them staticPolicyNames map[string]bool @@ -428,22 +434,38 @@ func NewIdentityAccessManagementWithStore(option *S3ApiServerOption, filerClient return iam } -// markStaticIdentities marks the identities declared in a static config file +// markStaticIdentities records the identities declared by a static config file // (-config, or -iam.config when it carries inline identities) as immutable, so -// dynamic filer reloads can't overwrite them. It is additive and scoped to the -// file's identities: a reload protects newly added ones without un-protecting -// the existing set or freezing dynamic filer-managed identities. useStaticConfig -// stays gated on whether any static identity exists, so an advanced-IAM file -// with no inline identities (OIDC/STS only) keeps the dynamic store live. +// dynamic filer reloads can't overwrite them, and remembers which names that file +// itself declares: a reload reads the file as the full set of its own identities, +// so one that has left the file is dropped by MergeS3ApiConfiguration together +// with its access keys, and its name stops being static. Names that did not come +// from the file (the AWS environment credentials) stay static, because the file +// cannot revoke what it never declared. useStaticConfig stays gated on whether +// any static identity exists, so an advanced-IAM file with no inline identities +// (OIDC/STS only) keeps the dynamic store live. func (iam *IdentityAccessManagement) markStaticIdentities(config *iam_pb.S3ApiConfiguration) { + fileNames := make(map[string]bool, len(config.Identities)) + for _, ident := range config.Identities { + fileNames[ident.Name] = true + } + iam.m.Lock() defer iam.m.Unlock() - if iam.staticIdentityNames == nil { - iam.staticIdentityNames = make(map[string]bool) + // staticIdentityNames is the union of the file's own names and the ones added + // outside it, which no reload of the file may un-protect. + staticNames := make(map[string]bool, len(fileNames)+len(iam.staticIdentityNames)) + for name := range fileNames { + staticNames[name] = true } - for _, ident := range config.Identities { - iam.staticIdentityNames[ident.Name] = true + for name := range iam.staticIdentityNames { + if !iam.fileIdentityNames[name] { + staticNames[name] = true + } } + iam.fileIdentityNames = fileNames + iam.staticIdentityNames = staticNames + if iam.staticPolicyNames == nil { iam.staticPolicyNames = make(map[string]bool) } @@ -778,7 +800,10 @@ func (iam *IdentityAccessManagement) loadS3ApiConfigurationWithSource(config *ia hasStaticConfig := iam.useStaticConfig && len(iam.staticIdentityNames) > 0 iam.m.RUnlock() - if hasStaticConfig { + // A static config file always merges. It is authoritative for the identities it + // declares and never for the dynamic store, so a file that no longer declares + // any identity must not fall back to replacing that store on the next reload. + if hasStaticConfig || fromStaticFile { // Merge mode: a dynamic load is the full store state, so it also reconciles deletions return iam.MergeS3ApiConfiguration(config, fromStaticFile, !fromStaticFile) } @@ -984,7 +1009,9 @@ func (iam *IdentityAccessManagement) ReplaceS3ApiConfiguration(config *iam_pb.S3 } // MergeS3ApiConfiguration adds/updates dynamic identities while preserving static -// ones. A config-file reload (fromStaticFile) may also overwrite its static identities. +// ones. A config-file reload (fromStaticFile) may also overwrite its static +// identities, and is authoritative for the ones that file declares: an identity +// it no longer lists is dropped here, with its access keys. // isFullState marks config as the complete store snapshot, so dynamic identities // absent from it are removed; partial merges (a single pushed identity) pass false. func (iam *IdentityAccessManagement) MergeS3ApiConfiguration(config *iam_pb.S3ApiConfiguration, fromStaticFile bool, isFullState bool) error { @@ -1017,6 +1044,10 @@ func (iam *IdentityAccessManagement) MergeS3ApiConfiguration(config *iam_pb.S3Ap for k, v := range iam.staticIdentityNames { staticNames[k] = v } + fileNames := make(map[string]bool) + for k, v := range iam.fileIdentityNames { + fileNames[k] = v + } staticPolicies := make(map[string]bool) for k, v := range iam.staticPolicyNames { staticPolicies[k] = v @@ -1125,15 +1156,28 @@ func (iam *IdentityAccessManagement) MergeS3ApiConfiguration(config *iam_pb.S3Ap nameToIdentity[t.Name] = t } - // full snapshot: drop dynamic identities the store no longer has - if isFullState { + // Reconcile against the loaded state. A store snapshot owns the dynamic + // identities it no longer has; a static config file owns the identities that + // file declares, so one that has left the file is removed here with its access + // keys - otherwise a key revoked by editing the file keeps authenticating + // until the process restarts. + if isFullState || fromStaticFile { present := make(map[string]bool, len(config.Identities)) for _, ident := range config.Identities { present[ident.Name] = true } kept := identities[:0] for _, existing := range identities { - if staticNames[existing.Name] || present[existing.Name] { + var drop bool + if fromStaticFile { + // Only the identities the file declared before are its to revoke: + // the AWS environment credentials and the dynamic filer-managed + // identities are left alone. + drop = fileNames[existing.Name] && !present[existing.Name] + } else { + drop = !staticNames[existing.Name] && !present[existing.Name] + } + if !drop { kept = append(kept, existing) continue } diff --git a/weed/s3api/auth_credentials_static_config_test.go b/weed/s3api/auth_credentials_static_config_test.go index 463ca7448..9c12436f5 100644 --- a/weed/s3api/auth_credentials_static_config_test.go +++ b/weed/s3api/auth_credentials_static_config_test.go @@ -471,3 +471,117 @@ func TestStaticConfigKeepsBracesComingFromTheEnvironment(t *testing.T) { t.Fatalf("expected the secret key from the environment verbatim, got %q", cred.SecretKey) } } + +// Editing an identity out of the static config file and reloading must revoke it: +// the identity, its access keys and those of its service accounts stop working on +// the running gateway, and its name stops being protected as static. What the file +// still declares, a dynamic filer-managed identity and the AWS environment +// identity are all untouched. +func TestReloadStaticConfigRevokesRemovedIdentity(t *testing.T) { + t.Setenv("AWS_ACCESS_KEY_ID", "AKIAENV1") + t.Setenv("AWS_SECRET_ACCESS_KEY", "c2VjcmV0") + + s3a := newTestS3ApiServerWithMemoryIAM(t, []*iam_pb.Identity{}) + + p1 := writeTempIamConfig(t, `{"identities":[{"name":"static-admin","credentials":[{"accessKey":"AKADMIN0","secretKey":"c2VjcmV0"}],"actions":["Admin"]},{"name":"revoked","credentials":[{"accessKey":"AKREVOKE","secretKey":"c2VjcmV0"}],"actions":["Read","List"]},{"name":"kept","credentials":[{"accessKey":"AKKEPT00","secretKey":"c2VjcmV0"}],"actions":["Read"]}],"serviceAccounts":[{"id":"sa-1","parentUser":"revoked","credential":{"accessKey":"AKSA0001","secretKey":"c2VjcmV0"}}]}`) + if err := s3a.iam.loadS3ApiConfigurationFromFile(p1); err != nil { + t.Fatalf("failed to load initial config: %v", err) + } + if _, _, found := s3a.iam.lookupByAccessKey("AKREVOKE"); !found { + t.Fatalf("expected the identity to authenticate before the reload") + } + if _, _, found := s3a.iam.lookupByAccessKey("AKSA0001"); !found { + t.Fatalf("expected the service account key to be loaded before the reload") + } + + // A dynamic identity arrives from the filer and must survive the reload. + if err := s3a.iam.credentialManager.CreateUser(context.Background(), &iam_pb.Identity{Name: "dynamic", Credentials: []*iam_pb.Credential{{AccessKey: "AKDYN000", SecretKey: "c2VjcmV0"}}}); err != nil { + t.Fatalf("failed to create dynamic identity: %v", err) + } + if err := s3a.iam.LoadS3ApiConfigurationFromCredentialManager(); err != nil { + t.Fatalf("failed to load from credential manager: %v", err) + } + if _, _, found := s3a.iam.lookupByAccessKey("AKDYN000"); !found { + t.Fatalf("expected the dynamic identity's key to authenticate before the reload") + } + + // Drop "revoked" and its service account from the file and reload. + p2 := writeTempIamConfig(t, `{"identities":[{"name":"static-admin","credentials":[{"accessKey":"AKADMIN0","secretKey":"c2VjcmV0"}],"actions":["Admin"]},{"name":"kept","credentials":[{"accessKey":"AKKEPT00","secretKey":"c2VjcmV0"}],"actions":["Read"]}]}`) + if err := s3a.iam.loadS3ApiConfigurationFromFile(p2); err != nil { + t.Fatalf("failed to reload config: %v", err) + } + + if hasIdentity(s3a.iam, "revoked") { + t.Fatalf("an identity removed from the config file must be dropped by the reload") + } + for _, key := range []string{"AKREVOKE", "AKSA0001"} { + if _, _, found := s3a.iam.lookupByAccessKey(key); found { + t.Fatalf("access key %s of the removed identity must stop authenticating", key) + } + } + if isStaticName(s3a.iam, "revoked") { + t.Fatalf("a name that left the config file must stop being static") + } + + // What the file still declares is untouched. + if !hasIdentity(s3a.iam, "kept") || !isStaticName(s3a.iam, "kept") { + t.Fatalf("identities still in the file must survive the reload") + } + if _, _, found := s3a.iam.lookupByAccessKey("AKKEPT00"); !found { + t.Fatalf("a kept identity's access key must keep working") + } + + // The file cannot revoke what it never declared. + if !hasIdentity(s3a.iam, "dynamic") { + t.Fatalf("a dynamic identity must survive a static-file reload") + } + if _, _, found := s3a.iam.lookupByAccessKey("AKDYN000"); !found { + t.Fatalf("a dynamic identity's access key must keep working after a static-file reload") + } + if !hasIdentity(s3a.iam, "admin-AKIAENV1") { + t.Fatalf("the AWS environment identity must survive a static-file reload") + } + if _, _, found := s3a.iam.lookupByAccessKey("AKIAENV1"); !found { + t.Fatalf("the AWS environment identity's access key must keep working") + } +} + +// A file reload always merges, so emptying the file must not flip the next reload +// into replacing the store and dropping the filer-managed identities. +func TestReloadStaticConfigWithoutIdentitiesKeepsDynamic(t *testing.T) { + // The AWS environment identity stays static across a file reload, so with credentials + // in the environment it would keep `hasStaticConfig` true and the regression would pass + // unnoticed. Clear them whatever the runner has set. + t.Setenv("AWS_ACCESS_KEY_ID", "") + t.Setenv("AWS_SECRET_ACCESS_KEY", "") + + s3a := newTestS3ApiServerWithMemoryIAM(t, []*iam_pb.Identity{}) + + p1 := writeTempIamConfig(t, `{"identities":[{"name":"static-admin","credentials":[{"accessKey":"AKADMIN0","secretKey":"c2VjcmV0"}],"actions":["Admin"]}]}`) + if err := s3a.iam.loadS3ApiConfigurationFromFile(p1); err != nil { + t.Fatalf("failed to load initial config: %v", err) + } + + if err := s3a.iam.credentialManager.CreateUser(context.Background(), &iam_pb.Identity{Name: "alice", Credentials: []*iam_pb.Credential{{AccessKey: "AKALICE0", SecretKey: "c2VjcmV0"}}}); err != nil { + t.Fatalf("failed to create alice: %v", err) + } + if err := s3a.iam.LoadS3ApiConfigurationFromCredentialManager(); err != nil { + t.Fatalf("failed to load from credential manager: %v", err) + } + + empty := writeTempIamConfig(t, `{"identities":[]}`) + for i := 1; i <= 2; i++ { + if err := s3a.iam.loadS3ApiConfigurationFromFile(empty); err != nil { + t.Fatalf("reload %d failed: %v", i, err) + } + if hasIdentity(s3a.iam, "static-admin") { + t.Fatalf("reload %d: an identity removed from the file must stay removed", i) + } + if !hasIdentity(s3a.iam, "alice") { + t.Fatalf("reload %d: a filer-managed identity must survive a reload of an emptied file", i) + } + if _, _, found := s3a.iam.lookupByAccessKey("AKALICE0"); !found { + t.Fatalf("reload %d: the filer-managed identity's access key must keep working", i) + } + } +}