From 2daf214516d29a55ca1b14628cd3b5e16919c8d7 Mon Sep 17 00:00:00 2001 From: Chenfeng Bao Date: Sun, 22 Dec 2024 00:05:50 -0800 Subject: [PATCH] fix: skip unsignable headers (#1207) Co-authored-by: Mattias Kindborg --- CHANGELOG.md | 4 ++ src/Private/CanonicalRequest.cs | 33 +++++++++-- .../ApiGateway/AwsSignatureHandlerShould.cs | 25 ++++++++ .../Integration/ApiGateway/SendAsyncShould.cs | 38 +++++++++++- test/Integration/ApiGateway/SendShould.cs | 36 ++++++++++++ .../S3/AwsSignatureHandlerShould.cs | 21 +++++++ test/Integration/S3/SendAsyncShould.cs | 23 ++++++++ test/Integration/S3/SendShould.cs | 23 ++++++++ .../CanonicalRequestGivenDotnetShould.cs | 4 +- .../CanonicalRequestGivenMonoShould.cs | 4 +- test/Unit/Private/CanonicalRequestShould.cs | 58 +++++++++++++++++-- 11 files changed, 256 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6eab63d..be469d00 100644 --- 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 index 913dab81..964d5d34 100644 --- a/src/Private/CanonicalRequest.cs +++ b/src/Private/CanonicalRequest.cs @@ -30,6 +30,24 @@ public static class CanonicalRequest /// 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 UnsignableHeaders = + [ + "connection", + "expect", + "keep-alive", + "proxy-authenticate", + "proxy-authorization", + "proxy-connection", + "range", + "te", + "trailer", + "transfer-encoding", + "upgrade", + HeaderKeys.XAmznTraceIdHeader + ]; + /// /// The first value is the canonical request, the second value is the signed headers. /// @@ -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> SortQueryParameters(string query) return sortedQueryParameters; } - public static SortedDictionary> SortHeaders( + public static SortedDictionary> PruneAndSortHeaders( HttpRequestHeaders headers, IEnumerable>> defaultHeaders) { @@ -202,6 +222,11 @@ void AddHeader(KeyValuePair> 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 index d08d5d8b..5914fabd 100644 --- 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 index 3727a96d..4341f07b 100644 --- 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 index bef7d15a..0b5adcef 100644 --- 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 index 5a82501d..f7bd246a 100644 --- 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 index 676663cf..bb83528b 100644 --- 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 index 453c14fd..e848e789 100644 --- 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 index c192e428..09bb997d 100644 --- 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 index 0fbb15ee..8ff3f882 100644 --- 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 index cf6912ba..9933ff8b 100644 --- a/test/Unit/Private/CanonicalRequestShould.cs +++ b/test/Unit/Private/CanonicalRequestShould.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Net.Http; using System.Threading.Tasks; using Amazon.Util; @@ -20,6 +21,23 @@ public CanonicalRequestShould(TestSuiteContext context) context.AdjustHeaderValueSeparator(); } + public static IEnumerable UnsignableHeadersTestCases => + new[] + { + new[] { "Connection", "keep-alive"}, + new[] { "Expect", "100-continue"}, + new[] { "Keep-Alive", "timeout=5"}, + new[] { "Proxy-Authenticate", "Basic"}, + new[] { "Proxy-Authorization", "Basic dXNlcm5hbWU6cGFzc3dvcmQ="}, + new[] { "Proxy-Connection", "keep-alive"}, + new[] { "Range", "bytes=0-499"}, + new[] { "TE", "gzip"}, + new[] { "Trailer", "Expires"}, + new[] { "Transfer-Encoding", "gzip"}, + new[] { "Upgrade", "websocket"} + }; + + [Theory] [InlineData("get-header-key-duplicate")] [InlineData("get-header-value-multiline")] @@ -109,7 +127,7 @@ public void TransformHeaderNamesToLowercase(string headerName, string expected) headers.Add(headerName, "some header value"); // Act - var actual = CanonicalRequest.SortHeaders(headers, null); + var actual = CanonicalRequest.PruneAndSortHeaders(headers, null); // Assert actual.Keys.ShouldBe(new[] { expected }); @@ -133,7 +151,7 @@ public void SortHeaderNames(string[] headerNames, string[] expected) } // Act - var actual = CanonicalRequest.SortHeaders(headers, null); + var actual = CanonicalRequest.PruneAndSortHeaders(headers, null); // Assert actual.Keys.ShouldBe(expected); @@ -156,7 +174,7 @@ public void TrimHeaderValue(string headerValue, string expected) headers.Add("some-header-name", headerValue); // Act - var actual = CanonicalRequest.SortHeaders(headers, null); + var actual = CanonicalRequest.PruneAndSortHeaders(headers, null); // Assert actual["some-header-name"].ShouldBe(new[] { expected }); @@ -176,12 +194,27 @@ public void RemoveSequentialSpacesInHeaderValue(string headerValue, string expec headers.Add("some-header-name", headerValue); // Act - var actual = CanonicalRequest.SortHeaders(headers, null); + var actual = CanonicalRequest.PruneAndSortHeaders(headers, null); // Assert actual["some-header-name"].ShouldBe(new[] { expected }); } + [Theory] + [MemberData(nameof(UnsignableHeadersTestCases))] + public void RemoveUnsignableHeaders(string headerName, string headerValue) + { + // Arrange + var headers = new HttpRequestMessage().Headers; + headers.Add(headerName, headerValue); + + // Act + var actual = CanonicalRequest.PruneAndSortHeaders(headers, null); + + // Assert + actual.ShouldBeEmpty(); + } + [Theory] [InlineData("?a=1&b=2&c=3", new[] { "a", "b", "c" })] [InlineData("?a=1&c=3&b=2", new[] { "a", "b", "c" })] @@ -223,6 +256,23 @@ public void SortQueryParameterValues(string query, string parameterName, string[ actual[parameterName].ShouldBe(expected); } + internal static void AddUnsignableHeaders(HttpRequestMessage request) + { + foreach (var testCase in UnsignableHeadersTestCases) + { + var headerName = (string)testCase[0]; + var headerValue = (string)testCase[0]; + + // Exclude the following headers due to them failing + if (headerValue is "Range" or "Transfer-Encoding") + { + continue; + } + + request.Headers.Add(headerName, headerValue); + } + } + public void Dispose() => context.ResetHeaderValueSeparator(); } }