fix(iam): keep a service account's inline policy on an edit that names none

An update-service-account request without a policy removed the inline policy of the account: the store took a nil session policy for an empty one, and the handler turned an empty one into nil, so "not given" and "given, empty" were one case. A status change, a secret key rotation, a rename or a new expiry (mcli admin user svcacct disable, enable, and edit without --policy, and the idp ldap accesskey equivalents) signed the token again as inherited-policy, and the account then had all its parent is allowed. A replication peer did the same with the forwarded edit. A nil session policy now leaves the policy as it is. The handler passes an empty policy document on as such, and only that removes the inline policy, as mcli admin user svcacct edit --policy with {"Statement":[]} already asks for. A JSON null policy counts as none. The peer handler passes nil for an edit without a policy and needs no change. What an update does with any other policy document is unchanged. The behaviour came with upstream f0462322f (minio/minio#14847). Signed-off-by: Feng Ruohang <rh@vonng.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
2026-11-10 09:35:31 +03:00 · 2026-11-10 09:42:43 +08:00
parent d998292387
commit c11bfda8f4
4 changed files with 257 additions and 9 deletions
@@ -1,5 +1,25 @@
 # Changelog

+## Unreleased
+
+- An edit of a service account or access key that names no policy keeps its
+  inline policy. `mcli admin user svcacct disable`, `enable` and `edit` without
+  `--policy`, the same `idp ldap accesskey` commands, and any other
+  `update-service-account` request without `newPolicy` removed the inline
+  policy at the site that took the request and, for an account of another user
+  than root, at its replication peers, and the account then had all its parent
+  is allowed. The inline policy is removed
+  only on request, by an empty policy document such as `{"Statement":[]}`
+  (`mcli admin user svcacct edit --policy`); a client that left the policy out
+  to remove it must send that document. A JSON `null` policy counts as none.
+  In the Console, clearing the policy editor of an access key no longer removes
+  the policy; enter `{}` to remove it. The fix does not bring back a policy
+  that was removed, and a site of a replicated deployment that does not run the
+  fix still removes the policy when such an edit reaches it, so sites can
+  differ. Upgrade every site, then review on each site the accounts that
+  `svcacct info` reports with `Policy: implied`, and set the policy again on
+  those that were meant to be restricted.
+
 ## RELEASE.2026-11-09T00-00-00Z

 Package version: `20261109000000.0.0`. Publication status and artifacts are
@@ -885,16 +885,16 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re
 		return
 	}

+	// A request without a policy, or with a JSON null for one, keeps the
+	// policy the account has. An empty policy document is passed on as
+	// such: it is the request to remove the embedded policy.
 	var sp *policy.Policy
