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 14:56:39 UTC
[GitHub] nifi pull request #1985: NIFI-2529: added support for SSLContextService prot...
GitHub user m-hogue opened a pull request:
https://github.com/apache/nifi/pull/1985
NIFI-2529: added support for SSLContextService protocols in HandleHtt…
…pRequest
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-2529
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1985.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 #1985
----
commit 130ccbaa02b443eba8684032e76c23173ed61c5e
Author: m-hogue <ho...@gmail.com>
Date: 2017-07-06T14:55:10Z
NIFI-2529: added support for SSLContextService protocols in HandleHttpRequest
----
---
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 #1985: NIFI-2529: added support for SSLContextService prot...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1985#discussion_r127328861
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -90,10 +90,11 @@
@WritesAttribute(attribute = "http.method", description = "The HTTP Method that was used for the request, such as GET or POST"),
@WritesAttribute(attribute = HTTPUtils.HTTP_LOCAL_NAME, description = "IP address/hostname of the server"),
@WritesAttribute(attribute = HTTPUtils.HTTP_PORT, description = "Listening port of the server"),
- @WritesAttribute(attribute = "http.query.string", description = "The query string portion of hte Request URL"),
+ @WritesAttribute(attribute = "http.query.string", description = "The query string portion of the Request URL"),
@WritesAttribute(attribute = HTTPUtils.HTTP_REMOTE_HOST, description = "The hostname of the requestor"),
@WritesAttribute(attribute = "http.remote.addr", description = "The hostname:port combination of the requestor"),
@WritesAttribute(attribute = "http.remote.user", description = "The username of the requestor"),
+ @WritesAttribute(attribute = "http.protocol", description = "The protocol used to communicate"),
--- End diff --
how did you decide on this attribute name? was it used in another processor? to me, it was a bit confusing, as I'd have thought maybe HTTP/1.1 or HTTP/2 would be the http protocol. would http.security.protocol make more sense? @alopresto may be able to weigh in on a preferred attribute name for 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 issue #1985: NIFI-2529: 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/1985
I have similar concerns with this as with [PR 1986](https://github.com/apache/nifi/pull/1986) about throwing exceptions on 71% of the available options in the protocol version dropdown.
---
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 #1985: NIFI-2529: added support for SSLContextService prot...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1985#discussion_r129051538
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -446,6 +447,7 @@ private SslContextFactory createSslFactory(final SSLContextService sslService, f
sslFactory.setNeedClientAuth(needClientAuth);
sslFactory.setWantClientAuth(wantClientAuth);
+ sslFactory.setProtocol(sslService.getSslAlgorithm());
--- End diff --
Approach from #1986 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 #1985: NIFI-2529: 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/1985
@trkurc : pushed same SSL protocol checks as added in #1985. 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 pull request #1985: NIFI-2529: 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/1985#discussion_r127465171
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -446,6 +447,7 @@ private SslContextFactory createSslFactory(final SSLContextService sslService, f
sslFactory.setNeedClientAuth(needClientAuth);
sslFactory.setWantClientAuth(wantClientAuth);
+ sslFactory.setProtocol(sslService.getSslAlgorithm());
--- End diff --
Yep, the exact same problem. Once resolved on https://github.com/apache/nifi/pull/1986, i'll do the same here.
---
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 #1985: NIFI-2529: 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/1985
---
[GitHub] nifi issue #1985: NIFI-2529: 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/1985
@m-hogue I apologize for the delay on this. I reviewed and verified that the tests pass, the checkstyle was good, and a flow I developed using `HandleHTTPRequest` and `HandleHTTPResponse` worked as expected (succeeded with TLS v1.2 incoming connection; failed when TLSv1.1 was specified). I merged and closed the Jira. Expect this in Apache NiFi 1.4.0. Thanks for your contribution.
---
[GitHub] nifi issue #1985: NIFI-2529: 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/1985
No worries, @alopresto. Thanks for the review!
---
[GitHub] nifi pull request #1985: NIFI-2529: added support for SSLContextService prot...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1985#discussion_r127329325
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -446,6 +447,7 @@ private SslContextFactory createSslFactory(final SSLContextService sslService, f
sslFactory.setNeedClientAuth(needClientAuth);
sslFactory.setWantClientAuth(wantClientAuth);
+ sslFactory.setProtocol(sslService.getSslAlgorithm());
--- End diff --
I didn't run this one yet, but does it have the same issue as the ListenHTTP with starting up successfullly, but not supporting the ssl/tls choice when a request is made?
---
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 #1985: NIFI-2529: 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/1985#discussion_r127464912
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -90,10 +90,11 @@
@WritesAttribute(attribute = "http.method", description = "The HTTP Method that was used for the request, such as GET or POST"),
@WritesAttribute(attribute = HTTPUtils.HTTP_LOCAL_NAME, description = "IP address/hostname of the server"),
@WritesAttribute(attribute = HTTPUtils.HTTP_PORT, description = "Listening port of the server"),
- @WritesAttribute(attribute = "http.query.string", description = "The query string portion of hte Request URL"),
+ @WritesAttribute(attribute = "http.query.string", description = "The query string portion of the Request URL"),
@WritesAttribute(attribute = HTTPUtils.HTTP_REMOTE_HOST, description = "The hostname of the requestor"),
@WritesAttribute(attribute = "http.remote.addr", description = "The hostname:port combination of the requestor"),
@WritesAttribute(attribute = "http.remote.user", description = "The username of the requestor"),
+ @WritesAttribute(attribute = "http.protocol", description = "The protocol used to communicate"),
--- End diff --
This actually is the `http.protocol`, so it'd be HTTP/1.1 or HTTP/2 as you noted. While potentially worthwhile, i'd admittedly added this while searching for a way to validate the SSL protocol negotiated by the client & server. I can drop the property if it's not wanted.
---
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 #1985: NIFI-2529: added support for SSLContextService prot...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1985#discussion_r127535464
--- Diff: nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/HandleHttpRequest.java ---
@@ -90,10 +90,11 @@
@WritesAttribute(attribute = "http.method", description = "The HTTP Method that was used for the request, such as GET or POST"),
@WritesAttribute(attribute = HTTPUtils.HTTP_LOCAL_NAME, description = "IP address/hostname of the server"),
@WritesAttribute(attribute = HTTPUtils.HTTP_PORT, description = "Listening port of the server"),
- @WritesAttribute(attribute = "http.query.string", description = "The query string portion of hte Request URL"),
+ @WritesAttribute(attribute = "http.query.string", description = "The query string portion of the Request URL"),
@WritesAttribute(attribute = HTTPUtils.HTTP_REMOTE_HOST, description = "The hostname of the requestor"),
@WritesAttribute(attribute = "http.remote.addr", description = "The hostname:port combination of the requestor"),
@WritesAttribute(attribute = "http.remote.user", description = "The username of the requestor"),
+ @WritesAttribute(attribute = "http.protocol", description = "The protocol used to communicate"),
--- End diff --
Ah, I missed this. Carry on.
---
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 #1985: NIFI-2529: 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/1985
@trkurc @alopresto : I've made changes here in line with #1986 and forced the use of `RestrictedSSLContextService` here as well. This is ready for review.
---
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 #1985: NIFI-2529: 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/1985
adding this to my review pile for the night
---
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.
---