-
Notifications
You must be signed in to change notification settings - Fork 863
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
Update request pipeline to use the AuthSchemeResolver to resolve the identity and signer #3552
Update request pipeline to use the AuthSchemeResolver to resolve the identity and signer #3552
Conversation
} | ||
} | ||
|
||
protected virtual AbstractAWSSigner GetSigner(IAuthScheme<BaseIdentity> scheme) |
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.
Should this return ISigner
. AbstractAWSSigner
seems tired to a SigV4(a) signer.
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.
All the signers do implement AbstractAWSSigner
, and this is the type of executionContext.RequestContext.Signer
so we will have to cast it again anyway.
Changing executionContext.RequestContext.Signer
to ISigner
wont be very simple since we do depend on signer referencing core internal types that need to be extracted / refactored (e.g. IRequest).
/// <inheritdoc/> | ||
public ISigner Signer() | ||
{ | ||
return new NullSigner(); |
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 each of these schemes should the signer be a member variable so we can reuse the same instance instead of recreating the signer for every 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.
Will add a static readonly variable to cache the signer.
// var identityResolver = scheme.IdentityResolver(); | ||
// var identity = identityResolver.GetIdentity(); | ||
// var signer = scheme.Signer(); | ||
executionContext.RequestContext.Identity = this.Identity; |
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.
Doesn't this have the problem that if a service client has some operations that take AWS SigV4 credentials and others that take a token we will always use the identity passed into the constructor for the service client. Should we only use the identity passed into the constructor for the operations that support that type. Otherwise use the resolver.
On that note, if a service does use both SigV4 and Token how do you configure the service client for both?
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.
Yep I did miss that, will rename this.Identity
to DefaultAWSCredentials
and make sure that we use it for SigV4/SigV4a.
For the other note, we didn't allow the users to provide a Token when creating the client, however the could provide IAWSTokenProvider
implementation, which should be respected when implementing SSO, right @dscpinheiro?
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.
Yes, they could provide an implementation for the token provider property.
Although I'm adding (in a separate PR) a default token identity resolver that should always be available. So my understanding is if an operation specifies both token and SigV4 as supported auth schemes we'll be able to resolve them automatically.
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.
@dscpinheiro I mean we should check if the user defined a token provider and use it before using the new default token identity 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.
@dscpinheiro I mean we should check if the user defined a token provider and use it before using the new default token identity resolver.
Sorry, the second sentence was replying to Norm's question (On that note, if a service does use both SigV4 and Token how do you configure the service client for both?)
sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseAuthResolverHandler.cs
Show resolved
Hide resolved
if (Credentials != null && !(Credentials is AnonymousAWSCredentials)) | ||
var identity = executionContext.RequestContext.Identity as AWSCredentials; | ||
|
||
if (identity != null && !(identity is AnonymousAWSCredentials)) |
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.
Wondering if we should update AnonymousAWSCredentials
to return null
for GetCredentialsAsync
so we can avoid having these special cases.
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.
It should be even simpler than that, we can just return null
for GetCredentials
and GetCredentialsAsync
has an implementation in the base class that just uses GetCredentials
and we don't need to change that.
Now this made me think that we shouldn't need the BearerTokenSigner
special case too that we have above, since the signer cannot be BearerTokenSigner
when the Identity is AWSCredentials
.
// var identityResolver = scheme.IdentityResolver(); | ||
// var identity = identityResolver.GetIdentity(); | ||
// var signer = scheme.Signer(); | ||
executionContext.RequestContext.Identity = this.Identity; |
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.
Yes, they could provide an implementation for the token provider property.
Although I'm adding (in a separate PR) a default token identity resolver that should always be available. So my understanding is if an operation specifies both token and SigV4 as supported auth schemes we'll be able to resolve them automatically.
80f0ae4
to
ff810d1
Compare
Co-authored-by: Daniel Pinheiro <[email protected]>
Co-authored-by: Daniel Pinheiro <[email protected]>
@@ -36,7 +37,9 @@ public IIdentityResolver GetIdentityResolver(IIdentityResolverConfiguration conf | |||
/// <inheritdoc/> | |||
public ISigner Signer() | |||
{ | |||
return new AWS4aSignerCRTWrapper(); | |||
if (_signer == 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.
Any reason why this one doesn't assign the value where the field is declared? Is it because it'll try to load the CRT dependency?
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.
Yep, which will cause an exception if the user doesn't have the CRT dependency even if this service doesn't need it.
So unless we call this signer we shouldn't create an instance of AWS4aSignerCRTWrapper
.
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.
Hm, that makes me think we should update the BaseAuthResolverHandler
to also check if the CRT is available when there are multiple auth schemes and one of them is SigV4A (in that case, we should skip SigV4A and try to use one of the other auth schemes).
This can be done in a separate PR; Here's what the endpoints resolver does:
aws-sdk-net/sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseEndpointResolver.cs
Lines 166 to 176 in 816c44e
case "sigv4a": | |
{ | |
// If there are multiple authentication schemes but the CRT dependency is not available, | |
// we will proceed to check the next value in authSchemes. | |
if (hasMultipleSchemes) | |
{ | |
if (!IsCrtDependencyAvailable()) | |
{ | |
continue; | |
} | |
} |
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.
One small comment and then I'm good.
public ISigner Signer() | ||
{ | ||
if (_signer == null) | ||
_signer = new AWS4aSignerCRTWrapper(); |
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.
To avoid weird threading issues can we use the Interlock class for this lazy setting.
Interlocked.Exchange(ref _signer, new AWS4aSignerCRTWrapper());
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.
…identity and signer (#3552) * Update request pipeline to use the AuthSchemeResolver to resolve the identity and signer * apply code review comments * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * update SignerTests * Add IdentityResolverConfiguration to ClientConfigTests.KNOWN_PROPERTIES * Add AuthSchemeHandler to MockElasticTranscoderClient.CustomizeRuntimePipeline * Use Interlocked when creating AwsV4aAuthScheme.Signer --------- Co-authored-by: Daniel Pinheiro <[email protected]>
…identity and signer (#3552) * Update request pipeline to use the AuthSchemeResolver to resolve the identity and signer * apply code review comments * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * update SignerTests * Add IdentityResolverConfiguration to ClientConfigTests.KNOWN_PROPERTIES * Add AuthSchemeHandler to MockElasticTranscoderClient.CustomizeRuntimePipeline * Use Interlocked when creating AwsV4aAuthScheme.Signer --------- Co-authored-by: Daniel Pinheiro <[email protected]>
…identity and signer (#3552) * Update request pipeline to use the AuthSchemeResolver to resolve the identity and signer * apply code review comments * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * update SignerTests * Add IdentityResolverConfiguration to ClientConfigTests.KNOWN_PROPERTIES * Add AuthSchemeHandler to MockElasticTranscoderClient.CustomizeRuntimePipeline * Use Interlocked when creating AwsV4aAuthScheme.Signer --------- Co-authored-by: Daniel Pinheiro <[email protected]>
…identity and signer (#3552) * Update request pipeline to use the AuthSchemeResolver to resolve the identity and signer * apply code review comments * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * update SignerTests * Add IdentityResolverConfiguration to ClientConfigTests.KNOWN_PROPERTIES * Add AuthSchemeHandler to MockElasticTranscoderClient.CustomizeRuntimePipeline * Use Interlocked when creating AwsV4aAuthScheme.Signer --------- Co-authored-by: Daniel Pinheiro <[email protected]>
…identity and signer (#3552) * Update request pipeline to use the AuthSchemeResolver to resolve the identity and signer * apply code review comments * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * update SignerTests * Add IdentityResolverConfiguration to ClientConfigTests.KNOWN_PROPERTIES * Add AuthSchemeHandler to MockElasticTranscoderClient.CustomizeRuntimePipeline * Use Interlocked when creating AwsV4aAuthScheme.Signer --------- Co-authored-by: Daniel Pinheiro <[email protected]>
…identity and signer (#3552) * Update request pipeline to use the AuthSchemeResolver to resolve the identity and signer * apply code review comments * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * Update sdk/src/Services/S3/Custom/AmazonS3Client.Extensions.cs Co-authored-by: Daniel Pinheiro <[email protected]> * update SignerTests * Add IdentityResolverConfiguration to ClientConfigTests.KNOWN_PROPERTIES * Add AuthSchemeHandler to MockElasticTranscoderClient.CustomizeRuntimePipeline * Use Interlocked when creating AwsV4aAuthScheme.Signer --------- Co-authored-by: Daniel Pinheiro <[email protected]>
Description
AuthScheme.Signer()
method to retrieve the signer associated with this authentication scheme.BaseAuthResolverHandler
.BaseAuthResolverHandler
to allow children to override the signer.AuthSchemeHandler
to the pipeline (limited to S3 and AutoScaling for now).Motivation and Context
Testing
S3
andAutoScaling
services.