fix: skip unsignable headers (#1207)

cfbao · FantasticFiasco · web-flow · commit 2daf214516d2 · 2024-12-22T09:05:50.000+01:00
Co-authored-by: Mattias Kindborg &lt;mattias@kindb.org&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@ This project adheres to [Semantic Versioning](http://semver.org/) and is followi
 
 ## Unreleased
 
+### :syringe: Fixed
+
+- [#1198](https://github.com/FantasticFiasco/aws-signature-version-4/issues/1198) Requests with certain HTTP headers are rejected by API Gateway. (contribution by [@cfbao](https://github.com/cfbao))
+
 ## [4.0.6] - 2024-09-18
 
 ### :syringe: Fixed
diff --git a/src/Private/CanonicalRequest.cs b/src/Private/CanonicalRequest.cs
@@ -30,6 +30,24 @@ public static class CanonicalRequest
         /// </summary>
         public static string HeaderValueSeparator { get; set; } = ", ";
 
+        // Including most headers from
+        // https://github.com/smithy-lang/smithy-typescript/blob/430021abf44f8a4d6c24de2dfa25709bf91a92c8/packages/signature-v4/src/constants.ts#L19-L35
+        private static readonly HashSet<string> UnsignableHeaders =
+        [
+            "connection",
+            "expect",
+            "keep-alive",
+            "proxy-authenticate",
+            "proxy-authorization",
+            "proxy-connection",
+            "range",
+            "te",
+            "trailer",
+            "transfer-encoding",
+            "upgrade",
+            HeaderKeys.XAmznTraceIdHeader
+        ];
+
         /// <returns>
         /// The first value is the canonical request, the second value is the signed headers.
         /// </returns>
@@ -91,8 +109,10 @@ public static (string, string) Build(
             builder.Append($"{string.Join("&", parameters)}\n");
 
             // Add the canonical headers, followed by a newline character. The canonical headers
-            // consist of a list of all the HTTP headers that you are including with the signed
-            // request.
+            // consist of a list of HTTP headers that you are including with the signed request.
+            //
+            // Some headers are unsignable, i.e. signatures including them are always deemed invalid.
+            // They are excluded from the canonical headers (but will be kept in the HTTP request).
             //
             // To create the canonical headers list, convert all header names to lowercase and
             // remove leading spaces and trailing spaces. Convert sequential spaces in the header
@@ -108,7 +128,7 @@ public static (string, string) Build(
             //   PLEASE NOTE: Microsoft has chosen to separate the header values with ", ", not ","
             //   as defined by the Canonical Request algorithm.
             // - Append a new line ('\n').
-            var sortedHeaders = SortHeaders(request.Headers, defaultHeaders);
+            var sortedHeaders = PruneAndSortHeaders(request.Headers, defaultHeaders);
 
             foreach (var header in sortedHeaders)
             {
@@ -187,7 +207,7 @@ public static SortedList<string, List<string>> SortQueryParameters(string query)
             return sortedQueryParameters;
         }
 
-        public static SortedDictionary<string, List<string>> SortHeaders(
+        public static SortedDictionary<string, List<string>> PruneAndSortHeaders(
             HttpRequestHeaders headers,
             IEnumerable<KeyValuePair<string, IEnumerable<string>>> defaultHeaders)
         {
@@ -202,6 +222,11 @@ void AddHeader(KeyValuePair<string, IEnumerable<string>> header)
             {
                 var headerName = FormatHeaderName(header.Key);
 
+                if (UnsignableHeaders.Contains(headerName))
+                {
+                    return;
+                }
+
                 // Create header if it doesn't already exist
                 if (!sortedHeaders.TryGetValue(headerName, out var headerValues))
                 {
diff --git a/test/Integration/ApiGateway/AwsSignatureHandlerShould.cs b/test/Integration/ApiGateway/AwsSignatureHandlerShould.cs
@@ -7,6 +7,7 @@
 using AwsSignatureVersion4.Integration.ApiGateway.Requests;
 using AwsSignatureVersion4.Private;
 using AwsSignatureVersion4.TestSuite;
+using AwsSignatureVersion4.Unit.Private;
 using Shouldly;
 using Xunit;
 
@@ -316,6 +317,30 @@ public async Task SucceedGivenUnorderedQuery(
             receivedRequest.Body.ShouldBeNull();
         }
 
+        [Theory]
+        [MemberData(nameof(TestCases))]
+        public async Task SucceedGivenUnsignableHeaders(
+            IamAuthenticationType iamAuthenticationType,
+            HttpMethod method)
+        {
+            // Arrange
+            using var httpClient = HttpClientFactory(iamAuthenticationType).CreateClient("integration");
+
+            var request = new HttpRequestMessage(method, Context.ApiGatewayUrl);
+            CanonicalRequestShould.AddUnsignableHeaders(request);
+
+            // Act
+            var response = await httpClient.SendAsync(request);
+
+            // Assert
+            response.StatusCode.ShouldBe(HttpStatusCode.OK);
+
+            var receivedRequest = await response.Content.ReadReceivedRequestAsync();
+            receivedRequest.Method.ShouldBe(method.ToString());
+            receivedRequest.Path.ShouldBe("/");
+            receivedRequest.Body.ShouldBeNull();
+        }
+
         [Theory]
         [MemberData(nameof(TestCases))]
         public async Task SucceedGivenHttpCompletionOption(
diff --git a/test/Integration/ApiGateway/SendAsyncShould.cs b/test/Integration/ApiGateway/SendAsyncShould.cs
@@ -7,6 +7,7 @@
 using AwsSignatureVersion4.Integration.ApiGateway.Requests;
 using AwsSignatureVersion4.Private;
 using AwsSignatureVersion4.TestSuite;
+using AwsSignatureVersion4.Unit.Private;
 using Shouldly;
 using Xunit;
 
@@ -662,6 +663,41 @@ public async Task SucceedGivenUnorderedQuery(
             receivedRequest.Body.ShouldBeNull();
         }
 
+        [Theory]
+        [InlineData(IamAuthenticationType.User, "GET")]
+        [InlineData(IamAuthenticationType.User, "POST")]
+        [InlineData(IamAuthenticationType.User, "PUT")]
+        [InlineData(IamAuthenticationType.User, "PATCH")]
+        [InlineData(IamAuthenticationType.User, "DELETE")]
+        [InlineData(IamAuthenticationType.Role, "GET")]
+        [InlineData(IamAuthenticationType.Role, "POST")]
+        [InlineData(IamAuthenticationType.Role, "PUT")]
+        [InlineData(IamAuthenticationType.Role, "PATCH")]
+        [InlineData(IamAuthenticationType.Role, "DELETE")]
+        public async Task SucceedGivenUnsignableHeaders(
+            IamAuthenticationType iamAuthenticationType,
+            string method)
+        {
+            // Arrange
+            var request = new HttpRequestMessage(new HttpMethod(method), Context.ApiGatewayUrl);
+            CanonicalRequestShould.AddUnsignableHeaders(request);
+
+            // Act
+            var response = await HttpClient.SendAsync(
+                request,
+                Context.RegionName,
+                Context.ServiceName,
+                ResolveMutableCredentials(iamAuthenticationType));
+
+            // Assert
+            response.StatusCode.ShouldBe(HttpStatusCode.OK);
+
+            var receivedRequest = await response.Content.ReadReceivedRequestAsync();
+            receivedRequest.Method.ShouldBe(method);
+            receivedRequest.Path.ShouldBe("/");
+            receivedRequest.Body.ShouldBeNull();
+        }
+
         internal static HttpRequestMessage BuildRequest(
             TestSuiteContext testSuiteContext,
             IntegrationTestContext integrationTestContext,
@@ -676,7 +712,7 @@ internal static HttpRequestMessage BuildRequest(
                 .ToUri();
 
             // The "Host" header is now invalid since we redirected the request to the AWS API
-            // Gateway. Lets remove the header and have the signature implementation re-add it
+            // Gateway. Let's remove the header and have the signature implementation re-add it
             // correctly.
             request.Headers.Remove("Host");
 
diff --git a/test/Integration/ApiGateway/SendShould.cs b/test/Integration/ApiGateway/SendShould.cs
@@ -7,6 +7,7 @@
 using AwsSignatureVersion4.Integration.ApiGateway.Requests;
 using AwsSignatureVersion4.Private;
 using AwsSignatureVersion4.TestSuite;
+using AwsSignatureVersion4.Unit.Private;
 using Shouldly;
 using Xunit;
 
@@ -661,5 +662,40 @@ public async Task SucceedGivenUnorderedQuery(
             receivedRequest.QueryStringParameters["Param1"].ShouldBe(new[] { "Value2", "Value1" });
             receivedRequest.Body.ShouldBeNull();
         }
+
+        [Theory]
+        [InlineData(IamAuthenticationType.User, "GET")]
+        [InlineData(IamAuthenticationType.User, "POST")]
+        [InlineData(IamAuthenticationType.User, "PUT")]
+        [InlineData(IamAuthenticationType.User, "PATCH")]
+        [InlineData(IamAuthenticationType.User, "DELETE")]
+        [InlineData(IamAuthenticationType.Role, "GET")]
+        [InlineData(IamAuthenticationType.Role, "POST")]
+        [InlineData(IamAuthenticationType.Role, "PUT")]
+        [InlineData(IamAuthenticationType.Role, "PATCH")]
+        [InlineData(IamAuthenticationType.Role, "DELETE")]
+        public async Task SucceedGivenUSucceedGivenUnsignableHeadersnorderedQuery(
+            IamAuthenticationType iamAuthenticationType,
+            string method)
+        {
+            // Arrange
+            var request = new HttpRequestMessage(new HttpMethod(method), Context.ApiGatewayUrl);
+            CanonicalRequestShould.AddUnsignableHeaders(request);
+            
+            // Act
+            var response = HttpClient.Send(
+                request,
+                Context.RegionName,
+                Context.ServiceName,
+                ResolveMutableCredentials(iamAuthenticationType));
+
+            // Assert
+            response.StatusCode.ShouldBe(HttpStatusCode.OK);
+
+            var receivedRequest = await response.Content.ReadReceivedRequestAsync();
+            receivedRequest.Method.ShouldBe(method);
+            receivedRequest.Path.ShouldBe("/");
+            receivedRequest.Body.ShouldBeNull();
+        }
     }
 }
diff --git a/test/Integration/S3/AwsSignatureHandlerShould.cs b/test/Integration/S3/AwsSignatureHandlerShould.cs
@@ -4,6 +4,7 @@
 using AwsSignatureVersion4.Integration.ApiGateway.Authentication;
 using AwsSignatureVersion4.Private;
 using AwsSignatureVersion4.TestSuite;
+using AwsSignatureVersion4.Unit.Private;
 using Shouldly;
 using Xunit;
 
@@ -136,6 +137,26 @@ public async Task SucceedGivenHttpCompletionOption(IamAuthenticationType iamAuth
             response.StatusCode.ShouldBe(HttpStatusCode.OK);
         }
 
+        [Theory]
+        [InlineData(IamAuthenticationType.User)]
+        [InlineData(IamAuthenticationType.Role)]
+        public async Task SucceedGivenUnsignableHeaders(IamAuthenticationType iamAuthenticationType)
+        {
+            // Arrange
+            var bucketObject = await Bucket.PutObjectAsync(BucketObjectKey.WithoutPrefix);
+
+            using var httpClient = HttpClientFactory(iamAuthenticationType).CreateClient("integration");
+            var requestUri = $"{Context.S3BucketUrl}/{bucketObject.Key}";
+            var request = new HttpRequestMessage(HttpMethod.Get, requestUri);
+            CanonicalRequestShould.AddUnsignableHeaders(request);
+            
+            // Act
+            var response = await httpClient.SendAsync(request);
+
+            // Assert
+            response.StatusCode.ShouldBe(HttpStatusCode.OK);
+        }
+
         private async Task UploadRequiredObjectAsync(params string[] scenarioName)
         {
             if (scenarioName[0] == "get-unreserved" || scenarioName[0] == "get-vanilla-query-unreserved")
diff --git a/test/Integration/S3/SendAsyncShould.cs b/test/Integration/S3/SendAsyncShould.cs
@@ -4,6 +4,7 @@
 using AwsSignatureVersion4.Integration.ApiGateway.Authentication;
 using AwsSignatureVersion4.Private;
 using AwsSignatureVersion4.TestSuite;
+using AwsSignatureVersion4.Unit.Private;
 using Shouldly;
 using Xunit;
 
@@ -124,6 +125,28 @@ public async Task PassTestSuiteGivenAssumedRole(params string[] scenarioName)
             response.StatusCode.ShouldBe(HttpStatusCode.OK);
         }
 
+        [Theory]
+        [InlineData(IamAuthenticationType.User)]
+        [InlineData(IamAuthenticationType.Role)]
+        public async Task SucceedGivenUnsignableHeaders(IamAuthenticationType iamAuthenticationType)
+        {
+            // Arrange
+            var bucketObject = await Bucket.PutObjectAsync(BucketObjectKey.WithoutPrefix);
+            var requestUri = $"{Context.S3BucketUrl}/{bucketObject.Key}";
+            var request = new HttpRequestMessage(HttpMethod.Get, requestUri);
+            CanonicalRequestShould.AddUnsignableHeaders(request);
+
+            // Act
+            var response = await HttpClient.SendAsync(
+                request,
+                Context.RegionName,
+                Context.ServiceName,
+                ResolveMutableCredentials(iamAuthenticationType));
+
+            // Assert
+            response.StatusCode.ShouldBe(HttpStatusCode.OK);
+        }
+
         [Theory]
         [InlineData(IamAuthenticationType.User)]
         [InlineData(IamAuthenticationType.Role)]
diff --git a/test/Integration/S3/SendShould.cs b/test/Integration/S3/SendShould.cs
@@ -3,6 +3,7 @@
 using System.Threading.Tasks;
 using AwsSignatureVersion4.Integration.ApiGateway.Authentication;
 using AwsSignatureVersion4.TestSuite;
+using AwsSignatureVersion4.Unit.Private;
 using Shouldly;
 using Xunit;
 
@@ -123,6 +124,28 @@ public async Task PassTestSuiteGivenAssumedRole(params string[] scenarioName)
             response.StatusCode.ShouldBe(HttpStatusCode.OK);
         }
 
+        [Theory]
+        [InlineData(IamAuthenticationType.User)]
+        [InlineData(IamAuthenticationType.Role)]
+        public async Task SucceedGivenUnsignableHeaders(IamAuthenticationType iamAuthenticationType)
+        {
+            // Arrange
+            var bucketObject = await Bucket.PutObjectAsync(BucketObjectKey.WithoutPrefix);
+            var requestUri = $"{Context.S3BucketUrl}/{bucketObject.Key}";
+            var request = new HttpRequestMessage(HttpMethod.Get, requestUri);
+            CanonicalRequestShould.AddUnsignableHeaders(request);
+            
+            // Act
+            var response = HttpClient.Send(
+                request,
+                Context.RegionName,
+                Context.ServiceName,
+                ResolveMutableCredentials(iamAuthenticationType));
+
+            // Assert
+            response.StatusCode.ShouldBe(HttpStatusCode.OK);
+        }
+
         [Theory]
         [InlineData(IamAuthenticationType.User)]
         [InlineData(IamAuthenticationType.Role)]
diff --git a/test/Unit/Private/CanonicalRequestGivenDotnetShould.cs b/test/Unit/Private/CanonicalRequestGivenDotnetShould.cs
@@ -33,7 +33,7 @@ public void RespectDefaultHeader()
             defaultHeaders.Add("some-header-name", "some-header-value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, defaultHeaders);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { "some-header-value" });
@@ -50,7 +50,7 @@ public void IgnoreDuplicateDefaultHeader()
             defaultHeaders.Add("some-header-name", "some-ignored-header-value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, defaultHeaders);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { "some-header-value" });
diff --git a/test/Unit/Private/CanonicalRequestGivenMonoShould.cs b/test/Unit/Private/CanonicalRequestGivenMonoShould.cs
@@ -33,7 +33,7 @@ public void RespectDefaultHeader()
             defaultHeaders.Add("some-header-name", "some-header-value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, defaultHeaders);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { "some-header-value" });
@@ -50,7 +50,7 @@ public void RespectDuplicateDefaultHeader()
             defaultHeaders.Add("some-header-name", "some-other-header-value");
 
             // Act
-            var actual = CanonicalRequest.SortHeaders(headers, defaultHeaders);
+            var actual = CanonicalRequest.PruneAndSortHeaders(headers, defaultHeaders);
 
             // Assert
             actual["some-header-name"].ShouldBe(new[] { "some-header-value", "some-other-header-value" });
diff --git a/test/Unit/Private/CanonicalRequestShould.cs b/test/Unit/Private/CanonicalRequestShould.cs
