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 2022/11/14 09:46:50 UTC
[GitHub] [kafka] patrik-marton opened a new pull request, #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
patrik-marton opened a new pull request, #12846:
URL: https://github.com/apache/kafka/pull/12846
… successful login
- Added the AUTHENTICATION Priority to the JaasBasicAuthFilter. This way if multiple filters are present, they will be processed in the right order. Without this change, the basic auth filter would run after authorization filters.
- Extended the filter to set the Security Context of the request in case of a successful login. The filters sets the user, the authentication type and and the isSecure property indicating whether this request was made using a secure channel, such as HTTPS.
- Extended the JaasBasicAuthFilterTest test with a new test to see if the security context is set correctly
- Previous test are working with the new change
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039440782
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
Review Comment:
Done
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1024994387
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
Review Comment:
Since these are only used inside the constructor, I moved them inside the method where they cannot be static. Using the SecurityContext.BASIC_AUTH would make sense, thanks.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039441717
##########
connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilterTest.java:
##########
@@ -211,6 +214,18 @@ public void testUnsupportedCallback() {
assertThrows(ConnectException.class, () -> callbackHandler.handle(new Callback[] {unsupportedCallback}));
}
+ @Test
+ public void testSecurityContextSet() throws IOException {
+ File credentialFile = setupPropertyLoginFile(true);
+ JaasBasicAuthFilter jaasBasicAuthFilter = setupJaasFilter("KafkaConnect", credentialFile.getPath());
+ ContainerRequestContext requestContext = setMock("Basic", "user1", "password1");
+ jaasBasicAuthFilter.filter(requestContext);
+
+ ArgumentCaptor<SecurityContext> argument = ArgumentCaptor.forClass(SecurityContext.class);
Review Comment:
Changed to `capturedContext`. Thanks!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1024987213
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
Review Comment:
I modified it to avoid null checks where the `BasicAuthenticationCredential` is used. I know the `BasicAuthCallBackHandler` would be fine with null, but setting the user in the security context would require additional null check. ( Theoretically it cannot be null there after the successful login, but I just felt like it is safer to have a default value) What do you think?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039440440
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
Review Comment:
Done
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on PR #12846:
URL: https://github.com/apache/kafka/pull/12846#issuecomment-1330445244
@C0urante can you take a look when you have some time? Thanks!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante merged pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
C0urante merged PR #12846:
URL: https://github.com/apache/kafka/pull/12846
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on PR #12846:
URL: https://github.com/apache/kafka/pull/12846#issuecomment-1315105088
@rhauch @ewencp Could you have a look? Thanks!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039439925
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
Review Comment:
Since these are used only once, I think it is reasonable to make them inline. Done!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039438778
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
Review Comment:
Done. Thanks!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039679491
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
Review Comment:
I personally prefer final fields as that guarantees that the only logic that computes them is in the constructor, but it's not worth blocking on.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039449554
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
Review Comment:
I removed the empty string initialization. Now the username / password will only be initialized inside the constructor if all checks passed, otherwise we make an early return and they will use their implicit default value, which is null.
I think it is more readable than making them final, since in that case we need to explicitly initialize them and there are 4 different return points (so this might not be good for readability) WDYT?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039441425
##########
connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilterTest.java:
##########
@@ -211,6 +214,18 @@ public void testUnsupportedCallback() {
assertThrows(ConnectException.class, () -> callbackHandler.handle(new Callback[] {unsupportedCallback}));
}
+ @Test
+ public void testSecurityContextSet() throws IOException {
+ File credentialFile = setupPropertyLoginFile(true);
+ JaasBasicAuthFilter jaasBasicAuthFilter = setupJaasFilter("KafkaConnect", credentialFile.getPath());
+ ContainerRequestContext requestContext = setMock("Basic", "user1", "password1");
+ jaasBasicAuthFilter.filter(requestContext);
+
+ ArgumentCaptor<SecurityContext> argument = ArgumentCaptor.forClass(SecurityContext.class);
+ verify(requestContext).setSecurityContext(argument.capture());
+ assertEquals("user1", argument.getValue().getUserPrincipal().getName());
Review Comment:
Added coverage for the `isSecure` method.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039439337
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
Review Comment:
All done. Thanks!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039679491
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
Review Comment:
I personally prefer final fields as that guarantees that the only logic that computes them is in the constructor, but it's not worth blocking on and the current changes are definitely good enough to merge.
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -111,41 +119,12 @@ private boolean isInternalRequest(ContainerRequestContext requestContext) {
public static class BasicAuthCallBackHandler implements CallbackHandler {
- private static final String BASIC = "basic";
- private static final char COLON = ':';
- private static final char SPACE = ' ';
private String username;
private String password;
Review Comment:
(Discussed [here](https://github.com/apache/kafka/pull/12846#discussion_r1024519136))
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
gharris1727 commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1024519136
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
Review Comment:
This is different than the previous behavior. Previously, if the authorizationHeader was null, the username/password would be null as well. Now they are empty string.
Is this an intentional change?
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
Review Comment:
nit: these can remain static constants. Maybe we can even pull from SecurityContext.BASIC_AUTH.
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
Review Comment:
nit: I think getScheme() can return null in the case of relative URIs, in which case we cannot determine from the URI whether the request is secure. I'm not sure if this is a realistic situation or a contrived case.
Looking through the rest of these calls, I think everything else is non-null.
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
Review Comment:
Note: The Javadoc indicates that this method should only return a non-null Principal if authentication has already been set. This invariant is preserved when `setSecurityContextForRequest` is called explicitly only after the `LoginContext::login` method successfully exits, which it is.
:+1:
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1034907024
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
+
+ public String getUsername() {
Review Comment:
Nit: we don't use the `get` prefix in this code base
```suggestion
public String username() {
```
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
+
+ public String getUsername() {
+ return username;
+ }
+
+ public String getPassword() {
Review Comment:
Same nit RE getters:
```suggestion
public String password() {
```
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
Review Comment:
While we're in the neighborhood, WDYT about removing these declarations and using `':'` and `' '` inline instead? Seems like it might be more readable; if you feel the same, we can clean things up, and if not, fine to leave as-is.
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
Review Comment:
Few nits:
- We can downgrade the visibility of this class
- I've never seen `credential` (singular) used to refer to a username/password combination
- `Auth` instead of `Authentication` is fine
```suggestion
// Visible for testing
static class BasicAuthCredentials {
```
##########
connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilterTest.java:
##########
@@ -211,6 +214,18 @@ public void testUnsupportedCallback() {
assertThrows(ConnectException.class, () -> callbackHandler.handle(new Callback[] {unsupportedCallback}));
}
+ @Test
+ public void testSecurityContextSet() throws IOException {
+ File credentialFile = setupPropertyLoginFile(true);
+ JaasBasicAuthFilter jaasBasicAuthFilter = setupJaasFilter("KafkaConnect", credentialFile.getPath());
+ ContainerRequestContext requestContext = setMock("Basic", "user1", "password1");
+ jaasBasicAuthFilter.filter(requestContext);
+
+ ArgumentCaptor<SecurityContext> argument = ArgumentCaptor.forClass(SecurityContext.class);
+ verify(requestContext).setSecurityContext(argument.capture());
+ assertEquals("user1", argument.getValue().getUserPrincipal().getName());
Review Comment:
Is it worth adding coverage for the context's `isSecure` method as well?
##########
connect/basic-auth-extension/src/test/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilterTest.java:
##########
@@ -211,6 +214,18 @@ public void testUnsupportedCallback() {
assertThrows(ConnectException.class, () -> callbackHandler.handle(new Callback[] {unsupportedCallback}));
}
+ @Test
+ public void testSecurityContextSet() throws IOException {
+ File credentialFile = setupPropertyLoginFile(true);
+ JaasBasicAuthFilter jaasBasicAuthFilter = setupJaasFilter("KafkaConnect", credentialFile.getPath());
+ ContainerRequestContext requestContext = setMock("Basic", "user1", "password1");
+ jaasBasicAuthFilter.filter(requestContext);
+
+ ArgumentCaptor<SecurityContext> argument = ArgumentCaptor.forClass(SecurityContext.class);
Review Comment:
Nit: `argument` isn't very descriptive; maybe `context`, `securityContext`, or `capturedContext` would be clearer?
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
Review Comment:
IMO null is better here, especially since we use it as a sentinel value [in the `PropertyFileLoginModule` class](https://github.com/apache/kafka/blob/d3ee9341cc894ab14a3ec013c8a590b268438f71/connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/PropertyFileLoginModule.java#L111-L113) to detect invalid credentials.
I think we should make `username` and `password` final, and set them to `null` if the credentials are invalid (i.e., cannot be parsed or use an unsupported auth mechanism)
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
Review Comment:
Nit: can invert the method call here to avoid a null check
```suggestion
return "https".equalsIgnoreCase(scheme);
```
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
Review Comment:
Nit: can use a method reference here
```suggestion
return credential::getUsername;
```
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
Review Comment:
Looks like this is still applicable?
IMO it'd be clearest if we removed the `authType` field altogether, statically imported `SecurityContext.BASIC_AUTH`, and used that inline wherever applicable.
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -111,41 +119,12 @@ private boolean isInternalRequest(ContainerRequestContext requestContext) {
public static class BasicAuthCallBackHandler implements CallbackHandler {
- private static final String BASIC = "basic";
- private static final char COLON = ':';
- private static final char SPACE = ' ';
private String username;
private String password;
Review Comment:
Nit: while we're in the neighborhood, would you mind making these final?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1039438953
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
+
+ public String getUsername() {
Review Comment:
Done. Thanks!
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,85 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ String scheme = requestContext.getUriInfo().getRequestUri().getScheme();
+ return scheme != null && scheme.equalsIgnoreCase("https");
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return BASIC_AUTH;
+ }
+ });
+ }
+
+
+ public static class BasicAuthenticationCredential {
+ private String username;
+ private String password;
+
+ public BasicAuthenticationCredential(String authorizationHeader) {
+ final char colon = ':';
+ final char space = ' ';
+ final String authType = "basic";
+
+ initializeEmptyCredentials();
+
+ if (authorizationHeader == null) {
+ log.trace("No credentials were provided with the request");
+ return;
+ }
+
+ int spaceIndex = authorizationHeader.indexOf(space);
+ if (spaceIndex <= 0) {
+ log.trace("Request credentials were malformed; no space present in value for authorization header");
+ return;
+ }
+
+ String method = authorizationHeader.substring(0, spaceIndex);
+ if (!authType.equalsIgnoreCase(method)) {
+ log.trace("Request credentials used {} authentication, but only {} supported; ignoring", method, authType);
+ return;
+ }
+
+ authorizationHeader = authorizationHeader.substring(spaceIndex + 1);
+ authorizationHeader = new String(Base64.getDecoder().decode(authorizationHeader),
+ StandardCharsets.UTF_8);
+ int i = authorizationHeader.indexOf(colon);
+ if (i <= 0) {
+ log.trace("Request credentials were malformed; no colon present between username and password");
+ return;
+ }
+
+ this.username = authorizationHeader.substring(0, i);
+ this.password = authorizationHeader.substring(i + 1);
+ }
+
+ private void initializeEmptyCredentials() {
+ this.username = "";
+ this.password = "";
+ }
+
+ public String getUsername() {
+ return username;
+ }
+
+ public String getPassword() {
Review Comment:
Done. Thanks!
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrik-marton commented on a diff in pull request #12846: KAFKA-14293: Basic Auth filter should set the SecurityContext after a…
Posted by GitBox <gi...@apache.org>.
patrik-marton commented on code in PR #12846:
URL: https://github.com/apache/kafka/pull/12846#discussion_r1031302727
##########
connect/basic-auth-extension/src/main/java/org/apache/kafka/connect/rest/basic/auth/extension/JaasBasicAuthFilter.java:
##########
@@ -174,4 +153,84 @@ public void handle(Callback[] callbacks) throws UnsupportedCallbackException {
));
}
}
+
+ private void setSecurityContextForRequest(ContainerRequestContext requestContext, BasicAuthenticationCredential credential) {
+ requestContext.setSecurityContext(new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return () -> credential.getUsername();
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return false;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return requestContext.getUriInfo().getRequestUri().getScheme().equalsIgnoreCase("https");
Review Comment:
I added a null check
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org