diff --git a/weed/iamapi/iamapi_authz_test.go b/weed/iamapi/iamapi_authz_test.go new file mode 100644 index 000000000..5684a163b --- /dev/null +++ b/weed/iamapi/iamapi_authz_test.go @@ -0,0 +1,143 @@ +package iamapi + +import ( + "context" + "crypto/sha256" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" + "github.com/gorilla/mux" + "github.com/seaweedfs/seaweedfs/weed/credential" + "github.com/seaweedfs/seaweedfs/weed/credential/memory" + "github.com/seaweedfs/seaweedfs/weed/filer" + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A data-plane identity holding nothing but the equivalent of AmazonS3FullAccess, +// and an admin, so the same request can be run from both sides. The admin key is +// obviously fake to keep secret scanners quiet. +const authzConfigJSON = `{ + "identities": [ + { + "name": "power_user", + "credentials": [{"accessKey": "AKIAIOSFODNN7EXAMPLE", "secretKey": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"}], + "policyNames": ["PowerUserPolicy"] + }, + { + "name": "iam_admin", + "credentials": [{"accessKey": "AKIATESTFAKEADMIN0001", "secretKey": "testAdminSecretFake0000000000000000000000"}], + "actions": ["Admin"] + } + ], + "policies": [ + { + "name": "PowerUserPolicy", + "content": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Action\":\"s3:*\",\"Resource\":\"*\"}]}" + } + ] +}` + +func newAuthzTestServer(t *testing.T) *httptest.Server { + t.Helper() + + config := &iam_pb.S3ApiConfiguration{} + require.NoError(t, filer.ParseS3ConfigurationFromBytes([]byte(authzConfigJSON), config)) + + store := &memory.MemoryStore{} + require.NoError(t, store.Initialize(nil, "")) + cm := &credential.CredentialManager{Store: store} + require.NoError(t, cm.SaveConfiguration(context.Background(), config)) + + iam := &s3api.IdentityAccessManagement{} + iam.SetCredentialManagerForTest(cm) + require.NoError(t, iam.LoadS3ApiConfigurationFromBytes([]byte(authzConfigJSON))) + + iama := &IamApiServer{ + iam: iam, + s3ApiConfig: &countingConfigSaver{cm: cm}, + } + router := mux.NewRouter().SkipClean(true) + iama.registerRouter(router) + + server := httptest.NewServer(router) + t.Cleanup(server.Close) + return server +} + +func postSignedIamAction(t *testing.T, serverURL, accessKey, secretKey, rawQuery, body string) *http.Response { + t.Helper() + + url := serverURL + "/" + if rawQuery != "" { + url += "?" + rawQuery + } + req, err := http.NewRequest(http.MethodPost, url, strings.NewReader(body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + payloadHash := fmt.Sprintf("%x", sha256.Sum256([]byte(body))) + require.NoError(t, v4.NewSigner().SignHTTP(context.Background(), + aws.Credentials{AccessKeyID: accessKey, SecretAccessKey: secretKey}, + req, payloadHash, "iam", "us-east-1", time.Now())) + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + t.Cleanup(func() { resp.Body.Close() }) + return resp +} + +// An S3 data-plane policy grants no IAM management, however broad it is: the +// standalone server used to check these actions as a coarse S3 action, which +// such a policy matches. +func TestIamManagementDeniedForDataPlanePolicy(t *testing.T) { + server := newAuthzTestServer(t) + + for _, action := range []string{"CreateUser", "CreateAccessKey", "PutUserPolicy", "AttachUserPolicy"} { + t.Run(action, func(t *testing.T) { + resp := postSignedIamAction(t, server.URL, "AKIAIOSFODNN7EXAMPLE", "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", + "", "Action="+action+"&UserName=victim&Version=2010-05-08") + assert.Equal(t, http.StatusForbidden, resp.StatusCode) + }) + } +} + +// Same expectation when the request carries an S3 query parameter, which the +// action resolver used to read even for an IAM action. +func TestIamManagementDeniedWithS3QueryParameter(t *testing.T) { + server := newAuthzTestServer(t) + + for _, rawQuery := range []string{"delete", "acl", "tagging", "policy"} { + t.Run(rawQuery, func(t *testing.T) { + resp := postSignedIamAction(t, server.URL, "AKIAIOSFODNN7EXAMPLE", "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", + rawQuery, "Action=CreateUser&UserName=victim&Version=2010-05-08") + assert.Equal(t, http.StatusForbidden, resp.StatusCode) + }) + } +} + +// Self-service still works without an IAM grant, matching AWS and the embedded +// IAM surface: a user may rotate its own access keys. +func TestIamSelfServiceAllowedForDataPlanePolicy(t *testing.T) { + server := newAuthzTestServer(t) + + resp := postSignedIamAction(t, server.URL, "AKIAIOSFODNN7EXAMPLE", "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", + "", "Action=ListAccessKeys&Version=2010-05-08") + assert.Equal(t, http.StatusOK, resp.StatusCode) +} + +func TestIamManagementAllowedForAdmin(t *testing.T) { + server := newAuthzTestServer(t) + + resp := postSignedIamAction(t, server.URL, "AKIATESTFAKEADMIN0001", "testAdminSecretFake0000000000000000000000", + "", "Action=CreateUser&UserName=new-user&Version=2010-05-08") + assert.Equal(t, http.StatusOK, resp.StatusCode) +} diff --git a/weed/iamapi/iamapi_server.go b/weed/iamapi/iamapi_server.go index 26218587f..26b2d9c60 100644 --- a/weed/iamapi/iamapi_server.go +++ b/weed/iamapi/iamapi_server.go @@ -19,7 +19,6 @@ import ( "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" "github.com/seaweedfs/seaweedfs/weed/s3api" "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" - . "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" "github.com/seaweedfs/seaweedfs/weed/util" util_http "github.com/seaweedfs/seaweedfs/weed/util/http" @@ -124,10 +123,7 @@ func (iama *IamApiServer) registerRouter(router *mux.Router) { // API Router apiRouter := router.PathPrefix("/").Subrouter() apiRouter.Use(request_id.Middleware) - // ListBuckets - - // apiRouter.Methods("GET").Path("/").HandlerFunc(track(s3a.iam.Auth(s3a.ListBucketsHandler, ACTION_ADMIN), "LIST")) - apiRouter.Methods(http.MethodPost).Path("/").HandlerFunc(iama.iam.Auth(iama.DoActions, ACTION_ADMIN)) + apiRouter.Methods(http.MethodPost).Path("/").HandlerFunc(iama.iam.AuthIamManagement(iama.DoActions)) // Health probes apiRouter.Methods(http.MethodGet, http.MethodHead).Path("/healthz").HandlerFunc(iama.healthzHandler) diff --git a/weed/s3api/s3_action_resolver.go b/weed/s3api/s3_action_resolver.go index b67af7d25..4f25ef537 100644 --- a/weed/s3api/s3_action_resolver.go +++ b/weed/s3api/s3_action_resolver.go @@ -26,6 +26,13 @@ import ( // - Falls back to base action mapping if no specific resolution is possible // - Always returns a valid S3 action string (never empty) func ResolveS3Action(r *http.Request, baseAction string, bucket string, object string) string { + // An action naming another service is already resolved, and an S3 request + // shape says nothing about it: a query parameter on an IAM or STS request + // must not turn it into the S3 action that parameter stands for. + if strings.HasPrefix(baseAction, "iam:") || strings.HasPrefix(baseAction, "sts:") { + return baseAction + } + if r == nil || r.URL == nil { // No HTTP context available: fall back to coarse-grained mapping // This ensures consistent behavior and avoids returning empty strings diff --git a/weed/s3api/s3_action_resolver_test.go b/weed/s3api/s3_action_resolver_test.go index 9e11f1dcb..2b5709d00 100644 --- a/weed/s3api/s3_action_resolver_test.go +++ b/weed/s3api/s3_action_resolver_test.go @@ -113,3 +113,28 @@ func TestResolveS3Action_AttributesBeforeVersionId(t *testing.T) { }) } } + +// A base action naming another service carries no S3 request shape, so a query +// parameter on the request must not redirect it to an S3 action. +func TestResolveS3ActionKeepsNonS3Service(t *testing.T) { + tests := []struct { + name string + method string + url string + baseAction string + }{ + {"iam action with batch delete query", http.MethodPost, "http://localhost/?delete", "iam:CreateUser"}, + {"iam action with acl query", http.MethodPut, "http://localhost/?acl", "iam:AttachUserPolicy"}, + {"iam action with tagging query", http.MethodGet, "http://localhost/?tagging", "iam:ListUsers"}, + {"sts action with batch delete query", http.MethodPost, "http://localhost/?delete", "sts:AssumeRole"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, _ := http.NewRequest(tt.method, tt.url, nil) + if got := ResolveS3Action(r, tt.baseAction, "", ""); got != tt.baseAction { + t.Errorf("ResolveS3Action() = %q, want %q", got, tt.baseAction) + } + }) + } +} diff --git a/weed/s3api/s3api_embedded_iam.go b/weed/s3api/s3api_embedded_iam.go index 6350e821e..4dfab6074 100644 --- a/weed/s3api/s3api_embedded_iam.go +++ b/weed/s3api/s3api_embedded_iam.go @@ -2474,14 +2474,39 @@ func iamRequiresAdminForOthers(action string) bool { return iamSelfServiceActions[action] } -// AuthIam provides IAM-specific authentication that allows self-service operations. -// Users can manage their own access keys without admin rights, but need admin for operations on other users. -// The action parameter is accepted for interface compatibility with cb.Limit but is not used -// since IAM permission checking is done based on the IAM Action parameter in the request. -func (e *EmbeddedIamApi) AuthIam(f http.HandlerFunc, _ Action) http.HandlerFunc { +// AuthorizeIamAction authorizes an IAM management action for identity, with +// targetUserName taken from the request's UserName parameter. +// +// 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 +// data-plane policy, which says nothing about administering the credential +// store. +// +// Users may run self-service actions against their own identity without any +// IAM grant, matching AWS. +func (iam *IdentityAccessManagement) AuthorizeIamAction(r *http.Request, identity *Identity, action, targetUserName string) s3err.ErrorCode { + // The anonymous identity has no user of its own, so every self-service + // action it names would run against someone else's. + if identity == nil || identity.Name == s3_constants.AccountAnonymousId { + return s3err.ErrAccessDenied + } + if iamRequiresAdminForOthers(action) && (targetUserName == "" || targetUserName == identity.Name) { + return s3err.ErrNone + } + if identity.isAdmin() { + return s3err.ErrNone + } + return iam.VerifyActionPermission(r, identity, Action("iam:"+action), "arn:aws:iam:::*", "") +} + +// AuthIamManagement authenticates an IAM management request and authorizes the +// action it carries. It is the entry point for both IAM API surfaces — the +// embedded one on the S3 port and the standalone `weed iam` server — so the two +// cannot drift apart. +func (iam *IdentityAccessManagement) AuthIamManagement(f http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { // If auth is not enabled, allow all - if !e.iam.isEnabled() { + if !iam.isEnabled() { f(w, r) return } @@ -2491,7 +2516,7 @@ func (e *EmbeddedIamApi) AuthIam(f http.HandlerFunc, _ Action) http.HandlerFunc // needs to hash the body for IAM requests (service != "s3"). // The streamHashRequestBody function in auth_signature_v4.go preserves the body // after reading it, so ParseForm() will work correctly after authentication. - identity, errCode := e.iam.AuthSignatureOnly(r) + identity, errCode := iam.AuthSignatureOnly(r) if errCode != s3err.ErrNone { s3err.WriteErrorResponse(w, r, errCode) return @@ -2503,50 +2528,29 @@ func (e *EmbeddedIamApi) AuthIam(f http.HandlerFunc, _ Action) http.HandlerFunc return } - action := r.Form.Get("Action") - targetUserName := r.PostForm.Get("UserName") - - // IAM API requests must be authenticated - reject nil identity - // (can happen for authTypePostPolicy or authTypeStreamingUnsigned) - if identity == nil { - s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) + // 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 { + s3err.WriteErrorResponse(w, r, errCode) return } - // Store identity in context - if identity != nil && identity.Name != "" { + if identity.Name != "" { r = r.WithContext(recordIdentityInContext(r, identity)) } - // Check permissions based on action type - if iamRequiresAdminForOthers(action) { - // Self-service action: allow if operating on own resources or no target specified - if targetUserName == "" || targetUserName == identity.Name { - // Self-service: allowed - f(w, r) - return - } - // Operating on another user: require admin or permission - if !identity.isAdmin() { - if e.iam.VerifyActionPermission(r, identity, Action("iam:"+action), "arn:aws:iam:::*", "") != s3err.ErrNone { - s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) - return - } - } - } else { - // All other IAM actions require admin or permission - if !identity.isAdmin() { - if e.iam.VerifyActionPermission(r, identity, Action("iam:"+action), "arn:aws:iam:::*", "") != s3err.ErrNone { - s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) - return - } - } - } - f(w, r) } } +// AuthIam provides IAM-specific authentication that allows self-service operations. +// Users can manage their own access keys without admin rights, but need admin for operations on other users. +// The action parameter is accepted for interface compatibility with cb.Limit but is not used +// since IAM permission checking is done based on the IAM Action parameter in the request. +func (e *EmbeddedIamApi) AuthIam(f http.HandlerFunc, _ Action) http.HandlerFunc { + return e.iam.AuthIamManagement(f) +} + // ExecuteAction executes an IAM action with the given values. // If skipPersist is true, the changed configuration is not saved to the persistent store. // reqID is set on the response; if empty, a new request ID is generated. diff --git a/weed/s3api/s3api_embedded_iam_authz_test.go b/weed/s3api/s3api_embedded_iam_authz_test.go new file mode 100644 index 000000000..d39ab19b3 --- /dev/null +++ b/weed/s3api/s3api_embedded_iam_authz_test.go @@ -0,0 +1,95 @@ +package s3api + +import ( + "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" +) + +const ( + dataPlanePolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:*","Resource":"*"}]}` + iamAdminPolicy = `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"iam:*","Resource":"*"}]}` +) + +func newIamAuthzTestIam(t *testing.T) *IdentityAccessManagement { + t.Helper() + iam := &IdentityAccessManagement{} + require.NoError(t, iam.PutPolicy("PowerUserPolicy", dataPlanePolicy)) + require.NoError(t, iam.PutPolicy("IamAdminPolicy", iamAdminPolicy)) + return iam +} + +func iamPostRequest(rawQuery string) *http.Request { + url := "http://s3.example.com/" + if rawQuery != "" { + url += "?" + rawQuery + } + return httptest.NewRequest(http.MethodPost, url, nil) +} + +// A policy granting the S3 data plane must not reach IAM management, including +// when the request carries an S3 query parameter — the action resolver used to +// read the request shape even for an iam: action. +func TestAuthorizeIamActionDeniesDataPlanePolicy(t *testing.T) { + iam := newIamAuthzTestIam(t) + identity := &Identity{Name: "power_user", PolicyNames: []string{"PowerUserPolicy"}} + + for _, rawQuery := range []string{"", "delete", "acl", "tagging", "policy", "versions"} { + t.Run("query="+rawQuery, func(t *testing.T) { + assert.Equal(t, s3err.ErrAccessDenied, + iam.AuthorizeIamAction(iamPostRequest(rawQuery), identity, "CreateUser", "victim")) + assert.Equal(t, s3err.ErrAccessDenied, + iam.AuthorizeIamAction(iamPostRequest(rawQuery), identity, "CreateAccessKey", "victim")) + }) + } +} + +func TestAuthorizeIamActionAllowsIamPolicy(t *testing.T) { + iam := newIamAuthzTestIam(t) + identity := &Identity{Name: "iam_operator", PolicyNames: []string{"IamAdminPolicy"}} + + for _, rawQuery := range []string{"", "delete"} { + t.Run("query="+rawQuery, func(t *testing.T) { + assert.Equal(t, s3err.ErrNone, + iam.AuthorizeIamAction(iamPostRequest(rawQuery), identity, "CreateUser", "victim")) + }) + } +} + +func TestAuthorizeIamActionSelfServiceAndAdmin(t *testing.T) { + iam := newIamAuthzTestIam(t) + powerUser := &Identity{Name: "power_user", PolicyNames: []string{"PowerUserPolicy"}} + admin := &Identity{Name: "admin", Actions: []Action{s3_constants.ACTION_ADMIN}} + + assert.Equal(t, s3err.ErrNone, iam.AuthorizeIamAction(iamPostRequest(""), powerUser, "CreateAccessKey", "")) + assert.Equal(t, s3err.ErrNone, iam.AuthorizeIamAction(iamPostRequest(""), powerUser, "CreateAccessKey", "power_user")) + assert.Equal(t, s3err.ErrNone, iam.AuthorizeIamAction(iamPostRequest(""), admin, "CreateUser", "victim")) + assert.Equal(t, s3err.ErrAccessDenied, iam.AuthorizeIamAction(iamPostRequest(""), nil, "CreateUser", "victim")) +} + +// An identity with no grant at all is the negative control: the route itself +// was never open, so a denial here has to come from the policy check. +func TestAuthorizeIamActionDeniesUngrantedIdentity(t *testing.T) { + iam := newIamAuthzTestIam(t) + identity := &Identity{Name: "nobody"} + + assert.Equal(t, s3err.ErrAccessDenied, + iam.AuthorizeIamAction(iamPostRequest(""), identity, "CreateUser", "victim")) +} + +// A configured anonymous identity must not slip in through the self-service +// carve-out, which would otherwise hand it the caller-implied user name. +func TestAuthorizeIamActionDeniesAnonymous(t *testing.T) { + iam := newIamAuthzTestIam(t) + anonymous := &Identity{Name: s3_constants.AccountAnonymousId, PolicyNames: []string{"IamAdminPolicy"}} + + assert.Equal(t, s3err.ErrAccessDenied, + iam.AuthorizeIamAction(iamPostRequest(""), anonymous, "CreateAccessKey", "")) + assert.Equal(t, s3err.ErrAccessDenied, + iam.AuthorizeIamAction(iamPostRequest(""), anonymous, "CreateUser", "victim")) +} diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index ba0317a6d..a1a5f59ce 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -723,21 +723,11 @@ func (s3a *S3ApiServer) UnifiedPostHandler(w http.ResponseWriter, r *http.Reques // Always set identity in context when non-nil to ensure downstream handlers have access r = r.WithContext(recordIdentityInContext(r, identity)) - targetUserName := r.Form.Get("UserName") - - // Check permissions based on action type - isSelfServiceAction := iamRequiresAdminForOthers(action) - isActingOnSelf := targetUserName == "" || targetUserName == identity.Name - - // Permission check is required for all actions except for self-service actions - // performed on the user's own identity. - if !(isSelfServiceAction && isActingOnSelf) { - if !identity.isAdmin() { - if s3a.iam.VerifyActionPermission(r, identity, Action("iam:"+action), "arn:aws:iam:::*", "") != s3err.ErrNone { - s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) - return - } - } + // 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 { + s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) + return } // Call Limit middleware + DoActions