Skip to content

Make AWS WebIdentityToken actually working and usable from inside EKS. #12339

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.

Sign up for GitHub

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

Closed
didip wants to merge 9 commits into from
Closed

Make AWS WebIdentityToken actually working and usable from inside EKS. #12339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3e8612c
Make AWS WebIdentityToken actually working and usable from inside EKS.
didip Mar 17, 2022
26faaf3
Fix apache-rat:check error.
didip Mar 17, 2022
307ba49
Fix stylistic issues.
didip Mar 18, 2022
7f15e88
Fix the import bug I introduced due to removing static import.
didip Mar 18, 2022
e0d5c64
Attempt to satisfy code coverage.
didip Mar 18, 2022
f81f85a
Make style checker happy.
Mar 18, 2022
d4753d8
Revert the exclusion of apache-rat-plugin.
Apr 3, 2022
b9b02da
Remove trim().
Apr 4, 2022
6b2897d
Prevents NPE.
didip Apr 6, 2022
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
4 changes: 4 additions & 0 deletions cloud/aws-common/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-s3</artifactId>
</dependency>
<dependency>
Copy link
Copy Markdown
Contributor

@cryptoe cryptoe Apr 3, 2022

Choose a reason for hiding this comment

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

Is this required? I do not see this package being used anywhere in aws-common

Copy link
Copy Markdown
Contributor Author

@didip didip Apr 3, 2022

Choose a reason for hiding this comment

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

@cryptoe yes this is required. Without it being defined here, the aws-java-sdk-sts-VERSION.jar would not be downloaded into the lib/ folder.

Copy link
Copy Markdown
Contributor

@cryptoe cryptoe Apr 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Which lib folder are you taking about ? Can you post the full path of the same

Copy link
Copy Markdown
Contributor Author

@didip didip Apr 6, 2022

Choose a reason for hiding this comment

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

@cryptoe The lib folder that the tar.gz artifact contains.

Copy link
Copy Markdown
Member

@xvrl xvrl May 9, 2022

Choose a reason for hiding this comment

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

this was fixed in #12482 already, the PR explains in detail why it is required

