Ensure client_id/client_secret patterns are configurable

Author: rmannibucau

CHANGELOG.md

@@ -60,6 +60,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti
 - Enhanced catalog federation with SigV4 authentication support, additional authentication types for credential vending, and location-based access restrictions to block credential vending for remote tables outside allowed location lists.
 - Added `topologySpreadConstraints` support in Helm chart.
 - Added support for including principal name in subscoped credentials. `INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL` (default: false) can be used to toggle this feature. If enabled, cached credentials issued to one principal will no longer be available for others.
+- Added `CREDENTIAL_RESET_CLIENT_ID_PATTERN` and `CREDENTIAL_RESET_CLIENT_SECRET_PATTERN` (default being backward compatible) feature configuration to enable to change (enforce/relax) `client_id`/`client_secret` patterns on reset calls.
 
 ### Changes
 

polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java

@@ -409,6 +409,20 @@ public static void enforceFeatureEnabledOrThrow(
                   + "Defaults to enabled, but service providers may want to disable it.")
           .defaultValue(true)
           .buildFeatureConfiguration();
+  public static final FeatureConfiguration<String> CREDENTIAL_RESET_CLIENT_ID_PATTERN =
+      PolarisConfiguration.<String>builder()
+          .key("CREDENTIAL_RESET_CLIENT_ID_PATTERN")
+          .description(
+              "Java regex (Pattern) client_id must match to pass ENABLE_CREDENTIAL_RESET validations.")
+          .defaultValue("^[0-9a-f]{16}$")
+          .buildFeatureConfiguration();
+  public static final FeatureConfiguration<String> CREDENTIAL_RESET_CLIENT_SECRET_PATTERN =
+      PolarisConfiguration.<String>builder()
+          .key("CREDENTIAL_RESET_CLIENT_SECRET_PATTERN")
+          .description(
+              "Java regex (Pattern) client_secret must match to pass ENABLE_CREDENTIAL_RESET validations.")
+          .defaultValue("^[0-9a-f]{32}$")
+          .buildFeatureConfiguration();
 
   public static final FeatureConfiguration<Boolean>
       ALLOW_SETTING_SUB_CATALOG_RBAC_FOR_FEDERATED_CATALOGS =

polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java

@@ -24,8 +24,6 @@
 import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 /**
  * An ABC for Polaris configurations that alter the service's behavior TODO: deprecate unsafe
@@ -34,9 +32,6 @@
  * @param <T> The type of the configuration
  */
 public abstract class PolarisConfiguration<T> {
-
-  private static final Logger LOGGER = LoggerFactory.getLogger(PolarisConfiguration.class);
-
   private static final List<PolarisConfiguration<?>> allConfigurations = new ArrayList<>();
 
   private final String key;

runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java

@@ -137,13 +137,15 @@ public Response createCatalog(
   }
 
   private void validateClientId(String clientId) {
-    if (!clientId.matches("^[0-9a-f]{16}$")) {
+    if (!clientId.matches(
+        realmConfig.getConfig(FeatureConfiguration.CREDENTIAL_RESET_CLIENT_ID_PATTERN))) {
       throw new IllegalArgumentException("Invalid clientId format");
     }
   }
 
   private void validateClientSecret(String clientSecret) {
-    if (!clientSecret.matches("^[0-9a-f]{32}$")) {
+    if (!clientSecret.matches(
+        realmConfig.getConfig(FeatureConfiguration.CREDENTIAL_RESET_CLIENT_SECRET_PATTERN))) {
       throw new IllegalArgumentException("Invalid clientSecret format");
     }
   }

runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java

@@ -18,10 +18,13 @@
  */
 package org.apache.polaris.service.admin;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.when;
 
+import jakarta.annotation.Nonnull;
 import java.lang.reflect.Method;
 import java.util.List;
 import org.apache.polaris.core.admin.model.AuthenticationParameters;
@@ -31,6 +34,8 @@
 import org.apache.polaris.core.admin.model.ExternalCatalog;
 import org.apache.polaris.core.admin.model.FileStorageConfigInfo;
 import org.apache.polaris.core.admin.model.PolarisCatalog;
+import org.apache.polaris.core.admin.model.PrincipalWithCredentials;
+import org.apache.polaris.core.admin.model.ResetPrincipalRequest;
 import org.apache.polaris.core.admin.model.StorageConfigInfo;
 import org.apache.polaris.core.auth.PolarisAuthorizer;
 import org.apache.polaris.core.auth.PolarisPrincipal;
@@ -78,6 +83,10 @@ void setUp() {
     when(realmConfig.getConfig(
             FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES))
         .thenReturn(List.of("OAUTH"));
+    when(realmConfig.getConfig(FeatureConfiguration.CREDENTIAL_RESET_CLIENT_ID_PATTERN))
+        .thenReturn("^[0-9a-f]{16}$");
+    when(realmConfig.getConfig(FeatureConfiguration.CREDENTIAL_RESET_CLIENT_SECRET_PATTERN))
+        .thenReturn("^[0-9a-f]{32}$");
 
     adminService =
         new PolarisAdminService(
@@ -94,6 +103,95 @@ void setUp() {
             realmConfig, reservedProperties, adminService, serviceIdentityProvider);
   }
 
+  @Test
+  void testResetCredentialsValidatesClientId() {
+    // just fake the admin service since we do validate the validations, not the service
+    final var service = noAdminResetCredentialPolarisService();
+
+    // ok
+    assertThat(
+            service.resetCredentials(
+                "pcp",
+                new ResetPrincipalRequest("aaaaaaaaaaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+                null,
+                null))
+        .isNotNull();
+
+    // too short
+    assertThatExceptionOfType(IllegalArgumentException.class)
+        .isThrownBy(
+            () ->
+                service.resetCredentials(
+                    "pcp",
+                    new ResetPrincipalRequest(
+                        "aaaaaaaaaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+                    null,
+                    null));
+
+    // too long
+    assertThatExceptionOfType(IllegalArgumentException.class)
+        .isThrownBy(
+            () ->
+                service.resetCredentials(
+                    "pcp",
+                    new ResetPrincipalRequest(
+                        "aaaaaaaaaaaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+                    null,
+                    null));
+
+    // bad char
+    assertThatExceptionOfType(IllegalArgumentException.class)
+        .isThrownBy(
+            () ->
+                service.resetCredentials(
+                    "pcp",
+                    new ResetPrincipalRequest(
+                        "aaaaaaaaaaaaaaag", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+                    null,
+                    null));
+  }
+
+  @Test
+  void testResetCredentialsValidatesClientSecret() {
+    // just fake the admin service since we do validate the validations, not the service
+    final var service = noAdminResetCredentialPolarisService();
+
+    // Note: ok already tested for client_id so not repeating there
+
+    // too short
+    assertThatExceptionOfType(IllegalArgumentException.class)
+        .isThrownBy(
+            () ->
+                service.resetCredentials(
+                    "pcp",
+                    new ResetPrincipalRequest(
+                        "aaaaaaaaaaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+                    null,
+                    null));
+
+    // too long
+    assertThatExceptionOfType(IllegalArgumentException.class)
+        .isThrownBy(
+            () ->
+                service.resetCredentials(
+                    "pcp",
+                    new ResetPrincipalRequest(
+                        "aaaaaaaaaaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+                    null,
+                    null));
+
+    // bad char
+    assertThatExceptionOfType(IllegalArgumentException.class)
+        .isThrownBy(
+            () ->
+                service.resetCredentials(
+                    "pcp",
+                    new ResetPrincipalRequest(
+                        "aaaaaaaaaaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaag"),
+                    null,
+                    null));
+  }
+
   @Test
   void testValidateExternalCatalog_InternalCatalog() {
     StorageConfigInfo storageConfig =
@@ -237,4 +335,27 @@ private void invokeValidateExternalCatalog(PolarisServiceImpl service, Catalog c
       }
     }
   }
+
+  private PolarisServiceImpl noAdminResetCredentialPolarisService() {
+    return new PolarisServiceImpl(
+        realmConfig,
+        reservedProperties,
+        new PolarisAdminService(
+            callContext,
+            resolutionManifestFactory,
+            metaStoreManager,
+            userSecretsManager,
+            serviceIdentityProvider,
+            Mockito.mock(PolarisPrincipal.class),
+            polarisAuthorizer,
+            reservedProperties) {
+          @Nonnull
+          @Override
+          public PrincipalWithCredentials resetCredentials(
+              final String principalName, final ResetPrincipalRequest resetPrincipalRequest) {
+            return new PrincipalWithCredentials(null, null);
+          }
+        },
+        serviceIdentityProvider);
+  }
 }