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/21 00:15:40 UTC
[GitHub] [cassandra] dcapwell commented on a change in pull request #770: CASSANDRA-13325 configure which TLS protocols should be accepted
dcapwell commented on a change in pull request #770:
URL: https://github.com/apache/cassandra/pull/770#discussion_r528030522
##########
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:
nit: `new String[0];`
##########
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:
nit: `new String[0]`
##########
File path: src/java/org/apache/cassandra/db/virtual/SettingsTable.java
##########
@@ -20,7 +20,9 @@
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.Arrays;
+import java.util.List;
Review comment:
can you remove import? not used.
##########
File path: src/java/org/apache/cassandra/config/EncryptionOptions.java
##########
@@ -204,6 +208,79 @@ 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
+ */
+ 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 protocl state
+ * is probably a bad idea.
+ * @param accepted_protocols value to set
+ */
+ public void setaccepted_protocols(List<String> accepted_protocols) {
Review comment:
we should try to find if snakeyaml has something like `@JsonProperty("accepted_protocols")` like Jackson does...
##########
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:
personal preference:
```
return ap == null ? new String[0] : ap.toArray(new String[0]);
```
##########
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);
Review comment:
The constructor now supports `null` but this doesn't, so should we do a null check to match?
```
this.accepted_protocols = accepted_protocols == null ? null : ImmutableList.copyOf(accepted_protocols);
```
##########
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:
personal preference
```
return cipher_suites == null ? new String[0] : cipher_suites.toArray(new String[0])
```
##########
File path: src/java/org/apache/cassandra/config/EncryptionOptions.java
##########
@@ -204,6 +208,79 @@ 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
+ */
+ 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 protocl state
+ * is probably a bad idea.
+ * @param accepted_protocols value to set
+ */
+ public void setaccepted_protocols(List<String> accepted_protocols) {
Review comment:
boo, it looks like there isn't a way, but we do own how yaml is loaded... sadly Ekaterina's patch wasn't merged, as it would make it trivial to have yaml names != java names ðŸ˜
----------------------------------------------------------------
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