From b5be7f2d3cd68439fca92939e35fe40b39f43f43 Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Fri, 28 Mar 2025 19:28:04 +0200 Subject: [PATCH] [#6654] fixed S3 canonical uri parts escaping --- CHANGELOG.md | 5 + tools/filesystem/internal/s3blob/s3/error.go | 4 +- .../internal/s3blob/s3/error_test.go | 10 +- tools/filesystem/internal/s3blob/s3/s3.go | 112 ++++++++++++++++-- .../internal/s3blob/s3/s3_escape_test.go | 35 ++++++ .../filesystem/internal/s3blob/s3/s3_test.go | 33 +++++- tools/filesystem/internal/s3blob/s3blob.go | 8 +- .../filesystem/internal/s3blob/s3blob_test.go | 20 +++- 8 files changed, 201 insertions(+), 26 deletions(-) create mode 100644 tools/filesystem/internal/s3blob/s3/s3_escape_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f74fa3b8..f6a0e472 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## v0.26.5 + +- Fixed S3 canonical parts escaping when generating the SigV4 request signature ([#6654](https://github.com/pocketbase/pocketbase/issues/6654)). + + ## v0.26.4 - Fixed `RecordErrorEvent.Error` and `CollectionErrorEvent.Error` sync with `ModelErrorEvent.Error` ([#6639](https://github.com/pocketbase/pocketbase/issues/6639)). diff --git a/tools/filesystem/internal/s3blob/s3/error.go b/tools/filesystem/internal/s3blob/s3/error.go index e98ef7c6..e8d76af8 100644 --- a/tools/filesystem/internal/s3blob/s3/error.go +++ b/tools/filesystem/internal/s3blob/s3/error.go @@ -6,6 +6,8 @@ import ( "strings" ) +var _ error = (*ResponseError)(nil) + // ResponseError defines a general S3 response error. // // https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html @@ -20,7 +22,7 @@ type ResponseError struct { } // Error implements the std error interface. -func (err ResponseError) Error() string { +func (err *ResponseError) Error() string { var strBuilder strings.Builder strBuilder.WriteString(strconv.Itoa(err.Status)) diff --git a/tools/filesystem/internal/s3blob/s3/error_test.go b/tools/filesystem/internal/s3blob/s3/error_test.go index b4020060..4728c391 100644 --- a/tools/filesystem/internal/s3blob/s3/error_test.go +++ b/tools/filesystem/internal/s3blob/s3/error_test.go @@ -19,7 +19,7 @@ func TestResponseErrorSerialization(t *testing.T) { ` - respErr := s3.ResponseError{ + respErr := &s3.ResponseError{ Status: 123, Raw: []byte("test"), } @@ -45,17 +45,17 @@ func TestResponseErrorSerialization(t *testing.T) { func TestResponseErrorErrorInterface(t *testing.T) { scenarios := []struct { name string - err s3.ResponseError + err *s3.ResponseError expected string }{ { "empty", - s3.ResponseError{}, + &s3.ResponseError{}, "0 S3ResponseError", }, { "with code and message (nil raw)", - s3.ResponseError{ + &s3.ResponseError{ Status: 123, Code: "test_code", Message: "test_message", @@ -64,7 +64,7 @@ func TestResponseErrorErrorInterface(t *testing.T) { }, { "with code and message (non-nil raw)", - s3.ResponseError{ + &s3.ResponseError{ Status: 123, Code: "test_code", Message: "test_message", diff --git a/tools/filesystem/internal/s3blob/s3/s3.go b/tools/filesystem/internal/s3blob/s3/s3.go index eca33285..4391a10e 100644 --- a/tools/filesystem/internal/s3blob/s3/s3.go +++ b/tools/filesystem/internal/s3blob/s3/s3.go @@ -29,6 +29,7 @@ import ( "fmt" "io" "net/http" + "net/url" "slices" "strings" "time" @@ -61,7 +62,7 @@ type S3 struct { } // URL constructs an S3 request URL based on the current configuration. -func (s3 *S3) URL(key string) string { +func (s3 *S3) URL(path string) string { scheme := "https" endpoint := strings.TrimRight(s3.Endpoint, "/") if after, ok := strings.CutPrefix(endpoint, "https://"); ok { @@ -71,13 +72,13 @@ func (s3 *S3) URL(key string) string { scheme = "http" } - key = strings.TrimLeft(key, "/") + path = strings.TrimLeft(path, "/") if s3.UsePathStyle { - return fmt.Sprintf("%s://%s/%s/%s", scheme, endpoint, s3.Bucket, key) + return fmt.Sprintf("%s://%s/%s/%s", scheme, endpoint, s3.Bucket, path) } - return fmt.Sprintf("%s://%s.%s/%s", scheme, s3.Bucket, endpoint, key) + return fmt.Sprintf("%s://%s.%s/%s", scheme, s3.Bucket, endpoint, path) } // SignAndSend signs the provided request per AWS Signature v4 and sends it. @@ -150,8 +151,8 @@ func (s3 *S3) sign(req *http.Request) { canonicalParts := []string{ req.Method, - req.URL.EscapedPath(), - encodeQuery(req), + escapePath(req.URL.Path), + escapeQuery(req.URL.Query()), canonicalHeaders, signedHeaders, req.Header.Get("x-amz-content-sha256"), @@ -202,12 +203,6 @@ func (s3 *S3) sign(req *http.Request) { req.Header.Set("authorization", authorization) } -// encodeQuery encodes the request query parameters according to the AWS requirements -// (see UriEncode description in https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html). -func encodeQuery(req *http.Request) string { - return strings.ReplaceAll(req.URL.Query().Encode(), "+", "%20") -} - func sha256Hex(content []byte) string { h := sha256.New() h.Write(content) @@ -280,3 +275,96 @@ func extractMetadata(headers http.Header) map[string]string { return result } + +// escapeQuery returns the URI encoded request query parameters according to the AWS S3 spec requirements +// (it is similar to url.Values.Encode but instead of url.QueryEscape uses our own escape method). +func escapeQuery(values url.Values) string { + if len(values) == 0 { + return "" + } + + var buf strings.Builder + + keys := make([]string, 0, len(values)) + for k := range values { + keys = append(keys, k) + } + slices.Sort(keys) + + for _, k := range keys { + vs := values[k] + keyEscaped := escape(k) + for _, values := range vs { + if buf.Len() > 0 { + buf.WriteByte('&') + } + buf.WriteString(keyEscaped) + buf.WriteByte('=') + buf.WriteString(escape(values)) + } + } + + return buf.String() +} + +// escapePath returns the URI encoded request path according to the AWS S3 spec requirments. +func escapePath(path string) string { + parts := strings.Split(path, "/") + + for i, part := range parts { + parts[i] = escape(part) + } + + return strings.Join(parts, "/") +} + +const upperhex = "0123456789ABCDEF" + +// escape is similar to the std url.escape but implements the AWS [UriEncode requirements]: +// - URI encode every byte except the unreserved characters: 'A'-'Z', 'a'-'z', '0'-'9', '-', '.', '_', and '~'. +// - The space character is a reserved character and must be encoded as "%20" (and not as "+"). +// - Each URI encoded byte is formed by a '%' and the two-digit hexadecimal value of the byte. +// - Letters in the hexadecimal value must be uppercase, for example "%1A". +// +// [UriEncode requirements]: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html +func escape(s string) string { + hexCount := 0 + for i := 0; i < len(s); i++ { + c := s[i] + if shouldEscape(c) { + hexCount++ + } + } + + if hexCount == 0 { + return s + } + + result := make([]byte, len(s)+2*hexCount) + + j := 0 + for i := 0; i < len(s); i++ { + c := s[i] + if shouldEscape(c) { + result[j] = '%' + result[j+1] = upperhex[c>>4] + result[j+2] = upperhex[c&15] + j += 3 + } else { + result[j] = c + j++ + } + } + + return string(result) +} + +// > "URI encode every byte except the unreserved characters: 'A'-'Z', 'a'-'z', '0'-'9', '-', '.', '_', and '~'." +func shouldEscape(c byte) bool { + isUnreserved := (c >= 'A' && c <= 'Z') || + (c >= 'a' && c <= 'z') || + (c >= '0' && c <= '9') || + c == '-' || c == '.' || c == '_' || c == '~' + + return !isUnreserved +} diff --git a/tools/filesystem/internal/s3blob/s3/s3_escape_test.go b/tools/filesystem/internal/s3blob/s3/s3_escape_test.go new file mode 100644 index 00000000..fedc330f --- /dev/null +++ b/tools/filesystem/internal/s3blob/s3/s3_escape_test.go @@ -0,0 +1,35 @@ +package s3 + +import ( + "net/url" + "testing" +) + +func TestEscapePath(t *testing.T) { + t.Parallel() + + escaped := escapePath("/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~ !@#$%^&*()+={}[]?><\\|,`'\"/@sub1/@sub2/a/b/c/1/2/3") + + expected := "/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~%20%21%40%23%24%25%5E%26%2A%28%29%2B%3D%7B%7D%5B%5D%3F%3E%3C%5C%7C%2C%60%27%22/%40sub1/%40sub2/a/b/c/1/2/3" + + if escaped != expected { + t.Fatalf("Expected\n%s\ngot\n%s", expected, escaped) + } +} + +func TestEscapeQuery(t *testing.T) { + t.Parallel() + + escaped := escapeQuery(url.Values{ + "abc": []string{"123"}, + "/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~ !@#$%^&*()+={}[]?><\\|,`'\"": []string{ + "/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~ !@#$%^&*()+={}[]?><\\|,`'\"", + }, + }) + + expected := "%2FABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~%20%21%40%23%24%25%5E%26%2A%28%29%2B%3D%7B%7D%5B%5D%3F%3E%3C%5C%7C%2C%60%27%22=%2FABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~%20%21%40%23%24%25%5E%26%2A%28%29%2B%3D%7B%7D%5B%5D%3F%3E%3C%5C%7C%2C%60%27%22&abc=123" + + if escaped != expected { + t.Fatalf("Expected\n%s\ngot\n%s", expected, escaped) + } +} diff --git a/tools/filesystem/internal/s3blob/s3/s3_test.go b/tools/filesystem/internal/s3blob/s3/s3_test.go index b17a788f..c2d76592 100644 --- a/tools/filesystem/internal/s3blob/s3/s3_test.go +++ b/tools/filesystem/internal/s3blob/s3/s3_test.go @@ -98,11 +98,13 @@ func TestS3SignAndSend(t *testing.T) { scenarios := []struct { name string + path string reqFunc func(req *http.Request) s3Client *s3.S3 }{ { "minimal", + "/test", func(req *http.Request) { req.Header.Set("x-amz-date", "20250102T150405Z") }, @@ -129,6 +131,7 @@ func TestS3SignAndSend(t *testing.T) { }, { "minimal with different access and secret keys", + "/test", func(req *http.Request) { req.Header.Set("x-amz-date", "20250102T150405Z") }, @@ -153,8 +156,36 @@ func TestS3SignAndSend(t *testing.T) { }), }, }, + { + "minimal with special characters", + "/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~!@#$^&*()=/@sub?a=1&@b=@2", + func(req *http.Request) { + req.Header.Set("x-amz-date", "20250102T150405Z") + }, + &s3.S3{ + Region: "test_region", + Bucket: "test_bucket", + Endpoint: "https://example.com/", + AccessKey: "456", + SecretKey: "def", + Client: tests.NewClient(&tests.RequestStub{ + Method: http.MethodGet, + URL: "https://test_bucket.example.com/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~!@#$%5E&*()=/@sub?a=1&@b=@2", + Response: testResponse(), + Match: func(req *http.Request) bool { + return tests.ExpectHeaders(req.Header, map[string]string{ + "Authorization": "AWS4-HMAC-SHA256 Credential=456/20250102/test_region/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=e0001982deef1652704f74503203e77d83d4c88369421f9fca644d96f2a62a3c", + "Host": "test_bucket.example.com", + "X-Amz-Content-Sha256": "UNSIGNED-PAYLOAD", + "X-Amz-Date": "20250102T150405Z", + }) + }, + }), + }, + }, { "with extra headers", + "/test", func(req *http.Request) { req.Header.Set("x-amz-date", "20250102T150405Z") req.Header.Set("x-amz-content-sha256", "test_sha256") @@ -191,7 +222,7 @@ func TestS3SignAndSend(t *testing.T) { for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { - req, err := http.NewRequest(http.MethodGet, s.s3Client.URL("/test"), strings.NewReader("test_request")) + req, err := http.NewRequest(http.MethodGet, s.s3Client.URL(s.path), strings.NewReader("test_request")) if err != nil { t.Fatal(err) } diff --git a/tools/filesystem/internal/s3blob/s3blob.go b/tools/filesystem/internal/s3blob/s3blob.go index db2ed952..ceaebd11 100644 --- a/tools/filesystem/internal/s3blob/s3blob.go +++ b/tools/filesystem/internal/s3blob/s3blob.go @@ -79,9 +79,13 @@ func (drv *driver) NormalizeError(err error) error { return err } - // normalize base on its S3 error code - var ae s3.ResponseError + // normalize base on its S3 error status or code + var ae *s3.ResponseError if errors.As(err, &ae) { + if ae.Status == 404 { + return errors.Join(err, blob.ErrNotFound) + } + switch ae.Code { case "NoSuchBucket", "NoSuchKey", "NotFound": return errors.Join(err, blob.ErrNotFound) diff --git a/tools/filesystem/internal/s3blob/s3blob_test.go b/tools/filesystem/internal/s3blob/s3blob_test.go index 9b0e8f18..20d0f3b7 100644 --- a/tools/filesystem/internal/s3blob/s3blob_test.go +++ b/tools/filesystem/internal/s3blob/s3blob_test.go @@ -99,29 +99,39 @@ func TestDriverNormilizeError(t *testing.T) { errors.New("test"), false, }, + { + "response error with only status (non-404)", + &s3.ResponseError{Status: 123}, + false, + }, + { + "response error with only status (404)", + &s3.ResponseError{Status: 404}, + true, + }, { "response error with custom code", - s3.ResponseError{Code: "test"}, + &s3.ResponseError{Code: "test"}, false, }, { "response error with NoSuchBucket code", - s3.ResponseError{Code: "NoSuchBucket"}, + &s3.ResponseError{Code: "NoSuchBucket"}, true, }, { "response error with NoSuchKey code", - s3.ResponseError{Code: "NoSuchKey"}, + &s3.ResponseError{Code: "NoSuchKey"}, true, }, { "response error with NotFound code", - s3.ResponseError{Code: "NotFound"}, + &s3.ResponseError{Code: "NotFound"}, true, }, { "wrapped response error with NotFound code", // ensures that the entire error's tree is checked - fmt.Errorf("test: %w", s3.ResponseError{Code: "NotFound"}), + fmt.Errorf("test: %w", &s3.ResponseError{Code: "NotFound"}), true, }, {