dont print provided creds in BootstrapCommand #1799
Conversation
`bootstrapRealms` is called from `BootstrapCommand.call` and `QuarkusProducers.maybeBootstrap`. The latter is already only printing credentials that were not in the provided `RootCredentialsSet`. we adjust `BootstrapCommand` to do the same. the general idea is to only print credentials that were generated as part of the bootstrap.
XN137
force-pushed
the
dont-print-root-credentials
branch
from
4346048 to
ebd63c7
Compare
| spec.commandLine().getOut().println(msg); | ||
| if (inputOptions.stdinOptions != null | ||
| && inputOptions.stdinOptions.printCredentials | ||
| && !rootCredentialsSet.credentials().containsKey(realm)) { |
There was a problem hiding this comment.
If the user has explicitly set printCredentials, why wouldn't we print?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The "print" option is still relevant when multiple realms bootstrap at the same time - some of them may get auto-generated credentials, I think.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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-credentialsand a realm does not have user-provided creds -> error - If
--generate-credentialsis set, realms without user-provided creds get generated creds and those get printed automatically?
There was a problem hiding this comment.
--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.
XN137
requested review from
adutra,
ashvina,
dennishuo,
jackye1995,
jbonofre,
vvcephei,
collado-mike,
snazy,
RussellSpitzer,
takidau,
MonkeyCanCode,
flyrain,
ebyhr,
ajantha-bhat,
HonahX,
singhpk234 and
pingtimeout
as code owners
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
bootstrapRealmsis called fromBootstrapCommand.callandQuarkusProducers.maybeBootstrap.The latter is already only printing credentials that were not in the provided
RootCredentialsSet.so we adjust
BootstrapCommandto do the same.the general idea is to only print credentials that were generated as part of the bootstrap.