<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-sts</artifactId>
</dependency>
<dependency>
<groupId>org.checkerframework</groupId>
<artifactId>checker-qual</artifactId>
Expand Down
2 changes: 1 addition & 1 deletion extensions-core/s3-extensions/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@
<dependency>
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-sts</artifactId>
<version>${aws.sdk.version}</version>
<scope>provided</scope>
</dependency>
<!-- Tests -->
<dependency>
Expand Down
47 changes: 31 additions & 16 deletions ...nsions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,22 +105,31 @@ public S3InputSource(
this.s3ClientSupplier = Suppliers.memoize(
() -> {
if (s3ClientBuilder != null && s3InputSourceConfig != null) {
if (s3InputSourceConfig.isCredentialsConfigured()) {
if (s3InputSourceConfig.getAssumeRoleArn() == null) {
s3ClientBuilder
.getAmazonS3ClientBuilder()
.withCredentials(createStaticCredentialsProvider(s3InputSourceConfig));
} else {
applyAssumeRole(
s3ClientBuilder,
s3InputSourceConfig,
createStaticCredentialsProvider(s3InputSourceConfig)
);
}
} else {
// Each of these if statements will manipulate s3ClientBuilder if the condition is fulfilled.

// If both static key-pair and assume role ARN are defined, use the static key-pair to assume role.
if (s3InputSourceConfig.isCredentialsConfigured() && s3InputSourceConfig.isAssumeRoleArnConfigured()) {
applyAssumeRole(
s3ClientBuilder,
s3InputSourceConfig,
createStaticCredentialsProvider(s3InputSourceConfig)
);

// If only static key-pair is defined, build the S3 client with the static key-pair
} else if (s3InputSourceConfig.isCredentialsConfigured() && !s3InputSourceConfig.isAssumeRoleArnConfigured()) {
s3ClientBuilder.getAmazonS3ClientBuilder().withCredentials(createStaticCredentialsProvider(s3InputSourceConfig));

// If assume role ARN is defined, static key-pair is undefined, and WebIdentityToken file from the environment variable is undefined.
} else if (s3InputSourceConfig.isAssumeRoleArnConfigured() && !s3InputSourceConfig.isCredentialsConfigured() && !s3InputSourceConfig.isWebIdentityTokenEnvConfigured()) {
applyAssumeRole(s3ClientBuilder, s3InputSourceConfig, awsCredentialsProvider);
}

// Actually build the ServerSideEncryptingAmazonS3 object.
return s3ClientBuilder.build();

} else if (s3ClientBuilder != null) {
return s3ClientBuilder.build();

} else {
return s3Client;
}
Expand Down Expand Up @@ -166,15 +175,21 @@ private void applyAssumeRole(
AWSCredentialsProvider awsCredentialsProvider
)
{
String assumeRoleArn = s3InputSourceConfig.getAssumeRoleArn();
if (assumeRoleArn != null) {
// Do not run if WebIdentityToken file and assumeRole ARN are detected from the environment variable,
// we want the default s3ClientBuilder behavior for ServiceAccount + eks.amazonaws.com/role-arn annotation to work.
Copy link
Copy Markdown
Contributor

@cryptoe cryptoe Apr 3, 2022

Choose a reason for hiding this comment

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

Can you please tell us how did you arrive at the above conclusion. Link to the relevant AWS documentation would be helpful.

Copy link
Copy Markdown
Contributor Author

@didip didip Apr 3, 2022

Choose a reason for hiding this comment

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

This is the default credentials provider which is turned on by default on S3 Client (provided as reference). You can see that the WebIdentityToken is in the middle (enabled when AWS_WEB_IDENTITY_TOKEN_FILE and AWS_ROLE_ARN are configured in environment variables) https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/auth/credentials/DefaultCredentialsProvider.html

I will give you a little background:

Everyone who runs Druid on EKS wants to use the ServiceAccount with annotated Role ARN because it's more secure and uses AWS's newer IMDS v2 API.

When we actually configured "ServiceAccount with annotated Role ARN", AWS_WEB_IDENTITY_TOKEN_FILE and AWS_ROLE_ARN will be configured inside the pod's environment variables.

Now, the old codebase somehow picked up the AWS_ROLE_ARN from the environment variable and populate the S3InputSourceConfig.assumeRole field. And that caused the code to execute that special assumeRole function. We don't want that to happen.

Copy link
Copy Markdown
Contributor

@cryptoe cryptoe Apr 4, 2022

Choose a reason for hiding this comment

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

Based on reading: https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRoleWithWebIdentity.html IMHO AWS_WEB_IDENTITY_TOKEN_FILE should be the lowest priority of authentication that we should support as it looks like its more supported for short duration access to AWS services.
However, I would somehow first check why AWS_ROLE_ARN got picked up. Are you specifying it in the ingestion spec somewhere?

Copy link
Copy Markdown
Contributor Author

@didip didip Apr 6, 2022

Choose a reason for hiding this comment

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

@cryptoe I would somehow first check why AWS_ROLE_ARN got picked up. Are you specifying it in the ingestion spec somewhere?

I am surprised about this too, I did not specify it anywhere inside the ingestion spec. Why did the S3InputConfig object picked it up? I could not figure that out.

if (s3InputSourceConfig.isWebIdentityTokenEnvConfigured() && s3InputSourceConfig.isAssumeRoleArnEnvConfigured()) {
Copy link
Copy Markdown
Contributor

@cryptoe cryptoe Apr 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Isnt just this condition required and we can remove all the refactoring from 108:132 ?
This is purely based on boolean logic or am I missing something

Copy link
Copy Markdown
Contributor Author

@didip didip Apr 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@cryptoe To be honest, the previous nested if statements makes it really difficult to understand what-goes-where. If in the future someone needs to add another custom function, it will be hard to do so (with the nested if statements).

return;
}

if (s3InputSourceConfig.isAssumeRoleArnConfigured()) {
String roleSessionName = StringUtils.format("druid-s3-input-source-%s", UUID.randomUUID().toString());
AWSSecurityTokenService securityTokenService = AWSSecurityTokenServiceClientBuilder.standard()
.withCredentials(awsCredentialsProvider)
.build();

STSAssumeRoleSessionCredentialsProvider.Builder roleCredentialsProviderBuilder;
roleCredentialsProviderBuilder = new STSAssumeRoleSessionCredentialsProvider
.Builder(assumeRoleArn, roleSessionName).withStsClient(securityTokenService);
.Builder(s3InputSourceConfig.getAssumeRoleArn(), roleSessionName).withStsClient(securityTokenService);

if (s3InputSourceConfig.getAssumeRoleExternalId() != null) {
roleCredentialsProviderBuilder.withExternalId(s3InputSourceConfig.getAssumeRoleExternalId());
Expand Down
23 changes: 23 additions & 0 deletions ...-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSourceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.apache.druid.data.input.s3;

import com.amazonaws.SDKGlobalConfiguration;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
Expand Down Expand Up @@ -90,6 +91,28 @@ public boolean isCredentialsConfigured()
secretAccessKey != null;
}

@JsonIgnore
public boolean isAssumeRoleArnConfigured()
{
return assumeRoleArn != null && !assumeRoleArn.isEmpty();
}

@JsonIgnore
public boolean isAssumeRoleArnEnvConfigured()
{
String conf = null;
conf = System.getenv(SDKGlobalConfiguration.AWS_ROLE_ARN_ENV_VAR);
return conf != null && !conf.isEmpty();
}

@JsonIgnore
public boolean isWebIdentityTokenEnvConfigured()
{
String conf = null;
conf = System.getenv(SDKGlobalConfiguration.AWS_WEB_IDENTITY_ENV_VAR);
return conf != null && !conf.isEmpty();
}

@Override
public String toString()
{
Expand Down
56 changes: 56 additions & 0 deletions ...ns-core/s3-extensions/src/test/java/org/apache/druid/data/input/s3/S3InputSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import com.amazonaws.SdkClientException;
import com.amazonaws.auth.AWSCredentialsProvider;
import com.amazonaws.SDKGlobalConfiguration;
import com.amazonaws.services.s3.AmazonS3;
import com.amazonaws.services.s3.AmazonS3Client;
import com.amazonaws.services.s3.AmazonS3ClientBuilder;
Expand Down Expand Up @@ -223,6 +224,61 @@ public void testSerdeWithCloudConfigPropertiesWithKeyAndSecret() throws Exceptio
EasyMock.verify(SERVER_SIDE_ENCRYPTING_AMAZON_S3_BUILDER);
}

@Test
public void testSerdeWithCloudConfigPropertiesWithIdentityFileConfigured() throws Exception
{
EasyMock.reset(SERVER_SIDE_ENCRYPTING_AMAZON_S3_BUILDER);
EasyMock.expect(SERVER_SIDE_ENCRYPTING_AMAZON_S3_BUILDER.getAmazonS3ClientBuilder())
.andStubReturn(AMAZON_S3_CLIENT_BUILDER);
EasyMock.expect(SERVER_SIDE_ENCRYPTING_AMAZON_S3_BUILDER.build())
.andReturn(SERVICE);
EasyMock.replay(SERVER_SIDE_ENCRYPTING_AMAZON_S3_BUILDER);

// Mock setting the AWS's environment variables.
String oldRoleARN = System.getProperty(SDKGlobalConfiguration.AWS_ROLE_ARN_ENV_VAR);
if(oldRoleARN.equals("")) {
System.setProperty(SDKGlobalConfiguration.AWS_ROLE_ARN_ENV_VAR, "mockROLEARN");
}

String oldIdentityFile = System.getProperty(SDKGlobalConfiguration.AWS_WEB_IDENTITY_ENV_VAR);
if(oldIdentityFile.equals("")) {
System.setProperty(SDKGlobalConfiguration.AWS_WEB_IDENTITY_ENV_VAR, "mockIdentityFile");
}

S3InputSourceConfig cloudConfigProperties = new S3InputSourceConfig(
null,
null,
null,
null
);

final S3InputSource withPrefixes = new S3InputSource(
SERVICE,
SERVER_SIDE_ENCRYPTING_AMAZON_S3_BUILDER,
INPUT_DATA_CONFIG,
null,
null,
EXPECTED_LOCATION,
cloudConfigProperties
);
final S3InputSource serdeWithPrefixes =
MAPPER.readValue(MAPPER.writeValueAsString(withPrefixes), S3InputSource.class);

// This is to force the s3ClientSupplier to initialize the ServerSideEncryptingAmazonS3
serdeWithPrefixes.createEntity(new CloudObjectLocation("bucket", "path"));

Assert.assertEquals(withPrefixes, serdeWithPrefixes);

if(oldRoleARN.equals("")) {
System.clearProperty(SDKGlobalConfiguration.AWS_ROLE_ARN_ENV_VAR);
}
if(oldIdentityFile.equals("")) {
System.clearProperty(SDKGlobalConfiguration.AWS_WEB_IDENTITY_ENV_VAR);
}

EasyMock.verify(SERVER_SIDE_ENCRYPTING_AMAZON_S3_BUILDER);
}

@Test
public void testS3InputSourceUseDefaultPasswordWhenCloudConfigPropertiesWithoutCrediential()
{
Expand Down