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

[SRA] Rules-based authentication resolvers #3540

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

dscpinheiro
Copy link
Contributor

Description

This PR focuses on rules-based resolvers (which delegate authentication resolution to their EP2.0 resolvers). Only a handful of services (such as S3 and EventBridge) will use this code path.

Motivation and Context

DOTNET-7651

Testing

Built the AutoScaling and S3 solutions locally (the handlers are not connected to the request pipeline yet).

License

  • I confirm that this pull request can be released under the Apache 2 license

/// </summary>
public Amazon<#=this.Config.ClassName#>AuthSchemeResolver AuthSchemeResolver { get; } = new();

/// <inheritdoc/>
protected override List<IAuthSchemeOption> ResolveAuthOptions(IRequestContext requestContext)
protected override List<IAuthSchemeOption> ResolveAuthOptions(IExecutionContext executionContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to get feedback on this approach I took: The SRA docs mention we should code generate the mapping from auth scheme parameters to endpoint resolver parameters, however that was a lot of duplication for S3. By calling the generated resolver instead, we'll use all endpoint parameters anyways (including new ones we might add in the future):

protected override EndpointParameters MapEndpointsParameters(IRequestContext requestContext)
{
var config = (AmazonS3Config)requestContext.ClientConfig;
var result = new S3EndpointParameters();
result.Region = config.RegionEndpoint?.SystemName;
result.UseFIPS = config.UseFIPSEndpoint;
result.UseDualStack = config.UseDualstackEndpoint;
result.Endpoint = config.ServiceURL;
result.ForcePathStyle = config.ForcePathStyle;
result.Accelerate = config.UseAccelerateEndpoint;
result.UseGlobalEndpoint = config.USEast1RegionalEndpointValue == S3UsEast1RegionalEndpointValue.Legacy;
result.DisableMultiRegionAccessPoints = config.DisableMultiregionAccessPoints;
result.UseArnRegion = config.UseArnRegion;

One benefit is that it allowed me to keep the implementation to retrieve auth schemes from an endpoint (in sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseAuthResolverHandler.cs) simpler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would rerunning the endpoint resolver twice work when the SDK supports account id based endpoint rules? Because we would need the identity to get the account id for the endpoint rules I believe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do need to run endpoint rules twice and it will use the exact same parameters we could stash the results in executionContext.RequestContext.ContextAttributes and then have the endpoint resolver check for the existence and if so reuse those results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the internal design doc (that recommended running the EP2.0 resolver twice), there was a comment on not allowing S3 to use the AWS::Auth::AccountId parameter in rules that have authentication schemes.

I'll follow up to see if that was ever built or if it's something we should do in our generator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the other comment, I thought about populating the existing EndpointAttributes property:

IPropertyBag EndpointAttributes { get; set; }

But it looks like when the endpoints resolver calls GetEndpoint it calculates them again anyway (this constructor is called by the generated code):

if (!string.IsNullOrEmpty(attributesJson))
{
var attributes = JsonMapper.ToObject(attributesJson);
Attributes = PropertyBag.FromJsonData(attributes);
}

Or did you mean I should cache the entire endpoint? (I don't think that's the case since we'd need to change whenever we add support for account-ID based endpoints).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was running the endpoint rules twice would only work if we the service didn't use account id as part of the rule. In that case would we need to change this when the SDK supports account id based rules? So yes I was suggesting caching the results within the scope of the current request in stead of running the endpoint rules twice. But maybe I'm misunderstanding something and the second time could cause a different results in some scenarios.

@dscpinheiro dscpinheiro marked this pull request as ready for review November 1, 2024 19:55
/// </summary>
public Amazon<#=this.Config.ClassName#>AuthSchemeResolver AuthSchemeResolver { get; } = new();

/// <inheritdoc/>
protected override List<IAuthSchemeOption> ResolveAuthOptions(IRequestContext requestContext)
protected override List<IAuthSchemeOption> ResolveAuthOptions(IExecutionContext executionContext)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do need to run endpoint rules twice and it will use the exact same parameters we could stash the results in executionContext.RequestContext.ContextAttributes and then have the endpoint resolver check for the existence and if so reuse those results.

@dscpinheiro dscpinheiro merged commit 680c9eb into sra-identity-auth Nov 6, 2024
1 check passed
@dscpinheiro dscpinheiro deleted the dspin/DOTNET-7651 branch November 6, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants