You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "maulin-vasavada (via GitHub)" <gi...@apache.org> on 2023/04/11 00:11:16 UTC
[GitHub] [cassandra] maulin-vasavada opened a new pull request, #2269: CASSANDRA-18428: Adding equals/hashCode override for the ServerEncryptionOptions
maulin-vasavada opened a new pull request, #2269:
URL: https://github.com/apache/cassandra/pull/2269
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/browse/CASSANDRA-18428)
--
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
[GitHub] [cassandra] maulin-vasavada commented on pull request #2269: CASSANDRA-18428: Adding equals/hashCode override for the ServerEncryptionOptions
Posted by "maulin-vasavada (via GitHub)" <gi...@apache.org>.
maulin-vasavada commented on PR #2269:
URL: https://github.com/apache/cassandra/pull/2269#issuecomment-1646350757
@Mmuzaf I have opened a new [PR#2507](https://github.com/apache/cassandra/pull/2507) hence will close this. Can you please review that new PR? Thanks
--
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
[GitHub] [cassandra] maulin-vasavada commented on pull request #2269: CASSANDRA-18428: Adding equals/hashCode override for the ServerEncryptionOptions
Posted by "maulin-vasavada (via GitHub)" <gi...@apache.org>.
maulin-vasavada commented on PR #2269:
URL: https://github.com/apache/cassandra/pull/2269#issuecomment-1644567662
Thanks @Mmuzaf for the review. I'll take a look at those soon.
--
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
[GitHub] [cassandra] Mmuzaf commented on a diff in pull request #2269: CASSANDRA-18428: Adding equals/hashCode override for the ServerEncryptionOptions
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #2269:
URL: https://github.com/apache/cassandra/pull/2269#discussion_r1269817515
##########
test/unit/org/apache/cassandra/config/EncryptionOptionsEqualityTest.java:
##########
@@ -35,6 +35,23 @@
*/
public class EncryptionOptionsEqualityTest
{
+ private EncryptionOptions.ServerEncryptionOptions createServerEncryptionOptions()
+ {
+ EncryptionOptions.ServerEncryptionOptions encryptionOptions =
Review Comment:
You can inline `encryptionOptions` for simplicity.
--
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
[GitHub] [cassandra] maulin-vasavada closed pull request #2269: CASSANDRA-18428: Adding equals/hashCode override for the ServerEncryptionOptions
Posted by "maulin-vasavada (via GitHub)" <gi...@apache.org>.
maulin-vasavada closed pull request #2269: CASSANDRA-18428: Adding equals/hashCode override for the ServerEncryptionOptions
URL: https://github.com/apache/cassandra/pull/2269
--
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
[GitHub] [cassandra] Mmuzaf commented on pull request #2269: CASSANDRA-18428: Adding equals/hashCode override for the ServerEncryptionOptions
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on PR #2269:
URL: https://github.com/apache/cassandra/pull/2269#issuecomment-1644385547
The code-style is here, just for reference:
https://cassandra.apache.org/_/development/code_style.html
--
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
[GitHub] [cassandra] Mmuzaf commented on a diff in pull request #2269: CASSANDRA-18428: Adding equals/hashCode override for the ServerEncryptionOptions
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
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: 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