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

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

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

 ##########
 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:
   Hm, this `isExcluded` parameter should actually be removed, it was part of the original Kerberos authentication extension before the larger auth system was added.
   
   This path exclusion functionality should now be handled by setting `druid.auth.unsecuredPaths`, a filter at the start of the filterChain would authenticate+authorize requests made to paths specified in this config.

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