[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14044: [broker][authentication]Support pass http auth status

[GitBox <gi...@apache.org>]

michaeljmarshall commented on a change in pull request #14044:
URL: https://github.com/apache/pulsar/pull/14044#discussion_r798237924



##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java
##########
@@ -76,8 +77,15 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
                 // not sasl type, return role directly.
                 String role = authenticationService.authenticateHttpRequest((HttpServletRequest) request);
                 request.setAttribute(AuthenticatedRoleAttributeName, role);
-                request.setAttribute(AuthenticatedDataAttributeName,
-                    new AuthenticationDataHttps((HttpServletRequest) request));
+                String authMethodName = httpRequest.getHeader("X-Pulsar-Auth-Method-Name");

Review comment:
       Once this variable is defined in the `AuthenticationDataProvider`, we should use the static variable here instead of `"X-Pulsar-Auth-Method-Name"`.

##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationDataOAuth2.java
##########
@@ -44,7 +43,10 @@ public boolean hasDataForHttp() {
 
     @Override
     public Set<Map.Entry<String, String>> getHttpHeaders() {
-        return this.headers;
+        Map<String, String> headers = new HashMap<>();
+        headers.put(HTTP_HEADER_NAME, "Bearer " + accessToken);
+        headers.put(PULSAR_AUTH_METHOD_NAME, "token");

Review comment:
       This auth method name is already defined in `AuthenticationOAuth2#AUTH_METHOD_NAME`. What do you think about using that static reference?

##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java
##########
@@ -76,8 +77,15 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
                 // not sasl type, return role directly.
                 String role = authenticationService.authenticateHttpRequest((HttpServletRequest) request);
                 request.setAttribute(AuthenticatedRoleAttributeName, role);
-                request.setAttribute(AuthenticatedDataAttributeName,
-                    new AuthenticationDataHttps((HttpServletRequest) request));
+                String authMethodName = httpRequest.getHeader("X-Pulsar-Auth-Method-Name");
+                if (authMethodName != null && authenticationService.getAuthenticationProvider(authMethodName) != null) {
+                    AuthenticationState authenticationState = authenticationService
+                            .getAuthenticationProvider(authMethodName).newHttpAuthState(httpRequest);
+                    request.setAttribute(AuthenticatedDataAttributeName, authenticationState.getAuthDataSource());

Review comment:
       This change will break the `FunctionApiResource#clientAuthData` method. We'll need to update that method's casting and return signature from `AuthenticationDataHttps` to `AuthenticationDataSource`.
   
   https://github.com/apache/pulsar/blob/8cb4c10fb705859efc68b4f556061abf14172d2c/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/FunctionApiResource.java#L57-L59
   
   This is also true in the `PulsarWebResource` class.

##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationDataBasic.java
##########
@@ -29,12 +29,13 @@
 
 public class AuthenticationDataBasic implements AuthenticationDataProvider {
     private static final String HTTP_HEADER_NAME = "Authorization";
+    private static final String PULSAR_AUTH_METHOD_NAME = "X-Pulsar-Auth-Method-Name";

Review comment:
       Instead of defining this in every implementation, we should put this in the `AuthenticationDataProvider`.

##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java
##########
@@ -76,8 +77,15 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
                 // not sasl type, return role directly.
                 String role = authenticationService.authenticateHttpRequest((HttpServletRequest) request);

Review comment:
       Since this PR is added the appropriate headers by default, I think we should clean up and possibly deprecate this `authenticateHttpRequest` method. If you look at its internal definition, it branches based on `"X-Pulsar-Auth-Method-Name"`. Note also that the current design of this PR will result in double verification of the authentication data, which should be avoided.




-- 
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: commits-unsubscribe@pulsar.apache.org

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