[#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 <m.biryukova@yadro.com>
2024-04-04 18:06:16 +03:00 · 2024-04-04 18:06:16 +03:00 · 37d05dcefd
commit 37d05dcefd
parent 8407b3ea4c
4 changed files with 202 additions and 22 deletions
--- 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
+}
--- 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)

--- 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
--- 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=