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

Add tests to assert signer and endpoint auth-scheme behaviors #4651

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static software.amazon.awssdk.core.interceptor.SdkExecutionAttribute.RESOLVED_CHECKSUM_SPECS;

import java.util.Map;
import java.util.TreeMap;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.auth.signer.AwsSignerExecutionAttribute;
import software.amazon.awssdk.awscore.AwsExecutionAttribute;
Expand Down Expand Up @@ -183,21 +184,32 @@ private static void putAuthSchemeResolutionAttributes(ExecutionAttributes execut
SdkClientConfiguration clientConfig,
SdkRequest originalRequest) {

// TODO(sra-identity-and-auth): When request-level auth scheme provider is added, use the request-level auth scheme
// provider if the customer specified an override, otherwise fall back to the one on the client.
AuthSchemeProvider authSchemeProvider = clientConfig.option(SdkClientOption.AUTH_SCHEME_PROVIDER);
// Use auth scheme provider that the user specified at the request level with
// preference over that which is configured on the client.
AuthSchemeProvider authSchemeProvider =
executionAttributes.getOptionalAttribute(SdkInternalExecutionAttribute.AUTH_SCHEME_RESOLVER)
Copy link
Contributor

Choose a reason for hiding this comment

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

Directly setting AUTH_SCHEME_RESOLVER into ExecutionAttributes is not how users would specify request level override for it. These attributes are SdkInternalExecutionAttributes (and SdkProtectedApi) which customers are not supposed to use directly. Even if they can set ExecutionAttributes in RequestOverrideConfiguration. To resolve this TODO we need to support a first class setter somewhere in RequestOverrideConfiguration(or sub-class) for each of these properties.

We have other internal tasks tracking these TODOs, let's not try to address those in the context of this PR.

I think you made this change to have the tests below with sigv4a. The supported way to do that is putAuthScheme on the clientBuilder. (also note that's also at client level not request level which is what this TODO is about).

.orElseGet(() -> clientConfig.option(SdkClientOption.AUTH_SCHEME_PROVIDER));

// Use auth schemes that the user specified at the request level with
// preference over those on the client.
// TODO(sra-identity-and-auth): The request level schemes should be "merged" with client level, with request preferred
// over client.
Map<String, AuthScheme<?>> authSchemes = clientConfig.option(SdkClientOption.AUTH_SCHEMES);
Map<String, AuthScheme<?>> authSchemes = executionAttributes.getAttribute(SdkInternalExecutionAttribute.AUTH_SCHEMES);
Map<String, AuthScheme<?>> clientAuthSchemes = clientConfig.option(SdkClientOption.AUTH_SCHEMES);
Map<String, AuthScheme<?>> mergedAuthSchemes = new TreeMap<>();

if (authSchemes == null) {
mergedAuthSchemes = clientAuthSchemes;
} else if (clientAuthSchemes == null) {
mergedAuthSchemes = authSchemes;
} else {
mergedAuthSchemes.putAll(authSchemes);
mergedAuthSchemes.putAll(clientAuthSchemes);
}

IdentityProviders identityProviders = resolveIdentityProviders(originalRequest, clientConfig);

executionAttributes
.putAttribute(SdkInternalExecutionAttribute.AUTH_SCHEME_RESOLVER, authSchemeProvider)
.putAttribute(SdkInternalExecutionAttribute.AUTH_SCHEMES, authSchemes)
.putAttribute(SdkInternalExecutionAttribute.AUTH_SCHEMES, mergedAuthSchemes)
.putAttribute(SdkInternalExecutionAttribute.IDENTITY_PROVIDERS, identityProviders);
}

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package software.amazon.awssdk.services;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static software.amazon.awssdk.core.client.config.SdkAdvancedClientOption.SIGNER;

import java.net.URI;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.CompletableFuture;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
import software.amazon.awssdk.auth.signer.AwsSignerExecutionAttribute;
import software.amazon.awssdk.awscore.endpoints.AwsEndpointAttribute;
import software.amazon.awssdk.awscore.endpoints.authscheme.SigV4AuthScheme;
import software.amazon.awssdk.awscore.endpoints.authscheme.SigV4aAuthScheme;
import software.amazon.awssdk.core.async.AsyncRequestBody;
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.core.interceptor.SdkInternalExecutionAttribute;
import software.amazon.awssdk.core.signer.Signer;
import software.amazon.awssdk.core.sync.RequestBody;
import software.amazon.awssdk.endpoints.Endpoint;
import software.amazon.awssdk.http.SdkHttpFullRequest;
import software.amazon.awssdk.http.auth.aws.scheme.AwsV4aAuthScheme;
import software.amazon.awssdk.http.auth.aws.signer.AwsV4FamilyHttpSigner;
import software.amazon.awssdk.http.auth.aws.signer.AwsV4HttpSigner;
import software.amazon.awssdk.http.auth.aws.signer.AwsV4aHttpSigner;
import software.amazon.awssdk.http.auth.aws.signer.RegionSet;
import software.amazon.awssdk.http.auth.spi.scheme.AuthSchemeOption;
import software.amazon.awssdk.regions.Region;
import software.amazon.awssdk.services.protocolquery.ProtocolQueryAsyncClient;
import software.amazon.awssdk.services.protocolquery.ProtocolQueryClient;
import software.amazon.awssdk.services.protocolquery.auth.scheme.ProtocolQueryAuthSchemeProvider;
import software.amazon.awssdk.services.protocolquery.endpoints.ProtocolQueryEndpointProvider;

/**
* Tests to ensure that parameters set when endpoint and auth-scheme resolution occurs get propagated to the overriden
* signer (i.e. pre-SRA signers).
*/
public class SignerAndEndpointOverridesTest {

private static Signer mockSigner = mock(Signer.class);

@BeforeAll
static void setup() {
when(mockSigner.sign(any(), any())).thenThrow(new RuntimeException("boom!"));
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea behind using the interceptor instead is two folded, it's the pattern used by the Java team and I been asked before to use it, and, the exceptions is thrown before transmission, IOW, you also validate that everything else in the execution pipeline runs as expected and it's not throwing somewhere else due to the setup you have here.

}

@Test
void test_whenV4EndpointAuthSchemeWithSignerOverride_thenEndpointParamsShouldPropagateToSigner() {
ProtocolQueryClient client = ProtocolQueryClient
.builder()
.authSchemeProvider(v4AuthSchemeProviderOverride())
.credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("akid", "skid")))
.endpointProvider(v4EndpointProviderOverride())
.region(Region.US_WEST_1)
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea behind the ask to change the region here was that you can validate that the override takes place, given that the region here is the same as the override we cannot know whether is it coming from the client builder or from the override.

.overrideConfiguration(o -> o.putAdvancedOption(SIGNER, mockSigner))
.build();

Exception ex = assertThrows(
RuntimeException.class, () -> client.streamingInputOperation(r -> {}, RequestBody.fromString("test"))
);

assertThat(ex.getMessage()).contains("boom!");
verify(mockSigner).sign(
any(SdkHttpFullRequest.class),
argThat(attrs ->
"us-west-1".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNING_REGION).id()) &&
"query-test-v4".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SERVICE_SIGNING_NAME)) &&
Boolean.FALSE.equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE)))
);
}

