You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/04/22 15:33:11 UTC

[GitHub] [nifi] teeberg opened a new pull request #4224: NIFI-6576 Add HTTP authentication to confluent schema registry controller

teeberg opened a new pull request #4224:
URL: https://github.com/apache/nifi/pull/4224


   #### Description of PR
   Allow schema registry controller to connect to schema registries requiring Basic/Digest HTTP Auth.
   
   Fixes NIFI-6576
   
   I'm still having trouble running the tests locally and will keep trying.
   
   ### For all changes:
   - [x] Is there a JIRA ticket associated with this PR? Is it referenced 
        in the commit message?
   
   - [x] 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.
   
   - [x] Has your PR been rebased against the latest commit within the target branch (typically `master`)?
   
   - [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
   
   ### 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?
   - [ ] Have you verified that the full build is successful on both JDK 8 and JDK 11?
   - [ ] 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)? 
   - [x] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
   - [x] 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.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] MikeThomsen commented on a change in pull request #4224: NIFI-6576 Add HTTP authentication to confluent schema registry controller

Posted by GitBox <gi...@apache.org>.
MikeThomsen commented on a change in pull request #4224:
URL: https://github.com/apache/nifi/pull/4224#discussion_r453338197



##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -52,6 +52,8 @@
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import java.util.regex.Pattern;
+import static org.apache.commons.lang3.StringUtils.trimToEmpty;

Review comment:
       I don't think static imports have ever been blocked by checkstyle. Is that something we need to verify as behavior?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] alopresto commented on a change in pull request #4224: NIFI-6576 Add HTTP authentication to confluent schema registry controller

Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4224:
URL: https://github.com/apache/nifi/pull/4224#discussion_r416273764



##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -52,6 +52,8 @@
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import java.util.regex.Pattern;
+import static org.apache.commons.lang3.StringUtils.trimToEmpty;

Review comment:
       I believe our coding guidelines and checkstyle discourage static imports. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] alopresto commented on a change in pull request #4224: NIFI-6576 Add HTTP authentication to confluent schema registry controller

Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4224:
URL: https://github.com/apache/nifi/pull/4224#discussion_r416272053



##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -113,6 +115,32 @@
         .required(true)
         .build();
 
+    static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+        .name("Basic Authentication Username")
+        .displayName("Basic Authentication Username")
+        .description("The username to be used by the client to authenticate against the Remote URL.  Cannot include control characters (0-31), ':', or DEL (127).")
+        .required(false)
+        .addValidator(StandardValidators.createRegexMatchingValidator(Pattern.compile("^[\\x20-\\x39\\x3b-\\x7e\\x80-\\xff]+$")))
+        .build();
+
+    static final PropertyDescriptor PROP_BASIC_AUTH_PASSWORD = new PropertyDescriptor.Builder()
+        .name("Basic Authentication Password")
+        .displayName("Basic Authentication Password")
+        .description("The password to be used by the client to authenticate against the Remote URL.")
+        .required(false)
+        .sensitive(true)
+        .addValidator(StandardValidators.createRegexMatchingValidator(Pattern.compile("^[\\x20-\\x7e\\x80-\\xff]+$")))
+        .build();
+
+    static final PropertyDescriptor PROP_AUTH_TYPE = new PropertyDescriptor.Builder()
+        .name("Authentication Type")
+        .displayName("Authentication Type")
+        .description("'Basic Authentication Username' and 'Basic Authentication Password' are used if Digest Authentication is used"
+                + "for authentication.")

