From a17ff669759beeed4bfe5a07e5ef26fef0c7720c Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 19 Dec 2023 12:47:28 +0300 Subject: [PATCH] [#282] policy: Use prefixes to distinguish s3/iam actions/resources Signed-off-by: Denis Kirillov --- api/handler/acl.go | 2 +- api/middleware/policy.go | 47 +++++++++++++++++++++++++--------------- api/router_mock_test.go | 20 ++++++++++++----- api/router_test.go | 47 +++++++++++++++++++++++++++++++++++++++- go.mod | 2 +- go.sum | 4 ++-- 6 files changed, 93 insertions(+), 29 deletions(-) diff --git a/api/handler/acl.go b/api/handler/acl.go index 50ab0e9d..5e526245 100644 --- a/api/handler/acl.go +++ b/api/handler/acl.go @@ -574,7 +574,7 @@ func (h *handler) PutBucketPolicyHandler(w http.ResponseWriter, r *http.Request) for _, rule := range s3Chain.Rules { for _, resource := range rule.Resources.Names { - if reqInfo.BucketName != strings.Split(resource, "/")[0] { + if reqInfo.BucketName != strings.Split(strings.TrimPrefix(resource, arnAwsPrefix), "/")[0] { h.logAndSendError(w, "policy resource mismatched bucket", reqInfo, errors.GetAPIError(errors.ErrMalformedPolicy)) return } diff --git a/api/middleware/policy.go b/api/middleware/policy.go index 60f7b3ba..be99a95d 100644 --- a/api/middleware/policy.go +++ b/api/middleware/policy.go @@ -91,30 +91,41 @@ const ( ) func determineOperationAndResource(r *http.Request, domains []string) (operation string, resource string) { - reqType := noneType + var ( + reqType ReqType + matchDomain bool + ) - var matchDomain bool for _, domain := range domains { - if ind := strings.Index(r.Host, "."+domain); ind != -1 { - matchDomain = true - reqType = bucketType - resource = r.Host[:ind] - trimmedObj := strings.TrimPrefix(r.URL.Path, "/") - if trimmedObj != "" { - reqType = objectType - resource += "/" + trimmedObj - } + ind := strings.Index(r.Host, "."+domain) + if ind == -1 { + continue } + + matchDomain = true + reqType = bucketType + bkt := r.Host[:ind] + if obj := strings.TrimPrefix(r.URL.Path, "/"); obj != "" { + reqType = objectType + resource = fmt.Sprintf(s3.ResourceFormatS3BucketObject, bkt, obj) + } else { + resource = fmt.Sprintf(s3.ResourceFormatS3Bucket, bkt) + } + + break } if !matchDomain { - resource = strings.TrimPrefix(r.URL.Path, "/") - if resource != "" { - if arr := strings.Split(resource, "/"); len(arr) == 1 { - reqType = bucketType - } else { - reqType = objectType + bktObj := strings.TrimPrefix(r.URL.Path, "/") + if ind := strings.IndexByte(bktObj, '/'); ind == -1 { + reqType = bucketType + resource = fmt.Sprintf(s3.ResourceFormatS3Bucket, bktObj) + if bktObj == "" { + reqType = noneType } + } else { + reqType = objectType + resource = fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktObj[:ind], bktObj[ind+1:]) } } @@ -127,7 +138,7 @@ func determineOperationAndResource(r *http.Request, domains []string) (operation operation = determineGeneralOperation(r) } - return operation, resource + return "s3:" + operation, resource } func determineBucketOperation(r *http.Request) string { diff --git a/api/router_mock_test.go b/api/router_mock_test.go index 2fc96470..2e5c3a64 100644 --- a/api/router_mock_test.go +++ b/api/router_mock_test.go @@ -302,9 +302,13 @@ func (h *handlerMock) CreateBucketHandler(http.ResponseWriter, *http.Request) { panic("implement me") } -func (h *handlerMock) HeadBucketHandler(http.ResponseWriter, *http.Request) { - //TODO implement me - panic("implement me") +func (h *handlerMock) HeadBucketHandler(w http.ResponseWriter, r *http.Request) { + res := &handlerResult{ + Method: middleware.HeadBucketOperation, + ReqInfo: middleware.GetReqInfo(r.Context()), + } + + h.writeResponse(w, res) } func (h *handlerMock) PostObject(http.ResponseWriter, *http.Request) { @@ -337,9 +341,13 @@ func (h *handlerMock) DeleteBucketHandler(http.ResponseWriter, *http.Request) { panic("implement me") } -func (h *handlerMock) ListBucketsHandler(http.ResponseWriter, *http.Request) { - //TODO implement me - panic("implement me") +func (h *handlerMock) ListBucketsHandler(w http.ResponseWriter, r *http.Request) { + res := &handlerResult{ + Method: middleware.ListBucketsOperation, + ReqInfo: middleware.GetReqInfo(r.Context()), + } + + h.writeResponse(w, res) } func (h *handlerMock) Preflight(http.ResponseWriter, *http.Request) { diff --git a/api/router_test.go b/api/router_test.go index 327a0586..2b77dd6e 100644 --- a/api/router_test.go +++ b/api/router_test.go @@ -14,9 +14,11 @@ import ( apiErrors "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" s3middleware "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/metrics" + engineiam "git.frostfs.info/TrueCloudLab/policy-engine/iam" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/engine" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/engine/inmemory" + "git.frostfs.info/TrueCloudLab/policy-engine/schema/s3" "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" "github.com/stretchr/testify/require" @@ -49,6 +51,7 @@ func prepareRouter(t *testing.T) *routerMock { Metrics: &metrics.AppMetrics{}, MiddlewareSettings: middlewareSettings, PolicyChecker: policyChecker, + Domains: []string{"domain1", "domain2"}, } return &routerMock{ router: NewRouter(cfg), @@ -163,7 +166,7 @@ func TestPolicyChecker(t *testing.T) { Rules: []chain.Rule{{ Status: chain.AccessDenied, Actions: chain.Actions{Names: []string{"*"}}, - Resources: chain.Resources{Names: []string{bktName + "/*"}}, + Resources: chain.Resources{Names: []string{fmt.Sprintf(s3.ResourceFormatS3BucketObjects, bktName)}}, }}, } @@ -190,6 +193,48 @@ func TestPolicyChecker(t *testing.T) { assertAPIError(t, w, apiErrors.ErrAccessDenied) } +func TestPolicyCheckerReqTypeDetermination(t *testing.T) { + chiRouter := prepareRouter(t) + bktName, objName := "bucket", "object" + + policy := engineiam.Policy{ + Statement: []engineiam.Statement{{ + Principal: map[engineiam.PrincipalType][]string{engineiam.Wildcard: {}}, + Effect: engineiam.AllowEffect, + Action: engineiam.Action{"s3:*"}, + Resource: engineiam.Resource{fmt.Sprintf(s3.ResourceFormatS3All)}, + }}, + } + + ruleChain, err := engineiam.ConvertToS3Chain(policy, nil) + require.NoError(t, err) + + _, _, err = chiRouter.policyChecker.MorphRuleChainStorage().AddMorphRuleChain(chain.S3, engine.NamespaceTarget(""), ruleChain) + require.NoError(t, err) + + chiRouter.middlewareSettings.denyByDefault = true + t.Run("can list buckets", func(t *testing.T) { + w, r := httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/", nil) + chiRouter.ServeHTTP(w, r) + resp := readResponse(t, w) + require.Equal(t, s3middleware.ListBucketsOperation, resp.Method) + }) + + t.Run("can head 'bucket'", func(t *testing.T) { + w, r := httptest.NewRecorder(), httptest.NewRequest(http.MethodHead, "/"+bktName, nil) + chiRouter.ServeHTTP(w, r) + resp := readResponse(t, w) + require.Equal(t, s3middleware.HeadBucketOperation, resp.Method) + }) + + t.Run("can put object into 'bucket'", func(t *testing.T) { + w, r := httptest.NewRecorder(), httptest.NewRequest(http.MethodPut, fmt.Sprintf("/%s/%s", bktName, objName), nil) + chiRouter.ServeHTTP(w, r) + resp := readResponse(t, w) + require.Equal(t, s3middleware.PutObjectOperation, resp.Method) + }) +} + func TestDefaultBehaviorPolicyChecker(t *testing.T) { chiRouter := prepareRouter(t) bktName, objName := "bucket", "object" diff --git a/go.mod b/go.mod index de67fa6d..1630b618 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( git.frostfs.info/TrueCloudLab/frostfs-contract v0.18.1-0.20231129062201-a1b61d394958 git.frostfs.info/TrueCloudLab/frostfs-observability v0.0.0-20230531082742-c97d21411eb6 git.frostfs.info/TrueCloudLab/frostfs-sdk-go v0.0.0-20231107114540-ab75edd70939 - git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20231213132038-1d07331f5df5 + git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20231220070831-3128352693fc git.frostfs.info/TrueCloudLab/zapjournald v0.0.0-20231018083019-2b6d84de9a3d github.com/aws/aws-sdk-go v1.44.6 github.com/bluele/gcache v0.0.2 diff --git a/go.sum b/go.sum index f3b2236b..d70ba9da 100644 --- a/go.sum +++ b/go.sum @@ -48,8 +48,8 @@ git.frostfs.info/TrueCloudLab/frostfs-sdk-go v0.0.0-20231107114540-ab75edd70939 git.frostfs.info/TrueCloudLab/frostfs-sdk-go v0.0.0-20231107114540-ab75edd70939/go.mod h1:t1akKcUH7iBrFHX8rSXScYMP17k2kYQXMbZooiL5Juw= git.frostfs.info/TrueCloudLab/hrw v1.2.1 h1:ccBRK21rFvY5R1WotI6LNoPlizk7qSvdfD8lNIRudVc= git.frostfs.info/TrueCloudLab/hrw v1.2.1/go.mod h1:C1Ygde2n843yTZEQ0FP69jYiuaYV0kriLvP4zm8JuvM= -git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20231213132038-1d07331f5df5 h1:vNDlTalmXHL4jVbDfquBdXeoevglOOFImOM/yanH14A= -git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20231213132038-1d07331f5df5/go.mod h1:iJMX6qk9aIHIu3WVSd4puF5CHsNk5eOi++MaJJfNbXM= +git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20231220070831-3128352693fc h1:ZBZkWBbDmqSdMoq7igIg4EYMIgbyFaLGcpHcU3urDnI= +git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20231220070831-3128352693fc/go.mod h1:v43imcuSmDwSNrePe4UTQh8jaE8FmsiKN3FcaEzmRzc= git.frostfs.info/TrueCloudLab/rfc6979 v0.4.0 h1:M2KR3iBj7WpY3hP10IevfIB9MURr4O9mwVfJ+SjT3HA= git.frostfs.info/TrueCloudLab/rfc6979 v0.4.0/go.mod h1:okpbKfVYf/BpejtfFTfhZqFP+sZ8rsHrP8Rr/jYPNRc= git.frostfs.info/TrueCloudLab/tzhash v1.8.0 h1:UFMnUIk0Zh17m8rjGHJMqku2hCgaXDqjqZzS4gsb4UA=