Copilot commented on code in PR #4499:
URL: https://github.com/apache/cassandra/pull/4499#discussion_r2573224806
##########
tools/stress/src/org/apache/cassandra/stress/settings/SettingsTransport.java:
##########
@@ -89,7 +89,7 @@ static class TOptions extends GroupedOptions implements
Serializable
final OptionSimple protocol = new OptionSimple("ssl-protocol=", ".*",
"TLS", "SSL: connection protocol to use", false);
final OptionSimple alg = new OptionSimple("ssl-alg=", ".*", null,
"SSL: algorithm", false);
final OptionSimple ciphers = new OptionSimple("ssl-ciphers=", ".*",
-
"TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA",
+
"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
Review Comment:
The cipher suite order should prioritize the 128-bit variant before the
256-bit variant for better performance. According to the Cassandra
configuration files (cassandra.yaml), the recommended order is
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 first, then
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384. Consider reordering to:
\"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384\"
```suggestion
"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384",
```
##########
tools/stress/src/org/apache/cassandra/stress/settings/SettingsTransport.java:
##########
@@ -89,7 +89,7 @@ static class TOptions extends GroupedOptions implements
Serializable
final OptionSimple protocol = new OptionSimple("ssl-protocol=", ".*",
"TLS", "SSL: connection protocol to use", false);
final OptionSimple alg = new OptionSimple("ssl-alg=", ".*", null,
"SSL: algorithm", false);
final OptionSimple ciphers = new OptionSimple("ssl-ciphers=", ".*",
-
"TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA",
+
"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
Review Comment:
The change in default cipher suites lacks test coverage. Consider adding a
test to verify that the new default cipher suites work correctly with TLS 1.3
servers. The test directory (tools/stress/test/unit) contains other settings
tests like SettingsCredentialsTest.java, but there's no test coverage for
SettingsTransport cipher configuration.
--
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]
