Skip to content

dont print provided creds in BootstrapCommand #1799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

dont print provided creds in BootstrapCommand #1799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 22 additions & 26 deletions quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,45 +90,41 @@ public Integer call() {
|| inputOptions.stdinOptions.credentials.isEmpty()
? RootCredentialsSet.EMPTY
: RootCredentialsSet.fromList(inputOptions.stdinOptions.credentials);
if (inputOptions.stdinOptions.credentials == null
|| inputOptions.stdinOptions.credentials.isEmpty()) {
if (!inputOptions.stdinOptions.printCredentials) {
spec.commandLine()
.getErr()
.println(
"Specify either `--credentials` or `--print-credentials` to ensure"
+ " the root user is accessible after bootstrapping.");
return EXIT_CODE_BOOTSTRAP_ERROR;
}
if (rootCredentialsSet.credentials().isEmpty()
&& !inputOptions.stdinOptions.printCredentials) {
spec.commandLine()
.getErr()
.println(
"Specify either `--credentials` or `--print-credentials` to ensure"
+ " the root user is accessible after bootstrapping.");
return EXIT_CODE_BOOTSTRAP_ERROR;
}
}

// Execute the bootstrap
Map<String, PrincipalSecretsResult> results =
metaStoreManagerFactory.bootstrapRealms(realms, rootCredentialsSet);

// Log any errors:
boolean success = true;
for (Map.Entry<String, PrincipalSecretsResult> result : results.entrySet()) {
if (result.getValue().isSuccess()) {
String realm = result.getKey();
String realm = result.getKey();
PrincipalSecretsResult secretsResult = result.getValue();
if (secretsResult.isSuccess()) {
spec.commandLine().getOut().printf("Realm '%s' successfully bootstrapped.%n", realm);
if (inputOptions.stdinOptions != null && inputOptions.stdinOptions.printCredentials) {
String msg =
String.format(
"realm: %1s root principal credentials: %2s:%3s",
result.getKey(),
result.getValue().getPrincipalSecrets().getPrincipalClientId(),
result.getValue().getPrincipalSecrets().getMainSecret());
spec.commandLine().getOut().println(msg);
if (inputOptions.stdinOptions != null
&& inputOptions.stdinOptions.printCredentials
&& !rootCredentialsSet.credentials().containsKey(realm)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user has explicitly set printCredentials, why wouldn't we print?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the assumption is that a user would want to only print the credentials that were generated on the fly, not the ones that were provided by the user via --credential or --credentials-file.

if the assumption is false, then yeah we should still print all of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to print user-provided credentials. From the general security POV, Polaris should not add risk of exposure to user secrets if it can be avoided. Polaris prints generated secrets to make the user aware of them, but that is not required for user inputs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "print" option is still relevant when multiple realms bootstrap at the same time - some of them may get auto-generated credentials, I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to print user-provided credentials.

Then don't set the --print-credentials flag, and they shouldn't print... but if a user sets this flag, it seems odd to explicitly disregard it.

Copy link
Contributor

@dimas-b dimas-b Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eric-maynard : please consider this use case:

java -Dpolaris.persistence.type=relational-jdbc [...] -jar quarkus/admin/build/quarkus-app/quarkus-run.jar \
 bootstrap --realm r1 --realm r2 --credential r1,c1,s1 --print-credentials

Current output:

Realm 'r1' successfully bootstrapped.
realm: r1 root principal credentials: c1:614cecf1a47a82920a2df4b86c477999
Realm 'r2' successfully bootstrapped.
realm: r2 root principal credentials: 1800cece48faa5b4:dd3ae1b88675f70795b534b294992774
Bootstrap completed successfully.

Side note: the credential printed for realm r1 is not what the user provided (rotated automatically).

More importantly, I believe in this case the user should receive credentials for realm r2 (which got auto-generated), but not for realm r1 (user-provided).

The fact that r1 credentials are rotated during bootstrap is irrelevant to this PR. We can probably deal with that separately.

Copy link
Contributor

@eric-maynard eric-maynard Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior looks completely correct to me.

If the user doesn't want to print credentials for r1, they should bootstrap r1 without setting --print-credentials. It's quite straightforward.

the credential printed for realm r1 is not what the user provided (rotated automatically).

All the more reason that printing the credentials is valuable, no? But I agree that this can/should change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: the credential printed for realm r1 is not what the user provided (rotated automatically).

This has been fixed already: #801 but since then, JdbcMetaStoreManagerFactory was introduced and still rotates the secrets.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eric-maynard : from the UX perspective, WDYT about converting the current --print-credentials options into a new --generate-credentials option? The behaviour would be:

  • If no --generate-credentials and a realm does not have user-provided creds -> error
  • If --generate-credentials is set, realms without user-provided creds get generated creds and those get printed automatically?

Copy link
Contributor

@eric-maynard eric-maynard Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--generate-credentials seems redundant with the supplied credentials. That is, there are 4 modes and 2 of them would be invalid:

Credentials Provided --generate-credentials Valid
false false true
false true false
true false false
true true true

Besides that, we'd lose what appears to me a valid behavior -- echoing back non-generated credentials.

In the contrived scenario where someone wants to bootstrap N different realms, and they want to generate credentials for a value in (0, N) of them, and they have an issue with printing the non-generated credentials, they have the option of simply running two bootstrap commands. I don't think we need a breaking change to better support this scenario.

spec.commandLine()
.getOut()
.printf(
"realm: %s root principal credentials: %s:%s%n",
realm,
secretsResult.getPrincipalSecrets().getPrincipalClientId(),
secretsResult.getPrincipalSecrets().getMainSecret());
}
} else {
String realm = result.getKey();
spec.commandLine()
.getErr()
.printf(
"Bootstrapping '%s' failed: %s%n",
realm, result.getValue().getReturnStatus().toString());
.printf("Bootstrapping '%s' failed: %s%n", realm, secretsResult.getReturnStatus());
success = false;
}
}
Expand Down
6 changes: 4 additions & 2 deletions quarkus/admin/src/test/java/org/apache/polaris/admintool/BootstrapCommandTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,16 @@ public void testBootstrapFromInvalidFile(QuarkusMainLauncher launcher) {
@Test
@Launch(
value = {"bootstrap", "-r", "realm1", "-c", "realm1,client1d,s3cr3t", "--print-credentials"})
public void testPrintCredentials(LaunchResult result) {
public void testPrintCredentialsProvided(LaunchResult result) {
assertThat(result.exitCode()).isEqualTo(0);
assertThat(result.getOutput()).contains("Bootstrap completed successfully.");
assertThat(result.getOutput()).contains("realm: realm1 root principal credentials: client1d:");
assertThat(result.getOutput()).doesNotContain("root principal credentials");
}

@Test
@Launch(value = {"bootstrap", "-r", "realm1", "--print-credentials"})
public void testPrintCredentialsSystemGenerated(LaunchResult result) {
assertThat(result.exitCode()).isEqualTo(0);
assertThat(result.getOutput()).contains("Bootstrap completed successfully.");
assertThat(result.getOutput()).contains("realm: realm1 root principal credentials: ");
}
Expand Down
Loading