[GitHub] [hive] dengzhhu653 commented on a diff in pull request #3233: [WIP]HIVE-26071: JWT authentication mechanism for Thrift over HTTP in HiveMetastore

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

dengzhhu653 commented on code in PR #3233:
URL: https://github.com/apache/hive/pull/3233#discussion_r864399754


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HmsThriftHttpServlet.java:
##########
@@ -39,75 +48,119 @@ public class HmsThriftHttpServlet extends TServlet {
       .getLogger(HmsThriftHttpServlet.class);
 
   private static final String X_USER = MetaStoreUtils.USER_NAME_HTTP_HEADER;
-
   private final boolean isSecurityEnabled;
+  private final boolean jwtAuthEnabled;
+  public static final String AUTHORIZATION = "Authorization";
+  private JWTValidator jwtValidator;
+  private Configuration conf;
 
   public HmsThriftHttpServlet(TProcessor processor,
-      TProtocolFactory inProtocolFactory, TProtocolFactory outProtocolFactory) {
-    super(processor, inProtocolFactory, outProtocolFactory);
-    // This should ideally be reveiving an instance of the Configuration which is used for the check
+      TProtocolFactory protocolFactory, Configuration conf) {
+    super(processor, protocolFactory);
+    this.conf = conf;
     isSecurityEnabled = UserGroupInformation.isSecurityEnabled();
+    if (MetastoreConf.getVar(conf,
+        ConfVars.THRIFT_METASTORE_AUTHENTICATION).equalsIgnoreCase("jwt")) {
+      jwtAuthEnabled = true;
+    } else {
+      jwtAuthEnabled = false;
+      jwtValidator = null;
+    }
   }
 
-  public HmsThriftHttpServlet(TProcessor processor,
-      TProtocolFactory protocolFactory) {
-    super(processor, protocolFactory);
-    isSecurityEnabled = UserGroupInformation.isSecurityEnabled();
+  public void init() throws ServletException {
+    super.init();
+    if (jwtAuthEnabled) {
+      try {
+        jwtValidator = new JWTValidator(this.conf);
+      } catch (Exception e) {
+        throw new ServletException("Failed to initialize HmsThriftHttpServlet."
+            + " Error: " + e);
+      }
+    }
   }
 
   @Override
   protected void doPost(HttpServletRequest request,
       HttpServletResponse response) throws ServletException, IOException {
-
-    Enumeration<String> headerNames = request.getHeaderNames();
     if (LOG.isDebugEnabled()) {
-      LOG.debug("Logging headers in request");
+      LOG.debug(" Logging headers in doPost request");
+      Enumeration<String> headerNames = request.getHeaderNames();
       while (headerNames.hasMoreElements()) {
         String headerName = headerNames.nextElement();
         LOG.debug("Header: [{}], Value: [{}]", headerName,
             request.getHeader(headerName));
       }
     }
-    String userFromHeader = request.getHeader(X_USER);
-    if (userFromHeader == null || userFromHeader.isEmpty()) {
-      LOG.error("No user header: {} found", X_USER);
-      response.sendError(HttpServletResponse.SC_FORBIDDEN,
-          "Header: " + X_USER + " missing in the request");
-      return;
-    }
-
-    // TODO: These should ideally be in some kind of a Cache with Weak referencse.
-    // If HMS were to set up some kind of a session, this would go into the session by having
-    // this filter work with a custom Processor / or set the username into the session
-    // as is done for HS2.
-    // In case of HMS, it looks like each request is independent, and there is no session
-    // information, so the UGI needs to be set up in the Connection layer itself.
-    UserGroupInformation clientUgi;
-    // Temporary, and useless for now. Here only to allow this to work on an otherwise kerberized
-    // server.
-    if (isSecurityEnabled) {
-      LOG.info("Creating proxy user for: {}", userFromHeader);
-      clientUgi = UserGroupInformation.createProxyUser(userFromHeader, UserGroupInformation.getLoginUser());
-    } else {
-      LOG.info("Creating remote user for: {}", userFromHeader);
-      clientUgi = UserGroupInformation.createRemoteUser(userFromHeader);
+    try {
+      String userFromHeader = extractUserName(request, response);
+      UserGroupInformation clientUgi;
+      // Temporary, and useless for now. Here only to allow this to work on an otherwise kerberized
+      // server.
+      if (isSecurityEnabled) {
+        LOG.info("Creating proxy user for: {}", userFromHeader);
+        clientUgi = UserGroupInformation.createProxyUser(userFromHeader, UserGroupInformation.getLoginUser());
+      } else {
+        LOG.info("Creating remote user for: {}", userFromHeader);
+        clientUgi = UserGroupInformation.createRemoteUser(userFromHeader);
+      }
+      PrivilegedExceptionAction<Void> action = new PrivilegedExceptionAction<Void>() {
+        @Override
+        public Void run() throws Exception {
+          HmsThriftHttpServlet.super.doPost(request, response);
+          return null;
+        }
+      };
+      try {
+        clientUgi.doAs(action);
+      } catch (InterruptedException | RuntimeException e) {
+        LOG.error("Exception when executing http request as user: " + clientUgi.getUserName(),
+            e);
+        throw new ServletException(e);
+      }
+    } catch (HttpAuthenticationException e) {
+      response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+      response.getWriter().println("Authentication error: " + e.getMessage());
+      // Also log the error message on server side
+      LOG.error("Authentication error: ", e);
     }
-
-
-    PrivilegedExceptionAction<Void> action = new PrivilegedExceptionAction<Void>() {
-      @Override
-      public Void run() throws Exception {
-        HmsThriftHttpServlet.super.doPost(request, response);
-        return null;
+  }
+  private String extractUserName(HttpServletRequest request, HttpServletResponse response)
+      throws HttpAuthenticationException {
+    if (!jwtAuthEnabled) {
+      String userFromHeader = request.getHeader(X_USER);
+      if (userFromHeader == null || userFromHeader.isEmpty()) {
+        throw new HttpAuthenticationException("User header " + X_USER + " missing in request");
       }
-    };
-
+      return userFromHeader;
+    }
+    String signedJwt = extractBearerToken(request, response);
+    if (signedJwt == null) {
+      throw new HttpAuthenticationException("Couldn't find bearer token in the auth header in the request");
+    }
+    String user;
     try {
-      clientUgi.doAs(action);
-    } catch (InterruptedException | RuntimeException e) {
-      LOG.error("Exception when executing http request as user: " + clientUgi.getUserName(),
-          e);
-      throw new ServletException(e);
+      user = jwtValidator.validateJWTAndExtractUser(signedJwt);
+      Preconditions.checkNotNull(user, "JWT needs to contain the user name as subject");
+      Preconditions.checkState(!user.isEmpty(), "User name should not be empty in JWT");
+      LOG.info("Successfully validated and extracted user name {} from JWT in Auth "
+          + "header in the request", user);
+    } catch (Exception e) {
+      throw new HttpAuthenticationException("Failed to validate JWT from Bearer token in "

Review Comment:
   Fine, that makes sense to me!



-- 
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: gitbox-unsubscribe@hive.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org