Skip to content

Conversation

@millems
Copy link
Contributor

@millems millems commented Sep 5, 2023

These execution attributes do not get stored in the execution attribute map. Instead, they read from or write to OTHER execution attributes in the execution attribute map. This allows us to deprecate old execution attributes and keep them in sync with the execution attributes that have replaced them without needing to read or write to the old attributes.

Included in this PR is adding derived execution attributes for all AwsSignerExecutionAttributes, SdkTokenExecutionAttributes and S3SignerExecutionAttributes, as well as deprecation of those classes and their attributes.

Other notable changes:

  1. When the SELECTED_AUTH_SCHEME is already set when we execute the auth scheme interceptor, carry forward any properties that were already set in the selected auth scheme. This allows setting those properties using the old execution attributes early in the request pipeline.
  2. Add dependency on http-auth-aws and http-auth from auth. This allows the mapping from old attributes to new properties to be defined where the old attributes are created. This also sets us up for a potential future where our existing signers can return a new-style signer.
  3. Simplify the decision of when to use the old signing path or the new signing path. For presigners, always use the old signers. For everything else, use the old signers when the customer has overridden the signer OR we're using an old client that doesn't define any auth schemes.

@millems millems marked this pull request as ready for review September 5, 2023 20:53
@millems millems requested a review from a team as a code owner September 5, 2023 20:53
These execution attributes do not get stored in the execution attribute map. Instead, they read
from or write to OTHER execution attributes in the execution attribute map. This allows us to
deprecate old execution attributes and keep them in sync with the execution attributes that have
replaced them without needing to read or write to the old attributes.

Included in this PR is adding derived execution attributes for all AwsSignerExecutionAttributes,
SdkTokenExecutionAttributes and S3SignerExecutionAttributes, as well as deprecation of those
classes and their attributes.

Other notable changes:
1. When the SELECTED_AUTH_SCHEME is already set when we execute the auth scheme interceptor, carry
forward any properties that were already set in the selected auth scheme. This allows setting those
properties using the old execution attributes early in the request pipeline.
2. Add dependency on http-auth-aws and http-auth from auth. This allows the mapping from old
attributes to new properties to be defined where the old attributes are created. This also sets us
up for a potential future where our existing signers can return a new-style signer.
3. Simplify the decision of when to use the old signing path or the new signing path. For
presigners, always use the old signers. For everything else, use the old signers when the customer
has overridden the signer OR we're using an old client that doesn't define any auth schemes.
@millems millems force-pushed the millem/derived-execution-attributes branch from 1d43285 to f06f05a Compare September 6, 2023 19:30
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 6, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 5 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 409 Code Smells

0.0% 0.0% Coverage
8.6% 8.6% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@millems millems merged commit 3c296d1 into feature/master/sra-identity-auth Sep 6, 2023
@millems millems deleted the millem/derived-execution-attributes branch September 6, 2023 23:44
Comment on lines +156 to +158
private static boolean loadOldSigner(ExecutionAttributes attributes, SdkRequest request) {
return attributes.getAttribute(SdkInternalExecutionAttribute.AUTH_SCHEMES) == null ||
SignerOverrideUtils.isSignerOverridden(request, attributes);
Copy link
Contributor

@gosar gosar Sep 7, 2023

Choose a reason for hiding this comment

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

I see this change and related change in (Async)SigingStage that checks IS_NONE_AUTH_TYPE_REQUEST to not sign. But with this approach,

  1. For an old client with new core, for operation with authType=none, an unnecessary aws credentials resolution will happen (part of addCredentialsToExecutionAttributes). It adds a potential failure path that shouldn't matter, and this didn't happen earlier, so is a behavior change.
  2. For a new client, if customer overrode the Signer at the client level (with the desire to use that Signer for the auth'ed operations), for request that resolves to NoAuthAuthScheme (e.g., authType=none operations), same unnecessary aws credentials resolution will happen, which I was avoiding with the additional conditionals.

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 missed that nuance - there must not be a test for it, yet. We should fix it and add a test to make sure no one else makes my mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

have added internal ticket to track this.

Copy link
Contributor

Choose a reason for hiding this comment

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

being fixed in #4523

return authSchemes;
}

public static <T extends Identity> void putSelectedAuthScheme(ExecutionAttributes attributes,
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is unrelated to endpoint rules, but I guess it is convenient to put this method in endpoint rules related utils class; instead of a new one or generate it in AuthSchemeInterceptorSpec itself.

Comment on lines +116 to +117
existingAuthScheme.authSchemeOption().forEachIdentityProperty(selectedOption::putIdentityPropertyIfAbsent);
existingAuthScheme.authSchemeOption().forEachSignerProperty(selectedOption::putSignerPropertyIfAbsent);
Copy link
Contributor

Choose a reason for hiding this comment

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

the putIfAbsent here means if the selectedAuthScheme overwrites the existingAuthScheme's values - this seems fine. IIUC, the only case where existingAuthScheme would be non-null when it reaches this code path would be for attributes set here before calling the AuthSchemeInterceptor (which calls this AuthSchemeUtils) few lines later here.

Though makes me wonder if this method should have comments on appropriate use and prevent it getting used in some other context. Not a big deal I think, though probably another reason to move this method inside AuthSchemeInterceptor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants