From 43abf58068b916766f974b43fb57e0f3cd21f953 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 5 Dec 2023 15:49:13 +0300 Subject: [PATCH] [#257] Support flag to deny access if policy rules not found Signed-off-by: Denis Kirillov --- api/middleware/policy.go | 3 ++- api/router_mock_test.go | 8 +++++++- api/router_test.go | 32 +++++++++++++++++++++++++++----- cmd/s3-gw/app.go | 15 +++++++++++++++ cmd/s3-gw/app_settings.go | 3 ++- config/config.env | 2 ++ config/config.yaml | 3 +++ docs/configuration.md | 9 ++++++--- 8 files changed, 64 insertions(+), 11 deletions(-) diff --git a/api/middleware/policy.go b/api/middleware/policy.go index f47613e4b..c6a34e50a 100644 --- a/api/middleware/policy.go +++ b/api/middleware/policy.go @@ -18,6 +18,7 @@ import ( type PolicySettings interface { ResolveNamespaceAlias(ns string) string + PolicyDenyByDefault() bool } func PolicyCheck(storage engine.ChainRouter, settings PolicySettings, domains []string, log *zap.Logger) Func { @@ -27,7 +28,7 @@ func PolicyCheck(storage engine.ChainRouter, settings PolicySettings, domains [] st, err := policyCheck(storage, settings, domains, r) if err == nil { - if st != chain.Allow && st != chain.NoRuleFound { // todo drop 'st != chain.NoRuleFound' + if st != chain.Allow && (st != chain.NoRuleFound || settings.PolicyDenyByDefault()) { err = apiErr.GetAPIErrorWithError(apiErr.ErrAccessDenied, fmt.Errorf("policy check: %s", st.String())) } } diff --git a/api/router_mock_test.go b/api/router_mock_test.go index 870c46794..2fc964705 100644 --- a/api/router_mock_test.go +++ b/api/router_mock_test.go @@ -20,7 +20,9 @@ func (c *centerMock) Authenticate(*http.Request) (*middleware.Box, error) { return &middleware.Box{}, nil } -type middlewareSettingsMock struct{} +type middlewareSettingsMock struct { + denyByDefault bool +} func (r *middlewareSettingsMock) NamespaceHeader() string { return FrostfsNamespaceHeader @@ -30,6 +32,10 @@ func (r *middlewareSettingsMock) ResolveNamespaceAlias(ns string) string { return ns } +func (r *middlewareSettingsMock) PolicyDenyByDefault() bool { + return r.denyByDefault +} + type handlerMock struct { t *testing.T } diff --git a/api/router_test.go b/api/router_test.go index a331e5922..56e03d44a 100644 --- a/api/router_test.go +++ b/api/router_test.go @@ -24,8 +24,9 @@ import ( ) type routerMock struct { - router *chi.Mux - cfg Config + router *chi.Mux + cfg Config + middlewareSettings *middlewareSettingsMock } func (m *routerMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -33,6 +34,8 @@ func (m *routerMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { } func prepareRouter(t *testing.T) *routerMock { + middlewareSettings := &middlewareSettingsMock{} + cfg := Config{ Throttle: middleware.ThrottleOpts{ Limit: 10, @@ -42,12 +45,13 @@ func prepareRouter(t *testing.T) *routerMock { Center: ¢erMock{}, Log: zaptest.NewLogger(t), Metrics: &metrics.AppMetrics{}, - MiddlewareSettings: &middlewareSettingsMock{}, + MiddlewareSettings: middlewareSettings, PolicyStorage: inmemory.NewInMemoryLocalOverrides(), } return &routerMock{ - router: NewRouter(cfg), - cfg: cfg, + router: NewRouter(cfg), + cfg: cfg, + middlewareSettings: middlewareSettings, } } @@ -183,6 +187,24 @@ func TestPolicyChecker(t *testing.T) { assertAPIError(t, w, apiErrors.ErrAccessDenied) } +func TestDefaultBehaviorPolicyChecker(t *testing.T) { + chiRouter := prepareRouter(t) + bktName, objName := "bucket", "object" + target := fmt.Sprintf("/%s/%s", bktName, objName) + + // check we can access bucket if rules not found + w, r := httptest.NewRecorder(), httptest.NewRequest(http.MethodPut, target, nil) + chiRouter.ServeHTTP(w, r) + resp := readResponse(t, w) + require.Equal(t, s3middleware.PutObjectOperation, resp.Method) + + // check we cannot access if rules not found when settings is enabled + chiRouter.middlewareSettings.denyByDefault = true + w, r = httptest.NewRecorder(), httptest.NewRequest(http.MethodPut, target, nil) + chiRouter.ServeHTTP(w, r) + assertAPIError(t, w, apiErrors.ErrAccessDenied) +} + func readResponse(t *testing.T, w *httptest.ResponseRecorder) handlerResult { var res handlerResult diff --git a/cmd/s3-gw/app.go b/cmd/s3-gw/app.go index e739644c3..5426b1997 100644 --- a/cmd/s3-gw/app.go +++ b/cmd/s3-gw/app.go @@ -102,6 +102,7 @@ type ( namespaceHeader string defaultNamespaces []string authorizedControlAPIKeys [][]byte + policyDenyByDefault bool } maxClientsConfig struct { @@ -212,6 +213,7 @@ func newAppSettings(log *Logger, v *viper.Viper, key *keys.PrivateKey) *appSetti settings.setBufferMaxSizeForPut(v.GetUint64(cfgBufferMaxSizeForPut)) settings.setMD5Enabled(v.GetBool(cfgMD5Enabled)) settings.setAuthorizedControlAPIKeys(append(fetchAuthorizedKeys(log.logger, v), key.PublicKey())) + settings.setPolicyDenyByDefault(v.GetBool(cfgPolicyDenyByDefault)) return settings } @@ -397,6 +399,18 @@ func (s *appSettings) ResolveNamespaceAlias(namespace string) string { return namespace } +func (s *appSettings) PolicyDenyByDefault() bool { + s.mu.RLock() + defer s.mu.RUnlock() + return s.policyDenyByDefault +} + +func (s *appSettings) setPolicyDenyByDefault(policyDenyByDefault bool) { + s.mu.Lock() + s.policyDenyByDefault = policyDenyByDefault + s.mu.Unlock() +} + func (a *App) initAPI(ctx context.Context) { a.initLayer(ctx) a.initHandler() @@ -775,6 +789,7 @@ func (a *App) updateSettings() { a.settings.setMD5Enabled(a.cfg.GetBool(cfgMD5Enabled)) a.settings.setDefaultNamespaces(a.cfg.GetStringSlice(cfgKludgeDefaultNamespaces)) a.settings.setAuthorizedControlAPIKeys(append(fetchAuthorizedKeys(a.log, a.cfg), a.key.PublicKey())) + a.settings.setPolicyDenyByDefault(a.cfg.GetBool(cfgPolicyDenyByDefault)) } func (a *App) startServices() { diff --git a/cmd/s3-gw/app_settings.go b/cmd/s3-gw/app_settings.go index 940e0b259..b5fb05a9d 100644 --- a/cmd/s3-gw/app_settings.go +++ b/cmd/s3-gw/app_settings.go @@ -198,7 +198,8 @@ const ( // Settings. cfgSoftMemoryLimit = "runtime.soft_memory_limit" // Enable return MD5 checksum in ETag. - cfgMD5Enabled = "features.md5.enabled" + cfgMD5Enabled = "features.md5.enabled" + cfgPolicyDenyByDefault = "features.policy.deny_by_default" // FrostfsID. cfgFrostfsIDEnabled = "frostfsid.enabled" diff --git a/config/config.env b/config/config.env index d44d16805..9c52d892a 100644 --- a/config/config.env +++ b/config/config.env @@ -160,6 +160,8 @@ S3_GW_TRACING_EXPORTER="otlp_grpc" S3_GW_RUNTIME_SOFT_MEMORY_LIMIT=1073741824 S3_GW_FEATURES_MD5_ENABLED=false +# Enable denying access for request that doesn't match any policy chain rules. +S3_GW_FEATURES_POLICY_DENY_BY_DEFAULT=false # ReadTimeout is the maximum duration for reading the entire # request, including the body. A zero or negative value means diff --git a/config/config.yaml b/config/config.yaml index 421d937c0..776e1c04f 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -189,6 +189,9 @@ runtime: soft_memory_limit: 1gb features: + policy: + # Enable denying access for request that doesn't match any policy chain rules. + deny_by_default: false md5: enabled: false diff --git a/docs/configuration.md b/docs/configuration.md index c9c0baeef..337038438 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -595,13 +595,16 @@ Contains parameters for enabling features. ```yaml features: + policy: + deny_by_default: false md5: enabled: false ``` -| Parameter | Type | SIGHUP reload | Default value | Description | -|---------------|--------|---------------|---------------|----------------------------------------------------------------| -| `md5.enabled` | `bool` | yes | false | Flag to enable return MD5 checksum in ETag headers and fields. | +| Parameter | Type | SIGHUP reload | Default value | Description | +|--------------------------|--------|---------------|---------------|------------------------------------------------------------------------------| +| `md5.enabled` | `bool` | yes | false | Flag to enable return MD5 checksum in ETag headers and fields. | +| `policy.deny_by_default` | `bool` | yes | false | Enable denying access for request that doesn't match any policy chain rules. | # `web` section Contains web server configuration parameters.