Review comment:
       While people familiar with [RFC 2617](https://tools.ietf.org/html/rfc2617) will understand that _basic authentication_ and _digest authentication_ both use a username and password as input to an authentication process with the server, the standalone message `'Basic Authentication Username' and 'Basic Authentication Password' are used if Digest Authentication is used for authentication.` can reasonably be expected to confuse a normal user. I would recommend changing the username and password property descriptors to remove the "basic" terminology and change the description for this PD to read `Both basic authentication and digest authentication will use the 'Authentication Username' and 'Authentication Password' property values. See Confluent Schema Registry documentation for more details.` The link to said documentation is [https://docs.confluent.io/current/schema-registry/security/index.html#configuring-the-rest-api-for-basic-http-authentication](https://docs.confluent.io/current/schema-registry/security/index.html#configuring-the-rest-api-for-basic-http-authentication) but does not need to be in the property descriptor description, but can be in the processor documentation. I actually don't see any details on that page about configuring digest authentication at all -- do you have a reference for Confluent Schema Registry supporting this authentication mechanism?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] alopresto commented on a change in pull request #4224: NIFI-6576 Add HTTP authentication to confluent schema registry controller

Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4224:
URL: https://github.com/apache/nifi/pull/4224#discussion_r416264801



##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -113,6 +115,32 @@
         .required(true)
         .build();
 
+    static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+        .name("Basic Authentication Username")
+        .displayName("Basic Authentication Username")
+        .description("The username to be used by the client to authenticate against the Remote URL.  Cannot include control characters (0-31), ':', or DEL (127).")
+        .required(false)
+        .addValidator(StandardValidators.createRegexMatchingValidator(Pattern.compile("^[\\x20-\\x39\\x3b-\\x7e\\x80-\\xff]+$")))
+        .build();
+
+    static final PropertyDescriptor PROP_BASIC_AUTH_PASSWORD = new PropertyDescriptor.Builder()
+        .name("Basic Authentication Password")
+        .displayName("Basic Authentication Password")
+        .description("The password to be used by the client to authenticate against the Remote URL.")
+        .required(false)
+        .sensitive(true)
+        .addValidator(StandardValidators.createRegexMatchingValidator(Pattern.compile("^[\\x20-\\x7e\\x80-\\xff]+$")))
+        .build();
+
+    static final PropertyDescriptor PROP_AUTH_TYPE = new PropertyDescriptor.Builder()
+        .name("Authentication Type")
+        .displayName("Authentication Type")
+        .description("'Basic Authentication Username' and 'Basic Authentication Password' are used if Digest Authentication is used"
+                + "for authentication.")

Review comment:
       There is a missing space between "usedfor" based on the String concatenation. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] alopresto commented on a change in pull request #4224: NIFI-6576 Add HTTP authentication to confluent schema registry controller

Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4224:
URL: https://github.com/apache/nifi/pull/4224#discussion_r416274639



##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -113,6 +115,32 @@
         .required(true)
         .build();
 
+    static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+        .name("Basic Authentication Username")
+        .displayName("Basic Authentication Username")
+        .description("The username to be used by the client to authenticate against the Remote URL.  Cannot include control characters (0-31), ':', or DEL (127).")
+        .required(false)
+        .addValidator(StandardValidators.createRegexMatchingValidator(Pattern.compile("^[\\x20-\\x39\\x3b-\\x7e\\x80-\\xff]+$")))
+        .build();
+
+    static final PropertyDescriptor PROP_BASIC_AUTH_PASSWORD = new PropertyDescriptor.Builder()
+        .name("Basic Authentication Password")
+        .displayName("Basic Authentication Password")
+        .description("The password to be used by the client to authenticate against the Remote URL.")
+        .required(false)
+        .sensitive(true)
+        .addValidator(StandardValidators.createRegexMatchingValidator(Pattern.compile("^[\\x20-\\x7e\\x80-\\xff]+$")))
+        .build();
+
+    static final PropertyDescriptor PROP_AUTH_TYPE = new PropertyDescriptor.Builder()
+        .name("Authentication Type")
+        .displayName("Authentication Type")
+        .description("'Basic Authentication Username' and 'Basic Authentication Password' are used if Digest Authentication is used"
+                + "for authentication.")
+        .required(false)
+        .allowableValues("DIGEST", "BASIC")
+        .build();

Review comment:
       We should probably add logic in the `#customValidate()` method to ensure that if the username and password are present, the auth type property is set and valid as well. Performing the validation earlier improves the user experience and helps with debugging. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] caldiver07 commented on pull request #4224: NIFI-6576 Add HTTP authentication to confluent schema registry controller

Posted by GitBox <gi...@apache.org>.
caldiver07 commented on pull request #4224:
URL: https://github.com/apache/nifi/pull/4224#issuecomment-657164804


   Just wanted to check up on this PR.  We are using confluent and would like to use nifi, but looks like authentication is required.  Is this PR ready to be merged.  Looks like someone has tested and appears to be working?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [nifi] asfgit closed pull request #4224: NIFI-6576 Add HTTP authentication to confluent schema registry controller

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4224:
URL: https://github.com/apache/nifi/pull/4224


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org