lmccay commented on code in PR #681: URL: https://github.com/apache/knox/pull/681#discussion_r1035199113
########## gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java: ########## @@ -187,21 +219,46 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } String principalName = getPrincipalName(subject); + String mappedPrincipalName = null; + try { + mappedPrincipalName = handleProxyUserImpersonation(request, principalName); + } catch(AuthorizationException e) { + LOG.hadoopAuthProxyUserFailed(e); + HttpExceptionUtils.createServletExceptionResponse((HttpServletResponse) response, HttpServletResponse.SC_FORBIDDEN, e); + return; + } - String mappedPrincipalName = mapUserPrincipalBase(principalName); + // mapping principal name using user principal mapping (if configured) + mappedPrincipalName = mapUserPrincipalBase(mappedPrincipalName); mappedPrincipalName = mapUserPrincipal(mappedPrincipalName); + String[] mappedGroups = mapGroupPrincipalsBase(mappedPrincipalName, subject); String[] groups = mapGroupPrincipals(mappedPrincipalName, subject); String[] virtualGroups = virtualGroupMapper.mapGroups(mappedPrincipalName, combine(subject, groups), request).toArray(new String[0]); groups = combineGroupMappings(mappedGroups, groups); groups = combineGroupMappings(virtualGroups, groups); - HttpServletRequestWrapper wrapper = wrapHttpServletRequest( - request, mappedPrincipalName); + HttpServletRequestWrapper wrapper = wrapHttpServletRequest(request, mappedPrincipalName); + continueChainAsPrincipal(wrapper, response, chain, mappedPrincipalName, unique(groups)); } + private String handleProxyUserImpersonation(ServletRequest request, String principalName) throws AuthorizationException { + if (impersonationEnabled) { + final String doAsUser = request.getParameter(AuthFilterUtils.QUERY_PARAMETER_DOAS); + if (doAsUser != null && !doAsUser.equals(principalName)) { + LOG.hadoopAuthDoAsUser(doAsUser, principalName, request.getRemoteAddr()); + if (principalName != null) { + AuthFilterUtils.authorizeImpersonationRequest((HttpServletRequest) request, principalName, doAsUser, topologyName, ROLE); + LOG.hadoopAuthProxyUserSuccess(); Review Comment: I think we have audit log details for mapped principals. If so, I think we need to add impersonation details as well. ########## gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java: ########## @@ -720,19 +705,21 @@ private Response getAuthenticationToken() { String createdBy = null; // checking the doAs user only makes sense if tokens are managed (this is where we store the userName information) // and if impersonation is enabled - if (impersonationEnabled && tokenStateService != null) { - final String doAsUser = request.getParameter(QUERY_PARAMETER_DOAS); - if (doAsUser != null && !doAsUser.equals(userName)) { - try { - //this call will authorize the doAs request - AuthFilterUtils.authorizeImpersonationRequest(request, doAsUser, getTopologyName(), TokenServiceDeploymentContributor.ROLE); - createdBy = userName; - userName = doAsUser; - log.tokenImpersonationSuccess(createdBy, doAsUser); - } catch (AuthorizationException e) { - log.tokenImpersonationFailed(e); - return Response.status(Response.Status.FORBIDDEN).entity("{ \"" + e.getMessage() + "\" }").build(); + if (tokenStateService != null) { + final String realUserName = (String) request.getAttribute(AuthFilterUtils.REAL_USER_NAME_ATTRIBUTE); + final Subject subject = SubjectUtils.getCurrentSubject(); + if (subject != null && SubjectUtils.isImpersonating(subject)) { + String primaryPrincipalName = SubjectUtils.getPrimaryPrincipalName(subject); + String impersonatedPrincipalName = SubjectUtils.getImpersonatedPrincipalName(subject); + if (!primaryPrincipalName.equals(impersonatedPrincipalName)) { + createdBy = primaryPrincipalName; + userName = impersonatedPrincipalName; + log.tokenImpersonationSuccess(createdBy, userName); Review Comment: I could use some walkthrough of the logic here. If it is not more clear in the full context of the file then it may help to describe here in comments or make the code more obvious. I do think that I have it but would like to make sure. It seems that we are setting the createdBy and userName for the token metadata when there is indeed a stateService in play. If impersonation/doAs is enabled then we use the primaryPrincipal from the Subject to reflect the createdBy. If it isn't then we use the realUserName from the request attributes. I DO NOT KNOW HOW THIS IS SET OR WHATUSER IT REPRESENTS. If realUser sounds like hadoop auth module and UGI terms. Does this assume that the HadoopAuthProvider and/or the HadoopGroupProvider is being used? Given that the doAs support is being added to a base identity assertion provider class, is this a safe assumption? If so, is that because we have added a dependency on hadoop for all identity assertion providers somewhere along the line? Anyway, back to the logic flow... In the case where subject is impersonating, we take the impersonatedPrincipalName as the userName. However, in the case that there is no impersonating, the userName is not set? Is this defaulted to the same as th e createdBy somwhere else? ########## gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java: ########## @@ -187,21 +219,46 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } String principalName = getPrincipalName(subject); + String mappedPrincipalName = null; + try { + mappedPrincipalName = handleProxyUserImpersonation(request, principalName); + } catch(AuthorizationException e) { + LOG.hadoopAuthProxyUserFailed(e); + HttpExceptionUtils.createServletExceptionResponse((HttpServletResponse) response, HttpServletResponse.SC_FORBIDDEN, e); + return; + } - String mappedPrincipalName = mapUserPrincipalBase(principalName); + // mapping principal name using user principal mapping (if configured) + mappedPrincipalName = mapUserPrincipalBase(mappedPrincipalName); mappedPrincipalName = mapUserPrincipal(mappedPrincipalName); + String[] mappedGroups = mapGroupPrincipalsBase(mappedPrincipalName, subject); String[] groups = mapGroupPrincipals(mappedPrincipalName, subject); String[] virtualGroups = virtualGroupMapper.mapGroups(mappedPrincipalName, combine(subject, groups), request).toArray(new String[0]); groups = combineGroupMappings(mappedGroups, groups); groups = combineGroupMappings(virtualGroups, groups); - HttpServletRequestWrapper wrapper = wrapHttpServletRequest( - request, mappedPrincipalName); + HttpServletRequestWrapper wrapper = wrapHttpServletRequest(request, mappedPrincipalName); + continueChainAsPrincipal(wrapper, response, chain, mappedPrincipalName, unique(groups)); } + private String handleProxyUserImpersonation(ServletRequest request, String principalName) throws AuthorizationException { + if (impersonationEnabled) { + final String doAsUser = request.getParameter(AuthFilterUtils.QUERY_PARAMETER_DOAS); + if (doAsUser != null && !doAsUser.equals(principalName)) { + LOG.hadoopAuthDoAsUser(doAsUser, principalName, request.getRemoteAddr()); + if (principalName != null) { + AuthFilterUtils.authorizeImpersonationRequest((HttpServletRequest) request, principalName, doAsUser, topologyName, ROLE); + LOG.hadoopAuthProxyUserSuccess(); + return doAsUser; Review Comment: nit; Wouldn't principalName = doAsUser; be better here than returning a separate variable and potentially having to accommodate this later if there is code needed before the final return? ########## gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java: ########## @@ -720,19 +705,21 @@ private Response getAuthenticationToken() { String createdBy = null; // checking the doAs user only makes sense if tokens are managed (this is where we store the userName information) // and if impersonation is enabled - if (impersonationEnabled && tokenStateService != null) { - final String doAsUser = request.getParameter(QUERY_PARAMETER_DOAS); - if (doAsUser != null && !doAsUser.equals(userName)) { - try { - //this call will authorize the doAs request - AuthFilterUtils.authorizeImpersonationRequest(request, doAsUser, getTopologyName(), TokenServiceDeploymentContributor.ROLE); - createdBy = userName; - userName = doAsUser; - log.tokenImpersonationSuccess(createdBy, doAsUser); - } catch (AuthorizationException e) { - log.tokenImpersonationFailed(e); - return Response.status(Response.Status.FORBIDDEN).entity("{ \"" + e.getMessage() + "\" }").build(); + if (tokenStateService != null) { + final String realUserName = (String) request.getAttribute(AuthFilterUtils.REAL_USER_NAME_ATTRIBUTE); + final Subject subject = SubjectUtils.getCurrentSubject(); + if (subject != null && SubjectUtils.isImpersonating(subject)) { + String primaryPrincipalName = SubjectUtils.getPrimaryPrincipalName(subject); + String impersonatedPrincipalName = SubjectUtils.getImpersonatedPrincipalName(subject); + if (!primaryPrincipalName.equals(impersonatedPrincipalName)) { + createdBy = primaryPrincipalName; + userName = impersonatedPrincipalName; + log.tokenImpersonationSuccess(createdBy, userName); } + } else if (StringUtils.isNotBlank(realUserName) && !realUserName.equals(userName)) { + // real user name is set by HadoopAuth filter for impersonated requests (part of 'doAs' processing) + createdBy = realUserName; Review Comment: The above comment does point to an assumption about the hadoopauth provider. Since we are adding this capability to the common identity assertion capabilities this is no longer true and it is not clear that the hadoop auth or hadoop groups providers are being used. I think that we need to make the above two conditional blocks more uniform and decoupled from hadoop terms/dependencies. In otherwords, by the time we get here we should only be relying on Subject Principals. -- 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: dev-unsubscr...@knox.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org