Copilot commented on code in PR #4499:
URL: https://github.com/apache/cassandra/pull/4499#discussion_r2578124146
##########
tools/stress/src/org/apache/cassandra/stress/settings/SettingsTransport.java:
##########
@@ -88,8 +88,9 @@ static class TOptions extends GroupedOptions implements
Serializable
TRANSPORT_KEYSTORE_PASSWORD_PROPERTY_KEY), false);
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);
+ // Default is to auto-negotiate
final OptionSimple ciphers = new OptionSimple("ssl-ciphers=", ".*",
-
"TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA",
+ "",
Review Comment:
Changing the default for ssl-ciphers to an empty string does not
"auto-negotiate" with the current implementation. Because the value is split
and passed directly to the cipher suite setter, an empty string yields an
invalid suite entry and will break TLS configuration. Recommend keeping the
default null/unspecified and only applying `withCipherSuites(...)` when the
option is explicitly set, or update the builder to treat empty as "use provider
defaults".
```suggestion
null,
```
##########
tools/stress/src/org/apache/cassandra/stress/settings/SettingsTransport.java:
##########
@@ -88,8 +88,9 @@ static class TOptions extends GroupedOptions implements
Serializable
TRANSPORT_KEYSTORE_PASSWORD_PROPERTY_KEY), false);
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);
+ // Default is to auto-negotiate
final OptionSimple ciphers = new OptionSimple("ssl-ciphers=", ".*",
-
"TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA",
+ "",
"SSL: comma delimited
list of encryption suites to use", false);
Review Comment:
The PR description states new defaults should be
`TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256`,
but the code sets the default to an empty string. This is inconsistent and will
not achieve the stated goal of supporting TLS 1.3 by default. Recommend setting
these modern suites as the default value here, or clarifying in the help text
that no default suites are applied and users must specify them.
```suggestion
"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
"SSL: comma delimited
list of encryption suites to use (default:
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256)",
false);
```
--
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]
