iam: preserve native Admin when IAM policies are attached (#11226) (#11232)

* iam: expose tri-state result from attached policy evaluation evaluateIAMPolicies returned a bool that collapsed explicit Deny and no-match into a single false, so the authorization path could not tell "policies forbid this" from "policies say nothing". Introduce evaluateAttachedIAMPolicies returning Allow/Deny/NoMatch and keep evaluateIAMPolicies as a bool projection for existing callers. This is preparation for unioning native permissions with attached policies while preserving deny-always-wins. * iam: preserve native Admin when IAM policies are attached Attaching an IAM policy routed authorization exclusively to the attached policies, dropping the identity native permissions. A user with native Admin lost all access after attaching a non-granting policy, and stayed locked out if that policy was deleted without being detached first (#11226). Treat a native bare Admin grant as a permission floor that survives attached policies: when the attached policies do not explicitly allow, fall back to isAdmin() on the attached-policy path, and on the IAM integration path allow unless an attached policy explicitly denies. Explicit Deny still wins on both paths. Only bare Admin is consulted because inline policies flatten lossily into Actions (dropping conditions), so scoped actions are not unambiguously native and must keep flowing through the policy engine. * iam: regression tests for native Admin surviving attached policies Reproduces issue #11226: - TestNativeAdminSurvivesAttachedPolicy: a user with native Admin keeps Write access after attaching a policy that does not grant it. - TestNativeAdminSurvivesDeletedPolicy: the same user keeps Write access after the attached policy is deleted without being detached. - TestAttachedPolicyExplicitDenyOverridesNativeAdmin: an explicit Deny in an attached policy still constrains a native admin (deny-always-wins). * iam: apply native Admin floor before IAM principal validation The native Admin floor in authorizeWithIAM ran after the auth-path switch, which denies when no session principal or PrincipalArn is present. An Admin identity without a PrincipalArn (no session token) was therefore denied before the floor executed. Move the floor ahead of the switch and derive the principal for its explicit-deny check with buildPrincipalARN, which already handles identities without a PrincipalArn. Adds a regression case for an Admin identity with an empty PrincipalArn. Addresses CodeRabbit review feedback on PR #11232.
2026-09-16 03:20:50 +02:00 · 2026-09-08 14:39:43 -07:00
parent 4a1d65939f
commit 557fffa350
2 changed files with 193 additions and 12 deletions
@@ -2452,12 +2452,22 @@ func determineIAMAuthPath(sessionToken, principal, principalArn string) iamAuthP
 	return iamAuthPathNone
 }

-// evaluateIAMPolicies evaluates attached IAM policies for a user identity.
-// Returns true if any matching statement explicitly allows the action.
-// Uses the cached iamPolicyEngine to avoid re-parsing policy JSON on every request.
-func (iam *IdentityAccessManagement) evaluateIAMPolicies(r *http.Request, identity *Identity, action Action, bucket, object string) bool {
+// attachedIAMPolicyResult is the tri-state outcome of evaluating an identity's
+// attached IAM policies: explicit Allow, explicit Deny, or no match.
+type attachedIAMPolicyResult int
+
+const (
+	attachedIAMPolicyNoMatch attachedIAMPolicyResult = iota
+	attachedIAMPolicyAllow
+	attachedIAMPolicyDeny
+)
+
+// evaluateAttachedIAMPolicies evaluates the identity's own and group attached
+// IAM policies and reports whether they explicitly allow, deny, or do not
+// match the action.
+func (iam *IdentityAccessManagement) evaluateAttachedIAMPolicies(r *http.Request, identity *Identity, action Action, bucket, object string) attachedIAMPolicyResult {
 	if identity == nil {
-		return false
+		return attachedIAMPolicyNoMatch
 	}

 	iam.m.RLock()
@@ -2479,11 +2489,11 @@ func (iam *IdentityAccessManagement) evaluateIAMPolicies(r *http.Request, identi

 	// Collect all policy names: user policies + group policies
 	if len(identity.PolicyNames) == 0 && len(groupPolicies) == 0 {
-		return false
+		return attachedIAMPolicyNoMatch
 	}

 	if engine == nil {
-		return false
+		return attachedIAMPolicyNoMatch
 	}

 	// List is bucket-level; the prefix promoted into object (for the legacy
@@ -2514,7 +2524,7 @@ func (iam *IdentityAccessManagement) evaluateIAMPolicies(r *http.Request, identi
 	for _, policyName := range identity.PolicyNames {
 		result := engine.EvaluatePolicy(policyName, evalArgs)
 		if result == policy_engine.PolicyResultDeny {
-			return false
+			return attachedIAMPolicyDeny
 		}
 		if result == policy_engine.PolicyResultAllow {
 			explicitAllow = true
@@ -2526,7 +2536,7 @@ func (iam *IdentityAccessManagement) evaluateIAMPolicies(r *http.Request, identi
 		for _, policyName := range policyNames {
 			result := engine.EvaluatePolicy(policyName, evalArgs)
 			if result == policy_engine.PolicyResultDeny {
-				return false
+				return attachedIAMPolicyDeny
 			}
 			if result == policy_engine.PolicyResultAllow {
 				explicitAllow = true
@@ -2534,7 +2544,16 @@ func (iam *IdentityAccessManagement) evaluateIAMPolicies(r *http.Request, identi
 		}
 	}

-	return explicitAllow
+	if explicitAllow {
+		return attachedIAMPolicyAllow
+	}
+	return attachedIAMPolicyNoMatch
+}
+
+// evaluateIAMPolicies is a bool projection of evaluateAttachedIAMPolicies for
+// callers that only need the allow outcome.
+func (iam *IdentityAccessManagement) evaluateIAMPolicies(r *http.Request, identity *Identity, action Action, bucket, object string) bool {
+	return iam.evaluateAttachedIAMPolicies(r, identity, action, bucket, object) == attachedIAMPolicyAllow
 }

 // isActionExplicitlyDeniedByIAM reports whether the identity's attached IAM
@@ -2671,10 +2690,20 @@ func (iam *IdentityAccessManagement) VerifyActionPermission(r *http.Request, ide
 		// field is a lossy projection that cannot represent deny statements,
 		// conditions, or fine-grained action differences such as PutObject vs
 		// DeleteObject.
-		if iam.evaluateIAMPolicies(r, identity, action, bucket, object) {
+		switch iam.evaluateAttachedIAMPolicies(r, identity, action, bucket, object) {
+		case attachedIAMPolicyAllow:
 			return s3err.ErrNone
+		case attachedIAMPolicyDeny:
+			return s3err.ErrAccessDenied
+		default:
+			// No matching statement: a native bare Admin grant survives
+			// attaching a policy (issue #11226). Scoped actions are not
+			// consulted because inline policies flatten lossily into Actions.
+			if identity.isAdmin() {
+				return s3err.ErrNone
+			}
+			return s3err.ErrAccessDenied
 		}
-		return s3err.ErrAccessDenied
 	case authorizeViaLegacyActions:
 		if !identity.CanDo(action, bucket, object) {
 			return s3err.ErrAccessDenied
@@ -2901,6 +2930,19 @@ func (iam *IdentityAccessManagement) authorizeWithIAM(r *http.Request, identity
 		Claims:      identity.Claims, // Copy claims for policy variable substitution
 	}

+	// A native bare Admin grant survives attaching a policy (issue #11226);
+	// an explicit Deny in an attached policy still wins. This runs before the
+	// auth-path switch so an Admin identity without a session principal or
+	// PrincipalArn is still authorized through its native grant.
+	if identity.isAdmin() {
+		s3Action, resourceArn := resolveS3AuthTarget(action, bucket, object, r)
+		principal := buildPrincipalARN(identity, r)
+		if !iam.isActionExplicitlyDeniedByIAM(r, identity, principal, s3Action, resourceArn) {
+			return s3err.ErrNone
+		}
+		return s3err.ErrAccessDenied
+	}
+
 	// Determine authorization path and configure identity
 	authPath := determineIAMAuthPath(sessionToken, principal, identity.PrincipalArn)
 	switch authPath {
@@ -2928,6 +2970,19 @@ func (iam *IdentityAccessManagement) authorizeWithIAM(r *http.Request, identity
 	return iam.iamIntegration.AuthorizeAction(ctx, iamIdentity, action, bucket, object, r)
 }

+// resolveS3AuthTarget mirrors the action and resource resolution that
+// AuthorizeAction applies, so the native-permission floor's explicit-deny
+// check evaluates the same action and resource ARN as the policy engine.
+func resolveS3AuthTarget(action Action, bucket, object string, r *http.Request) (s3Action, resourceArn string) {
+	resourceObjectKey := object
+	if action == s3_constants.ACTION_LIST {
+		resourceObjectKey = ""
+	}
+	resourceArn = buildS3ResourceArn(bucket, resourceObjectKey)
+	s3Action = ResolveS3Action(r, string(action), bucket, object)
+	return
+}
+
 // PutPolicy adds or updates a policy
 func (iam *IdentityAccessManagement) PutPolicy(name string, content string) error {
 	iam.m.Lock()
@@ -0,0 +1,126 @@
+package s3api
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func newNativeFloorTestIAM(t *testing.T) (*IdentityAccessManagement, *Identity) {
+	t.Helper()
+	mgr := newTestIAMManager(t)
+	iam := &IdentityAccessManagement{}
+	iam.SetIAMIntegration(NewS3IAMIntegration(mgr, ""))
+
+	doc, _ := json.Marshal(map[string]interface{}{
+		"Version": "2012-10-17",
+		"Statement": []map[string]interface{}{
+			{"Effect": "Allow", "Action": "s3:ListAllMyBuckets", "Resource": "*"},
+		},
+	})
+	require.NoError(t, iam.PutPolicy("ListBucketsOnly", string(doc)))
+
+	identity := &Identity{
+		Name:         "admin",
+		Account:      &Account{DisplayName: "admin", Id: "admin"},
+		Actions:      []Action{s3_constants.ACTION_ADMIN},
+		PolicyNames:  []string{"ListBucketsOnly"},
+		PrincipalArn: "arn:aws:iam::111122223333:user/admin",
+	}
+	return iam, identity
+}
+
+func putObjectRequest() *http.Request {
+	return httptest.NewRequest(http.MethodPut, "/mybucket/file.txt", nil)
+}
+
+// TestNativeAdminSurvivesAttachedPolicy reproduces issue #11226: attaching an
+// IAM policy to a user with native Admin must not override the native grant.
+// The effective permissions are the union of native permissions and attached
+// policy grants, so a Write the attached policy never mentions still succeeds.
+func TestNativeAdminSurvivesAttachedPolicy(t *testing.T) {
+	iam, identity := newNativeFloorTestIAM(t)
+
+	errCode := iam.VerifyActionPermission(putObjectRequest(), identity,
+		s3_constants.ACTION_WRITE, "mybucket", "file.txt")
+	assert.Equal(t, s3err.ErrNone, errCode,
+		"native Admin must remain effective after attaching a policy")
+}
+
+// TestNativeAdminSurvivesDeletedPolicy reproduces the second half of #11226:
+// deleting the attached policy without detaching it first must not leave the
+// user locked out of their native permissions.
+func TestNativeAdminSurvivesDeletedPolicy(t *testing.T) {
+	iam, identity := newNativeFloorTestIAM(t)
+
+	require.NoError(t, iam.DeletePolicy("ListBucketsOnly"))
+
+	errCode := iam.VerifyActionPermission(putObjectRequest(), identity,
+		s3_constants.ACTION_WRITE, "mybucket", "file.txt")
+	assert.Equal(t, s3err.ErrNone, errCode,
+		"native Admin must remain effective after the attached policy is deleted")
+}
+
+// TestNativeAdminSurvivesAttachedPolicyWithoutPrincipalArn covers the IAM
+// integration path when the Admin identity has no PrincipalArn (and no session
+// token), so the auth-path switch would otherwise deny before the native floor.
+func TestNativeAdminSurvivesAttachedPolicyWithoutPrincipalArn(t *testing.T) {
+	mgr := newTestIAMManager(t)
+	iam := &IdentityAccessManagement{}
+	iam.SetIAMIntegration(NewS3IAMIntegration(mgr, ""))
+
+	doc, _ := json.Marshal(map[string]interface{}{
+		"Version": "2012-10-17",
+		"Statement": []map[string]interface{}{
+			{"Effect": "Allow", "Action": "s3:ListAllMyBuckets", "Resource": "*"},
+		},
+	})
+	require.NoError(t, iam.PutPolicy("ListBucketsOnly", string(doc)))
+
+	identity := &Identity{
+		Name:        "admin",
+		Account:     &Account{DisplayName: "admin", Id: "admin"},
+		Actions:     []Action{s3_constants.ACTION_ADMIN},
+		PolicyNames: []string{"ListBucketsOnly"},
+	}
+
+	errCode := iam.VerifyActionPermission(putObjectRequest(), identity,
+		s3_constants.ACTION_WRITE, "mybucket", "file.txt")
+	assert.Equal(t, s3err.ErrNone, errCode,
+		"native Admin must remain effective without a PrincipalArn")
+}
+
+// TestAttachedPolicyExplicitDenyOverridesNativeAdmin ensures deny-always-wins:
+// an explicit Deny in an attached policy still constrains a native admin.
+func TestAttachedPolicyExplicitDenyOverridesNativeAdmin(t *testing.T) {
+	mgr := newTestIAMManager(t)
+	iam := &IdentityAccessManagement{}
+	iam.SetIAMIntegration(NewS3IAMIntegration(mgr, ""))
+
+	doc, _ := json.Marshal(map[string]interface{}{
+		"Version": "2012-10-17",
+		"Statement": []map[string]interface{}{
+			{"Effect": "Deny", "Action": "s3:PutObject", "Resource": "arn:aws:s3:::mybucket/*"},
+		},
+	})
+	require.NoError(t, iam.PutPolicy("DenyPutMyBucket", string(doc)))
+
+	identity := &Identity{
+		Name:         "admin",
+		Account:      &Account{DisplayName: "admin", Id: "admin"},
+		Actions:      []Action{s3_constants.ACTION_ADMIN},
+		PolicyNames:  []string{"DenyPutMyBucket"},
+		PrincipalArn: "arn:aws:iam::111122223333:user/admin",
+	}
+
+	errCode := iam.VerifyActionPermission(putObjectRequest(), identity,
+		s3_constants.ACTION_WRITE, "mybucket", "file.txt")
+	assert.Equal(t, s3err.ErrAccessDenied, errCode,
+		"explicit Deny in an attached policy must override native Admin")
+}