-
Notifications
You must be signed in to change notification settings - Fork 874
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
Override RegionSet in EnpointResolverInterceptor after fetching the Signing Properties from Endpoint rules #5825
Override RegionSet in EnpointResolverInterceptor after fetching the Signing Properties from Endpoint rules #5825
Conversation
…igning Properties from Endpoint rules
e5fb5b7
to
5953fdf
Compare
@@ -187,8 +201,7 @@ public void async_endpointProviderReturnsHeaders_appendedToExistingRequest() { | |||
assertThat(interceptor.context.httpRequest().matchingHeaders("TestHeader")).containsExactly("TestValue", "TestValue0"); | |||
} | |||
|
|||
// TODO(sra-identity-auth): Enable for useSraAuth=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.
Uncommented these test cases
@@ -74,7 +74,8 @@ public static Metadata constructMetadata(ServiceModel serviceModel, | |||
.withJsonVersion(serviceMetadata.getJsonVersion()) | |||
.withEndpointPrefix(serviceMetadata.getEndpointPrefix()) | |||
.withSigningName(serviceMetadata.getSigningName()) | |||
.withAuthType(AuthType.fromValue(serviceMetadata.getSignatureVersion())) | |||
.withAuthType(serviceMetadata.getSignatureVersion() != 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.
When can this be 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.
The specifications states
The authType and signatureVersion traits will be deprecated in favor of the auth and unsignedPayload traits.
Thus for new services which will be completed based on multi-auth signatureVersion
will be null.
I added a test case for in codegen-tst when a new service is added with just multi-auth supporting only sigv4a
@@ -219,7 +227,11 @@ private MethodSpec modifyRequestMethod(String endpointAuthSchemeStrategyFieldNam | |||
SelectedAuthScheme.class, SdkInternalExecutionAttribute.class); | |||
b.beginControlFlow("if (endpointAuthSchemes != null && selectedAuthScheme != 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.
Is selectedAuthScheme
ever null
nowadays? It feels like this should be a dead code branch now?
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.
Removed this check
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.
My bad , we need this check , the S3 integ test failed , will revert this
java.lang.NullPointerException: Cannot invoke "software.amazon.awssdk.core.SelectedAuthScheme.authSchemeOption()" because "selectedAuthScheme" is null
at software.amazon.awssdk.services.s3.endpoints.internal.S3ResolveEndpointInterceptor.authSchemeWithEndpointSignerProperties(S3ResolveEndpointInterceptor.java:1383)
at software.amazon.awssdk.services.s3.endpoints.internal.S3ResolveEndpointInterceptor.modifyRequest(S3ResolveEndpointInterceptor.java:177)
at software.amazon.awssdk.core.interceptor.ExecutionInterceptorChain.modifyRequest(ExecutionInterceptorC
if (multiAuthSigv4a) { | ||
code.beginControlFlow("if (!hasRegionSet(selectedAuthScheme) && v4aAuthScheme.signingRegionSet() != null)"); | ||
} else { | ||
code.beginControlFlow("if (v4aAuthScheme.signingRegionSet() != 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.
hasRegionSet
seems like a relatively small method. Same with updateAuthSchemeWithRegionSet
. Do we want to just always generate those methods, so that we don't have to do this branching in the code generator? It feels simpler to have a single code path regardless of sigv4a or sigv4 (both in the code generator, and in the generated code).
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 have made updateAuthSchemeWithRegionSet
and hasRegionSet
inline. The reason I added a specific check for multiAuthSigv4a
is because for non-Sigv4a existing features, that check is dead code and might confuse readers of generated packages while debugging or during code walks.
if (!hasRegionSet(selectedAuthScheme) && v4aAuthScheme.signingRegionSet() != null) { | ||
RegionSet regionSet = RegionSet.create(v4aAuthScheme.signingRegionSet()); | ||
option.putSignerProperty(AwsV4aHttpSigner.REGION_SET, 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.
If I'm remembering the priorities right from the discussion with Alex, the endpoint provider's setting takes precedence over the auth scheme resolver's setting. This looks like we're favoring the auth scheme resolver. Am I misreading or misremembering?
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 discussion covered two distinct concepts: Signing Region and RegionSet. The current implementation specifically addresses SigV4a RegionSet, where Endpoint Parameters have the lowest precedence according to the Multi-auth specification. The precedence order is documented in the attached table.
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 table doesn't include the auth scheme resolver. In the discussion with Alex, when we added that, endpoint resolver came out higher than auth scheme resolver.
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.
Updated a comment
|
3001dad
into
feature/master/multi-auth-sigv4a
* Add support for Multi Auth Sigv4a for Service metadata (#5761) * Add support for Multi Auth Sigv4a for Service metadata * handled comments * Sigv4aSigningRegionSetProvider and updating Execution Attributes (#5762) * Sigv4aSigningRegionSetProvider and updating Execution Attributes * Fixed typos * Handled comments * extra space * Handled comments * Handled comments to make RegionSet Provider as Internal API * Update AuthScemeParams with RegionSet for Sigv4a auth Scheme (#5766) * Codegen changes for AuthSchemeInterceptorSpecs to update the RegionSet from Execution Attributes (#5768) * Update AuthScemeParams with RegionSet for Sigv4a auth Scheme * Update Codegen AuthSchemeInterceptorSpec to update RegionSet for AuthSchemeParams * updated method name * Add sigv4aRegionSet to Client Builder for Multi-Auth Services Supporting Sigv4a (#5772) * Update AuthScemeParams with RegionSet for Sigv4a auth Scheme * Update Codegen AuthSchemeInterceptorSpec to update RegionSet for AuthSchemeParams * updated method name * Adding sigv4aResionSet client builder for services which has Sigv4a in Multi Auth trait * Adding Codegen support for unsignedPayload model trait for multi-auth auth schemes (#5776) * Update AuthScemeParams with RegionSet for Sigv4a auth Scheme * Update Codegen AuthSchemeInterceptorSpec to update RegionSet for AuthSchemeParams * updated method name * Adding sigv4aResionSet client builder for services which has Sigv4a in Multi Auth trait * Adding Codegen support for unsignedPayload model trait for multi-auth auth schemes * Adding Codegen support for unsignedPayload model trait for multi-auth auth schemes * Adding Codegen support for unsignedPayload model trait for multi-auth auth schemes * Handled comments * updated variable names * Remove `useMultiAuth` customization (#5800) * Remove multiAuth customization * Handle cases where AuthType gets updated from Legacy signature version * Override RegionSet in EnpointResolverInterceptor after fetching the Signing Properties from Endpoint rules (#5825) * Override RegionSet in EnpointResolverInterceptor after fetching the Signing Properties from Endpoint rules * Endpoint Resolver Spec test added * Add test case for service with MultiAuth only and not using Sigv4 * checkstyle fixed * Handled comments * Updated after Integ test failure and some comments * Resolve Sigv4a regionset from Client configs if Endpoint-Based-Auth legacy modules doesnot have it defined in ruleset (#5860) * Override RegionSet in EnpointResolverInterceptor after fetching the Signing Properties from Endpoint rules * Endpoint Resolver Spec test added * Add test case for service with MultiAuth only and not using Sigv4 * checkstyle fixed * Handled comments * Updated after Integ test failure and some comments * Sigv4a Regionset updated for Endpoint-based-Auth Legacy services * Added endpoint-resolve-interceptor-with-endpointsbasedauth.java * Added test cases to check runtime resolution of RegionSet * Updated test cases to make sure we check Empty set * fixed test build * Changed precedence after internal discussion * Renamed API name `sigv4aRegionSet` to `sigv4aSigningRegionSet` to be conssistent with Environment variable and System Property name for this option (#5867) * Changed API names sigv4aRegionSet to sigv4aSigningRegionSet to be consistent with Environmant variable and System Property name for this option * Fixed checkstyle issues * Added S3 junit with wiremock tests for Mult Auth Sigv4a (#5872)
Motivation and Context
AwsClientOption.AWS_SIGV4A_SIGNING_REGION_SET
weresolveSigv4aRegionSet
in folllowing orderAwsExecutionAttribute.AWS_SIGV4A_SIGNING_REGION_SET
from AwsClientOption.AWS_SIGV4A_SIGNING_REGION_SETauthSchemeParams
from above and then can callresolveAuthScheme
where the Auth option is updated with this optionModifications
Testing
Screenshots (if appropriate)
Appendix
The Sigv4a signing region set should be resolved based on below specifications
Sigv4a signing region set resolution
Configuration follows the same resolution order as other client configuration
options, where more specific configuration is preferred over less specific. The
following table shows cases that demonstrate this. Each column except for
Result
shows a source for the signing region set, andResult
is the valuethat should be used. Note that
*
indcates the request is valid in all regions.License