Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-3193: added ability to authenticate with AMQP using cert common names #1971

Closed
wants to merge 7 commits into from
Closed

Conversation

m-hogue
Copy link
Member

@m-hogue m-hogue commented Jul 3, 2017

This change allows for 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?
  • 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.

@m-hogue m-hogue changed the title NIFI-3193: added ability to authenticate using cert common names NIFI-3193: added ability to authenticate with AMQP using cert common names Jul 3, 2017
Copy link
Contributor

@pvillard31 pvillard31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @m-hogue, it LGTM, left few comments.

@@ -16,14 +16,14 @@
*/
package org.apache.nifi.amqp.processors;

import java.io.IOException;

import org.apache.nifi.logging.ComponentLog;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'll make sure to avoid this in the future as well

@@ -97,6 +98,14 @@
.required(false)
.identifiesControllerService(SSLContextService.class)
.build();
public static final PropertyDescriptor USE_CERT_AUTHENTICATION = new PropertyDescriptor.Builder()
.name("Cert Authentication")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, no problem!

@m-hogue
Copy link
Member Author

m-hogue commented Jul 5, 2017

@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.

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to have this match the displayName text?

@trkurc
Copy link
Contributor

trkurc commented Jul 5, 2017

@m-hogue if you can fix that description in the html, I can merge in.

@m-hogue
Copy link
Member Author

m-hogue commented Jul 5, 2017

Fixed in 02956f8.

Should be gtg. Let me know if there are any other changes needed.

@trkurc
Copy link
Contributor

trkurc commented Jul 5, 2017

looks good, I'll work on merging it in

@asfgit asfgit closed this in 47eece5 Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants