Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skip unsignable headers #1207

Merged
merged 11 commits into from
Dec 22, 2024
33 changes: 29 additions & 4 deletions src/Private/CanonicalRequest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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 HashSet<string> UnsignableHeaders = new(StringComparer.OrdinalIgnoreCase) {
"authorization",
"connection",
"expect",
"keep-alive",
"proxy-authenticate",
"proxy-authorization",
"proxy-connection",
"range",
"te",
"trailer",
"transfer-encoding",
"upgrade",
HeaderKeys.XAmznTraceIdHeader,
};
FantasticFiasco marked this conversation as resolved.
Show resolved Hide resolved

/// <returns>
/// The first value is the canonical request, the second value is the signed headers.
/// </returns>
Expand Down Expand Up @@ -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
Expand All @@ -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)
{
Expand Down Expand Up @@ -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)
{
Expand All @@ -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))
{
Expand Down
4 changes: 2 additions & 2 deletions test/Unit/Private/CanonicalRequestGivenDotnetShould.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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" });
Expand All @@ -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" });
Expand Down
4 changes: 2 additions & 2 deletions test/Unit/Private/CanonicalRequestGivenMonoShould.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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" });
Expand All @@ -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" });
Expand Down
28 changes: 24 additions & 4 deletions test/Unit/Private/CanonicalRequestShould.cs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,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 });
Expand All @@ -133,7 +133,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);
Expand All @@ -156,7 +156,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 });
Expand All @@ -176,12 +176,32 @@ 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]
[InlineData("Connection", "keep-alive")]
[InlineData("Expect", "100-continue")]
[InlineData("Keep-Alive", "timeout=5")]
[InlineData("Proxy-Authenticate", "Basic")]
[InlineData("Range", "bytes=500-999")]
[InlineData("TE", "gzip")]
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.ShouldNotContainKey(headerName.ToLowerInvariant());
}

[Theory]
[InlineData("?a=1&b=2&c=3", new[] { "a", "b", "c" })]
[InlineData("?a=1&c=3&b=2", new[] { "a", "b", "c" })]
Expand Down