You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/05/13 04:39:36 UTC
[GitHub] [cassandra] yifan-c commented on a diff in pull request #1535: CASSANDRA-17513 Adding new property to server encryption options for creating outbound keystore for internode mTLS
yifan-c commented on code in PR #1535:
URL: https://github.com/apache/cassandra/pull/1535#discussion_r871975131
##########
src/java/org/apache/cassandra/config/EncryptionOptions.java:
##########
@@ -121,7 +116,9 @@ public String description()
REQUIRE_CLIENT_AUTH("require_client_auth"),
REQUIRE_ENDPOINT_VERIFICATION("require_endpoint_verification"),
ENABLED("enabled"),
- OPTIONAL("optional");
+ OPTIONAL("optional"),
+ OUTBOUND_KEYSTORE("outbound_keystore"),
+ OUTBOUND_KEYSTORE_PASSWORD("outbound_keystore_password");
Review Comment:
Very very weak nit: probably move those 2 items up after `KEYSTORE_PASSWORD`. The `ConfigKey` enums has no order requirement.
##########
conf/cassandra.yaml:
##########
@@ -1259,6 +1259,11 @@ server_encryption_options:
# Set to a valid keystore if internode_encryption is dc, rack or all
keystore: conf/.keystore
keystore_password: cassandra
+ # During internode mTLS authentication, Inbound connections uses keystore, keystore_password containing server certificate
+ # for creating SSLContext and outbound connections use outbound_keystore & outbound_keystore_password with client certificates
+ # for creating SSLContext. By default, outbound keystore is set to keystore.
Review Comment:
Updated the format (adjusted with the line width) and rephrased some sentences a bit. Please take a look.
```suggestion
# During internode mTLS authentication, inbound connections (acting as servers) use keystore, keystore_password
# containing server certificate to create SSLContext and
# outbound connections (acting as clients) use outbound_keystore & outbound_keystore_password with client certificates
# to create SSLContext. By default, outbound_keystore is the same as keystore indicating mTLS is not enabled.
```
##########
src/java/org/apache/cassandra/config/EncryptionOptions.java:
##########
@@ -296,7 +297,7 @@ private void initializeSslContextFactory()
}
}
- private void putSslContextFactoryParameter(Map<String,Object> existingParameters, ConfigKey configKey,
+ protected void putSslContextFactoryParameter(Map<String,Object> existingParameters, ConfigKey configKey,
Review Comment:
nit: the helper method can be static.
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -102,15 +116,20 @@ private boolean hasTruststore()
public synchronized void initHotReloading()
{
boolean hasKeystore = hasKeystore();
+ boolean hasOutboundKeystore = hasKeystore();
Review Comment:
I think you want to call `hasOutboundKeystore()` here. This is just an example that code duplication makes it error prone.
##########
src/java/org/apache/cassandra/security/PEMBasedSslContextFactory.java:
##########
@@ -286,7 +340,7 @@ private String readPEMFile(String file) throws IOException
* Builds KeyStore object given the {@link #DEFAULT_TARGET_STORETYPE} out of the PEM formatted private key material.
* It uses {@code cassandra-ssl-keystore} as the alias for the created key-entry.
*/
- private KeyStore buildKeyStore() throws GeneralSecurityException, IOException
+ private KeyStore buildKeyStore(final String pemEncodedKey, final String keyPassword) throws GeneralSecurityException, IOException
Review Comment:
nit: this can be `static`.
##########
src/java/org/apache/cassandra/config/EncryptionOptions.java:
##########
@@ -296,7 +297,7 @@ private void initializeSslContextFactory()
}
}
- private void putSslContextFactoryParameter(Map<String,Object> existingParameters, ConfigKey configKey,
+ protected void putSslContextFactoryParameter(Map<String,Object> existingParameters, ConfigKey configKey,
Object value)
Review Comment:
wrong alignment on new line.
##########
src/java/org/apache/cassandra/security/AbstractSslContextFactory.java:
##########
@@ -263,4 +264,11 @@ protected SslProvider getSslProvider()
abstract protected KeyManagerFactory buildKeyManagerFactory() throws SSLException;
abstract protected TrustManagerFactory buildTrustManagerFactory() throws SSLException;
+
+ /**
+ * Internode mTLS needs separate keystore for outbound connections.
+ * @return
+ * @throws SSLException
+ */
Review Comment:
Consider the following.
```suggestion
/**
* Create a {@code KeyManagerFactory} for outbound connections.
* It provides a seperate keystore for internode mTLS connections.
* @return {@code KeyManagerFactory}
* @throws SSLException
*/
```
##########
src/java/org/apache/cassandra/config/EncryptionOptions.java:
##########
@@ -421,8 +422,7 @@ else if (getEnabled())
}
}
- public EncryptionOptions withSslContextFactory(ParameterizedClass sslContextFactoryClass)
- {
+ public EncryptionOptions withSslContextFactory(ParameterizedClass sslContextFactoryClass) {
Review Comment:
move `{` to new line
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -53,6 +54,8 @@ abstract public class FileBasedSslContextFactory extends AbstractSslContextFacto
@VisibleForTesting
protected volatile boolean checkedExpiry = false;
Review Comment:
At the configuration level, I think it is good to not alter the existing names, since they are user-facing.
But at the implementation, what do you think about renaming the variables to reflect that they are used for inbound? e.g. `checkedExpiry` --> `checkedExpiryInboundKeystore`, so the code is more consistent and easier for maintainers to understand. This is suggestion 1.
We can even take a step further to create a data class to avoid field and logic duplications. This is suggestion 2. For example,
```java
static class KeyStoreContext
{
String file; // keystore
String password; // keystore_password
volatile boolean checkExpiry;
public KeyStoreContext(String file, String password, boolean checkExpiry)
{
this.file = file;
this.password = password;
this.checkExpiry = checkExpiry;
}
// add appropriate method. e.g. a hasKeystore equivelant exist()
}
```
Whatever suggestion you take, please make sure the other files are also changed.
##########
src/java/org/apache/cassandra/security/PEMBasedSslContextFactory.java:
##########
@@ -120,6 +123,23 @@ else if (!StringUtils.isEmpty(keystore_password) && !keyPassword.equals(keystore
"okay. Ideally you should only specify one of them.");
}
+ pemEncodedOutboundKey = StringUtils.defaultString(getString(ConfigKey.OUTBOUND_KEY.getKeyName()), pemEncodedKey);
+ outboundKeyPassword = StringUtils.defaultString(getString(ConfigKey.OUTBOUND_KEY_PASSWORD.getKeyName()), keyPassword);
+ if (StringUtils.isEmpty(outboundKeyPassword))
+ {
+ outboundKeyPassword = outbound_keystore_password;
+ }
+ else if (!StringUtils.isEmpty(outboundKeystore) && !outboundKeyPassword.equals(outbound_keystore_password))
+ {
+ throw new IllegalArgumentException("'outbound_keystore_password' and 'outbound_key_password' both configurations are given and the " +
+ "values do not match");
+ }
+ else
+ {
+ logger.warn("'keystore_password' and 'key_password' both are configured but since the values match it's " +
Review Comment:
should the warning about `outbound_keystore_password` and `outbound_key_password` instead? The `if-elseif-else` logic basically duplicates the above. It could use a refactor.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org