soto-core icon indicating copy to clipboard operation
soto-core copied to clipboard

AWSSigner processURL does not handle query parameters with no equal sign or value correctly

Open mildm8nnered opened this issue 2 years ago • 2 comments

Describe the bug

So I think there's an issue with the implementation of processURL in AWSSigner. Consider a request to begin a multipart upload, with bucket "MyBucket", the path according to Amazon should be "MyBucket?uploads".

But for signing, Amazon considers the encoded query string to be uploads=, and the = is important. Amazon will fail signatures that do not use that form as the second line of the canonical request string constructed during signing.

The issue is that AWSSigner.processURL() does not add the = in this case. At or around line 65, I think

        let urlQueryString = urlComponents.queryItems?
            .sorted {
                if $0.name < $1.name { return true }
                if $0.name > $1.name { return false }
                guard let value1 = $0.value, let value2 = $1.value else { return false }
                return value1 < value2
            }
            .map { item in item.value.map { "\(item.name)=\($0.uriEncode())" } ?? item.name }
            .joined(separator: "&")

really wants to be

        let urlQueryString = urlComponents.queryItems?
            .sorted {
                if $0.name < $1.name { return true }
                if $0.name > $1.name { return false }
                guard let value1 = $0.value, let value2 = $1.value else { return false }
                return value1 < value2
            }
            .map { item in item.value.map { "\(item.name)=\($0.uriEncode())" } ?? "\(item.name)=" }
            .joined(separator: "&")

Assuming I'm right, it's really surprising to me that this hasn't come up before. I grabbed soto-s3-file-transfer, but looking there, the begin multipart request gets passed through another very similar piece of code, in AWSRequest.swift (around line 285), that performs an almost identical operation, but taking care of nil values correctly:

        // Set query params. Percent encode these ourselves as Foundation and AWS disagree on what should be percent encoded in the query values
        // Also the signer doesn't percent encode the queries so they need to be encoded here
        if queryParams.count > 0 {
            let urlQueryString = queryParams
                .map { (key: $0.key, value: "\($0.value)") }
                .sorted {
                    // sort by key. if key are equal then sort by value
                    if $0.key < $1.key { return true }
                    if $0.key > $1.key { return false }
                    return $0.value < $1.value
                }
                .map { "\($0.key)=\(Self.urlEncodeQueryParam($0.value))" }
                .joined(separator: "&")
            urlComponents.percentEncodedQuery = urlQueryString
        }

To Reproduce

Steps to reproduce the behavior:

Just pass a URL with path MyBucket?uploads to AWSSigner.processURL() to see that it does not add the =

Expected behavior

processed URLs should be in the correct format for Amazon canonical request string

Setup (please complete the following information):

  • OS: Monterey
  • Version of aws-sdk-swift : HEAD of main
  • Authentication mechanism hard-coded credentials

Additional context Add any other context about the problem here.

mildm8nnered avatar Oct 11 '22 22:10 mildm8nnered

processURL isn't actually used by SotoCore service operations. It is there just to provide support for signing URLs. That is why this hasn't been caught earlier. If you want to add a PR for this that would be most helpful.

adam-fowler avatar Oct 12 '22 10:10 adam-fowler

https://github.com/soto-project/soto-core/pull/524

mildm8nnered avatar Oct 16 '22 17:10 mildm8nnered