[#562] Support TLS termination header for SSE-C #566

Merged
alexvanin merged 2 commits from r.loginov/frostfs-s3-gw:feature/562-support_tls_termination into master 2024-12-11 14:16:55 +00:00
Member

close #562

Added the ability to specify a header that is used to indicate that TLS has been completed at the proxy level.
The default header name is X-Frostfs-TLS-Termination.

close #562 Added the ability to specify a header that is used to indicate that TLS has been completed at the proxy level. The default header name is `X-Frostfs-TLS-Termination`.
r.loginov self-assigned this 2024-12-03 12:52:36 +00:00
r.loginov added 1 commit 2024-12-03 12:52:37 +00:00
[#562] Support TLS termination header for SSE-C
All checks were successful
/ DCO (pull_request) Successful in 2m6s
/ Vulncheck (pull_request) Successful in 2m20s
/ Builds (pull_request) Successful in 2m4s
/ Lint (pull_request) Successful in 3m8s
/ Tests (pull_request) Successful in 2m6s
56025dab9f
The TLS termination header added 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 SSE-C.

Signed-off-by: Roman Loginov <r.loginov@yadro.com>
r.loginov requested review from alexvanin 2024-12-03 12:52:37 +00:00
r.loginov requested review from dkirillov 2024-12-03 12:52:37 +00:00
r.loginov removed review request for dkirillov 2024-12-03 12:52:43 +00:00
r.loginov removed review request for alexvanin 2024-12-03 12:52:44 +00:00
r.loginov reviewed 2024-12-03 13:03:42 +00:00
@ -362,3 +362,3 @@
}
func formEncryptionParamsBase(r *http.Request, isCopySource bool) (enc encryption.Params, err error) {
func (h *handler) formEncryptionParamsBase(r *http.Request, isCopySource bool) (enc encryption.Params, err error) {
Author
Member

question: At the moment, I have not found any tests for the formEncryptionParamsBase function. Should I add them?

question: At the moment, I have not found any tests for the `formEncryptionParamsBase` function. Should I add them?
Owner

Any correct tests are appreciated :)

Any correct tests are appreciated :)
r.loginov marked this conversation as resolved
@ -381,0 +381,4 @@
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.Error(err))
Author
Member

I'm not completely sure that you should just output a warning here, maybe you should return an error here after all.
On the one hand, there is an incorrect header from outside. On the other hand, this is not an SEC error in the context of AWS S3.

I'm not completely sure that you should just output a warning here, maybe you should return an error here after all. On the one hand, there is an incorrect header from outside. On the other hand, this is not an SEC error in the context of AWS S3.
Member

I suppose warning is enough. But can we also log tlsTerminationStr?

I suppose warning is enough. But can we also log `tlsTerminationStr`?
dkirillov marked this conversation as resolved
r.loginov requested review from alexvanin 2024-12-03 13:04:52 +00:00
r.loginov requested review from dkirillov 2024-12-03 13:04:53 +00:00
r.loginov requested review from pogpp 2024-12-03 13:04:55 +00:00
r.loginov requested review from mbiryukova 2024-12-03 13:04:56 +00:00
r.loginov requested review from nzinkevich 2024-12-03 13:04:57 +00:00
r.loginov force-pushed feature/562-support_tls_termination from 56025dab9f to dea857cda8 2024-12-04 10:53:08 +00:00 Compare
dkirillov approved these changes 2024-12-04 13:31:34 +00:00
Dismissed
dkirillov left a comment
Member

LGTM

LGTM
@ -196,6 +196,7 @@ There are some custom types used for brevity:
| `containers` | [Containers configuration](#containers-section) |
| `vhs` | [VHS configuration](#vhs-section) |
| `multinet` | [Multinet configuration](#multinet-section) |
| `sse_c` | [SSE-C configuration](#sse_c-section) |
Member

I would use different section name. Maybe encryption or tls something like that. sse_c name too specific I suppose

I would use different section name. Maybe `encryption` or `tls` something like that. `sse_c` name too specific I suppose
dkirillov marked this conversation as resolved
r.loginov force-pushed feature/562-support_tls_termination from dea857cda8 to efde9fe2e7 2024-12-04 15:01:47 +00:00 Compare
r.loginov dismissed dkirillov's review 2024-12-04 15:01:47 +00:00
Reason:

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

dkirillov approved these changes 2024-12-06 06:31:32 +00:00
Dismissed
mbiryukova approved these changes 2024-12-06 10:31:58 +00:00
Dismissed
pogpp approved these changes 2024-12-06 13:41:00 +00:00
Dismissed
nzinkevich reviewed 2024-12-09 06:36:54 +00:00
@ -637,0 +789,4 @@
}
}
func prepareRequestForEnctyption(algo, key, md5, tlsTermination string, reqWithoutTLS, reqWithoutSSE, isCopySource bool) *http.Request {
Member

nitpick: typo in Encryption

nitpick: typo in `Encryption`
r.loginov marked this conversation as resolved
r.loginov force-pushed feature/562-support_tls_termination from efde9fe2e7 to d6c451c782 2024-12-09 13:05:00 +00:00 Compare
r.loginov dismissed dkirillov's review 2024-12-09 13:05:01 +00:00
Reason:

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

r.loginov dismissed mbiryukova's review 2024-12-09 13:05:01 +00:00
Reason:

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

r.loginov dismissed pogpp's review 2024-12-09 13:05:01 +00:00
Reason:

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

alexvanin added this to the v0.32.0 milestone 2024-12-11 07:39:45 +00:00
dkirillov approved these changes 2024-12-11 08:10:12 +00:00
Dismissed
r.loginov force-pushed feature/562-support_tls_termination from d6c451c782 to 128939c01e 2024-12-11 13:11:07 +00:00 Compare
r.loginov dismissed dkirillov's review 2024-12-11 13:11:07 +00:00
Reason:

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

alexvanin approved these changes 2024-12-11 14:16:43 +00:00
alexvanin merged commit 128939c01e into master 2024-12-11 14:16:55 +00:00
alexvanin deleted branch feature/562-support_tls_termination 2024-12-11 14:17:00 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
6 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#566
No description provided.