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/09/02 19:17:21 UTC
[GitHub] [nifi] mark-weghorst opened a new pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
mark-weghorst opened a new pull request #4508:
URL: https://github.com/apache/nifi/pull/4508
#### Description of PR
_Adds basic authentication support to the Confluent schema registry controller; fixes bug #4224_
A pull request (https://github.com/apache/nifi/pull/4224) was originally created by @teeberg to resolve this issue, and @alopresto provided code review feedback on 27-Apr-2020, however no additional commits were provided to address the code review feedback.
This PR updates the code contributed in https://github.com/apache/nifi/pull/4224 to incorporate the code review changes requested in the original PR.
Current versions of the Confluent Schema Registry only support basic authentication, so support for digest authentication which was present in the original PR has been removed.
### 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 `main`)?
- [ ] 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?
- [X] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on 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)?
- [ ] 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 GitHub Actions 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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542040639
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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()
Review comment:
renamed as requested
----------------------------------------------------------------
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] exceptionfactory commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r540181191
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
Review comment:
Recommend renaming this variable to PROP_USERNAME for simplicity and more generalized implementation.
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("Authentication Username")
Review comment:
Recommend shortening the property name and display name to _Username_ since _Authentication_ is already implied.
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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()
Review comment:
Recommend renaming this variable to PROP_PASSWORD.
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("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 will use the 'Authentication Username' " +
+ "and 'Authentication Password' property values. See Confluent Schema Registry documentation for more details.")
Review comment:
Recommend reviewing the recent addition of the dependsOn property support and implementing if needed, as opposed to repeating the other property names in the description.
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
Review comment:
Recommend changing these checks to use StringUtils.isNotEmpty() to include null checking and improve readability.
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("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()
Review comment:
Based on the current implementation of the Confluent Service supporting only HTTP Basic authentication, should this property be removed? Presence of the Username and Password properties could indicate the intention to configure authentication. Alternatively, if the purpose is to use this property as an indicator of the intention to configure authentication, having an alternative value of _NONE_ or _DISABLED_ seems more explicit than having an unset property.
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -166,6 +202,21 @@ public void onEnabled(final ConfigurationContext context) {
}
}
+ final boolean authTypeSet = validationContext.getProperty(PROP_AUTH_TYPE).isSet();
+ final boolean authUsernameSet = validationContext.getProperty(PROP_BASIC_AUTH_USERNAME).isSet();
+ final boolean authPasswordSet = validationContext.getProperty(PROP_BASIC_AUTH_PASSWORD).isSet();
+ if (authTypeSet) {
+ if (!authUsernameSet || !authPasswordSet) {
Review comment:
See comments on the Authentication Type property and recommend reviewing and integrating dependsOn property support if needed, which might remove the need for some of this custom validation.
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("Authentication Password")
Review comment:
Similar to the _Username_ property, recommend renaming to _Password_ since that appears to be the approach used in other Processors and Controller Services.
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
+ if (authType.equals("BASIC")) {
Review comment:
If HTTP Basic is the only type of authentication supported, then recommend removing authType as a parameter and passing just the username and password.
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542042743
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
Review comment:
Changed as requested
----------------------------------------------------------------
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] exceptionfactory commented on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-744524769
@mark-weghorst Thanks for responding to comments and making updates. Regarding the dependsOn properties, the current version of the Developer Guide has some helpful documentation under [Validating Processor Properties.](https://github.com/apache/nifi/blob/d773521ee01f3a34fdd21365a86c8de7b82077a6/nifi-docs/src/main/asciidoc/developer-guide.adoc#validating-processor-properties). Following the example described, the `Username` and `Password` properties described could include a `dependsOn(PROP_AUTHENTICATION_TYPE, "BASIC")` builder method which would make them dependent on selecting `BASIC` as the Authentication Type property.
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r482577651
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -166,6 +202,21 @@ public void onEnabled(final ConfigurationContext context) {
}
}
+ final boolean authTypeSet = validationContext.getProperty(PROP_AUTH_TYPE).isSet();
+ final boolean authUsernameSet = validationContext.getProperty(PROP_BASIC_AUTH_USERNAME).isSet();
+ final boolean authPasswordSet = validationContext.getProperty(PROP_BASIC_AUTH_PASSWORD).isSet();
Review comment:
Good point, as i suggested in the other comment i think I need a decision on whether we want to support null passwords, and then i can rework this for you.
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542039686
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -166,6 +202,21 @@ public void onEnabled(final ConfigurationContext context) {
}
}
+ final boolean authTypeSet = validationContext.getProperty(PROP_AUTH_TYPE).isSet();
+ final boolean authUsernameSet = validationContext.getProperty(PROP_BASIC_AUTH_USERNAME).isSet();
+ final boolean authPasswordSet = validationContext.getProperty(PROP_BASIC_AUTH_PASSWORD).isSet();
Review comment:
In the re-worked validator, I first trimToEmpty() and base my empty determination on the trimmed version.
----------------------------------------------------------------
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] mark-weghorst commented on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-741103015
@pvillard31 yes I'm still planning on re-working this to address the code review comments from @alopresto
Unfortunately, I haven't had the time to revisit this due to some more pressing work, I should have some time towards the end of the week to pick back up on this.
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542040523
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("Authentication Username")
Review comment:
renamed as requested
----------------------------------------------------------------
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] pvillard31 commented on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-740021284
Hi @mark-weghorst and @alopresto - I'm interested by this additional feature.
Mark, do you think you're going to follow up on this (or are you waiting for additional feedback)?
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542040714
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("Authentication Password")
Review comment:
renamed as requested
----------------------------------------------------------------
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] KonstantinCodes edited a comment on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
KonstantinCodes edited a comment on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-751492105
This feature would be absolutely amazing. Thanks everyone for the work so far. Hope to use it soon :)
Currently, even if i specify `username:password` in the "Schema Registry URLs" field, I get this exception:
```bash
Caused by: java.io.IOException: Failed to retrieve Schema with name [redacted] from any of the
Confluent Schema Registry URL's provided; failure response message:
<html><body><h1>401 Unauthorized</h1>You need a valid user and password to access this content.</body></html>
```
<img width="1083" alt="nifi" src="https://user-images.githubusercontent.com/844484/103176472-2a6a1900-4872-11eb-8a28-11eda88b0413.png">
----------------------------------------------------------------
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] exceptionfactory edited a comment on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
exceptionfactory edited a comment on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-744524769
@mark-weghorst Thanks for responding to comments and making updates. Regarding the dependsOn properties, the current version of the Developer Guide has some helpful documentation under [Validating Processor Properties](https://github.com/apache/nifi/blob/d773521ee01f3a34fdd21365a86c8de7b82077a6/nifi-docs/src/main/asciidoc/developer-guide.adoc#validating-processor-properties). Following the example described, the `Username` and `Password` properties described could include a `dependsOn(PROP_AUTHENTICATION_TYPE, "BASIC")` builder method which would make them dependent on selecting `BASIC` as the Authentication Type property.
----------------------------------------------------------------
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 #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r482547597
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("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 will use the 'Authentication Username' " +
+ "and 'Authentication Password' property values. See Confluent Schema Registry documentation for more details.")
+ .required(false)
+ .allowableValues("BASIC")
Review comment:
I don't think setting the allowable value here to a static string is sufficient to enforce that the provided value will always be provided in all uppercase (via API call or manipulated submission), and there is no default value to offer an absence of authentication.
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
+ if (authType.equals("BASIC")) {
+ client.register(HttpAuthenticationFeature.basic(authUser, authPass));
Review comment:
The string comparison should probably not be case sensitive.
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r482575747
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
+ if (authType.equals("BASIC")) {
+ client.register(HttpAuthenticationFeature.basic(authUser, authPass));
Review comment:
You raise an interesting question, AFAIK basic HTTP authentication requires a username, but will work with a password that is an empty string.
A quick search found bugs in other projects where they are failing to support null passwords. **_From my point of view the question is really whether we should allow empty passwords or not._**
In your review of the original PR in April, you suggested that customValidate for the controller be enhanced to throw an error if auth type was basic and either username or password were unset.
I've done as you requested in that orignal PR, and the UX will display a validation error with a meaningful message that both username and password are required when auth type is basic.
Bottom line here is I'll bend to your judgement on this, let me know what you think we should do
----------------------------------------------------------------
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 #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r482541303
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
+ if (authType.equals("BASIC")) {
+ client.register(HttpAuthenticationFeature.basic(authUser, authPass));
Review comment:
Is there any requirement that the password is populated? If the `authType` is `BASIC` but the `authUser` is empty, basic auth won't be enabled but no error/warning will be communicated to the user.
----------------------------------------------------------------
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 #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r482543144
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -166,6 +202,21 @@ public void onEnabled(final ConfigurationContext context) {
}
}
+ final boolean authTypeSet = validationContext.getProperty(PROP_AUTH_TYPE).isSet();
+ final boolean authUsernameSet = validationContext.getProperty(PROP_BASIC_AUTH_USERNAME).isSet();
+ final boolean authPasswordSet = validationContext.getProperty(PROP_BASIC_AUTH_PASSWORD).isSet();
Review comment:
A password value of ` ` (4 space characters) for example would register as `isSet()` here but `StringUtils.trimToEmpty()` on line 165 above would return an empty string.
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542041990
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -166,6 +202,21 @@ public void onEnabled(final ConfigurationContext context) {
}
}
+ final boolean authTypeSet = validationContext.getProperty(PROP_AUTH_TYPE).isSet();
+ final boolean authUsernameSet = validationContext.getProperty(PROP_BASIC_AUTH_USERNAME).isSet();
+ final boolean authPasswordSet = validationContext.getProperty(PROP_BASIC_AUTH_PASSWORD).isSet();
+ if (authTypeSet) {
+ if (!authUsernameSet || !authPasswordSet) {
Review comment:
@pvillard31 can you help with this one?
----------------------------------------------------------------
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] exceptionfactory commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542475418
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
+ if (authType.equals("BASIC")) {
Review comment:
The [Confluent Server documentation](https://docs.confluent.io/platform/current/kafka-rest/production-deployment/confluent-server/security.html) mentions HTTP Basic and mutual TLS, as well as additional methods for SASL-based authentication between components. Maintaining the parameter makes sense given that background.
Rather than relying on a particular string value, however, adding an AuthenticationType enum with the values of BASIC and NONE would help make the implementation clearer.
----------------------------------------------------------------
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 #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
alopresto commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r482676432
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
+ if (authType.equals("BASIC")) {
+ client.register(HttpAuthenticationFeature.basic(authUser, authPass));
Review comment:
Thanks Mark. I don't see anything in [RFC 7617](https://tools.ietf.org/html/rfc7617#section-2) which states that the username or password are actually required to be non-null. I think it depends on what the Confluent Schema Registry enforces in this case. As HTTP Basic Authentication provides no confidentiality of the credentials, we tend not to use it anywhere within NiFi, so when I made the recommendation I probably wasn't considering the use case where both the username and password are expected to be empty if CSR supports it. We also, to my knowledge, are not ensuring that the connection to CSR uses TLS if Basic Auth is enabled, in which case (often reused) credentials could be transmitted in cleartext.
Personally I don't think we should introduce features to NiFi which allow the user to obliviously take actions which are insecure. If the CSR deployment accepts empty usernames/passwords, or accepts basic auth over HTTP, I generally think it's not a feature we should add to the platform, and users who are in scenarios that require this integration should be required to download/write/compile/deploy custom processors which we (the project community) are not responsible for as an explicit acknowledgment of this risk. I accept however, that doesn't always translate to the behaviors demonstrated in the real world.
My suggestion then, is two-fold:
1. Check if the CSR URL is `http://` and if so, warn that basic auth credentials will be exposed (`WARN` level, not `ERROR` level)
1. Check if either the username or password is null/solely whitespace and warn that this is likely incorrect (but allow it if CSR allows it). This has the increased risk of potentially exposing to an observer that the password is empty, but if they are using basic auth with an empty password over plaintext HTTP, I don't know what we can do to help them at that point.
From RFC 7617:
>
> To receive authorization, the client
>
> 1. obtains the user-id and password from the user,
>
> 2. constructs the user-pass by concatenating the user-id, a single
> colon (":") character, and the password,
>
> 3. encodes the user-pass into an octet sequence (see below for a
> discussion of character encoding schemes),
>
> 4. and obtains the basic-credentials by encoding this octet sequence
> using Base64 ([RFC4648], Section 4) into a sequence of US-ASCII
> characters ([RFC0020]).
>
> The original definition of this authentication scheme failed to
> specify the character encoding scheme used to convert the user-pass
> into an octet sequence. In practice, most implementations chose
> either a locale-specific encoding such as ISO-8859-1 ([ISO-8859-1]),
> or UTF-8 ([RFC3629]). For backwards compatibility reasons, this
> specification continues to leave the default encoding undefined, as
> long as it is compatible with US-ASCII (mapping any US-ASCII
> character to a single octet matching the US-ASCII character code).
>
> The user-id and password MUST NOT contain any control characters (see
> "CTL" in Appendix B.1 of [RFC5234]).
>
> Furthermore, a user-id containing a colon character is invalid, as
> the first colon in a user-pass string separates user-id and password
> from one another; text after the first colon is part of the password.
> User-ids containing colons cannot be encoded in user-pass strings.
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542040452
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
Review comment:
Renamed as requested
----------------------------------------------------------------
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] KonstantinCodes commented on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
KonstantinCodes commented on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-751492105
This feature would be absolutely amazing. Thanks everyone for the work so far. Hope to use it soon :)
----------------------------------------------------------------
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] exceptionfactory commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542516691
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("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()
Review comment:
Leaving the property in place and adding an allowable value of _NONE_ sounds like a good approach.
----------------------------------------------------------------
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] pvillard31 commented on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-755163165
We pushed this over the finish line with @exceptionfactory, thanks for all the work on this PR @mark-weghorst !
----------------------------------------------------------------
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] KonstantinCodes edited a comment on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
KonstantinCodes edited a comment on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-751492105
This feature would be absolutely amazing. Thanks everyone for the work so far. Hope to use it soon :)
Currently, even if I specify `username:password` in the "Schema Registry URLs" field, I get this exception:
```bash
Caused by: java.io.IOException: Failed to retrieve Schema with name [redacted] from any of the
Confluent Schema Registry URL's provided; failure response message:
<html><body><h1>401 Unauthorized</h1>You need a valid user and password to access this content.</body></html>
```
<img width="1083" alt="nifi" src="https://user-images.githubusercontent.com/844484/103176472-2a6a1900-4872-11eb-8a28-11eda88b0413.png">
----------------------------------------------------------------
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] pvillard31 commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r548452229
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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_PASSWORD = new PropertyDescriptor.Builder()
+ .name("Authentication Password")
+ .displayName("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 will use the 'Authentication Username' " +
+ "and 'Authentication Password' property values. See Confluent Schema Registry documentation for more details.")
+ .required(false)
+ .allowableValues("BASIC")
Review comment:
```suggestion
.allowableValues("BASIC", "NONE")
.defaultValue("NONE")
```
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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_PASSWORD = new PropertyDescriptor.Builder()
+ .name("Authentication Password")
+ .displayName("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]+$")))
Review comment:
```suggestion
.addValidator(StandardValidators.createRegexMatchingValidator(Pattern.compile("^[\\x20-\\x7e\\x80-\\xff]+$")))
.dependsOn(PROP_AUTH_TYPE, "BASIC")
```
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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]+$")))
Review comment:
```suggestion
.addValidator(StandardValidators.createRegexMatchingValidator(Pattern.compile("^[\\x20-\\x39\\x3b-\\x7e\\x80-\\xff]+$")))
.dependsOn(PROP_AUTH_TYPE, "BASIC")
```
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542039470
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
+ if (authType.equals("BASIC")) {
+ client.register(HttpAuthenticationFeature.basic(authUser, authPass));
Review comment:
I went ahead and re-worked the validator to generate warnings when using schema registry without TLS encyption.
As for the null password, while it is in fact RFC compliant the Confluent Schema registry does not list this as supported in their documentation.
100% of the examples they give for configuration include a username/passsword combination. So I've kept the requirement that both the username and password be set.
----------------------------------------------------------------
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] exceptionfactory commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542479942
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("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 will use the 'Authentication Username' " +
+ "and 'Authentication Password' property values. See Confluent Schema Registry documentation for more details.")
+ .required(false)
+ .allowableValues("BASIC")
Review comment:
The [Confluent Server documentation](https://docs.confluent.io/platform/current/kafka-rest/production-deployment/confluent-server/security.html#http-basic-authentication) lists BASIC and NONE as options for the authentication method for certain clients. Following that approach and having a default value of NONE, in combination with an AuthenticationType enum, would help make the configuration properties and implementation approach clearer.
----------------------------------------------------------------
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 #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4508:
URL: https://github.com/apache/nifi/pull/4508
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542041906
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("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 will use the 'Authentication Username' " +
+ "and 'Authentication Password' property values. See Confluent Schema Registry documentation for more details.")
Review comment:
@pvillard31 can you help out with this one?
----------------------------------------------------------------
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] mark-weghorst commented on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-756264341
Thank you @exceptionfactory for your help getting this pushed over the finishing line over the holidays!
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542042224
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
- public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+ public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+ final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
this.baseUrls = new ArrayList<>(baseUrls);
final ClientConfig clientConfig = new ClientConfig();
clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
client = WebUtils.createClient(clientConfig, sslContext);
+ if (!authUser.isEmpty() && !authType.isEmpty()) {
+ if (authType.equals("BASIC")) {
Review comment:
See comments above about additional auth type forthcoming
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542040268
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("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 will use the 'Authentication Username' " +
+ "and 'Authentication Password' property values. See Confluent Schema Registry documentation for more details.")
+ .required(false)
+ .allowableValues("BASIC")
Review comment:
Your point is well taken about API generated configurations bypassing the UI validation. All comparisons will this value are now case insensitive. I don't think we can really set a default value here, as the default is the absence of value (unset) unless .default("") == unset?
----------------------------------------------------------------
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] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r542041874
##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
##########
@@ -112,6 +114,32 @@
.required(true)
.build();
+ static final PropertyDescriptor PROP_BASIC_AUTH_USERNAME = new PropertyDescriptor.Builder()
+ .name("Authentication Username")
+ .displayName("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("Authentication Password")
+ .displayName("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()
Review comment:
I'd like to leave this as is, since I think additional authentication types will be forthcoming in the future.
----------------------------------------------------------------
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] pvillard31 commented on pull request #4508: NIFI-6576 add basic auth to confluent schema registry service
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#issuecomment-741915423
awesome, let me know if I can be of any help!
----------------------------------------------------------------
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