s3api: revoke the identities a config reload no longer declares (#11679)

* s3api: revoke the identities a config reload no longer declares

A static config file reload merges with isFullState=false, so an identity
deleted from -s3.config kept authenticating with all of its access keys until
the process restarted. An operator revoking a leaked key got no error and no
revocation.

The file is the source of truth for the identities it declares, so a reload now
drops the ones that have left it, together with their access keys and those of
their service accounts. staticIdentityNames was only ever added to, which kept a
removed name protected as well; the names the file itself declares are tracked
separately from the AWS environment credentials, so a reload never revokes what
the file never declared.

* s3api: keep a file reload from replacing the dynamic store

Removing the last identity from a config file emptied staticIdentityNames, so
useStaticConfig turned false and the next reload of that file took the replace
path: every filer-managed identity and its access keys disappeared until a
dynamic reload brought them back.

A static config file is authoritative for the identities it declares and never
for the dynamic store, so a file load now always merges. The first file load at
startup takes the merge path as well, from an empty state.

Reported by the Devin and Greptile reviews of this PR.
TestReloadStaticConfigWithoutIdentitiesKeepsDynamic reloads an emptied file twice
and asserts that a filer-managed identity and its access key survive both; the
existing test now also checks that the service account key was loaded before the
reload and that the environment identity's key still works after it.

* s3api: clear the environment credentials in the emptied-file test

The AWS environment identity is static and is re-added after every merge, so on
a runner that has AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY set it kept
hasStaticConfig true once the file was emptied, and the next reload still took
the merge path: the regression this test guards passed unnoticed.

Clear both variables whatever the runner has set. With the pre-fix condition
(hasStaticConfig alone) and the variables present in the environment, the test
now fails with "reload 2: a filer-managed identity must survive a reload of an
emptied file".

Reported by the Greptile review of this PR.

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
 			}

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)
+		}
+	}
+}