-	if len(updateReq.NewPolicy) > 0 {
+	if len(updateReq.NewPolicy) > 0 && !bytes.Equal(updateReq.NewPolicy, []byte("null")) {
 		sp, err = policy.ParseConfigStrict(bytes.NewReader(updateReq.NewPolicy))
 		if err != nil {
 			writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
 			return
 		}
-		if sp.Version == "" && len(sp.Statements) == 0 {
-			sp = nil
-		}
 	}
 	opts := updateServiceAccountOpts{
 		secretKey:     updateReq.NewSecretKey,
@@ -2930,11 +2930,12 @@ func updatedServiceAccountCredentials(ctx context.Context, accessKey string, cr
 	// at this point.
 	m.Delete(sessionPolicyNameExtracted)

-	nosp := opts.sessionPolicy == nil || opts.sessionPolicy.Version == "" && len(opts.sessionPolicy.Statements) == 0
-
-	// sessionPolicy is nil and there is embedded policy attached we remove
-	// embedded policy at that point.
-	if _, ok := m.Lookup(policy.SessionPolicyName); ok && nosp {
+	// A nil sessionPolicy is an edit that does not concern the policy, and
+	// the embedded one stays. Only a policy that was given, and is empty,
+	// removes it: the account then has all its parent is allowed, which is
+	// usually more.
+	reset := opts.sessionPolicy != nil && opts.sessionPolicy.Version == "" && len(opts.sessionPolicy.Statements) == 0
+	if _, ok := m.Lookup(policy.SessionPolicyName); ok && reset {
 		m.Delete(policy.SessionPolicyName)
 		m.Set(iamPolicyClaimNameSA(), inheritedPolicyType)
 	}
@@ -0,0 +1,227 @@
+// Copyright (c) 2026 PGSTY
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+package cmd
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"runtime"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/minio/madmin-go/v3"
+	minio "github.com/minio/minio-go/v7"
+	"github.com/minio/minio/internal/auth"
+	"github.com/pgsty/silo-pkg/v3/policy"
+)
+
+// An edit of a service account that names no policy keeps the embedded
+// policy: the account must not come out of a status change, a key rotation
+// or a rename with all its parent is allowed. Only a policy that is given
+// and empty removes the embedded one.
+
+const svcAccEditPolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::edit/*"]}]}`
+
+func TestIAMServiceAccountEditKeepsPolicy(t *testing.T) {
+	f := newGroupRefreshFixture(t)
+	want, err := policy.ParseConfig(strings.NewReader(svcAccEditPolicy))
+	mustIAM(t, err)
+
+	// restricted creates a service account with the embedded policy.
+	restricted := func(t *testing.T, key, status string) {
+		t.Helper()
+		_, _, err := f.sys.NewServiceAccount(f.ctx, f.parent.AccessKey, nil, newServiceAccountOpts{accessKey: key, secretKey: "edit-original-secret", status: status, sessionPolicy: want})
+		mustIAM(t, err)
+	}
+	// embedded reports whether this server and storage have the policy, and
+	// returns the stored credential.
+	embedded := func(t *testing.T, key string) (cached, stored bool, cr auth.Credentials) {
+		t.Helper()
+		_, got, err := f.sys.GetServiceAccount(f.ctx, key)
+		mustIAM(t, err)
+		if got != nil && !got.Equals(*want) {
+			t.Fatalf("the embedded policy changed: %+v", got)
+		}
+		u, ok := f.stored(t, key)
+		if !ok {
+			t.Fatal("service account missing in storage")
+		}
+		claims, err := getClaimsFromTokenWithSecret(u.Credentials.SessionToken, u.Credentials.SecretKey)
+		mustIAM(t, err)
+		_, hasPolicy := claims.Lookup(policy.SessionPolicyName)
+		return got != nil, hasPolicy && claims.MapClaims[iamPolicyClaimNameSA()] == embeddedPolicyType, u.Credentials
+	}
+	// check wants the edit applied, and the policy kept or removed.
+	check := func(t *testing.T, key string, keep bool, took func(auth.Credentials) bool) {
+		t.Helper()
+		cached, stored, cr := embedded(t, key)
+		if cached != keep || stored != keep {
+			t.Errorf("embedded policy: cached=%v stored=%v, want %v", cached, stored, keep)
+		}
+		if took != nil && !took(cr) {
+			t.Errorf("the edit did not apply: status=%q name=%q description=%q expiration=%v", cr.Status, cr.Name, cr.Description, cr.Expiration)
+		}
+	}
+
+	later := UTCNow().Add(time.Hour)
+	for _, edit := range []struct {
+		name, start string
+		opts        updateServiceAccountOpts
+		took        func(auth.Credentials) bool
+	}{
+		{"disable", auth.AccountOn, updateServiceAccountOpts{status: string(madmin.AccountDisabled)}, func(c auth.Credentials) bool { return c.Status == auth.AccountOff }},
+		{"enable", auth.AccountOff, updateServiceAccountOpts{status: string(madmin.AccountEnabled)}, func(c auth.Credentials) bool { return c.Status == auth.AccountOn }},
+		{"status-off", auth.AccountOn, updateServiceAccountOpts{status: auth.AccountOff}, func(c auth.Credentials) bool { return c.Status == auth.AccountOff }},
+		{"status-on", auth.AccountOff, updateServiceAccountOpts{status: auth.AccountOn}, func(c auth.Credentials) bool { return c.Status == auth.AccountOn }},
+		{"secret-key", auth.AccountOn, updateServiceAccountOpts{secretKey: "edit-rotated-secret"}, func(c auth.Credentials) bool { return c.SecretKey == "edit-rotated-secret" }},
+		{"name", auth.AccountOn, updateServiceAccountOpts{name: "renamed"}, func(c auth.Credentials) bool { return c.Name == "renamed" }},
+		{"description", auth.AccountOn, updateServiceAccountOpts{description: "described"}, func(c auth.Credentials) bool { return c.Description == "described" }},
+		{"expiration", auth.AccountOn, updateServiceAccountOpts{expiration: &later}, func(c auth.Credentials) bool { return c.Expiration.Equal(later) }},
+	} {
+		t.Run(edit.name, func(t *testing.T) {
+			key := "edit-" + edit.name
+			restricted(t, key, edit.start)
+			_, err := f.sys.UpdateServiceAccount(f.ctx, key, edit.opts)
+			mustIAM(t, err)
+			check(t, key, true, edit.took)
+		})
+	}
+
+	t.Run("empty-policy", func(t *testing.T) {
+		const key = "edit-empty-policy"
+		restricted(t, key, auth.AccountOn)
+		_, err := f.sys.UpdateServiceAccount(f.ctx, key, updateServiceAccountOpts{sessionPolicy: &policy.Policy{}})
+		mustIAM(t, err)
+		check(t, key, false, nil)
+	})
+
+	// A peer applies the edit as the site that took it did. The edit goes
+	// through the encoding of the wire, which is where no policy and a null
+	// one become the same.
+	for _, edit := range []struct {
+		name   string
+		update madmin.SRSvcAccUpdate
+		keep   bool
+		took   func(auth.Credentials) bool
+	}{
+		{"peer-status", madmin.SRSvcAccUpdate{Status: auth.AccountOff}, true, func(c auth.Credentials) bool { return c.Status == auth.AccountOff }},
+		{"peer-null", madmin.SRSvcAccUpdate{Description: "described", SessionPolicy: madmin.SRSessionPolicy("null")}, true, func(c auth.Credentials) bool { return c.Description == "described" }},
+		{"peer-empty", madmin.SRSvcAccUpdate{SessionPolicy: madmin.SRSessionPolicy(`{"Statement":[]}`)}, false, nil},
+	} {
+		t.Run(edit.name, func(t *testing.T) {
+			key := "edit-" + edit.name
+			restricted(t, key, auth.AccountOn)
+			edit.update.AccessKey = key
+			sent, err := json.Marshal(iamReplicationItem{SRIAMItem: madmin.SRIAMItem{
+				Type:         madmin.SRIAMItemSvcAcc,
+				SvcAccChange: &madmin.SRSvcAccChange{Update: &edit.update},
+				UpdatedAt:    UTCNow(),
+			}})
+			mustIAM(t, err)
+			var got iamReplicationItem
+			mustIAM(t, json.Unmarshal(sent, &got))
+			mustIAM(t, applyIAMReplicationItem(f.ctx, got))
+			check(t, key, edit.keep, edit.took)
+		})
+	}
+}
+
+// The same over the admin API, with what a client can send for no policy.
+func TestServiceAccountUpdateKeepsPolicy(t *testing.T) {
+	if runtime.GOOS == globalWindowsOSName {
+		t.Skip("IAM integration harness is disabled on Windows")
+	}
+	s := newTestSuiteIAM(TestSuiteCommon{serverType: "ErasureSD", signer: signerV4}, false)
+	c := &check{t, s.serverType}
+	s.SetUpSuite(c)
+	defer s.TearDownSuite(c)
+
+	ctx, cancel := context.WithTimeout(context.Background(), testDefaultTimeout)
+	defer cancel()
+
+	bucket := getRandomBucketName()
+	if err := s.client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{}); err != nil {
+		c.Fatalf("bucket create error: %v", err)
+	}
+	if err := s.adm.AddCannedPolicy(ctx, "keeps-policy-parent", fmt.Appendf(nil, `{"Version":"2012-10-17","Statement":[
+{"Effect":"Allow","Action":["s3:ListBucket"],"Resource":["arn:aws:s3:::%s"]},
+{"Effect":"Allow","Action":["s3:PutObject","s3:GetObject"],"Resource":["arn:aws:s3:::%s/*"]}]}`, bucket, bucket)); err != nil {
+		c.Fatalf("policy add error: %v", err)
+	}
+	parentAK, parentSK := mustGenerateCredentials(c)
+	if err := s.adm.SetUser(ctx, parentAK, parentSK, madmin.AccountEnabled); err != nil {
+		c.Fatalf("unable to set user: %v", err)
+	}
+	if _, err := s.adm.AttachPolicy(ctx, madmin.PolicyAssociationReq{Policies: []string{"keeps-policy-parent"}, User: parentAK}); err != nil {
+		c.Fatalf("unable to attach policy: %v", err)
+	}
+
+	// The account may read objects; listing is its parent's to do.
+	svcAK, svcSK := mustGenerateCredentials(c)
+	cr, err := s.adm.AddServiceAccount(ctx, madmin.AddServiceAccountReq{
+		Policy:     fmt.Appendf(nil, `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::%s/*"]}]}`, bucket),
+		TargetUser: parentAK,
+		AccessKey:  svcAK,
+		SecretKey:  svcSK,
+	})
+	if err != nil {
+		c.Fatalf("unable to create svc acc: %v", err)
+	}
+	c.mustUpload(ctx, s.client, bucket)
+	svcClient := s.getUserClient(c, cr.AccessKey, cr.SecretKey, "")
+	c.mustDownload(ctx, svcClient, bucket)
+	c.mustNotListObjects(ctx, svcClient, bucket)
+
+	info := func() madmin.InfoServiceAccountResp {
+		info, err := s.adm.InfoServiceAccount(ctx, svcAK)
+		if err != nil {
+			c.Fatalf("unable to get svc acc info: %v", err)
+		}
+		return info
+	}
+
+	_, rotatedSK := mustGenerateCredentials(c)
+	later := UTCNow().Add(time.Hour).Truncate(time.Second)
+	for _, edit := range []struct {
+		name string
+		req  madmin.UpdateServiceAccountReq
+		took func(madmin.InfoServiceAccountResp) bool
+	}{
+		{"disable", madmin.UpdateServiceAccountReq{NewStatus: "off"}, func(i madmin.InfoServiceAccountResp) bool { return i.AccountStatus == auth.AccountOff }},
+		{"enable", madmin.UpdateServiceAccountReq{NewStatus: "on"}, func(i madmin.InfoServiceAccountResp) bool { return i.AccountStatus == auth.AccountOn }},
+		{"description", madmin.UpdateServiceAccountReq{NewDescription: "described"}, func(i madmin.InfoServiceAccountResp) bool { return i.Description == "described" }},
+		{"name", madmin.UpdateServiceAccountReq{NewName: "renamed"}, func(i madmin.InfoServiceAccountResp) bool { return i.Name == "renamed" }},
+		{"expiry", madmin.UpdateServiceAccountReq{NewExpiration: &later}, func(i madmin.InfoServiceAccountResp) bool { return i.Expiration != nil && i.Expiration.Equal(later) }},
+		{"secret-key", madmin.UpdateServiceAccountReq{NewSecretKey: rotatedSK}, func(madmin.InfoServiceAccountResp) bool {
+			// The new key signs a request the embedded policy allows.
+			svcClient = s.getUserClient(c, svcAK, rotatedSK, "")
+			_, err := svcClient.StatObject(ctx, bucket, "some-object", minio.StatObjectOptions{})
+			return err == nil
+		}},
+		{"null-policy", madmin.UpdateServiceAccountReq{NewPolicy: json.RawMessage("null"), NewDescription: "null"}, func(i madmin.InfoServiceAccountResp) bool { return i.Description == "null" }},
+	} {
+		if err := s.adm.UpdateServiceAccount(ctx, svcAK, edit.req); err != nil {
+			c.Fatalf("%s: unable to update svc acc: %v", edit.name, err)
+		}
+		got := info()
+		if got.ImpliedPolicy {
+			c.Fatalf("%s: the account lost its embedded policy", edit.name)
+		}
+		if !edit.took(got) {
+			c.Fatalf("%s: the edit did not apply: %+v", edit.name, got)
+		}
+	}
+	c.mustNotListObjects(ctx, svcClient, bucket)
+
+	// An empty policy document asks for the parent's policies.
+	if err := s.adm.UpdateServiceAccount(ctx, svcAK, madmin.UpdateServiceAccountReq{NewPolicy: []byte(`{"Statement":[]}`)}); err != nil {
+		c.Fatalf("unable to remove the policy of svc acc: %v", err)
+	}
+	if !info().ImpliedPolicy {
+		c.Fatalf("an empty policy document must remove the embedded policy")
+	}
+	c.mustListObjects(ctx, svcClient, bucket)
+}