diff --git a/weed/s3api/s3api_embedded_iam.go b/weed/s3api/s3api_embedded_iam.go index 4dfab6074..80dab1e87 100644 --- a/weed/s3api/s3api_embedded_iam.go +++ b/weed/s3api/s3api_embedded_iam.go @@ -2474,8 +2474,31 @@ 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). +var iamSelfTargetActions = map[string]bool{ + "CreateServiceAccount": true, +} + +func iamRequiresSelfTarget(action string) bool { + return iamSelfTargetActions[action] +} + +// iamTargetUserName returns the request's target identity for authorization. +// Most IAM actions target UserName; CreateServiceAccount targets ParentUser. +// Both IAM dispatch surfaces (AuthIamManagement and UnifiedPostHandler) use +// this so the authorized target and the acted-on target cannot differ. +func iamTargetUserName(action string, r *http.Request) string { + if action == "CreateServiceAccount" { + return r.PostForm.Get("ParentUser") + } + return r.PostForm.Get("UserName") +} + // AuthorizeIamAction authorizes an IAM management action for identity, with -// targetUserName taken from the request's UserName parameter. +// targetUserName taken from the request's target parameter (UserName, or +// ParentUser for CreateServiceAccount). // // IAM management is not part of the S3 data plane, so the grant is checked as // iam:. A coarse S3 action would instead be matched by an ordinary @@ -2496,7 +2519,13 @@ func (iam *IdentityAccessManagement) AuthorizeIamAction(r *http.Request, identit if identity.isAdmin() { return s3err.ErrNone } - return iam.VerifyActionPermission(r, identity, Action("iam:"+action), "arn:aws:iam:::*", "") + if errCode := iam.VerifyActionPermission(r, identity, Action("iam:"+action), "arn:aws:iam:::*", ""); errCode != s3err.ErrNone { + return errCode + } + if iamRequiresSelfTarget(action) && targetUserName != "" && targetUserName != identity.Name { + return s3err.ErrAccessDenied + } + return s3err.ErrNone } // AuthIamManagement authenticates an IAM management request and authorizes the @@ -2530,7 +2559,8 @@ func (iam *IdentityAccessManagement) AuthIamManagement(f http.HandlerFunc) http. // UserName comes from the body only, the same place the handlers read it // from, so the authorized target and the acted-on target cannot differ. - if errCode := iam.AuthorizeIamAction(r, identity, r.Form.Get("Action"), r.PostForm.Get("UserName")); errCode != s3err.ErrNone { + action := r.Form.Get("Action") + if errCode := iam.AuthorizeIamAction(r, identity, action, iamTargetUserName(action, r)); errCode != s3err.ErrNone { s3err.WriteErrorResponse(w, r, errCode) return } diff --git a/weed/s3api/s3api_embedded_iam_authz_test.go b/weed/s3api/s3api_embedded_iam_authz_test.go index d39ab19b3..911d9feb5 100644 --- a/weed/s3api/s3api_embedded_iam_authz_test.go +++ b/weed/s3api/s3api_embedded_iam_authz_test.go @@ -12,8 +12,9 @@ import ( ) const ( - dataPlanePolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:*","Resource":"*"}]}` - iamAdminPolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"iam:*","Resource":"*"}]}` + 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":["*"]}]}` ) func newIamAuthzTestIam(t *testing.T) *IdentityAccessManagement { @@ -93,3 +94,23 @@ func TestAuthorizeIamActionDeniesAnonymous(t *testing.T) { assert.Equal(t, s3err.ErrAccessDenied, 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) { + iam := newIamAuthzTestIam(t) + require.NoError(t, iam.PutPolicy("CreateSvcAcctPolicy", iamCreateSvcAcctPolicy)) + dev := &Identity{Name: "dev", PolicyNames: []string{"CreateSvcAcctPolicy"}} + admin := &Identity{Name: "admin", Actions: []Action{s3_constants.ACTION_ADMIN}} + + 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, + iam.AuthorizeIamAction(iamPostRequest(""), dev, "CreateServiceAccount", "admin")) + assert.Equal(t, s3err.ErrNone, + iam.AuthorizeIamAction(iamPostRequest(""), admin, "CreateServiceAccount", "victim")) +} diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 34780547c..027f3068b 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -742,7 +742,7 @@ func (s3a *S3ApiServer) UnifiedPostHandler(w http.ResponseWriter, r *http.Reques // UserName comes from the body only, the same place DoActions reads it // from, so the authorized target and the acted-on target cannot differ. - if s3a.iam.AuthorizeIamAction(r, identity, action, r.PostForm.Get("UserName")) != s3err.ErrNone { + if s3a.iam.AuthorizeIamAction(r, identity, action, iamTargetUserName(action, r)) != s3err.ErrNone { s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) return } diff --git a/weed/s3api/s3api_server_routing_test.go b/weed/s3api/s3api_server_routing_test.go index 6194abf60..bce44e2b9 100644 --- a/weed/s3api/s3api_server_routing_test.go +++ b/weed/s3api/s3api_server_routing_test.go @@ -205,6 +205,72 @@ 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. +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, + }}, + } + 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) { + router := mux.NewRouter() + s3a := setupRoutingTestServerWithCreator(t) + s3a.registerRouter(router) + + signCreator := func(t *testing.T, req *http.Request, body string) { + t.Helper() + creds := credentials.NewStaticCredentials("creator-ak", "creator-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 { + data := url.Values{} + data.Set("Action", "CreateServiceAccount") + data.Set("Version", "2010-05-08") + data.Set("ParentUser", parentUser) + 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) + 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()) + + // 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()) +} + // TestRouting_IAMMatcherLogic verifies the iamMatcher correctly distinguishes auth types func TestRouting_IAMMatcherLogic(t *testing.T) { tests := []struct {