diff --git a/CHANGELOG.md b/CHANGELOG.md index 12c5c73e0..f27bf99ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,10 +25,30 @@ 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. + fix still removes the policy on such an edit that it takes itself or receives + from another site that does not run 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. +- Site replication sends a service account or access key as the record its + site has stored, whole: for a create, for every edit, at the first sync of a + new site and when healing. IAM export writes that record too. Until now an + edit was sent as the fields it changed. A peer that an earlier edit had not + reached, or reached late, kept the old secret key, status or inline policy + under the revision of the later edit, and healing took the sites for equal: + a key that was rotated, disabled or restricted on one site stayed as it was + on the other. Sync, heal and export took the claims and the inline policy, and + heal the secret key and status as well, from the cache of the server that ran + them, which can predate an edit made through another server of the site, and + sent them under the revision of the stored record. A service account created + for a user that had been deleted and created again did not reach the peers + before healing. Peers on 20261109 and 20261026 apply the new items as they + apply a healed account, and an edit that names no policy no longer removes the + inline policy on such a peer; a site that is not upgraded still sends its own + edits the old way. An edit now gives every site the whole account of the site + that took it, so accounts that already differ between sites are not repaired + by the upgrade, but by the next edit: after upgrading every site, review such + accounts and edit each one at a site that has it right. ## RELEASE.2026-11-09T00-00-00Z diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index db9764359..bfd859ddd 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -914,21 +914,13 @@ func (a adminAPIHandlers) UpdateServiceAccount(w http.ResponseWriter, r *http.Re } // Call site replication hook - non-root user accounts are replicated. + // The hook sends the account as it is stored after this edit, not the + // fields the request changed (see replicationItem). if svcAccount.ParentUser != globalActiveCred.AccessKey { replLogIf(ctx, globalSiteReplicationSys.IAMChangeHook(ctx, madmin.SRIAMItem{ - Type: madmin.SRIAMItemSvcAcc, - SvcAccChange: &madmin.SRSvcAccChange{ - Update: &madmin.SRSvcAccUpdate{ - AccessKey: accessKey, - SecretKey: opts.secretKey, - Status: opts.status, - Name: opts.name, - Description: opts.description, - SessionPolicy: madmin.SRSessionPolicy(updateReq.NewPolicy), - Expiration: updateReq.NewExpiration, - }, - }, - UpdatedAt: updatedAt, + Type: madmin.SRIAMItemSvcAcc, + SvcAccChange: &madmin.SRSvcAccChange{Update: &madmin.SRSvcAccUpdate{AccessKey: accessKey}}, + UpdatedAt: updatedAt, })) } @@ -2399,37 +2391,13 @@ func (a adminAPIHandlers) ExportIAM(w http.ResponseWriter, r *http.Request) { // skip site-replication service account. continue } - claims, err := globalIAMSys.GetClaimsForSvcAcc(ctx, acc.Credentials.AccessKey) + // The stored record only: this server's cache may not have + // heard of an edit made through another server. + svcAccts[user], err = serviceAccountSnapshot(acc) if err != nil { writeErrorResponse(ctx, w, exportError(ctx, err, iamFile, ""), r.URL) return } - sa, policy, err := globalIAMSys.GetServiceAccount(ctx, acc.Credentials.AccessKey) - if err != nil { - writeErrorResponse(ctx, w, exportError(ctx, err, iamFile, ""), r.URL) - return - } - - var policyJSON []byte - if policy != nil { - policyJSON, err = json.Marshal(policy) - if err != nil { - writeErrorResponse(ctx, w, exportError(ctx, err, iamFile, ""), r.URL) - return - } - } - svcAccts[user] = madmin.SRSvcAccCreate{ - Parent: acc.Credentials.ParentUser, - AccessKey: user, - SecretKey: acc.Credentials.SecretKey, - Groups: acc.Credentials.Groups, - Claims: claims, - SessionPolicy: policyJSON, - Status: acc.Credentials.Status, - Name: sa.Name, - Description: sa.Description, - Expiration: &sa.Expiration, - } } svcAccData, err := json.Marshal(svcAccts) diff --git a/cmd/iam-replication-protocol.go b/cmd/iam-replication-protocol.go index ec2061e99..fe1814823 100644 --- a/cmd/iam-replication-protocol.go +++ b/cmd/iam-replication-protocol.go @@ -172,14 +172,41 @@ func (c *SiteReplicationSys) replicationItem(ctx context.Context, item madmin.SR key = item.SvcAccChange.Update.AccessKey } if key != "" { - r, err := loadIAMRevision(ctx, globalIAMSys.store, getUserIdentityPath(key, svcUser)) + // A create or an edit goes out as the account stored now: the + // whole record, under its revision, whatever the caller's item + // says. A peer orders items by revision and drops an older one, + // so an edit sent as the fields it changed would lose every + // earlier edit that did not reach the peer, or reaches it late. + // A later write that won before sending goes out in place of + // this one. + stored := make(map[string]UserIdentity, 1) + err := globalIAMSys.store.loadUser(ctx, key, svcUser, stored) + if err != nil && !errors.Is(err, errNoSuchUser) { + return out, err + } + u, ok := stored[key] + if !ok { + // Nothing is sent for an account that is deleted, expired or + // of a revoked parent meanwhile; a deletion replicates itself. + // A stored token that cannot be read is an error, and the + // account is not taken for sent. + var raw UserIdentity + err := globalIAMSys.store.loadIAMConfig(ctx, &raw, getUserIdentityPath(key, svcUser)) + if err == nil && !raw.Deleted && !raw.Credentials.IsExpired() { + _, err = extractJWTClaims(raw) + } + if err != nil && !errors.Is(err, errConfigNotFound) { + return out, err + } + return out, errIAMStaleUpdate + } + snapshot, err := serviceAccountSnapshot(u) if err != nil { return out, err } - if r.Deleted || r.timestamp().After(item.UpdatedAt) { - return out, errIAMStaleUpdate - } - out.RevokedBefore = r.RevokedBefore + out.SvcAccChange = &madmin.SRSvcAccChange{Create: &snapshot} + out.UpdatedAt = u.UpdatedAt + out.RevokedBefore = u.RevokedBefore } } if item.Type == madmin.SRIAMItemGroupInfo && item.GroupInfo != nil && !item.GroupInfo.UpdateReq.IsRemove { diff --git a/cmd/iam-svcacct-replication-snapshot_test.go b/cmd/iam-svcacct-replication-snapshot_test.go new file mode 100644 index 000000000..e8259e7f2 --- /dev/null +++ b/cmd/iam-svcacct-replication-snapshot_test.go @@ -0,0 +1,482 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cmd + +import ( + "archive/zip" + "bytes" + "context" + "encoding/base64" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "path" + "strings" + "sync" + "testing" + "time" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/auth" + "github.com/pgsty/silo-pkg/v3/policy" +) + +// Site replication sends a service account as the record its site has +// stored, whole. A peer that applies the last item it was sent then has +// the account of the sending site, whatever it received or lost before, +// and whatever the sending server holds in its cache. + +const svcAccSnapshotPolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::snapshot/*"]}]}` + +// svcAccRecord is what a site has of a service account. +type svcAccRecord struct { + Parent, Secret, Status, Name, Description, Policy string + Expiration, UpdatedAt time.Time +} + +// svcAccPolicyText is a policy document in the one form it is compared in; +// an account without an embedded policy has "". +func svcAccPolicyText(t *testing.T, document []byte) string { + t.Helper() + if len(document) == 0 { + return "" + } + p, err := policy.ParseConfig(bytes.NewReader(document)) + mustIAM(t, err) + text, err := json.Marshal(p) + mustIAM(t, err) + return string(text) +} + +// svcAccStored is the account as a server that starts now loads it. +func svcAccStored(t *testing.T, f *groupRefreshFixture, key string) svcAccRecord { + t.Helper() + u, ok := f.stored(t, key) + if !ok { + t.Fatalf("service account %s is not stored", key) + } + cr := u.Credentials + claims, err := getClaimsFromTokenWithSecret(cr.SessionToken, cr.SecretKey) + mustIAM(t, err) + rec := svcAccRecord{Parent: cr.ParentUser, Secret: cr.SecretKey, Status: cr.Status, Name: cr.Name, Description: cr.Description, Expiration: cr.Expiration, UpdatedAt: u.UpdatedAt} + if sp, ok := claims.Lookup(policy.SessionPolicyName); ok && claims.MapClaims[iamPolicyClaimNameSA()] == embeddedPolicyType { + document, err := base64.StdEncoding.DecodeString(sp) + mustIAM(t, err) + rec.Policy = svcAccPolicyText(t, document) + } + return rec +} + +// svcAccSite is a site that replicates to one peer, which records the IAM +// items it is sent. +type svcAccSite struct { + *groupRefreshFixture + mu sync.Mutex + sent []iamReplicationItem +} + +func newSvcAccSite(t *testing.T) *svcAccSite { + t.Helper() + s := &svcAccSite{groupRefreshFixture: newGroupRefreshFixture(t)} + globalObjLayerMutex.Lock() + globalObjectAPI = s.obj + globalObjLayerMutex.Unlock() + + peer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPut && strings.HasSuffix(r.URL.Path, iamRevisionPeerPath) { + var batch iamRevisionBatch + if err := json.NewDecoder(r.Body).Decode(&batch); err != nil { + t.Error(err) + } + s.mu.Lock() + s.sent = append(s.sent, batch.Items...) + s.mu.Unlock() + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(iamRevisionResponse{iamRevisionStatus: iamRevisionStatus{Version: iamRevisionProtocol, Node: "node", Instance: "boot", Digest: "fixture"}}) + })) + globalSiteReplicatorCred.Set("snapshot-site-signing-key") + globalSiteReplicationSys.Lock() + globalSiteReplicationSys.enabled = true + globalSiteReplicationSys.state = srState{ServiceAccountAccessKey: s.parent.AccessKey, Peers: s.peers(peer.URL)} + globalSiteReplicationSys.Unlock() + t.Cleanup(func() { + s.leave() + peer.Close() + }) + return s +} + +func (s *svcAccSite) peers(endpoint string) map[string]madmin.PeerInfo { + return map[string]madmin.PeerInfo{ + globalDeploymentID(): {Name: "local", DeploymentID: globalDeploymentID()}, + "remote": {Name: "remote", DeploymentID: "remote", Endpoint: endpoint}, + } +} + +// leave ends the replication of this process, for the site that comes next. +func (s *svcAccSite) leave() { + globalSiteReplicationSys.Lock() + globalSiteReplicationSys.enabled = false + globalSiteReplicationSys.state = srState{} + globalSiteReplicationSys.Unlock() + globalSiteReplicatorCred.Set("") +} + +// items are the creates and edits of account key the peer was sent, in the +// order of sending. +func (s *svcAccSite) items(key string) []iamReplicationItem { + s.mu.Lock() + defer s.mu.Unlock() + var items []iamReplicationItem + for _, item := range s.sent { + c := item.SvcAccChange + if c != nil && ((c.Create != nil && c.Create.AccessKey == key) || (c.Update != nil && c.Update.AccessKey == key)) { + items = append(items, item) + } + } + return items +} + +// create and edit are an administrator's requests to this site. +func (s *svcAccSite) create(t *testing.T, key string) { + t.Helper() + rec := iamAdminCall(t, globalActiveCred, http.MethodPut, "/minio/admin/v3/add-service-account", + madmin.AddServiceAccountReq{TargetUser: s.parent.AccessKey, AccessKey: key, SecretKey: "snapshot-original-secret"}, nil, adminAPIHandlers{}.AddServiceAccount) + if rec.Code != http.StatusOK { + t.Fatalf("add-service-account: %d %s", rec.Code, rec.Body.String()) + } +} + +func (s *svcAccSite) edit(t *testing.T, key string, req madmin.UpdateServiceAccountReq) { + t.Helper() + rec := iamAdminCall(t, globalActiveCred, http.MethodPost, "/minio/admin/v3/update-service-account?accessKey="+key, + req, map[string]string{"accessKey": key}, adminAPIHandlers{}.UpdateServiceAccount) + if rec.Code != http.StatusNoContent { + t.Fatalf("update-service-account: %d %s", rec.Code, rec.Body.String()) + } +} + +// svcAccPeer is the site that applies items, in the order given, and what +// it then has of account key. It has the parent user, as a peer does. +func svcAccPeer(t *testing.T, from *svcAccSite, key string, items ...iamReplicationItem) svcAccRecord { + t.Helper() + from.leave() + peer := newGroupRefreshFixture(t) + for i, item := range items { + // As the peer reads it off the wire. + raw, err := json.Marshal(item) + mustIAM(t, err) + var received iamReplicationItem + mustIAM(t, json.Unmarshal(raw, &received)) + if err := applyIAMReplicationItem(peer.ctx, received); err != nil { + t.Fatalf("the peer refused item %d: %v", i, err) + } + } + return svcAccStored(t, peer, key) +} + +func wantSvcAccRecord(t *testing.T, where string, got, want svcAccRecord) { + t.Helper() + if got != want { + t.Errorf("%s:\n got %+v\nwant %+v", where, got, want) + } +} + +// An edit of a service account reaches a peer whatever became of the edits +// before it: one that arrives late, or never, is not lost. +func TestSiteReplicationServiceAccountEdits(t *testing.T) { + const key = "snapshot-edits" + for _, tc := range []struct { + name string + edits []madmin.UpdateServiceAccountReq + took func(svcAccRecord) bool // control: what the site has after them + }{ + {"policy-secret-description", []madmin.UpdateServiceAccountReq{ + {NewPolicy: json.RawMessage(svcAccSnapshotPolicy)}, + {NewSecretKey: "snapshot-rotated-secret"}, + {NewDescription: "described"}, + }, func(r svcAccRecord) bool { + return r.Policy != "" && r.Secret == "snapshot-rotated-secret" && r.Description == "described" + }}, + {"secret-disable", []madmin.UpdateServiceAccountReq{ + {NewPolicy: json.RawMessage(svcAccSnapshotPolicy), NewSecretKey: "snapshot-rotated-secret"}, + {NewStatus: string(madmin.AccountDisabled)}, + }, func(r svcAccRecord) bool { + return r.Policy != "" && r.Secret == "snapshot-rotated-secret" && r.Status == auth.AccountOff + }}, + // The last edit removes the embedded policy: the peer gets an + // account without one, and the description set before. + {"policy-removed", []madmin.UpdateServiceAccountReq{ + {NewPolicy: json.RawMessage(svcAccSnapshotPolicy)}, + {NewDescription: "described"}, + {NewPolicy: json.RawMessage(`{"Statement":[]}`)}, + }, func(r svcAccRecord) bool { return r.Policy == "" && r.Description == "described" }}, + // Policy documents an edit ignores: one with a version and no + // statement, one with statements and no version. The site keeps the + // policy it has, and so does the peer. + {"policy-ignored", []madmin.UpdateServiceAccountReq{ + {NewPolicy: json.RawMessage(svcAccSnapshotPolicy)}, + {NewPolicy: json.RawMessage(`{"Version":"2012-10-17","Statement":[]}`), NewName: "named"}, + {NewPolicy: json.RawMessage(`{"Statement":[{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::ignored/*"]}]}`), NewDescription: "described"}, + }, func(r svcAccRecord) bool { + return r.Policy == svcAccPolicyText(t, []byte(svcAccSnapshotPolicy)) && r.Name == "named" && r.Description == "described" + }}, + } { + for _, delivery := range []string{"late", "lost"} { + t.Run(tc.name+"/"+delivery, func(t *testing.T) { + site := newSvcAccSite(t) + site.create(t, key) + for _, edit := range tc.edits { + site.edit(t, key, edit) + } + sent := site.items(key) + if len(sent) != 1+len(tc.edits) { + t.Fatalf("the site sent %d items for the account, want one for the create and each edit", len(sent)) + } + want := svcAccStored(t, site.groupRefreshFixture, key) + if !tc.took(want) { + t.Fatalf("control: the site did not apply the edits: %+v", want) + } + // The peer has the account and gets the last edit. The edits + // before it were lost, or reach it after the last. + last := len(sent) - 1 + deliver := []iamReplicationItem{sent[0], sent[last]} + if delivery == "late" { + deliver = append(deliver, sent[1:last]...) + } + wantSvcAccRecord(t, "peer", svcAccPeer(t, site, key, deliver...), want) + }) + } + } + + // Two requests edit the account through this site at once, and the + // first comes to replicate when the second is stored. + t.Run("concurrent", func(t *testing.T) { + site := newSvcAccSite(t) + site.create(t, key) + sp, err := policy.ParseConfig(strings.NewReader(svcAccSnapshotPolicy)) + mustIAM(t, err) + first, err := site.sys.UpdateServiceAccount(site.ctx, key, updateServiceAccountOpts{sessionPolicy: sp}) + mustIAM(t, err) + second, err := site.sys.UpdateServiceAccount(site.ctx, key, updateServiceAccountOpts{description: "described"}) + mustIAM(t, err) + for _, at := range []time.Time{first, second} { + mustIAM(t, globalSiteReplicationSys.IAMChangeHook(site.ctx, madmin.SRIAMItem{ + Type: madmin.SRIAMItemSvcAcc, + SvcAccChange: &madmin.SRSvcAccChange{Update: &madmin.SRSvcAccUpdate{AccessKey: key}}, + UpdatedAt: at, + })) + } + want := svcAccStored(t, site.groupRefreshFixture, key) + if want.Policy == "" || want.Description != "described" { + t.Fatalf("control: the site did not apply the edits: %+v", want) + } + wantSvcAccRecord(t, "peer", svcAccPeer(t, site, key, site.items(key)...), want) + }) +} + +// Another server of the site restricted, rotated and disabled the account, +// and this server's cache has not heard of it. What this server sends, and +// exports, is the account that is stored. +func TestSiteReplicationServiceAccountStaleCache(t *testing.T) { + const key = "snapshot-stale" + // prepare returns the site, the stored account and what it sent for the + // create. + prepare := func(t *testing.T) (*svcAccSite, svcAccRecord, iamReplicationItem) { + t.Helper() + site := newSvcAccSite(t) + site.create(t, key) + created := site.items(key) + if len(created) != 1 { + t.Fatalf("the site sent %d items for the create", len(created)) + } + sp, err := policy.ParseConfig(strings.NewReader(svcAccSnapshotPolicy)) + mustIAM(t, err) + _, err = site.sibling(t).UpdateServiceAccount(site.ctx, key, updateServiceAccountOpts{ + sessionPolicy: sp, secretKey: "snapshot-rotated-secret", status: auth.AccountOff, description: "restricted", + }) + mustIAM(t, err) + if cached, _ := site.sys.store.GetUser(key); cached.Credentials.SecretKey != "snapshot-original-secret" || cached.Credentials.Status != auth.AccountOn { + t.Fatal("control: this server's cache must not have heard of the edit") + } + want := svcAccStored(t, site.groupRefreshFixture, key) + if want.Policy == "" || want.Secret != "snapshot-rotated-secret" || want.Status != auth.AccountOff { + t.Fatalf("control: the edit is not stored: %+v", want) + } + return site, want, created[0] + } + + t.Run("heal", func(t *testing.T) { + site, want, created := prepare(t) + local := madmin.UserInfo{Status: madmin.AccountStatus(want.Status), UpdatedAt: want.UpdatedAt} + remote := madmin.UserInfo{Status: madmin.AccountStatus(auth.AccountOn), UpdatedAt: created.UpdatedAt} + info := srStatusInfo{Sites: globalSiteReplicationSys.state.Peers, UserStats: map[string]map[string]srUserStatsSummary{key: { + globalDeploymentID(): {userInfo: srUserInfo{UserInfo: local}}, + "remote": {SRUserStatsSummary: madmin.SRUserStatsSummary{UserInfoMismatch: true}, userInfo: srUserInfo{UserInfo: remote}}, + }}} + mustIAM(t, globalSiteReplicationSys.healUsers(site.ctx, site.obj, key, info)) + sent := site.items(key) + if len(sent) != 2 { + t.Fatalf("heal sent %d items for the account, want 1", len(sent)-1) + } + wantSvcAccRecord(t, "healed peer", svcAccPeer(t, site, key, sent...), want) + }) + + t.Run("sync", func(t *testing.T) { + site, want, _ := prepare(t) + mustIAM(t, globalSiteReplicationSys.syncToAllPeers(site.ctx, madmin.SRAddOptions{})) + sent := site.items(key) + if len(sent) != 2 { + t.Fatalf("sync sent %d items for the account, want 1", len(sent)-1) + } + wantSvcAccRecord(t, "new peer", svcAccPeer(t, site, key, sent[1]), want) + }) + + t.Run("export", func(t *testing.T) { + _, want, _ := prepare(t) + rec := iamAdminCall(t, globalActiveCred, http.MethodGet, "/minio/admin/v3/export-iam", nil, nil, adminAPIHandlers{}.ExportIAM) + if rec.Code != http.StatusOK { + t.Fatalf("export-iam: %d %s", rec.Code, rec.Body.String()) + } + archive, err := zip.NewReader(bytes.NewReader(rec.Body.Bytes()), int64(rec.Body.Len())) + mustIAM(t, err) + file, err := archive.Open(path.Join(iamAssetsDir, allSvcAcctsFile)) + mustIAM(t, err) + raw, err := io.ReadAll(file) + mustIAM(t, err) + var accounts map[string]madmin.SRSvcAccCreate + mustIAM(t, json.Unmarshal(raw, &accounts)) + a, ok := accounts[key] + if !ok || a.Expiration == nil { + t.Fatalf("the export has no account %s with an expiration (present=%v)", key, ok) + } + got := svcAccRecord{Parent: a.Parent, Secret: a.SecretKey, Status: a.Status, Name: a.Name, Description: a.Description, Policy: svcAccPolicyText(t, a.SessionPolicy), Expiration: *a.Expiration} + want.UpdatedAt = time.Time{} // an export has no revisions + wantSvcAccRecord(t, "export", got, want) + if a.Claims[iamPolicyClaimNameSA()] != embeddedPolicyType { + t.Errorf("exported claims say %v, want %s", a.Claims[iamPolicyClaimNameSA()], embeddedPolicyType) + } + }) +} + +// The parent was deleted and created again on both sites. The token of a +// service account created afterwards names that revocation, and a peer +// refuses a credential of the parent that does not: the create must carry +// the claims of the token. +func TestSiteReplicationServiceAccountOfRecreatedParent(t *testing.T) { + const key = "snapshot-reparent" + site := newSvcAccSite(t) + mustIAM(t, site.sys.DeleteUser(site.ctx, site.parent.AccessKey, false)) + iamAdminUser(site.ctx, t, site.sys, site.parent.AccessKey, "refresh-parent") + revoked, err := loadIAMRevision(site.ctx, site.sys.store, getUserIdentityPath(site.parent.AccessKey, regUser)) + mustIAM(t, err) + if revoked.RevokedBefore.IsZero() { + t.Fatal("control: the parent must have a revocation boundary") + } + site.create(t, key) + sent := site.items(key) + if len(sent) != 1 { + t.Fatalf("the site sent %d items for the create", len(sent)) + } + want := svcAccStored(t, site.groupRefreshFixture, key) + + // The peer's parent is newer than the deletion, which leaves it live + // with the boundary: a parent that was deleted and created again. + site.leave() + peer := newGroupRefreshFixture(t) + mustIAM(t, peer.sys.DeleteUser(withIAMReplicationTime(peer.ctx, revoked.RevokedBefore), peer.parent.AccessKey, false)) + if r, err := loadIAMRevision(peer.ctx, peer.sys.store, getUserIdentityPath(peer.parent.AccessKey, regUser)); err != nil || r.Deleted || !r.RevokedBefore.Equal(revoked.RevokedBefore) { + t.Fatalf("control: the peer's parent must be live with the boundary (err=%v deleted=%v)", err, r.Deleted) + } + mustIAM(t, applyIAMReplicationItem(peer.ctx, sent[0])) + if _, ok := peer.stored(t, key); !ok { + t.Fatal("the peer dropped the new service account") + } + wantSvcAccRecord(t, "peer", svcAccStored(t, peer, key), want) +} + +// An account that is deleted is not sent, and that is not an error. An +// account whose stored token cannot be read is not taken for deleted: the +// site signing key is not available here, and no token is read without it. +func TestSiteReplicationServiceAccountUnreadable(t *testing.T) { + const key = "snapshot-unreadable" + site := newSvcAccSite(t) + site.create(t, key) + item := madmin.SRIAMItem{Type: madmin.SRIAMItemSvcAcc, SvcAccChange: &madmin.SRSvcAccChange{Update: &madmin.SRSvcAccUpdate{AccessKey: key}}} + + globalSiteReplicatorCred.Set("") + if _, err := globalSiteReplicationSys.replicationItem(site.ctx, item); err == nil || errors.Is(err, errIAMStaleUpdate) { + t.Errorf("an account whose token cannot be read must be an error, got %v", err) + } + if err := globalSiteReplicationSys.IAMChangeHook(site.ctx, item); err == nil { + t.Error("the hook reported an account as sent that it could not read") + } + globalSiteReplicatorCred.Set("snapshot-site-signing-key") + + mustIAM(t, site.sys.DeleteServiceAccount(site.ctx, key, false)) + if _, err := globalSiteReplicationSys.replicationItem(site.ctx, item); !errors.Is(err, errIAMStaleUpdate) { + t.Errorf("a deleted account must not be sent, got %v", err) + } + if sent := site.items(key); len(sent) != 1 { + t.Errorf("the site sent %d items for the account, want only the create", len(sent)) + } +} + +// svcAccPausedStore holds every load of a user until proceed is closed. +type svcAccPausedStore struct { + IAMStorageAPI + once sync.Once + loading, proceed chan struct{} +} + +func (s *svcAccPausedStore) loadUser(ctx context.Context, user string, userType IAMUserType, m map[string]UserIdentity) error { + s.once.Do(func() { close(s.loading) }) + <-s.proceed + return s.IAMStorageAPI.loadUser(ctx, user, userType, m) +} + +// The hook reads the account from storage without the lock of the +// replication configuration. Loading an identity takes that lock for +// reading; if the hook held it too, a change of the configuration that +// arrives in between would wait for the hook, and the hook for it. +func TestSiteReplicationHookReadsStorageUnlocked(t *testing.T) { + const key = "snapshot-unlocked" + site := newSvcAccSite(t) + site.create(t, key) + paused := &svcAccPausedStore{IAMStorageAPI: site.sys.store.IAMStorageAPI, loading: make(chan struct{}), proceed: make(chan struct{})} + site.sys.store.IAMStorageAPI = paused + + sent := make(chan error, 1) + go func() { + sent <- globalSiteReplicationSys.IAMChangeHook(site.ctx, madmin.SRIAMItem{ + Type: madmin.SRIAMItemSvcAcc, + SvcAccChange: &madmin.SRSvcAccChange{Update: &madmin.SRSvcAccUpdate{AccessKey: key}}, + }) + }() + select { + case <-paused.loading: + case err := <-sent: + t.Fatalf("the hook did not read the account from storage (err=%v)", err) + } + // A writer that had to wait here would never get the lock, nor the + // hook its next read lock behind that writer: try, do not wait. + if globalSiteReplicationSys.TryLock() { + globalSiteReplicationSys.Unlock() //nolint:staticcheck // only the lock is of interest + } else { + t.Error("the hook holds the lock of the replication configuration while it reads storage") + } + close(paused.proceed) + select { + case err := <-sent: + mustIAM(t, err) + case <-time.After(10 * time.Second): + t.Fatal("the hook did not return") + } + if got := site.items(key); len(got) != 2 { + t.Errorf("the site sent %d items for the account, want the create and the edit", len(got)) + } +} diff --git a/cmd/iam.go b/cmd/iam.go index 6f8ffad18..a89415c27 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -1311,22 +1311,62 @@ func (sys *IAMSys) getServiceAccount(ctx context.Context, accessKey string) (Use return UserIdentity{}, nil, errNoSuchServiceAccount } - var embeddedPolicy *policy.Policy + embeddedPolicy, err := embeddedServiceAccountPolicy(jwtClaims) + if err != nil { + return UserIdentity{}, nil, err + } + return sa, embeddedPolicy, nil +} +// embeddedServiceAccountPolicy returns the policy embedded in the token of a +// service account, or nil when the account has the policies of its parent. +func embeddedServiceAccountPolicy(jwtClaims *jwt.MapClaims) (*policy.Policy, error) { pt, ptok := jwtClaims.Lookup(iamPolicyClaimNameSA()) sp, spok := jwtClaims.Lookup(policy.SessionPolicyName) - if ptok && spok && pt == embeddedPolicyType { - policyBytes, err := base64.StdEncoding.DecodeString(sp) - if err != nil { - return UserIdentity{}, nil, err - } - embeddedPolicy, err = policy.ParseConfig(bytes.NewReader(policyBytes)) - if err != nil { - return UserIdentity{}, nil, err + if !ptok || !spok || pt != embeddedPolicyType { + return nil, nil + } + policyBytes, err := base64.StdEncoding.DecodeString(sp) + if err != nil { + return nil, err + } + return policy.ParseConfig(bytes.NewReader(policyBytes)) +} + +// serviceAccountSnapshot returns service account u, a record loaded from +// storage, as the create that replication, the first sync, heal and export +// carry. Its claims and its embedded policy are read from the token of that +// record, so that every part of the snapshot is of one revision: a server's +// cache may hold the account as it was before an edit made through another +// server. +func serviceAccountSnapshot(u UserIdentity) (madmin.SRSvcAccCreate, error) { + jwtClaims, err := extractJWTClaims(u) + if err != nil { + return madmin.SRSvcAccCreate{}, err + } + embeddedPolicy, err := embeddedServiceAccountPolicy(jwtClaims) + if err != nil { + return madmin.SRSvcAccCreate{}, err + } + var policyJSON []byte + if embeddedPolicy != nil { + if policyJSON, err = json.Marshal(embeddedPolicy); err != nil { + return madmin.SRSvcAccCreate{}, err } } - - return sa, embeddedPolicy, nil + cr := u.Credentials + return madmin.SRSvcAccCreate{ + Parent: cr.ParentUser, + AccessKey: cr.AccessKey, + SecretKey: cr.SecretKey, + Groups: cr.Groups, + Claims: jwtClaims.Map(), + SessionPolicy: policyJSON, + Status: cr.Status, + Name: cr.Name, + Description: cr.Description, + Expiration: &cr.Expiration, + }, nil } // GetTemporaryAccount - wrapper method to get information about a temporary account @@ -1398,25 +1438,6 @@ func (sys *IAMSys) getAccountWithClaims(ctx context.Context, accessKey string) ( return acc, jwtClaims, nil } -// GetClaimsForSvcAcc - gets the claims associated with the service account. -func (sys *IAMSys) GetClaimsForSvcAcc(ctx context.Context, accessKey string) (map[string]any, error) { - if !sys.Initialized() { - return nil, errServerNotInitialized - } - - sa, ok := sys.store.GetUser(accessKey) - if !ok || !sa.Credentials.IsServiceAccount() { - return nil, errNoSuchServiceAccount - } - - jwtClaims, err := extractJWTClaims(sa) - if err != nil { - return nil, err - } - - return jwtClaims.Map(), nil -} - // DeleteServiceAccount - delete a service account func (sys *IAMSys) DeleteServiceAccount(ctx context.Context, accessKey string, notifyPeers bool) error { if !sys.Initialized() { diff --git a/cmd/site-replication.go b/cmd/site-replication.go index 6c82fa52b..0d0765d42 100644 --- a/cmd/site-replication.go +++ b/cmd/site-replication.go @@ -1256,9 +1256,10 @@ func (c *SiteReplicationSys) IAMChangeHook(ctx context.Context, item madmin.SRIA // The IAM item has already been applied to the local cluster at this // point, and only needs to be updated on all remote peer clusters. - c.RLock() - defer c.RUnlock() - if !c.enabled { + // What is sent is read from storage before the lock is taken: loading + // an identity asks whether site replication is enabled, and a read lock + // asked for a second time behind a waiting writer is never granted. + if !c.isEnabled() { return nil } @@ -1284,6 +1285,12 @@ func (c *SiteReplicationSys) IAMChangeHook(ctx context.Context, item madmin.SRIA if err != nil { return err } + + c.RLock() + defer c.RUnlock() + if !c.enabled { + return nil + } cerr := c.concDo(nil, func(d string, p madmin.PeerInfo) error { admClient, err := c.getAdminClient(ctx, d) if err != nil { @@ -2365,41 +2372,11 @@ func (c *SiteReplicationSys) syncToAllPeers(ctx context.Context, addOpts madmin. continue } - claims, err := globalIAMSys.GetClaimsForSvcAcc(ctx, acc.Credentials.AccessKey) - if err != nil { - return errSRBackendIssue(err) - } - - _, policy, err := globalIAMSys.GetServiceAccount(ctx, acc.Credentials.AccessKey) - if err != nil { - return errSRBackendIssue(err) - } - - var policyJSON []byte - if policy != nil { - policyJSON, err = json.Marshal(policy) - if err != nil { - return wrapSRErr(err) - } - } - + // The hook sends the account as it is stored (see replicationItem). err = c.IAMChangeHook(ctx, madmin.SRIAMItem{ - Type: madmin.SRIAMItemSvcAcc, - SvcAccChange: &madmin.SRSvcAccChange{ - Create: &madmin.SRSvcAccCreate{ - Parent: acc.Credentials.ParentUser, - AccessKey: user, - SecretKey: acc.Credentials.SecretKey, - Groups: acc.Credentials.Groups, - Claims: claims, - SessionPolicy: policyJSON, - Status: acc.Credentials.Status, - Name: acc.Credentials.Name, - Description: acc.Credentials.Description, - Expiration: &acc.Credentials.Expiration, - }, - }, - UpdatedAt: acc.UpdatedAt, + Type: madmin.SRIAMItemSvcAcc, + SvcAccChange: &madmin.SRSvcAccChange{Create: &madmin.SRSvcAccCreate{AccessKey: user}}, + UpdatedAt: acc.UpdatedAt, }) if err != nil { return errSRIAMError(err) @@ -5485,50 +5462,12 @@ func (c *SiteReplicationSys) healUsers(ctx context.Context, objAPI ObjectLayer, } creds := u.Credentials if creds.IsServiceAccount() { - claims, err := globalIAMSys.GetClaimsForSvcAcc(ctx, creds.AccessKey) - if err != nil { - replLogOnceIf(ctx, - fmt.Errorf("Unable to heal service account from peer site %s -> %s : %w", latestPeerName, peerName, err), - fmt.Sprintf("heal-user-%s", user)) - continue - } - - _, policy, err := globalIAMSys.GetServiceAccount(ctx, creds.AccessKey) - if err != nil { - replLogOnceIf(ctx, - fmt.Errorf("Unable to heal service account from peer site %s -> %s : %w", latestPeerName, peerName, err), - fmt.Sprintf("heal-user-%s", user)) - continue - } - - var policyJSON []byte - if policy != nil { - policyJSON, err = json.Marshal(policy) - if err != nil { - replLogOnceIf(ctx, - fmt.Errorf("Unable to heal service account from peer site %s -> %s : %w", latestPeerName, peerName, err), - fmt.Sprintf("heal-user-%s", user)) - continue - } - } - + // The hook sends the account as it is stored (see + // replicationItem), not as this server has it cached. if err := c.IAMChangeHook(ctx, madmin.SRIAMItem{ - Type: madmin.SRIAMItemSvcAcc, - SvcAccChange: &madmin.SRSvcAccChange{ - Create: &madmin.SRSvcAccCreate{ - Parent: creds.ParentUser, - AccessKey: creds.AccessKey, - SecretKey: creds.SecretKey, - Groups: creds.Groups, - Claims: claims, - SessionPolicy: policyJSON, - Status: creds.Status, - Name: creds.Name, - Description: creds.Description, - Expiration: &creds.Expiration, - }, - }, - UpdatedAt: lastUpdate, + Type: madmin.SRIAMItemSvcAcc, + SvcAccChange: &madmin.SRSvcAccChange{Create: &madmin.SRSvcAccCreate{AccessKey: creds.AccessKey}}, + UpdatedAt: lastUpdate, }); err != nil { replLogOnceIf(ctx, fmt.Errorf("Unable to heal service account from peer site %s -> %s : %w", latestPeerName, peerName, err),