add compatibility with CEPH S3 #10
Conversation
remove white space on authorization header compare
handler.go
Outdated
| @@ -225,9 +225,12 @@ func (h *Handler) buildUpstreamRequest(req *http.Request) (*http.Request, error) | |||
| return nil, err | |||
| } | |||
|
|
|||
| //Sanitize fakeReq to remove some white spaces | |||
| fakeAuthorizationStr := strings.Replace(strings.Replace(fakeReq.Header["Authorization"][0],", Signature",",Signature",1),", SignedHeaders",",SignedHeaders",1); | |||
There was a problem hiding this comment.
for improved readability: please split this into one replace operation per line of code
Also: please add tests to validate the new and old behaviour.
There was a problem hiding this comment.
it 's a dirty workaround because s3cmd send the authorization header without this white spaces... s3cmd work's well with amazon s3. So, do you have any advice about the way to validate it in a test ?
There was a problem hiding this comment.
You could compute a valid signature and then simply add/remove whitespace as you need to make s3cmd happy.
See https://github.com/Kriechi/aws-s3-reverse-proxy/blob/master/handler_test.go#L47 and https://github.com/Kriechi/aws-s3-reverse-proxy/blob/master/handler_test.go#L136 for reference.
There was a problem hiding this comment.
okay i will add some tests to validate this
| @@ -17,7 +17,7 @@ import ( | |||
| log "github.com/sirupsen/logrus" | |||
| ) | |||
|
|
|||
| var awsAuthorizationCredentialRegexp = regexp.MustCompile("Credential=([a-zA-Z0-9]+)/[0-9]+/([a-z]+-?[a-z]+-?[0-9]+)/s3/aws4_request") | |||
| var awsAuthorizationCredentialRegexp = regexp.MustCompile("Credential=([a-zA-Z0-9]+)/[0-9]+/([a-zA-Z-0-9]+)/s3/aws4_request") | |||
There was a problem hiding this comment.
please add a code comment with sample values for the new less-strict regex.
There was a problem hiding this comment.
ok i will refactor this and add comments
add comments on less restrictive region regexp add /health path on metrics httpserve
|
go test passed with s3cmd compatibility |
handler_test.go
Outdated
| authorizationReq := req.Header.Get("Authorization"); | ||
| authorizationReq = strings.Replace(authorizationReq,", Signature",",Signature",1) | ||
| authorizationReq = strings.Replace(authorizationReq,", SignedHeaders",",SignedHeaders",1) | ||
| req.Header.Set("Authorization",authorizationReq); |
There was a problem hiding this comment.
please run go fmt or similar. The whitespace around the commas inside a string and as function argument is confusing to read + also fix indentation.
main.go
Outdated
| @@ -136,6 +139,8 @@ func main() { | |||
| var wrappedHandler http.Handler = handler | |||
| if len(opts.MetricsListenAddr) > 0 && len(strings.Split(opts.MetricsListenAddr, ":")) == 2 { | |||
| metricsHandler := http.NewServeMux() | |||
| //add health on metrics http to serve k8s liveness | |||
| metricsHandler.HandleFunc("/health",health) | |||
There was a problem hiding this comment.
unrelated changes - please send as separate PR - and address indentation + formatting.
refactor indentations
handler.go
Outdated
| fakeAuthorizationStr := fakeReq.Header.Get("Authorization") | ||
| // Sanitize fakeReq to remove white spaces before the comma signature | ||
| authorizationStr := strings.Replace(req.Header["Authorization"][0], ",Signature", ", Signature", 1) | ||
| // Sanitize fakeReq to remove white spaces before the comma signheaders |
There was a problem hiding this comment.
is this comment in the correct direction?
the strings.Replace function call is "adding" a space if it is missing. Or am I reading this wrong?
|
In the next commit i ve reverse this feature to add space if it s missing . I ve rewrote this part to pass the go test.
But you re now right, it add a space when it s missing.
…
Le 9 mars 2025 à 16:43, Thomas Kriechbaumer ***@***.***> a écrit :
@Kriechi commented on this pull request.
In handler.go:
> @@ -225,9 +228,16 @@ func (h *Handler) buildUpstreamRequest(req *http.Request) (*http.Request, error) return nil, err } + // WORKAROUND S3CMD which dont use white space before the some commas in the authorization header + fakeAuthorizationStr := fakeReq.Header.Get("Authorization") + // Sanitize fakeReq to remove white spaces before the comma signature + authorizationStr := strings.Replace(req.Header["Authorization"][0], ",Signature", ", Signature", 1) + // Sanitize fakeReq to remove white spaces before the comma signheaders
is this comment in the correct direction?
the strings.Replace function call is "adding" a space if it is missing. Or am I reading this wrong?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
@Kriechi Hi, if you have few minutes, can you review this PR ? i have some more feature depending on this one to propose ... in the next draft PR i 've rewrote this part to only check the signature part in order to allow presign url. |
permit alternative region
remove white space on authorization header compare