From 1d714834c5457d0485f5ebd9e4b0aaff8a454eed Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 11 Nov 2026 11:01:50 +0800 Subject: [PATCH] fix(iam): normalize service policies and preserve imported state Keep omitted edit policies unchanged, reset valid empty policies to inheritance, and normalize nonempty policies without a version before storage. Preserve caller policy objects and exclude derived authorization claims from snapshots and new JWTs. Restore imported service-account and group status, report the actual group error, and cover creation, editing, replication, import/export and raw JWT claims with focused server tests. Refs #339, #345. Signed-off-by: Feng Ruohang (cherry picked from commit 86e4c09d47f48cb2f44fb2f63f02f222ac6ae102) --- cmd/admin-handlers-users-authz_test.go | 5 +- cmd/admin-handlers-users.go | 16 +- cmd/iam-import-state_test.go | 196 +++++++++++++++++++ cmd/iam-store.go | 27 +-- cmd/iam-svcacct-policy-edit_test.go | 131 +++++++++++++ cmd/iam-svcacct-replication-snapshot_test.go | 60 +++++- cmd/iam.go | 54 +++-- 7 files changed, 444 insertions(+), 45 deletions(-) create mode 100644 cmd/iam-import-state_test.go diff --git a/cmd/admin-handlers-users-authz_test.go b/cmd/admin-handlers-users-authz_test.go index 9d91a1167..f766e2961 100644 --- a/cmd/admin-handlers-users-authz_test.go +++ b/cmd/admin-handlers-users-authz_test.go @@ -169,8 +169,9 @@ func TestIAMAdminEmptyPolicyDelegation(t *testing.T) { {"{}", http.StatusOK, true}, {`{"Statement":null}`, http.StatusOK, true}, {limitedPolicy, http.StatusOK, false}, - {`{"Version":"2012-10-17"}`, http.StatusForbidden, false}, - {`{"Statement":[]}`, http.StatusForbidden, false}, + {`{"Version":"2012-10-17"}`, http.StatusOK, true}, + {`{"Statement":[]}`, http.StatusOK, true}, + {`{"Version":"2012-10-17","Statement":[]}`, http.StatusOK, true}, } { expires := UTCNow().Add(time.Hour) req := madmin.AddServiceAccountReq{Expiration: &expires} diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index bfd859ddd..8d82b846a 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -2618,8 +2618,17 @@ func (a adminAPIHandlers) importIAM(w http.ResponseWriter, r *http.Request, apiV return } } - if _, gerr := globalIAMSys.AddUsersToGroup(ctx, group, grpInfo.Members); gerr != nil { - failed.Groups = append(failed.Groups, madmin.IAMErrEntity{Name: group, Error: err}) + _, gerr := globalIAMSys.AddUsersToGroup(ctx, group, grpInfo.Members) + if gerr == nil { + switch grpInfo.Status { + case statusDisabled: + _, gerr = globalIAMSys.SetGroupStatus(ctx, group, false) + case statusEnabled: + _, gerr = globalIAMSys.SetGroupStatus(ctx, group, true) + } + } + if gerr != nil { + failed.Groups = append(failed.Groups, madmin.IAMErrEntity{Name: group, Error: gerr}) } else { added.Groups = append(added.Groups, group) } @@ -2699,6 +2708,7 @@ func (a adminAPIHandlers) importIAM(w http.ResponseWriter, r *http.Request, apiV opts := newServiceAccountOpts{ accessKey: user, secretKey: svcAcctReq.SecretKey, + status: svcAcctReq.Status, sessionPolicy: sp, claims: svcAcctReq.Claims, name: svcAcctReq.Name, @@ -3055,7 +3065,7 @@ func commonAddServiceAccount(r *http.Request, ldap bool) (context.Context, auth. } // Empty service policies mean inheritance, including for expiring keys. // Normalize the replication request as well as the local credential. - if sp.Version == "" && sp.Statements == nil && sp.ID == "" { + if sp.IsEmpty() { sp = nil createReq.Policy = nil } diff --git a/cmd/iam-import-state_test.go b/cmd/iam-import-state_test.go new file mode 100644 index 000000000..b7fe0f03b --- /dev/null +++ b/cmd/iam-import-state_test.go @@ -0,0 +1,196 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "archive/zip" + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/auth" + "github.com/pgsty/silo-pkg/v3/policy" +) + +func iamImportArchive(t *testing.T, assets map[string]any) []byte { + t.Helper() + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + for name, value := range assets { + w, err := zw.Create(pathJoin(iamAssetsDir, name)) + mustIAM(t, err) + mustIAM(t, json.NewEncoder(w).Encode(value)) + } + mustIAM(t, zw.Close()) + return buf.Bytes() +} + +func iamImportCall(t *testing.T, data []byte) *httptest.ResponseRecorder { + t.Helper() + req, err := newTestSignedRequestV4(http.MethodPut, "http://localhost/minio/admin/v3/import-iam-v2", int64(len(data)), bytes.NewReader(data), globalActiveCred.AccessKey, globalActiveCred.SecretKey, nil) + mustIAM(t, err) + mustIAM(t, req.ParseForm()) + rec := httptest.NewRecorder() + adminAPIHandlers{}.ImportIAMV2(rec, req.WithContext(newContext(req, rec, "ImportIAM"))) + if rec.Code != http.StatusOK { + t.Fatalf("import-iam-v2: %d %s", rec.Code, rec.Body.String()) + } + return rec +} + +func TestIAMImportExportState(t *testing.T) { + for _, target := range []string{"fresh", "existing"} { + t.Run(target, func(t *testing.T) { + ctx, sys := iamAdminFixture(t) + parent := iamAdminUser(ctx, t, sys, "import-parent", "") + for _, status := range []string{statusDisabled, statusEnabled} { + group := "import-" + status + _, err := sys.AddUsersToGroup(ctx, group, []string{parent.AccessKey}) + mustIAM(t, err) + _, err = sys.SetGroupStatus(ctx, group, status == statusEnabled) + mustIAM(t, err) + } + _, err := sys.PolicyDBSet(ctx, "import-disabled", "readwrite", regUser, true) + mustIAM(t, err) + sp, err := policy.ParseConfig(strings.NewReader(svcAccEditPolicy)) + mustIAM(t, err) + expires := UTCNow().Add(time.Hour).Truncate(time.Second) + _, _, err = sys.NewServiceAccount(ctx, parent.AccessKey, nil, newServiceAccountOpts{ + accessKey: "import-service", secretKey: "import-service-secret", status: auth.AccountOff, + sessionPolicy: sp, expiration: &expires, name: "disabled service", claims: map[string]any{"identity-attribute": "preserved"}, + }) + mustIAM(t, err) + exported := iamAdminCall(t, globalActiveCred, http.MethodGet, "/minio/admin/v3/export-iam", nil, nil, adminAPIHandlers{}.ExportIAM) + if exported.Code != http.StatusOK { + t.Fatalf("export-iam: %d %s", exported.Code, exported.Body.String()) + } + if target == "fresh" { + ctx, sys = iamAdminFixture(t) + } else { + // Import must also replace an existing enabled account/group and + // restore an enabled group that is disabled on the target. + _, err = sys.UpdateServiceAccount(ctx, "import-service", updateServiceAccountOpts{status: auth.AccountOn}) + mustIAM(t, err) + _, err = sys.SetGroupStatus(ctx, "import-disabled", true) + mustIAM(t, err) + _, err = sys.SetGroupStatus(ctx, "import-enabled", false) + mustIAM(t, err) + } + rec := iamImportCall(t, exported.Body.Bytes()) + var result madmin.ImportIAMResult + mustIAM(t, json.Unmarshal(rec.Body.Bytes(), &result)) + if len(result.Failed.Groups) != 0 || len(result.Failed.ServiceAccounts) != 0 || len(result.Added.ServiceAccounts) != 1 { + t.Fatalf("import did not restore the exported entities: %s", rec.Body.String()) + } + mustIAM(t, sys.store.LoadIAMCache(ctx, false)) + for _, status := range []string{statusDisabled, statusEnabled} { + group, err := sys.GetGroupDescription("import-" + status) + mustIAM(t, err) + if group.Status != status || len(group.Members) != 1 || group.Members[0] != parent.AccessKey { + t.Fatalf("group status or members changed: %+v", group) + } + } + if sys.IsAllowed(policy.Args{AccountName: parent.AccessKey, Action: policy.GetObjectAction, BucketName: "edit", ObjectName: "object"}) { + t.Error("the disabled imported group grants access") + } + cr, gotPolicy, err := sys.GetServiceAccount(ctx, "import-service") + mustIAM(t, err) + if cr.Status != auth.AccountOff || !cr.Expiration.Equal(expires) || cr.Name != "disabled service" || gotPolicy == nil || !gotPolicy.Equals(*sp) { + t.Fatal("import changed the service account status, expiration, name or inline policy") + } + if _, valid, err := sys.CheckKey(ctx, "import-service"); err != nil || valid { + t.Fatalf("imported disabled credential is usable: valid=%v err=%v", valid, err) + } + var stored UserIdentity + mustIAM(t, sys.store.loadIAMConfig(ctx, &stored, getUserIdentityPath("import-service", svcUser))) + raw, err := auth.ExtractClaims(stored.Credentials.SessionToken, stored.Credentials.SecretKey) + mustIAM(t, err) + if _, ok := raw.Map()[sessionPolicyNameExtracted]; ok || raw.Map()["identity-attribute"] != "preserved" { + t.Fatal("import polluted the raw JWT or lost identity claims") + } + // Old exports may contain a derived claim and omit the policy version. + snapshot, err := serviceAccountSnapshot(stored) + mustIAM(t, err) + snapshot.Claims[sessionPolicyNameExtracted] = "stale decoded policy" + snapshot.SessionPolicy = []byte(strings.ReplaceAll(svcAccEditPolicy, `"Version":"2012-10-17",`, "")) + rec = iamImportCall(t, iamImportArchive(t, map[string]any{allSvcAcctsFile: map[string]madmin.SRSvcAccCreate{"import-service": snapshot}})) + mustIAM(t, json.Unmarshal(rec.Body.Bytes(), &result)) + if len(result.Failed.ServiceAccounts) != 0 { + t.Fatalf("legacy snapshot import failed: %s", rec.Body.String()) + } + _, gotPolicy, err = sys.GetServiceAccount(ctx, "import-service") + mustIAM(t, err) + if gotPolicy == nil || !gotPolicy.Equals(*sp) { + t.Fatal("import did not normalize the inline policy") + } + mustIAM(t, sys.store.loadIAMConfig(ctx, &stored, getUserIdentityPath("import-service", svcUser))) + raw, err = auth.ExtractClaims(stored.Credentials.SessionToken, stored.Credentials.SecretKey) + mustIAM(t, err) + if _, ok := raw.Map()[sessionPolicyNameExtracted]; ok { + t.Fatal("legacy import signed a derived claim into the JWT") + } + }) + } +} + +type iamImportGroupFailureStore struct { + IAMStorageAPI + statusOnly bool + err error +} + +func (s *iamImportGroupFailureStore) saveIAMConfig(ctx context.Context, item any, path string, opts ...options) error { + mutation, _ := ctx.Value(iamGroupMutationKey{}).(iamGroupMutation) + if path == getGroupInfoPath("import-failure") && mutation.StatusOnly == s.statusOnly { + return s.err + } + return s.IAMStorageAPI.saveIAMConfig(ctx, item, path, opts...) +} + +func TestIAMImportGroupFailure(t *testing.T) { + for _, operation := range []string{"members", "status"} { + t.Run(operation, func(t *testing.T) { + _, sys := iamAdminFixture(t) + injected := madmin.ErrorResponse{Code: "InjectedGroupWrite", Message: operation + " write failed"} + sys.store.IAMStorageAPI = &iamImportGroupFailureStore{IAMStorageAPI: sys.store.IAMStorageAPI, statusOnly: operation == "status", err: injected} + rec := iamImportCall(t, iamImportArchive(t, map[string]any{allGroupsFile: map[string]GroupInfo{ + "import-failure": {Status: statusDisabled}, + }})) + var result struct { + Added madmin.IAMEntities `json:"added"` + Failed struct { + Groups []struct { + Name string `json:"name"` + Error json.RawMessage `json:"error"` + } `json:"groups"` + } `json:"failed"` + } + mustIAM(t, json.Unmarshal(rec.Body.Bytes(), &result)) + wantError, err := json.Marshal(injected) + mustIAM(t, err) + if len(result.Added.Groups) != 0 || len(result.Failed.Groups) != 1 || result.Failed.Groups[0].Name != "import-failure" || !bytes.Equal(result.Failed.Groups[0].Error, wantError) { + t.Fatalf("import did not report the actual %s error: %s", operation, rec.Body.String()) + } + }) + } +} diff --git a/cmd/iam-store.go b/cmd/iam-store.go index 685fa46a8..d7d483a48 100644 --- a/cmd/iam-store.go +++ b/cmd/iam-store.go @@ -20,7 +20,6 @@ package cmd import ( "context" "encoding/base64" - "encoding/json" "errors" "fmt" "maps" @@ -2934,27 +2933,15 @@ func updatedServiceAccountCredentials(ctx context.Context, accessKey string, cr // 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) - } - - if opts.sessionPolicy != nil { // session policies is being updated - if err := opts.sessionPolicy.Validate(); err != nil { + if opts.sessionPolicy != nil { + policyBuf, err := marshalServiceAccountPolicy(opts.sessionPolicy) + if err != nil { return cr, err } - - if opts.sessionPolicy.Version != "" && len(opts.sessionPolicy.Statements) > 0 { - policyBuf, err := json.Marshal(opts.sessionPolicy) - if err != nil { - return cr, err - } - - if len(policyBuf) > maxSVCSessionPolicySize { - return cr, errSessionPolicyTooLarge - } - + if len(policyBuf) == 0 { + m.Delete(policy.SessionPolicyName) + m.Set(iamPolicyClaimNameSA(), inheritedPolicyType) + } else { // Overwrite session policy claims. m.Set(policy.SessionPolicyName, base64.StdEncoding.EncodeToString(policyBuf)) m.Set(iamPolicyClaimNameSA(), embeddedPolicyType) diff --git a/cmd/iam-svcacct-policy-edit_test.go b/cmd/iam-svcacct-policy-edit_test.go index 5d00c6b75..4240629dc 100644 --- a/cmd/iam-svcacct-policy-edit_test.go +++ b/cmd/iam-svcacct-policy-edit_test.go @@ -4,9 +4,12 @@ package cmd import ( + "bytes" "context" + "encoding/base64" "encoding/json" "fmt" + "reflect" "runtime" "strings" "testing" @@ -25,6 +28,133 @@ import ( const svcAccEditPolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::edit/*"]}]}` +func TestIAMServiceAccountPolicySemantics(t *testing.T) { + for _, expiring := range []bool{false, true} { + t.Run(fmt.Sprintf("expiring=%v", expiring), func(t *testing.T) { + f := newGroupRefreshFixture(t) + readPolicy, err := policy.ParseConfig(strings.NewReader(svcAccEditPolicy)) + mustIAM(t, err) + writePolicy, err := policy.ParseConfig(strings.NewReader(strings.ReplaceAll(svcAccEditPolicy, "s3:GetObject", "s3:PutObject"))) + mustIAM(t, err) + _, err = f.sys.PolicyDBSet(f.ctx, f.parent.AccessKey, "readwrite", regUser, false) + mustIAM(t, err) + for _, action := range []policy.Action{policy.GetObjectAction, policy.PutObjectAction} { + if !f.sys.IsAllowed(policy.Args{AccountName: f.parent.AccessKey, Action: action, BucketName: "edit", ObjectName: "object"}) { + t.Fatal("control: the parent must allow reads and writes to the test object") + } + } + noVersion, invalidVersion, oversized := *writePolicy, *writePolicy, *writePolicy + noVersion.Version = "" + invalidVersion.Version = "2008-10-17" + oversized.ID = policy.ID(strings.Repeat("x", maxSVCSessionPolicySize)) + var expiration *time.Time + if expiring { + later := UTCNow().Add(time.Hour) + expiration = &later + } + for i, tc := range []struct { + name string + input *policy.Policy + invalid bool + }{ + {"nil", nil, false}, + {"empty", &policy.Policy{}, false}, + {"empty-statements", &policy.Policy{Statements: []policy.Statement{}}, false}, + {"empty-with-version", &policy.Policy{Version: policy.DefaultVersion, Statements: []policy.Statement{}}, false}, + {"missing-version", &noVersion, false}, + {"version", writePolicy, false}, + {"invalid-version", &invalidVersion, true}, + {"empty-invalid-version", &policy.Policy{Version: "2008-10-17"}, true}, + {"invalid-statement", &policy.Policy{Statements: []policy.Statement{{}}}, true}, + {"oversized", &oversized, true}, + } { + for _, edit := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/edit=%v", tc.name, edit), func(t *testing.T) { + key := fmt.Sprintf("policy-%d-%v", i, edit) + inputBefore, err := json.Marshal(tc.input) + mustIAM(t, err) + var before UserIdentity + if edit { + _, _, err = f.sys.NewServiceAccount(f.ctx, f.parent.AccessKey, nil, newServiceAccountOpts{ + accessKey: key, secretKey: "policy-valid-secret", sessionPolicy: readPolicy, expiration: expiration, + }) + mustIAM(t, err) + mustIAM(t, f.sys.store.loadIAMConfig(f.ctx, &before, getUserIdentityPath(key, svcUser))) + _, err = f.sys.UpdateServiceAccount(f.ctx, key, updateServiceAccountOpts{sessionPolicy: tc.input, description: "edited"}) + } else { + // Snapshots can carry a stale copy of the policy in claims. + var claims map[string]any + if tc.input != nil { + claims = map[string]any{policy.SessionPolicyName: base64.StdEncoding.EncodeToString([]byte(svcAccEditPolicy))} + } + _, _, err = f.sys.NewServiceAccount(f.ctx, f.parent.AccessKey, nil, newServiceAccountOpts{ + accessKey: key, secretKey: "policy-valid-secret", sessionPolicy: tc.input, expiration: expiration, + claims: claims, + }) + } + if (err != nil) != tc.invalid { + t.Fatalf("operation error = %v, want rejected=%v", err, tc.invalid) + } + inputAfter, err := json.Marshal(tc.input) + mustIAM(t, err) + if !bytes.Equal(inputBefore, inputAfter) { + t.Fatal("operation changed the caller's policy") + } + var stored UserIdentity + loadErr := f.sys.store.loadIAMConfig(f.ctx, &stored, getUserIdentityPath(key, svcUser)) + if tc.invalid { + if edit { + mustIAM(t, loadErr) + if !reflect.DeepEqual(stored, before) { + t.Fatal("rejected edit changed the stored account") + } + } else if loadErr != errConfigNotFound { + t.Fatalf("rejected create left an account: %v", loadErr) + } + return + } + mustIAM(t, loadErr) + claims, err := auth.ExtractClaims(stored.Credentials.SessionToken, stored.Credentials.SecretKey) + mustIAM(t, err) + if _, ok := claims.Map()[sessionPolicyNameExtracted]; ok { + t.Fatal("raw JWT contains the derived policy claim") + } + var want *policy.Policy + if tc.input != nil && !tc.input.IsEmpty() { + want = writePolicy + } else if tc.input == nil && edit { + want = readPolicy + } + got, err := embeddedServiceAccountPolicy(claims) + mustIAM(t, err) + if (got == nil) != (want == nil) || (got != nil && !got.Equals(*want)) { + t.Fatalf("stored policy = %+v, want %+v", got, want) + } + if want == nil { + if _, ok := claims.Lookup(policy.SessionPolicyName); ok || claims.Map()[iamPolicyClaimNameSA()] != inheritedPolicyType { + t.Fatal("inheritance must remove the embedded policy claim") + } + } + mustIAM(t, f.sys.store.LoadIAMCache(f.ctx, false)) + u, valid, err := f.sys.CheckKey(f.ctx, key) + mustIAM(t, err) + if !valid || u.Credentials.IsTemp() != expiring { + t.Fatal("credential validity or expiration changed") + } + for _, action := range []policy.Action{policy.GetObjectAction, policy.PutObjectAction} { + allowed := f.sys.IsAllowed(policy.Args{AccountName: key, Claims: u.Credentials.Claims, Action: action, BucketName: "edit", ObjectName: "object"}) + wantAllowed := want == nil || (want == readPolicy && action == policy.GetObjectAction) || (want == writePolicy && action == policy.PutObjectAction) + if allowed != wantAllowed { + t.Errorf("%s allowed=%v, want %v", action, allowed, wantAllowed) + } + } + }) + } + } + }) + } +} + func TestIAMServiceAccountEditKeepsPolicy(t *testing.T) { f := newGroupRefreshFixture(t) want, err := policy.ParseConfig(strings.NewReader(svcAccEditPolicy)) @@ -110,6 +240,7 @@ func TestIAMServiceAccountEditKeepsPolicy(t *testing.T) { {"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}, + {"peer-empty-v", madmin.SRSvcAccUpdate{SessionPolicy: madmin.SRSessionPolicy(`{"Version":"2012-10-17","Statement":[]}`)}, false, nil}, } { t.Run(edit.name, func(t *testing.T) { key := "edit-" + edit.name diff --git a/cmd/iam-svcacct-replication-snapshot_test.go b/cmd/iam-svcacct-replication-snapshot_test.go index e8259e7f2..e7bfc96e3 100644 --- a/cmd/iam-svcacct-replication-snapshot_test.go +++ b/cmd/iam-svcacct-replication-snapshot_test.go @@ -14,6 +14,7 @@ import ( "net/http" "net/http/httptest" "path" + "reflect" "strings" "sync" "testing" @@ -59,6 +60,11 @@ func svcAccStored(t *testing.T, f *groupRefreshFixture, key string) svcAccRecord t.Fatalf("service account %s is not stored", key) } cr := u.Credentials + rawClaims, err := auth.ExtractClaims(cr.SessionToken, cr.SecretKey) + mustIAM(t, err) + if _, ok := rawClaims.Map()[sessionPolicyNameExtracted]; ok { + t.Fatal("stored JWT contains the derived policy claim") + } 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} @@ -172,6 +178,11 @@ func svcAccPeer(t *testing.T, from *svcAccSite, key string, items ...iamReplicat mustIAM(t, err) var received iamReplicationItem mustIAM(t, json.Unmarshal(raw, &received)) + if create := received.SvcAccChange.Create; create != nil { + if _, ok := create.Claims[sessionPolicyNameExtracted]; ok { + t.Fatal("replication snapshot contains the derived policy claim") + } + } if err := applyIAMReplicationItem(peer.ctx, received); err != nil { t.Fatalf("the peer refused item %d: %v", i, err) } @@ -215,15 +226,19 @@ func TestSiteReplicationServiceAccountEdits(t *testing.T) { {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{ + {"policy-removed-with-version", []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"}, + {NewDescription: "described"}, }, func(r svcAccRecord) bool { - return r.Policy == svcAccPolicyText(t, []byte(svcAccSnapshotPolicy)) && r.Name == "named" && r.Description == "described" + return r.Policy == "" && r.Name == "named" && r.Description == "described" + }}, + {"policy-missing-version", []madmin.UpdateServiceAccountReq{ + {NewPolicy: json.RawMessage(svcAccSnapshotPolicy)}, + {NewPolicy: json.RawMessage(strings.ReplaceAll(strings.ReplaceAll(svcAccSnapshotPolicy, `"Version":"2012-10-17",`, ""), "s3:GetObject", "s3:PutObject"))}, + {NewDescription: "described"}, + }, func(r svcAccRecord) bool { + return r.Policy == svcAccPolicyText(t, []byte(strings.ReplaceAll(svcAccSnapshotPolicy, "s3:GetObject", "s3:PutObject"))) && r.Description == "described" }}, } { for _, delivery := range []string{"late", "lost"} { @@ -360,9 +375,42 @@ func TestSiteReplicationServiceAccountStaleCache(t *testing.T) { if a.Claims[iamPolicyClaimNameSA()] != embeddedPolicyType { t.Errorf("exported claims say %v, want %s", a.Claims[iamPolicyClaimNameSA()], embeddedPolicyType) } + if _, ok := a.Claims[sessionPolicyNameExtracted]; ok { + t.Error("exported claims contain the derived policy claim") + } }) } +func TestIAMServiceAccountSnapshotClaims(t *testing.T) { + f := newGroupRefreshFixture(t) + sp, err := policy.ParseConfig(strings.NewReader(svcAccSnapshotPolicy)) + mustIAM(t, err) + _, _, err = f.sys.NewServiceAccount(f.ctx, f.parent.AccessKey, nil, newServiceAccountOpts{ + accessKey: "snapshot-claims", secretKey: "snapshot-valid-secret", sessionPolicy: sp, + claims: map[string]any{"identity-attribute": "preserved"}, + }) + mustIAM(t, err) + u, ok := f.stored(t, "snapshot-claims") + if !ok { + t.Fatal("service account missing") + } + cr := u.Credentials + rawClaims, err := auth.ExtractClaims(cr.SessionToken, cr.SecretKey) + mustIAM(t, err) + derived, ok := cr.Claims[sessionPolicyNameExtracted] + if !ok { + t.Fatal("control: loaded claims must contain the derived policy") + } + snapshot, err := serviceAccountSnapshot(u) + mustIAM(t, err) + if !reflect.DeepEqual(snapshot.Claims, rawClaims.Map()) { + t.Fatal("snapshot claims differ from the raw JWT") + } + if u.Credentials.SessionToken != cr.SessionToken || u.Credentials.Claims[sessionPolicyNameExtracted] != derived { + t.Fatal("snapshot changed the source token or its cached claims") + } +} + // 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 diff --git a/cmd/iam.go b/cmd/iam.go index a89415c27..aa115c0a1 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -1098,6 +1098,32 @@ type newServiceAccountOpts struct { claims map[string]any } +// marshalServiceAccountPolicy validates a supplied policy and normalizes its +// version without changing the caller's policy. An empty policy means inheritance. +func marshalServiceAccountPolicy(sp *policy.Policy) ([]byte, error) { + if sp == nil { + return nil, nil + } + p := *sp + if p.Version == "" { + p.Version = policy.DefaultVersion + } + if err := p.Validate(); err != nil { + return nil, err + } + if p.IsEmpty() { + return nil, nil + } + buf, err := json.Marshal(p) + if err != nil { + return nil, err + } + if len(buf) > maxSVCSessionPolicySize { + return nil, errSessionPolicyTooLarge + } + return buf, nil +} + // NewServiceAccount - create a new service account func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, groups []string, opts newServiceAccountOpts) (auth.Credentials, time.Time, error) { if !sys.Initialized() { @@ -1115,19 +1141,9 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, gro return auth.Credentials{}, time.Time{}, auth.ErrNoAccessKeyWithSecretKey } - var policyBuf []byte - if opts.sessionPolicy != nil { - err := opts.sessionPolicy.Validate() - if err != nil { - return auth.Credentials{}, time.Time{}, err - } - policyBuf, err = json.Marshal(opts.sessionPolicy) - if err != nil { - return auth.Credentials{}, time.Time{}, err - } - if len(policyBuf) > maxSVCSessionPolicySize { - return auth.Credentials{}, time.Time{}, errSessionPolicyTooLarge - } + policyBuf, err := marshalServiceAccountPolicy(opts.sessionPolicy) + if err != nil { + return auth.Credentials{}, time.Time{}, err } // found newly requested service account, to be same as @@ -1150,6 +1166,14 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, gro // Add all the necessary claims for the service account. for k, v := range opts.claims { + // Older snapshots may carry this cache-only, decoded policy claim. + if k == sessionPolicyNameExtracted { + continue + } + // An explicit policy, including an empty one, owns this claim. + if k == policy.SessionPolicyName && opts.sessionPolicy != nil { + continue + } _, ok := m[k] if !ok { m[k] = v @@ -1162,7 +1186,6 @@ func (sys *IAMSys) NewServiceAccount(ctx context.Context, parentUser string, gro } var accessKey, secretKey string - var err error if len(opts.accessKey) > 0 || len(opts.secretKey) > 0 { accessKey, secretKey = opts.accessKey, opts.secretKey } else { @@ -1344,6 +1367,9 @@ func serviceAccountSnapshot(u UserIdentity) (madmin.SRSvcAccCreate, error) { if err != nil { return madmin.SRSvcAccCreate{}, err } + // Extraction adds a decoded policy for local authorization. Transport only + // the token's claims, so peers and imports do not sign this derived value. + jwtClaims.Delete(sessionPolicyNameExtracted) embeddedPolicy, err := embeddedServiceAccountPolicy(jwtClaims) if err != nil { return madmin.SRSvcAccCreate{}, err