You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by m-hogue <gi...@git.apache.org> on 2017/07/06 15:44:33 UTC
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
GitHub user m-hogue opened a pull request:
https://github.com/apache/nifi/pull/1986
NIFI-2528: added support for SSLContextService protocols in ListenHTTP
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/m-hogue/nifi NIFI-2528
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1986.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1986
----
commit 399d34af8725330d5d8b947ca69e643623fe908c
Author: m-hogue <ho...@gmail.com>
Date: 2017-07-06T15:42:46Z
NIFI-2528: added support for SSLContextService protocols in ListenHTTP
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135371152
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardRestrictedSSLContextService.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.nifi.annotation.documentation.CapabilityDescription;
+import org.apache.nifi.annotation.documentation.Tags;
+import org.apache.nifi.components.PropertyDescriptor;
+import org.apache.nifi.components.ValidationContext;
+import org.apache.nifi.processor.util.StandardValidators;
+
+/**
+ * This class is functionally the same as {@link StandardSSLContextService}, but it restricts the allowable
+ * values that can be selected for SSL protocols.
+ */
+@Tags({"ssl", "secure", "certificate", "keystore", "truststore", "jks", "p12", "pkcs12", "pkcs"})
+@CapabilityDescription("Restricted implementation of the SSLContextService. Provides the ability to configure "
+ + "keystore and/or truststore properties once and reuse that configuration throughout the application, "
+ + "but only allows a restricted set of SSL protocols to be chosen. The set of protocols selectable will "
+ + "evolve over time as new protocols emerge and older protocols are deprecated. This service is recommended "
+ + "over StandardSSLContextService if a component doesn't expect to communicate with legacy systems since it's "
+ + "unlikely that legacy systems will support these protocols.")
+public class StandardRestrictedSSLContextService extends StandardSSLContextService implements RestrictedSSLContextService {
+
+ public static final PropertyDescriptor RESTRICTED_SSL_ALGORITHM = new PropertyDescriptor.Builder()
+ .name("SSL Protocol")
+ .defaultValue("TLSv1.2")
--- End diff --
We should include `.displayName("TLS Protocol")` here for human-readable naming without conflicting with the legacy property descriptor name.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1986
@alopresto - I'm not sure I understand your concerns. The current implementation won't support SSLv3, and will throw an error if someone selects that protocol in their StandardSSLContextService and allows selection of supported protocols StandardSSLContextService (that jetty also supports).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135885847
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/SSLContextServiceUtils.java ---
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.SSLContext;
+
+import org.apache.nifi.components.AllowableValue;
+
+public class SSLContextServiceUtils {
--- End diff --
With Java 8, you can provide a `static` method implementation on an `interface`. I'd define a static method `buildAllowableValues()` in `SSLContextService` and implement it with the complete list, and "override" (define a method with the same signature but it's not technically an override in this case) in `RestrictedSSLContextService` which returns the limited list.
Am I missing something here? I haven't tried it myself but I think this should work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135547306
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/SSLContextServiceUtils.java ---
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.SSLContext;
+
+import org.apache.nifi.components.AllowableValue;
+
+public class SSLContextServiceUtils {
+
+ /**
+ * Build a set of allowable SSL protocol algorithms based on JVM configuration and whether
+ * or not the list should be restricted to include only certain protocols.
+ *
+ * @param restricted whether the set of allowable protocol values should be restricted.
+ *
+ * @return the computed set of allowable values
+ */
+ public static AllowableValue[] buildSSLAlgorithmAllowableValues(final boolean restricted) {
+ final Set<String> supportedProtocols = new HashSet<>();
+
+ // if restricted, only allow the below set of protocols.
+ if(restricted) {
+ supportedProtocols.add("TLSv1.2");
--- End diff --
Good call. I'll add this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135886286
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardRestrictedSSLContextService.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.nifi.annotation.documentation.CapabilityDescription;
+import org.apache.nifi.annotation.documentation.Tags;
+import org.apache.nifi.components.PropertyDescriptor;
+import org.apache.nifi.components.ValidationContext;
+import org.apache.nifi.processor.util.StandardValidators;
+
+/**
+ * This class is functionally the same as {@link StandardSSLContextService}, but it restricts the allowable
+ * values that can be selected for SSL protocols.
+ */
+@Tags({"ssl", "secure", "certificate", "keystore", "truststore", "jks", "p12", "pkcs12", "pkcs"})
+@CapabilityDescription("Restricted implementation of the SSLContextService. Provides the ability to configure "
+ + "keystore and/or truststore properties once and reuse that configuration throughout the application, "
+ + "but only allows a restricted set of SSL protocols to be chosen. The set of protocols selectable will "
+ + "evolve over time as new protocols emerge and older protocols are deprecated. This service is recommended "
+ + "over StandardSSLContextService if a component doesn't expect to communicate with legacy systems since it's "
+ + "unlikely that legacy systems will support these protocols.")
+public class StandardRestrictedSSLContextService extends StandardSSLContextService implements RestrictedSSLContextService {
+
+ public static final PropertyDescriptor RESTRICTED_SSL_ALGORITHM = new PropertyDescriptor.Builder()
+ .name("SSL Protocol")
+ .defaultValue("TLSv1.2")
--- End diff --
No, I left `.name()` the same to be backward compatible (changing the name means that the value stored in the flow will not be retrieved on load). This is the whole reason `.displayName()` was introduced -- it provides a human-facing value that isn't used for value resolution.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135370899
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/SSLContextServiceUtils.java ---
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.SSLContext;
+
+import org.apache.nifi.components.AllowableValue;
+
+public class SSLContextServiceUtils {
--- End diff --
Does this need to be a separate utility class? It seems to only have one method. I would have anticipated this method being added to the `SSLContextService` interface and then implemented by each of the concrete classes. Thus the logic would be encapsulated in the implementations rather than outside of the provided implementations.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1986
Also, as @joewitt noted earlier, we should change the available interface for other "listener" processors. Here's a preliminary list I put together, but I would like confirmation from another member:
* `HandleHTTPRequest`
* `ListenBeats`
* `DistributedCacheServer`/`DistributedSetCacheServer`/`DistributedMapCacheServer`
* `ListenSMTP`
* `ListenGRPC`
* `ListenLumberjack` (*Deprecated*)
* `ListenRELP`
* `ListenSyslog`
* `ListenTCP`/`ListenTCPRecord`
Also:
* `AbstractSiteToSiteReportingTask`
* `org.apache.nifi.processors.slack.TestServer`
* `WebSocketService`/`JettyWebSocketService`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by jskora <gi...@git.apache.org>.
Github user jskora commented on the issue:
https://github.com/apache/nifi/pull/1986
@trkurc Have you [enabled SSLv3](https://stackoverflow.com/questions/28236091/how-to-enable-ssl-3-in-java) in the JVM?
It is disabled by default starting with [Java 8 Update 31](http://www.oracle.com/technetwork/java/javase/8u31-relnotes-2389094.html) and [Java 7 Update 75](http://www.oracle.com/technetwork/java/javase/7u75-relnotes-2389086.html)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1986
@alopresto : I've replied with a couple of questions on your comments.
Also, i'll create an issue to update the SSL Context Service interface for the listed processors (once confirmed).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1986
Jetty explicitly disables it as well. The default set of protocols disallowed by Jetty are {"SSL", "SSLv2", "SSLv2Hello", "SSLv3"}
I'm happy to alter Jetty's default config, but should we encourage use of SSLv3?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1986
What was the previous behavior if an unavailable protocol version was selected? My understanding is that it will silently use a more secure available protocol. This is debatable about which is better -- from an "informed user" perspective, being notified that `SSLv3` is not valid is good. From a "just works" perspective, stopping the flow (especially with an error that doesn't tell them *why* `SSLv3`, an option they selected from a dropdown as opposed to typing in manually, doesn't work or *how* to fix it) is worse.
I reproduced this on 1.4.0 master and even manually selecting `SSLv3` for the controller service, it exposed a port that only accepted `TLSv1.2`:
```
hw12203:...space/nifi/nifi-assembly/target/nifi-1.4.0-SNAPSHOT-bin/nifi-1.4.0-SNAPSHOT (master) alopresto
🔓 9s @ 17:58:59 $ openssl s_client -connect localhost:9999 -debug -state -CAfile conf/nifi-cert.pem
...
New, TLSv1/SSLv3, Cipher is ECDHE-RSA-AES256-SHA384 ## This is a legacy output of s_client, it's not actually using SSLv3 as shown below
Server public key is 2048 bit
Secure Renegotiation IS supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
Protocol : TLSv1.2
Cipher : ECDHE-RSA-AES256-SHA384
Session-ID: 597698...709D69
Session-ID-ctx:
Master-Key: 4CA2AD...6926BA
Key-Arg : None
PSK identity: None
PSK identity hint: None
SRP username: None
Start Time: 1500944417
Timeout : 300 (sec)
Verify return code: 0 (ok)
---
hw12203:...space/nifi/nifi-assembly/target/nifi-1.4.0-SNAPSHOT-bin/nifi-1.4.0-SNAPSHOT (master) alopresto
🔓 87s @ 18:00:18 $ openssl s_client -connect localhost:9999 -debug -state -CAfile conf/nifi-cert.pem -ssl3
...
---
New, (NONE), Cipher is (NONE)
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
Protocol : SSLv3
Cipher : 0000
Session-ID:
Session-ID-ctx:
Master-Key:
Key-Arg : None
PSK identity: None
PSK identity hint: None
SRP username: None
Start Time: 1500944681
Timeout : 7200 (sec)
Verify return code: 0 (ok)
---
```
I am not opposed to throwing an error if an invalid protocol is somehow provided to the context factory, I just think we should be doing more to prevent that scenario from happening in the first place, and this behavior isn't compelling enough to me that I think it needs to be merged immediately and the UX improvement added later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by jskora <gi...@git.apache.org>.
Github user jskora commented on the issue:
https://github.com/apache/nifi/pull/1986
I don't want to complicate this, but I feel like I must be missing something.
As much as possible, the validation at configuration time should provide the user feedback, not failure upon execution. So,
- SSLContextService should only allow selection of supported protocols, if SSLv3 will not work it should be removed from the list, and
- if we need SSLv3 in some places we either need 2 variations of SSLContextService like @alopresto mentioned earlier or the ability to configure and query the security level so processors requiring only the higher security protocols can invalidate in the GUI (before execution) if provided a less secure service.
Does that make sense?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1986
@alopresto : All of your recommendations have been implemented. Please let me know if you'd like any more changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1986
reviewing now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1986
@m-hogue - do you have ideas on how to improve the user experience given the above feedback?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1986
From a "just works" perspective, if I happened to have a client that only worked with SSLv3 for whatever reason and attempted to configure it as such, I think no user feedback and an unexpected configuration is far less compelling
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1986
Ok I left some minor comments on the code. If Michael can reply to those and make the changes, I think this is good and ready to be merged. I set up a flow with a `ListenHTTP` processor and verified that I could only provide it with a `StandardRestrictedSSLContextService` implementation. I verified that it received incoming requests (*only*) over TLS v1.2.
```
hw12203:/Users/alopresto/Workspace/scratch (master) alopresto
🔓 27314s @ 18:11:29 $ openssl s_client -connect localhost:9999 -debug -showcerts
CONNECTED(00000003)
write to 0x7f80b0d89fd0 [0x7f80b1807e00] (308 bytes => 308 (0x134))
0000 - 16 03 01 01 2f 01 00 01-2b 03 03 29 cb d3 e6 54 ..../...+..)...T
...
0050 - 64 f9 0d 7b c4 03 6b 71-03 4d a4 1d 8a f7 4d 45 d..{..kq.M....ME
---
Certificate chain
0 s:/OU=NIFI/CN=nifi.nifi.apache.org
i:/OU=NIFI/CN=localhost
...
---
Server certificate
subject=/OU=NIFI/CN=nifi.nifi.apache.org
issuer=/OU=NIFI/CN=localhost
---
No client certificate CA names sent
Peer signing digest: SHA512
Server Temp Key: ECDH, P-256, 256 bits
---
SSL handshake has read 2241 bytes and written 490 bytes
---
New, TLSv1/SSLv3, Cipher is ECDHE-RSA-AES256-SHA384
Server public key is 2048 bit
Secure Renegotiation IS supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
Protocol : TLSv1.2
Cipher : ECDHE-RSA-AES256-SHA384
Session-ID: 59A0CAC680787984AD9B43E8A39BCFB0F4C5EA4F8AC10223C073296EDB8FB66B
Session-ID-ctx:
Master-Key: 236BC9B03CD3F7B02C363C8DA15F36EA908A631DB0D3828A0CE05E3834D07BB58E9D1A7023A5161DCE13BF58029BCD61
Key-Arg : None
PSK identity: None
PSK identity hint: None
SRP username: None
Start Time: 1503709893
Timeout : 300 (sec)
Verify return code: 19 (self signed certificate in certificate chain)
---
Q
DONE
hw12203:/Users/alopresto/Workspace/scratch (master) alopresto
🔓 27323s @ 18:11:38 $ openssl s_client -connect localhost:9999 -debug -showcerts -tls1_1
CONNECTED(00000003)
write to 0x7fd06181a060 [0x7fd06280f003] (200 bytes => 200 (0xC8))
0000 - 16 03 01 00 c3 01 00 00-bf 03 02 18 09 95 74 f0 ..............t.
... .(
140735215808592:error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:s3_pkt.c:1494:SSL alert number 40
140735215808592:error:1409E0E5:SSL routines:ssl3_write_bytes:ssl handshake failure:s3_pkt.c:659:
---
no peer certificate available
---
No client certificate CA names sent
---
SSL handshake has read 7 bytes and written 0 bytes
---
New, (NONE), Cipher is (NONE)
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
Protocol : TLSv1.1
Cipher : 0000
Session-ID:
Session-ID-ctx:
Master-Key:
Key-Arg : None
PSK identity: None
PSK identity hint: None
SRP username: None
Start Time: 1503712071
Timeout : 7200 (sec)
Verify return code: 0 (ok)
---
hw12203:/Users/alopresto/Workspace/scratch (master) alopresto
🔓 29497s @ 18:47:53 $
```
I also set up two `InvokeHTTP` processors and used a `StandardSSLContextService` and `StandardRestrictedSSLContextService` for each. Both were able to successfully make outgoing `GET` requests to `https://nifi.apache.org`.
Contrib-check and all tests pass. Just need Michael to respond to the few comments above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1986
@alopresto - fair points, however, I do think this is a step in the right direction in terms of user experience - previously, when selecting an StandardSSLContextService and protocol, the choice was ignored (i.e. , if the user did select SSLv3 in the StandardSSLContextService, expecting the ListenHTTP to support SSLv3, that didn't happen).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1986
@trkurc My initial reaction was to earlier comments that I interpreted to open the idea of manually overriding the current state, which does not allow `SSLv3` to be used. I should have cleaned that up after understanding the entire thread. However, I feel that my second point, regarding restricting the dropdown list of available protocol versions to provide a better user experience rather than throwing an exception on 71% of the available options still stands. I understand it's more work for the developers at this time, but in general I favor more work for us to vastly improve the user experience, as once this is used by thousands of people, those little problems add up quickly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135371045
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardRestrictedSSLContextService.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.nifi.annotation.documentation.CapabilityDescription;
+import org.apache.nifi.annotation.documentation.Tags;
+import org.apache.nifi.components.PropertyDescriptor;
+import org.apache.nifi.components.ValidationContext;
+import org.apache.nifi.processor.util.StandardValidators;
+
+/**
+ * This class is functionally the same as {@link StandardSSLContextService}, but it restricts the allowable
--- End diff --
I think in the documentation, tags, and descriptions, *TLS* should be emphasized over *SSL*, as SSL is deprecated. I would not recommend changing the name of the class though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1986
I'm sorry I'm late on this, but why are we trying to allow `SSLv3` for anything? I understand the original reason for this work was to be in line with the `PostHTTP` processor, but this makes outgoing connections to remote services, which may be legacy systems that do not support new TLS protocol versions. While I would discourage integrating with those systems as well, as `SSLv3` provides little effective security at this point, we are not exposing a vulnerability in NiFi by supporting that protocol version.
Jetty intentionally disables any protocol version below `TLSv1.2`, and thus we require any incoming connections to support this protocol version. I believe the proper solution here is to remove `SSLv2` and `SSLv3` (along with `TLSv1` and `TLSv1.1`) from the dropdown list of the `SSLContextService` when used for *listening* (i.e. accepting incoming connections) as they will not be supported, rather than throwing an exception if an invalid one is selected (by my count, 5 of the 7 available are invalid for incoming connections -- `SSL`, `SSLv2Hello`, `SSLv3`, `TLSv1`, and `TLSv1.1`; `TLS` and `TLSv1.2` are valid).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1986
@trkurc : It'd occur when the processor {{@OnScheduled}} method is called, which would prevent the processor from starting. This would be evident in the UI as a red flag on the processor when you start it:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135896059
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/SSLContextServiceUtils.java ---
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.SSLContext;
+
+import org.apache.nifi.components.AllowableValue;
+
+public class SSLContextServiceUtils {
--- End diff --
You can indeed have static methods in interfaces, but they cannot be overridden by child classes. They 'belong' to the interface. There were also default methods added in Java 8, which can be overridden by child classes, but they aren't static meaning that they can't be used in statically initialized variables. For these reasons, i elected to put the methods in a util class - but i totally understand the concern here.
See this SO article: https://stackoverflow.com/questions/27833168/difference-between-static-and-default-methods-in-interface
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1986
I am trying to walk the line between "make everything configurable" and "sometimes people who don't understand this configure it". If you have a client that only supports `SSLv3`, it won't work with `ListenHTTP` period.
* Current situation: the connection will fail, and the error presented to the client will be some form of `INVALID PROTOCOL VERSION`. No error in NiFi.
* Proposed error on config: New flows can't start, as the processor is invalid. Existing flows which use `SSLv3` will stop working, and the error simply says "SSLv3 isn't valid". No open port for client to connect to, so that's the error on that end.
* Proposed restricted list implementation: *Can't get to invalid state on new processor config.* Existing flows which use `SSLv3` will stop working, and the error says "SSLv3 isn't valid -- pick TLSv1.2". No open port for client to connect to, so that's the error on that end, but the NiFi DFM at least knows a "valid" option to report back to the external client admin/operator.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1986
@m-hogue - where would the exception occur? would it prevent the processor from starting, or just when it fires up the jetty server? would the problem be evident on the UI?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/1986
Some thoughts on this thread. First it is good to get this cleaned up and easier for the user so this is a great discussion.
1. It sounds prudent that we delineate our SSLContextService along two lines. One is StandardSSLContextService which works as our current one does, supports numerous protocols including those we do not recommend be in continued use but that we need to support for interacting/initiating connections to systems using legacy protocols. The other would be a more restricted/conservative set of protocols like TLSv1.2. I recommend we call that RestrictedSSLContextService. We can document on the existing Standard service what it is for and allowable for. And we can do the same on restricted indicating it is only for 'safe' protocols and may evolve as more is learned/made available. Both would follow the same interface most likely.
2. We update ListenHTTP,HandleRequest,(any proc that acts as a listen/server) to allow only the RestrictedSSLContextService. This is within the documented transition changes between minor versions, can be captured in our release/migration notes, and will improve the user experience to only accept allowed protocols.
3. We should avoid to the extent possible ever doing validation of a given configuration of a component/processor only once started. And we should avoid using onScheduled to block startup/etc.. Once a user is passed the validation phase this is what process yielding is for. The logic to test whether the protocols used, which would not matter if (1) and (2) above are done, should be done as early in the lifecycle as possible to let the user resolve the issue before we slap their hand by stopping their process (and that is only when considering this done in a non-programmatic case). I realize this isn't purely clear all the time but am saying this from an intent of the API, thinking about when we bind validation issues, and considering that the API is for both programmatic and human interaction.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1986
I'll track down error reporting.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1986
I realize that performing this logic (controller service used for listening vs. outgoing connections) dynamically may be challenging or not possible (CS can be used by multiple referencing components). This is now leading me to think we may need to create a new implementation of the [`SSLContextService`](https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-service-api/src/main/java/org/apache/nifi/ssl/SSLContextService.java#L33-L33) interface called `ModernSSLContextService` or `IncomingSSLContextService` which only supports the `TLSv1.2` protocol version. Existing processors would not change because they can still accept `StandardSSLContextService`, but the `ListenHTTP` component would only accept this new implementation. It could even extend `StandardSSLContextService` to delegate most of the code there and simply override the protocol versions by implementing its own [`#buildAlgorithmAllowableValues()`](https://github.com/apache/n
ifi/blob/master/nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardSSLContextService.java#L473) method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1986
Joe and Mike, sort of to my point, and both of your comments reinforced it, it was counter intuitive that I could select a protocol, and have it not work or give an error message that was substantive.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1986
the approach looks good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1986
All - I've added a `RestrictedSSLContextService` interface which extends the `SSLContextService` interface and a `StandardRestrictedSSLContextService` implementation which extends `StandardSSLContextService` to allow only certain protocols. In particular, _only_ TLSv1.2.
I then changed `ListenHTTP` to only allow `RestrictedSSLContextService` implementations. Please let me know if there are any other changes you'd like for me to make.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135548481
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardRestrictedSSLContextService.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.nifi.annotation.documentation.CapabilityDescription;
+import org.apache.nifi.annotation.documentation.Tags;
+import org.apache.nifi.components.PropertyDescriptor;
+import org.apache.nifi.components.ValidationContext;
+import org.apache.nifi.processor.util.StandardValidators;
+
+/**
+ * This class is functionally the same as {@link StandardSSLContextService}, but it restricts the allowable
+ * values that can be selected for SSL protocols.
+ */
+@Tags({"ssl", "secure", "certificate", "keystore", "truststore", "jks", "p12", "pkcs12", "pkcs"})
+@CapabilityDescription("Restricted implementation of the SSLContextService. Provides the ability to configure "
+ + "keystore and/or truststore properties once and reuse that configuration throughout the application, "
+ + "but only allows a restricted set of SSL protocols to be chosen. The set of protocols selectable will "
+ + "evolve over time as new protocols emerge and older protocols are deprecated. This service is recommended "
+ + "over StandardSSLContextService if a component doesn't expect to communicate with legacy systems since it's "
+ + "unlikely that legacy systems will support these protocols.")
+public class StandardRestrictedSSLContextService extends StandardSSLContextService implements RestrictedSSLContextService {
+
+ public static final PropertyDescriptor RESTRICTED_SSL_ALGORITHM = new PropertyDescriptor.Builder()
+ .name("SSL Protocol")
+ .defaultValue("TLSv1.2")
--- End diff --
Do you suggest that i change the `.name()` to "TLS Protocol" here as well?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r136231936
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardRestrictedSSLContextService.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.nifi.annotation.documentation.CapabilityDescription;
+import org.apache.nifi.annotation.documentation.Tags;
+import org.apache.nifi.components.PropertyDescriptor;
+import org.apache.nifi.components.ValidationContext;
+import org.apache.nifi.processor.util.StandardValidators;
+
+/**
+ * This class is functionally the same as {@link StandardSSLContextService}, but it restricts the allowable
--- End diff --
I meant to add TLS in addition to SSL (some people will still search for or only recognize SSL). This is a minor thing and I'll fix it before merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r129185483
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java ---
@@ -227,7 +227,19 @@ private void createHttpServerFromService(final ProcessContext context) throws Ex
contextFactory.setKeyStoreType(keyStoreType);
}
- if(sslContextService != null) {
+ if (sslContextService != null) {
+ // if the configured protocol isn't supported by Jetty, throw an exception
+ final String[] excludeProtocols = contextFactory.getExcludeProtocols();
+ if (excludeProtocols != null) {
+ for (final String protocol : excludeProtocols) {
+ if (protocol.equals(sslContextService.getSslAlgorithm())) {
+ final IllegalArgumentException e = new IllegalArgumentException("The configured SSL Protocol '" + sslContextService.getSslAlgorithm()
+ + "' is not supported by this processor. Please choose another.");
--- End diff --
It may be helpful here to provide a list of supported protocols (I believe [`#getSelectedProtocols()`](http://download.eclipse.org/jetty/stable-9/apidocs/org/eclipse/jetty/util/ssl/SslContextFactory.html#getSelectedProtocols--) will do this; `#getIncludeProtocols()` also exists, but be aware that *excluded* protocols will always override *included* [see `SSLContextFactory#selectProtocols()`](https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#L1186)).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/1986
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1986
It seems as though if I change to SSLv3, for example, that I am unable to POST.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135370201
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/SSLContextServiceUtils.java ---
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.SSLContext;
+
+import org.apache.nifi.components.AllowableValue;
+
+public class SSLContextServiceUtils {
+
+ /**
+ * Build a set of allowable SSL protocol algorithms based on JVM configuration and whether
+ * or not the list should be restricted to include only certain protocols.
+ *
+ * @param restricted whether the set of allowable protocol values should be restricted.
+ *
+ * @return the computed set of allowable values
+ */
+ public static AllowableValue[] buildSSLAlgorithmAllowableValues(final boolean restricted) {
+ final Set<String> supportedProtocols = new HashSet<>();
+
+ // if restricted, only allow the below set of protocols.
+ if(restricted) {
+ supportedProtocols.add("TLSv1.2");
--- End diff --
I know having `{TLS, TLSv1.2}` may look ambiguous or confusing in the restricted set, but I think including `TLS` is good here because when [TLSv1.3](https://blog.cloudflare.com/introducing-tls-1-3/) (see also [2](https://www.openssl.org/blog/blog/2017/05/04/tlsv1.3/) [3](https://kinsta.com/blog/tls-1-3/)) and so on are supported, users won't have to revisit this and explicitly enable/upgrade to that protocol version. We should improve the `TLS` tooltip to explain that it chooses the highest supported TLS protocol version automatically, and perhaps identify the minimum supported version.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1986
@trkurc : Pushed changes. Please let me know if you'd like any additional changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1986
@alopresto @jskora : So i mentioned above that there were two reasons why I opted for this approach. Previous to this PR and confirmed by @alopresto and @trkurc, the protocol used by ListenHTTP was automatically negotiated with the client and the configured SSLContextService protocol was ignored. Since the fact that this is misleading and in an effort to not change processor communications behavior, i decided to stop the processor on startup if an invalid protocol was selected and log that the protocol selected wasn't supported with a recommendation to choose another -- this is evident from the screenshot i posted above. As pointed out, this will cause processors to break if they were configured with SSLv3, for example, prior to this change. Additionally, I didn't want to change the global list of selectable protocols in SSLContextService if only one (or a few) processor(s) impacted that list. That's why i attempted to localize the restriction to the one processor.
So instead of breaking the processor if the SSLContextService is configured with a protocol that isn't supported by ListenHTTP, i see 2 options:
1. If the SSLContextService is configured with something that ListenHTTP doesn't support, override the protocol to (possibly configured) TLSv1.2 since that's what it was doing previously and log a warning indicating that this happened.
2. Build another SSLContextService in which a processor can inform which protocols should be selectable.
The second is a bit of work and perhaps outside the scope of this issue, but i'm happy to do whatever is recommended.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1986
@trkurc and @jskora : After working through a few test cases, I have a proposal i'd like your thoughts on.
What if we allow the user to select any SSL protocol available through the UI, but throw an exception with a message explaining why if the processor doesn't support that protocol. In the ListenHTTP case, Jetty has some SSL protocols and ciphers disabled by default that may be available to the JVM. There are two reasons i wouldn't want to tweak ListenHTTP to allow any configured protocol. 1) It changes the processor behavior since those Jetty-disabled protocols wouldn't have worked previously anyway and 2) it possibly opens another can of worms with cipher suite configuration since Jetty has a set of ciphers disabled by default as well.
Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135896956
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/SSLContextServiceUtils.java ---
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.SSLContext;
+
+import org.apache.nifi.components.AllowableValue;
+
+public class SSLContextServiceUtils {
--- End diff --
Mike, I did read the SO link you had posted but I also read [Java 8 Default and Static Methods](https://www.intertech.com/Blog/java-8-tutorial-default-and-static-methods-guide/) and I think as long as the definitions are on the interfaces, it will be ok.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135895369
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/StandardRestrictedSSLContextService.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.nifi.annotation.documentation.CapabilityDescription;
+import org.apache.nifi.annotation.documentation.Tags;
+import org.apache.nifi.components.PropertyDescriptor;
+import org.apache.nifi.components.ValidationContext;
+import org.apache.nifi.processor.util.StandardValidators;
+
+/**
+ * This class is functionally the same as {@link StandardSSLContextService}, but it restricts the allowable
+ * values that can be selected for SSL protocols.
+ */
+@Tags({"ssl", "secure", "certificate", "keystore", "truststore", "jks", "p12", "pkcs12", "pkcs"})
+@CapabilityDescription("Restricted implementation of the SSLContextService. Provides the ability to configure "
+ + "keystore and/or truststore properties once and reuse that configuration throughout the application, "
+ + "but only allows a restricted set of SSL protocols to be chosen. The set of protocols selectable will "
+ + "evolve over time as new protocols emerge and older protocols are deprecated. This service is recommended "
+ + "over StandardSSLContextService if a component doesn't expect to communicate with legacy systems since it's "
+ + "unlikely that legacy systems will support these protocols.")
+public class StandardRestrictedSSLContextService extends StandardSSLContextService implements RestrictedSSLContextService {
+
+ public static final PropertyDescriptor RESTRICTED_SSL_ALGORITHM = new PropertyDescriptor.Builder()
+ .name("SSL Protocol")
+ .defaultValue("TLSv1.2")
--- End diff --
Gotcha. I can make this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1986: NIFI-2528: added support for SSLContextService protocols i...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1986
Ran `contrib-check` and all tests pass. +1, merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1986: NIFI-2528: added support for SSLContextService prot...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1986#discussion_r135545489
--- Diff: nifi-nar-bundles/nifi-standard-services/nifi-ssl-context-bundle/nifi-ssl-context-service/src/main/java/org/apache/nifi/ssl/SSLContextServiceUtils.java ---
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.ssl;
+
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.SSLContext;
+
+import org.apache.nifi.components.AllowableValue;
+
+public class SSLContextServiceUtils {
--- End diff --
@alopresto : I agree and I looked into doing this. However, the problem is that the allowable values for the SSL algorithm PropertyDescriptors are statically initialized with the appropriate values. Adding a method to the `SSLContextService` interface wouldn't enable you to build the values before the class is loaded, which is why i threw a static method into a utility class. This actually reminded me that i forgot to remove the original `buildAllowableValues()` method in `StandardSSLContextService`. I'll do that on my next push.
I can approach this in a different way if you have a recommendation. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---