Skip to content

Prevent the bootstrap command from leaving root credentials unrecoverable #461

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

Merged
merged 2 commits into from
Mar 3, 2025
Merged

Prevent the bootstrap command from leaving root credentials unrecoverable #461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af50be5
rebase
eric-maynard Feb 28, 2025
e9e5305
autolint
eric-maynard Feb 28, 2025
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
25 changes: 25 additions & 0 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 @@ -57,6 +57,11 @@ static class StandardInputOptions {
description =
"Root principal credentials to bootstrap. Must be of the form 'realm,clientId,clientSecret'.")
List<String> credentials;

@CommandLine.Option(
names = {"-p", "--print-credentials"},
description = "Print root credentials to stdout")
boolean printCredentials;
}

static class FileInputOptions {
Expand Down Expand Up @@ -85,6 +90,17 @@ 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;
}
}
}

// Execute the bootstrap
Expand All @@ -97,6 +113,15 @@ public Integer call() {
if (result.getValue().isSuccess()) {
String realm = result.getKey();
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);
}
} else {
String realm = result.getKey();
spec.commandLine()
Expand Down
41 changes: 40 additions & 1 deletion quarkus/admin/src/test/java/org/apache/polaris/admintool/BootstrapCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public void testBootstrapInvalidCredentials(LaunchResult result) {
public void testBootstrapInvalidArguments(LaunchResult result) {
assertThat(result.getErrorOutput())
.contains(
"Error: (-r=<realm> [-r=<realm>]... [-c=<realm,clientId,clientSecret>]...) "
"Error: (-r=<realm> [-r=<realm>]... [-c=<realm,clientId,clientSecret>]... [-p]) "
+ "and -f=<file> are mutually exclusive (specify only one)");
}

Expand Down Expand Up @@ -132,6 +132,45 @@ public void testBootstrapFromInvalidFile(QuarkusMainLauncher launcher) {
.contains("Bootstrap encountered errors during operation.");
}

@Test
@Launch(
value = {"bootstrap", "-r", "realm1", "-c", "realm1,client1d,s3cr3t", "--print-credentials"})
public void testPrintCredentials(LaunchResult result) {
assertThat(result.getOutput()).contains("Bootstrap completed successfully.");
assertThat(result.getOutput()).contains("realm: realm1 root principal credentials: client1d:");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not assert that s3cr3t is printed?

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

It's actually not printed -- maybe it should be. Because of #801 the secret supplied becomes the secondary secret while the primary secret is printed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would rather we just solve #801 rather than add special logic around printing the secondary secret here

Copy link
Contributor

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

Choose a reason for hiding this comment

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

TIL. I think this PR is good to merge then, and we'll follow-up on #801

}

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

@Test
@Launch(
value = {"bootstrap", "-r", "realm1"},
exitCode = EXIT_CODE_BOOTSTRAP_ERROR)
public void testNoPrintCredentialsSystemGenerated(LaunchResult result) {
assertThat(result.getErrorOutput()).contains("--credentials");
assertThat(result.getErrorOutput()).contains("--print-credentials");
}

@Test
@Launch(
value = {
"bootstrap",
"-r",
"realm1",
"--not-real-arg",
},
exitCode = EXIT_CODE_USAGE)
public void testBootstrapInvalidArg(LaunchResult result) {
assertThat(result.getErrorOutput())
.contains("Unknown option: '--not-real-arg'")
.contains("Usage:");
}

private static Path copyResource(Path temp, String resource) throws IOException {
URL source = Objects.requireNonNull(BootstrapCommandTest.class.getResource(resource));
Path dest = temp.resolve(resource);
Expand Down