tsibelman / aws-signer-v4-dot-net

Sign HttpRequestMessage using AWS Signature v4 using request information and credentials.
Apache License 2.0
72 stars 27 forks source link

Wrong canonical string created while trying multiple comma separated values in IncludedData #38

Open krutipatel-8794 opened 2 years ago

krutipatel-8794 commented 2 years ago

I am trying to call getCatalogItem operation with multiple comma separated values in IncludedData parameter. When I am including more than one value (i.e. attributes,identifiers,images,summaries), it has generated wrong canonical string and throws a signature error. (Note: It is adding 'includedData=' for each values)

@tsibelman I think this is caused by https://github.com/tsibelman/aws-signer-v4-dot-net/pull/25/files changes. I have tried to revert those changes in my code with some modification and it gives me correct Querystring (includedData=attributes%2Csummaries%2Cidentifiers%2Cimages&locale=en-US&marketplaceIds=XXXXXXXXXXXXXX)

I am using 1.0.3.0 package version of Aws4RequestSigner.

Generated canonical string using current code/package: GET /catalog/2020-12-01/items/ includedData=attributes&includedData=identifiers&includedData=images&includedData=summaries&locale=en-US&marketplaceIds=XXXXXXXXXXXXXX host:sellingpartnerapi-fe.amazon.com x-amz-access-token: x-amz-content-sha256: x-amz-date:20220420T001006Z x-amz-security-token:

