From eea3f232da067c8eb8bde13bb06dbdb5f7da3b20 Mon Sep 17 00:00:00 2001 From: Fedor Batonogov Date: Sun, 18 Oct 2026 21:48:01 +0300 Subject: [PATCH] iam: scope CreateServiceAccount's ParentUser by policy resource (#11736) * iam: scope CreateServiceAccount's ParentUser by policy resource AuthorizeIamAction evaluated the iam:CreateServiceAccount grant against a hardcoded arn:aws:iam:::* resource, so the policy's Resource never had a say; #11218 then bound the target to the caller outright. Evaluate self-target actions against the target identity's ARN instead: a wildcard resource keeps the deliberate any-target grant, a scoped resource confines issuance to the named parent identity, anything else is denied. buildResourceARN gains the ARN pass-through buildS3ResourceArn already has, so iam: ARNs reach the policy engine intact on the attached-policies route. Closes #11735 * ci: retrigger after unrelated weed/storage metrics flake * iam: fold IAM-ARN matching into awsIAMMatch, drop debug test - remove zz_debug_test.go (local debugging leftover) - fold awsIAMMatchResource into awsIAMMatch: the arn:aws:iam:: value prefix selects case-sensitive matching, so no separate name is needed - trim comments --------- Co-authored-by: batonogov Co-authored-by: Chris Lu --- weed/iam/policy/policy_engine.go | 12 +++ weed/s3api/s3api_bucket_handlers.go | 5 + weed/s3api/s3api_embedded_iam.go | 20 ++-- weed/s3api/s3api_embedded_iam_authz_test.go | 43 ++++++-- weed/s3api/s3api_server_routing_test.go | 110 +++++++++++++------- 5 files changed, 140 insertions(+), 50 deletions(-) diff --git a/weed/iam/policy/policy_engine.go b/weed/iam/policy/policy_engine.go index 4b82a5ad0..a109d20f0 100644 --- a/weed/iam/policy/policy_engine.go +++ b/weed/iam/policy/policy_engine.go @@ -1240,6 +1240,18 @@ func awsIAMMatch(pattern, value string, evalCtx *EvaluationContext) bool { return true // Universal wildcard } + // IAM identity ARNs match case-sensitively: identity lookup is + // case-sensitive, so a case-variant names a different identity. + if strings.HasPrefix(value, "arn:aws:iam::") { + if expandedPattern == value { + return true + } + if strings.Contains(expandedPattern, "*") || strings.Contains(expandedPattern, "?") { + return wildcard.MatchesWildcard(expandedPattern, value) + } + return false + } + // Step 3: Case-insensitive exact match if strings.EqualFold(expandedPattern, value) { return true diff --git a/weed/s3api/s3api_bucket_handlers.go b/weed/s3api/s3api_bucket_handlers.go index f6b1d32dd..1a3c75641 100644 --- a/weed/s3api/s3api_bucket_handlers.go +++ b/weed/s3api/s3api_bucket_handlers.go @@ -895,6 +895,11 @@ func isPublicReadGrants(grants []*s3.Grant) bool { // buildResourceARN builds a resource ARN from bucket and object // Used by the policy engine wrapper func buildResourceARN(bucket, object string) string { + // An already-ARN resource (IAM management actions authorize against + // arn:aws:iam::...) passes through; no bucket name can contain ':'. + if strings.HasPrefix(bucket, "arn:") { + return bucket + } if object == "" || object == "/" { return fmt.Sprintf("arn:aws:s3:::%s", bucket) } diff --git a/weed/s3api/s3api_embedded_iam.go b/weed/s3api/s3api_embedded_iam.go index b032bd35b..d364176a0 100644 --- a/weed/s3api/s3api_embedded_iam.go +++ b/weed/s3api/s3api_embedded_iam.go @@ -2528,9 +2528,10 @@ func iamRequiresAdminForOthers(action string) bool { return iamSelfServiceActions[action] } -// iamSelfTargetActions require an explicit iam: grant, but a non-admin -// grant holder may only target their own identity. The target parameter is -// action-specific (UserName for most, ParentUser for CreateServiceAccount). +// iamSelfTargetActions are authorized against the target identity's ARN so a +// policy can scope the grant (arn:aws:iam:::user/team-parent). The target +// parameter is action-specific (UserName for most, ParentUser for +// CreateServiceAccount). var iamSelfTargetActions = map[string]bool{ "CreateServiceAccount": true, } @@ -2573,13 +2574,14 @@ func (iam *IdentityAccessManagement) AuthorizeIamAction(r *http.Request, identit if identity.isAdmin() { return s3err.ErrNone } - if errCode := iam.VerifyActionPermission(r, identity, Action("iam:"+action), "arn:aws:iam:::*", ""); errCode != s3err.ErrNone { - return errCode + // Self-target actions authorize against the target identity's ARN. The + // account segment stays empty so existing arn:aws:iam:::* wildcard + // grants keep matching. + resource := "arn:aws:iam:::*" + if iamRequiresSelfTarget(action) && targetUserName != "" { + resource = fmt.Sprintf("arn:aws:iam:::user/%s", targetUserName) } - if iamRequiresSelfTarget(action) && targetUserName != "" && targetUserName != identity.Name { - return s3err.ErrAccessDenied - } - return s3err.ErrNone + return iam.VerifyActionPermission(r, identity, Action("iam:"+action), resource, "") } // AuthIamManagement authenticates an IAM management request and authorizes the diff --git a/weed/s3api/s3api_embedded_iam_authz_test.go b/weed/s3api/s3api_embedded_iam_authz_test.go index 911d9feb5..7f4725fab 100644 --- a/weed/s3api/s3api_embedded_iam_authz_test.go +++ b/weed/s3api/s3api_embedded_iam_authz_test.go @@ -15,6 +15,11 @@ const ( dataPlanePolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:*","Resource":"*"}]}` iamAdminPolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"iam:*","Resource":"*"}]}` iamCreateSvcAcctPolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["iam:CreateServiceAccount"],"Resource":["*"]}]}` + // The wildcard ARN shape this authorization always used; it must keep + // matching any parent so existing grants survive. + iamCreateSvcAcctWildcardArnPolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["iam:CreateServiceAccount"],"Resource":["arn:aws:iam:::*"]}]}` + // Scoped to one parent identity. + iamCreateSvcAcctZonePolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["iam:CreateServiceAccount"],"Resource":["arn:aws:iam:::user/team-parent"]}]}` ) func newIamAuthzTestIam(t *testing.T) *IdentityAccessManagement { @@ -95,22 +100,48 @@ func TestAuthorizeIamActionDeniesAnonymous(t *testing.T) { iam.AuthorizeIamAction(iamPostRequest(""), anonymous, "CreateUser", "victim")) } -// CreateServiceAccount takes its target from the ParentUser parameter, not -// UserName. A non-admin holding only iam:CreateServiceAccount must not be able -// to mint a service account for another identity (privilege escalation to that -// identity's permissions). -func TestAuthorizeIamActionCreateServiceAccountBindsTargetToCaller(t *testing.T) { +// CreateServiceAccount takes its target from the ParentUser parameter. A +// non-admin holding iam:CreateServiceAccount targets the identities its +// policy resource covers: a wildcard resource allows any target, a scoped +// resource confines the grant to the named parent identity. +func TestAuthorizeIamActionCreateServiceAccountScopedByPolicyResource(t *testing.T) { iam := newIamAuthzTestIam(t) require.NoError(t, iam.PutPolicy("CreateSvcAcctPolicy", iamCreateSvcAcctPolicy)) + require.NoError(t, iam.PutPolicy("CreateSvcAcctWildcardArnPolicy", iamCreateSvcAcctWildcardArnPolicy)) + require.NoError(t, iam.PutPolicy("CreateSvcAcctZonePolicy", iamCreateSvcAcctZonePolicy)) dev := &Identity{Name: "dev", PolicyNames: []string{"CreateSvcAcctPolicy"}} + wildcardArn := &Identity{Name: "wildcard-arn", PolicyNames: []string{"CreateSvcAcctWildcardArnPolicy"}} + keymaker := &Identity{Name: "keymaker", PolicyNames: []string{"CreateSvcAcctZonePolicy"}} admin := &Identity{Name: "admin", Actions: []Action{s3_constants.ACTION_ADMIN}} + // Wildcard resource: any target, including the caller itself. assert.Equal(t, s3err.ErrNone, iam.AuthorizeIamAction(iamPostRequest(""), dev, "CreateServiceAccount", "dev")) assert.Equal(t, s3err.ErrNone, iam.AuthorizeIamAction(iamPostRequest(""), dev, "CreateServiceAccount", "")) - assert.Equal(t, s3err.ErrAccessDenied, + assert.Equal(t, s3err.ErrNone, iam.AuthorizeIamAction(iamPostRequest(""), dev, "CreateServiceAccount", "admin")) + + // Wildcard ARN: any target. + assert.Equal(t, s3err.ErrNone, + iam.AuthorizeIamAction(iamPostRequest(""), wildcardArn, "CreateServiceAccount", "team-parent")) + assert.Equal(t, s3err.ErrNone, + iam.AuthorizeIamAction(iamPostRequest(""), wildcardArn, "CreateServiceAccount", "admin")) + + // Scoped resource: the named parent only. IAM resource matching is + // case-sensitive, so a case variant is a different identity. + assert.Equal(t, s3err.ErrNone, + iam.AuthorizeIamAction(iamPostRequest(""), keymaker, "CreateServiceAccount", "team-parent")) + assert.Equal(t, s3err.ErrAccessDenied, + iam.AuthorizeIamAction(iamPostRequest(""), keymaker, "CreateServiceAccount", "Team-Parent")) + assert.Equal(t, s3err.ErrAccessDenied, + iam.AuthorizeIamAction(iamPostRequest(""), keymaker, "CreateServiceAccount", "admin")) + assert.Equal(t, s3err.ErrAccessDenied, + iam.AuthorizeIamAction(iamPostRequest(""), keymaker, "CreateServiceAccount", "keymaker")) + assert.Equal(t, s3err.ErrAccessDenied, + iam.AuthorizeIamAction(iamPostRequest(""), keymaker, "CreateServiceAccount", "")) + + // Admins remain unrestricted. assert.Equal(t, s3err.ErrNone, iam.AuthorizeIamAction(iamPostRequest(""), admin, "CreateServiceAccount", "victim")) } diff --git a/weed/s3api/s3api_server_routing_test.go b/weed/s3api/s3api_server_routing_test.go index bce44e2b9..9459fe588 100644 --- a/weed/s3api/s3api_server_routing_test.go +++ b/weed/s3api/s3api_server_routing_test.go @@ -1,6 +1,7 @@ package s3api import ( + "context" "net/http" "net/http/httptest" "net/url" @@ -12,9 +13,14 @@ import ( v4 "github.com/aws/aws-sdk-go/aws/signer/v4" "github.com/gorilla/mux" "github.com/seaweedfs/seaweedfs/weed/credential" + "github.com/seaweedfs/seaweedfs/weed/iam/integration" + "github.com/seaweedfs/seaweedfs/weed/iam/policy" + "github.com/seaweedfs/seaweedfs/weed/iam/sts" + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/seaweedfs/seaweedfs/weed/util" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // routingTestAccessKey/routingTestSecretKey are the credentials seeded into @@ -205,50 +211,73 @@ func TestRouting_AuthenticatedIAM(t *testing.T) { assert.Contains(t, []int{http.StatusBadRequest, http.StatusForbidden}, rr.Code, "Should route to IAM handler (400/403 due to invalid signature)") } -// setupRoutingTestServerWithCreator seeds a non-admin identity (creator) that -// holds only the iam:CreateServiceAccount action, plus a victim identity, so a -// SigV4-signed CreateServiceAccount request can exercise UnifiedPostHandler's -// authorization against the ParentUser target. +// setupRoutingTestServerWithCreator seeds two non-admin identities holding +// iam:CreateServiceAccount — creator with the coarse (unscoped) action, +// keymaker with a policy scoped to the team-parent identity ARN — plus a +// victim identity, so SigV4-signed CreateServiceAccount requests can exercise +// UnifiedPostHandler's authorization against the ParentUser target. func setupRoutingTestServerWithCreator(t *testing.T) *S3ApiServer { s3a := setupRoutingTestServer(t) const creatorAK, creatorSK = "creator-ak", "creator-sk" - creator := &Identity{ - Name: "creator", - Actions: []Action{Action("iam:CreateServiceAccount")}, - IsStatic: true, - Credentials: []*Credential{{ - AccessKey: creatorAK, - SecretKey: creatorSK, - }}, + const keymakerAK, keymakerSK = "keymaker-ak", "keymaker-sk" + + // Policy-bearing identities route through the IAM integration. + mgr := integration.NewIAMManager() + require.NoError(t, mgr.Initialize(&integration.IAMConfig{ + STS: &sts.STSConfig{ + TokenDuration: sts.FlexibleDuration{Duration: time.Hour}, + MaxSessionLength: sts.FlexibleDuration{Duration: 12 * time.Hour}, + Issuer: "test", + SigningKey: []byte("test-signing-key-32-characters!!"), + AccountId: "111122223333", + }, + Policy: &policy.PolicyEngineConfig{DefaultEffect: "Deny", StoreType: "memory"}, + Roles: &integration.RoleStoreConfig{StoreType: "memory"}, + }, func() string { return "localhost:8888" })) + s3a.iam.SetIAMIntegration(NewS3IAMIntegration(mgr, "")) + + const zonePolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["iam:CreateServiceAccount"],"Resource":["arn:aws:iam:::user/team-parent"]}]}` + require.NoError(t, s3a.iam.PutPolicy("CreateSvcAcctZonePolicy", zonePolicy)) + // Store and memory must agree: SigV4 auth reads memory, the handler + // re-reads the store, and a successful creation reloads memory from it. + idents := []*iam_pb.Identity{ + {Name: "creator", Actions: []string{"iam:CreateServiceAccount"}, + Credentials: []*iam_pb.Credential{{AccessKey: creatorAK, SecretKey: creatorSK}}}, + {Name: "keymaker", PolicyNames: []string{"CreateSvcAcctZonePolicy"}, + Credentials: []*iam_pb.Credential{{AccessKey: keymakerAK, SecretKey: keymakerSK}}}, + {Name: "victim"}, + {Name: "team-parent"}, + } + require.NoError(t, s3a.credentialManager.SaveConfiguration(context.Background(), &iam_pb.S3ApiConfiguration{ + Identities: idents, + Policies: []*iam_pb.Policy{{Name: "CreateSvcAcctZonePolicy", Content: zonePolicy}}, + })) + for _, ident := range idents { + require.NoError(t, s3a.iam.UpsertIdentity(ident)) } - victim := &Identity{Name: "victim", IsStatic: true} - s3a.iam.m.Lock() - s3a.iam.identities = append(s3a.iam.identities, creator, victim) - s3a.iam.accessKeyIdent[creatorAK] = creator - s3a.iam.nameToIdentity["creator"] = creator - s3a.iam.nameToIdentity["victim"] = victim - s3a.iam.m.Unlock() s3a.cb = NewCircuitBreaker(s3a.option) return s3a } -// TestRouting_CreateServiceAccountBindsParentUser verifies that on the S3-port -// IAM route a non-admin holding iam:CreateServiceAccount cannot mint a service -// account for another identity (ParentUser=victim), and can for itself. -func TestRouting_CreateServiceAccountBindsParentUser(t *testing.T) { +// TestRouting_CreateServiceAccountParentUserScopedByPolicy verifies that on +// the S3-port IAM route a non-admin holding iam:CreateServiceAccount targets +// the identities its grant covers: the coarse action is unscoped (like a +// wildcard resource), a policy scoped to one parent identity's ARN confines +// the grant to that parent. +func TestRouting_CreateServiceAccountParentUserScopedByPolicy(t *testing.T) { router := mux.NewRouter() s3a := setupRoutingTestServerWithCreator(t) s3a.registerRouter(router) - signCreator := func(t *testing.T, req *http.Request, body string) { + sign := func(t *testing.T, req *http.Request, body, ak, sk string) { t.Helper() - creds := credentials.NewStaticCredentials("creator-ak", "creator-sk", "") + creds := credentials.NewStaticCredentials(ak, sk, "") if _, err := v4.NewSigner(creds).Sign(req, strings.NewReader(body), "iam", "us-east-1", time.Now()); err != nil { t.Fatalf("sign request: %v", err) } } - makeReq := func(parentUser string) *http.Request { + makeReq := func(parentUser, ak, sk string) *http.Request { data := url.Values{} data.Set("Action", "CreateServiceAccount") data.Set("Version", "2010-05-08") @@ -256,19 +285,30 @@ func TestRouting_CreateServiceAccountBindsParentUser(t *testing.T) { body := data.Encode() req, _ := http.NewRequest("POST", "http://localhost/", strings.NewReader(body)) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - signCreator(t, req, body) + sign(t, req, body, ak, sk) return req } - // Targeting another identity must be denied at authorization. - rr := httptest.NewRecorder() - router.ServeHTTP(rr, makeReq("victim")) - assert.Equal(t, http.StatusForbidden, rr.Code, "cross-identity ParentUser must be denied; got body=%s", rr.Body.String()) + serve := func(req *http.Request) *httptest.ResponseRecorder { + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + return rr + } - // Targeting self must pass authorization (handler may still error, but not 403). - rr2 := httptest.NewRecorder() - router.ServeHTTP(rr2, makeReq("creator")) - assert.NotEqual(t, http.StatusForbidden, rr2.Code, "self ParentUser must pass authorization; got body=%s", rr2.Body.String()) + // Coarse (unscoped) action: any target passes authorization and the + // service account is actually created. + rr := serve(makeReq("victim", "creator-ak", "creator-sk")) + assert.Equal(t, http.StatusOK, rr.Code, "coarse grant is unscoped; got body=%s", rr.Body.String()) + + // Scoped policy: the named parent passes (200), any other identity is denied. + rr2 := serve(makeReq("team-parent", "keymaker-ak", "keymaker-sk")) + assert.Equal(t, http.StatusOK, rr2.Code, "scoped grant must cover its named parent; got body=%s", rr2.Body.String()) + + rr3 := serve(makeReq("victim", "keymaker-ak", "keymaker-sk")) + assert.Equal(t, http.StatusForbidden, rr3.Code, "target outside the scoped grant must be denied; got body=%s", rr3.Body.String()) + + rr4 := serve(makeReq("keymaker", "keymaker-ak", "keymaker-sk")) + assert.Equal(t, http.StatusForbidden, rr4.Code, "even self is denied when the scoped grant does not name it; got body=%s", rr4.Body.String()) } // TestRouting_IAMMatcherLogic verifies the iamMatcher correctly distinguishes auth types