flyingImer commented on code in PR #3750: URL: https://github.com/apache/polaris/pull/3750#discussion_r2891940172
########## runtime/service/src/main/java/org/apache/polaris/service/catalog/DefaultAccessDelegationModeResolver.java: ########## @@ -0,0 +1,213 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.catalog; + +import static org.apache.polaris.service.catalog.AccessDelegationMode.REMOTE_SIGNING; +import static org.apache.polaris.service.catalog.AccessDelegationMode.UNKNOWN; +import static org.apache.polaris.service.catalog.AccessDelegationMode.VENDED_CREDENTIALS; + +import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; +import jakarta.enterprise.context.RequestScoped; +import jakarta.inject.Inject; +import java.util.EnumSet; +import org.apache.polaris.core.config.FeatureConfiguration; +import org.apache.polaris.core.config.RealmConfig; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.entity.CatalogEntity; +import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo; +import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Default implementation of {@link AccessDelegationModeResolver} that resolves the optimal access + * delegation mode based on catalog capabilities and configuration. + * + * <p>The resolution logic: + * + * <ol> + * <li>If no delegation mode is requested, returns {@link AccessDelegationMode#UNKNOWN} + * <li>If exactly one delegation mode is requested (excluding UNKNOWN), returns that mode + * <li>If both {@link AccessDelegationMode#VENDED_CREDENTIALS} and {@link + * AccessDelegationMode#REMOTE_SIGNING} are requested: + * <ul> + * <li>If STS is unavailable for the catalog's AWS storage, returns {@link + * AccessDelegationMode#REMOTE_SIGNING} + * <li>If credential subscoping is skipped, returns {@link + * AccessDelegationMode#REMOTE_SIGNING} + * <li>Otherwise, returns {@link AccessDelegationMode#VENDED_CREDENTIALS} + * </ul> + * </ol> + */ +@RequestScoped +public class DefaultAccessDelegationModeResolver implements AccessDelegationModeResolver { + + private static final Logger LOGGER = + LoggerFactory.getLogger(DefaultAccessDelegationModeResolver.class); + + private final RealmConfig realmConfig; + + @Inject + public DefaultAccessDelegationModeResolver(CallContext callContext) { + this.realmConfig = callContext.getRealmConfig(); + } + + @Override + @Nonnull + public AccessDelegationMode resolve( + @Nonnull EnumSet<AccessDelegationMode> requestedModes, + @Nullable CatalogEntity catalogEntity) { + + // Case 1: No delegation mode requested + if (requestedModes.isEmpty()) { + LOGGER.debug("No delegation mode requested, returning UNKNOWN"); + return UNKNOWN; + } + + // Filter out UNKNOWN mode from consideration for selection logic + EnumSet<AccessDelegationMode> effectiveModes = EnumSet.copyOf(requestedModes); + effectiveModes.remove(UNKNOWN); + + if (effectiveModes.isEmpty()) { + LOGGER.debug("Only UNKNOWN mode requested, returning UNKNOWN"); + return UNKNOWN; + } + + // Case 2: Exactly one delegation mode requested + if (effectiveModes.size() == 1) { + AccessDelegationMode mode = effectiveModes.iterator().next(); + LOGGER.debug("Single delegation mode requested: {}", mode); + return mode; + } + + // Case 3: Both VENDED_CREDENTIALS and REMOTE_SIGNING requested + if (effectiveModes.contains(VENDED_CREDENTIALS) && effectiveModes.contains(REMOTE_SIGNING)) { + return resolveVendedCredentialsVsRemoteSigning(catalogEntity); + } + + // Case 4: Unknown combination - reject to prevent unintended credential exposure + throw new IllegalArgumentException( + "Unsupported access delegation mode combination: " + requestedModes); + } + + /** + * Resolves between VENDED_CREDENTIALS and REMOTE_SIGNING based on catalog capabilities. + * + * <p>The logic prefers VENDED_CREDENTIALS when: + * + * <ul> + * <li>STS is available for the catalog's storage (for AWS) + * <li>Credential subscoping is not skipped + * </ul> + * + * <p>Otherwise, REMOTE_SIGNING is preferred as it doesn't require STS. + */ + private AccessDelegationMode resolveVendedCredentialsVsRemoteSigning( + @Nullable CatalogEntity catalogEntity) { + + // If no catalog entity available, default to VENDED_CREDENTIALS for backward compatibility + if (catalogEntity == null) { + LOGGER.debug( + "No catalog entity available for mode resolution, defaulting to VENDED_CREDENTIALS"); + return VENDED_CREDENTIALS; + } + + // Check if credential vending is enabled for this catalog. + // For internal catalogs, credential vending is always enabled. + // For external/federated catalogs, check if ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING is Review Comment: I'm a bit confused here, thought `ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING` is meant for external catalogs while `ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING` is for federated catalogs Resolver picks VENDED_CREDENTIALS (because `ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING`=true) but the handler's subsequent check rejects it (if `ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING`=false), seems one of them to be redundant ########## runtime/service/src/test/java/org/apache/polaris/service/catalog/AccessDelegationModeResolverTest.java: ########## @@ -0,0 +1,353 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.catalog; + +import static org.apache.polaris.service.catalog.AccessDelegationMode.REMOTE_SIGNING; +import static org.apache.polaris.service.catalog.AccessDelegationMode.UNKNOWN; +import static org.apache.polaris.service.catalog.AccessDelegationMode.VENDED_CREDENTIALS; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; + +import java.util.EnumSet; +import java.util.Map; +import org.apache.polaris.core.admin.model.Catalog; +import org.apache.polaris.core.config.PolarisConfiguration; +import org.apache.polaris.core.config.RealmConfig; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.entity.CatalogEntity; +import org.apache.polaris.core.entity.PolarisEntityConstants; +import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo; +import org.apache.polaris.core.storage.azure.AzureStorageConfigurationInfo; +import org.apache.polaris.core.storage.gcp.GcpStorageConfigurationInfo; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +@SuppressWarnings("unchecked") +class AccessDelegationModeResolverTest { + + @Mock private RealmConfig realmConfig; + @Mock private CallContext callContext; + + private AccessDelegationModeResolver resolver; + + @BeforeEach + void setUp() { + when(callContext.getRealmConfig()).thenReturn(realmConfig); + resolver = new DefaultAccessDelegationModeResolver(callContext); + } + + /** Helper to set up config mock for tests that need it */ + private void mockSkipCredentialSubscopingConfig(boolean skipCredentialSubscoping) { + when(realmConfig.getConfig(org.mockito.ArgumentMatchers.<PolarisConfiguration<Boolean>>any())) + .thenReturn(skipCredentialSubscoping); + } + + /** + * Helper to set up config mock for external catalog tests. + * + * @param skipCredentialSubscoping whether to skip credential subscoping + * @param allowFederatedCredentialVending whether to allow federated catalog credential vending + */ + private void mockConfigForExternalCatalog( + boolean skipCredentialSubscoping, boolean allowFederatedCredentialVending) { + // Mock ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING (catalog-level) + when(realmConfig.getConfig( + org.mockito.ArgumentMatchers.<PolarisConfiguration<Boolean>>any(), + org.mockito.ArgumentMatchers.<CatalogEntity>any())) + .thenReturn(allowFederatedCredentialVending); + + // Mock SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION (realm-level only) + when(realmConfig.getConfig(org.mockito.ArgumentMatchers.<PolarisConfiguration<Boolean>>any())) + .thenReturn(skipCredentialSubscoping); + } + + @Test + void resolveEmptyModes_returnsUnknown() { Review Comment: is it the same as `resolve_emptyModes_returnsUnknown`? ########## runtime/service/src/test/java/org/apache/polaris/service/catalog/AccessDelegationModeResolverTest.java: ########## @@ -0,0 +1,353 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.catalog; + +import static org.apache.polaris.service.catalog.AccessDelegationMode.REMOTE_SIGNING; +import static org.apache.polaris.service.catalog.AccessDelegationMode.UNKNOWN; +import static org.apache.polaris.service.catalog.AccessDelegationMode.VENDED_CREDENTIALS; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; + +import java.util.EnumSet; +import java.util.Map; +import org.apache.polaris.core.admin.model.Catalog; +import org.apache.polaris.core.config.PolarisConfiguration; +import org.apache.polaris.core.config.RealmConfig; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.entity.CatalogEntity; +import org.apache.polaris.core.entity.PolarisEntityConstants; +import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo; +import org.apache.polaris.core.storage.azure.AzureStorageConfigurationInfo; +import org.apache.polaris.core.storage.gcp.GcpStorageConfigurationInfo; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +@SuppressWarnings("unchecked") +class AccessDelegationModeResolverTest { Review Comment: I believe we missed a negative test for `IllegalArgumentException` ########## runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java: ########## @@ -1313,13 +1339,53 @@ private void checkAllowExternalCatalogCredentialVending( && !realmConfig() .getConfig( FeatureConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING, catalogEntity)) { + + String modeDescription = + switch (resolvedMode) { + case VENDED_CREDENTIALS -> "Credential vending"; + case REMOTE_SIGNING -> "Request signing"; + default -> "Access delegation"; + }; + throw new ForbiddenException( - "Access Delegation is not enabled for this catalog. Please consult applicable " + "%s is not enabled for this external catalog. Please consult applicable " + "documentation for the catalog config property '%s' to enable this feature", + modeDescription, FeatureConfiguration.ALLOW_EXTERNAL_CATALOG_CREDENTIAL_VENDING.catalogConfig()); } } + /** + * Resolves the access delegation mode by delegating to the configured {@link + * AccessDelegationModeResolver}. + * + * <p>If no modes are requested, returns {@link AccessDelegationMode#UNKNOWN} immediately. + * Otherwise, delegates to the resolver to determine the optimal mode based on catalog + * capabilities. + * + * @param requestedModes The set of delegation modes requested by the client + * @return The resolved access delegation mode + */ + protected AccessDelegationMode resolveAccessDelegationModes( + EnumSet<AccessDelegationMode> requestedModes) { + if (requestedModes.isEmpty()) { + return AccessDelegationMode.UNKNOWN; + } Review Comment: nit: do we need to duplicate this logic here as well? where IcebergCatalogHandler already covers it -- 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]
