dimas-b commented on code in PR #3699:
URL: https://github.com/apache/polaris/pull/3699#discussion_r3337698100
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java:
##########
@@ -68,41 +72,96 @@
/** Azure credential vendor that supports generating SAS token */
public class AzureCredentialsStorageIntegration
- extends InMemoryStorageIntegration<AzureStorageConfigurationInfo> {
+ extends CachingStorageIntegration<AzureStorageConfigurationInfo> {
private static final Logger LOGGER =
LoggerFactory.getLogger(AzureCredentialsStorageIntegration.class);
final DefaultAzureCredential defaultAzureCredential;
- public AzureCredentialsStorageIntegration(AzureStorageConfigurationInfo
config) {
- super(config, AzureCredentialsStorageIntegration.class.getName());
+ public AzureCredentialsStorageIntegration(
+ AzureStorageConfigurationInfo storageConfig, RealmConfig realmConfig) {
+ this(null, storageConfig, realmConfig);
+ }
+
+ public AzureCredentialsStorageIntegration(
+ org.apache.polaris.core.storage.cache.StorageCredentialCache cache,
+ AzureStorageConfigurationInfo storageConfig,
+ RealmConfig realmConfig) {
+ super(cache, realmConfig, storageConfig);
// The DefaultAzureCredential will by default load the environment
variables for client id,
// client secret, tenant id
defaultAzureCredential = new DefaultAzureCredentialBuilder().build();
}
@Override
- public StorageAccessConfig getSubscopedCreds(
- @NonNull RealmConfig realmConfig,
- boolean allowListOperation,
- @NonNull Set<String> allowedReadLocations,
- @NonNull Set<String> allowedWriteLocations,
- @NonNull PolarisPrincipal polarisPrincipal,
- Optional<String> refreshCredentialsEndpoint,
- @NonNull CredentialVendingContext credentialVendingContext) {
- // Note: Azure SAS tokens do not support session tags like AWS STS.
- // The credentialVendingContext is accepted for interface compatibility
but not used.
+ protected StorageCredentialCacheKey buildCacheKey(
+ @NonNull List<LocationGrant> grants,
+ @NonNull Optional<String> refreshEndpoint,
+ @NonNull CredentialVendingContext context) {
+ return buildCacheKey(
+ allowList(grants), readLocations(grants), writeLocations(grants),
refreshEndpoint, context);
+ }
+
+ private static boolean allowList(List<LocationGrant> grants) {
+ return grants.stream()
+ .flatMap(g -> g.actions().stream())
+ .anyMatch(a -> a == PolarisStorageActions.LIST || a ==
PolarisStorageActions.ALL);
+ }
+
+ private static Set<String> readLocations(List<LocationGrant> grants) {
Review Comment:
Passing through feedback from artificial helpers (I have not reviewed this
finding yet myself 😅 )
> High
polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java:112:
readLocations() includes every grant location regardless of requested
action. Then polaris-core/src/main/java/org/
apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java:178
turns on SAS read permission whenever
that set is non-empty. A write-only LocationGrant therefore receives
read permission too, which contradicts the new
action model and the existing test intent at
polaris-core/src/test/java/org/apache/polaris/core/storage/azure/
AzureCredentialStorageIntegrationTest.java:300. For Blob SAS this can
over-grant container read access. Filter read
locations by READ/ALL instead of flattening all grants.
--
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]
