You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/05/07 23:45:53 UTC
[GitHub] [kafka] rhauch commented on a change in pull request #8357: KAFKA-9767: Add logging to basic auth rest extension
rhauch commented on a change in pull request #8357:
URL: https://github.com/apache/kafka/pull/8357#discussion_r421855204
##########
File path: connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java
##########
@@ -67,36 +84,61 @@ public void filter(ContainerRequestContext requestContext) throws IOException {
private String password;
public BasicAuthCallBackHandler(String credentials) {
- if (credentials != null) {
- int space = credentials.indexOf(SPACE);
- if (space > 0) {
- String method = credentials.substring(0, space);
- if (BASIC.equalsIgnoreCase(method)) {
- credentials = credentials.substring(space + 1);
- credentials = new String(Base64.getDecoder().decode(credentials),
- StandardCharsets.UTF_8);
- int i = credentials.indexOf(COLON);
- if (i > 0) {
- username = credentials.substring(0, i);
- password = credentials.substring(i + 1);
- }
- }
- }
+ if (credentials == null) {
+ log.trace("No credentials were provided with the request");
+ return;
}
+
+ int space = credentials.indexOf(SPACE);
+ if (space <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = credentials.substring(0, space);
+ if (!BASIC.equalsIgnoreCase(method)) {
+ log.trace("Request credentials did not use basic authentication; ignoring");
+ return;
+ }
+
+ credentials = credentials.substring(space + 1);
+ credentials = new String(Base64.getDecoder().decode(credentials),
+ StandardCharsets.UTF_8);
+ int i = credentials.indexOf(COLON);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ username = credentials.substring(0, i);
+ password = credentials.substring(i + 1);
}
@Override
public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
+ Callback unsupportedCallback = null;
for (Callback callback : callbacks) {
if (callback instanceof NameCallback) {
((NameCallback) callback).setName(username);
} else if (callback instanceof PasswordCallback) {
((PasswordCallback) callback).setPassword(password.toCharArray());
} else {
- throw new UnsupportedCallbackException(callback, "Supports only NameCallback "
- + "and PasswordCallback");
+ // Log at WARN level here as this indicates incompatibility between the Connect basic auth
+ // extension and the JAAS login module that the user has configured and it is likely that the
+ // worker will need to be reconfigured and restarted
+ log.warn(
+ "Asked to handle unsupported callback '{}' of type {}; request authentication will fail",
+ callback,
+ callback.getClass()
+ );
+ if (unsupportedCallback == null)
+ unsupportedCallback = callback;
}
}
+ if (unsupportedCallback != null)
+ throw new UnsupportedCallbackException(
+ unsupportedCallback,
+ "Supports only NameCallback and PasswordCallback");
Review comment:
Here we're only reporting the last of potentially multiple unsupported callbacks, right? Should we be generating a list of unsupported callbacks here if we're waiting to throw the exception?
##########
File path: connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java
##########
@@ -67,36 +84,61 @@ public void filter(ContainerRequestContext requestContext) throws IOException {
private String password;
public BasicAuthCallBackHandler(String credentials) {
- if (credentials != null) {
- int space = credentials.indexOf(SPACE);
- if (space > 0) {
- String method = credentials.substring(0, space);
- if (BASIC.equalsIgnoreCase(method)) {
- credentials = credentials.substring(space + 1);
- credentials = new String(Base64.getDecoder().decode(credentials),
- StandardCharsets.UTF_8);
- int i = credentials.indexOf(COLON);
- if (i > 0) {
- username = credentials.substring(0, i);
- password = credentials.substring(i + 1);
- }
- }
- }
+ if (credentials == null) {
+ log.trace("No credentials were provided with the request");
+ return;
}
+
+ int space = credentials.indexOf(SPACE);
+ if (space <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = credentials.substring(0, space);
+ if (!BASIC.equalsIgnoreCase(method)) {
+ log.trace("Request credentials did not use basic authentication; ignoring");
Review comment:
Would it help to actually list the method that was used, in case somebody thought they were using basic?
```suggestion
log.trace("Request credentials used {} authentication, but only {} supported; ignoring", BASIC, method);
```
##########
File path: connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java
##########
@@ -67,36 +84,61 @@ public void filter(ContainerRequestContext requestContext) throws IOException {
private String password;
public BasicAuthCallBackHandler(String credentials) {
- if (credentials != null) {
- int space = credentials.indexOf(SPACE);
- if (space > 0) {
- String method = credentials.substring(0, space);
- if (BASIC.equalsIgnoreCase(method)) {
- credentials = credentials.substring(space + 1);
- credentials = new String(Base64.getDecoder().decode(credentials),
- StandardCharsets.UTF_8);
- int i = credentials.indexOf(COLON);
- if (i > 0) {
- username = credentials.substring(0, i);
- password = credentials.substring(i + 1);
- }
- }
- }
+ if (credentials == null) {
+ log.trace("No credentials were provided with the request");
+ return;
}
+
+ int space = credentials.indexOf(SPACE);
+ if (space <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = credentials.substring(0, space);
+ if (!BASIC.equalsIgnoreCase(method)) {
+ log.trace("Request credentials did not use basic authentication; ignoring");
+ return;
+ }
+
+ credentials = credentials.substring(space + 1);
+ credentials = new String(Base64.getDecoder().decode(credentials),
+ StandardCharsets.UTF_8);
+ int i = credentials.indexOf(COLON);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ username = credentials.substring(0, i);
+ password = credentials.substring(i + 1);
}
@Override
public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
+ Callback unsupportedCallback = null;
for (Callback callback : callbacks) {
if (callback instanceof NameCallback) {
((NameCallback) callback).setName(username);
} else if (callback instanceof PasswordCallback) {
((PasswordCallback) callback).setPassword(password.toCharArray());
} else {
- throw new UnsupportedCallbackException(callback, "Supports only NameCallback "
- + "and PasswordCallback");
+ // Log at WARN level here as this indicates incompatibility between the Connect basic auth
+ // extension and the JAAS login module that the user has configured and it is likely that the
+ // worker will need to be reconfigured and restarted
+ log.warn(
+ "Asked to handle unsupported callback '{}' of type {}; request authentication will fail",
Review comment:
The comment above says:
> and it is likely that the worker will need to be reconfigured and restarted
Should the log message say this? Something more like:
> Unsupported callback '{}' of type {}; request authentication will fail. This indicates the Connect worker was configured incorrectly, and will need to be corrected and restarted.
The more specific we could be about what might have been misconfigured, the better.
##########
File path: connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/PropertyFileLoginModule.java
##########
@@ -62,35 +62,63 @@ public void initialize(Subject subject, CallbackHandler callbackHandler, Map<Str
if (fileName == null || fileName.trim().isEmpty()) {
throw new ConfigException("Property Credentials file must be specified");
}
+
if (!credentialPropertiesMap.containsKey(fileName)) {
+ log.trace("Opening credential properties file '{}'", fileName);
Properties credentialProperties = new Properties();
try {
try (InputStream inputStream = Files.newInputStream(Paths.get(fileName))) {
+ log.trace("Parsing credential properties file '{}'", fileName);
credentialProperties.load(inputStream);
}
credentialPropertiesMap.putIfAbsent(fileName, credentialProperties);
+ if (credentialProperties.isEmpty())
+ log.warn("Credential properties file '{}' is empty; all requests will be permitted",
+ fileName);
} catch (IOException e) {
log.error("Error loading credentials file ", e);
throw new ConfigException("Error loading Property Credentials file");
}
+ } else {
+ log.trace(
+ "Credential properties file '{}' has already been opened and parsed; will read from cached, in-memory store",
+ fileName);
}
}
@Override
public boolean login() throws LoginException {
Callback[] callbacks = configureCallbacks();
try {
+ log.trace("Authenticating user; invoking JAAS login callbacks");
callbackHandler.handle(callbacks);
} catch (Exception e) {
+ log.warn("Authentication failed while invoking JAAS login callbacks");
Review comment:
Doesn't this catch handle all exceptions, including authentication failures? Do we really want to log all of those auth failures at WARN?
----------------------------------------------------------------
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