Repository navigation
Support account-ID based endpoints for IMDS and container credentials #7513
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
ad376a7
376215d
29aa1f3
ebcd11e
c9e7678
958e550
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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "type": "feature", | ||
| "category": "AWS SDK for Java v2", | ||
| "contributor": "", | ||
| "description": "Add support for account-ID based endpoints when using IMDS credential provider. The SDK now sends the `X-amzn-extended: 1` header on IMDS credential requests and parses the `AccountId` field from the response when available." | ||
|
Member
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. Might also be worth reiterating how to opt out (if we did that before when adding support).
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 it! |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.function.Supplier; | ||
|
|
@@ -76,6 +77,8 @@ public final class InstanceProfileCredentialsProvider | |
| private static final String TOKEN_RESOURCE = "/latest/api/token"; | ||
| private static final String EC2_METADATA_TOKEN_TTL_HEADER = "x-aws-ec2-metadata-token-ttl-seconds"; | ||
| private static final String DEFAULT_TOKEN_TTL = "21600"; | ||
| private static final String EXTENDED_CREDENTIALS_HEADER = "X-amzn-extended"; | ||
| private static final String EXTENDED_CREDENTIALS_VALUE = "1"; | ||
|
|
||
| private final Clock clock; | ||
| private final String endpoint; | ||
|
|
@@ -225,7 +228,7 @@ private ResourcesEndpointProvider createEndpointProvider() { | |
| return StaticResourcesEndpointProvider.builder() | ||
| .endpoint(URI.create(imdsHostname + SECURITY_CREDENTIALS_RESOURCE | ||
| + securityCredentials[0])) | ||
| .headers(getTokenHeaders(token)) | ||
| .headers(getCredentialHeaders(token)) | ||
|
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. Not introduced by this PR, but related.
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. Yeah we are not implementing profile caching currently in Java, didn't merge that with this change as well, will do that as a followup. |
||
| .connectionTimeout(Duration.ofMillis( | ||
| this.configProvider.serviceTimeout())) | ||
| .build(); | ||
|
|
@@ -320,6 +323,19 @@ private Map<String, String> getTokenHeaders(String metadataToken) { | |
| return Collections.singletonMap(EC2_METADATA_TOKEN_HEADER, metadataToken); | ||
| } | ||
|
|
||
| /** | ||
| * Returns headers for the credential GET request. Includes the extended credentials header to signal IMDS to | ||
| * return the AccountId field in the response, and the IMDSv2 token if available. | ||
| */ | ||
| private Map<String, String> getCredentialHeaders(String metadataToken) { | ||
| Map<String, String> headers = new HashMap<>(); | ||
| if (metadataToken != null) { | ||
| headers.put(EC2_METADATA_TOKEN_HEADER, metadataToken); | ||
| } | ||
| headers.put(EXTENDED_CREDENTIALS_HEADER, EXTENDED_CREDENTIALS_VALUE); | ||
| return Collections.unmodifiableMap(headers); | ||
| } | ||
|
|
||
| @Override | ||
| public Builder toBuilder() { | ||
| return new BuilderImpl(this); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,14 +51,24 @@ public class HttpCredentialsLoaderTest { | |
|
|
||
| private static String successResponseWithInvalidBody; | ||
|
|
||
| private static String successResponseWithAccountId; | ||
|
|
||
| private static String successResponseWithUnexpectedFields; | ||
|
|
||
| @BeforeClass | ||
| public static void setup() throws IOException { | ||
| try (InputStream successInputStream = HttpCredentialsLoaderTest.class.getResourceAsStream | ||
| ("/resources/wiremock/successResponse.json"); | ||
| InputStream responseWithInvalidBodyInputStream = HttpCredentialsLoaderTest.class.getResourceAsStream | ||
| ("/resources/wiremock/successResponseWithInvalidBody.json")) { | ||
| ("/resources/wiremock/successResponseWithInvalidBody.json"); | ||
| InputStream responseWithAccountIdInputStream = HttpCredentialsLoaderTest.class.getResourceAsStream | ||
| ("/resources/wiremock/successResponseWithAccountId.json"); | ||
| InputStream responseWithUnexpectedFieldsInputStream = HttpCredentialsLoaderTest.class.getResourceAsStream | ||
| ("/resources/wiremock/successResponseWithUnexpectedFields.json")) { | ||
| successResponse = IoUtils.toUtf8String(successInputStream); | ||
| successResponseWithInvalidBody = IoUtils.toUtf8String(responseWithInvalidBodyInputStream); | ||
| successResponseWithAccountId = IoUtils.toUtf8String(responseWithAccountIdInputStream); | ||
| successResponseWithUnexpectedFields = IoUtils.toUtf8String(responseWithUnexpectedFieldsInputStream); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -76,6 +86,58 @@ public void testLoadCredentialsParsesJsonResponseProperly() { | |
| assertThat(credentials.accessKeyId()).isEqualTo("ACCESS_KEY_ID"); | ||
| assertThat(credentials.secretAccessKey()).isEqualTo("SECRET_ACCESS_KEY"); | ||
| assertThat(credentials.sessionToken()).isEqualTo("TOKEN_TOKEN_TOKEN"); | ||
| assertThat(credentials.accountId()).isNotPresent(); | ||
| } | ||
|
|
||
| /** | ||
| * Test that loadCredentials parses AccountId when present in the response. | ||
| */ | ||
| @Test | ||
| public void testLoadCredentialsParsesAccountIdWhenPresent() { | ||
| stubForSuccessResponseWithCustomBody(successResponseWithAccountId); | ||
|
|
||
| HttpCredentialsLoader credentialsProvider = HttpCredentialsLoader.create(PROVIDER_NAME); | ||
| AwsSessionCredentials credentials = (AwsSessionCredentials) credentialsProvider.loadCredentials(testEndpointProvider()) | ||
| .getAwsCredentials(); | ||
|
|
||
| assertThat(credentials.accessKeyId()).isEqualTo("ACCESS_KEY_ID"); | ||
| assertThat(credentials.secretAccessKey()).isEqualTo("SECRET_ACCESS_KEY"); | ||
| assertThat(credentials.sessionToken()).isEqualTo("TOKEN_TOKEN_TOKEN"); | ||
| assertThat(credentials.accountId()).isPresent().hasValue("123456789012"); | ||
| } | ||
|
|
||
| /** | ||
| * Test that loadCredentials succeeds without AccountId when it's absent from the response. | ||
| */ | ||
| @Test | ||
| public void testLoadCredentialsSucceedsWithoutAccountId() { | ||
| stubForSuccessResponseWithCustomBody(successResponse); | ||
|
|
||
| HttpCredentialsLoader credentialsProvider = HttpCredentialsLoader.create(PROVIDER_NAME); | ||
| AwsSessionCredentials credentials = (AwsSessionCredentials) credentialsProvider.loadCredentials(testEndpointProvider()) | ||
| .getAwsCredentials(); | ||
|
|
||
| assertThat(credentials.accessKeyId()).isEqualTo("ACCESS_KEY_ID"); | ||
| assertThat(credentials.secretAccessKey()).isEqualTo("SECRET_ACCESS_KEY"); | ||
| assertThat(credentials.accountId()).isNotPresent(); | ||
| } | ||
|
|
||
| /** | ||
| * Test that loadCredentials does not fail when response contains unexpected/unknown fields. | ||
| * This verifies the SEP requirement: "Parsing MUST NOT fail in the presence of unexpected fields." | ||
|
Member
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: reference to SEP.
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! |
||
| */ | ||
| @Test | ||
| public void testLoadCredentialsIgnoresUnexpectedFields() { | ||
| stubForSuccessResponseWithCustomBody(successResponseWithUnexpectedFields); | ||
|
|
||
| HttpCredentialsLoader credentialsProvider = HttpCredentialsLoader.create(PROVIDER_NAME); | ||
| AwsSessionCredentials credentials = (AwsSessionCredentials) credentialsProvider.loadCredentials(testEndpointProvider()) | ||
| .getAwsCredentials(); | ||
|
|
||
| assertThat(credentials.accessKeyId()).isEqualTo("ACCESS_KEY_ID"); | ||
| assertThat(credentials.secretAccessKey()).isEqualTo("SECRET_ACCESS_KEY"); | ||
| assertThat(credentials.sessionToken()).isEqualTo("TOKEN_TOKEN_TOKEN"); | ||
| assertThat(credentials.accountId()).isPresent().hasValue("123456789012"); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "AccessKeyId": "ACCESS_KEY_ID", | ||
| "SecretAccessKey": "SECRET_ACCESS_KEY", | ||
| "Token": "TOKEN_TOKEN_TOKEN", | ||
| "Expiration": "3000-05-03T04:55:54Z", | ||
| "AccountId": "123456789012" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "AccessKeyId": "ACCESS_KEY_ID", | ||
| "SecretAccessKey": "SECRET_ACCESS_KEY", | ||
| "Token": "TOKEN_TOKEN_TOKEN", | ||
| "Expiration": "3000-05-03T04:55:54Z", | ||
| "AccountId": "123456789012", | ||
| "UnexpectedElement1": { | ||
| "Name": "ignore-me-1" | ||
| }, | ||
| "AnotherUnexpected": "should-be-ignored" | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
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 describe more specifically what will change for users of container credentials (ECS/EKS)? Wouldn't the endpoint change for them as well? If they are allowlisting traffic, adding the account ID to the hostname may break that; the effect on container credential providers isn't called out here.
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.
Good point! ECS and EKS don't return AccountId today, so there's no change for container users right now. If they start returning it, the SDK will pick it up, so good to update the changelog to mention about it, and added tests for container credentials as well for this.