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:
[email protected]