@Test
void testAsync_whenV4EndpointAuthSchemeWithSignerOverride_thenEndpointParamsShouldPropagateToSigner() {
ProtocolQueryAsyncClient client = ProtocolQueryAsyncClient
.builder()
.authSchemeProvider(v4AuthSchemeProviderOverride())
.credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("akid", "skid")))
.endpointProvider(v4EndpointProviderOverride())
.region(Region.US_WEST_1)
.overrideConfiguration(o -> o.putAdvancedOption(SIGNER, mockSigner))
.build();

Exception ex = assertThrows(
RuntimeException.class, () -> client.streamingInputOperation(r -> {}, AsyncRequestBody.fromString("test")).join()
);

assertThat(ex.getMessage()).contains("boom!");
verify(mockSigner).sign(
any(SdkHttpFullRequest.class),
argThat(attrs ->
"us-west-1".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNING_REGION).id()) &&
"query-test-v4".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SERVICE_SIGNING_NAME)) &&
Boolean.FALSE.equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE)))
);
}

@Test
void test_whenV4aEndpointAuthSchemeWithSignerOverride_thenEndpointParamsShouldPropagateToSigner() {
ProtocolQueryClient client = ProtocolQueryClient
.builder()
.authSchemeProvider(v4aAuthSchemeProviderOverride())
.credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("akid", "skid")))
.endpointProvider(v4aEndpointProviderOverride())
.region(Region.US_EAST_1)
.overrideConfiguration(
o -> o.putAdvancedOption(SIGNER, mockSigner)
.putExecutionAttribute(SdkInternalExecutionAttribute.AUTH_SCHEMES, Collections.singletonMap(
"aws.auth#sigv4a", AwsV4aAuthScheme.create()
)))
.build();

