From 37d05dcefd27295a6b86a3fabc10983a39de83bd Mon Sep 17 00:00:00 2001 From: Marina Biryukova Date: Thu, 4 Apr 2024 18:06:16 +0300 Subject: [PATCH] [#353] Add check of listing parameters and versionID Add properties in policy check: * s3:delimiter * s3:prefix * s3:max-keys * s3:VersionId Signed-off-by: Marina Biryukova --- api/middleware/policy.go | 49 ++++++++++-- api/router_test.go | 169 ++++++++++++++++++++++++++++++++++++--- go.mod | 2 +- go.sum | 4 +- 4 files changed, 202 insertions(+), 22 deletions(-) diff --git a/api/middleware/policy.go b/api/middleware/policy.go index 02cf50a8..35d1e7ac 100644 --- a/api/middleware/policy.go +++ b/api/middleware/policy.go @@ -20,6 +20,13 @@ import ( "go.uber.org/zap" ) +const ( + QueryVersionID = "versionId" + QueryPrefix = "prefix" + QueryDelimiter = "delimiter" + QueryMaxKeys = "max-keys" +) + type PolicySettings interface { PolicyDenyByDefault() bool ACLEnabled() bool @@ -139,15 +146,12 @@ func getPolicyRequest(r *http.Request, frostfsid FrostFSIDInformer, reqType ReqT res = fmt.Sprintf(s3.ResourceFormatS3Bucket, bktName) } - reqLogOrDefault(r.Context(), log).Debug(logs.PolicyRequest, zap.String("action", op), - zap.String("resource", res), zap.String("owner", owner)) + properties := determineProperties(ctx, reqType, op, owner, groups) - return testutil.NewRequest(op, testutil.NewResource(res, nil), - map[string]string{ - s3.PropertyKeyOwner: owner, - common.PropertyKeyFrostFSIDGroupID: chain.FormCondSliceContainsValue(groups), - }, - ), nil + reqLogOrDefault(r.Context(), log).Debug(logs.PolicyRequest, zap.String("action", op), + zap.String("resource", res), zap.Any("properties", properties)) + + return testutil.NewRequest(op, testutil.NewResource(res, nil), properties), nil } type ReqType int @@ -372,3 +376,32 @@ func determineGeneralOperation(r *http.Request) string { } return "UnmatchedOperation" } + +func determineProperties(ctx context.Context, reqType ReqType, op, owner string, groups []string) map[string]string { + res := map[string]string{ + s3.PropertyKeyOwner: owner, + common.PropertyKeyFrostFSIDGroupID: chain.FormCondSliceContainsValue(groups), + } + queries := GetReqInfo(ctx).URL.Query() + + if reqType == objectType { + if versionID := queries.Get(QueryVersionID); len(versionID) > 0 { + res[s3.PropertyKeyVersionID] = versionID + } + } + + if reqType == bucketType && (strings.HasSuffix(op, ListObjectsV1Operation) || strings.HasSuffix(op, ListObjectsV2Operation) || + strings.HasSuffix(op, ListBucketObjectVersionsOperation) || strings.HasSuffix(op, ListMultipartUploadsOperation)) { + if prefix := queries.Get(QueryPrefix); len(prefix) > 0 { + res[s3.PropertyKeyPrefix] = prefix + } + if delimiter := queries.Get(QueryDelimiter); len(delimiter) > 0 { + res[s3.PropertyKeyDelimiter] = delimiter + } + if maxKeys := queries.Get(QueryMaxKeys); len(maxKeys) > 0 { + res[s3.PropertyKeyMaxKeys] = maxKeys + } + } + + return res +} diff --git a/api/router_test.go b/api/router_test.go index fd7efec3..bca051cb 100644 --- a/api/router_test.go +++ b/api/router_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strconv" "testing" "time" @@ -270,7 +271,7 @@ func TestACLAPE(t *testing.T) { listBucketsErr(router, ns, apiErrors.ErrAccessDenied) // Allow operations and check - allowOperations(router, ns, []string{"s3:CreateBucket", "s3:ListAllMyBuckets"}) + allowOperations(router, ns, []string{"s3:CreateBucket", "s3:ListAllMyBuckets"}, nil) createBucket(router, ns, bktName) listBuckets(router, ns) }) @@ -296,7 +297,7 @@ func TestACLAPE(t *testing.T) { listBuckets(router, ns) // Deny operations and check - denyOperations(router, ns, []string{"s3:CreateBucket", "s3:ListAllMyBuckets"}) + denyOperations(router, ns, []string{"s3:CreateBucket", "s3:ListAllMyBuckets"}, nil) createBucketErr(router, ns, bktName, apiErrors.ErrAccessDenied) listBucketsErr(router, ns, apiErrors.ErrAccessDenied) }) @@ -344,22 +345,136 @@ func TestACLAPE(t *testing.T) { }) } -func allowOperations(router *routerMock, ns string, operations []string) { - addPolicy(router, ns, "allow", engineiam.AllowEffect, operations) +func TestRequestParametersCheck(t *testing.T) { + t.Run("prefix parameter, allow specific value", func(t *testing.T) { + router := prepareRouter(t) + + ns, bktName, prefix := "", "bucket", "prefix" + router.middlewareSettings.denyByDefault = true + + allowOperations(router, ns, []string{"s3:CreateBucket"}, nil) + createBucket(router, ns, bktName) + + // Add policies and check + denyOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondStringNotEquals: engineiam.Condition{s3.PropertyKeyPrefix: []string{prefix}}, + }) + allowOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondStringEquals: engineiam.Condition{s3.PropertyKeyPrefix: []string{prefix}}, + }) + + listObjectsV1(router, ns, bktName, prefix, "", "") + listObjectsV1Err(router, ns, bktName, "", "", "", apiErrors.ErrAccessDenied) + listObjectsV1Err(router, ns, bktName, "invalid", "", "", apiErrors.ErrAccessDenied) + }) + + t.Run("delimiter parameter, prohibit specific value", func(t *testing.T) { + router := prepareRouter(t) + + ns, bktName, delimiter := "", "bucket", "delimiter" + router.middlewareSettings.denyByDefault = true + + allowOperations(router, ns, []string{"s3:CreateBucket"}, nil) + createBucket(router, ns, bktName) + + // Add policies and check + denyOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondStringEquals: engineiam.Condition{s3.PropertyKeyDelimiter: []string{delimiter}}, + }) + allowOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondStringNotEquals: engineiam.Condition{s3.PropertyKeyDelimiter: []string{delimiter}}, + }) + + listObjectsV1(router, ns, bktName, "", "", "") + listObjectsV1(router, ns, bktName, "", "some-delimiter", "") + listObjectsV1Err(router, ns, bktName, "", delimiter, "", apiErrors.ErrAccessDenied) + }) + + t.Run("max-keys parameter, allow specific value", func(t *testing.T) { + router := prepareRouter(t) + + ns, bktName, maxKeys := "", "bucket", 10 + router.middlewareSettings.denyByDefault = true + + allowOperations(router, ns, []string{"s3:CreateBucket"}, nil) + createBucket(router, ns, bktName) + + // Add policies and check + denyOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondNumericNotEquals: engineiam.Condition{s3.PropertyKeyMaxKeys: []string{strconv.Itoa(maxKeys)}}, + }) + allowOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondNumericEquals: engineiam.Condition{s3.PropertyKeyMaxKeys: []string{strconv.Itoa(maxKeys)}}, + }) + + listObjectsV1(router, ns, bktName, "", "", strconv.Itoa(maxKeys)) + listObjectsV1Err(router, ns, bktName, "", "", "", apiErrors.ErrAccessDenied) + listObjectsV1Err(router, ns, bktName, "", "", strconv.Itoa(maxKeys-1), apiErrors.ErrAccessDenied) + listObjectsV1Err(router, ns, bktName, "", "", "invalid", apiErrors.ErrAccessDenied) + }) + + t.Run("max-keys parameter, allow range of values", func(t *testing.T) { + router := prepareRouter(t) + + ns, bktName, maxKeys := "", "bucket", 10 + router.middlewareSettings.denyByDefault = true + + allowOperations(router, ns, []string{"s3:CreateBucket"}, nil) + createBucket(router, ns, bktName) + + // Add policies and check + denyOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondNumericGreaterThan: engineiam.Condition{s3.PropertyKeyMaxKeys: []string{strconv.Itoa(maxKeys)}}, + }) + allowOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondNumericLessThanEquals: engineiam.Condition{s3.PropertyKeyMaxKeys: []string{strconv.Itoa(maxKeys)}}, + }) + + listObjectsV1(router, ns, bktName, "", "", strconv.Itoa(maxKeys)) + listObjectsV1(router, ns, bktName, "", "", strconv.Itoa(maxKeys-1)) + listObjectsV1Err(router, ns, bktName, "", "", strconv.Itoa(maxKeys+1), apiErrors.ErrAccessDenied) + }) + + t.Run("max-keys parameter, prohibit specific value", func(t *testing.T) { + router := prepareRouter(t) + + ns, bktName, maxKeys := "", "bucket", 10 + router.middlewareSettings.denyByDefault = true + + allowOperations(router, ns, []string{"s3:CreateBucket"}, nil) + createBucket(router, ns, bktName) + + // Add policies and check + denyOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondNumericEquals: engineiam.Condition{s3.PropertyKeyMaxKeys: []string{strconv.Itoa(maxKeys)}}, + }) + allowOperations(router, ns, []string{"s3:ListBucket"}, engineiam.Conditions{ + engineiam.CondNumericNotEquals: engineiam.Condition{s3.PropertyKeyMaxKeys: []string{strconv.Itoa(maxKeys)}}, + }) + + listObjectsV1(router, ns, bktName, "", "", "") + listObjectsV1(router, ns, bktName, "", "", strconv.Itoa(maxKeys-1)) + listObjectsV1Err(router, ns, bktName, "", "", strconv.Itoa(maxKeys), apiErrors.ErrAccessDenied) + }) } -func denyOperations(router *routerMock, ns string, operations []string) { - addPolicy(router, ns, "deny", engineiam.DenyEffect, operations) +func allowOperations(router *routerMock, ns string, operations []string, conditions engineiam.Conditions) { + addPolicy(router, ns, "allow", engineiam.AllowEffect, operations, conditions) } -func addPolicy(router *routerMock, ns string, id string, effect engineiam.Effect, operations []string) { +func denyOperations(router *routerMock, ns string, operations []string, conditions engineiam.Conditions) { + addPolicy(router, ns, "deny", engineiam.DenyEffect, operations, conditions) +} + +func addPolicy(router *routerMock, ns string, id string, effect engineiam.Effect, operations []string, conditions engineiam.Conditions) { policy := engineiam.Policy{ Version: "2012-10-17", Statement: []engineiam.Statement{{ - Principal: map[engineiam.PrincipalType][]string{engineiam.Wildcard: {}}, - Effect: effect, - Action: engineiam.Action(operations), - Resource: engineiam.Resource{fmt.Sprintf(s3.ResourceFormatS3All)}, + Principal: map[engineiam.PrincipalType][]string{engineiam.Wildcard: {}}, + Effect: effect, + Action: engineiam.Action(operations), + Resource: engineiam.Resource{fmt.Sprintf(s3.ResourceFormatS3All)}, + Conditions: conditions, }}, } @@ -441,6 +556,38 @@ func putObjectBase(router *routerMock, namespace, bktName, objName string) *http return w } +func listObjectsV1(router *routerMock, namespace, bktName, prefix, delimiter, maxKeys string) handlerResult { + w := listObjectsV1Base(router, namespace, bktName, prefix, delimiter, maxKeys) + resp := readResponse(router.t, w) + require.Equal(router.t, s3middleware.ListObjectsV1Operation, resp.Method) + return resp +} + +func listObjectsV1Err(router *routerMock, namespace, bktName, prefix, delimiter, maxKeys string, errCode apiErrors.ErrorCode) { + w := listObjectsV1Base(router, namespace, bktName, prefix, delimiter, maxKeys) + assertAPIError(router.t, w, errCode) +} + +func listObjectsV1Base(router *routerMock, namespace, bktName, prefix, delimiter, maxKeys string) *httptest.ResponseRecorder { + queries := url.Values{} + if len(prefix) > 0 { + queries.Add(s3middleware.QueryPrefix, prefix) + } + if len(delimiter) > 0 { + queries.Add(s3middleware.QueryDelimiter, delimiter) + } + if len(maxKeys) > 0 { + queries.Add(s3middleware.QueryMaxKeys, maxKeys) + } + encoded := queries.Encode() + + w, r := httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, "/"+bktName, nil) + r.URL.RawQuery = encoded + r.Header.Set(FrostfsNamespaceHeader, namespace) + router.ServeHTTP(w, r) + return w +} + func TestOwnerIDRetrieving(t *testing.T) { chiRouter := prepareRouter(t) diff --git a/go.mod b/go.mod index 6a2b8643..b61b8fe0 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( git.frostfs.info/TrueCloudLab/frostfs-contract v0.19.0 git.frostfs.info/TrueCloudLab/frostfs-observability v0.0.0-20230531082742-c97d21411eb6 git.frostfs.info/TrueCloudLab/frostfs-sdk-go v0.0.0-20240301150205-6fe4e2541d0b - git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20240402080942-42497ad2424c + git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20240408113043-84c6be01de16 git.frostfs.info/TrueCloudLab/zapjournald v0.0.0-20240124114243-cb2e66427d02 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 7466a657..87443b5f 100644 --- a/go.sum +++ b/go.sum @@ -48,8 +48,8 @@ git.frostfs.info/TrueCloudLab/frostfs-sdk-go v0.0.0-20240301150205-6fe4e2541d0b git.frostfs.info/TrueCloudLab/frostfs-sdk-go v0.0.0-20240301150205-6fe4e2541d0b/go.mod h1:XcgrbZ88XfvhAMxmZCQJ0dv6FyRSq6Mg2J7nN8uuO0k= 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-20240402080942-42497ad2424c h1:0aYo2YNjrC4cc/os4b1+4weSlvbP2eliXQJouxJoaAk= -git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20240402080942-42497ad2424c/go.mod h1:H/AW85RtYxVTbcgwHW76DqXeKlsiCIOeNXHPqyDBrfQ= +git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20240408113043-84c6be01de16 h1:Q1pMaAM4DNRJZMB4quYjRmPDJMS3n3vfkoYojto13oQ= +git.frostfs.info/TrueCloudLab/policy-engine v0.0.0-20240408113043-84c6be01de16/go.mod h1:H/AW85RtYxVTbcgwHW76DqXeKlsiCIOeNXHPqyDBrfQ= 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=