createPresignedHttpRequestFromCtx() should escape both path and query args

[zhucaifeng81]

Describe the bug createPresignedHttpRequestFromCtx() in s3api/utils/utils.go does not generate a canonical URL, and presigned url check will fail for any url if its query args contains ';' or its path contains UTF-8 code.

This problem is caused by ctx.Request().URI() returning decoded path and query args. Thus the newly assembled url by createPresignedHttpRequestFromCtx is not canonical and may contain unescaped semicolon ';'. This will make signer.PresignHTTP() in CheckPresignedSignature fails to generate correct signature because query args containing semicolon is omitted by net/url.ParseQuery, as the stack below shows

net/url.parseQuery() /snap/go/10553/src/net/url/url.go:944 (PC: 0x79ef33)
   939:        func parseQuery(m Values, query string) (err error) {
   940:                for query != "" {
   941:                        var key string
   942:                        key, query, _ = strings.Cut(query, "&")
   943:                        if strings.Contains(key, ";") {
=> 944:                                err = fmt.Errorf("invalid semicolon separator in query")
   945:                                continue
   946:                        }
   947:                        if key == "" {
   948:                                continue
   949:                        }
(dlv) bt
bt
 0  0x000000000079ef33 in net/url.parseQuery
    at /snap/go/10553/src/net/url/url.go:944
 1  0x000000000079ed85 in net/url.ParseQuery
    at /snap/go/10553/src/net/url/url.go:935
 2  0x00000000007a050d in net/url.(*URL).Query
    at /snap/go/10553/src/net/url/url.go:1122
 3  0x00000000009b80f7 in github.com/aws/aws-sdk-go-v2/aws/signer/v4.(*httpSigner).Build
    at ./go/pkg/mod/github.com/aws/aws-sdk-go-v2@v1.24.1/aws/signer/v4/v4.go:153
 4  0x00000000009b9c25 in github.com/aws/aws-sdk-go-v2/aws/signer/v4.(*Signer).PresignHTTP
    at ./go/pkg/mod/github.com/aws/aws-sdk-go-v2@v1.24.1/aws/signer/v4/v4.go:377
 5  0x00000000012c97ba in github.com/versity/versitygw/s3api/utils.CheckPresignedSignature
    at ./go/src/github.com/versity/versitygw/s3api/utils/presign-auth-reader.go:98
 6  0x0000000001493ec5 in github.com/versity/versitygw/s3api/middlewares.VerifyPresignedV4Signature.func1
    at ./go/src/github.com/versity/versitygw/s3api/middlewares/presign-auth.go:62
 7  0x0000000001297b29 in github.com/gofiber/fiber/v2.(*Ctx).Next
    at ./go/pkg/mod/github.com/gofiber/fiber/v2@v2.52.0/ctx.go:1027

The same analysis applies to ute-8 code.

According to https://docs.aws.amazon.com/IAM/latest/UserGuide/create-signed-request.html, url should be canonical when checking presigned url. A proposed fix is listed below, subject to consideration by the VGW author.

diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go
index aa4346b..7567e28 100644
--- a/s3api/utils/utils.go
+++ b/s3api/utils/utils.go
@@ -20,11 +20,13 @@ import (
        "fmt"
        "io"
        "net/http"
+       "net/url"
        "regexp"
        "strconv"
        "strings"
        "time"

+       "github.com/aws/smithy-go/encoding/httpbinding"
        "github.com/gofiber/fiber/v2"
        "github.com/valyala/fasthttp"
        "github.com/versity/versitygw/s3err"
@@ -107,16 +109,18 @@ func createPresignedHttpRequestFromCtx(ctx *fiber.Ctx, signedHdrs []string, cont
        }

        uri := string(ctx.Request().URI().Path())
+       uri = httpbinding.EscapePath(uri, false)
        isFirst := true

        ctx.Request().URI().QueryArgs().VisitAll(func(key, value []byte) {
                _, ok := signedQueryArgs[string(key)]
                if !ok {
+                       escapeValue := url.QueryEscape(string(value))
                        if isFirst {
-                               uri += fmt.Sprintf("?%s=%s", key, value)
+                               uri += fmt.Sprintf("?%s=%s", key, escapeValue)
                                isFirst = false
                        } else {
-                               uri += fmt.Sprintf("&%s=%s", key, value)
+                               uri += fmt.Sprintf("&%s=%s", key, escapeValue)
                        }
                }
        })

To Reproduce Use AWS SDK to presign a URL whose query argument contains a semicolon, like below

2024/03/15 21:55:24 Request query arguments:
2024/03/15 21:55:24 response-content-disposition: inline; filename="The_Rust_Programming_Language.pdf"; filename*=UTF-8''The_Rust_Programming_Language.pdf
2024/03/15 21:55:24 response-content-type: application/pdf

Server Version output of

./versitygw --version
Version  : v0.14
Build    : 905b283
BuildTime: 2024-03-16_04:27:15AM

Additional context Describe s3 client and version if applicable.

[benmcclelland]

@zhucaifeng81 Thanks for the bug report and a fix! Feel free to open a PR with this patch if you want to want to get the github contribution credit for it. If not, no problem and I'll open a PR for this.

[zhucaifeng81]

No credit is needed. It is already a pleasure to study the clean VGW code. Thanks for your excellent work.