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/03 16:38:45 UTC
[GitHub] nifi pull request #1971: NIFI-3193: added ability to authenticate using cert...
GitHub user m-hogue opened a pull request:
https://github.com/apache/nifi/pull/1971
NIFI-3193: added ability to authenticate using cert common names
This change allows for the PublishAMQP and ConsumeAMQP to use SSL cert authentication instead of username/password. The change is backward compatible, so it shouldn't affect anyone who plans to continue using SSL as a transport _and_ username/password authentication.
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-3193
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1971.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 #1971
----
commit cd66c877b706b1f5b3418301e7e3aed651036bce
Author: m-hogue <ho...@gmail.com>
Date: 2017-06-27T20:19:10Z
NIFI-3193: added ability to authenticate using cert common names
----
---
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 #1971: NIFI-3193: added ability to authenticate with AMQP ...
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/1971#discussion_r125505285
--- Diff: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AMQPPublisher.java ---
@@ -16,14 +16,14 @@
*/
package org.apache.nifi.amqp.processors;
-import java.io.IOException;
-
-import org.apache.nifi.logging.ComponentLog;
--- End diff --
Yeah, I'll make sure to avoid this in the future 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 #1971: NIFI-3193: added ability to authenticate with AMQP ...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1971#discussion_r125421301
--- Diff: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AMQPPublisher.java ---
@@ -16,14 +16,14 @@
*/
package org.apache.nifi.amqp.processors;
-import java.io.IOException;
-
-import org.apache.nifi.logging.ComponentLog;
--- End diff --
Hey @m-hogue, thanks for this PR. Can you take care of the not needed changes that have been (most likely) introduced by your IDE? For example this class is marked as changed but there is no actual change in it. It's similar for some files modified by your PR. 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.
---
[GitHub] nifi pull request #1971: NIFI-3193: added ability to authenticate with AMQP ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/1971
---
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 #1971: NIFI-3193: added ability to authenticate with AMQP using c...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1971
@m-hogue if you can fix that description in the html, I can merge in.
---
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 #1971: NIFI-3193: added ability to authenticate with AMQP using c...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1971
Fixed in cd66c877b706b1f5b3418301e7e3aed651036bce.
Should be gtg. Let me know if there are any other changes needed.
---
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 #1971: NIFI-3193: added ability to authenticate with AMQP ...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1971#discussion_r125421693
--- Diff: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java ---
@@ -97,6 +98,14 @@
.required(false)
.identifiesControllerService(SSLContextService.class)
.build();
+ public static final PropertyDescriptor USE_CERT_AUTHENTICATION = new PropertyDescriptor.Builder()
+ .name("Cert Authentication")
--- End diff --
Could you use ``.displayName()`` for what is displayed on the UI? and ``.name`` for a more "computer-friendly" name? It's not mandatory but that's a convention we are trying to progressively adopt.
---
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 #1971: NIFI-3193: added ability to authenticate with AMQP using c...
Posted by m-hogue <gi...@git.apache.org>.
Github user m-hogue commented on the issue:
https://github.com/apache/nifi/pull/1971
@pvillard31 : Thanks for the review! After wrestling with IntelliJ formatting rules following the import of the nifi style config i think it's good. Please let me know if you'd like any more changes 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 #1971: NIFI-3193: added ability to authenticate with AMQP ...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1971#discussion_r125669424
--- Diff: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/resources/docs/org.apache.nifi.amqp.processors.ConsumeAMQP/additionalDetails.html ---
@@ -63,6 +63,9 @@
<li><b>Password</b> - [REQUIRED] password to use with user name to connect to AMQP broker.
Usually provided by the administrator. Defaults to 'guest'.
</li>
+ <li><b>Cert Authentication</b> - [OPTIONAL] whether or not to use the SSL certificate for authentication rather than user name and password.
--- End diff --
Would it be possible to have this match the displayName text?
---
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 #1971: NIFI-3193: added ability to authenticate with AMQP using c...
Posted by trkurc <gi...@git.apache.org>.
Github user trkurc commented on the issue:
https://github.com/apache/nifi/pull/1971
looks good, I'll work on merging it in
---
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 #1971: NIFI-3193: added ability to authenticate with AMQP ...
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/1971#discussion_r125505175
--- Diff: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java ---
@@ -97,6 +98,14 @@
.required(false)
.identifiesControllerService(SSLContextService.class)
.build();
+ public static final PropertyDescriptor USE_CERT_AUTHENTICATION = new PropertyDescriptor.Builder()
+ .name("Cert Authentication")
--- End diff --
Yes, no problem!
---
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.
---