From 77f8bdac58a1ed60d2cb41324240f96102758f1e Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 28 May 2024 11:47:26 +0300 Subject: [PATCH] [#372] Drop kludge.acl_enabled flag Now only APE container can be created using s3-gw Signed-off-by: Denis Kirillov --- api/handler/acl_test.go | 53 ------------------------- api/handler/api.go | 1 - api/handler/handlers_test.go | 5 --- api/handler/put.go | 77 ------------------------------------ api/handler/put_test.go | 15 ------- api/middleware/policy.go | 3 +- api/router_mock_test.go | 7 +--- api/router_test.go | 67 +++++++++---------------------- cmd/s3-gw/app.go | 14 ------- cmd/s3-gw/app_settings.go | 3 -- config/config.env | 2 - config/config.yaml | 2 - docs/configuration.md | 2 - 13 files changed, 20 insertions(+), 231 deletions(-) diff --git a/api/handler/acl_test.go b/api/handler/acl_test.go index 490d321a7..1081e8f88 100644 --- a/api/handler/acl_test.go +++ b/api/handler/acl_test.go @@ -1301,21 +1301,6 @@ func TestBucketAclToAst(t *testing.T) { require.Equal(t, expectedAst, actualAst) } -func TestPutBucketACL(t *testing.T) { - tc := prepareHandlerContext(t) - tc.config.aclEnabled = true - bktName := "bucket-for-acl" - - info := createBucket(tc, bktName) - - header := map[string]string{api.AmzACL: "public-read"} - putBucketACL(tc, bktName, info.Box, header) - - header = map[string]string{api.AmzACL: "private"} - putBucketACL(tc, bktName, info.Box, header) - checkLastRecords(t, tc, info.BktInfo, eacl.ActionDeny) -} - func TestPutBucketAPE(t *testing.T) { hc := prepareHandlerContext(t) bktName := "bucket-for-acl-ape" @@ -1361,27 +1346,6 @@ func TestCreateObjectACLErrorAPE(t *testing.T) { createMultipartUpload(hc, bktName, objName, map[string]string{api.AmzACL: basicACLPrivate}) } -func TestPutObjectACLBackwardCompatibility(t *testing.T) { - hc := prepareHandlerContext(t) - hc.config.aclEnabled = true - bktName, objName := "bucket-for-acl-ape", "object" - - info := createBucket(hc, bktName) - - putObjectWithHeadersBase(hc, bktName, objName, map[string]string{api.AmzACL: basicACLPrivate}, info.Box, nil) - putObjectWithHeadersBase(hc, bktName, objName, map[string]string{api.AmzACL: basicACLPublic}, info.Box, nil) - - aclRes := getObjectACL(hc, bktName, objName) - require.Len(t, aclRes.AccessControlList, 2) - require.Equal(t, hex.EncodeToString(info.Key.PublicKey().Bytes()), aclRes.AccessControlList[0].Grantee.ID) - require.Equal(t, aclFullControl, aclRes.AccessControlList[0].Permission) - require.Equal(t, allUsersGroup, aclRes.AccessControlList[1].Grantee.URI) - require.Equal(t, aclFullControl, aclRes.AccessControlList[1].Permission) - - aclBody := &AccessControlPolicy{} - putObjectACLBase(hc, bktName, objName, info.Box, nil, aclBody) -} - func TestBucketACLAPE(t *testing.T) { hc := prepareHandlerContext(t) bktName := "bucket-for-acl-ape" @@ -1648,23 +1612,6 @@ func putBucketPolicy(hc *handlerContext, bktName string, bktPolicy engineiam.Pol } } -func checkLastRecords(t *testing.T, tc *handlerContext, bktInfo *data.BucketInfo, action eacl.Action) { - bktACL, err := tc.Layer().GetBucketACL(tc.Context(), bktInfo) - require.NoError(t, err) - - length := len(bktACL.EACL.Records()) - - if length < 7 { - t.Fatalf("length of records is less than 7: '%d'", length) - } - - for _, rec := range bktACL.EACL.Records()[length-7:] { - if rec.Action() != action || rec.Targets()[0].Role() != eacl.RoleOthers { - t.Fatalf("inavid last record: '%s', '%s', '%s',", rec.Action(), rec.Operation(), rec.Targets()[0].Role()) - } - } -} - func createAccessBox(t *testing.T) (*accessbox.Box, *keys.PrivateKey) { key, err := keys.NewPrivateKey() require.NoError(t, err) diff --git a/api/handler/api.go b/api/handler/api.go index 2b3552728..559977aa6 100644 --- a/api/handler/api.go +++ b/api/handler/api.go @@ -38,7 +38,6 @@ type ( IsResolveListAllow() bool BypassContentEncodingInChunks() bool MD5Enabled() bool - ACLEnabled() bool RetryMaxAttempts() int RetryMaxBackoff() time.Duration RetryStrategy() RetryStrategy diff --git a/api/handler/handlers_test.go b/api/handler/handlers_test.go index 5280c0f0b..254087a96 100644 --- a/api/handler/handlers_test.go +++ b/api/handler/handlers_test.go @@ -72,7 +72,6 @@ type configMock struct { defaultCopiesNumbers []uint32 bypassContentEncodingInChunks bool md5Enabled bool - aclEnabled bool } func (c *configMock) DefaultPlacementPolicy(_ string) netmap.PlacementPolicy { @@ -120,10 +119,6 @@ func (c *configMock) MD5Enabled() bool { return c.md5Enabled } -func (c *configMock) ACLEnabled() bool { - return c.aclEnabled -} - func (c *configMock) ResolveNamespaceAlias(ns string) string { return ns } diff --git a/api/handler/put.go b/api/handler/put.go index 2c180f9fc..050596b36 100644 --- a/api/handler/put.go +++ b/api/handler/put.go @@ -814,11 +814,6 @@ func parseCannedACL(header http.Header) (string, error) { } func (h *handler) CreateBucketHandler(w http.ResponseWriter, r *http.Request) { - if h.cfg.ACLEnabled() { - h.createBucketHandlerACL(w, r) - return - } - h.createBucketHandlerPolicy(w, r) } @@ -941,78 +936,6 @@ func (h *handler) putBucketSettingsRetryer() aws.RetryerV2 { }) } -func (h *handler) createBucketHandlerACL(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() - reqInfo := middleware.GetReqInfo(ctx) - - boxData, err := middleware.GetBoxData(ctx) - if err != nil { - h.logAndSendError(w, "get access box from request", reqInfo, err) - return - } - - key, p, err := h.parseCommonCreateBucketParams(reqInfo, boxData, r) - if err != nil { - h.logAndSendError(w, "parse create bucket params", reqInfo, err) - return - } - - aclPrm := &layer.PutBucketACLParams{SessionToken: boxData.Gate.SessionTokenForSetEACL()} - if aclPrm.SessionToken == nil { - h.logAndSendError(w, "couldn't find session token for setEACL", reqInfo, errors.GetAPIError(errors.ErrAccessDenied)) - return - } - - bktACL, err := parseACLHeaders(r.Header, key) - if err != nil { - h.logAndSendError(w, "could not parse bucket acl", reqInfo, err) - return - } - resInfo := &resourceInfo{Bucket: reqInfo.BucketName} - - aclPrm.EACL, err = bucketACLToTable(bktACL, resInfo) - if err != nil { - h.logAndSendError(w, "could translate bucket acl to eacl", reqInfo, err) - return - } - - bktInfo, err := h.obj.CreateBucket(ctx, p) - if err != nil { - h.logAndSendError(w, "could not create bucket", reqInfo, err) - return - } - h.reqLogger(ctx).Info(logs.BucketIsCreated, zap.Stringer("container_id", bktInfo.CID)) - - aclPrm.BktInfo = bktInfo - if err = h.obj.PutBucketACL(r.Context(), aclPrm); err != nil { - h.logAndSendError(w, "could not put bucket e/ACL", reqInfo, err) - return - } - - sp := &layer.PutSettingsParams{ - BktInfo: bktInfo, - Settings: &data.BucketSettings{ - OwnerKey: key, - Versioning: data.VersioningUnversioned, - }, - } - - if p.ObjectLockEnabled { - sp.Settings.Versioning = data.VersioningEnabled - } - - if err = h.obj.PutBucketSettings(ctx, sp); err != nil { - h.logAndSendError(w, "couldn't save bucket settings", reqInfo, err, - zap.String("container_id", bktInfo.CID.EncodeToString())) - return - } - - if err = middleware.WriteSuccessResponseHeadersOnly(w); err != nil { - h.logAndSendError(w, "write response", reqInfo, err) - return - } -} - const s3ActionPrefix = "s3:" var ( diff --git a/api/handler/put_test.go b/api/handler/put_test.go index 4bf7078bb..152a96945 100644 --- a/api/handler/put_test.go +++ b/api/handler/put_test.go @@ -381,21 +381,6 @@ func TestCreateBucket(t *testing.T) { createBucketAssertS3Error(hc, bktName, box2, s3errors.ErrBucketAlreadyExists) } -func TestCreateOldBucketPutVersioning(t *testing.T) { - hc := prepareHandlerContext(t) - hc.config.aclEnabled = true - bktName := "bkt-name" - - info := createBucket(hc, bktName) - settings, err := hc.tree.GetSettingsNode(hc.Context(), info.BktInfo) - require.NoError(t, err) - settings.OwnerKey = nil - err = hc.tree.PutSettingsNode(hc.Context(), info.BktInfo, settings) - require.NoError(t, err) - - putBucketVersioning(t, hc, bktName, true) -} - func TestCreateNamespacedBucket(t *testing.T) { hc := prepareHandlerContext(t) bktName := "bkt-name" diff --git a/api/middleware/policy.go b/api/middleware/policy.go index e03e36c18..27865352e 100644 --- a/api/middleware/policy.go +++ b/api/middleware/policy.go @@ -51,7 +51,6 @@ var withoutResourceOps = []string{ type PolicySettings interface { PolicyDenyByDefault() bool - ACLEnabled() bool } type FrostFSIDInformer interface { @@ -150,7 +149,7 @@ func policyCheck(r *http.Request, cfg PolicyConfig) error { return apiErr.GetAPIErrorWithError(apiErr.ErrAccessDenied, fmt.Errorf("policy check: %s", st.String())) } - isAPE := !cfg.Settings.ACLEnabled() + isAPE := true if bktInfo != nil { isAPE = bktInfo.APEEnabled } diff --git a/api/router_mock_test.go b/api/router_mock_test.go index 0b72813ae..9ecd94778 100644 --- a/api/router_mock_test.go +++ b/api/router_mock_test.go @@ -72,7 +72,6 @@ func (c *centerMock) Authenticate(*http.Request) (*middleware.Box, error) { type middlewareSettingsMock struct { denyByDefault bool - aclEnabled bool sourceIPHeader string } @@ -92,10 +91,6 @@ func (r *middlewareSettingsMock) PolicyDenyByDefault() bool { return r.denyByDefault } -func (r *middlewareSettingsMock) ACLEnabled() bool { - return r.aclEnabled -} - type frostFSIDMock struct { tags map[string]string validateError bool @@ -430,7 +425,7 @@ func (h *handlerMock) CreateBucketHandler(w http.ResponseWriter, r *http.Request h.buckets[reqInfo.Namespace+reqInfo.BucketName] = &data.BucketInfo{ Name: reqInfo.BucketName, - APEEnabled: !h.cfg.ACLEnabled(), + APEEnabled: true, } res := &handlerResult{ diff --git a/api/router_test.go b/api/router_test.go index bbe51466a..b34101265 100644 --- a/api/router_test.go +++ b/api/router_test.go @@ -17,7 +17,9 @@ 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" + cidtest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id/test" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object" + usertest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/user/test" engineiam "git.frostfs.info/TrueCloudLab/policy-engine/iam" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/engine" @@ -37,6 +39,7 @@ type routerMock struct { cfg Config middlewareSettings *middlewareSettingsMock policyChecker engine.LocalOverrideEngine + handler *handlerMock } func (m *routerMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -64,12 +67,14 @@ func prepareRouter(t *testing.T, opts ...option) *routerMock { Enabled: true, } + handlerTestMock := &handlerMock{t: t, cfg: middlewareSettings, buckets: map[string]*data.BucketInfo{}} + cfg := Config{ Throttle: middleware.ThrottleOpts{ Limit: 10, BacklogTimeout: 30 * time.Second, }, - Handler: &handlerMock{t: t, cfg: middlewareSettings, buckets: map[string]*data.BucketInfo{}}, + Handler: handlerTestMock, Center: ¢erMock{t: t}, Log: logger, Metrics: metrics.NewAppMetrics(metricsConfig), @@ -91,6 +96,7 @@ func prepareRouter(t *testing.T, opts ...option) *routerMock { cfg: cfg, middlewareSettings: middlewareSettings, policyChecker: policyChecker, + handler: handlerTestMock, } } @@ -308,7 +314,6 @@ func TestACLAPE(t *testing.T) { createOldBucket(router, bktNameOld) createNewBucket(router, bktNameNew) - router.middlewareSettings.aclEnabled = false router.middlewareSettings.denyByDefault = true // Allow because of using old bucket @@ -334,7 +339,6 @@ func TestACLAPE(t *testing.T) { createOldBucket(router, bktNameOld) createNewBucket(router, bktNameNew) - router.middlewareSettings.aclEnabled = false router.middlewareSettings.denyByDefault = false // Allow because of using old bucket @@ -351,48 +355,6 @@ func TestACLAPE(t *testing.T) { createBucketErr(router, ns, bktName, nil, apiErrors.ErrAccessDenied) listBucketsErr(router, ns, apiErrors.ErrAccessDenied) }) - - t.Run("acl enabled, ape deny by default", func(t *testing.T) { - router := prepareRouter(t) - - ns, bktName, objName := "", "bucket", "object" - bktNameOld, bktNameNew := "old-bucket", "new-bucket" - createOldBucket(router, bktNameOld) - createNewBucket(router, bktNameNew) - - router.middlewareSettings.aclEnabled = true - router.middlewareSettings.denyByDefault = true - - // Allow because of using old bucket - putObject(router, ns, bktNameOld, objName, nil) - // Deny because of deny by default - putObjectErr(router, ns, bktNameNew, objName, nil, apiErrors.ErrAccessDenied) - - // Allow because of old behavior - createBucket(router, ns, bktName) - listBuckets(router, ns) - }) - - t.Run("acl enabled, ape allow by default", func(t *testing.T) { - router := prepareRouter(t) - - ns, bktName, objName := "", "bucket", "object" - bktNameOld, bktNameNew := "old-bucket", "new-bucket" - createOldBucket(router, bktNameOld) - createNewBucket(router, bktNameNew) - - router.middlewareSettings.aclEnabled = true - router.middlewareSettings.denyByDefault = false - - // Allow because of using old bucket - putObject(router, ns, bktNameOld, objName, nil) - // Allow because of allow by default - putObject(router, ns, bktNameNew, objName, nil) - - // Allow because of old behavior - createBucket(router, ns, bktName) - listBuckets(router, ns) - }) } func TestRequestParametersCheck(t *testing.T) { @@ -726,10 +688,17 @@ func createNewBucket(router *routerMock, bktName string) { } func createSpecificBucket(router *routerMock, bktName string, old bool) { - aclEnabled := router.middlewareSettings.ACLEnabled() - router.middlewareSettings.aclEnabled = old - createBucket(router, "", bktName) - router.middlewareSettings.aclEnabled = aclEnabled + router.handler.buckets[bktName] = &data.BucketInfo{ + Name: bktName, + Zone: "container", + CID: cidtest.ID(), + Owner: usertest.ID(), + Created: time.Now(), + LocationConstraint: "default", + ObjectLockEnabled: false, + HomomorphicHashDisabled: false, + APEEnabled: !old, + } } func createBucket(router *routerMock, namespace, bktName string) { diff --git a/cmd/s3-gw/app.go b/cmd/s3-gw/app.go index 3ccc425f7..f46c39dc6 100644 --- a/cmd/s3-gw/app.go +++ b/cmd/s3-gw/app.go @@ -97,7 +97,6 @@ type ( clientCut bool maxBufferSizeForPut uint64 md5Enabled bool - aclEnabled bool namespaceHeader string defaultNamespaces []string policyDenyByDefault bool @@ -210,7 +209,6 @@ func newAppSettings(log *Logger, v *viper.Viper) *appSettings { func (s *appSettings) update(v *viper.Viper, log *zap.Logger) { s.updateNamespacesSettings(v, log) s.useDefaultXMLNamespace(v.GetBool(cfgKludgeUseDefaultXMLNS)) - s.setACLEnabled(v.GetBool(cfgKludgeACLEnabled)) s.setBypassContentEncodingInChunks(v.GetBool(cfgKludgeBypassContentEncodingCheckInChunks)) s.setClientCut(v.GetBool(cfgClientCut)) s.setBufferMaxSizeForPut(v.GetUint64(cfgBufferMaxSizeForPut)) @@ -347,18 +345,6 @@ func (s *appSettings) setMD5Enabled(md5Enabled bool) { s.mu.Unlock() } -func (s *appSettings) setACLEnabled(enableACL bool) { - s.mu.Lock() - s.aclEnabled = enableACL - s.mu.Unlock() -} - -func (s *appSettings) ACLEnabled() bool { - s.mu.RLock() - defer s.mu.RUnlock() - return s.aclEnabled -} - func (s *appSettings) NamespaceHeader() string { s.mu.RLock() defer s.mu.RUnlock() diff --git a/cmd/s3-gw/app_settings.go b/cmd/s3-gw/app_settings.go index ca20cc719..18ad39ef1 100644 --- a/cmd/s3-gw/app_settings.go +++ b/cmd/s3-gw/app_settings.go @@ -160,8 +160,6 @@ const ( // Settings. cfgKludgeUseDefaultXMLNS = "kludge.use_default_xmlns" cfgKludgeBypassContentEncodingCheckInChunks = "kludge.bypass_content_encoding_check_in_chunks" cfgKludgeDefaultNamespaces = "kludge.default_namespaces" - cfgKludgeACLEnabled = "kludge.acl_enabled" - // Web. cfgWebReadTimeout = "web.read_timeout" cfgWebReadHeaderTimeout = "web.read_header_timeout" @@ -731,7 +729,6 @@ func newSettings() *viper.Viper { v.SetDefault(cfgKludgeUseDefaultXMLNS, false) v.SetDefault(cfgKludgeBypassContentEncodingCheckInChunks, false) v.SetDefault(cfgKludgeDefaultNamespaces, defaultDefaultNamespaces) - v.SetDefault(cfgKludgeACLEnabled, false) // web v.SetDefault(cfgWebReadHeaderTimeout, defaultReadHeaderTimeout) diff --git a/config/config.env b/config/config.env index a16ecffef..69e12f56a 100644 --- a/config/config.env +++ b/config/config.env @@ -154,8 +154,6 @@ S3_GW_KLUDGE_USE_DEFAULT_XMLNS=false S3_GW_KLUDGE_BYPASS_CONTENT_ENCODING_CHECK_IN_CHUNKS=false # Namespaces that should be handled as default S3_GW_KLUDGE_DEFAULT_NAMESPACES="" "root" -# Enable bucket/object ACL support for newly created buckets. -S3_GW_KLUDGE_ACL_ENABLED=false S3_GW_TRACING_ENABLED=false S3_GW_TRACING_ENDPOINT="localhost:4318" diff --git a/config/config.yaml b/config/config.yaml index d9cc827c5..4ed6be356 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -182,8 +182,6 @@ kludge: bypass_content_encoding_check_in_chunks: false # Namespaces that should be handled as default default_namespaces: [ "", "root" ] - # Enable bucket/object ACL support for newly created buckets. - acl_enabled: false runtime: soft_memory_limit: 1gb diff --git a/docs/configuration.md b/docs/configuration.md index d25837a75..524f91bdf 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -558,7 +558,6 @@ kludge: use_default_xmlns: false bypass_content_encoding_check_in_chunks: false default_namespaces: [ "", "root" ] - acl_enabled: false ``` | Parameter | Type | SIGHUP reload | Default value | Description | @@ -566,7 +565,6 @@ kludge: | `use_default_xmlns` | `bool` | yes | `false` | Enable using default xml namespace `http://s3.amazonaws.com/doc/2006-03-01/` when parse xml bodies. | | `bypass_content_encoding_check_in_chunks` | `bool` | yes | `false` | Use this flag to be able to use [chunked upload approach](https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-streaming.html) without having `aws-chunked` value in `Content-Encoding` header. | | `default_namespaces` | `[]string` | yes | `["","root"]` | Namespaces that should be handled as default. | -| `acl_enabled` | `bool` | yes | `false` | Enable bucket/object ACL support for newly created buckets. | # `runtime` section Contains runtime parameters.