Support providing root client ID via env. variables when bootstrapping #422
Conversation
dimas-b
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike and
ebyhr
as code owners
|
I recall that @collado-mike and I discussed this as one option when bootstrapping, but I can't remember why we went with the current approach. For my part, I would ideally like to see as little a difference in the UX of the different metastores as possible. |
Do you mean supporting the same overrides under the |
|
Taking a step back, and thinking about real-life scenarios: how is an operator supposed to get hold of the root credentials, after installing and bootstrapping Polaris? I read the Polaris documentation about production setups, it does cover bootstrapping, but it doesn't cover this detail in particular, so I guess the exact procedure depends on the metastore being used? Wouldn't it be possible to standardize that? I'm also raising this point because while working on the Quarkus port, I realized that it is not possible to create a true integration test currently, that is, a test that spawns Polaris as a black box, and only interacts with it through its external APIs. This is because once started and bootstrapped, and regardless of the metastore used, the test is unable to infer which credentials to use to communicate with Polaris. All in all, I wonder if we aren't missing a simple and standard way to pass the root credentials to the boostrapping process, even in production setups – or alternatively, a way to retrieve those, that would also be standardized across metastores. |
My personal take on that is that the |
|
A few thoughts here.
|
|
@eric-maynard : thanks for sharing your views on this matter. Re: security of bootstrapping, I did not mean printing credentials to STDOUT. The "output" can be a file at a user-specified location. I think this is quite similar to downloading ssh certificates for a VM from a cloud vendor. I wonder what options end users currently have for bootstrapping, though 🤔 How will a user be able to discover a generated root credential? (Apologies if I missed this in somewhere in docs). |
|
I agree that we need to proceed carefully since it's a security-related issue. I'd note, fwiw, that Keycloak allows to bootstrap the root credentials via environment variables. I don't think we can suspect Keycloak of taking security lightly :-) In another place of Keycloak docs, we can learn more about how the environment variables are processed:
Couldn't we do something similar to that? |
If we go down this route, maybe check that the file permissions are as restrictive as possible. |
So if you're just doing testing like this PR would address, you can just use passwordless auth. You normally don't need the root credentials. However if you're asking more generally how to retrieve the root credentials it is metastore-dependent. Ideally your metastore is set up with your auth provider and allows you to do something nice like set/retrieve the credentials using SSO. Or perhaps they are exposed through some API which is already secured. But in the simplest case where you're just using a postgres metastore, you can retrieve them through something like: |
I could use So this PR proposes to make this an option for the user to define the root credential if the user so chooses. I think it could be convenient for other people too. As for the general bootstrapping case, the above discussion is interesting, but maybe we can continue that on the dev list or separate PR. As for this PR, do you think the idea of allowing user overrides for root credentials is reasonable given that it only applies to the "test" authentication implementation, which is already not "secret" given the fixed root token? |
|
I see. If you want to test the auth flow and you can't call Could you call Barring that, I am open to allowing the user to set the credentials using an env variable for the in-memory metastore. |
|
It is a fairly common and accepted practice to generate random secrets during bootstrapping - e.g., terraform has built-in support for this - https://registry.terraform.io/providers/hashicorp/random/latest/docs/resources/password . Allowing Terraform to randomly generate a password, bootstrap Polaris, and stick the secrets in Vault or k8s secrets is a very secure pattern and something I think we ought to support. |
And I'd note that that is the usual pattern that most Helm charts adopt when bootstrapping things like databases. |
|
SGTM, based on the above discussion, I think I'll extend the env. variables support to the normal |
For "in memory" use cases allow the user to define the root client ID via environment variables. This is to simplify local testing. The standalone `bootstrap` command and on-demand principal creation call paths still generate random IDs and secrets as before.
dimas-b
force-pushed
the
root-id-override-for-test
branch
from
aff2823
to
3d5640f
Compare
dimas-b
changed the title
| public synchronized Map<String, PolarisMetaStoreManager.PrincipalSecretsResult> bootstrapRealms( | ||
| List<String> realms) { | ||
| public Map<String, PolarisMetaStoreManager.PrincipalSecretsResult> bootstrapRealms( | ||
| List<String> realms, Function<String, PrincipalSecretsGenerator> rootSecretsPerRealm) { |
There was a problem hiding this comment.
I like the PrincipalSecretsGenerator. Is it necessary to have it per realm, though? Can it take the realm as a method argument instead
There was a problem hiding this comment.
I'd probably have to add realm to PolarisCallContext.
From my POV, I wanted to avoid exposing realm to lower-level code for better abstraction.
In practice only one realm is normally bootstrapped at a time, so we can probably drop the realm parameter completely and use env. vars. like POLARIS_BOOTSTRAP_ROOT_CLIENT_ID. WDYT?
| public @NotNull BaseResult bootstrapPolarisService( | ||
| @NotNull PolarisCallContext callCtx, PrincipalSecretsGenerator rootSecretsGenerator) { |
There was a problem hiding this comment.
Rather than changing the public interface, can we make the PrincipalSecretsGenerator a constructor param? I don't think callers need to know anything about this
There was a problem hiding this comment.
Different generators are used during bootstrapping and (REST) API-driven principal creation.
| public @NotNull PolarisPrincipalSecrets generateNewPrincipalSecrets( | ||
| @NotNull PolarisCallContext callCtx, @NotNull String principalName, long principalId) { | ||
| @NotNull PolarisCallContext callCtx, | ||
| @NotNull String principalName, | ||
| long principalId, | ||
| PrincipalSecretsGenerator generator) { |
There was a problem hiding this comment.
Same here - rather than passing in the generator as an argument, I think it should be a constructor argument
Allow the root client ID and secrets to be provided via environment variables when bootstrapping.
For "in memory" use cases, this mean the main Polaris server will read user-provided root secrets from the env., if propvided.
For "persistent" use cases, the
bootstrapcommand will read user-provided root secrets from the env., if propvided.The env. variables are:
POLARIS_BOOTSTRAP_<REALM>_ROOT_CLIENT_IDPOLARIS_BOOTSTRAP_<REALM>_ROOT_CLIENT_SECRETIf these variables are not provided, random values are generated as before.
How Has This Been Tested?
Manual smoke tests.