[GitHub] [incubator-druid] gianm commented on a change in pull request #7685: Remove unnecessary principal handling in KerberosAuthenticator

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

gianm commented on a change in pull request #7685: Remove unnecessary principal handling in KerberosAuthenticator
URL: https://github.com/apache/incubator-druid/pull/7685#discussion_r286087615
 
 

 ##########
 File path: extensions-core/druid-kerberos/src/main/java/org/apache/druid/security/kerberos/KerberosAuthenticator.java
 ##########
 @@ -230,56 +218,36 @@ protected AuthenticationToken getToken(HttpServletRequest request) throws Authen
       public void doFilter(ServletRequest request, ServletResponse response, FilterChain filterChain)
           throws IOException, ServletException
       {
-        HttpServletRequest httpReq = (HttpServletRequest) request;
-
         // If there's already an auth result, then we have authenticated already, skip this.
         if (request.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) != null) {
           filterChain.doFilter(request, response);
           return;
         }
 
+        // In the hadoop-auth 2.7.3 code that this was adapted from, the login would've occurred during init() of
+        // the AuthenticationFilter via `initializeAuthHandler(authHandlerClassName, filterConfig)`.
+        // Since we co-exist with other authentication schemes, don't login until we've checked that
+        // some other Authenticator didn't already validate this request.
         if (loginContext == null) {
           initializeKerberosLogin();
         }
 
+        // Checking for excluded paths is Druid-specific, not from hadoop-auth
         String path = ((HttpServletRequest) request).getRequestURI();
         if (isExcluded(path)) {
           filterChain.doFilter(request, response);
         } else {
-          String clientPrincipal;
-          try {
-            Cookie[] cookies = httpReq.getCookies();
-            if (cookies == null) {
-              clientPrincipal = getPrincipalFromRequestNew((HttpServletRequest) request);
-            } else {
-              clientPrincipal = null;
-              for (Cookie cookie : cookies) {
-                if ("hadoop.auth".equals(cookie.getName())) {
-                  Matcher matcher = HADOOP_AUTH_COOKIE_REGEX.matcher(cookie.getValue());
-                  if (matcher.matches()) {
-                    clientPrincipal = matcher.group(1);
-                    break;
-                  }
-                }
-              }
-            }
-          }
-          catch (Exception ex) {
-            clientPrincipal = null;
-          }
-
-          if (clientPrincipal != null) {
-            request.setAttribute(
-                AuthConfig.DRUID_AUTHENTICATION_RESULT,
-                new AuthenticationResult(clientPrincipal, authorizerName, name, null)
-            );
-          }
+          // Run the original doFilter method, but with modifications to error handling
+          doFilterSuper(request, response, filterChain);
         }
-
-        doFilterSuper(request, response, filterChain);
 
 Review comment:
   This used to doFilterSuper in both branches of the above `if`, but now it will only do it if `!isExcluded(path)`. Is this new behavior better?

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org