Mmuzaf commented on code in PR #2269:
URL: https://github.com/apache/cassandra/pull/2269#discussion_r1269805282
##########
test/unit/org/apache/cassandra/config/EncryptionOptionsEqualityTest.java:
##########
@@ -139,4 +156,47 @@ public void
testDifferentCustomSslContextFactoryParameters() {
assertNotEquals(encryptionOptions1, encryptionOptions2);
assertNotEquals(encryptionOptions1.hashCode(),
encryptionOptions2.hashCode());
}
+
+ @Test
+ public void testServerEncryptionOptions() {
Review Comment:
nl for the left curly bracket.
##########
test/unit/org/apache/cassandra/config/EncryptionOptionsEqualityTest.java:
##########
@@ -139,4 +156,47 @@ public void
testDifferentCustomSslContextFactoryParameters() {
assertNotEquals(encryptionOptions1, encryptionOptions2);
assertNotEquals(encryptionOptions1.hashCode(),
encryptionOptions2.hashCode());
}
+
+ @Test
+ public void testServerEncryptionOptions() {
+ EncryptionOptions.ServerEncryptionOptions encryptionOptions1 =
createServerEncryptionOptions();
+ EncryptionOptions.ServerEncryptionOptions encryptionOptions2
=createServerEncryptionOptions();
+
+ assertEquals(encryptionOptions1, encryptionOptions2);
+ assertEquals(encryptionOptions1.hashCode(),
encryptionOptions2.hashCode());
+ }
+
+ @Test
+ public void testServerEncryptionOptionsMismatchForOutboundKeystore() {
Review Comment:
nl for the left curly bracket.
##########
src/java/org/apache/cassandra/config/EncryptionOptions.java:
##########
@@ -718,6 +718,43 @@ public boolean isExplicitlyOptional()
return optional != null && optional;
}
+ /**
+ * The method is being mainly used to cache SslContexts therefore, we
only consider
+ * fields that would make a difference when the TrustStore or KeyStore
files are updated
+ */
+ @Override
+ public boolean equals(Object o)
+ {
+ if (o == this)
+ return true;
+ if (o == null || getClass() != o.getClass())
+ return false;
+ if (!super.equals(o)) {
Review Comment:
I would remove the brackets at all here :-)
##########
src/java/org/apache/cassandra/config/EncryptionOptions.java:
##########
@@ -627,8 +627,8 @@ public ServerEncryptionOptions(ParameterizedClass
sslContextFactoryClass, String
InternodeEncryption
internode_encryption, boolean legacy_ssl_storage_port_enabled)
{
super(sslContextFactoryClass, keystore, keystore_password,
truststore, truststore_password, cipher_suites,
- protocol, accepted_protocols, algorithm, store_type,
require_client_auth, require_endpoint_verification,
- null, optional);
+ protocol, accepted_protocols, algorithm, store_type,
require_client_auth, require_endpoint_verification,
Review Comment:
I would exclude this from the PR as the formatting has nothing to do with
the patch itself. Just not to avoid drawing attention to it in simple patches.
##########
src/java/org/apache/cassandra/config/EncryptionOptions.java:
##########
@@ -672,10 +672,10 @@ private ServerEncryptionOptions applyConfigInternal()
if (require_client_auth && (internode_encryption ==
InternodeEncryption.rack || internode_encryption == InternodeEncryption.dc))
{
logger.warn("Setting require_client_auth is incompatible with
'rack' and 'dc' internode_encryption values."
- + " It is possible for an internode connection to
pretend to be in the same rack/dc by spoofing"
- + " its broadcast address in the handshake and
bypass authentication. To ensure that mutual TLS"
- + " authentication is not bypassed, please set
internode_encryption to 'all'. Continuing with"
- + " insecure configuration.");
+ + " It is possible for an internode connection to
pretend to be in the same rack/dc by spoofing"
Review Comment:
The same as above - as the formatting is not related to the change itself it
is worth excluding it from the PR to avoid drawing attention to these lines.
##########
test/unit/org/apache/cassandra/config/EncryptionOptionsEqualityTest.java:
##########
@@ -139,4 +156,47 @@ public void
testDifferentCustomSslContextFactoryParameters() {
assertNotEquals(encryptionOptions1, encryptionOptions2);
assertNotEquals(encryptionOptions1.hashCode(),
encryptionOptions2.hashCode());
}
+
+ @Test
+ public void testServerEncryptionOptions() {
+ EncryptionOptions.ServerEncryptionOptions encryptionOptions1 =
createServerEncryptionOptions();
+ EncryptionOptions.ServerEncryptionOptions encryptionOptions2
=createServerEncryptionOptions();
Review Comment:
```suggestion
EncryptionOptions.ServerEncryptionOptions encryptionOptions2 =
createServerEncryptionOptions();
```
##########
src/java/org/apache/cassandra/config/EncryptionOptions.java:
##########
@@ -718,6 +718,43 @@ public boolean isExplicitlyOptional()
return optional != null && optional;
}
+ /**
+ * The method is being mainly used to cache SslContexts therefore, we
only consider
+ * fields that would make a difference when the TrustStore or KeyStore
files are updated
+ */
+ @Override
+ public boolean equals(Object o)
+ {
+ if (o == this)
+ return true;
+ if (o == null || getClass() != o.getClass())
+ return false;
+ if (!super.equals(o)) {
+ return false;
+ }
+
+ ServerEncryptionOptions opt = (ServerEncryptionOptions)o;
+ return internode_encryption == opt.internode_encryption &&
+ legacy_ssl_storage_port_enabled ==
opt.legacy_ssl_storage_port_enabled &&
+ Objects.equals(outbound_keystore, opt.outbound_keystore) &&
+ Objects.equals(outbound_keystore_password,
opt.outbound_keystore_password);
+ }
+
+ /**
+ * The method is being mainly used to cache SslContexts therefore, we
only consider
+ * fields that would make a difference when the TrustStore or KeyStore
files are updated
+ */
+ @Override
+ public int hashCode()
+ {
+ int result = 0;
+ result += 31 * super.hashCode();
+ result += 31 * (internode_encryption == null ? 0 :
internode_encryption.hashCode());
+ result += 31 * Boolean.hashCode(legacy_ssl_storage_port_enabled);
+ result += 31 * (outbound_keystore == null ? 0 :
outbound_keystore.hashCode());
+ result += 31 * (outbound_keystore_password == null ? 0 :
outbound_keystore_password.hashCode());
+ return result;
+ }
Review Comment:
newline between methods.
##########
test/unit/org/apache/cassandra/config/EncryptionOptionsEqualityTest.java:
##########
@@ -139,4 +156,47 @@ public void
testDifferentCustomSslContextFactoryParameters() {
assertNotEquals(encryptionOptions1, encryptionOptions2);
assertNotEquals(encryptionOptions1.hashCode(),
encryptionOptions2.hashCode());
}
+
+ @Test
+ public void testServerEncryptionOptions() {
+ EncryptionOptions.ServerEncryptionOptions encryptionOptions1 =
createServerEncryptionOptions();
+ EncryptionOptions.ServerEncryptionOptions encryptionOptions2
=createServerEncryptionOptions();
+
+ assertEquals(encryptionOptions1, encryptionOptions2);
+ assertEquals(encryptionOptions1.hashCode(),
encryptionOptions2.hashCode());
+ }
+
+ @Test
+ public void testServerEncryptionOptionsMismatchForOutboundKeystore() {
+ EncryptionOptions.ServerEncryptionOptions encryptionOptions1 =
createServerEncryptionOptions();
+ EncryptionOptions.ServerEncryptionOptions encryptionOptions2 =
createServerEncryptionOptions();
+
+ encryptionOptions1 = encryptionOptions1
+
.withOutboundKeystore("test/conf/cassandra_outbound1.keystore")
+ .withOutboundKeystorePassword("cassandra1");
+
+ encryptionOptions2 = encryptionOptions2
+
.withOutboundKeystore("test/conf/cassandra_outbound2.keystore")
+ .withOutboundKeystorePassword("cassandra2");
+
+ assertNotEquals(encryptionOptions1, encryptionOptions2);
+ assertNotEquals(encryptionOptions1.hashCode(),
encryptionOptions2.hashCode());
+ }
+
+ @Test
+ public void testServerEncryptionOptionsMismatchForInboundKeystore() {
Review Comment:
nl for the left curly bracket.
--
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]
