From 04b8fc2b5f1f118306f40cc60b3a575e30987585 Mon Sep 17 00:00:00 2001 From: Roman Loginov Date: Wed, 11 Dec 2024 18:57:02 +0300 Subject: [PATCH] [#562] Empty default value for TLS termination header param If the service is accessed not through a proxy and the default value of the parameter with the header key is not empty, then the system administrator does not control disabling TLS verification in any way, because the client can simply add a known header, thereby skipping the verification. Therefore, the default value of the header parameter is made empty. If it is empty, then TLS verification cannot be disabled in any way. Thus, the system administrator will be able to control the enabling/disabling of TLS. Signed-off-by: Roman Loginov --- api/handler/handlers_test.go | 3 ++- api/handler/put.go | 32 +++++++++++++++++++++----------- api/handler/put_test.go | 11 ++++++----- cmd/s3-gw/app_settings.go | 10 +++------- docs/configuration.md | 6 +++--- 5 files changed, 35 insertions(+), 27 deletions(-) diff --git a/api/handler/handlers_test.go b/api/handler/handlers_test.go index 42db0eb0..b4aa87ae 100644 --- a/api/handler/handlers_test.go +++ b/api/handler/handlers_test.go @@ -78,6 +78,7 @@ type configMock struct { defaultCopiesNumbers []uint32 bypassContentEncodingInChunks bool md5Enabled bool + tlsTerminationHeader string } func (c *configMock) DefaultPlacementPolicy(_ string) netmap.PlacementPolicy { @@ -142,7 +143,7 @@ func (c *configMock) RetryStrategy() RetryStrategy { } func (c *configMock) TLSTerminationHeader() string { - return "X-Frostfs-TLS-Termination" + return c.tlsTerminationHeader } func prepareHandlerContext(t *testing.T) *handlerContext { diff --git a/api/handler/put.go b/api/handler/put.go index 68dbffa3..bebc493d 100644 --- a/api/handler/put.go +++ b/api/handler/put.go @@ -387,17 +387,7 @@ func (h *handler) formEncryptionParamsBase(r *http.Request, isCopySource bool) ( return } - needCheckTLS := true - if tlsTerminationStr := r.Header.Get(h.cfg.TLSTerminationHeader()); len(tlsTerminationStr) > 0 { - tlsTermination, err := strconv.ParseBool(tlsTerminationStr) - if err != nil { - h.reqLogger(r.Context()).Warn(logs.WarnInvalidTypeTLSTerminationHeader, zap.String("header", tlsTerminationStr), zap.Error(err)) - } else { - needCheckTLS = !tlsTermination - } - } - - if needCheckTLS && r.TLS == nil { + if h.isTLSCheckRequired(r) && r.TLS == nil { return enc, apierr.GetAPIError(apierr.ErrInsecureSSECustomerRequest) } @@ -445,6 +435,26 @@ func (h *handler) formEncryptionParamsBase(r *http.Request, isCopySource bool) ( return enc, err } +func (h *handler) isTLSCheckRequired(r *http.Request) bool { + header := h.cfg.TLSTerminationHeader() + if header == "" { + return true + } + + tlsTerminationStr := r.Header.Get(header) + if tlsTerminationStr == "" { + return true + } + + tlsTermination, err := strconv.ParseBool(tlsTerminationStr) + if err != nil { + h.reqLogger(r.Context()).Warn(logs.WarnInvalidTypeTLSTerminationHeader, zap.String("header", tlsTerminationStr), zap.Error(err)) + return true + } + + return !tlsTermination +} + func (h *handler) PostObject(w http.ResponseWriter, r *http.Request) { var ( tagSet map[string]string diff --git a/api/handler/put_test.go b/api/handler/put_test.go index d06fee75..53968a62 100644 --- a/api/handler/put_test.go +++ b/api/handler/put_test.go @@ -636,8 +636,6 @@ func TestPutObjectWithContentLanguage(t *testing.T) { } func TestFormEncryptionParamsBase(t *testing.T) { - hc := prepareHandlerContext(t) - userSecret := "test1customer2secret3with32char4" expectedEncKey := []byte(userSecret) emptyEncKey := []byte(nil) @@ -770,7 +768,8 @@ func TestFormEncryptionParamsBase(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - r := prepareRequestForEncryption(tc.algo, tc.key, tc.md5, tc.tlsTermination, tc.reqWithoutTLS, tc.reqWithoutSSE, tc.isCopySource) + hc := prepareHandlerContext(t) + r := prepareRequestForEncryption(hc, tc.algo, tc.key, tc.md5, tc.tlsTermination, tc.reqWithoutTLS, tc.reqWithoutSSE, tc.isCopySource) enc, err := hc.h.formEncryptionParamsBase(r, tc.isCopySource) if tc.err != nil { @@ -789,7 +788,7 @@ func TestFormEncryptionParamsBase(t *testing.T) { } } -func prepareRequestForEncryption(algo, key, md5, tlsTermination string, reqWithoutTLS, reqWithoutSSE, isCopySource bool) *http.Request { +func prepareRequestForEncryption(hc *handlerContext, algo, key, md5, tlsTermination string, reqWithoutTLS, reqWithoutSSE, isCopySource bool) *http.Request { r := httptest.NewRequest(http.MethodPost, "/", nil) if !reqWithoutTLS { @@ -808,8 +807,10 @@ func prepareRequestForEncryption(algo, key, md5, tlsTermination string, reqWitho } } + customHeader := "X-Frostfs-TLS-Termination" if tlsTermination != "" { - r.Header.Set("X-Frostfs-TLS-Termination", tlsTermination) + hc.config.tlsTerminationHeader = customHeader + r.Header.Set(customHeader, tlsTermination) } return r diff --git a/cmd/s3-gw/app_settings.go b/cmd/s3-gw/app_settings.go index 87414147..fc6fd02d 100644 --- a/cmd/s3-gw/app_settings.go +++ b/cmd/s3-gw/app_settings.go @@ -59,10 +59,9 @@ const ( defaultAccessBoxCacheRemovingCheckInterval = 5 * time.Minute - defaultNamespaceHeader = "X-Frostfs-Namespace" - defaultVHSHeader = "X-Frostfs-S3-VHS" - defaultServernameHeader = "X-Frostfs-Servername" - defaultTLSTerminationHeader = "X-Frostfs-TLS-Termination" + defaultNamespaceHeader = "X-Frostfs-Namespace" + defaultVHSHeader = "X-Frostfs-S3-VHS" + defaultServernameHeader = "X-Frostfs-Servername" defaultMultinetFallbackDelay = 300 * time.Millisecond @@ -950,9 +949,6 @@ func newSettings() *viper.Viper { // multinet v.SetDefault(cfgMultinetFallbackDelay, defaultMultinetFallbackDelay) - // encryption - v.SetDefault(cfgEncryptionTLSTerminationHeader, defaultTLSTerminationHeader) - // Bind flags if err := bindFlags(v, flags); err != nil { panic(fmt.Errorf("bind flags: %w", err)) diff --git a/docs/configuration.md b/docs/configuration.md index 24f90e6d..77cf4dc5 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -869,6 +869,6 @@ encryption: tls_termination_header: X-Frostfs-TLS-Termination ``` -| Parameter | Type | SIGHUP reload | Default value | Description | -|--------------------------|----------|---------------|-----------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `tls_termination_header` | `string` | yes | `X-Frostfs-TLS-Termination` | The header for determining whether TLS needs to be checked. If the system requests come through a proxy server and TLS can terminate at the proxy level, you should use this header to disable TLS verification at server-side encryption. | +| Parameter | Type | SIGHUP reload | Default value | Description | +|--------------------------|----------|---------------|---------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `tls_termination_header` | `string` | yes | | The header for determining whether TLS needs to be checked. If the system requests come through a proxy server and TLS can terminate at the proxy level, you should use this header to disable TLS verification at server-side encryption. If the header is not specified or an empty string is set as the value, TLS will always be checked. |