adnanhemani commented on code in PR #3170:
URL: https://github.com/apache/polaris/pull/3170#discussion_r2566086594
##########
client/python/apache_polaris/cli/command/__init__.py:
##########
@@ -54,6 +54,7 @@ def options_get(key, f=lambda x: x):
allowed_locations=options_get(Arguments.ALLOWED_LOCATION),
role_arn=options_get(Arguments.ROLE_ARN),
external_id=options_get(Arguments.EXTERNAL_ID),
+ user_token_sts=options_get(Arguments.USER_TOKEN_STS),
Review Comment:
I find "user_token_sts" not so clear. Given that these tokens must be
WebIdentityTokens (as per STS spec), I'd suggest renaming to something more
specific, like "use_sts_web_identity_token".
Or, I think this property could be useful for similar features in Azure/GCP
in the future (e.g. Azure's Federated Credentials or GCP's Workload Identity
Federation) - and maybe we keep it something a bit vague like
"use_subject_token" so as to not make it specifically named for STS.
@dimas-b WDYT?
##########
runtime/service/build.gradle.kts:
##########
@@ -30,7 +30,6 @@ dependencies {
implementation(project(":polaris-api-management-service"))
implementation(project(":polaris-api-iceberg-service"))
implementation(project(":polaris-api-catalog-service"))
-
Review Comment:
Unnecessary change
##########
runtime/service/src/test/java/org/apache/polaris/service/auth/external/OidcPolarisCredentialAugmentorTest.java:
##########
@@ -109,7 +109,7 @@ public void testAugmentOidcPrincipal() {
.addRole("ROLE1")
.addAttribute(TENANT_CONFIG_ATTRIBUTE, config)
.build();
-
+ when(oidcPrincipal.getRawToken()).thenReturn("this_is_a_token");
Review Comment:
nit: let's separate "this_is_a_token" into a shared variable.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -90,35 +96,46 @@ public StorageAccessConfig getSubscopedCreds(
StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder();
if (shouldUseSts(storageConfig)) {
- AssumeRoleRequest.Builder request =
- AssumeRoleRequest.builder()
- .externalId(storageConfig.getExternalId())
- .roleArn(storageConfig.getRoleARN())
- .roleSessionName("PolarisAwsCredentialsStorageIntegration")
- .policy(
- policyString(
- storageConfig,
- allowListOperation,
- allowedReadLocations,
- allowedWriteLocations,
- region,
- accountId)
- .toJson())
- .durationSeconds(storageCredentialDurationSeconds);
- credentialsProvider.ifPresent(
- cp -> request.overrideConfiguration(b -> b.credentialsProvider(cp)));
@SuppressWarnings("resource")
// Note: stsClientProvider returns "thin" clients that do not need
closing
StsClient stsClient =
stsClientProvider.stsClient(StsDestination.of(storageConfig.getStsEndpointUri(),
region));
-
- AssumeRoleResponse response = stsClient.assumeRole(request.build());
- accessConfig.put(StorageAccessProperty.AWS_KEY_ID,
response.credentials().accessKeyId());
- accessConfig.put(
- StorageAccessProperty.AWS_SECRET_KEY,
response.credentials().secretAccessKey());
- accessConfig.put(StorageAccessProperty.AWS_TOKEN,
response.credentials().sessionToken());
- Optional.ofNullable(response.credentials().expiration())
+ Credentials credentials;
+ if (storageConfig.getUserTokenSTS()) {
+ AssumeRoleWithWebIdentityRequest.Builder request =
+ AssumeRoleWithWebIdentityRequest.builder()
+ .webIdentityToken(
Review Comment:
To be clear, I'm making this assumption that the token is a user's token
based off of
https://github.com/apache/polaris/pull/3170/files#diff-204e04f3365ea6ec898018ebfe943ca22277ae4643b58d1ca31462f70662c618R94.
If the SecurityIdentity is populating with the server's token (not sure this
makes sense to me, but feel free to correct me), then that may open a different
set of questions.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -90,35 +96,46 @@ public StorageAccessConfig getSubscopedCreds(
StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder();
if (shouldUseSts(storageConfig)) {
- AssumeRoleRequest.Builder request =
- AssumeRoleRequest.builder()
- .externalId(storageConfig.getExternalId())
- .roleArn(storageConfig.getRoleARN())
- .roleSessionName("PolarisAwsCredentialsStorageIntegration")
- .policy(
- policyString(
- storageConfig,
- allowListOperation,
- allowedReadLocations,
- allowedWriteLocations,
- region,
- accountId)
- .toJson())
- .durationSeconds(storageCredentialDurationSeconds);
- credentialsProvider.ifPresent(
- cp -> request.overrideConfiguration(b -> b.credentialsProvider(cp)));
@SuppressWarnings("resource")
// Note: stsClientProvider returns "thin" clients that do not need
closing
StsClient stsClient =
stsClientProvider.stsClient(StsDestination.of(storageConfig.getStsEndpointUri(),
region));
-
- AssumeRoleResponse response = stsClient.assumeRole(request.build());
- accessConfig.put(StorageAccessProperty.AWS_KEY_ID,
response.credentials().accessKeyId());
- accessConfig.put(
- StorageAccessProperty.AWS_SECRET_KEY,
response.credentials().secretAccessKey());
- accessConfig.put(StorageAccessProperty.AWS_TOKEN,
response.credentials().sessionToken());
- Optional.ofNullable(response.credentials().expiration())
+ Credentials credentials;
+ if (storageConfig.getUserTokenSTS()) {
+ AssumeRoleWithWebIdentityRequest.Builder request =
+ AssumeRoleWithWebIdentityRequest.builder()
+ .webIdentityToken(
Review Comment:
I'm not sure I understand this part properly (I read through the email
thread as well) - are we passing in the user's token directly to STS? If so,
what type of token is this: a Polaris authN token or a WebIdentityToken?
If a Polaris AuthN token, how does STS translate between this token and an
IAM trust policy enforcement - which should state which users are allowed to
assume this role? And if such a translation does exist, where in Polaris is
this logic?
If a WebIdentityToken, then what stops this token from contacting STS
directly to assume the role and get credentials for S3?
I understand there may be some special rules you have in place for an
on-prem solution that may limit the ability of a user to contact STS directly
or to translate between a Polaris token and an identity that STS can understand
- but this seems to be breaking the general architecture of Polaris where
Polaris serves as a trusted credential broker and clients are assumed to not be
able to bypass Polaris in order to get storage credentials.
##########
.gitignore:
##########
@@ -105,4 +105,5 @@ hs_err_pid*
.vscode/
# Python Virtual Env
-.venv
\ No newline at end of file
+.venv
+venv
Review Comment:
Unnecessary change
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -90,35 +96,46 @@ public StorageAccessConfig getSubscopedCreds(
StorageAccessConfig.Builder accessConfig = StorageAccessConfig.builder();
if (shouldUseSts(storageConfig)) {
- AssumeRoleRequest.Builder request =
- AssumeRoleRequest.builder()
- .externalId(storageConfig.getExternalId())
- .roleArn(storageConfig.getRoleARN())
- .roleSessionName("PolarisAwsCredentialsStorageIntegration")
- .policy(
- policyString(
- storageConfig,
- allowListOperation,
- allowedReadLocations,
- allowedWriteLocations,
- region,
- accountId)
- .toJson())
- .durationSeconds(storageCredentialDurationSeconds);
- credentialsProvider.ifPresent(
- cp -> request.overrideConfiguration(b -> b.credentialsProvider(cp)));
@SuppressWarnings("resource")
// Note: stsClientProvider returns "thin" clients that do not need
closing
StsClient stsClient =
stsClientProvider.stsClient(StsDestination.of(storageConfig.getStsEndpointUri(),
region));
-
- AssumeRoleResponse response = stsClient.assumeRole(request.build());
- accessConfig.put(StorageAccessProperty.AWS_KEY_ID,
response.credentials().accessKeyId());
- accessConfig.put(
- StorageAccessProperty.AWS_SECRET_KEY,
response.credentials().secretAccessKey());
- accessConfig.put(StorageAccessProperty.AWS_TOKEN,
response.credentials().sessionToken());
- Optional.ofNullable(response.credentials().expiration())
+ Credentials credentials;
+ if (storageConfig.getUserTokenSTS()) {
+ AssumeRoleWithWebIdentityRequest.Builder request =
+ AssumeRoleWithWebIdentityRequest.builder()
+ .webIdentityToken(
+
securityIdentity.getCredential(PolarisCredential.class).getToken())
+ .roleArn(storageConfig.getRoleARN())
+ .roleSessionName("PolarisAwsCredentialsStorageIntegration")
+ .durationSeconds(storageCredentialDurationSeconds);
Review Comment:
Don't see a `policy` block added here (like it is in the non-token code
path). If this is intentional, please explain why the scoping-down of
credentials isn't applicable here?
##########
runtime/service/build.gradle.kts:
##########
@@ -103,7 +102,7 @@ dependencies {
implementation("com.fasterxml.jackson.core:jackson-annotations")
implementation("com.fasterxml.jackson.core:jackson-core")
implementation("com.fasterxml.jackson.core:jackson-databind")
-
Review Comment:
Unnecessary change
--
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]
