-
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?
Conversation
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.
Pull Request Overview
This PR implements comprehensive multi-authentication scheme support for the AWS .NET SDK, enabling flexible authentication scheme configuration and prioritization with SigV4a region set support. The implementation introduces new configuration options while maintaining backwards compatibility with existing SignatureMethod patterns.
Key changes include:
- Added authentication scheme preference configuration with multiple sources (client, environment, config file, global)
- Implemented SigV4a region set configuration support
- Created comprehensive resolver infrastructure for authentication scheme prioritization
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
sdk/test/UnitTests/Custom/Runtime/SigV4aRegionSetConfigurationTests.cs | Comprehensive test coverage for SigV4a region set configuration including validation, parsing, and source precedence |
sdk/test/UnitTests/Custom/Runtime/AuthSchemeTests.cs | Unit tests for AuthScheme class covering predefined schemes, equality, and validation |
sdk/test/UnitTests/Custom/Runtime/AuthSchemeResolverTests.cs | Tests for DefaultAuthSchemeResolver including preference application and configuration hierarchy |
sdk/test/UnitTests/Custom/Runtime/AuthSchemePreferenceTests.cs | Test coverage for AuthSchemePreference parsing, validation, and case-sensitive matching |
sdk/test/UnitTests/Custom/Runtime/AuthSchemeIntegrationTests.cs | Integration tests for auth scheme preference application in BaseAuthResolverHandler |
sdk/test/UnitTests/Custom/Runtime/AuthSchemeIntegrationSimpleTests.cs | Basic integration tests for AuthSchemeOption constants and simple resolver functionality |
sdk/test/UnitTests/Custom/Runtime/AuthSchemeConfigurationTests.cs | Tests for configuration resolution from environment variables and global settings |
sdk/test/UnitTests/Custom/Runtime/AuthSchemeBackwardsCompatibilityTests.cs | Backwards compatibility tests ensuring legacy SignatureMethod behavior is preserved |
sdk/test/NetStandard/UnitTests/ClientConfigTests.cs | Updated client config property list to include new authentication scheme properties |
sdk/src/Core/Amazon.Util/Internal/RootConfig.cs | Added AuthSchemePreference and SigV4aRegionSetConfiguration to root configuration |
sdk/src/Core/Amazon.Runtime/SigV4aRegionSetConfiguration.cs | Implementation of SigV4a region set configuration with multiple source support |
sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseAuthResolverHandler.cs | Enhanced auth resolution with preference application and scheme conversion logic |
sdk/src/Core/Amazon.Runtime/Internal/Util/SafeConfigurationResolver.cs | Utility for safe configuration resolution with consistent error handling |
sdk/src/Core/Amazon.Runtime/Internal/Util/HashCodeHelper.cs | Helper utility for consistent hash code generation across SDK types |
sdk/src/Core/Amazon.Runtime/Internal/Settings/SettingsConstants.cs | Added constants for new authentication scheme configuration keys |
sdk/src/Core/Amazon.Runtime/IClientConfig.cs | Interface updates to accommodate new authentication scheme properties |
sdk/src/Core/Amazon.Runtime/IAuthSchemeResolver.cs | Interface definition for authentication scheme resolution |
sdk/src/Core/Amazon.Runtime/EnvironmentConfigurationProvider.cs | Provider for reading authentication scheme configuration from environment variables |
sdk/src/Core/Amazon.Runtime/DefaultAuthSchemeResolver.cs | Default implementation of auth scheme resolution with preference-based prioritization |
sdk/src/Core/Amazon.Runtime/ClientConfigExtensions.cs | Extension methods for backwards-compatible access to new authentication properties |
sdk/src/Core/Amazon.Runtime/ClientConfig.cs | Updated ClientConfig with new authentication scheme properties and legacy compatibility tracking |
sdk/src/Core/Amazon.Runtime/AuthSchemePreference.cs | Implementation of authentication scheme preference list with parsing and validation |
sdk/src/Core/Amazon.Runtime/AuthSchemeConfigurationResolver.cs | Central resolver for authentication scheme configuration with precedence hierarchy |
sdk/src/Core/Amazon.Runtime/AuthScheme.cs | Core AuthScheme class with predefined schemes and equality implementation |
sdk/src/Core/AWSConfigs.cs | Global configuration properties for authentication scheme preferences and SigV4a region sets |
generator/.DevConfigs/3aa6313d-9526-40ba-b09c-e046e0d4ef2f.json | Development configuration for minor version release with detailed changelog |
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.
Pull Request Overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
generator/.DevConfigs/3aa6313d-9526-40ba-b09c-e046e0d4ef2f.json:10
- The dev config correctly specifies a minor version bump for this new feature addition, which aligns with the non-breaking nature of the changes.
"type": "minor",
sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseAuthResolverHandler.cs
Outdated
Show resolved
Hide resolved
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.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseAuthResolverHandler.cs
Outdated
Show resolved
Hide resolved
5275bbd
to
6291c96
Compare
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.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
new AuthSchemeOption { SchemeId = AuthSchemeOption.SigV4A } | ||
}; | ||
Environment.SetEnvironmentVariable("AWS_AUTH_SCHEME_PREFERENCE", "sigv4a,sigv4"); | ||
Amazon.Runtime.Internal.FallbackInternalConfigurationFactory.Reset(); // Reset to pick up new env var |
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.
The call to FallbackInternalConfigurationFactory.Reset() is duplicated across multiple test methods. Consider extracting this to a helper method or including it in TestInitialize to reduce code duplication and improve maintainability.
Copilot uses AI. Check for mistakes.
catch (ArgumentException ex) | ||
{ | ||
Logger.Error(ex, "Failed to apply auth scheme preferences due to invalid argument, falling back to service-defined order"); | ||
return authOptions; | ||
} | ||
catch (InvalidOperationException ex) | ||
{ | ||
Logger.Error(ex, "Failed to apply auth scheme preferences due to invalid operation, falling back to service-defined order"); | ||
return authOptions; | ||
} |
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.
The exception handling blocks have identical behavior and nearly identical error messages. Consider consolidating into a single catch block for Exception with a generic error message, or use a helper method to reduce code duplication.
catch (ArgumentException ex) | |
{ | |
Logger.Error(ex, "Failed to apply auth scheme preferences due to invalid argument, falling back to service-defined order"); | |
return authOptions; | |
} | |
catch (InvalidOperationException ex) | |
{ | |
Logger.Error(ex, "Failed to apply auth scheme preferences due to invalid operation, falling back to service-defined order"); | |
return authOptions; | |
} | |
catch (Exception ex) | |
{ | |
Logger.Error(ex, "Failed to apply auth scheme preferences, falling back to service-defined order"); | |
return authOptions; | |
} |
Copilot uses AI. Check for mistakes.
var parts = schemeId.Split('#'); | ||
return parts.Length > 1 ? parts[1] : schemeId; |
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.
The Split('#') operation allocates a new array on each call. For better performance, consider using string.IndexOf('#') and string.Substring() to extract the portion after '#' without array allocation, especially since this method may be called frequently during auth scheme resolution.
var parts = schemeId.Split('#'); | |
return parts.Length > 1 ? parts[1] : schemeId; | |
int hashIndex = schemeId.IndexOf('#'); | |
return hashIndex >= 0 && hashIndex < schemeId.Length - 1 | |
? schemeId.Substring(hashIndex + 1) | |
: schemeId; |
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 seems like a good suggestion to accept.
/// <summary> | ||
/// Gets a simple string value from an environment variable. | ||
/// </summary> | ||
private string GetStringEnvironmentVariable(string name) |
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.
Does the SEP say anything about trimming the value? There's already a GetEnvironmentVariable<T>
method that could be re-used (T
for the new variable would be string
).
{ | ||
"core": { | ||
"changeLogMessages": [ | ||
"Implement multi-auth SEP with authentication scheme preference and SigV4a region set configuration", |
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.
For the final changelog, we will want to refactor these (SEP doesn't mean anything to customers - and even internal terms like auth resolver are not relevant).
We should focus on the actual impact - e.g. ability to set SigV4A explicitly.
var parts = schemeId.Split('#'); | ||
return parts.Length > 1 ? parts[1] : schemeId; |
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 seems like a good suggestion to accept.
/// This is used for backwards compatibility to determine when legacy SignatureMethod configuration | ||
/// should take precedence over auth scheme preferences. | ||
/// </summary> | ||
public bool IsSignatureMethodExplicitlySet |
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.
I don't follow why this is needed... The naming is confusing but signature method is not going to be SigV4 / SigV4A, it's this enum instead:
aws-sdk-net/sdk/src/Core/Amazon.Runtime/Enumerations.cs
Lines 24 to 28 in 0980c7c
public enum SigningAlgorithm | |
{ | |
HmacSHA1, | |
HmacSHA256 | |
}; |
CorrectForClockSkew = true; | ||
DisableLegacyPersistenceStore = AWSConfigs._disableLegacyPersistenceStore; | ||
|
||
// Authentication configuration is resolved on-demand from environment and config files |
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.
Out-of-place comment?
// Check for legacy SignatureMethod configuration (backwards compatibility for S3) | ||
if (clientConfig?.SignatureMethod != null && clientConfig.IsSignatureMethodExplicitlySet) | ||
{ | ||
bool isS3Service = clientConfig.ServiceId?.Equals("S3", StringComparison.OrdinalIgnoreCase) == true || |
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.
(I left the other comment first...) I see the SEP mentions S3 (and Control) but I don't think this logic is needed here (the SEP was referring how the service model - which the auth resolvers are generated from - would keep older properties with the same values for backwards compatibility).
/// endpoints metadata, or fall back to the client's configured region. | ||
/// </para> | ||
/// </summary> | ||
public string SigV4aSigningRegionSet { get; set; } |
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.
Unless I'm missing something obvious... Where is this used? I don't see it referenced in the auth resolver handler (and also for legacy reasons, we have SigV4A logic in the endpoint resolver too...)
6291c96
to
694cfae
Compare
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.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseAuthResolverHandler.cs
Outdated
Show resolved
Hide resolved
694cfae
to
77ae30f
Compare
…lti-region signing
77ae30f
to
5a8d2fb
Compare
… reference exceptions in auth resolution logging. nit: adjust comments to customer audience
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.
Pull Request Overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
generator/.DevConfigs/3aa6313d-9526-40ba-b09c-e046e0d4ef2f.json
Outdated
Show resolved
Hide resolved
sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseAuthResolverHandler.cs
Show resolved
Hide resolved
sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseAuthResolverHandler.cs
Show resolved
Hide resolved
…al testing for multi-region SigV4a
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.
Pull Request Overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
{ | ||
regionSet = AWS4Signer.DetermineSigningRegion(clientConfig, clientConfig.RegionEndpointServiceName, request.AlternateEndpoint, request); | ||
} | ||
request.DeterminedSigningRegion = regionSet; |
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.
request.DeterminedSigningRegion = regionSet; | |
// Set DeterminedSigningRegion to a single region (first entry if list, null if "*") | |
if (regionSet == "*") | |
{ | |
request.DeterminedSigningRegion = null; | |
request.SigV4aSigningRegionSet = regionSet; | |
} | |
else if (regionSet.Contains(",")) | |
{ | |
var regions = regionSet.Split(','); | |
request.DeterminedSigningRegion = regions[0].Trim(); | |
request.SigV4aSigningRegionSet = regionSet; | |
} | |
else | |
{ | |
request.DeterminedSigningRegion = regionSet; | |
request.SigV4aSigningRegionSet = null; | |
} |
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.
// Clear AuthenticationRegion to avoid confusion with SigV4 single-region | ||
requestContext.Request.AuthenticationRegion = null; |
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.
[nitpick] Clearing AuthenticationRegion removes potentially useful endpoint metadata that other pipeline components may still rely on for diagnostics or fallback behavior. Prefer retaining the original value and relying on the newly introduced SigV4aSigningRegionSet for multi-region contexts, or only clear it after verifying no other consumers read it. If disambiguation is required, consider updating documentation instead of nulling.
// Clear AuthenticationRegion to avoid confusion with SigV4 single-region | |
requestContext.Request.AuthenticationRegion = null; | |
// Retain AuthenticationRegion for diagnostics/fallback; SigV4aSigningRegionSet is used for multi-region contexts. |
Copilot uses AI. Check for mistakes.
/// <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; | ||
} | ||
set { this.sigV4aSigningRegionSet = value; } | ||
} |
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.
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.
/// <summary> | ||
/// Gets 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> | ||
string AuthSchemePreference { get; } | ||
|
||
/// <summary> | ||
/// Gets the SigV4aSigningRegionSet property. | ||
/// A comma-separated list of regions that a SigV4a signature will be valid for. | ||
/// Use "*" to indicate all regions. | ||
/// </summary> | ||
string SigV4aSigningRegionSet { get; } |
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.
[nitpick] Consider explicitly documenting the corresponding environment variables (AWS_AUTH_SCHEME_PREFERENCE, AWS_SIGV4A_SIGNING_REGION_SET) and profile keys (auth_scheme_preference, sigv4a_signing_region_set) in these summaries to improve discoverability for users configuring these options outside code.
Copilot uses AI. Check for mistakes.
private static string GetFullSchemeId(string schemeName) | ||
{ | ||
// Handle common auth scheme names (case-sensitive as per specification) | ||
if (schemeName == "sigv4") | ||
return "aws.auth#sigv4"; | ||
if (schemeName == "sigv4a") | ||
return "aws.auth#sigv4a"; | ||
if (schemeName == "httpBearerAuth" || schemeName == "bearer") | ||
return "smithy.api#httpBearerAuth"; | ||
if (schemeName == "noAuth") | ||
return "smithy.api#noAuth"; | ||
|
||
// If it already looks like a full ID (contains #), return as-is | ||
if (schemeName.Contains("#")) | ||
return schemeName; | ||
|
||
// Otherwise, assume it's an AWS auth scheme | ||
return $"aws.auth#{schemeName}"; | ||
} |
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.
[nitpick] Preference matching is case-sensitive—user input like "SigV4A" or "SIGV4" will be silently ignored. To reduce configuration friction, consider normalizing to lower-invariant before comparisons (and documenting case sensitivity if intentionally strict).
Copilot uses AI. Check for mistakes.
extensions/src/AWSSDK.Extensions.CrtIntegration/CrtAWS4aSigner.cs
Outdated
Show resolved
Hide resolved
extensions/src/AWSSDK.Extensions.CrtIntegration/CrtAWS4aSigner.cs
Outdated
Show resolved
Hide resolved
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 |
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.
nit: multiple ternary operator is is bit hard to read for me, I think simple if conditions is more readable.
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.
I actually just swapped it to be ternary because the branching looked quite messy 😅
{ | ||
// 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 |
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.
Multiple comments are updated in this file, is this intentional?
{ | ||
regionSet = AWS4Signer.DetermineSigningRegion(clientConfig, clientConfig.RegionEndpointServiceName, request.AlternateEndpoint, request); | ||
} | ||
request.DeterminedSigningRegion = regionSet; |
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.
/// Creates a test request with mutable collections to verify header modifications | ||
/// during signing. | ||
/// </summary> | ||
internal static IRequest CreateDefaultRequest(string resourcePath) |
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.
Is there a reason to create this method instead of using BuildHeaderRequestToSign
?
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.
We need the real DefaultRequest because CRT modifies the headers directly during signing. The mock version returns an immutable dictionary so the CRT can't add headers to it. The tests check that those headers actually got added to the request
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.
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.
I'm still not sure we actually need to change any tests in the extensions folder.
/// | ||
/// </summary> | ||
[Fact] | ||
public void TestMultiRegionSigV4a_DifferentialVerification() |
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.
I think we should use InlineData
for the three test cases that are tested here, similar to SignRequestViaHeadersWithSigv4a
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.
Pull Request Overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
{ | ||
"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" | ||
] | ||
} | ||
} No newline at end of file |
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.
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.
/// <summary> | ||
/// Gets 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> | ||
string AuthSchemePreference { get; } | ||
|
||
/// <summary> | ||
/// Gets the SigV4aSigningRegionSet property. | ||
/// A comma-separated list of regions that a SigV4a signature will be valid for. | ||
/// Use "*" to indicate all regions. | ||
/// </summary> | ||
string SigV4aSigningRegionSet { get; } |
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.
Adding new members to a public interface is a breaking change for consumers implementing IClientConfig. To maintain backward compatibility per project guidelines, consider: (1) implementing these as extension methods over an internal accessor, (2) introducing a new derived interface while keeping the original stable, or (3) using default interface implementations (only if all supported target frameworks allow it) with safe defaults. Current approach requires a recompilation and will break third-party custom clients.
Copilot generated this review using guidance from repository custom instructions.
/// The signing region set to use for SigV4a requests. | ||
/// Contains a comma-separated list of regions for multi-region signing. | ||
/// Set from Config.SigV4aSigningRegionSet or endpoints metadata. | ||
/// </summary> | ||
string SigV4aSigningRegionSet { get; set; } | ||
|
||
/// <summary> | ||
/// The region or region set used for signing the service request. | ||
/// For standard SigV4 signing, this contains a single region (e.g., "us-west-2"). | ||
/// For SigV4a multi-region signing, this can be a comma-separated list of regions (e.g., "us-west-2,us-east-1") |
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.
Adding SigV4aSigningRegionSet to IRequest is a breaking interface change. Recommend introducing this via a wrapper (e.g., an associated metadata container), an opt-in capability interface, or an extension method pattern instead of modifying an existing widely-implemented interface to preserve compatibility.
Copilot generated this review using guidance from repository custom instructions.
|
||
/// <summary> | ||
/// The region set for SigV4a signing. | ||
/// </summary> | ||
string SigV4aSigningRegionSet { get; set; } |
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.
Adding SigV4aSigningRegionSet to IRequestContext modifies a public interface and is a breaking change. Consider an alternative (e.g., a context extension dictionary key, an auxiliary interface, or extension methods) to maintain backward compatibility.
Copilot generated this review using guidance from repository custom instructions.
private static List<IAuthSchemeOption> ApplyAuthSchemePreference(List<IAuthSchemeOption> authOptions, IClientConfig clientConfig) | ||
{ | ||
var preferenceList = clientConfig.AuthSchemePreference; | ||
if (string.IsNullOrEmpty(preferenceList)) | ||
{ | ||
return authOptions; | ||
} | ||
|
||
// Parse the preference list (comma-separated, trimming spaces and tabs between names) | ||
var preferences = preferenceList | ||
.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries) | ||
.Select(s => s.Trim(' ', '\t')) // Trim spaces and tabs between auth scheme names | ||
.Where(s => !string.IsNullOrEmpty(s)) | ||
.ToList(); | ||
|
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.
[nitpick] Auth scheme preference parsing and reordering executes on every request; repeated string Split/Trim and list reconstruction could be avoided by caching a parsed preference list per client instance (e.g., compute once on first access or when AuthSchemePreference changes). Additionally, this private static is tested via reflection (AuthSchemePreferenceTests), which is brittle—consider making it internal and using InternalsVisibleTo for the test assembly or integrating logic into a protected virtual method so tests exercise behavior indirectly.
private static List<IAuthSchemeOption> ApplyAuthSchemePreference(List<IAuthSchemeOption> authOptions, IClientConfig clientConfig) | |
{ | |
var preferenceList = clientConfig.AuthSchemePreference; | |
if (string.IsNullOrEmpty(preferenceList)) | |
{ | |
return authOptions; | |
} | |
// Parse the preference list (comma-separated, trimming spaces and tabs between names) | |
var preferences = preferenceList | |
.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries) | |
.Select(s => s.Trim(' ', '\t')) // Trim spaces and tabs between auth scheme names | |
.Where(s => !string.IsNullOrEmpty(s)) | |
.ToList(); | |
// Cache for parsed preference list per client config instance | |
private static readonly object _preferenceCacheLock = new object(); | |
// Use a dictionary to cache parsed preferences per client config instance | |
private static readonly Dictionary<IClientConfig, Tuple<string, List<string>>> _preferenceCache = new Dictionary<IClientConfig, Tuple<string, List<string>>>(); | |
/// <summary> | |
/// Applies the configured auth scheme preference to reorder the auth options. | |
/// </summary> | |
internal static List<IAuthSchemeOption> ApplyAuthSchemePreference(List<IAuthSchemeOption> authOptions, IClientConfig clientConfig) | |
{ | |
var preferenceList = clientConfig.AuthSchemePreference; | |
List<string> preferences; | |
lock (_preferenceCacheLock) | |
{ | |
if (_preferenceCache.TryGetValue(clientConfig, out var cached) && cached.Item1 == preferenceList) | |
{ | |
preferences = cached.Item2; | |
} | |
else | |
{ | |
if (string.IsNullOrEmpty(preferenceList)) | |
{ | |
preferences = new List<string>(); | |
} | |
else | |
{ | |
preferences = preferenceList | |
.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries) | |
.Select(s => s.Trim(' ', '\t')) | |
.Where(s => !string.IsNullOrEmpty(s)) | |
.ToList(); | |
} | |
_preferenceCache[clientConfig] = Tuple.Create(preferenceList, preferences); | |
} | |
} |
Copilot uses AI. Check for mistakes.
} | ||
} | ||
|
||
// CRITICAL: Ensure NoAuth/Anonymous is always last in the list. |
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.
[nitpick] Auth scheme preference parsing and reordering executes on every request; repeated string Split/Trim and list reconstruction could be avoided by caching a parsed preference list per client instance (e.g., compute once on first access or when AuthSchemePreference changes). Additionally, this private static is tested via reflection (AuthSchemePreferenceTests), which is brittle—consider making it internal and using InternalsVisibleTo for the test assembly or integrating logic into a protected virtual method so tests exercise behavior indirectly.
Copilot uses AI. Check for mistakes.
…figTests KNOWN_PROPERTIES
…ed for CRT compatibility
/// Creates a test request with mutable collections to verify header modifications | ||
/// during signing. | ||
/// </summary> | ||
internal static IRequest CreateDefaultRequest(string resourcePath) |
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.
I'm still not sure we actually need to change any tests in the extensions folder.
/// Contains a comma-separated list of regions for multi-region signing. | ||
/// Set from Config.SigV4aSigningRegionSet or endpoints metadata. | ||
/// </summary> | ||
public string SigV4aSigningRegionSet { get; set; } |
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.
I don't think this is needed. We should be able to leverage the existing logic for SigV4A without adding new properties here.
{ | ||
// Current auth scheme option is not enabled / supported, continue iterating. | ||
Logger.DebugFormat($"{authOptions[i].SchemeId} scheme is not supported for {executionContext.RequestContext.RequestName}"); | ||
Logger?.DebugFormat($"{authOptions[i].SchemeId} scheme is not supported for {executionContext.RequestContext.RequestName}"); |
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.
Why this change? Logger
won't be null.
} | ||
} | ||
|
||
// CRITICAL: Ensure NoAuth/Anonymous is always last in the list. |
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.
I thought we discussed this, but if we didn't: This is not necessary.
return schemeName; | ||
|
||
// Otherwise, assume it's an AWS auth scheme | ||
return $"aws.auth#{schemeName}"; |
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.
We should not be assuming anything here. I think the logic could be a lot simpler by comparing the scheme name from preferences with the supported value from the model.
} | ||
|
||
// Set SigV4a region set if configured (either from endpoint metadata or explicit config) | ||
if (requestContext.Request.SignatureVersion == SignatureVersion.SigV4a) |
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.
Similar to my other comment: This should use the existing signing region set logic.
/// <summary> | ||
/// The region set for SigV4a signing. | ||
/// </summary> | ||
public string SigV4aSigningRegionSet { get; set; } |
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.
Can probably be removed too.
return this.authSchemePreference; | ||
|
||
// Fallback to environment variable or config file: | ||
// 1. Environment variable: AWS_AUTH_SCHEME_PREFERENCE |
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.
These comments are redundant. I don't expect the variable name to ever change but these comments don't add any value.
"ResponseChecksumValidation", | ||
"IdentityResolverConfiguration", | ||
"DefaultAWSCredentials" | ||
"DefaultAWSCredentials", |
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.
I should've noticed this before: When this test fails, it mentions other places that should change too (e.g. AWSSDK.Extensions.NETCore.Setup
- see the comment on line 97)
|
||
/// <summary> | ||
/// Test implementation of BaseAuthResolverHandler that provides test auth options. | ||
/// This simulates how endpoints 2.0 auth schemes override service defaults. |
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.
I'm being annoying again, but EP2.0 does not matter here. All of these tests seem more redundant than anything.
Description
Implements multi-auth scheme support. This enables clients to configure and prioritize the available authentication schemes (SigV4, SigV4a, Bearer, NoAuth) through multiple configuration sources.
Key changes made:
AuthScheme
,AuthSchemePreference
, and resolver infrastructureMotivation and Context
Required for upcoming SigV4a adoption and services that need flexible auth scheme selection. Several teams have been waiting on this capability for cross-region signing and failover scenarios.
Testing
.NET Dryrun Build ID: 845e71d4-3ab2-4488-ad9d-9cf0365df798
PS Dryrun Build ID: d483576d-fb7d-436c-acb8-ef12dbfdbdce
Types of changes
Checklist
License