dimas-b commented on code in PR #3750:
URL: https://github.com/apache/polaris/pull/3750#discussion_r2849312891


##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -598,15 +605,19 @@ public LoadTableResponse createTableStaged(
       EnumSet<AccessDelegationMode> delegationModes,
       Optional<String> refreshCredentialsEndpoint) {
 
+    // Authorize first with original delegation modes to initialize resolution 
manifest
     authorizeCreateTableStaged(namespace, request, delegationModes);
 
+    // Resolve the optimal delegation mode based on catalog capabilities 
(after authorization)
+    AccessDelegationMode resolvedMode = 
resolveAccessDelegationModes(delegationModes);

Review Comment:
   I believe `resolvedMode` should go into `authorizeCreateTableStaged` (line 
609) and trickle down to `checkAllowExternalCatalogCredentialVending(...)`



##########
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 subscoping is authorized for this catalog.
+    // For internal catalogs, credential subscoping is always authorized.
+    // For external/federated catalogs, check if 
ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING is
+    // enabled.
+    boolean credentialSubscopingAuthorized =

Review Comment:
   nit: it's not "authorized" but rather "enabled" by this config.



##########
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 subscoping is authorized for this catalog.
+    // For internal catalogs, credential subscoping is always authorized.
+    // For external/federated catalogs, check if 
ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING is
+    // enabled.
+    boolean credentialSubscopingAuthorized =

Review Comment:
   also, the config check is not about "subscoping" but 
`ALLOW_FEDERATED_CATALOGS_CREDENTIAL_VENDING`



-- 
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]

Reply via email to