Exception ex = assertThrows(
RuntimeException.class, () -> client.streamingInputOperation(r -> {}, RequestBody.fromString("test"))
);

assertThat(ex.getMessage()).contains("boom!");
verify(mockSigner).sign(
any(SdkHttpFullRequest.class),
argThat(attrs ->
"us-east-1".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNING_REGION).id()) &&
"query-test-v4a".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SERVICE_SIGNING_NAME)) &&
Boolean.FALSE.equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE)))
);
}

@Test
void testAsync_whenV4aEndpointAuthSchemeWithSignerOverride_thenEndpointParamsShouldPropagateToSigner() {
ProtocolQueryAsyncClient client = ProtocolQueryAsyncClient
.builder()
.authSchemeProvider(v4aAuthSchemeProviderOverride())
.credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("akid", "skid")))
.endpointProvider(v4aEndpointProviderOverride())
.region(Region.US_EAST_1)
.overrideConfiguration(
o -> o.putAdvancedOption(SIGNER, mockSigner)
.putExecutionAttribute(SdkInternalExecutionAttribute.AUTH_SCHEMES, Collections.singletonMap(
"aws.auth#sigv4a", AwsV4aAuthScheme.create()
)))
.build();

Exception ex = assertThrows(
SdkClientException.class, () -> client.streamingInputOperation(r -> {}, AsyncRequestBody.fromString("test")).join()
);

assertThat(ex.getMessage()).contains("boom!");
verify(mockSigner).sign(
any(SdkHttpFullRequest.class),
argThat(attrs ->
"us-east-1".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNING_REGION).id()) &&
"query-test-v4a".equals(attrs.getAttribute(AwsSignerExecutionAttribute.SERVICE_SIGNING_NAME)) &&
Boolean.FALSE.equals(attrs.getAttribute(AwsSignerExecutionAttribute.SIGNER_DOUBLE_URL_ENCODE)))
);
}

private static ProtocolQueryAuthSchemeProvider v4AuthSchemeProviderOverride() {
return __ -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, can we use a letter here instead? (e.g., x -> {...})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unused, and this is a fairly common when the variable is unused.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not in Java, and I can't find any other test in the project using it (didn't search thoroughly), but up to the team.

Copy link
Contributor

Choose a reason for hiding this comment

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

(c.f., the commonly used pattern on some other languages _ is not even possible in Java since a single underscore is reserved).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, hence the usage of two underscores. Changed to 'x'.

Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot this one and the other below.

List<AuthSchemeOption> options = new ArrayList<>();
options.add(
AuthSchemeOption.builder().schemeId("aws.auth#sigv4")
.putSignerProperty(AwsV4FamilyHttpSigner.SERVICE_SIGNING_NAME, "query-will-be-overriden")
.putSignerProperty(AwsV4HttpSigner.REGION_NAME, "region-will-be-overriden")
.build()
);
return Collections.unmodifiableList(options);
};
}

private static ProtocolQueryAuthSchemeProvider v4aAuthSchemeProviderOverride() {
return __ -> {
List<AuthSchemeOption> options = new ArrayList<>();
options.add(
AuthSchemeOption.builder().schemeId("aws.auth#sigv4a")
.putSignerProperty(AwsV4FamilyHttpSigner.SERVICE_SIGNING_NAME, "query-will-be-overriden")
.putSignerProperty(AwsV4aHttpSigner.REGION_SET, RegionSet.create("region-will-be-overriden"))
.build()
);
return Collections.unmodifiableList(options);
};
}

private static ProtocolQueryEndpointProvider v4EndpointProviderOverride() {
return x -> {
Endpoint endpoint =
Endpoint.builder()
.url(URI.create("https://testv4.query.us-west-1"))
.putAttribute(
AwsEndpointAttribute.AUTH_SCHEMES,
Collections.singletonList(SigV4AuthScheme.builder()
.signingRegion("us-west-1")
.signingName("query-test-v4")
.disableDoubleEncoding(true)
.build()))
.build();

return CompletableFuture.completedFuture(endpoint);
};
}

private static ProtocolQueryEndpointProvider v4aEndpointProviderOverride() {
return x -> {
Endpoint endpoint =
Endpoint.builder()
.url(URI.create("https://testv4a.query.us-east-1"))
.putAttribute(
AwsEndpointAttribute.AUTH_SCHEMES,
Collections.singletonList(SigV4aAuthScheme.builder()
.addSigningRegion("us-east-1")
.signingName("query-test-v4a")
.disableDoubleEncoding(true)
.build()))
.build();

return CompletableFuture.completedFuture(endpoint);
};
}
}