From 66d9b89cd2bb2b9411e436f26e4fd59b2fddbd62 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 4 May 2026 17:56:10 -0700 Subject: [PATCH] fix(iam): deny IAM users with no policies instead of granting full access (#9317) * fix(iam): deny IAM users with zero policies instead of falling through to DefaultEffect=Allow A user created via the S3 IAM API with no policies attached was inheriting full S3 access. With `weed s3 -iam` and no explicit IAM config, the policy engine's DefaultEffect defaults to Allow for the in-memory zero-config path. The "no matching statement" guard in IsActionAllowed only triggered when the user already had at least one policy, so a fresh user with PolicyNames=[] slipped through and got allow-all. Track hasManagedSubject whenever the principal resolves to a registered user or role (or PolicyNames are supplied directly) and deny on no-match. The DefaultEffect=Allow fallback now only applies to truly unmanaged callers. * test(iam): cover non-matching attached-policy case for managed-subject deny * test(iam): cover role-with-empty-AttachedPolicies deny path Sibling case to TestIsActionAllowed_RegisteredUserWithoutPoliciesIsDenied: a managed role that resolves but has zero AttachedPolicies must also fall through to deny under DefaultEffect=Allow, not inherit full access. The fix already handles this branch via the hasManagedSubject flag; this test pins the regression class so we don't lose coverage on either side of the user/role split. Addresses coderabbit nitpick on PR #9317. --- weed/iam/integration/iam_manager.go | 22 ++- .../integration/iam_manager_no_policy_test.go | 145 ++++++++++++++++++ 2 files changed, 160 insertions(+), 7 deletions(-) create mode 100644 weed/iam/integration/iam_manager_no_policy_test.go diff --git a/weed/iam/integration/iam_manager.go b/weed/iam/integration/iam_manager.go index bfff9cefd..e3af56328 100644 --- a/weed/iam/integration/iam_manager.go +++ b/weed/iam/integration/iam_manager.go @@ -450,13 +450,20 @@ func (m *IAMManager) IsActionAllowed(ctx context.Context, request *ActionRequest var baseResult *policy.EvaluationResult var err error - subjectPolicyCount := 0 + // hasManagedSubject is true once we've resolved the principal to a registered + // IAM user or role (or the caller has supplied PolicyNames directly). For a + // managed subject, "no matching statement" must deny — the DefaultEffect=Allow + // fallback is only meant for the unmanaged zero-config startup case. + hasManagedSubject := false if isAdmin { // Admin always has base access allowed baseResult = &policy.EvaluationResult{Effect: policy.EffectAllow} } else { policies := request.PolicyNames + if len(policies) > 0 { + hasManagedSubject = true + } if len(policies) == 0 { // Extract role name from principal ARN roleName := utils.ExtractRoleNameFromPrincipal(request.Principal) @@ -472,6 +479,7 @@ func (m *IAMManager) IsActionAllowed(ctx context.Context, request *ActionRequest if err != nil || user == nil { return false, fmt.Errorf("user not found for principal: %s (user=%s)", request.Principal, userName) } + hasManagedSubject = true policies = user.GetPolicyNames() } else { // Get role definition @@ -480,11 +488,10 @@ func (m *IAMManager) IsActionAllowed(ctx context.Context, request *ActionRequest return false, fmt.Errorf("role not found: %s", roleName) } + hasManagedSubject = true policies = roleDef.AttachedPolicies } } - subjectPolicyCount = len(policies) - if bucketPolicyName != "" { // Enforce an upper bound on the number of policies to avoid excessive allocations if len(policies) >= maxPoliciesForEvaluation { @@ -508,10 +515,11 @@ func (m *IAMManager) IsActionAllowed(ctx context.Context, request *ActionRequest } // Zero-config IAM uses DefaultEffect=Allow to preserve open-by-default behavior - // for requests without any subject policies. Once a user or role has attached - // policies, "no matching statement" must fall back to deny so the attachment - // actually scopes access. - if subjectPolicyCount > 0 && len(baseResult.MatchingStatements) == 0 { + // for requests without any subject policies. Once we resolve the principal to + // a registered IAM user or role (or the caller hands us policy names), + // "no matching statement" must fall back to deny — otherwise a freshly + // created user with zero policies would inherit full access. + if hasManagedSubject && len(baseResult.MatchingStatements) == 0 { return false, nil } diff --git a/weed/iam/integration/iam_manager_no_policy_test.go b/weed/iam/integration/iam_manager_no_policy_test.go new file mode 100644 index 000000000..500f0c43a --- /dev/null +++ b/weed/iam/integration/iam_manager_no_policy_test.go @@ -0,0 +1,145 @@ +package integration + +import ( + "context" + "testing" + "time" + + "github.com/seaweedfs/seaweedfs/weed/iam/policy" + "github.com/seaweedfs/seaweedfs/weed/iam/sts" + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type fakeUserStore struct { + users map[string]*iam_pb.Identity +} + +func (f *fakeUserStore) GetUser(_ context.Context, name string) (*iam_pb.Identity, error) { + if u, ok := f.users[name]; ok { + return u, nil + } + return nil, nil +} + +// TestIsActionAllowed_RegisteredUserWithoutPoliciesIsDenied is a regression test +// for a security issue where a user created via the S3 IAM API with no policies +// attached was granted access to every S3 action. The default-allow path was +// intended for unmanaged zero-config startup, not for explicitly registered IAM +// users with an empty policy set. +func TestIsActionAllowed_RegisteredUserWithoutPoliciesIsDenied(t *testing.T) { + manager := NewIAMManager() + require.NoError(t, manager.Initialize(&IAMConfig{ + STS: &sts.STSConfig{ + TokenDuration: sts.FlexibleDuration{Duration: time.Hour}, + MaxSessionLength: sts.FlexibleDuration{Duration: 12 * time.Hour}, + Issuer: "test-sts", + SigningKey: []byte("test-signing-key-32-characters-long"), + }, + Policy: &policy.PolicyEngineConfig{ + // Reproduce the zero-config setup: with no IAM config file, the + // loader picks DefaultEffect=Allow for in-memory mode. + DefaultEffect: string(policy.EffectAllow), + StoreType: sts.StoreTypeMemory, + }, + Roles: &RoleStoreConfig{StoreType: sts.StoreTypeMemory}, + }, func() string { return "localhost:8888" })) + + manager.SetUserStore(&fakeUserStore{users: map[string]*iam_pb.Identity{ + "newuser": {Name: "newuser"}, // no PolicyNames, no Actions + }}) + + allowed, err := manager.IsActionAllowed(context.Background(), &ActionRequest{ + Principal: "arn:aws:iam::seaweedfs:user/newuser", + Action: "s3:DeleteBucket", + Resource: "arn:aws:s3:::any-bucket", + }) + require.NoError(t, err) + assert.False(t, allowed, "registered user with no policies must not get default-allow access") +} + +// TestIsActionAllowed_RegisteredRoleWithoutPoliciesIsDenied is the role-side +// counterpart of the registered-user regression: a managed role that exists +// but has zero AttachedPolicies must not inherit full access from the +// zero-config DefaultEffect=Allow fallback. Coderabbit flagged this as an +// uncovered branch of the same fix on PR #9317. +func TestIsActionAllowed_RegisteredRoleWithoutPoliciesIsDenied(t *testing.T) { + manager := NewIAMManager() + require.NoError(t, manager.Initialize(&IAMConfig{ + STS: &sts.STSConfig{ + TokenDuration: sts.FlexibleDuration{Duration: time.Hour}, + MaxSessionLength: sts.FlexibleDuration{Duration: 12 * time.Hour}, + Issuer: "test-sts", + SigningKey: []byte("test-signing-key-32-characters-long"), + }, + Policy: &policy.PolicyEngineConfig{ + DefaultEffect: string(policy.EffectAllow), + StoreType: sts.StoreTypeMemory, + }, + Roles: &RoleStoreConfig{StoreType: sts.StoreTypeMemory}, + }, func() string { return "localhost:8888" })) + + ctx := context.Background() + require.NoError(t, manager.CreateRole(ctx, "", "EmptyRole", &RoleDefinition{ + RoleName: "EmptyRole", + RoleArn: "arn:aws:iam::seaweedfs:role/EmptyRole", + AttachedPolicies: nil, // explicit: managed role with no policies attached + })) + + allowed, err := manager.IsActionAllowed(ctx, &ActionRequest{ + // Use the assumed-role principal form the STS path produces. The + // IAM manager extracts role name via ExtractRoleNameFromPrincipal, + // finds the role definition, and must deny because AttachedPolicies + // is empty even though DefaultEffect is Allow. + Principal: "arn:aws:sts::seaweedfs:assumed-role/EmptyRole/some-session", + Action: "s3:DeleteBucket", + Resource: "arn:aws:s3:::any-bucket", + }) + require.NoError(t, err) + assert.False(t, allowed, "registered role with no attached policies must not get default-allow access") +} + +// TestIsActionAllowed_RegisteredUserWithNonMatchingPolicyIsDenied covers the +// adjacent case: the user has policies attached, but none of the policy +// statements match the requested action/resource. The DefaultEffect=Allow +// fallback must not silently grant access when an attached policy simply +// does not say anything about this action. +func TestIsActionAllowed_RegisteredUserWithNonMatchingPolicyIsDenied(t *testing.T) { + manager := NewIAMManager() + require.NoError(t, manager.Initialize(&IAMConfig{ + STS: &sts.STSConfig{ + TokenDuration: sts.FlexibleDuration{Duration: time.Hour}, + MaxSessionLength: sts.FlexibleDuration{Duration: 12 * time.Hour}, + Issuer: "test-sts", + SigningKey: []byte("test-signing-key-32-characters-long"), + }, + Policy: &policy.PolicyEngineConfig{ + DefaultEffect: string(policy.EffectAllow), + StoreType: sts.StoreTypeMemory, + }, + Roles: &RoleStoreConfig{StoreType: sts.StoreTypeMemory}, + }, func() string { return "localhost:8888" })) + + ctx := context.Background() + require.NoError(t, manager.CreatePolicy(ctx, "", "ReadOnlyOneBucket", &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{{ + Effect: "Allow", + Action: []string{"s3:GetObject"}, + Resource: []string{"arn:aws:s3:::reports/*"}, + }}, + })) + + manager.SetUserStore(&fakeUserStore{users: map[string]*iam_pb.Identity{ + "alice": {Name: "alice", PolicyNames: []string{"ReadOnlyOneBucket"}}, + }}) + + allowed, err := manager.IsActionAllowed(ctx, &ActionRequest{ + Principal: "arn:aws:iam::seaweedfs:user/alice", + Action: "s3:DeleteBucket", + Resource: "arn:aws:s3:::other-bucket", + }) + require.NoError(t, err) + assert.False(t, allowed, "action outside the user's attached policy must be denied") +}