Error: { "errors": [ { "message": "The request signature we calculated does not match the signature you provided. Check your AWS Secret Access Key and signing method. Consult the service documentation for details.

The Canonical String for this request should have been 'GET /catalog/2020-12-01/items/ includedData=attributes%2Csummaries%2Cidentifiers%2Cimages&locale=en-US&marketplaceIds=XXXXXXXXXXXXXX host:sellingpartnerapi-fe.amazon.com x-amz-access-token: x-amz-content-sha256: x-amz-date:20220419T235917Z x-amz-security-token:

host;x-amz-access-token;x-amz-content-sha256;x-amz-date;x-amz-security-token

' The String-to-Sign should have been 'AWS4-HMAC-SHA256 20220419T235917Z 20220419/us-west-2/execute-api/aws4_request ' ", "code": "InvalidSignature" } ] } See below for reference: https://developer-docs.amazon.com/sp-api/docs/catalog-items-api-v2020-12-01-use-case-guide Endpoint: GET https://sellingpartnerapi-na.amazon.com/catalog/2020-12-01/items/XXXXXXXXXX ?marketplaceIds=ATVPDKIKX0DER &includedData=attributes,identifiers,images,summaries Does anyone have any idea how I can resolve this issue?
krutipatel-8794 commented 2 years ago

@tsibelman Could you please help me with this? or do you have any other alternatives to resolve this issue?

krutipatel-8794 commented 2 years ago

@ronenfe Could you please help me with the issue here?

ronenfe commented 2 years ago

Hi, please provide your usage of the library, ommiting private data. I am not sure what is getCatalogItem and what is includedData.

krutipatel-8794 commented 2 years ago

Hi @ronenfe, I am facing a problem while generating URLs with query parameters having multiple values.

When I have a query parameter with more than one value, it generates a URL by adding a query parameter name for each value.

For example: Here includedData is one of the query parameters for the getCatalogItem endpoint and it has values like attributes, identifiers, images, and summaries. I have added my current result URL generated by the method GetCanonicalQueryParams() in AWS4RequestSigner.cs

Current result: (Here you can see there is "includedData=" for each value){URL}/includedData=attributes&includedData=identifiers&includedData=images&includedData=summaries&locale=en-US&marketplaceIds=XXXXXXXXXXXXXX

Expected result: (Expected result should have multiple comma separated values for one query parameter) {URL}/includedData=attributes,summaries,identifiers,images&locale=en-US&marketplaceIds=XXXXXXXXXXXXXX

Let me know if you need more information, Thanks.

ronenfe commented 2 years ago

Hi, are you saying you found the problem that's causing the issue and fixed it? It's hard for me to understand and reproduce without you sending a sample code that is not working for you.

krutipatel-8794 commented 2 years ago

Hi @ronenfe , The below code for generating query parameters is not working for me:

private static string GetCanonicalQueryParams(HttpRequestMessage request)
    {
      SortedDictionary<string, IEnumerable<string>> source = new SortedDictionary<string, IEnumerable<string>>((IComparer<string>) StringComparer.Ordinal);
      NameValueCollection queryString = HttpUtility.ParseQueryString(request.RequestUri.Query);
      foreach (string allKey in queryString.AllKeys)
      {
        string key = allKey;
        if (key == null)
        {
          source.Add(Uri.EscapeDataString(queryString[key]), (IEnumerable<string>) new string[1]
          {
            Uri.EscapeDataString(queryString[key]) + "="
          });
        }
        else
        {
          IEnumerable<string> strings = ((IEnumerable<string>) queryString[key].Split(',')).OrderBy<string, string>((Func<string, string>) (v => v)).Select<string, string>((Func<string, string>) (v => Uri.EscapeDataString(key) + "=" + Uri.EscapeDataString(v)));
          source.Add(Uri.EscapeDataString(key), strings);
        }
      }
      return string.Join("&", source.SelectMany<KeyValuePair<string, IEnumerable<string>>, string>((Func<KeyValuePair<string, IEnumerable<string>>, IEnumerable<string>>) (a => a.Value)));
    }

I feel like changes from https://github.com/tsibelman/aws-signer-v4-dot-net/pull/25/files are causing the issue somewhere. I might be wrong, as I am not 100% sure what scenarios you want to achieve using PR.

But you got the scenario that is not working in my case, right?

ronenfe commented 2 years ago

I see it's commited by John Lagnese, maybe reach out for him, I can try to debug if you give me a code sample of your usage of the library.

ronenfe commented 2 years ago

I see here there is no standard when sending multiple values per key in the querystring: https://stackoverflow.com/questions/24059773/correct-way-to-pass-multiple-values-for-same-parameter-name-in-get-request If amazon requires the format for the signature to be one key and separated by comma values, maybe we should change it back to that. But then why was it changed to that by John Lagnese? I don't know what's his github username so I can't tag him here.

krutipatel-8794 commented 2 years ago

@jlagnese-allata Could you please have a look at this issue and the above conversation?

@ronenfe I am not getting what actually you want as a code sample of my usage of the library? I have added some information in my very first comment by omitting private data, was it not useful?

ronenfe commented 2 years ago

@krutipatel-8794 Yes, it's ok, I understand that your request.uri has a querystring key with multiple values and the code change made duplication of the key to each value which fails the Aws signature.

krutipatel-8794 commented 2 years ago

Hi @ronenfe Have you had a chance to look more into this issue and how and when we are going to fix this? Unfortunately, @jlagnese-allata is not replying, so we are not aware of why he has changed the code in that PR?

gao87926 commented 1 year ago

The data for calculating "Canonical Query String" needs to do uriEncode first, which means the comma should be replaced by "%2C"

https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html

devgitprojects commented 1 year ago

the same issue when you try to use Amazon Managed Service for Prometheus APIs for reading prometheus metrics. Prometheus query may contain multiple comma separated values:

query={instance=\"987654321\",label=\"123456789\"}

https://aps-workspaces.eu-central-1.amazonaws.com/workspaces/ws-963ed5f0-12f5-4f19-8911-2b41dc12397f/api/v1/query?query={instance=\"987654321\",label=\"123456789\"}

The solution is to change GetCanonicalQueryParams - remove Split

` private static string GetCanonicalQueryParams(HttpRequestMessage request) { var values = new SortedDictionary<string, IEnumerable>(StringComparer.Ordinal);

        var querystring = HttpUtility.ParseQueryString(request.RequestUri.Query);
        foreach (var key in querystring.AllKeys)
        {
            if (key == null)//Handles keys without values
            {
                values.Add(Uri.EscapeDataString(querystring[key]), new[] { $"{Uri.EscapeDataString(querystring[key])}=" });
            }
            else
            {
                // Handles multiple values per query parameter
                var queryValues = new[] { querystring[key] }
                    // Order by value alphanumerically (required for correct canonical string)
                    .OrderBy(v => v)
                    // Query params must be escaped in upper case (i.e. "%2C", not "%2c").
                    .Select(v => $"{Uri.EscapeDataString(key)}={Uri.EscapeDataString(v)}");

                values.Add(Uri.EscapeDataString(key), queryValues);
            }
        }

        var queryParams = values.SelectMany(a => a.Value);
        var canonicalQueryParams = string.Join("&", queryParams);
        return canonicalQueryParams;
    }

`

nrcdvyskrebets commented 7 months ago

Same issue here When I try to sign request to https://111111111-vpce-2222222.execute-api.us-east-1.amazonaws.com/dev/me/videos?filter_tag_all_of=value1,value2 (I replaced some values), response from Gateway is 403 Forbidden. Though same request from the Postman has correct response with correct data. Also, doing this thing helps me string uri = $"me/videos?filter_tag_all_of={HttpUtility.UrlEncode($"{envTag},{moduleTag}")}", but then wrong value passed to Gateway and parameters basically ignored.

signing process looks strightforward

        protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
        {
            var creds = await _credentials.GetCredentialsAsync();
            var signer = new AWS4RequestSigner(creds.AccessKey, creds.SecretKey);

            var signedRequest = await signer.Sign(request, _awsServiceName, _region);
            return await base.SendAsync(signedRequest, cancellationToken);
        }