Mmuzaf commented on code in PR #2505: URL: https://github.com/apache/cassandra/pull/2505#discussion_r1272251148
########## src/java/org/apache/cassandra/security/AbstractCryptoProvider.java: ########## @@ -0,0 +1,171 @@ +/* + * 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.cassandra.security; + +import java.security.Provider; +import java.security.Security; +import java.util.Map; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.cassandra.exceptions.ConfigurationException; + +import static java.lang.String.format; + +public abstract class AbstractCryptoProvider +{ + protected static final Logger logger = LoggerFactory.getLogger(AbstractCryptoProvider.class); + public static final String FAIL_ON_MISSING_PROVIDER_KEY = "fail_on_missing_provider"; + + protected final boolean failOnMissingProvider; + + public AbstractCryptoProvider(Map<String, String> properties) + { + failOnMissingProvider = properties != null && Boolean.parseBoolean(properties.getOrDefault(FAIL_ON_MISSING_PROVIDER_KEY, "false")); + } + + /** + * Returns name of the provider, as returned from {@link Provider#getName()} + * + * @return name of the provider + */ + public abstract String getProviderName(); + + /** + * Returns the name of the class which installs specific provider of name {@link #getProviderName()}. + * + * @return name of class of provider + */ + public abstract String getProviderClassAsString(); + + /** + * Returns a runnable which installs this crypto provider. + * + * @return runnable which installs this provider + */ + protected abstract Runnable installator() throws Exception; + + /** + * Returns boolean telling if this provider was installed properly. + * + * @return {@code true} if provider was installed properly, {@code false} otherwise. + */ + protected abstract boolean isHealthyInstallation() throws Exception; + + /** + * The default installation runs {@link AbstractCryptoProvider#installator()} and after that + * {@link AbstractCryptoProvider#isHealthyInstallation()}. + * <p> + * If any step fails, it will not throw an exception unless the parameter + * {@link AbstractCryptoProvider#FAIL_ON_MISSING_PROVIDER_KEY} is {@code true}. + */ + public void install() throws Exception + { + if (InBuiltJREProvider.class.getName().equals(getProviderClassAsString())) + { + logger.debug("Installation of a crypto provider was skipped."); + return; + } + + String failureMessage = null; + Throwable t = null; + try + { + Class.forName(getProviderClassAsString()); Review Comment: I have seen a lot of cases using the following `FBUtilities#classForName` I think here is a good place for that as well. It also throws `ConfigurationException` which is good for us. ########## src/java/org/apache/cassandra/security/AbstractCryptoProvider.java: ########## @@ -0,0 +1,171 @@ +/* + * 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.cassandra.security; + +import java.security.Provider; +import java.security.Security; +import java.util.Map; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.cassandra.exceptions.ConfigurationException; + +import static java.lang.String.format; + +public abstract class AbstractCryptoProvider +{ + protected static final Logger logger = LoggerFactory.getLogger(AbstractCryptoProvider.class); + public static final String FAIL_ON_MISSING_PROVIDER_KEY = "fail_on_missing_provider"; Review Comment: I would also expose this property to the `CassandraRelevantProperties`, so that we can easily override it if necessary. ########## test/distributed/org/apache/cassandra/distributed/impl/Instance.java: ########## @@ -923,6 +923,8 @@ public Future<Void> shutdown(boolean graceful) error = parallelRun(error, executor, this::stopJmx); + DatabaseDescriptor.getCryptoProvider().uninstall(); Review Comment: The `uninstall' method throws a `SecurityException'. If an exception is thrown everything below is skipped. You handle this case in the `AbstractCryptoProvider`, but not for `Instance#shutdown(boolean)`. Is this correct? ########## conf/cassandra.yaml: ########## @@ -1375,6 +1375,21 @@ dynamic_snitch_reset_interval: 600000ms # until the pinned host was 20% worse than the fastest. dynamic_snitch_badness_threshold: 1.0 +# Configures Java crypto provider. By default, it will use DefaultCryptoProvider +# which will install Amazon Correto Crypto Provider. +# +# Amazon Correto Crypto Provider works currently for x86_64 and aarch_64 platforms. Review Comment: I think it is worth adding a link to the project with documentation pages. Wdyt? https://github.com/corretto/amazon-corretto-crypto-provider ########## .build/parent-pom-template.xml: ########## @@ -233,12 +233,52 @@ <id>zznate</id> <name>Nate McCall</name> </developer> + <developer> + <id>smiklosovic</id> + <name>Stefan Miklosovic</name> + </developer> </developers> <scm> <connection>scm:https://gitbox.apache.org/repos/asf/cassandra.git</connection> <developerConnection>scm:https://gitbox.apache.org/repos/asf/cassandra.git</developerConnection> <url>https://gitbox.apache.org/repos/asf?p=cassandra.git;a=tree</url> </scm> + + <profiles> + <profile> + <id>x86_64</id> + <activation> + <os> + <arch>!aarch64</arch> Review Comment: Should we also add `<family>unix</family>'? This will look more correct even if we never run on other operating systems. Is the condition correct? Should we place here `x86_64` to avoid matching other types of `x86`? ########## .build/parent-pom-template.xml: ########## @@ -233,12 +233,52 @@ <id>zznate</id> <name>Nate McCall</name> </developer> + <developer> + <id>smiklosovic</id> + <name>Stefan Miklosovic</name> + </developer> </developers> <scm> <connection>scm:https://gitbox.apache.org/repos/asf/cassandra.git</connection> <developerConnection>scm:https://gitbox.apache.org/repos/asf/cassandra.git</developerConnection> <url>https://gitbox.apache.org/repos/asf?p=cassandra.git;a=tree</url> </scm> + + <profiles> + <profile> + <id>x86_64</id> + <activation> + <os> + <arch>!aarch64</arch> + </os> + </activation> + <dependencies> Review Comment: This is the `cassandra-parent` pom, so it should probably be placed under the `<dependencyManagement>` section to avoid any problems for those projects that depend on this pom, but still don't want to see those dependencies in the classpath. The `<dependency>` probably should be used in the `cassandra-deps-template.xml` which defines the `cassandra-all` artifact. So, the question here is - is it safe putting this dependency in the classpath for all? ########## src/java/org/apache/cassandra/security/DefaultCryptoProvider.java: ########## @@ -0,0 +1,67 @@ +/* + * 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.cassandra.security; + +import java.util.Map; +import javax.crypto.Cipher; + +import com.amazon.corretto.crypto.provider.AmazonCorrettoCryptoProvider; + +/** + * Default crypto provider tries to install AmazonCorrettoCryptoProvider. + * <p> + * The implementation falls back to in-built crypto provider in JRE if the installation + * is not successful. + */ +public class DefaultCryptoProvider extends AbstractCryptoProvider +{ + public DefaultCryptoProvider(Map<String, String> args) + { + super(args); + } + + @Override + public String getProviderName() + { + return AmazonCorrettoCryptoProvider.PROVIDER_NAME; + } + + @Override + public String getProviderClassAsString() + { + return "com.amazon.corretto.crypto.provider.AmazonCorrettoCryptoProvider"; Review Comment: I think this should be `AmazonCorrettoCryptoProvider.PACKAGE_PREFIX + AmazonCorrettoCryptoProvider.PROVIDER_NAME` and placed as the class public constant. ########## src/java/org/apache/cassandra/security/InBuiltJREProvider.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.cassandra.security; + +import java.util.Map; + +/** + * Crypto provider which does nothing. Handy for situations when a user + * wants to completely bypass crypto provider installation. + */ +public final class InBuiltJREProvider extends AbstractCryptoProvider +{ + public InBuiltJREProvider(Map<String, String> properties) + { + super(properties); + } + + @Override + public String getProviderName() + { + return InBuiltJREProvider.class.getSimpleName(); + } + + @Override + public String getProviderClassAsString() + { + return InBuiltJREProvider.class.getName(); + } + + @Override + protected Runnable installator() throws Exception Review Comment: Neither implementation throws a checked exception, should we still keep the method signature as 'throws Exception`? ########## src/java/org/apache/cassandra/config/DatabaseDescriptor.java: ########## @@ -1500,6 +1534,15 @@ private static IFailureDetector createFailureDetector(String detectorClassName) return detector; } + public static AbstractCryptoProvider getCryptoProvider() + { + return cryptoProvider; + } + + public static void setCryptoProvider(AbstractCryptoProvider cryptoProvider) Review Comment: Do you think we need this method even if it is not used? ########## src/java/org/apache/cassandra/config/DatabaseDescriptor.java: ########## @@ -426,6 +429,8 @@ private static void applyAll() throws ConfigurationException //InetAddressAndPort cares that applySimpleConfig runs first applySSTableFormats(); + applyCryptoProvider(); Review Comment: Should it also be part of the tool initialisation and client initialisation? I'm referring to `toolInitialization` and `clientInitialization` methods. ########## src/java/org/apache/cassandra/security/InBuiltJREProvider.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.cassandra.security; + +import java.util.Map; + +/** + * Crypto provider which does nothing. Handy for situations when a user + * wants to completely bypass crypto provider installation. + */ +public final class InBuiltJREProvider extends AbstractCryptoProvider Review Comment: I would call it `NoOpCryptoProvider` instead, but it doesn't really matter. ########## src/java/org/apache/cassandra/config/DatabaseDescriptor.java: ########## @@ -1217,6 +1222,35 @@ public static void applySslContext() } } + public static void applyCryptoProvider() + { + if (conf.crypto_provider == null) + conf.crypto_provider = new ParameterizedClass(DefaultCryptoProvider.class.getName(), null); + + if (conf.crypto_provider.parameters == null) + conf.crypto_provider.parameters = new HashMap<>(); + + if (conf.crypto_provider.class_name == null) + throw new ConfigurationException("Failed to initialize crypto provider, class_name cannot be null"); + + conf.crypto_provider.parameters.putIfAbsent(AbstractCryptoProvider.FAIL_ON_MISSING_PROVIDER_KEY, "false"); + + try + { + Class<?> cryptoProviderClass = Class.forName(conf.crypto_provider.class_name); Review Comment: I think we can use `FBUtilities#classForName` here. OR alternatively use `FBUtilities#construct(java.lang.String, java.lang.String)` and pass the `ParameterizedClass` instance from the `config` directly to the `install` method. ########## src/java/org/apache/cassandra/config/DatabaseDescriptor.java: ########## @@ -1217,6 +1222,35 @@ public static void applySslContext() } } + public static void applyCryptoProvider() + { + if (conf.crypto_provider == null) + conf.crypto_provider = new ParameterizedClass(DefaultCryptoProvider.class.getName(), null); + + if (conf.crypto_provider.parameters == null) + conf.crypto_provider.parameters = new HashMap<>(); + + if (conf.crypto_provider.class_name == null) + throw new ConfigurationException("Failed to initialize crypto provider, class_name cannot be null"); + + conf.crypto_provider.parameters.putIfAbsent(AbstractCryptoProvider.FAIL_ON_MISSING_PROVIDER_KEY, "false"); + + try + { + Class<?> cryptoProviderClass = Class.forName(conf.crypto_provider.class_name); + cryptoProvider = (AbstractCryptoProvider)cryptoProviderClass.getConstructor(Map.class).newInstance(conf.crypto_provider.parameters); + cryptoProvider.install(); Review Comment: In my humble opinion, it is good for the install method to only throw runtime exceptions, so we don't need to handle them here and they are passed directly to `CassandraDaemon#activate()` for handling. In fact, I have checked the `install()` method and it throws only runtime exceptions. ########## src/java/org/apache/cassandra/security/DefaultCryptoProvider.java: ########## @@ -0,0 +1,67 @@ +/* + * 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.cassandra.security; + +import java.util.Map; +import javax.crypto.Cipher; + +import com.amazon.corretto.crypto.provider.AmazonCorrettoCryptoProvider; + +/** + * Default crypto provider tries to install AmazonCorrettoCryptoProvider. + * <p> + * The implementation falls back to in-built crypto provider in JRE if the installation + * is not successful. + */ +public class DefaultCryptoProvider extends AbstractCryptoProvider Review Comment: In my opinion, a better name would be `AmazonCorrettoCryptoProvider` or something like that, so that a user reading the configuration file can see exactly what the default provider is without having to go into the depths of the code. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
