From 633c9bfe092f3e4ffcaf2a17a075081c53fa737b Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sun, 8 Nov 2026 11:39:03 +0800 Subject: [PATCH] fix(batch): keep a job across edits that keep its credential The submitter binding compared the UpdatedAt of the identity record, so any rewrite of the record ended every running job of the credential with "changed since submission": a service account's description, name or later expiry, and the hourly LDAP group refresh of every LDAP-backed service account and STS credential whose groups changed. Compare only what identifies the credential: its type, access key and parent, the revocation boundary of the record, and the keyed binding of its secret and session token. Status, expiry, groups and policies are read again on every start and action. A deletion advances the revocation boundary, so a user created again under the access key with the same secret still fails the binding, and adding an expiry to a service account turns it into a temporary credential, which still ends the job. UpdatedAt stays in the binding for diagnosis. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Feng Ruohang --- cmd/batch-authz.go | 24 ++++-- cmd/batch-submitter-edits_test.go | 122 ++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 5 deletions(-) create mode 100644 cmd/batch-submitter-edits_test.go diff --git a/cmd/batch-authz.go b/cmd/batch-authz.go index 0ce940212..3e30e2936 100644 --- a/cmd/batch-authz.go +++ b/cmd/batch-authz.go @@ -34,8 +34,12 @@ import ( // (batchJobSubmitter). Nothing in the submitted definition names it. // - Every start of the job, and every object it touches, resolves the // binding against IAM again. A credential that is gone, disabled, -// expired, revoked or edited since submission fails the job; a reused -// access key does not match the binding. +// expired or revoked fails the job, and so does one whose secret, +// session token or parent changed since submission; a reused access key +// does not match the binding. Edits that leave the credential itself as +// it was (a name, a description, a later expiry of an expiring +// credential, the LDAP groups) keep the job: its policies, groups, +// status and expiry are read again each time. // - Each action is evaluated with the submitter's policies and session // policy but without a request. A Deny that may apply and would need // request context the job does not have (source address, transport, @@ -55,8 +59,13 @@ type batchJobSubmitter struct { AccessKey string `msg:"ak"` // ParentUser is the parent of a derived credential. ParentUser string `msg:"pu"` - // UpdatedAt and RevokedBefore are the revision of the identity record; - // any edit of the identity changes them. They are unset for root. + // UpdatedAt is the revision of the identity record at submission. It is + // recorded for diagnosis only: an edit that keeps the credential, such as + // a description, a later expiry or an LDAP group refresh, changes it and + // must not end the job. RevokedBefore is the revocation boundary of the + // record, which edits keep and a deletion advances, so a credential + // created again under the access key does not match. Both are unset for + // root. UpdatedAt time.Time `msg:"ua"` RevokedBefore time.Time `msg:"rb"` // Binding is a keyed fingerprint of the credential itself, see @@ -229,8 +238,13 @@ func (s *batchJobSubmitter) principal(ctx context.Context) (batchJobPrincipal, e } cred := u.Credentials typ := batchSubmitterType(cred) + // The binding covers the secret, the session token and the parent, so a + // rotated or reissued credential does not match it. The rest of the + // identity record (UpdatedAt) changes with edits that keep the + // credential and is not compared; what those edits decide, status, + // expiry, groups and policies, is evaluated below and per action. if typ != s.Type || cred.AccessKey != s.AccessKey || cred.ParentUser != s.ParentUser || - !u.UpdatedAt.Equal(s.UpdatedAt) || !u.RevokedBefore.Equal(s.RevokedBefore) || + !u.RevokedBefore.Equal(s.RevokedBefore) || !hmac.Equal(s.Binding, batchSubmitterBinding(typ, cred)) { return batchJobPrincipal{}, errBatchJobChanged } diff --git a/cmd/batch-submitter-edits_test.go b/cmd/batch-submitter-edits_test.go new file mode 100644 index 000000000..372f7536d --- /dev/null +++ b/cmd/batch-submitter-edits_test.go @@ -0,0 +1,122 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cmd + +import ( + "testing" + "time" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/auth" +) + +// Edits that keep the credential keep a job running; edits of the +// credential itself, and its status, still end it. +func TestBatchSubmitterCredentialEdits(t *testing.T) { + f := newBatchAuthzFixture(t) + batchAuthzPolicy(f.ctx, t, f.sys, "edit-parent", batchAuthzFullAccess) + parent := iamAdminUser(f.ctx, t, f.sys, "edit-parent", "edit-parent") + soon := UTCNow().Add(time.Hour) + later := UTCNow().Add(24 * time.Hour) + newService := func(t *testing.T, expiring bool) (auth.Credentials, *batchJobSubmitter) { + t.Helper() + var opts newServiceAccountOpts + if expiring { + opts.expiration = &soon + } + created, _, err := f.sys.NewServiceAccount(f.ctx, parent.AccessKey, nil, opts) + mustIAM(t, err) + stored, ok := f.sys.GetUser(f.ctx, created.AccessKey) + if !ok { + t.Fatal("service account missing") + } + sub, err := newBatchJobSubmitter(stored.Credentials) + mustIAM(t, err) + mustIAM(t, sub.authorize(f.ctx, batchContextRead(f))) + return stored.Credentials, sub + } + for _, tc := range []struct { + name string + expiring bool + edit func(t *testing.T, cred auth.Credentials) + keep bool + }{ + {"description", false, func(t *testing.T, cred auth.Credentials) { + _, err := f.sys.UpdateServiceAccount(f.ctx, cred.AccessKey, updateServiceAccountOpts{description: "cosmetic"}) + mustIAM(t, err) + }, true}, + {"name", false, func(t *testing.T, cred auth.Credentials) { + _, err := f.sys.UpdateServiceAccount(f.ctx, cred.AccessKey, updateServiceAccountOpts{name: "renamed"}) + mustIAM(t, err) + }, true}, + {"expiry-extended", true, func(t *testing.T, cred auth.Credentials) { + _, err := f.sys.UpdateServiceAccount(f.ctx, cred.AccessKey, updateServiceAccountOpts{expiration: &later}) + mustIAM(t, err) + }, true}, + // An expiry makes a service account a temporary credential, which + // IsAllowed evaluates differently. + {"expiry-added", false, func(t *testing.T, cred auth.Credentials) { + _, err := f.sys.UpdateServiceAccount(f.ctx, cred.AccessKey, updateServiceAccountOpts{expiration: &later}) + mustIAM(t, err) + }, false}, + {"groups-refreshed", false, func(t *testing.T, cred auth.Credentials) { + // As the LDAP group refresh rewrites a derived credential. + u, ok := f.sys.GetUser(f.ctx, cred.AccessKey) + if !ok { + t.Fatal("service account missing") + } + c := u.Credentials + c.Groups = append(c.Groups, "refreshed-group") + mustIAM(t, f.sys.store.UpdateUserIdentity(f.ctx, c)) + }, true}, + {"secret-rotated", false, func(t *testing.T, cred auth.Credentials) { + _, err := f.sys.UpdateServiceAccount(f.ctx, cred.AccessKey, updateServiceAccountOpts{secretKey: "edit-rotated-secret-key"}) + mustIAM(t, err) + }, false}, + {"disabled", false, func(t *testing.T, cred auth.Credentials) { + _, err := f.sys.UpdateServiceAccount(f.ctx, cred.AccessKey, updateServiceAccountOpts{status: string(madmin.AccountDisabled)}) + mustIAM(t, err) + }, false}, + } { + t.Run(tc.name, func(t *testing.T) { + cred, sub := newService(t, tc.expiring) + before, _ := f.sys.GetUser(f.ctx, cred.AccessKey) + time.Sleep(2 * time.Millisecond) // a distinct UpdatedAt + tc.edit(t, cred) + after, _ := f.sys.GetUser(f.ctx, cred.AccessKey) + if after.UpdatedAt.Equal(before.UpdatedAt) { + t.Fatal("control: the edit did not change the identity record") + } + err := sub.authorize(f.ctx, batchContextRead(f)) + if tc.keep && err != nil { + t.Errorf("an edit that keeps the credential ended the job: %v", err) + } + if !tc.keep && !isBatchJobAuthzError(err) { + t.Errorf("an edit of the credential must end the job, got %v", err) + } + }) + } +} + +// A user deleted and created again under its access key, with the same +// secret, is not the credential that submitted the job. +func TestBatchSubmitterRecreatedSameSecret(t *testing.T) { + f := newBatchAuthzFixture(t) + batchAuthzPolicy(f.ctx, t, f.sys, "recreate-full", batchAuthzFullAccess) + cred := iamAdminUser(f.ctx, t, f.sys, "recreate-user", "recreate-full") + sub, err := newBatchJobSubmitter(cred) + mustIAM(t, err) + mustIAM(t, sub.authorize(f.ctx, batchContextRead(f))) + mustIAM(t, f.sys.DeleteUser(f.ctx, cred.AccessKey, false)) + time.Sleep(10 * time.Millisecond) + again := iamAdminUser(f.ctx, t, f.sys, cred.AccessKey, "recreate-full") + if again.SecretKey != cred.SecretKey { + t.Fatal("control: the user must be created again with the same secret") + } + err = sub.authorize(f.ctx, batchContextRead(f)) + t.Logf("recreated: %v", err) + if !isBatchJobAuthzError(err) { + t.Errorf("a recreated user must not run the job of the deleted one: %v", err) + } +}