-
Notifications
You must be signed in to change notification settings - Fork 132
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.
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
Add a flag to control whether credentials are printed during bootstrapping #461
base: main
Are you sure you want to change the base?
Changes from 6 commits
65c3012
d9a1698
7b1e258
312453b
6e14558
4a3b1fd
ffcdf49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.polaris.core.persistence.secrets; | ||
|
||
import org.apache.polaris.core.entity.PolarisPrincipalSecrets; | ||
import org.jetbrains.annotations.NotNull; | ||
import org.jetbrains.annotations.Nullable; | ||
|
||
public class DefaultPrincipalSecretsGenerator extends PrincipalSecretsGenerator { | ||
|
||
public DefaultPrincipalSecretsGenerator(@Nullable String realmName) { | ||
super(realmName); | ||
} | ||
|
||
private PrincipalSecretsGenerator getDelegate( | ||
@Nullable String realmName, @NotNull String principalName) { | ||
var envVarGenerator = new EnvVariablePrincipalSecretsGenerator(realmName); | ||
if (envVarGenerator.systemGeneratedSecrets(principalName)) { | ||
return new RandomPrincipalSecretsGenerator(realmName); | ||
} else { | ||
return envVarGenerator; | ||
} | ||
} | ||
|
||
@Override | ||
public PolarisPrincipalSecrets produceSecrets(@NotNull String principalName, long principalId) { | ||
PrincipalSecretsGenerator delegate = getDelegate(realmName, principalName); | ||
return delegate.produceSecrets(principalName, principalId); | ||
} | ||
|
||
@Override | ||
public boolean systemGeneratedSecrets(@NotNull String principalName) { | ||
PrincipalSecretsGenerator delegate = getDelegate(realmName, principalName); | ||
return delegate.systemGeneratedSecrets(principalName); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.polaris.core.persistence.secrets; | ||
|
||
import org.apache.polaris.core.entity.PolarisPrincipalSecrets; | ||
import org.jetbrains.annotations.NotNull; | ||
import org.jetbrains.annotations.Nullable; | ||
|
||
public class EnvVariablePrincipalSecretsGenerator extends PrincipalSecretsGenerator { | ||
|
||
public EnvVariablePrincipalSecretsGenerator(@Nullable String realmName) { | ||
super(realmName); | ||
} | ||
|
||
/** {@inheritDoc} */ | ||
@Override | ||
public PolarisPrincipalSecrets produceSecrets(@NotNull String principalName, long principalId) { | ||
String clientIdKey = clientIdEnvironmentVariable(realmName, principalName); | ||
String clientSecretKey = clientSecretEnvironmentVariable(realmName, principalName); | ||
|
||
String clientId = getEnvironmentVariable(clientIdKey); | ||
String clientSecret = getEnvironmentVariable(clientSecretKey); | ||
if (clientId == null || clientSecret == null) { | ||
return null; | ||
} else { | ||
return new PolarisPrincipalSecrets(principalId, clientId, clientSecret, null); | ||
} | ||
} | ||
|
||
/** {@inheritDoc} */ | ||
@Override | ||
public boolean systemGeneratedSecrets(@NotNull String principalName) { | ||
String clientIdKey = clientIdEnvironmentVariable(realmName, principalName); | ||
String clientSecretKey = clientSecretEnvironmentVariable(realmName, principalName); | ||
return getEnvironmentVariable(clientIdKey) != null | ||
&& getEnvironmentVariable(clientSecretKey) != null; | ||
} | ||
|
||
/** Load a single environment variable */ | ||
private static String getEnvironmentVariable(String key) { | ||
return System.getenv(key); | ||
} | ||
|
||
/** Build the key for the env variable used to store client ID */ | ||
private static String clientIdEnvironmentVariable(String realmName, String principalName) { | ||
return String.format("POLARIS_BOOTSTRAP_%s_%s_CLIENT_ID", realmName, principalName); | ||
} | ||
|
||
/** Build the key for the env variable used to store client secret */ | ||
private static String clientSecretEnvironmentVariable(String realmName, String principalName) { | ||
return String.format("POLARIS_BOOTSTRAP_%s_%s_CLIENT_SECRET", realmName, principalName); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,12 +16,11 @@ | |
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.polaris.core.persistence; | ||
package org.apache.polaris.core.persistence.secrets; | ||
|
||
import java.util.Locale; | ||
import java.util.function.Function; | ||
import org.apache.polaris.core.entity.PolarisPrincipalSecrets; | ||
import org.jetbrains.annotations.NotNull; | ||
import org.jetbrains.annotations.Nullable; | ||
|
||
/** | ||
* An interface for generating principal secrets. It enables detaching the secret generation logic | ||
|
@@ -38,48 +37,48 @@ | |
* <li>{@code POLARIS_BOOTSTRAP_<REALM-NAME>_<PRINCIPAL-NAME>_CLIENT_SECRET} | ||
* </ul> | ||
* | ||
* For example: {@code POLARIS_BOOTSTRAP_DEFAULT-REALM_ROOT_CLIENT_ID} and {@code | ||
* POLARIS_BOOTSTRAP_DEFAULT-REALM_ROOT_CLIENT_SECRET}. | ||
* For example: {@code POLARIS_BOOTSTRAP_default-realm_root_CLIENT_ID} and {@code | ||
* POLARIS_BOOTSTRAP_default-realm_root_CLIENT_SECRET}. | ||
*/ | ||
@FunctionalInterface | ||
public interface PrincipalSecretsGenerator { | ||
public abstract class PrincipalSecretsGenerator { | ||
|
||
/** | ||
* A secret generator that produces cryptographically random client ID and client secret values. | ||
*/ | ||
PrincipalSecretsGenerator RANDOM_SECRETS = (name, id) -> new PolarisPrincipalSecrets(id); | ||
protected final String realmName; | ||
|
||
public PrincipalSecretsGenerator() { | ||
this.realmName = null; | ||
} | ||
|
||
public PrincipalSecretsGenerator(@Nullable String realmName) { | ||
this.realmName = realmName; | ||
} | ||
|
||
/** | ||
* Produces a new {@link PolarisPrincipalSecrets} object for the given principal ID. The returned | ||
* secrets may or may not be random, depending on context. In bootstrapping contexts, the returned | ||
* secrets can be predefined. After bootstrapping, the returned secrets can be expected to be | ||
* cryptographically random. | ||
* secrets may or may not be random, depending on context. The returned secrets can be predefined. | ||
* | ||
* @param principalName the name of the related principal. This parameter is a hint for | ||
* pre-defined secrets lookup during bootstrapping it is not included in the returned data. | ||
* @param principalId the ID of the related principal. This ID is part of the returned data. | ||
* @return a new {@link PolarisPrincipalSecrets} instance for the specified principal. | ||
*/ | ||
PolarisPrincipalSecrets produceSecrets(@NotNull String principalName, long principalId); | ||
|
||
static PrincipalSecretsGenerator bootstrap(String realmName) { | ||
return bootstrap(realmName, System.getenv()::get); | ||
} | ||
public abstract PolarisPrincipalSecrets produceSecrets( | ||
@NotNull String principalName, long principalId); | ||
|
||
static PrincipalSecretsGenerator bootstrap(String realmName, Function<String, String> config) { | ||
return (principalName, principalId) -> { | ||
String propId = String.format("POLARIS_BOOTSTRAP_%s_%s_CLIENT_ID", realmName, principalName); | ||
String propSecret = | ||
String.format("POLARIS_BOOTSTRAP_%s_%s_CLIENT_SECRET", realmName, principalName); | ||
/** | ||
* @param principalName the name of the related principal. This parameter is a hint for | ||
* pre-defined secrets lookup during bootstrapping it is not included in the returned data. | ||
* @return true if the secrets generated by this {@link PrincipalSecretsGenerator} are | ||
* Polaris-generated as opposed to being provided by the user or another system. | ||
*/ | ||
public abstract boolean systemGeneratedSecrets(@NotNull String principalName); | ||
|
||
String clientId = config.apply(propId.toUpperCase(Locale.ROOT)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did we lose uppercasing in the new code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I actually think this is wrong isn't it? It seems like you can have both a This is assuming we now allow the use of env variables for non-root users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mixed case env. variable look odd, but technically we can support them. I do not mind ;) |
||
String secret = config.apply(propSecret.toUpperCase(Locale.ROOT)); | ||
// use config values at most once (do not interfere with secret rotation) | ||
if (clientId != null && secret != null) { | ||
return new PolarisPrincipalSecrets(principalId, clientId, secret, secret); | ||
} else { | ||
return RANDOM_SECRETS.produceSecrets(principalName, principalId); | ||
} | ||
}; | ||
/** | ||
* Build a PrincipalSecretsGenerator for bootstrapping | ||
* | ||
* @param realmName the name of the realm | ||
* @return A {@link PrincipalSecretsGenerator} that can generate secrets through `produceSecrets` | ||
*/ | ||
public static PrincipalSecretsGenerator bootstrap(String realmName) { | ||
return new DefaultPrincipalSecretsGenerator(realmName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. It's just a wrapper, so I was not sure what to call it. Since it's only used during bootstrap, let me change to |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic belongs to the secrets generator. The MetaStoreManager doesn't need to know anything about whether the secrets generated are provided by the user or if they've been generated randomly. So why would it be concerned with printing the credentials? The secrets generator knows if the secrets were provided explicitly or if they were randomly generated.
I think the
bootstrap
command should take aprint-credentials
config flag and the constructed secrets generator can react accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the
PrincipalSecretsGenerator
is doing exactly what the name suggests: generating secrets.Whatever is done with those secrets -- persisting them, using them, printing them -- is outside the purview of the generator itself.
You are right that the
MetaStoreManager
doesn't need to know anything about printing either (it doesn't in this PR) and clearly this should be outside the purview of the metastore itself.And so we landed on the factory. I would be happy to take this bootstrapping logic and excise it to somewhere more idiomatic if that is a concern. But right now the bootstrapping logic lives here (e.g. the
purge
check) and this seems like the most appropriate place that doesn't change the responsibility of either the metastore or generator classes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking again, is your objection specifically to the protected method
printCredentials
?That only exists to support the legacy behavior of the in-memory metastore always printing credentials, and if possible I would very much be in favor of removing that.
However it feels like pushing that logic down into an existing method (whether
secretsGenerator
,createMetaStoreSession
, or elsewhere) could be a bit hacky if it winds up somewhere it doesn't belong.