Copilot commented on code in PR #915:
URL: https://github.com/apache/ranger/pull/915#discussion_r3125185825
##########
security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerJwtAuthFilter.java:
##########
@@ -117,6 +124,36 @@ public void doFilter(ServletRequest request,
ServletResponse response, FilterCha
}
}
+ @Override
+ protected boolean isProxyEnabled() {
+ return
PropertiesUtil.getBooleanProperty("ranger.authentication.allow.trustedproxy",
false);
+ }
+
+ @Override
+ protected boolean authorizeProxyUser(String realUser, String doAsUser,
String remoteAddr) {
+ try {
+ UserGroupInformation ugi =
UserGroupInformation.createRemoteUser(realUser);
+ ugi = UserGroupInformation.createProxyUser(doAsUser, ugi);
+ ProxyUsers.authorize(ugi, remoteAddr);
+ LOG.debug("RangerJwtAuthFilter.authorizeProxyUser():
ProxyUsers.authorize SUCCEEDED for realUser=[{}], doAs=[{}]",
+ realUser, doAsUser);
+ return true;
+ } catch (AuthorizationException ex) {
+ LOG.warn("JWT ProxyUsers.authorize failed for doAs=[{}],
realUser=[{}]: {}", doAsUser, realUser, ex.getMessage());
+ return false;
+ }
+ }
+
+ private Configuration getProxyuserConfiguration() {
+ Configuration conf = new Configuration(false);
+ PropertiesUtil.getPropertiesMap().forEach((k, v) -> {
+ if (k.startsWith("ranger.proxyuser.")) {
+ conf.set(k, v);
Review Comment:
`getProxyuserConfiguration()` is introduced as `private`, which forces tests
to use reflection and prevents reuse/overrides. Consider making this method
`protected` (similar to
`RangerKRBAuthenticationFilter#getProxyuserConfiguration`) or package-private
so it can be unit-tested and extended without reflection.
##########
ranger-authn/src/main/java/org/apache/ranger/authz/handler/jwt/RangerDefaultJwtAuthHandler.java:
##########
@@ -81,12 +84,42 @@ public RangerAuth authenticate(HttpServletRequest
httpServletRequest) {
String jwtAuthHeaderStr = getJwtAuthHeader(httpServletRequest);
String jwtCookieStr = StringUtils.isBlank(jwtAuthHeaderStr) ?
getJwtCookie(httpServletRequest) : null;
String doAsUser =
httpServletRequest.getParameter(DO_AS_PARAMETER);
- String username = authenticate(jwtAuthHeaderStr,
jwtCookieStr, doAsUser);
+ // authenticate against the JWT first to get the real (token-verified)
user
+ String realUser = authenticate(jwtAuthHeaderStr, jwtCookieStr);
- if (username != null) {
- rangerAuth = new RangerAuth(username,
RangerAuth.AuthType.JWT_JWKS);
- }
+ if (realUser != null) {
+ String effectiveUser = realUser;
+
+ if (StringUtils.isNotBlank(doAsUser)) {
+ LOG.debug("RangerDefaultJwtAuthHandler.authenticate():
doAs=[{}] requested. isProxyEnabled=[{}]", doAsUser, isProxyEnabled());
+
+ if (!isProxyEnabled()) {
+ LOG.warn("doAs [{}] requested but trusted proxy is not
enabled. Ignoring doAs, proceeding with real user [{}].",
+ doAsUser, effectiveUser);
+ } else {
+ LOG.debug("RangerDefaultJwtAuthHandler.authenticate():
Calling authorizeProxyUser: realUser=[{}], doAs=[{}], remoteAddr=[{}]",
+ realUser, doAsUser,
httpServletRequest.getRemoteAddr());
+ // Check: is realUser authorized to impersonate doAsUser
+ if (!authorizeProxyUser(realUser, doAsUser,
httpServletRequest.getRemoteAddr())) {
+ LOG.warn("RangerDefaultJwtAuthHandler.authenticate():
doAs=[{}] not authorized for realUser=[{}]. Rejecting.", doAsUser, realUser);
+ return null;
+ }
+ //Checks passed → switch to doAs user
+ effectiveUser = doAsUser.trim();
+ LOG.info("JWT doAs authorized: effectiveUser=[{}],
realUser=[{}]", effectiveUser, realUser);
Review Comment:
`doAsUser` is trimmed only after the proxy authorization check, but the
authorization call uses the untrimmed request parameter. This can cause
legitimate impersonation to be rejected (or mismatched) when the parameter has
leading/trailing whitespace. Trim (and ideally normalize) `doAsUser` once
up-front and use the trimmed value consistently for `authorizeProxyUser(...)`,
logging, and `effectiveUser`.
##########
security-admin/src/test/java/org/apache/ranger/security/web/filter/TestRangerJwtAuthFilter.java:
##########
@@ -120,4 +123,54 @@ public void
testDoFilter_leavesAuthenticationNullWhenAuthenticateReturnsNull()
assertNull(SecurityContextHolder.getContext().getAuthentication());
}
+
+ @Test
+ void testIsProxyEnabled_defaultFalse() {
+
PropertiesUtil.getPropertiesMap().remove("ranger.authentication.allow.trustedproxy");
+ RangerJwtAuthFilter filter = new RangerJwtAuthFilter();
+ assertFalse(filter.isProxyEnabled());
+ }
+
+ @Test
+ void testIsProxyEnabled_trueWhenConfigured() {
+
PropertiesUtil.getPropertiesMap().put("ranger.authentication.allow.trustedproxy",
"true");
+ RangerJwtAuthFilter filter = new RangerJwtAuthFilter();
+ assertTrue(filter.isProxyEnabled());
+
PropertiesUtil.getPropertiesMap().remove("ranger.authentication.allow.trustedproxy");
+ }
+
+ @Test
+ void testAuthorizeProxyUser_returnsFalseWhenNoProxyConfigLoaded() {
+ RangerJwtAuthFilter filter = new RangerJwtAuthFilter();
+ // no proxyuser config loaded into ProxyUsers -> should fail safely
+ assertFalse(filter.authorizeProxyUser("knoxui", "admin", "10.0.0.1"));
+ }
+
+ @Test
+ void testGetProxyuserConfiguration_copiesOnlyProxyuserKeys() throws
Exception {
+ // Arrange: put both proxyuser keys and non-proxyuser keys
+ PropertiesUtil.getPropertiesMap().put("ranger.proxyuser.knoxui.hosts",
"*");
+
PropertiesUtil.getPropertiesMap().put("ranger.proxyuser.knoxui.groups", "*");
+ PropertiesUtil.getPropertiesMap().put("ranger.some.other.key",
"shouldNotBeCopied");
+
+ RangerJwtAuthFilter filter = new RangerJwtAuthFilter();
+
+ // Call private getProxyuserConfiguration() via reflection
+ Method m =
RangerJwtAuthFilter.class.getDeclaredMethod("getProxyuserConfiguration");
+ m.setAccessible(true);
+
+ Configuration conf = (Configuration) m.invoke(filter);
Review Comment:
This test uses reflection to access `getProxyuserConfiguration()`, which is
brittle and tightly couples the test to private implementation details. If
`getProxyuserConfiguration()` is made `protected`/package-private (as in
`RangerKRBAuthenticationFilter`), the test can call it directly and avoid
reflection.
##########
security-admin/src/main/java/org/apache/ranger/security/web/filter/RangerJwtAuthFilter.java:
##########
@@ -117,6 +124,36 @@ public void doFilter(ServletRequest request,
ServletResponse response, FilterCha
}
}
+ @Override
+ protected boolean isProxyEnabled() {
+ return
PropertiesUtil.getBooleanProperty("ranger.authentication.allow.trustedproxy",
false);
+ }
+
+ @Override
+ protected boolean authorizeProxyUser(String realUser, String doAsUser,
String remoteAddr) {
+ try {
+ UserGroupInformation ugi =
UserGroupInformation.createRemoteUser(realUser);
+ ugi = UserGroupInformation.createProxyUser(doAsUser, ugi);
+ ProxyUsers.authorize(ugi, remoteAddr);
+ LOG.debug("RangerJwtAuthFilter.authorizeProxyUser():
ProxyUsers.authorize SUCCEEDED for realUser=[{}], doAs=[{}]",
+ realUser, doAsUser);
+ return true;
+ } catch (AuthorizationException ex) {
+ LOG.warn("JWT ProxyUsers.authorize failed for doAs=[{}],
realUser=[{}]: {}", doAsUser, realUser, ex.getMessage());
Review Comment:
`authorizeProxyUser()` passes `doAsUser` directly into
`UserGroupInformation.createProxyUser(...)`. Since the effective user in
`RangerDefaultJwtAuthHandler` is trimmed, the authorization check should also
use a trimmed `doAsUser` to avoid authorization mismatches for values with
surrounding whitespace.
```suggestion
String trimmedDoAsUser = doAsUser == null ? null : doAsUser.trim();
try {
UserGroupInformation ugi =
UserGroupInformation.createRemoteUser(realUser);
ugi = UserGroupInformation.createProxyUser(trimmedDoAsUser, ugi);
ProxyUsers.authorize(ugi, remoteAddr);
LOG.debug("RangerJwtAuthFilter.authorizeProxyUser():
ProxyUsers.authorize SUCCEEDED for realUser=[{}], doAs=[{}]",
realUser, trimmedDoAsUser);
return true;
} catch (AuthorizationException ex) {
LOG.warn("JWT ProxyUsers.authorize failed for doAs=[{}],
realUser=[{}]: {}", trimmedDoAsUser, realUser, ex.getMessage());
```
##########
security-admin/src/test/java/org/apache/ranger/security/web/filter/TestRangerJwtAuthWrapper.java:
##########
@@ -124,4 +129,124 @@ public void testDoFilter_skipsJwtWhenSsoEnabled() throws
IOException, ServletExc
verify(jwtFilter, never()).doFilter(any(ServletRequest.class),
any(ServletResponse.class), any(FilterChain.class));
verify(chain, times(1)).doFilter(req, res);
}
+
+ @Test
+ void testDoFilter_invokesJwtFilter_whenBearerHeaderPresent() throws
Exception {
+ RangerContextHolder.resetSecurityContext();
+ SecurityContextHolder.clearContext();
+ PropertiesUtil.getPropertiesMap().put("ranger.sso.enabled", "false");
+
+ HttpServletRequest req = Mockito.mock(HttpServletRequest.class);
+ HttpServletResponse res = Mockito.mock(HttpServletResponse.class);
+ FilterChain chain = Mockito.mock(FilterChain.class);
+
+ Mockito.when(req.getHeader("Authorization")).thenReturn("Bearer
token");
+
+ RangerJwtAuthFilter jwt = Mockito.mock(RangerJwtAuthFilter.class);
+
+ RangerJwtAuthWrapper wrapper = new RangerJwtAuthWrapper();
+ setField(wrapper, "rangerJwtAuthFilter", jwt);
+
+ wrapper.doFilter(req, res, chain);
+
+ verify(jwt, times(1)).doFilter(req, res, chain);
+ verify(chain, times(1)).doFilter(req, res);
+ }
+
+ @Test
+ void
testDoFilter_skipsJwt_whenAlreadyAuthenticated_evenIfBearerHeaderPresent()
throws Exception {
+ PropertiesUtil.getPropertiesMap().put("ranger.sso.enabled", "false");
+
+ // mark request authenticated
+ SecurityContextHolder.getContext().setAuthentication(
+ new UsernamePasswordAuthenticationToken(
+ "kafka",
+ "",
+ Collections.singletonList(new
SimpleGrantedAuthority("ROLE_USER"))));
+
+ HttpServletRequest req = Mockito.mock(HttpServletRequest.class);
+ HttpServletResponse res = Mockito.mock(HttpServletResponse.class);
+ FilterChain chain = Mockito.mock(FilterChain.class);
+ RangerJwtAuthFilter jwt = Mockito.mock(RangerJwtAuthFilter.class);
+ RangerJwtAuthWrapper wrapper = new RangerJwtAuthWrapper();
+ setField(wrapper, "rangerJwtAuthFilter", jwt);
+
+ wrapper.doFilter(req, res, chain);
+
+ verify(jwt, never()).doFilter(req, res, chain);
+ verify(chain, times(1)).doFilter(req, res);
+ }
+
+ @Test
+ void testDoFilter_skipsJwtFilter_whenNoBearerAndNoCookie() throws
Exception {
+ PropertiesUtil.getPropertiesMap().put("ranger.sso.enabled", "false");
+
+ HttpServletRequest req = Mockito.mock(HttpServletRequest.class);
+ HttpServletResponse res = Mockito.mock(HttpServletResponse.class);
+ FilterChain chain = Mockito.mock(FilterChain.class);
+
+ // no bearer header, no cookies
+ Mockito.when(req.getHeader("Authorization")).thenReturn(null);
+ Mockito.when(req.getCookies()).thenReturn(null);
+
+ RangerJwtAuthFilter jwt = Mockito.mock(RangerJwtAuthFilter.class);
+ RangerJwtAuthWrapper wrapper = new RangerJwtAuthWrapper();
+ setField(wrapper, "rangerJwtAuthFilter", jwt);
+
+ wrapper.doFilter(req, res, chain);
+
+ verify(jwt, never()).doFilter(req, res, chain);
+ verify(chain, times(1)).doFilter(req, res);
+ }
+
+ @Test
+ void testDoFilter_invokesJwtFilter_whenJwtCookiePresent() throws Exception
{
+ PropertiesUtil.getPropertiesMap().put("ranger.sso.enabled", "false");
+
+ HttpServletRequest req = Mockito.mock(HttpServletRequest.class);
+ HttpServletResponse res = Mockito.mock(HttpServletResponse.class);
+ FilterChain chain = Mockito.mock(FilterChain.class);
+
+ Mockito.when(req.getHeader("Authorization")).thenReturn(null);
+ Mockito.when(req.getCookies()).thenReturn(new Cookie[] {new
Cookie("hadoop-jwt", "abc")});
+ RangerJwtAuthFilter jwt = Mockito.mock(RangerJwtAuthFilter.class);
+ RangerJwtAuthWrapper wrapper = new RangerJwtAuthWrapper();
+ setField(wrapper, "rangerJwtAuthFilter", jwt);
+
+ wrapper.doFilter(req, res, chain);
+
+ verify(jwt, times(1)).doFilter(req, res, chain);
+ verify(chain, times(1)).doFilter(req, res);
+ }
+
+ @Test
+ void
testDoFilter_redirectsToLogin_whenJwtAttemptedButUnauthenticated_andBrowserAgent()
throws Exception {
+ PropertiesUtil.getPropertiesMap().put("ranger.sso.enabled", "false");
+ System.setProperty("ranger.default.browser-useragents", "Mozilla");
+
+ HttpServletRequest req = Mockito.mock(HttpServletRequest.class);
+ HttpServletResponse res = Mockito.mock(HttpServletResponse.class);
+ FilterChain chain = Mockito.mock(FilterChain.class);
+
+ Mockito.when(req.getHeader("Authorization")).thenReturn("Bearer
token");
+ Mockito.when(req.getHeader("User-Agent")).thenReturn("Mozilla/5.0");
+
+ RangerJwtAuthFilter jwt = Mockito.mock(RangerJwtAuthFilter.class);
+ Mockito.doNothing().when(jwt).doFilter(req, res, chain);
+
+ RangerJwtAuthWrapper wrapper = new RangerJwtAuthWrapper();
+ wrapper.initialize(); // loads browser agents from properties/system
property
+ setField(wrapper, "rangerJwtAuthFilter", jwt);
+
+ wrapper.doFilter(req, res, chain);
+
+ verify(res, times(1)).sendRedirect("/login.jsp");
+ verify(chain, times(1)).doFilter(req, res);
Review Comment:
`System.setProperty("ranger.default.browser-useragents", ...)` is set in
this test but never restored/cleared, which can leak into other tests in the
JVM. Save the previous value and restore it in a `finally` block, or
clear/restore it in `@AfterEach`.
```suggestion
String previousBrowserUserAgents =
System.getProperty("ranger.default.browser-useragents");
System.setProperty("ranger.default.browser-useragents", "Mozilla");
try {
HttpServletRequest req = Mockito.mock(HttpServletRequest.class);
HttpServletResponse res =
Mockito.mock(HttpServletResponse.class);
FilterChain chain = Mockito.mock(FilterChain.class);
Mockito.when(req.getHeader("Authorization")).thenReturn("Bearer
token");
Mockito.when(req.getHeader("User-Agent")).thenReturn("Mozilla/5.0");
RangerJwtAuthFilter jwt =
Mockito.mock(RangerJwtAuthFilter.class);
Mockito.doNothing().when(jwt).doFilter(req, res, chain);
RangerJwtAuthWrapper wrapper = new RangerJwtAuthWrapper();
wrapper.initialize(); // loads browser agents from
properties/system property
setField(wrapper, "rangerJwtAuthFilter", jwt);
wrapper.doFilter(req, res, chain);
verify(res, times(1)).sendRedirect("/login.jsp");
verify(chain, times(1)).doFilter(req, res);
} finally {
if (previousBrowserUserAgents == null) {
System.clearProperty("ranger.default.browser-useragents");
} else {
System.setProperty("ranger.default.browser-useragents",
previousBrowserUserAgents);
}
}
```
##########
security-admin/src/test/java/org/apache/ranger/security/web/filter/TestRangerJwtAuthFilter.java:
##########
@@ -120,4 +123,54 @@ public void
testDoFilter_leavesAuthenticationNullWhenAuthenticateReturnsNull()
assertNull(SecurityContextHolder.getContext().getAuthentication());
}
+
+ @Test
+ void testIsProxyEnabled_defaultFalse() {
+
PropertiesUtil.getPropertiesMap().remove("ranger.authentication.allow.trustedproxy");
+ RangerJwtAuthFilter filter = new RangerJwtAuthFilter();
+ assertFalse(filter.isProxyEnabled());
+ }
+
+ @Test
+ void testIsProxyEnabled_trueWhenConfigured() {
+
PropertiesUtil.getPropertiesMap().put("ranger.authentication.allow.trustedproxy",
"true");
+ RangerJwtAuthFilter filter = new RangerJwtAuthFilter();
+ assertTrue(filter.isProxyEnabled());
+
PropertiesUtil.getPropertiesMap().remove("ranger.authentication.allow.trustedproxy");
+ }
+
+ @Test
+ void testAuthorizeProxyUser_returnsFalseWhenNoProxyConfigLoaded() {
+ RangerJwtAuthFilter filter = new RangerJwtAuthFilter();
+ // no proxyuser config loaded into ProxyUsers -> should fail safely
+ assertFalse(filter.authorizeProxyUser("knoxui", "admin", "10.0.0.1"));
+ }
Review Comment:
This assertion depends on the global static state inside Hadoop `ProxyUsers`
(and/or any prior `ProxyUsers.refresh...` calls) but the test doesn't reset
that state. To make the test deterministic, explicitly refresh `ProxyUsers`
with an empty `Configuration(false)` for the `ranger.proxyuser.` prefix (or
call `filter.initialize()` after clearing `ranger.proxyuser.*` properties)
before calling `authorizeProxyUser(...)`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
