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 2020/11/30 21:42:03 UTC
[GitHub] [cassandra] jonmeredith commented on a change in pull request #770: CASSANDRA-13325 configure which TLS protocols should be accepted
jonmeredith commented on a change in pull request #770:
URL: https://github.com/apache/cassandra/pull/770#discussion_r532921693
##########
File path: src/java/org/apache/cassandra/config/EncryptionOptions.java
##########
@@ -209,6 +214,89 @@ public void setOptional(boolean optional) {
this.optional = optional;
}
+ /**
+ * Sets accepted TLS protocol for this channel. Note that this should only be called by
+ * the configuration parser or tests. It is public only for that purpose, mutating protocol state
+ * is probably a bad idea.
+ * @param protocol value to set
+ */
+ @VisibleForTesting
+ public void setProtocol(String protocol) {
+ this.protocol = protocol;
+ }
+
+ /**
+ * Sets accepted TLS protocols for this channel. Note that this should only be called by
+ * the configuration parser or tests. It is public only for that purpose, mutating protocol state
+ * is probably a bad idea. The function casing is required for snakeyaml to find this setter for the protected field.
+ * @param accepted_protocols value to set
+ */
+ public void setaccepted_protocols(List<String> accepted_protocols) {
+ this.accepted_protocols = ImmutableList.copyOf(accepted_protocols);
+ }
+
+ /* This list is substituted in configurations that have explicitly specified the original "TLS" default,
+ * it is not a 'default' list or 'support protocol versions' list. It is just an attempt to preserve the
+ * original intent for the user configuration
+ */
+ private final List<String> tlsProtocolSubstitution = ImmutableList.of("TLSv1.3", "TLSv1.2", "TLSv1.1", "TLSv1");
+
+ /**
+ * Combine the pre-4.0 protocol field with the accepted_protocols list, substituting a list of
+ * explicit protocols for the previous catchall default of "TLS"
+ * @return array of protocol names suitable for passing to SslContextBuilder.protocols, or null if the default
+ */
+ public List<String> acceptedProtocols()
+ {
+ if (accepted_protocols == null)
+ {
+ if (protocol == null)
+ {
+ return null;
+ }
+ // TLS is accepted by SSLContext.getInstance as a shorthand for give me an engine that
+ // can speak some of the TLS protocols. It is not supported by SSLEngine.setAcceptedProtocols
+ // so substitute if the user hasn't provided an accepted protocol configuration
+ else if (protocol.equalsIgnoreCase("TLS"))
+ {
+ return tlsProtocolSubstitution;
+ }
+ else // the user was trying to limit to a single specific protocol, so try that
+ {
+ return ImmutableList.of(protocol);
+ }
+ }
+
+ if (protocol != null && !protocol.equalsIgnoreCase("TLS") &&
+ accepted_protocols.stream().noneMatch(ap -> ap.equalsIgnoreCase(protocol)))
+ {
+ // If the user provided a non-generic default protocol, append it to accepted_protocols - they wanted
+ // it after all.
+ return ImmutableList.<String>builder().addAll(accepted_protocols).add(protocol).build();
+ }
+ else
+ {
+ return accepted_protocols;
+ }
+ }
+
+ public String[] acceptedProtocolsArray()
+ {
+ List<String> ap = acceptedProtocols();
+ if (ap == null)
+ return new String[]{};
Review comment:
ack
##########
File path: src/java/org/apache/cassandra/config/EncryptionOptions.java
##########
@@ -209,6 +214,89 @@ public void setOptional(boolean optional) {
this.optional = optional;
}
+ /**
+ * Sets accepted TLS protocol for this channel. Note that this should only be called by
+ * the configuration parser or tests. It is public only for that purpose, mutating protocol state
+ * is probably a bad idea.
+ * @param protocol value to set
+ */
+ @VisibleForTesting
+ public void setProtocol(String protocol) {
+ this.protocol = protocol;
+ }
+
+ /**
+ * Sets accepted TLS protocols for this channel. Note that this should only be called by
+ * the configuration parser or tests. It is public only for that purpose, mutating protocol state
+ * is probably a bad idea. The function casing is required for snakeyaml to find this setter for the protected field.
+ * @param accepted_protocols value to set
+ */
+ public void setaccepted_protocols(List<String> accepted_protocols) {
+ this.accepted_protocols = ImmutableList.copyOf(accepted_protocols);
+ }
+
+ /* This list is substituted in configurations that have explicitly specified the original "TLS" default,
+ * it is not a 'default' list or 'support protocol versions' list. It is just an attempt to preserve the
+ * original intent for the user configuration
+ */
+ private final List<String> tlsProtocolSubstitution = ImmutableList.of("TLSv1.3", "TLSv1.2", "TLSv1.1", "TLSv1");
+
+ /**
+ * Combine the pre-4.0 protocol field with the accepted_protocols list, substituting a list of
+ * explicit protocols for the previous catchall default of "TLS"
+ * @return array of protocol names suitable for passing to SslContextBuilder.protocols, or null if the default
+ */
+ public List<String> acceptedProtocols()
+ {
+ if (accepted_protocols == null)
+ {
+ if (protocol == null)
+ {
+ return null;
+ }
+ // TLS is accepted by SSLContext.getInstance as a shorthand for give me an engine that
+ // can speak some of the TLS protocols. It is not supported by SSLEngine.setAcceptedProtocols
+ // so substitute if the user hasn't provided an accepted protocol configuration
+ else if (protocol.equalsIgnoreCase("TLS"))
+ {
+ return tlsProtocolSubstitution;
+ }
+ else // the user was trying to limit to a single specific protocol, so try that
+ {
+ return ImmutableList.of(protocol);
+ }
+ }
+
+ if (protocol != null && !protocol.equalsIgnoreCase("TLS") &&
+ accepted_protocols.stream().noneMatch(ap -> ap.equalsIgnoreCase(protocol)))
+ {
+ // If the user provided a non-generic default protocol, append it to accepted_protocols - they wanted
+ // it after all.
+ return ImmutableList.<String>builder().addAll(accepted_protocols).add(protocol).build();
+ }
+ else
+ {
+ return accepted_protocols;
+ }
+ }
+
+ public String[] acceptedProtocolsArray()
+ {
+ List<String> ap = acceptedProtocols();
+ if (ap == null)
Review comment:
unlikely to be throwing stacktraces so happy to take your more concise suggestion.
##########
File path: src/java/org/apache/cassandra/config/EncryptionOptions.java
##########
@@ -209,6 +214,89 @@ public void setOptional(boolean optional) {
this.optional = optional;
}
+ /**
+ * Sets accepted TLS protocol for this channel. Note that this should only be called by
+ * the configuration parser or tests. It is public only for that purpose, mutating protocol state
+ * is probably a bad idea.
+ * @param protocol value to set
+ */
+ @VisibleForTesting
+ public void setProtocol(String protocol) {
+ this.protocol = protocol;
+ }
+
+ /**
+ * Sets accepted TLS protocols for this channel. Note that this should only be called by
+ * the configuration parser or tests. It is public only for that purpose, mutating protocol state
+ * is probably a bad idea. The function casing is required for snakeyaml to find this setter for the protected field.
+ * @param accepted_protocols value to set
+ */
+ public void setaccepted_protocols(List<String> accepted_protocols) {
+ this.accepted_protocols = ImmutableList.copyOf(accepted_protocols);
+ }
+
+ /* This list is substituted in configurations that have explicitly specified the original "TLS" default,
+ * it is not a 'default' list or 'support protocol versions' list. It is just an attempt to preserve the
+ * original intent for the user configuration
+ */
+ private final List<String> tlsProtocolSubstitution = ImmutableList.of("TLSv1.3", "TLSv1.2", "TLSv1.1", "TLSv1");
+
+ /**
+ * Combine the pre-4.0 protocol field with the accepted_protocols list, substituting a list of
+ * explicit protocols for the previous catchall default of "TLS"
+ * @return array of protocol names suitable for passing to SslContextBuilder.protocols, or null if the default
+ */
+ public List<String> acceptedProtocols()
+ {
+ if (accepted_protocols == null)
+ {
+ if (protocol == null)
+ {
+ return null;
+ }
+ // TLS is accepted by SSLContext.getInstance as a shorthand for give me an engine that
+ // can speak some of the TLS protocols. It is not supported by SSLEngine.setAcceptedProtocols
+ // so substitute if the user hasn't provided an accepted protocol configuration
+ else if (protocol.equalsIgnoreCase("TLS"))
+ {
+ return tlsProtocolSubstitution;
+ }
+ else // the user was trying to limit to a single specific protocol, so try that
+ {
+ return ImmutableList.of(protocol);
+ }
+ }
+
+ if (protocol != null && !protocol.equalsIgnoreCase("TLS") &&
+ accepted_protocols.stream().noneMatch(ap -> ap.equalsIgnoreCase(protocol)))
+ {
+ // If the user provided a non-generic default protocol, append it to accepted_protocols - they wanted
+ // it after all.
+ return ImmutableList.<String>builder().addAll(accepted_protocols).add(protocol).build();
+ }
+ else
+ {
+ return accepted_protocols;
+ }
+ }
+
+ public String[] acceptedProtocolsArray()
+ {
+ List<String> ap = acceptedProtocols();
+ if (ap == null)
+ return new String[]{};
+ else
+ return ap.toArray(new String[0]);
+ }
+
+ public String[] cipherSuitesArray()
+ {
+ if (cipher_suites == null)
+ return new String[]{};
Review comment:
ack
##########
File path: src/java/org/apache/cassandra/config/EncryptionOptions.java
##########
@@ -209,6 +214,89 @@ public void setOptional(boolean optional) {
this.optional = optional;
}
+ /**
+ * Sets accepted TLS protocol for this channel. Note that this should only be called by
+ * the configuration parser or tests. It is public only for that purpose, mutating protocol state
+ * is probably a bad idea.
+ * @param protocol value to set
+ */
+ @VisibleForTesting
+ public void setProtocol(String protocol) {
+ this.protocol = protocol;
+ }
+
+ /**
+ * Sets accepted TLS protocols for this channel. Note that this should only be called by
+ * the configuration parser or tests. It is public only for that purpose, mutating protocol state
+ * is probably a bad idea. The function casing is required for snakeyaml to find this setter for the protected field.
+ * @param accepted_protocols value to set
+ */
+ public void setaccepted_protocols(List<String> accepted_protocols) {
+ this.accepted_protocols = ImmutableList.copyOf(accepted_protocols);
+ }
+
+ /* This list is substituted in configurations that have explicitly specified the original "TLS" default,
+ * it is not a 'default' list or 'support protocol versions' list. It is just an attempt to preserve the
+ * original intent for the user configuration
+ */
+ private final List<String> tlsProtocolSubstitution = ImmutableList.of("TLSv1.3", "TLSv1.2", "TLSv1.1", "TLSv1");
+
+ /**
+ * Combine the pre-4.0 protocol field with the accepted_protocols list, substituting a list of
+ * explicit protocols for the previous catchall default of "TLS"
+ * @return array of protocol names suitable for passing to SslContextBuilder.protocols, or null if the default
+ */
+ public List<String> acceptedProtocols()
+ {
+ if (accepted_protocols == null)
+ {
+ if (protocol == null)
+ {
+ return null;
+ }
+ // TLS is accepted by SSLContext.getInstance as a shorthand for give me an engine that
+ // can speak some of the TLS protocols. It is not supported by SSLEngine.setAcceptedProtocols
+ // so substitute if the user hasn't provided an accepted protocol configuration
+ else if (protocol.equalsIgnoreCase("TLS"))
+ {
+ return tlsProtocolSubstitution;
+ }
+ else // the user was trying to limit to a single specific protocol, so try that
+ {
+ return ImmutableList.of(protocol);
+ }
+ }
+
+ if (protocol != null && !protocol.equalsIgnoreCase("TLS") &&
+ accepted_protocols.stream().noneMatch(ap -> ap.equalsIgnoreCase(protocol)))
+ {
+ // If the user provided a non-generic default protocol, append it to accepted_protocols - they wanted
+ // it after all.
+ return ImmutableList.<String>builder().addAll(accepted_protocols).add(protocol).build();
+ }
+ else
+ {
+ return accepted_protocols;
+ }
+ }
+
+ public String[] acceptedProtocolsArray()
+ {
+ List<String> ap = acceptedProtocols();
+ if (ap == null)
+ return new String[]{};
+ else
+ return ap.toArray(new String[0]);
+ }
+
+ public String[] cipherSuitesArray()
+ {
+ if (cipher_suites == null)
Review comment:
ack
----------------------------------------------------------------
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.
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