singhpk234 commented on code in PR #3327:
URL: https://github.com/apache/polaris/pull/3327#discussion_r2651774626
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -366,4 +388,67 @@ private static String arnPrefixForPartition(String
awsPartition) {
}
return path;
}
+
+ /**
+ * Builds a list of AWS STS session tags from the credential vending context
and principal name.
+ * These tags will appear in CloudTrail events for correlation purposes.
+ *
+ * @param principalName the name of the principal requesting credentials
+ * @param context the credential vending context containing catalog,
namespace, table, and roles
+ * @return a list of STS Tags to attach to the AssumeRole request
+ */
+ private List<Tag> buildSessionTags(String principalName,
CredentialVendingContext context) {
Review Comment:
should we move this to a dedicated utility class ?
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -91,6 +91,19 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();
+ public static final FeatureConfiguration<Boolean>
INCLUDE_SESSION_TAGS_IN_SUBSCOPED_CREDENTIAL =
+ PolarisConfiguration.<Boolean>builder()
+ .key("INCLUDE_SESSION_TAGS_IN_SUBSCOPED_CREDENTIAL")
+ .description(
+ "If set to true, session tags (catalog, namespace, table,
principal, roles) will be included\n"
+ + "in AWS STS AssumeRole requests for credential vending.
These tags appear in CloudTrail events,\n"
+ + "enabling correlation between catalog operations and S3
data access.\n"
+ + "Requires the IAM role trust policy to allow
sts:TagSession action.\n"
+ + "Note that enabling this feature leads to degradation in
temporary credential caching as \n"
+ + "catalog will no longer be able to reuse credentials for
different tables/namespaces/roles.")
Review Comment:
```suggestion
+ "Note that enabling this feature may lead to degradation
in temporary credential caching as \n"
+ "catalog will no longer be able to reuse credentials for
different tables/namespaces/roles.")
```
i see allowedLocation is part of the table for general case we have
overlapping check, having MAY is neutral in a sense it will not scare folks.
though i am not sure why just principal is in the key and not the roles,
@dimas-b you know why ?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -366,4 +388,67 @@ private static String arnPrefixForPartition(String
awsPartition) {
}
return path;
}
+
+ /**
+ * Builds a list of AWS STS session tags from the credential vending context
and principal name.
+ * These tags will appear in CloudTrail events for correlation purposes.
+ *
+ * @param principalName the name of the principal requesting credentials
+ * @param context the credential vending context containing catalog,
namespace, table, and roles
+ * @return a list of STS Tags to attach to the AssumeRole request
+ */
+ private List<Tag> buildSessionTags(String principalName,
CredentialVendingContext context) {
+ List<Tag> tags = new ArrayList<>();
+
+ // Always include all tags with "unknown" placeholder for missing values
+ // This ensures consistent tag presence in CloudTrail for correlation
+ tags.add(
+ Tag.builder()
+ .key(CredentialVendingContext.TAG_KEY_PRINCIPAL)
+ .value(truncateTagValue(principalName))
+ .build());
+ tags.add(
+ Tag.builder()
+ .key(CredentialVendingContext.TAG_KEY_CATALOG)
+ .value(truncateTagValue(context.catalogName().orElse(null)))
+ .build());
+ tags.add(
+ Tag.builder()
+ .key(CredentialVendingContext.TAG_KEY_NAMESPACE)
+ .value(truncateTagValue(context.namespace().orElse(null)))
+ .build());
+ tags.add(
+ Tag.builder()
+ .key(CredentialVendingContext.TAG_KEY_TABLE)
+ .value(truncateTagValue(context.tableName().orElse(null)))
+ .build());
+ tags.add(
+ Tag.builder()
+ .key(CredentialVendingContext.TAG_KEY_ROLES)
+ .value(truncateTagValue(context.activatedRoles().orElse(null)))
+ .build());
Review Comment:
minor: i would place this next to the prinicipal role for better readibility
--
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]
