diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go index 29a240f41..3a67c6688 100644 --- a/cmd/admin-handlers-users.go +++ b/cmd/admin-handlers-users.go @@ -622,7 +622,11 @@ func (a adminAPIHandlers) TemporaryAccountInfo(w http.ResponseWriter, r *http.Re policiesNames = policySet.ToSlice() } - stsAccountPolicy = globalIAMSys.GetCombinedPolicy(policiesNames...) + stsAccountPolicy, err = globalIAMSys.GetCombinedPolicy(policiesNames...) + if err != nil { + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) + return + } } policyJSON, err := json.MarshalIndent(stsAccountPolicy, "", " ") @@ -987,7 +991,11 @@ func (a adminAPIHandlers) InfoServiceAccount(w http.ResponseWriter, r *http.Requ writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return } - svcAccountPolicy = globalIAMSys.GetCombinedPolicy(policiesNames...) + svcAccountPolicy, err = globalIAMSys.GetCombinedPolicy(policiesNames...) + if err != nil { + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) + return + } } policyJSON, err := json.MarshalIndent(svcAccountPolicy, "", " ") @@ -1466,6 +1474,7 @@ func (a adminAPIHandlers) AccountInfoHandler(w http.ResponseWriter, r *http.Requ roleArn := policy.Args{Claims: cred.Claims}.GetRoleArn() policySetFromClaims, hasPolicyClaim := policy.GetPoliciesFromClaims(cred.Claims, iamPolicyClaimNameOpenID()) var effectivePolicy policy.Policy + var policyErr error var buf []byte switch { @@ -1489,10 +1498,10 @@ func (a adminAPIHandlers) AccountInfoHandler(w http.ResponseWriter, r *http.Requ return } policySlice := newMappedPolicy(policy).toSlice() - effectivePolicy = globalIAMSys.GetCombinedPolicy(policySlice...) + effectivePolicy, policyErr = globalIAMSys.GetCombinedPolicy(policySlice...) case hasPolicyClaim: - effectivePolicy = globalIAMSys.GetCombinedPolicy(policySetFromClaims.ToSlice()...) + effectivePolicy, policyErr = globalIAMSys.GetCombinedPolicy(policySetFromClaims.ToSlice()...) default: policies, err := globalIAMSys.PolicyDBGet(accountName, cred.Groups...) @@ -1501,7 +1510,11 @@ func (a adminAPIHandlers) AccountInfoHandler(w http.ResponseWriter, r *http.Requ writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return } - effectivePolicy = globalIAMSys.GetCombinedPolicy(policies...) + effectivePolicy, policyErr = globalIAMSys.GetCombinedPolicy(policies...) + } + if policyErr != nil { + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, policyErr), r.URL) + return } buf, err = json.MarshalIndent(effectivePolicy, "", " ") @@ -2189,7 +2202,11 @@ func (a adminAPIHandlers) InfoAccessKey(w http.ResponseWriter, r *http.Request) writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) return } - svcAccountPolicy = globalIAMSys.GetCombinedPolicy(policiesNames...) + svcAccountPolicy, err = globalIAMSys.GetCombinedPolicy(policiesNames...) + if err != nil { + writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL) + return + } } policyJSON, err := json.MarshalIndent(svcAccountPolicy, "", " ") @@ -2949,7 +2966,15 @@ func canManageRootCredentials(cred auth.Credentials) bool { func isAllowedServiceAccountAction(cred auth.Credentials, args policy.Args) bool { if args.DenyOnly && !args.IsOwner && !cred.IsTemp() && !cred.IsServiceAccount() && newGlobalAuthZPluginFn() == nil { policies, err := globalIAMSys.PolicyDBGet(args.AccountName, args.Groups...) - return err == nil && globalIAMSys.GetCombinedPolicy(policies...).IsAllowed(args) + if err != nil { + return false + } + combined, err := globalIAMSys.GetCombinedPolicy(policies...) + if err != nil { + iamLogIf(GlobalContext, err) + return false + } + return combined.IsAllowed(args) } return globalIAMSys.IsAllowed(args) } diff --git a/cmd/iam-policy-merge_test.go b/cmd/iam-policy-merge_test.go new file mode 100644 index 000000000..9ca55c3c7 --- /dev/null +++ b/cmd/iam-policy-merge_test.go @@ -0,0 +1,171 @@ +// 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 ( + "context" + "errors" + "net/http" + "testing" + + "github.com/pgsty/silo-pkg/v3/policy" +) + +func TestIAMPolicyMergeFailureDeniesAuthorization(t *testing.T) { + ctx, sys, _ := prepareIAMRevisionFixture(t) + const names = "merge-allow,merge-deny" + batchAuthzPolicy(ctx, t, sys, "merge-allow", `{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::merge-bucket/*"]}`) + batchAuthzPolicy(ctx, t, sys, "merge-deny", `{"Effect":"Deny","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::merge-bucket/protected"]}, +{"Effect":"Deny","Action":["admin:ListServiceAccounts"]}`) + cred := iamAdminUser(ctx, t, sys, "merge-user", names) + store := sys.store + original := store.IAMStorageAPI + t.Cleanup(func() { store.IAMStorageAPI = original }) + evict := func() { + cache := store.lock() + delete(cache.iamPolicyDocsMap, "merge-deny") + store.unlock() + } + args := policy.Args{AccountName: cred.AccessKey, Action: policy.GetObjectAction, BucketName: "merge-bucket", ObjectName: "protected"} + checks := []struct { + name string + allowed func() bool + }{ + {"regular", func() bool { return sys.IsAllowed(args) }}, + {"sts", func() bool { return sys.IsAllowedSTS(args, cred.AccessKey) }}, + {"service-account", func() bool { + saArgs := args + saArgs.Claims = map[string]any{parentClaim: cred.AccessKey, iamPolicyClaimNameSA(): inheritedPolicyType} + return sys.IsAllowedServiceAccount(saArgs, cred.AccessKey) + }}, + {"deny-only", func() bool { + return sys.doesPolicyAllow(names, policy.Args{DenyOnly: true, Action: policy.ListServiceAccountsAdminAction}) + }}, + {"self-service", func() bool { + return isAllowedServiceAccountAction(cred, policy.Args{AccountName: cred.AccessKey, DenyOnly: true, Action: policy.ListServiceAccountsAdminAction}) + }}, + } + for _, fault := range []struct { + name string + err error + }{ + {"quorum", errErasureReadQuorum}, + {"timeout", context.DeadlineExceeded}, + {"malformed", errors.New("invalid policy document")}, + } { + t.Run(fault.name, func(t *testing.T) { + evict() + store.IAMStorageAPI = &batchPolicyOutage{IAMStorageAPI: original, failed: "merge-deny", err: fault.err} + for _, check := range checks { + t.Run(check.name, func(t *testing.T) { + if check.allowed() { + t.Fatal("policy load failure discarded Deny and granted authorization") + } + }) + } + }) + } + // A successful retry must enforce the loaded Deny and keep its unaffected + // Allow. Neither an outage nor a deny may permanently poison the cache. + store.IAMStorageAPI = original + for _, check := range checks { + if check.allowed() { + t.Errorf("%s discarded Deny after recovery", check.name) + } + } + args.ObjectName = "unprotected" + if !sys.IsAllowed(args) { + t.Fatal("unaffected permission did not recover") + } +} + +func TestIAMPolicyMergeLoadResult(t *testing.T) { + ctx, sys, _ := prepareIAMRevisionFixture(t) + batchAuthzPolicy(ctx, t, sys, "merge-allow", `{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::merge-bucket/*"]}`) + store := sys.store + original := store.IAMStorageAPI + t.Cleanup(func() { store.IAMStorageAPI = original }) + for _, tc := range []struct { + name string + loadErr error + wantErr bool + }{ + {"confirmed-absent", errNoSuchPolicy, false}, + {"unavailable", errErasureReadQuorum, true}, + {"empty-success", nil, true}, + } { + t.Run(tc.name, func(t *testing.T) { + store.IAMStorageAPI = &batchPolicyOutage{IAMStorageAPI: original, failed: "merge-missing", err: tc.loadErr} + names, combined, err := store.MergePolicies("merge-allow,merge-missing") + if tc.wantErr { + if err == nil || names != "" || len(combined.Statements) != 0 { + t.Fatalf("incomplete policy selection returned a usable result: names=%q statements=%d err=%v", names, len(combined.Statements), err) + } + if tc.loadErr != nil && !errors.Is(err, tc.loadErr) { + t.Fatalf("lost policy load error: %v", err) + } + if _, err := sys.CurrentPolicies("merge-allow,merge-missing"); err == nil { + t.Fatal("STS policy selection hid a load failure") + } + if _, err := sys.GetCombinedPolicy("merge-allow", "merge-missing"); err == nil { + t.Fatal("combined policy hid a load failure") + } + } else { + mustIAM(t, err) + if names != "merge-allow" || !combined.IsAllowed(policy.Args{Action: policy.GetObjectAction, BucketName: "merge-bucket", ObjectName: "object"}) { + t.Fatal("confirmed absence changed the remaining policy") + } + } + }) + } + store.IAMStorageAPI = original + // A regular user's self inspection without any policy must still work. + cred := iamAdminUser(ctx, t, sys, "merge-no-policy", "") + if !isAllowedServiceAccountAction(cred, policy.Args{AccountName: cred.AccessKey, DenyOnly: true, Action: policy.ListServiceAccountsAdminAction}) { + t.Fatal("a valid empty policy set was mistaken for an unavailable policy") + } +} + +func TestIAMPolicyMergeInfoFailure(t *testing.T) { + ctx, sys := iamAdminFixture(t) + batchAuthzPolicy(ctx, t, sys, "merge-info-allow", `{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::merge-bucket/*"]}`) + batchAuthzPolicy(ctx, t, sys, "merge-info-deny", `{"Effect":"Deny","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::merge-bucket/protected"]}`) + parent := iamAdminUser(ctx, t, sys, "merge-info-user", "merge-info-allow,merge-info-deny") + svc := iamAdminServiceAccount(ctx, t, sys, parent.AccessKey, "merge-info-service", "") + store := sys.store + original := store.IAMStorageAPI + t.Cleanup(func() { store.IAMStorageAPI = original }) + cache := store.lock() + delete(cache.iamPolicyDocsMap, "merge-info-deny") + store.unlock() + store.IAMStorageAPI = &batchPolicyOutage{IAMStorageAPI: original, failed: "merge-info-deny", err: errErasureReadQuorum} + api := adminAPIHandlers{} + for _, tc := range []struct { + name string + handler http.HandlerFunc + }{ + {"info-service-account", api.InfoServiceAccount}, + {"info-access-key", api.InfoAccessKey}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := iamAdminCall(t, globalActiveCred, http.MethodGet, "/minio/admin/v3/"+tc.name+"?accessKey="+svc.AccessKey, nil, map[string]string{"accessKey": svc.AccessKey}, tc.handler) + if rec.Code < http.StatusInternalServerError { + t.Fatalf("incomplete effective policy returned as success: %d %s", rec.Code, rec.Body.String()) + } + }) + } +} diff --git a/cmd/iam-store.go b/cmd/iam-store.go index 1cfa9de4f..ddae74c47 100644 --- a/cmd/iam-store.go +++ b/cmd/iam-store.go @@ -1706,48 +1706,64 @@ func filterPolicies(cache *iamCache, policyName string, bucketName string) (stri return strings.Join(policies, ","), policy.MergePolicies(toMerge...) } -// MergePolicies - accepts a comma separated list of policy names as a string -// and returns only policies that currently exist in MinIO. It includes hot loading -// of policies if not in the memory -func (store *IAMStoreSys) MergePolicies(policyName string) (string, policy.Policy) { - var policies []string - var missingPolicies []string - var toMerge []policy.Policy - +// MergePolicies loads and combines the named policies. Only confirmed absent +// policies may be omitted: a failed read could hide an explicit Deny. +func (store *IAMStoreSys) MergePolicies(policyName string) (string, policy.Policy, error) { + names := newMappedPolicy(policyName).toSlice() + allCached := true cache := store.rlock() - for _, policy := range newMappedPolicy(policyName).toSlice() { - if policy == "" { + for _, name := range names { + if name == "" { continue } - p, found := cache.iamPolicyDocsMap[policy] - if !found { - missingPolicies = append(missingPolicies, policy) - continue + if _, found := cache.iamPolicyDocsMap[name]; !found { + allCached = false + break } - policies = append(policies, policy) - toMerge = append(toMerge, p.Policy) + } + if allCached { + names, combined := filterPolicies(cache, policyName, "") + store.runlock() + return names, combined, nil } store.runlock() - if len(missingPolicies) > 0 { - m := make(map[string]PolicyDoc) - for _, policy := range missingPolicies { + // Re-read every document under the writer lock so a concurrent edit or + // deletion cannot be overwritten by a stale lazy load. Publish the loaded + // documents only when the whole selection succeeds. + cache = store.lock() + defer store.unlock() + var policies []string + var toMerge []policy.Policy + loaded := make(map[string]PolicyDoc) + for _, name := range names { + if name == "" { + continue + } + p, found := cache.iamPolicyDocsMap[name] + if !found { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - _ = store.loadPolicyDoc(ctx, policy, m) + err := store.loadPolicyDoc(ctx, name, loaded) cancel() + if errors.Is(err, errNoSuchPolicy) { + continue + } + if err != nil { + return "", policy.Policy{}, fmt.Errorf("load IAM policy %q: %w", name, err) + } + p, found = loaded[name] + if !found { + return "", policy.Policy{}, fmt.Errorf("load IAM policy %q returned no document", name) + } } - - cache := store.lock() - maps.Copy(cache.iamPolicyDocsMap, m) - store.unlock() - - for policy, p := range m { - policies = append(policies, policy) - toMerge = append(toMerge, p.Policy) - } + policies = append(policies, name) + toMerge = append(toMerge, p.Policy) } - - return strings.Join(policies, ","), policy.MergePolicies(toMerge...) + if len(loaded) > 0 { + maps.Copy(cache.iamPolicyDocsMap, loaded) + cache.updatedAt = time.Now() + } + return strings.Join(policies, ","), policy.MergePolicies(toMerge...), nil } // GetBucketUsers - returns users (not STS or service accounts) that have access diff --git a/cmd/iam.go b/cmd/iam.go index 3cd5fffc2..2ff019e0c 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -763,13 +763,13 @@ func (sys *IAMSys) DeleteUser(ctx context.Context, accessKey string, notifyPeers // CurrentPolicies - returns comma separated policy string, from // an input policy after validating if there are any current // policies which exist on MinIO corresponding to the input. -func (sys *IAMSys) CurrentPolicies(policyName string) string { +func (sys *IAMSys) CurrentPolicies(policyName string) (string, error) { if !sys.Initialized() { - return "" + return "", errServerNotInitialized } - policies, _ := sys.store.MergePolicies(policyName) - return policies + policies, _, err := sys.store.MergePolicies(policyName) + return policies, err } func (sys *IAMSys) notifyForUser(ctx context.Context, accessKey string, isTemp bool) { @@ -2304,7 +2304,11 @@ func (sys *IAMSys) IsAllowedServiceAccount(args policy.Args, parentUser string) var combinedPolicy policy.Policy // Policies were found, evaluate all of them. if !isOwnerDerived { - availablePoliciesStr, c := sys.store.MergePolicies(strings.Join(svcPolicies, ",")) + availablePoliciesStr, c, err := sys.store.MergePolicies(strings.Join(svcPolicies, ",")) + if err != nil { + iamLogIf(GlobalContext, err) + return false + } if availablePoliciesStr == "" { return false } @@ -2397,7 +2401,11 @@ func (sys *IAMSys) IsAllowedSTS(args policy.Args, parentUser string) bool { var combinedPolicy policy.Policy // Policies were found, evaluate all of them. if !isOwnerDerived { - availablePoliciesStr, c := sys.store.MergePolicies(strings.Join(policies, ",")) + availablePoliciesStr, c, err := sys.store.MergePolicies(strings.Join(policies, ",")) + if err != nil { + iamLogIf(GlobalContext, err) + return false + } if availablePoliciesStr == "" { // all policies presented in the claim should exist iamLogIf(GlobalContext, fmt.Errorf("expected policy (%s) missing from the JWT claim %s, rejecting the request", policies, iamPolicyClaimNameOpenID())) @@ -2527,16 +2535,21 @@ func isAllowedBySessionPolicy(args policy.Args) (hasSessionPolicy bool, isAllowe } // GetCombinedPolicy returns a combined policy combining all policies -func (sys *IAMSys) GetCombinedPolicy(policies ...string) policy.Policy { - _, policy := sys.store.MergePolicies(strings.Join(policies, ",")) - return policy +func (sys *IAMSys) GetCombinedPolicy(policies ...string) (policy.Policy, error) { + _, combined, err := sys.store.MergePolicies(strings.Join(policies, ",")) + return combined, err } // doesPolicyAllow - checks if the given policy allows the passed action with given args. This is rarely needed. // Notice there is no account name involved, so this is a dangerous function. func (sys *IAMSys) doesPolicyAllow(policy string, args policy.Args) bool { // Policies were found, evaluate all of them. - return sys.GetCombinedPolicy(policy).IsAllowed(args) + combined, err := sys.GetCombinedPolicy(policy) + if err != nil { + iamLogIf(GlobalContext, err) + return false + } + return combined.IsAllowed(args) } // IsAllowed - checks given policy args is allowed to continue the Rest API. @@ -2590,7 +2603,12 @@ func (sys *IAMSys) isAllowedLocally(args policy.Args) bool { } // Policies were found, evaluate all of them. - return sys.GetCombinedPolicy(policies...).IsAllowed(args) + combined, err := sys.GetCombinedPolicy(policies...) + if err != nil { + iamLogIf(GlobalContext, err) + return false + } + return combined.IsAllowed(args) } // SetUsersSysType - sets the users system type, regular or LDAP. diff --git a/cmd/sts-handlers.go b/cmd/sts-handlers.go index 80f1038eb..4ffc649c7 100644 --- a/cmd/sts-handlers.go +++ b/cmd/sts-handlers.go @@ -797,7 +797,12 @@ func (sts *stsAPIHandlers) AssumeRoleWithSSO(w http.ResponseWriter, r *http.Requ policySet, ok := policy.GetPoliciesFromClaims(claims, iamPolicyClaimNameOpenID()) policies := strings.Join(policySet.ToSlice(), ",") if ok { - policyName = globalIAMSys.CurrentPolicies(policies) + current, err := globalIAMSys.CurrentPolicies(policies) + if err != nil { + writeSTSErrorResponse(ctx, w, ErrSTSInternalError, err) + return + } + policyName = current } if newGlobalAuthZPluginFn() == nil { @@ -881,7 +886,12 @@ func (sts *stsAPIHandlers) AssumeRoleWithSSO(w http.ResponseWriter, r *http.Requ } if newGlobalAuthZPluginFn() == nil { // if authZ is not set - we expect the policies to be present. - if globalIAMSys.CurrentPolicies(p) == "" { + current, err := globalIAMSys.CurrentPolicies(p) + if err != nil { + writeSTSErrorResponse(ctx, w, ErrSTSInternalError, err) + return + } + if current == "" { writeSTSErrorResponse(ctx, w, ErrSTSInvalidParameterValue, fmt.Errorf("None of the given policies (`%s`) are defined, credentials will not be generated", p)) return @@ -1366,7 +1376,12 @@ func (sts *stsAPIHandlers) AssumeRoleWithCustomToken(w http.ResponseWriter, r *h } if newGlobalAuthZPluginFn() == nil { // if authZ is not set - we expect the policyname to be present. - if globalIAMSys.CurrentPolicies(policyName) == "" { + current, err := globalIAMSys.CurrentPolicies(policyName) + if err != nil { + writeSTSErrorResponse(ctx, w, ErrSTSInternalError, err) + return + } + if current == "" { writeSTSErrorResponse(ctx, w, ErrSTSInvalidParameterValue, fmt.Errorf("None of the given policies (`%s`) are defined, credentials will not be generated", policyName)) return