-
Notifications
You must be signed in to change notification settings - Fork 1k
Preserve interceptor-set signer properties across auth scheme and endpoint resolution #6961
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
Changes from 2 commits
6ab9802
bd27de1
6d725f2
258bd85
e092ae8
2d96dc2
1b3d1ed
18e02f5
00e558a
8d486f9
8590c62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.function.Supplier; | ||
| import java.util.stream.Collectors; | ||
|
|
@@ -35,6 +36,7 @@ | |
| import software.amazon.awssdk.http.auth.spi.scheme.AuthScheme; | ||
| import software.amazon.awssdk.http.auth.spi.scheme.AuthSchemeOption; | ||
| import software.amazon.awssdk.http.auth.spi.signer.HttpSigner; | ||
| import software.amazon.awssdk.http.auth.spi.signer.SignerProperty; | ||
| import software.amazon.awssdk.identity.spi.AwsCredentialsIdentity; | ||
| import software.amazon.awssdk.identity.spi.Identity; | ||
| import software.amazon.awssdk.identity.spi.IdentityProvider; | ||
|
|
@@ -137,9 +139,28 @@ public static <T extends Identity> SelectedAuthScheme<T> mergePreExistingAuthSch | |
| return selectedAuthScheme; | ||
| } | ||
|
|
||
| SelectedAuthScheme<?> beforeInterceptors = | ||
| executionAttributes.getAttribute(SdkInternalExecutionAttribute.AUTH_SCHEME_BEFORE_INTERCEPTORS); | ||
|
|
||
| if (beforeInterceptors != null && | ||
| beforeInterceptors.authSchemeOption() == existingAuthScheme.authSchemeOption()) { | ||
| return selectedAuthScheme; | ||
| } | ||
|
|
||
| AuthSchemeOption.Builder mergedOption = selectedAuthScheme.authSchemeOption().toBuilder(); | ||
|
|
||
| existingAuthScheme.authSchemeOption().forEachSignerProperty(new AuthSchemeOption.SignerPropertyConsumer() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have we benchmarked this? I'm a little concerned about the performance impact - I'm wondering if theres some way we could at least short circuit this when no interceptors have run. Or can we do an identity check on the authSchemeOption (like beforeInterceptors.authSchemeOption() == selectedAuthScheme.authSchemeOption() ) -since they are immutable, if you change them, it requires building a new object, so could be a quick way to see if any mutation has happened?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! Added the check to skips the merge when no interceptors have run.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add some inline comments explaining each step. This class is too complex to understand...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comments explaining that. |
||
| @Override | ||
| public <S> void accept(SignerProperty<S> key, S value) { | ||
| if (wasModifiedByInterceptor(beforeInterceptors, key, value)) { | ||
| mergedOption.putSignerProperty(key, value); | ||
| } else { | ||
| mergedOption.putSignerPropertyIfAbsent(key, value); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| existingAuthScheme.authSchemeOption().forEachIdentityProperty(mergedOption::putIdentityPropertyIfAbsent); | ||
| existingAuthScheme.authSchemeOption().forEachSignerProperty(mergedOption::putSignerPropertyIfAbsent); | ||
|
|
||
| return new SelectedAuthScheme<>( | ||
| selectedAuthScheme.identity(), | ||
|
|
@@ -148,6 +169,57 @@ public static <T extends Identity> SelectedAuthScheme<T> mergePreExistingAuthSch | |
| ); | ||
| } | ||
|
|
||
| private static <T> boolean wasModifiedByInterceptor(SelectedAuthScheme<?> beforeInterceptors, | ||
| SignerProperty<T> key, T currentValue) { | ||
| if (beforeInterceptors == null) { | ||
| return true; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: why we return true if there are no auth schemes before interceptors?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, actually beforeInterceptors can never be null, its always set in AwsExecutionContextBuilder, so whatever we return doesn't matter, maybe its better to remove the check since it never happen. Will remove it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the check |
||
| T originalValue = beforeInterceptors.authSchemeOption().signerProperty(key); | ||
| return !Objects.equals(originalValue, currentValue); | ||
| } | ||
|
|
||
| /** | ||
| * Re-applies interceptor-modified signer properties onto the current auth scheme. | ||
| * Called after endpoint resolution, which may have overwritten properties that interceptors set. | ||
| */ | ||
| public static void applyInterceptorModifiedProperties(SelectedAuthScheme<?> currentScheme, | ||
| SelectedAuthScheme<?> beforeInterceptors, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: formatting is a bit off here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed it |
||
| SelectedAuthScheme<?> afterInterceptors, | ||
| ExecutionAttributes attrs) { | ||
| if (afterInterceptors == null) { | ||
| return; | ||
| } | ||
| doApplyInterceptorModifiedProperties(currentScheme, beforeInterceptors, afterInterceptors, attrs); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static <T extends Identity> void doApplyInterceptorModifiedProperties( | ||
| SelectedAuthScheme<T> currentScheme, | ||
| SelectedAuthScheme<?> beforeInterceptors, | ||
| SelectedAuthScheme<?> afterInterceptors, | ||
| ExecutionAttributes attrs) { | ||
|
|
||
| AuthSchemeOption.Builder mergedOption = currentScheme.authSchemeOption().toBuilder(); | ||
| boolean[] changed = {false}; | ||
|
|
||
| afterInterceptors.authSchemeOption().forEachSignerProperty(new AuthSchemeOption.SignerPropertyConsumer() { | ||
| @Override | ||
| public <S> void accept(SignerProperty<S> key, S value) { | ||
| if (wasModifiedByInterceptor(beforeInterceptors, key, value)) { | ||
| mergedOption.putSignerProperty(key, value); | ||
| changed[0] = true; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if (changed[0]) { | ||
| attrs.putAttribute(SdkInternalExecutionAttribute.SELECTED_AUTH_SCHEME, | ||
| new SelectedAuthScheme<>(currentScheme.identity(), | ||
| currentScheme.signer(), | ||
| mergedOption.build())); | ||
| } | ||
| } | ||
|
|
||
| private static <T extends Identity> SelectedAuthScheme<T> trySelectAuthScheme( | ||
| AuthSchemeOption authOption, | ||
| AuthScheme<T> authScheme, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -204,6 +204,22 @@ public final class SdkInternalExecutionAttribute extends SdkExecutionAttribute { | |
| public static final ExecutionAttribute<SelectedAuthScheme<?>> SELECTED_AUTH_SCHEME = | ||
| new ExecutionAttribute<>("SelectedAuthScheme"); | ||
|
|
||
| /** | ||
| * Snapshot of {@link #SELECTED_AUTH_SCHEME} taken before execution interceptors run. | ||
| * Used by {@code AuthSchemeResolver#mergePreExistingAuthSchemeProperties} to detect which signer properties | ||
| * were explicitly modified by interceptors (and should therefore override the freshly-resolved values). | ||
| */ | ||
| public static final ExecutionAttribute<SelectedAuthScheme<?>> AUTH_SCHEME_BEFORE_INTERCEPTORS = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggesting renaming it to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed the fields. |
||
| new ExecutionAttribute<>("AuthSchemeBeforeInterceptors"); | ||
|
|
||
| /** | ||
| * Snapshot of {@link #SELECTED_AUTH_SCHEME} taken after interceptors run but before auth scheme resolution. | ||
| * Together with {@link #AUTH_SCHEME_BEFORE_INTERCEPTORS}, this allows detecting which signer properties | ||
| * were explicitly modified by interceptors so they can be re-applied after endpoint resolution. | ||
| */ | ||
| public static final ExecutionAttribute<SelectedAuthScheme<?>> AUTH_SCHEME_AFTER_INTERCEPTORS = | ||
| new ExecutionAttribute<>("AuthSchemeAfterInterceptors"); | ||
|
|
||
| /** | ||
| * The supported compression algorithms for an operation, and whether the operation is streaming or not. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,10 +72,7 @@ public void canSetSignerExecutionAttributes_beforeExecution() { | |
| public void beforeExecution(Context.BeforeExecution context, ExecutionAttributes executionAttributes) { | ||
| attributeModifications.accept(executionAttributes); | ||
| } | ||
| }, | ||
| AwsSignerExecutionAttribute.SERVICE_SIGNING_NAME, // Endpoint rules override signing name | ||
| AwsSignerExecutionAttribute.SIGNING_REGION, // Endpoint rules override signing region | ||
| AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE); // Endpoint rules override double-url-encode | ||
|
Comment on lines
-76
to
-78
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did we remove those three lines?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously (before our changes), these three properties were already not working in beforeExecution, endpoint rules would overwrite them. That's why they were excluded from the test. But other stages modifyRequest, modifyHttpRequest used to work because they ran after endpoint rules. Now with our changes it works correctly for beforeExecution as well, so removed those exclusions.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, could it break users who rely on the buggy behavior?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I don't think so. If the user is explicitly setting those attributes in interceptors, previously we didn't use them, we used the values resolved by endpoint rule. But now with our fix, we will use the values. If they are explicitly setting they are intending to use them.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I was thinking about the case where an attribute was set to a wrong value by accident, no one has noticed it because the application is working (SDK not honoring it) and it'll start to fail once we start to honor it
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should let it prevent us from fixing it. Just need to document it somewhere... probably changelog entry?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hmm yeah right, I guess that would be rare. But I'll make sure to document it in changelog. |
||
| }); | ||
| } | ||
|
|
||
| @Test | ||
|
|
||
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: can we rename it to
authSchemeBeforeInterceptors?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!