CASSANDRA-21546: Support pluggable default role initialization - #4990
CASSANDRA-21546: Support pluggable default role initialization#4990aparna0522 wants to merge 9 commits into
Conversation
8759820 to
96a5f44
Compare
aparna0522
changed the title
smiklosovic
changed the title
…ssword-like entry
96a5f44 to
16cc98f
Compare
| # password: cassandra | ||
| # # password_hash: "$2a$04$wsvzFamDJPDrTwMjgfcgpO.mKc.CMEuHBFZSjhGz2Ts6.v8PUO2rC" | ||
| # | ||
| # default_role_initializer: |
There was a problem hiding this comment.
This will add two sections into doc/modules/cassandra/pages/managing/configuration/cass_yaml_file.adoc (and wrongly formatted) when docs are generated, you should make it one configuration block instead.
| } | ||
|
|
||
| private static <T> T authInstantiate(ParameterizedClass authCls, Class<T> expectedType, Class<? extends T> defaultCls) { | ||
| public static <T> T authInstantiate(ParameterizedClass authCls, Class<T> expectedType, Class<? extends T> defaultCls) |
There was a problem hiding this comment.
this can go back to be private
| return ParameterizedClass.newInstance(authCls, List.of("", authPackage), expectedType); | ||
| } | ||
|
|
||
| if (defaultCls == null) |
There was a problem hiding this comment.
not sure we need this anymore? This patch does not call this (changed) method.
There was a problem hiding this comment.
Actually this can stay, it is a bug if we dont check for nullity because it would NPE next on defaultCls.newInstance().
|
|
||
| IRoleManager roleManager = authInstantiate(conf.role_manager, IRoleManager.class, CassandraRoleManager.class); | ||
|
|
||
| if (authenticator instanceof PasswordAuthenticator && !(roleManager instanceof CassandraRoleManager)) |
There was a problem hiding this comment.
should not we add something similar for IDefaultRoleInitializer? Something like ...
if ((defaultRoleInitializer instanceof PasswordDefaultRoleInitializer
|| defaultRoleInitializer instanceof MutualTlsDefaultRoleInitializer))
{
if (!(roleManager instanceof CassandraRoleManager))
throw new ConfigurationException(...)
}
This is what I was trying to avoid by having it embedded, now we are basically in a situation when we somehow need to be sure that it integrates together.
Maybe having additional method on IDefaultRoleInitializer returning the supported IRoleManager's would be OK? That means an implementator of IDefaultRoleInitializer would need to declare what kind of IRoleManagers it can service.
There was a problem hiding this comment.
We are wiring PasswordDefaultRoleInitializer with CassandraRoleManager as we call consistencyForRoleWrite from there. So if we use PasswordDefaultRoleInitializer with something else from CassandraRoleManager the results are unpredictable, basically. So I would make it necessary for the implementator to explicitly enumerate what role managers it is supposed to work with.
| throw new OverloadedException(failure); | ||
| } | ||
|
|
||
| private static String hashpw(String password) |
There was a problem hiding this comment.
It would be better if these helper methods were moved to AuthUtils if they are to be reused in various classes (hashpw, escape).
| * | ||
| * @throws ConfigurationException when there is a configuration error. | ||
| */ | ||
| default void validateConfiguration() throws ConfigurationException |
There was a problem hiding this comment.
I would not make this method default. IRoleManager has validateConfiguration method which is not default either. I think it is a good practice to force people to implement their validations explicitly and keep it empty on purpose if nothing to validate.
| * gives the role the default password so PasswordAuthenticator can be used to log in (if | ||
| * configured) | ||
| */ | ||
| default void setupDefaultRole() |
There was a problem hiding this comment.
I have a hard time to distinguish between "createDefaultRole" and "setupDefaultRole". If this method was not implemented already I would not know what to put into it. Maybe coverting IDefaultRoleInitializer to an abstract class (or making intermediate abstract class to implement this) and making this method public void would be better? Do we ever have a need to actually override this method?
| default void setupDefaultRole() | ||
| { | ||
| if (ClusterMetadata.current().tokenMap.tokens().isEmpty()) | ||
| throw new IllegalStateException("CassandraRoleManager skipped default role setup: no known tokens in ring"); |
There was a problem hiding this comment.
I think the log message is wrong, change the log message so it does not reference CassandraRoleManager:
throw new IllegalStateException(IDefaultRoleInitializer.this.getClass().getName() + " skipped default role setup: no known tokens in ring");
| } | ||
| catch (RequestExecutionException e) | ||
| { | ||
| logger.warn("CassandraRoleManager skipped default role setup: some nodes were not ready"); |
|
|
||
| public PasswordDefaultRoleInitializer(Map<String, String> parameters) | ||
| { | ||
| for (String param : parameters.keySet()) |
There was a problem hiding this comment.
this is going to happen all over again in other implementations too, we might put this logic to interface / abstract class and just call it from here (static method) with SUPPORTED_PARAMS and parameters as arguments or similar.
| * {@link MutualTlsDefaultRoleInitializer} which gives no password and instead | ||
| * maps a client certificate identity to itself. | ||
| */ | ||
| public interface IDefaultRoleInitializer |
There was a problem hiding this comment.
after my latest refactor this interface is "implementation free"
3 participants
Description
Cassandra bootstraps every new cluster with a hardcoded cassandra superuser whose password is also cassandra, so credential operators must remember to rotate.
This PR adds
IDefaultRoleInitializer, a pluggable strategy for bootstrapping the initial role, configured via a newdefault_role_initializeroption in cassandra.yaml:PasswordDefaultRoleInitializer(default): same behavior as today, role/password now configurable.MutualTlsDefaultRoleInitializer: creates the superuser with no password, mapping a client cert identity instead. Rejected at startup if the authenticator doesn't support MTLS.patch by Aparna Naik; reviewed by for CASSANDRA-21546