amaechler commented on code in PR #20540:
URL: https://github.com/apache/druid/pull/20540#discussion_r4261054945
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java:
##########
@@ -461,6 +469,31 @@ private static StaticCredentialsProvider
createStaticCredentialsProvider(S3Input
}
}
+ /**
+ * Assumes an intermediate role if one is configured.
+ */
+ public static AwsCredentialsProvider maybeAssumeIntermediateRole(
+ AwsCredentialsProvider baseCredentialsProvider,
+ @Nullable S3IntermediateRoleConfig intermediateRoleConfig,
+ @Nullable AWSEndpointConfig awsEndpointConfig,
+ @Nullable AWSClientConfig awsClientConfig
+ )
+ {
+ final String intermediateAssumeRoleArn =
+ intermediateRoleConfig == null ? null :
intermediateRoleConfig.getIntermediateAssumeRoleArn();
+ if (Strings.isNullOrEmpty(intermediateAssumeRoleArn)) {
+ return baseCredentialsProvider;
+ }
+
+ return createAssumeRoleCredentialsProvider(
+ intermediateAssumeRoleArn,
+ null,
+ awsEndpointConfig,
Review Comment:
For `S3InputSource`, `awsEndpointConfig` here is the spec's
`endpointConfig`, so the STS call for the operator's intermediate role goes to
the region of the user's bucket, or to the default region chain when the spec
has none. If that bucket is in an opt-in region the operator's account hasn't
enabled, this hop fails even though the role itself is fine. Could the
intermediate hop use the process's own region instead, for example by passing
`null` here so the default region chain applies?
##########
docs/development/extensions-core/s3.md:
##########
@@ -127,6 +129,7 @@ For example, to set the region to 'us-east-1' through
system properties:
|`druid.s3.accessKey`|S3 access key. See [S3 authentication
methods](#s3-authentication-methods) for more details|Can be omitted according
to authentication methods chosen.|
|`druid.s3.secretKey`|S3 secret key. See [S3 authentication
methods](#s3-authentication-methods) for more details|Can be omitted according
to authentication methods chosen.|
|`druid.s3.fileSessionCredentials`|Path to properties file containing
`sessionToken`, `accessKey` and `secretKey` value. One key/value pair per line
(format `key=value`). See [S3 authentication
methods](#s3-authentication-methods) for more details |Can be omitted according
to authentication methods chosen.|
+|`druid.s3.intermediateAssumeRoleArn`|AWS ARN of a role to assume before
assuming the `assumeRoleArn` given by an ingestion spec or export destination.
Lets a bucket owner's trust policy name one fixed ARN instead of a different
principal for every Druid cluster. See [S3 authentication
methods](#s3-authentication-methods) for more details.|None|
Review Comment:
With one intermediate ARN shared across clusters, the bucket owner's trust
policy can no longer tell clusters apart, and `assumeRoleArn` and
`assumeRoleExternalId` both come from whoever writes the spec or `EXPORT`
statement. Could the docs add a sentence saying that sharing one intermediate
role across clusters lets users of any of them assume any role that trusts it,
so the external ID should be set by something spec authors don't control?
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3.java:
##########
@@ -411,6 +412,13 @@ public static ServerSideEncryptingAmazonS3.Builder builder(
// Apply assume role if configured
if (!Strings.isNullOrEmpty(assumeRoleArn)) {
+ credentialsProvider = maybeAssumeIntermediateRole(
Review Comment:
When the spec supplies `accessKeyId`/`secretAccessKey`,
`credentialsProvider` here is the user's static keys, so those keys are what
assume the intermediate role. The intermediate role trusts the cluster
principal, so once an operator sets `druid.s3.intermediateAssumeRoleArn`, an
existing spec with its own keys plus `assumeRoleArn` starts failing with
AccessDenied. I checked this with a quick test on this branch: the intermediate
provider's STS client is backed by `StaticCredentialsProvider`.
It also doesn't match the new sentence in `input-sources.md`, which sits
under "If `accessKeyId` and `secretAccessKey` are not given". Should the
intermediate hop only apply when the spec has no credentials of its own?
```java
if (s3InputSourceConfig == null ||
!s3InputSourceConfig.isCredentialsConfigured()) {
credentialsProvider = maybeAssumeIntermediateRole(...);
}
```
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/data/input/s3/S3InputSource.java:
##########
@@ -129,7 +134,8 @@ public S3InputSource(
awsEndpointConfig,
awsClientConfig,
s3InputSourceConfig,
- null
+ null,
+ intermediateRoleConfig
Review Comment:
Rather than adding a new `@JacksonInject` parameter here (and to
`S3InputSourceFactory`, the test constructors and `withSplit`), could the
config sit on the `ServerSideEncryptingAmazonS3.Builder` that's already
injected, the same way `s3ClientBuilder.getS3StorageConfig()` is read a few
lines up? `getServerSideEncryptingAmazonS3Builder` already receives the config,
so it would only need to set it on the builder, and the `S3InputSourceFactory`
changes, the test-only constructor changes and the `EqualsVerifier` exclusion
drop out of the diff. I think this is close to what @kgyrtkirk was asking
about. `S3ExportStorageProvider` would still need its own injected field.
##########
extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/ServerSideEncryptingAmazonS3Test.java:
##########
@@ -282,13 +305,91 @@ public RetryStrategy getRetryStrategy()
null,
clientConfig,
null,
+ null,
null
);
Assertions.assertEquals(2, issued.size(), "one strategy per client, sync
and async");
Assertions.assertNotSame(issued.get(0), issued.get(1));
}
+ @Test
+ public void testIntermediateRoleIsAssumedBeforeTheRoleTheSpecNames() throws
Exception
Review Comment:
Could we add a test for spec-supplied `accessKeyId`/`secretAccessKey` plus
`assumeRoleArn` (see the comment on `ServerSideEncryptingAmazonS3`), so the
intended behaviour for that case is pinned down?
##########
extensions-core/s3-extensions/pom.xml:
##########
@@ -142,6 +142,11 @@
<artifactId>auth</artifactId>
<version>${aws.sdk.v2.version}</version>
</dependency>
+ <dependency>
+ <groupId>software.amazon.awssdk</groupId>
+ <artifactId>identity-spi</artifactId>
Review Comment:
Only `ServerSideEncryptingAmazonS3Test` references `identity-spi` (through
`serviceClientConfiguration().credentialsProvider()`); no main source imports
it. Should this be `<scope>test</scope>`?
##########
docs/development/extensions-core/s3.md:
##########
@@ -81,6 +81,8 @@ For more information, refer to the [Amazon Developer
Guide](https://docs.aws.ama
Alternatively, you can bypass this chain by specifying an access key and
secret key using a [Properties
Object](../../ingestion/input-sources.md#s3-input-source) inside your ingestion
specification.
+Whichever credentials are resolved are then used to assume any `assumeRoleArn`
named by an ingestion spec or export destination. Setting
`druid.s3.intermediateAssumeRoleArn` inserts one role in between: the resolved
credentials assume that role first, and the resulting credentials assume
`assumeRoleArn`.
Review Comment:
Could this section say which processes need the property? It has to be set
on every process that builds an S3 input source or export destination: Peons
(through the MiddleManager's runtime properties), Indexers, and the Overlord,
which runs the sampler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
