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