exceptionfactory commented on a change in pull request #5008:
URL: https://github.com/apache/nifi/pull/5008#discussion_r616699567
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -1012,6 +1012,30 @@ protected static void
configureSslContextFactory(SslContextFactory.Server contex
contextFactory.setIncludeProtocols(TlsConfiguration.getCurrentSupportedTlsProtocolVersions());
contextFactory.setExcludeProtocols("TLS", "TLSv1", "TLSv1.1", "SSL",
"SSLv2", "SSLv2Hello", "SSLv3");
+ // on configuration, replace default application cipher suites with
those configured
+ final String includeCipherSuitesProps =
props.getProperty(NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES);
+ if (!StringUtils.isEmpty(includeCipherSuitesProps)) {
+ logger.info(String.format("NiFiProperties.%s = %s",
+ NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES,
includeCipherSuitesProps));
Review comment:
In light of the following informational log message, this log message
does not seem necessary.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -1012,6 +1012,30 @@ protected static void
configureSslContextFactory(SslContextFactory.Server contex
contextFactory.setIncludeProtocols(TlsConfiguration.getCurrentSupportedTlsProtocolVersions());
contextFactory.setExcludeProtocols("TLS", "TLSv1", "TLSv1.1", "SSL",
"SSLv2", "SSLv2Hello", "SSLv3");
+ // on configuration, replace default application cipher suites with
those configured
+ final String includeCipherSuitesProps =
props.getProperty(NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES);
+ if (!StringUtils.isEmpty(includeCipherSuitesProps)) {
+ logger.info(String.format("NiFiProperties.%s = %s",
+ NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES,
includeCipherSuitesProps));
+ final String[] includeCipherSuitesRuntime =
contextFactory.getIncludeCipherSuites();
+ final String[] includeCipherSuites =
includeCipherSuitesProps.split(",\\s*");
+ logger.info(String.format("Replacing include cipher suites with
configuration; FROM [%s] TO [%s].",
+ ((includeCipherSuitesRuntime == null) ? null :
Arrays.asList(includeCipherSuitesRuntime)),
+ String.join(", ", Arrays.asList(includeCipherSuites))));
Review comment:
Recommend restructuring this statement to use logging placeholders
instead of String.format(). Using the configured property value seems more
straightforward than reformatting the parsed string array.
```suggestion
logger.info("Configuring TLS included cipher suites [{}]
replacing default [{}]", includeCipherSuitesProps,
Arrays.asList(includeCipherSuitesRuntime));
```
##########
File path:
nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java
##########
@@ -207,6 +207,8 @@
public static final String WEB_HTTPS_PORT = "nifi.web.https.port";
public static final String WEB_HTTPS_PORT_FORWARDING =
"nifi.web.https.port.forwarding";
public static final String WEB_HTTPS_HOST = "nifi.web.https.host";
+ public static final String WEB_HTTPS_INCLUDE_CIPHERSUITES =
"nifi.web.https.includeciphersuites";
+ public static final String WEB_HTTPS_EXCLUDE_CIPHERSUITES =
"nifi.web.https.excludeciphersuites";
Review comment:
Given the multiple words included, and the parallel nature of the
properties, what do you think about adjusting the naming to
`nifi.web.https.ciphersuites.include` and `nifi.web.https.ciphersuites.exclude`?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -1012,6 +1012,30 @@ protected static void
configureSslContextFactory(SslContextFactory.Server contex
contextFactory.setIncludeProtocols(TlsConfiguration.getCurrentSupportedTlsProtocolVersions());
contextFactory.setExcludeProtocols("TLS", "TLSv1", "TLSv1.1", "SSL",
"SSLv2", "SSLv2Hello", "SSLv3");
+ // on configuration, replace default application cipher suites with
those configured
+ final String includeCipherSuitesProps =
props.getProperty(NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES);
+ if (!StringUtils.isEmpty(includeCipherSuitesProps)) {
Review comment:
This could be streamlined to `StringUtils.isNotEmpty()`
```suggestion
if (StringUtils.isNotEmpty(includeCipherSuitesProps)) {
```
##########
File path: nifi-docs/src/main/asciidoc/administration-guide.adoc
##########
@@ -209,6 +209,28 @@ In order to facilitate the secure setup of NiFi, you can
use the `tls-toolkit` c
* <<toolkit-guide.adoc#tls_intermediate_ca,Using An Existing Intermediate
Certificate Authority>>
* <<toolkit-guide.adoc#additional_certificate_commands,Additional Certificate
Commands>>
+[[tls_cipher_suites]]
+=== TLS Cipher Suites
+
+The Java Runtime Environment provides the ability to specify custom TLS cipher
suites to be used by servers when accepting client connections. See
+link:https://java.com/en/configure_crypto.html[here^] for more information.
To use this feature for the NiFi web service, the following NiFi properties
+may be set:
+
+[options="header,footer"]
+|==================================================================================================================================================
+| Property Name | Description
+|`nifi.web.https.includeciphersuites` | Additional ciphers that may be used by
incoming client connections.
Review comment:
The description seems a little confusing on initial read. The Jetty
`SslContextFactory.selectCipherSuites` method appears to limit selected cipher
suites based on included values, is that correct? In other words, the list of
included cipher suites would not be in addition to the default set, but limited
to what is included. If that is not the case, it would be helpful to indicate
that this is in addition to system defaults.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/test/java/org/apache/nifi/web/server/JettyServerTest.java
##########
@@ -142,4 +146,64 @@ public void
testConfigureSslContextFactoryWithPkcsTrustStore() {
verify(mockSCF).setTrustStoreType(trustStoreType);
verify(mockSCF).setTrustStoreProvider(BouncyCastleProvider.PROVIDER_NAME);
}
+
+ /**
+ * Verify correct processing of cipher suites with multiple elements.
Verify call to override runtime ciphers.
+ */
+ @Test
+ public void testConfigureSslIncludeExcludeCiphers() {
+ final String[] includeCipherSuites = {"TLS_AES_256_GCM_SHA384",
"TLS_AES_128_GCM_SHA256"};
+ final String includeCipherSuitesProp = String.join(", ",
Arrays.asList(includeCipherSuites));
+ final String[] excludeCipherSuites = {".*DHE.*", ".*ECDH.*"};
+ final String excludeCipherSuitesProp = String.join(", ",
Arrays.asList(excludeCipherSuites));
+ final Map<String, String> addProps = new HashMap<>();
+ addProps.put(NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES,
includeCipherSuitesProp);
+ addProps.put(NiFiProperties.WEB_HTTPS_EXCLUDE_CIPHERSUITES,
excludeCipherSuitesProp);
+ final NiFiProperties nifiProperties =
NiFiProperties.createBasicNiFiProperties(null, addProps);
+
+ final SslContextFactory.Server mockSCF =
mock(SslContextFactory.Server.class);
+ JettyServer.configureSslContextFactory(mockSCF, nifiProperties);
+ verify(mockSCF, times(1)).getIncludeCipherSuites();
+ verify(mockSCF, times(1)).getExcludeCipherSuites();
+ verify(mockSCF, times(1)).setIncludeCipherSuites(includeCipherSuites);
+ verify(mockSCF, times(1)).setExcludeCipherSuites(excludeCipherSuites);
+ }
+
+ /**
+ * Verify skip cipher configuration when NiFiProperties are not specified.
+ */
+ @Test
+ public void testDoNotConfigureSslIncludeExcludeCiphers() {
+ final NiFiProperties nifiProperties =
NiFiProperties.createBasicNiFiProperties(null);
+ final SslContextFactory.Server mockSCF =
mock(SslContextFactory.Server.class);
+ JettyServer.configureSslContextFactory(mockSCF, nifiProperties);
+ verify(mockSCF, times(0)).getIncludeCipherSuites();
+ verify(mockSCF, times(0)).getExcludeCipherSuites();
+ verify(mockSCF, times(0)).setIncludeCipherSuites(any());
+ verify(mockSCF, times(0)).setExcludeCipherSuites(any());
+ }
+
+ /**
+ * Verify strategy for translating between String representation (in
NiFiProperties)
+ * and String[] representation (hand off to JCE API).
+ */
+ @Test
+ public void testTransformStringToStringArray() {
+ final String[] strings = {
+ "ABC",
+ "ABC, DEF",
+ "ABC, DEF, GHI", // single space is specified below in
`String.join()` delimiter
+ };
+ final String[][] stringArrays = {
+ { "ABC" },
+ { "ABC", "DEF" },
+ { "ABC", "DEF", "GHI" },
+ };
+ for (int i = 0; (i < strings.length); ++i) {
+ final String string = strings[i];
+ final String[] stringArray = stringArrays[i];
+ Assert.assertEquals(string, String.join(", ",
Arrays.asList(stringArray)));
+ Assert.assertArrayEquals(stringArray, string.split(",\\s*"));
+ }
+ }
Review comment:
Is this test method necessary? Since it does not exercise any of the
JettyServer methods, it looks like it should be removed.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -1012,6 +1012,30 @@ protected static void
configureSslContextFactory(SslContextFactory.Server contex
contextFactory.setIncludeProtocols(TlsConfiguration.getCurrentSupportedTlsProtocolVersions());
contextFactory.setExcludeProtocols("TLS", "TLSv1", "TLSv1.1", "SSL",
"SSLv2", "SSLv2Hello", "SSLv3");
+ // on configuration, replace default application cipher suites with
those configured
+ final String includeCipherSuitesProps =
props.getProperty(NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES);
+ if (!StringUtils.isEmpty(includeCipherSuitesProps)) {
+ logger.info(String.format("NiFiProperties.%s = %s",
+ NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES,
includeCipherSuitesProps));
+ final String[] includeCipherSuitesRuntime =
contextFactory.getIncludeCipherSuites();
+ final String[] includeCipherSuites =
includeCipherSuitesProps.split(",\\s*");
+ logger.info(String.format("Replacing include cipher suites with
configuration; FROM [%s] TO [%s].",
+ ((includeCipherSuitesRuntime == null) ? null :
Arrays.asList(includeCipherSuitesRuntime)),
+ String.join(", ", Arrays.asList(includeCipherSuites))));
+ contextFactory.setIncludeCipherSuites(includeCipherSuites);
+ }
+ final String excludeCipherSuitesProps =
props.getProperty(NiFiProperties.WEB_HTTPS_EXCLUDE_CIPHERSUITES);
+ if (!StringUtils.isEmpty(excludeCipherSuitesProps)) {
Review comment:
As mentioned above, this could be replaced with StringUtils.isNotEmpty().
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -1012,6 +1012,30 @@ protected static void
configureSslContextFactory(SslContextFactory.Server contex
contextFactory.setIncludeProtocols(TlsConfiguration.getCurrentSupportedTlsProtocolVersions());
contextFactory.setExcludeProtocols("TLS", "TLSv1", "TLSv1.1", "SSL",
"SSLv2", "SSLv2Hello", "SSLv3");
+ // on configuration, replace default application cipher suites with
those configured
+ final String includeCipherSuitesProps =
props.getProperty(NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES);
+ if (!StringUtils.isEmpty(includeCipherSuitesProps)) {
+ logger.info(String.format("NiFiProperties.%s = %s",
+ NiFiProperties.WEB_HTTPS_INCLUDE_CIPHERSUITES,
includeCipherSuitesProps));
+ final String[] includeCipherSuitesRuntime =
contextFactory.getIncludeCipherSuites();
+ final String[] includeCipherSuites =
includeCipherSuitesProps.split(",\\s*");
+ logger.info(String.format("Replacing include cipher suites with
configuration; FROM [%s] TO [%s].",
+ ((includeCipherSuitesRuntime == null) ? null :
Arrays.asList(includeCipherSuitesRuntime)),
+ String.join(", ", Arrays.asList(includeCipherSuites))));
+ contextFactory.setIncludeCipherSuites(includeCipherSuites);
+ }
+ final String excludeCipherSuitesProps =
props.getProperty(NiFiProperties.WEB_HTTPS_EXCLUDE_CIPHERSUITES);
+ if (!StringUtils.isEmpty(excludeCipherSuitesProps)) {
+ logger.info(String.format("NiFiProperties.%s = %s",
+ NiFiProperties.WEB_HTTPS_EXCLUDE_CIPHERSUITES,
excludeCipherSuitesProps));
Review comment:
Recommend removing this log message in the light of the following
informational message that includes the same configuration property.
--
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:
us...@infra.apache.org
