-
Notifications
You must be signed in to change notification settings - Fork 866
Add Support for Multiple Auth Schemes and SigV4a #3999
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
base: development
Are you sure you want to change the base?
Changes from 4 commits
5a8d2fb
c591ddd
1201ab6
20730ab
c33aa26
fb989a0
1490cdb
c6c4013
f1cf998
baa0916
4659c51
1327bbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -119,12 +119,21 @@ public AWS4aSigningResult SignRequest(IRequest request, | |
: AWS4Signer.DetermineService(clientConfig, request); | ||
if (serviceSigningName == "s3") | ||
{ | ||
// Older versions of the S3 package can be used with newer versions of Core, this guarantees no double encoding will be used. | ||
// The new behavior uses endpoint resolution rules, which are not present prior to 3.7.100 | ||
// S3 requests require special URI encoding handling for compatibility | ||
request.UseDoubleEncoding = false; | ||
} | ||
|
||
var regionSet = AWS4Signer.DetermineSigningRegion(clientConfig, clientConfig.RegionEndpointServiceName, request.AlternateEndpoint, request); | ||
// Use the configured SigV4aSigningRegionSet if available (configured for multi-region signing), | ||
// otherwise fall back to single region determination for backward compatibility | ||
string regionSet; | ||
if (!string.IsNullOrEmpty(request.SigV4aSigningRegionSet)) | ||
{ | ||
regionSet = request.SigV4aSigningRegionSet; | ||
} | ||
else | ||
{ | ||
regionSet = AWS4Signer.DetermineSigningRegion(clientConfig, clientConfig.RegionEndpointServiceName, request.AlternateEndpoint, request); | ||
} | ||
request.DeterminedSigningRegion = regionSet; | ||
AWS4Signer.SetXAmzTrailerHeader(request.Headers, request.TrailingHeaders); | ||
|
||
|
@@ -194,14 +203,18 @@ public AWS4aSigningResult Presign4a(IRequest request, | |
{ | ||
if (serviceSigningName == "s3") | ||
{ | ||
// Older versions of the S3 package can be used with newer versions of Core, this guarantees no double encoding will be used. | ||
// The new behavior uses endpoint resolution rules, which are not present prior to 3.7.100 | ||
// S3 requests require special URI encoding handling for compatibility | ||
|
||
request.UseDoubleEncoding = false; | ||
} | ||
|
||
var signedAt = AWS4Signer.InitializeHeaders(request.Headers, request.Endpoint); | ||
request.SignedAt = CorrectClockSkew.GetCorrectedUtcNowForEndpoint(request.Endpoint.ToString()); | ||
var regionSet = overrideSigningRegion ?? AWS4Signer.DetermineSigningRegion(clientConfig, clientConfig.RegionEndpointServiceName, request.AlternateEndpoint, request); | ||
|
||
// Use explicit override, then SigV4aSigningRegionSet, then fall back to single region | ||
var regionSet = overrideSigningRegion | ||
|
||
?? (!string.IsNullOrEmpty(request.SigV4aSigningRegionSet) | ||
? request.SigV4aSigningRegionSet | ||
: AWS4Signer.DetermineSigningRegion(clientConfig, clientConfig.RegionEndpointServiceName, request.AlternateEndpoint, request)); | ||
|
||
var signingConfig = PrepareCRTSigningConfig( | ||
AwsSignatureType.HTTP_REQUEST_VIA_QUERY_PARAMS, | ||
|
@@ -331,7 +344,7 @@ public AwsSigningConfig PrepareCRTSigningConfig(AwsSignatureType signatureType, | |
signingConfig.UseDoubleUriEncode = useDoubleEncoding; | ||
signingConfig.ShouldNormalizeUriPath = useDoubleEncoding; | ||
|
||
// The request headers aren't an input for chunked signing, so don't pass the callback that filters headers. | ||
// The request headers aren't an input for chunked signing, so header filtering is not required | ||
var addCallback = signatureType != AwsSignatureType.HTTP_REQUEST_CHUNK && signatureType != AwsSignatureType.HTTP_REQUEST_TRAILING_HEADERS; | ||
if (addCallback) | ||
{ | ||
|
@@ -359,7 +372,7 @@ public AwsSigningConfig PrepareCRTSigningConfig(AwsSignatureType signatureType, | |
/// </para> | ||
/// </summary> | ||
/// <remarks> | ||
/// Based on the example from the CRT repository: https://github.com/awslabs/aws-crt-dotnet/blob/v0.4.4/tests/SigningTest.cs#L40-L43 | ||
/// Implements AWS CRT best practices for header filtering | ||
/// </remarks> | ||
private static bool ShouldSignHeader(byte[] headerName, uint length) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,13 +52,13 @@ public class V4aSignerTests : IDisposable | |
|
||
public V4aSignerTests() | ||
{ | ||
// Override the SDK's AWSConfigs.utcNowSource to return a fixed time to test predictable signatures | ||
// Configure a fixed timestamp for predictable test signatures | ||
SetUtcNowSource(() => SigningTestTimepoint); | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
// Reset back to the SDK's usual GetUtcNow function | ||
// Restore default timestamp behavior | ||
SetUtcNowSource((Func<DateTime>)Delegate.CreateDelegate(typeof(Func<DateTime>), | ||
typeof(AWSConfigs).GetMethod("GetUtcNow", BindingFlags.Static | BindingFlags.NonPublic))); | ||
} | ||
|
@@ -136,6 +136,36 @@ internal static IRequest BuildHeaderRequestToSign(string resourcePath, Dictionar | |
return request; | ||
} | ||
|
||
/// <summary> | ||
/// Dummy request class needed for DefaultRequest constructor | ||
/// </summary> | ||
private class DummyRequest : AmazonWebServiceRequest { } | ||
|
||
/// <summary> | ||
/// Creates a test request with mutable collections to verify header modifications | ||
/// during signing. | ||
/// </summary> | ||
internal static IRequest CreateDefaultRequest(string resourcePath) | ||
|
||
{ | ||
var publicRequest = new DummyRequest(); | ||
var request = new DefaultRequest(publicRequest, SigningTestService) | ||
{ | ||
HttpMethod = "POST", | ||
ResourcePath = resourcePath, | ||
Endpoint = new Uri($"https://{SigningTestHost}/"), | ||
Content = Encoding.ASCII.GetBytes("Param1=value1") | ||
}; | ||
|
||
request.Headers["Content-Type"] = "application/x-www-form-urlencoded"; | ||
request.Headers["Content-Length"] = "13"; | ||
// Add SDK tracking headers that should NOT be signed | ||
request.Headers["amz-sdk-request"] = "attempt=1; max=5"; | ||
request.Headers["amz-sdk-invocation-id"] = "a7d0c828-1fc1-43e8-9f9e-367a7011fc84"; | ||
request.Headers["x-amzn-trace-id"] = "Root=1-63441c4a-abcdef012345678912345678"; | ||
|
||
return request; | ||
} | ||
|
||
internal string GetExpectedCanonicalRequestForHeaderSigningTest(string canonicalizedResourePath) | ||
{ | ||
return String.Join('\n', | ||
|
@@ -440,5 +470,103 @@ public void TestChunkedRequestWithTrailingHeaders() | |
Encoding.ASCII.GetBytes(trailerChunkResult), SigningTestEccPubX, SigningTestEccPubY)); | ||
} | ||
#endregion | ||
|
||
#region Multi-Region SigV4a Signing Tests | ||
|
||
/// <summary> | ||
/// Tests multi-region SigV4a signing with different region set configurations. | ||
/// | ||
/// The CRT library handles both signature calculation and adding the x-amz-region-set header during | ||
/// the signing process. This test verifies the header is correctly set and included in the signature | ||
/// for different region configurations (single region, multi-region, and wildcard). | ||
/// | ||
/// </summary> | ||
[Theory] | ||
[InlineData("us-west-2", "us-west-2")] | ||
[InlineData("us-west-2,us-east-1", "us-west-2,us-east-1")] | ||
[InlineData("*", "*")] | ||
public void TestMultiRegionSigV4a_DifferentialVerification(string regionSet, string expectedHeaderValue) | ||
{ | ||
var signer = new CrtAWS4aSigner(); | ||
var clientConfig = BuildSigningClientConfig(SigningTestService); | ||
|
||
var request = CreateDefaultRequest("/"); | ||
request.SigV4aSigningRegionSet = regionSet; | ||
|
||
var result = signer.SignRequest(request, clientConfig, null, SigningTestCredentials); | ||
|
||
// Verify x-amz-region-set header is in HTTP request with correct value | ||
Assert.True(request.Headers.ContainsKey(HeaderKeys.XAmzRegionSetHeader), | ||
$"Request must have x-amz-region-set header for region set: {regionSet}"); | ||
Assert.Equal(expectedHeaderValue, request.Headers[HeaderKeys.XAmzRegionSetHeader]); | ||
|
||
// Verify region set is in signed headers list | ||
Assert.Contains("x-amz-region-set", result.SignedHeaders); | ||
|
||
// Build expected canonical request with the specific region set | ||
var canonicalRequest = String.Join('\n', | ||
"POST", "/", "", | ||
"content-length:13", | ||
"content-type:application/x-www-form-urlencoded", | ||
"host:example.amazonaws.com", | ||
"x-amz-content-sha256:9095672bbd1f56dfc5b65f3e153adc8731a4a654192329106275f4c7b24d0b6e", | ||
"x-amz-date:20150830T123600Z", | ||
$"x-amz-region-set:{regionSet}", | ||
"", | ||
"content-length;content-type;host;x-amz-content-sha256;x-amz-date;x-amz-region-set", | ||
"9095672bbd1f56dfc5b65f3e153adc8731a4a654192329106275f4c7b24d0b6e"); | ||
|
||
var config = BuildDefaultSigningConfig(SigningTestService); | ||
config.SignatureType = AwsSignatureType.CANONICAL_REQUEST_VIA_HEADERS; | ||
config.Region = regionSet; | ||
|
||
Assert.True(AwsSigner.VerifyV4aCanonicalSigning( | ||
canonicalRequest, config, result.Signature, | ||
SigningTestEccPubX, SigningTestEccPubY), | ||
$"Signature verification failed for region set: {regionSet}"); | ||
} | ||
|
||
/// <summary> | ||
/// Tests backward compatibility: when SigV4aSigningRegionSet is not set, | ||
/// the signer should fall back to single-region behavior. | ||
/// </summary> | ||
[Fact] | ||
public void TestSigV4aFallbackToSingleRegion() | ||
{ | ||
var signer = new CrtAWS4aSigner(); | ||
var request = CreateDefaultRequest("/"); | ||
// Explicitly NOT setting request.SigV4aSigningRegionSet | ||
|
||
var clientConfig = BuildSigningClientConfig(SigningTestService); | ||
var result = signer.SignRequest(request, clientConfig, null, SigningTestCredentials); | ||
|
||
// Should fall back to us-east-1 (from SigningTestRegion) | ||
Assert.Equal(SigningTestRegion, request.DeterminedSigningRegion); | ||
Assert.True(request.Headers.ContainsKey(HeaderKeys.XAmzRegionSetHeader)); | ||
Assert.Equal(SigningTestRegion, request.Headers[HeaderKeys.XAmzRegionSetHeader]); | ||
|
||
var expectedCanonicalRequest = String.Join('\n', | ||
"POST", "/", "", | ||
"content-length:13", | ||
"content-type:application/x-www-form-urlencoded", | ||
"host:example.amazonaws.com", | ||
"x-amz-content-sha256:9095672bbd1f56dfc5b65f3e153adc8731a4a654192329106275f4c7b24d0b6e", | ||
"x-amz-date:20150830T123600Z", | ||
$"x-amz-region-set:{SigningTestRegion}", | ||
"", | ||
"content-length;content-type;host;x-amz-content-sha256;x-amz-date;x-amz-region-set", | ||
"9095672bbd1f56dfc5b65f3e153adc8731a4a654192329106275f4c7b24d0b6e"); | ||
|
||
var config = BuildDefaultSigningConfig(SigningTestService); | ||
config.SignatureType = AwsSignatureType.CANONICAL_REQUEST_VIA_HEADERS; | ||
config.Region = SigningTestRegion; | ||
|
||
Assert.True(AwsSigner.VerifyV4aCanonicalSigning( | ||
expectedCanonicalRequest, config, result.Signature, | ||
SigningTestEccPubX, SigningTestEccPubY), | ||
"Fallback signature verification failed"); | ||
} | ||
|
||
#endregion | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
{ | ||
"core": { | ||
"changeLogMessages": [ | ||
"Added ability to configure authentication scheme preferences (e.g., prioritize SigV4a over SigV4)", | ||
"Added support for AWS_AUTH_SCHEME_PREFERENCE environment variable and auth_scheme_preference configuration file setting", | ||
"Added support for AWS_SIGV4A_SIGNING_REGION_SET environment variable and sigv4a_signing_region_set profile key to configure SigV4a signing region set" | ||
], | ||
"type": "minor", | ||
"updateMinimum": true, | ||
"backwardIncompatibilitiesToIgnore": [ | ||
"Amazon.Runtime.Internal.IRequest/MethodAbstractMethodAdded", | ||
"Amazon.Runtime.IClientConfig/MethodAbstractMethodAdded", | ||
"Amazon.Runtime.IRequestContext/MethodAbstractMethodAdded" | ||
] | ||
} | ||
} | ||
Comment on lines
+1
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Public interface members were added (IClientConfig, IRequest, IRequestContext) which is a breaking change for any external implementers. Classifying this as a minor version while explicitly ignoring the incompatibilities does not align with the project's API compatibility guideline; either (a) avoid modifying existing public interfaces (e.g., use extension methods, wrapper abstractions, or default interface members with backward-compatible defaults if allowed), or (b) bump the version appropriately (major) instead of suppressing. Recommend reworking to preserve binary/source compatibility or reclassifying the dev config. Copilot generated this review using guidance from repository custom instructions. Positive FeedbackNegative Feedback |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,8 @@ public abstract partial class ClientConfig : IClientConfig | |
private string serviceURL = null; | ||
private string authRegion = null; | ||
private string authServiceName = null; | ||
private string authSchemePreference = null; | ||
private string sigV4aSigningRegionSet = null; | ||
private string clientAppId = null; | ||
private SigningAlgorithm signatureMethod = SigningAlgorithm.HmacSHA256; | ||
private bool logResponse = false; | ||
|
@@ -444,6 +446,46 @@ public string AuthenticationServiceName | |
get { return this.authServiceName; } | ||
set { this.authServiceName = value; } | ||
} | ||
|
||
/// <summary> | ||
/// Gets and sets the AuthSchemePreference property. | ||
/// A comma-separated list of authentication scheme names to use in order of preference. | ||
/// For example: "sigv4a,sigv4" to prefer SigV4a over SigV4. | ||
/// </summary> | ||
public string AuthSchemePreference | ||
{ | ||
get | ||
{ | ||
if (!string.IsNullOrEmpty(this.authSchemePreference)) | ||
return this.authSchemePreference; | ||
|
||
// Fallback to environment variable or config file: | ||
// 1. Environment variable: AWS_AUTH_SCHEME_PREFERENCE | ||
|
||
// 2. Config file: auth_scheme_preference | ||
return FallbackInternalConfigurationFactory.AuthSchemePreference; | ||
} | ||
AlexDaines marked this conversation as resolved.
Show resolved
Hide resolved
|
||
set { this.authSchemePreference = value; } | ||
} | ||
|
||
/// <summary> | ||
/// Gets and sets the SigV4aSigningRegionSet property. | ||
/// A comma-separated list of regions that a SigV4a signature will be valid for. | ||
/// Use "*" to indicate all regions. | ||
/// </summary> | ||
public string SigV4aSigningRegionSet | ||
{ | ||
get | ||
{ | ||
if (!string.IsNullOrEmpty(this.sigV4aSigningRegionSet)) | ||
return this.sigV4aSigningRegionSet; | ||
|
||
// Fallback to environment variable or config file: | ||
// 1. Environment variable: AWS_SIGV4A_SIGNING_REGION_SET | ||
// 2. Config file: sigv4a_signing_region_set | ||
return FallbackInternalConfigurationFactory.SigV4aSigningRegionSet; | ||
} | ||
AlexDaines marked this conversation as resolved.
Show resolved
Hide resolved
|
||
set { this.sigV4aSigningRegionSet = value; } | ||
} | ||
Comment on lines
502
to
551
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no integration test exercising the environment-variable or profile fallback path for SigV4aSigningRegionSet feeding into actual signed requests (x-amz-region-set header and signature variation). Current tests set the request property directly; add a test that sets AWS_SIGV4A_SIGNING_REGION_SET (and one via profile) and asserts the signer produces expected header/signature. Copilot generated this review using guidance from repository custom instructions. Positive FeedbackNegative Feedback |
||
|
||
/// <summary> | ||
/// The serviceId for the service, which is specified in the metadata in the ServiceModel. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeterminedSigningRegion is documented elsewhere as a single region, but here it may be set to a multi-region list (e.g., "us-west-2,us-east-1") or "" for SigV4a. This overloads its semantics and could break downstream logic expecting a single region string. Consider (1) keeping DeterminedSigningRegion set to a primary region (first entry when list, or null for "") and/or (2) introducing a dedicated property (already adding SigV4aSigningRegionSet) and leaving DeterminedSigningRegion for single-region semantics only, plus updating any necessary docs if multi-value is intended.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting take, I don't think we have to add another property, but we should test some requests with multiple regions and update the documentation of
DeterminedSigningRegion
to mention that it can be multiple regions.