[#562] Empty default value for TLS termination header param #575

r.loginov · 2024-12-11T16:07:04Z

r.loginov commented

2024-12-11 16:07:04 +00:00

close #562

For additional information see #562 (comment)

close #562 For additional information see https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/issues/562#issuecomment-61212

r.loginov self-assigned this 2024-12-11 16:07:04 +00:00

requested reviews from storage-services-committers, storage-services-developers

2024-12-11 16:07:04 +00:00

r.loginov added this to the v0.32.0 milestone 2024-12-11 16:12:49 +00:00

r.loginov force-pushed fix/562-use_empty_default_value_for_tls_termination_header from 4f7bd5c149 to b5a3ebed69

2024-12-11 16:18:53 +00:00

Compare

mbiryukova approved these changes 2024-12-12 03:30:43 +00:00

Dismissed

dkirillov reviewed 2024-12-12 06:15:00 +00:00

api/handler/put.go Outdated

					
				@ -445,6 +440,19 @@ func (h *handler) formEncryptionParamsBase(r *http.Request, isCopySource bool) (

					return enc, err

				}

				func (h *handler) isTLSCheckRequired(r *http.Request, needCheckTLS bool) bool {

dkirillov commented

2024-12-12 06:15:00 +00:00

Suggestion: can we write something like this:

diff --git a/api/handler/put.go b/api/handler/put.go
index 1295fdf2..bebc493d 100644
--- a/api/handler/put.go
+++ b/api/handler/put.go
@@ -387,12 +387,7 @@ func (h *handler) formEncryptionParamsBase(r *http.Request, isCopySource bool) (
 		return
 	}
 
-	needCheckTLS := true
-	if headerKey := h.cfg.TLSTerminationHeader(); headerKey != "" {
-		needCheckTLS = h.isTLSCheckRequired(r, needCheckTLS)
-	}
-
-	if needCheckTLS && r.TLS == nil {
+	if h.isTLSCheckRequired(r) && r.TLS == nil {
 		return enc, apierr.GetAPIError(apierr.ErrInsecureSSECustomerRequest)
 	}
 
@@ -440,17 +435,24 @@ func (h *handler) formEncryptionParamsBase(r *http.Request, isCopySource bool) (
 	return enc, err
 }
 
-func (h *handler) isTLSCheckRequired(r *http.Request, needCheckTLS bool) bool {
-	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
-		}
+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 needCheckTLS
+	return !tlsTermination
 }
 
 func (h *handler) PostObject(w http.ResponseWriter, r *http.Request) {

Suggestion: can we write something like this: ```diff diff --git a/api/handler/put.go b/api/handler/put.go index 1295fdf2..bebc493d 100644 --- a/api/handler/put.go +++ b/api/handler/put.go @@ -387,12 +387,7 @@ func (h *handler) formEncryptionParamsBase(r *http.Request, isCopySource bool) ( return } - needCheckTLS := true - if headerKey := h.cfg.TLSTerminationHeader(); headerKey != "" { - needCheckTLS = h.isTLSCheckRequired(r, needCheckTLS) - } - - if needCheckTLS && r.TLS == nil { + if h.isTLSCheckRequired(r) && r.TLS == nil { return enc, apierr.GetAPIError(apierr.ErrInsecureSSECustomerRequest) } @@ -440,17 +435,24 @@ func (h *handler) formEncryptionParamsBase(r *http.Request, isCopySource bool) ( return enc, err } -func (h *handler) isTLSCheckRequired(r *http.Request, needCheckTLS bool) bool { - 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 - } +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 needCheckTLS + return !tlsTermination } func (h *handler) PostObject(w http.ResponseWriter, r *http.Request) { ```

👍 1

dkirillov marked this conversation as resolved

dkirillov approved these changes 2024-12-12 06:17:35 +00:00

Dismissed

alexvanin approved these changes 2024-12-12 06:42:25 +00:00

Dismissed

alexvanin left a comment

LGTM, I would like to apply suggestion from @dkirillov, it makes code a little bit easier to read. The rule of thumb for me is: if you see else statement, check if you can rewrite without it :)

LGTM, I would like to apply suggestion from @dkirillov, it makes code a little bit easier to read. The rule of thumb for me is: if you see `else` statement, check if you can rewrite without it :)

👍 1

r.loginov force-pushed fix/562-use_empty_default_value_for_tls_termination_header from b5a3ebed69 to b83e6e1258

2024-12-12 07:12:14 +00:00

Compare

r.loginov dismissed mbiryukova's review 2024-12-12 07:12:14 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

r.loginov dismissed dkirillov's review 2024-12-12 07:12:14 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

r.loginov dismissed alexvanin's review 2024-12-12 07:12:14 +00:00

Reason: