Simplify credentials format when bootstrapping (#806)

It turns out that only `root` credentials can be created
during bootstrap, since the principal name is hard-coded
to `PolarisEntityConstants.getRootPrincipalName()`:

https://github.com/apache/polaris/blob/390f1fa57bb1af24a21aa95fdbff49a46e31add7/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java#L612-L615

Because of that, the second element in the quadruplet `realm,name,id,secret` is useless and can be removed.

Author: adutra

polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisCredentialsBootstrap.java

@@ -25,8 +25,8 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Optional;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
 import org.apache.polaris.core.entity.PolarisPrincipalSecrets;
 
 /**
@@ -54,7 +54,7 @@ public static PolarisCredentialsBootstrap fromEnvironment() {
    * Parse a string of credentials in the format:
    *
    * <pre>
-   * realm1,user1a,client1a,secret1a;realm1,user1b,client1b,secret1b;realm2,user2a,client2a,secret2a;...
+   * realm1,client1,secret1;realm2,client2,secret2;...
    * </pre>
    */
   public static PolarisCredentialsBootstrap fromString(@Nullable String credentialsString) {
@@ -65,52 +65,51 @@ public static PolarisCredentialsBootstrap fromString(@Nullable String credential
 
   /**
    * Parse a list of credentials; each element should be in the format: {@code
-   * realm,principal,clientId,clientSecret}.
+   * realm,clientId,clientSecret}.
    */
   public static PolarisCredentialsBootstrap fromList(List<String> credentialsList) {
-    Map<String, Map<String, Map.Entry<String, String>>> credentials = new HashMap<>();
-    for (String quadruple : credentialsList) {
-      if (!quadruple.isBlank()) {
-        List<String> parts = Splitter.on(',').trimResults().splitToList(quadruple);
-        if (parts.size() != 4) {
-          throw new IllegalArgumentException("Invalid credentials format: " + quadruple);
+    Map<String, Map.Entry<String, String>> credentials = new HashMap<>();
+    for (String triplet : credentialsList) {
+      if (!triplet.isBlank()) {
+        List<String> parts = Splitter.on(',').trimResults().splitToList(triplet);
+        if (parts.size() != 3) {
+          throw new IllegalArgumentException("Invalid credentials format: " + triplet);
         }
         String realmName = parts.get(0);
-        String principalName = parts.get(1);
-        String clientId = parts.get(2);
-        String clientSecret = parts.get(3);
-        credentials
-            .computeIfAbsent(realmName, k -> new HashMap<>())
-            .merge(
-                principalName,
-                new SimpleEntry<>(clientId, clientSecret),
-                (a, b) -> {
-                  throw new IllegalArgumentException("Duplicate principal: " + principalName);
-                });
+        String clientId = parts.get(1);
+        String clientSecret = parts.get(2);
+
+        if (credentials.containsKey(realmName)) {
+          throw new IllegalArgumentException("Duplicate realm: " + realmName);
+        }
+        credentials.put(realmName, new SimpleEntry<>(clientId, clientSecret));
       }
     }
     return credentials.isEmpty() ? EMPTY : new PolarisCredentialsBootstrap(credentials);
   }
 
-  @VisibleForTesting final Map<String, Map<String, Map.Entry<String, String>>> credentials;
+  @VisibleForTesting final Map<String, Map.Entry<String, String>> credentials;
 
-  private PolarisCredentialsBootstrap(Map<String, Map<String, Entry<String, String>>> credentials) {
+  private PolarisCredentialsBootstrap(Map<String, Map.Entry<String, String>> credentials) {
     this.credentials = credentials;
   }
 
   /**
    * Get the secrets for the specified principal in the specified realm, if available among the
-   * credentials that were supplied for bootstrap.
+   * credentials that were supplied for bootstrap. Only credentials for the root principal are
+   * supported.
    */
   public Optional<PolarisPrincipalSecrets> getSecrets(
       String realmName, long principalId, String principalName) {
-    return Optional.ofNullable(credentials.get(realmName))
-        .flatMap(principals -> Optional.ofNullable(principals.get(principalName)))
-        .map(
-            credentials -> {
-              String clientId = credentials.getKey();
-              String secret = credentials.getValue();
-              return new PolarisPrincipalSecrets(principalId, clientId, secret, secret);
-            });
+    if (principalName.equals(PolarisEntityConstants.getRootPrincipalName())) {
+      return Optional.ofNullable(credentials.get(realmName))
+          .map(
+              credentials -> {
+                String clientId = credentials.getKey();
+                String secret = credentials.getValue();
+                return new PolarisPrincipalSecrets(principalId, clientId, secret, secret);
+              });
+    }
+    return Optional.empty();
   }
 }

polaris-core/src/test/java/org/apache/polaris/core/persistence/PolarisCredentialsBootstrapTest.java

@@ -21,22 +21,11 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
-import java.util.Comparator;
 import java.util.List;
-import org.apache.polaris.core.entity.PolarisPrincipalSecrets;
 import org.junit.jupiter.api.Test;
 
 class PolarisCredentialsBootstrapTest {
 
-  private final Comparator<PolarisPrincipalSecrets> comparator =
-      (a, b) ->
-          a.getPrincipalId() == b.getPrincipalId()
-                  && a.getPrincipalClientId().equals(b.getPrincipalClientId())
-                  && a.getMainSecret().equals(b.getMainSecret())
-                  && a.getSecondarySecret().equals(b.getSecondarySecret())
-              ? 0
-              : 1;
-
   @Test
   void nullString() {
     PolarisCredentialsBootstrap credentials = PolarisCredentialsBootstrap.fromString(null);
@@ -62,67 +51,63 @@ void invalidString() {
   }
 
   @Test
-  void duplicatePrincipal() {
+  void duplicateRealm() {
     assertThatThrownBy(
             () ->
                 PolarisCredentialsBootstrap.fromString(
-                    "realm1,user1a,client1a,secret1a;realm1,user1a,client1b,secret1b"))
-        .hasMessage("Duplicate principal: user1a");
+                    "realm1,client1a,secret1a;realm1,client1b,secret1b"))
+        .hasMessage("Duplicate realm: realm1");
   }
 
   @Test
   void getSecretsValidString() {
     PolarisCredentialsBootstrap credentials =
         PolarisCredentialsBootstrap.fromString(
-            " ; realm1 , user1a , client1a , secret1a ; realm1 , user1b , client1b , secret1b ; realm2 , user2a , client2a , secret2a ; ");
-    assertThat(credentials.getSecrets("realm1", 123, "nonexistent")).isEmpty();
-    assertThat(credentials.getSecrets("nonexistent", 123, "user1a")).isEmpty();
-    assertThat(credentials.getSecrets("realm1", 123, "user1a"))
-        .usingValueComparator(comparator)
-        .contains(new PolarisPrincipalSecrets(123, "client1a", "secret1a", "secret1a"));
-    assertThat(credentials.getSecrets("realm1", 123, "user1b"))
-        .usingValueComparator(comparator)
-        .contains(new PolarisPrincipalSecrets(123, "client1b", "secret1b", "secret1b"));
-    assertThat(credentials.getSecrets("realm2", 123, "user2a"))
-        .usingValueComparator(comparator)
-        .contains(new PolarisPrincipalSecrets(123, "client2a", "secret2a", "secret2a"));
+            " ; realm1 , client1 , secret1 ; realm2 , client2 , secret2 ; ");
+    assertCredentials(credentials);
   }
 
   @Test
   void getSecretsValidList() {
     PolarisCredentialsBootstrap credentials =
         PolarisCredentialsBootstrap.fromList(
-            List.of(
-                "realm1,user1a,client1a,secret1a",
-                "realm1,user1b,client1b,secret1b",
-                "realm2,user2a,client2a,secret2a"));
-    assertThat(credentials.getSecrets("realm1", 123, "nonexistent")).isEmpty();
-    assertThat(credentials.getSecrets("nonexistent", 123, "user1a")).isEmpty();
-    assertThat(credentials.getSecrets("realm1", 123, "user1a"))
-        .usingValueComparator(comparator)
-        .contains(new PolarisPrincipalSecrets(123, "client1a", "secret1a", "secret1a"));
-    assertThat(credentials.getSecrets("realm1", 123, "user1b"))
-        .usingValueComparator(comparator)
-        .contains(new PolarisPrincipalSecrets(123, "client1b", "secret1b", "secret1b"));
-    assertThat(credentials.getSecrets("realm2", 123, "user2a"))
-        .usingValueComparator(comparator)
-        .contains(new PolarisPrincipalSecrets(123, "client2a", "secret2a", "secret2a"));
+            List.of("realm1,client1,secret1", "realm2,client2,secret2"));
+    assertCredentials(credentials);
   }
 
   @Test
   void getSecretsValidSystemProperty() {
     PolarisCredentialsBootstrap credentials = PolarisCredentialsBootstrap.fromEnvironment();
     assertThat(credentials.credentials).isEmpty();
     try {
-      System.setProperty("polaris.bootstrap.credentials", "realm1,user1a,client1a,secret1a");
+      System.setProperty(
+          "polaris.bootstrap.credentials", "realm1,client1,secret1;realm2,client2,secret2");
       credentials = PolarisCredentialsBootstrap.fromEnvironment();
-      assertThat(credentials.getSecrets("realm1", 123, "nonexistent")).isEmpty();
-      assertThat(credentials.getSecrets("nonexistent", 123, "user1a")).isEmpty();
-      assertThat(credentials.getSecrets("realm1", 123, "user1a"))
-          .usingValueComparator(comparator)
-          .contains(new PolarisPrincipalSecrets(123, "client1a", "secret1a", "secret1a"));
+      assertCredentials(credentials);
     } finally {
       System.clearProperty("polaris.bootstrap.credentials");
     }
   }
+
+  private void assertCredentials(PolarisCredentialsBootstrap credentials) {
+    assertThat(credentials.getSecrets("realm3", 123, "root")).isEmpty();
+    assertThat(credentials.getSecrets("nonexistent", 123, "root")).isEmpty();
+    assertThat(credentials.getSecrets("realm1", 123, "non-root")).isEmpty();
+    assertThat(credentials.getSecrets("realm1", 123, "root"))
+        .hasValueSatisfying(
+            secrets -> {
+              assertThat(secrets.getPrincipalId()).isEqualTo(123);
+              assertThat(secrets.getPrincipalClientId()).isEqualTo("client1");
+              assertThat(secrets.getMainSecret()).isEqualTo("secret1");
+              assertThat(secrets.getSecondarySecret()).isEqualTo("secret1");
+            });
+    assertThat(credentials.getSecrets("realm2", 123, "root"))
+        .hasValueSatisfying(
+            secrets -> {
+              assertThat(secrets.getPrincipalId()).isEqualTo(123);
+              assertThat(secrets.getPrincipalClientId()).isEqualTo("client2");
+              assertThat(secrets.getMainSecret()).isEqualTo("secret2");
+              assertThat(secrets.getSecondarySecret()).isEqualTo("secret2");
+            });
+  }
 }

polaris-core/src/test/java/org/apache/polaris/core/persistence/PrincipalSecretsGeneratorTest.java

@@ -39,9 +39,8 @@ void testRandomSecrets() {
   @Test
   void testSecretOverride() {
     PrincipalSecretsGenerator gen =
-        bootstrap(
-            "test-Realm", PolarisCredentialsBootstrap.fromString("test-Realm,user1,client1,sec2"));
-    PolarisPrincipalSecrets s = gen.produceSecrets("user1", 123);
+        bootstrap("test-Realm", PolarisCredentialsBootstrap.fromString("test-Realm,client1,sec2"));
+    PolarisPrincipalSecrets s = gen.produceSecrets("root", 123);
     assertThat(s).isNotNull();
     assertThat(s.getPrincipalId()).isEqualTo(123);
     assertThat(s.getPrincipalClientId()).isEqualTo("client1");

quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java

@@ -39,7 +39,7 @@ public class BootstrapCommand extends BaseCommand {
   @CommandLine.Option(
       names = {"-c", "--credential"},
       description =
-          "Principal credentials to bootstrap. Must be of the form 'realm,userName,clientId,clientSecret'.")
+          "Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
   List<String> credentials;
 
   @Override

quarkus/admin/src/test/java/org/apache/polaris/admintool/BootstrapCommandTest.java

@@ -37,9 +37,9 @@ class BootstrapCommandTest {
         "-r",
         "realm2",
         "-c",
-        "realm1,root,root,s3cr3t",
+        "realm1,root,s3cr3t",
         "-c",
-        "realm2,root,root,s3cr3t"
+        "realm2,root,s3cr3t"
       })
   public void testBootstrap(LaunchResult result) {
     assertThat(result.getOutput()).contains("Bootstrap completed successfully.");

quarkus/service/build.gradle.kts

@@ -150,8 +150,9 @@ tasks.withType(Test::class.java).configureEach {
   if (System.getenv("AWS_REGION") == null) {
     environment("AWS_REGION", "us-west-2")
   }
-  // Note: the test secrets are referenced in DropwizardServerManager
-  environment("POLARIS_BOOTSTRAP_CREDENTIALS", "POLARIS,root,test-admin,test-secret")
+  // Note: the test secrets are referenced in
+  // org.apache.polaris.service.quarkus.it.QuarkusServerManager
+  environment("POLARIS_BOOTSTRAP_CREDENTIALS", "POLARIS,test-admin,test-secret")
   jvmArgs("--add-exports", "java.base/sun.nio.ch=ALL-UNNAMED")
   // Need to allow a java security manager after Java 21, for Subject.getSubject to work
   // "getSubject is supported only if a security manager is allowed".

site/content/in-dev/unreleased/configuring-polaris-for-production.md

@@ -75,21 +75,19 @@ Before using Polaris when using a metastore manager other than `in-memory`, you
 By default, Polaris will create randomised `CLIENT_ID` and `CLIENT_SECRET` for the `root` principal and store their hashes in the metastore backend. In order to provide your own credentials for `root` principal (so you can request tokens via `api/catalog/v1/oauth/tokens`), set the `POLARIS_BOOTSTRAP_CREDENTIALS` environment variable as follows:
 
 ```
-export POLARIS_BOOTSTRAP_CREDENTIALS=my_realm,root,my-client-id,my-client-secret
+export POLARIS_BOOTSTRAP_CREDENTIALS=my_realm,my-client-id,my-client-secret
 ```
 
-The format of the environment variable is `realm,principal,client_id,client_secret`. You can provide multiple credentials separated by `;`. For example, to provide credentials for two realms `my_realm` and `my_realm2`:
+The format of the environment variable is `realm,client_id,client_secret`. You can provide multiple credentials separated by `;`. For example, to provide credentials for two realms `my_realm` and `my_realm2`:
 
 ```
-export POLARIS_BOOTSTRAP_CREDENTIALS=my_realm,root,my-client-id,my-client-secret;my_realm2,root,my-client-id2,my-client-secret2
+export POLARIS_BOOTSTRAP_CREDENTIALS=my_realm,my-client-id,my-client-secret;my_realm2,my-client-id2,my-client-secret2
 ```
 
-You can also provide credentials for other users too. 
-
 It is also possible to use system properties to provide the credentials:
 
 ```
-java -Dpolaris.bootstrap.credentials=my_realm,root,my-client-id,my-client-secret -jar /path/to/jar/polaris-service-all.jar bootstrap polaris-server.yml
+java -Dpolaris.bootstrap.credentials=my_realm,my-client-id,my-client-secret -jar /path/to/jar/polaris-service-all.jar bootstrap polaris-server.yml
 ```
 
 Now, to bootstrap Polaris, run: