Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
210 changes: 209 additions & 1 deletion security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,13 @@

import java.util.ArrayList;
import java.util.Calendar;
import java.util.Comparator;
import java.util.Date;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.CopyOnWriteArraySet;

Expand All @@ -77,6 +80,14 @@
public class SessionMgr {
static final Logger logger = LoggerFactory.getLogger(SessionMgr.class);

public static final String PROP_SESSION_LIMIT_CONCURRENCY = "ranger.session.limit.concurrency";
public static final String SESSION_ATTR_CONCURRENT_EXPIRED = "RANGER_CONCURRENT_SESSION_EXPIRED";
public static final String SESSION_ATTR_CONCURRENT_EXPIRED_SSO = "RANGER_CONCURRENT_SESSION_EXPIRED_SSO";
public static final String SESSION_ATTR_DOWNLOAD_ONLY = "RANGER_SESSION_DOWNLOAD_ONLY";
public static final String SESSION_ATTR_NON_UI = "RANGER_SESSION_NON_UI";
private static final String DEFAULT_BROWSER_USER_AGENTS = "Mozilla,Opera,Chrome";
private static final ConcurrentHashMap<String, Object> CONCURRENT_SESSION_LOCKS = new ConcurrentHashMap<>();

private static final Long SESSION_UPDATE_INTERVAL_IN_MILLIS = 30 * DateUtils.MILLIS_PER_MINUTE;

@Autowired
Expand Down Expand Up @@ -175,7 +186,7 @@ public UserSessionBase processSuccessLogin(int authType, String userAgent, HttpS
gjAuthSession = storeAuthSession(gjAuthSession);

session.setAttribute("auditLoginId", gjAuthSession.getId());
} else if (!StringUtils.isEmpty(httpRequest.getRequestURI()) && !(httpRequest.getRequestURI().contains("/secure/policies/download/") || httpRequest.getRequestURI().contains("/secure/download/"))) {
} else if (!StringUtils.isEmpty(httpRequest.getRequestURI()) && !isPluginOrSecureDownloadRequest(httpRequest.getRequestURI())) {
gjAuthSession = storeAuthSession(gjAuthSession);

session.setAttribute("auditLoginId", gjAuthSession.getId());
Expand Down Expand Up @@ -226,6 +237,24 @@ public UserSessionBase processSuccessLogin(int authType, String userAgent, HttpS
logger.debug("Login Success: loginId={}, sessionId={}, details is null, epoch={}", currentLoginId, gjAuthSession.getId(), cal.getTimeInMillis());
}
}

if (session != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block runs whenever processSuccessLogin() takes the newSessionCreation path which fires on the first request of any new HttpSession for an authenticated principal, not specifically on a UI login. For SPNEGO/Kerberos/trusted-proxy auth there's no discrete "login" step, so any non-interactive caller that doesn't reuse a session cookie (e.g. a kinit+curl script, RangerClient usage, or any authenticated REST call outside the download-URL allowlist in isPluginOrSecureDownloadRequest()) will consume a slot in the same per-user quota as the browser UI session, and can evict the admin's actual browser tab.

The JIRA describes this as limiting UI sessions specifically. As written, it limits "any non-download authenticated session." Worth either:

a--> restricting this to actual UI traffic (user-agent check, a UI marker/referer, or scoping to specific URL prefixes), or

b--> adding a second property to opt API-style sessions in/out of the quota (default: excluded), or

c--> if the current broader scope is intentional, updating the JIRA/description to say so explicitly so this doesn't surprise anyone running automation against the same login ID.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Agreed this should be UI-only. Quota now applies only when the User-Agent matches ranger.krb.browser-useragents-regex. Non-browser REST/API clients are marked non-UI and do not count.

if (isPluginOrSecureDownloadRequest(httpRequest.getRequestURI())) {
try {
session.setAttribute(SESSION_ATTR_DOWNLOAD_ONLY, Boolean.TRUE);
} catch (IllegalStateException e) {
logger.debug("Could not mark download-only session", e);
}
} else if (!isBrowserUserAgent(resolveUserAgent(userAgent, httpRequest))) {
try {
session.setAttribute(SESSION_ATTR_NON_UI, Boolean.TRUE);
} catch (IllegalStateException e) {
logger.debug("Could not mark non-UI session", e);
}
} else {
enforceConcurrentSessionLimit(currentLoginId, session);
}
}
}

return userSession;
Expand Down Expand Up @@ -510,6 +539,185 @@ public Date getLastSuccessLoginAuthTimeByUserId(String loginId) {
return null;
}

public static boolean isConcurrentSessionExpired(HttpSession session) {
if (session == null) {
return false;
}

try {
return Boolean.TRUE.equals(session.getAttribute(SESSION_ATTR_CONCURRENT_EXPIRED));
} catch (IllegalStateException e) {
return false;
}
}

public static boolean isConcurrentSessionExpiredSso(HttpSession session) {
if (session == null) {
return false;
}

try {
return Boolean.TRUE.equals(session.getAttribute(SESSION_ATTR_CONCURRENT_EXPIRED_SSO));
} catch (IllegalStateException e) {
return false;
}
}

/**
* When {@code ranger.session.limit.concurrency} is exceeded, expire the oldest UI sessions
* so the new login succeeds. SSO sessions are marked expired for Knox logout redirect.
* The count is taken from this JVM's in-memory session list, not cluster-wide.
* Find-and-expire is serialized per loginId so two concurrent UI logins for the same user
* cannot both observe a count under the limit.
*/
protected void enforceConcurrentSessionLimit(String loginId, HttpSession currentSession) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This read-then-expire sequence isn't atomic per loginId. Two near-simultaneous logins for the same user (e.g. two browser tabs, or a script racing the UI) can both read findActiveUiSessionsForUser() before either's new session is reflected, and both could independently decide nothing needs expiring momentarily letting the user exceed limit by one. CopyOnWriteArrayList only makes the iteration thread-safe, not this check-then-act sequence.

Given Admin login rate is low, a simple per-loginId lock (e.g. a striped lock, or ConcurrentHashMap<String,Object>.computeIfAbsent used as a lock table) around the find+expire sequence would close this without much cost. Not blocking, but worth a follow-up if not fixed here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Find-and-expire is now serialized per loginId with a ConcurrentHashMap lock table.

int limit = PropertiesUtil.getIntProperty(PROP_SESSION_LIMIT_CONCURRENCY, 0);

if (limit <= 0 || StringUtils.isBlank(loginId) || currentSession == null) {
return;
}

Object lock = CONCURRENT_SESSION_LOCKS.computeIfAbsent(loginId.toLowerCase(Locale.ROOT), id -> new Object());

synchronized (lock) {
List<HttpSession> otherSessions = findActiveUiSessionsForUser(loginId, currentSession);

if (otherSessions.size() < limit) {
return;
}

otherSessions.sort(Comparator.comparingLong(session -> {
try {
return session.getCreationTime();
} catch (IllegalStateException e) {
return 0L;
}
}));

int toExpire = otherSessions.size() - limit + 1;

logger.info("Concurrent session limit {} exceeded for user {}; expiring {} older session(s)", limit, loginId, toExpire);

for (int i = 0; i < toExpire; i++) {
expireConcurrentSession(otherSessions.get(i));
}
}
}

/**
* Plugin and secure download URLs. Used both to skip x_auth_sess rows (unless
* {@code ranger.downloadpolicy.session.log.enabled} is true) and to exclude
* those sessions from the UI concurrent-session quota.
*/
static boolean isPluginOrSecureDownloadRequest(String uri) {
if (StringUtils.isEmpty(uri)) {
return false;
}

return uri.contains("/secure/policies/download/")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are now two different "is this a plugin/download request" checks in this class: the existing audit-skip logic a few lines up only tests /secure/policies/download/ and /secure/download/, while this new method (used for the session-quota bypass) tests seven different path substrings. I checked all seven against the actual @Path mappings (GdsREST, RoleREST, ServiceREST, XUserREST, TagREST/TagRESTConstants) and they do match real endpoints, so this method itself looks correct — but having two separately-maintained definitions of the same concept in one class is a drift risk going forward.

Was it intentional that the audit-skip check and the quota-skip check cover different URL sets? If they're meant to represent the same "plugin/download traffic" concept, it'd be worth centralizing into one helper and using it in both places. If they're deliberately different scopes (audit logging vs. quota accounting), a short comment explaining why would help the next person who touches this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Audit-skip and quota-skip now share isPluginOrSecureDownloadRequest().

|| uri.contains("/secure/download/")
|| uri.contains("/plugins/policies/download/")
|| uri.contains("/tags/download/")
|| uri.contains("/roles/download/")
|| uri.contains("/xusers/download/")
|| uri.contains("/gds/download/");
}

static boolean isBrowserUserAgent(String userAgent) {
if (StringUtils.isBlank(userAgent)) {
return false;
}

String agents = PropertiesUtil.getProperty("ranger.krb.browser-useragents-regex", DEFAULT_BROWSER_USER_AGENTS);

if (StringUtils.isBlank(agents)) {
agents = DEFAULT_BROWSER_USER_AGENTS;
}

String userAgentLower = userAgent.toLowerCase(Locale.ROOT);

for (String agentPrefix : agents.split(",")) {
if (StringUtils.isNotBlank(agentPrefix) && userAgentLower.startsWith(agentPrefix.trim().toLowerCase(Locale.ROOT))) {
return true;
}
}

return false;
}

private static String resolveUserAgent(String userAgent, HttpServletRequest httpRequest) {
if (StringUtils.isNotBlank(userAgent)) {
return userAgent;
}

return httpRequest != null ? httpRequest.getHeader(HTTPUtil.USER_AGENT) : null;
}

private List<HttpSession> findActiveUiSessionsForUser(String loginId, HttpSession currentSession) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getActiveSessionOnServer() returns a single-JVM static list, so this only sees sessions handled by the node that's processing the current login. In an HA/load-balanced Ranger Admin deployment (multiple nodes), a user can hold up to limit sessions per node rather than limit sessions cluster-wide — the concurrency limit isn't actually enforced globally.

Could you either document this explicitly in the ranger.session.limit.concurrency property description (so operators aren't surprised in HA setups), or consider a DB-backed check against XXAuthSession/a shared store if cluster-wide enforcement is the intended guarantee?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Cluster-wide enforcement is out of scope for this change. The property description and javadoc now say this is per Ranger Admin process using that node's in-memory session list.

CopyOnWriteArrayList<HttpSession> activeHttpSessions = RangerHttpSessionListener.getActiveSessionOnServer();
List<HttpSession> matching = new ArrayList<>();

if (CollectionUtils.isEmpty(activeHttpSessions)) {
return matching;
}

for (HttpSession httpSession : activeHttpSessions) {
if (httpSession == null || httpSession == currentSession) {
continue;
}

try {
if (Boolean.TRUE.equals(httpSession.getAttribute(SESSION_ATTR_CONCURRENT_EXPIRED))) {
continue;
}

if (Boolean.TRUE.equals(httpSession.getAttribute(SESSION_ATTR_DOWNLOAD_ONLY))
|| Boolean.TRUE.equals(httpSession.getAttribute(SESSION_ATTR_NON_UI))) {
continue;
}

if (httpSession.getAttribute(RangerSecurityContextFormationFilter.AKA_SC_SESSION_KEY) == null) {
continue;
}

RangerSecurityContext securityContext = (RangerSecurityContext) httpSession.getAttribute(RangerSecurityContextFormationFilter.AKA_SC_SESSION_KEY);
UserSessionBase userSession = securityContext != null ? securityContext.getUserSession() : null;

if (userSession != null && loginId.equalsIgnoreCase(userSession.getLoginId())) {
matching.add(httpSession);
}
} catch (IllegalStateException e) {
logger.debug("Skipping invalidated session while counting concurrent sessions", e);
}
}

return matching;
}

private void expireConcurrentSession(HttpSession httpSession) {
try {
RangerSecurityContext context = (RangerSecurityContext) httpSession.getAttribute(RangerSecurityContextFormationFilter.AKA_SC_SESSION_KEY);
UserSessionBase userSession = context != null ? context.getUserSession() : null;
boolean ssoOrProxy = userSession != null
&& (Boolean.TRUE.equals(userSession.isSSOEnabled()) || Boolean.TRUE.equals(userSession.isSpnegoEnabled()));

httpSession.setAttribute(SESSION_ATTR_CONCURRENT_EXPIRED, Boolean.TRUE);
httpSession.setAttribute(SESSION_ATTR_CONCURRENT_EXPIRED_SSO, ssoOrProxy);

if (context != null) {
context.setUserSession(null);
}

logger.info("Expired concurrent Ranger Admin session (ssoOrTrustedProxy={})", ssoOrProxy);

if (!ssoOrProxy) {
httpSession.invalidate();
}
} catch (IllegalStateException e) {
logger.debug("Session already invalidated while enforcing concurrent session limit", e);
}
}

protected boolean validateUserSession(UserSessionBase userSession, String currentLoginId) {
if (currentLoginId.equalsIgnoreCase(userSession.getXXPortalUser().getLoginId())) {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@
import org.apache.hadoop.security.authorize.AuthorizationException;
import org.apache.hadoop.security.authorize.ProxyUsers;
import org.apache.hadoop.util.HttpExceptionUtils;
import org.apache.ranger.biz.SessionMgr;
import org.apache.ranger.biz.UserMgr;
import org.apache.ranger.common.PropertiesUtil;
import org.apache.ranger.common.RESTErrorUtil;
import org.apache.ranger.common.RangerConstants;
import org.apache.ranger.util.RestUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -465,9 +467,17 @@ public Enumeration<String> getInitParameterNames() {

@Override
public void doFilter(ServletRequest request, ServletResponse response, FilterChain filterChain) throws IOException, ServletException {
String authtype = PropertiesUtil.getProperty(RANGER_AUTH_TYPE);
HttpServletRequest httpRequest = (HttpServletRequest) request;
Authentication existingAuth = SecurityContextHolder.getContext().getAuthentication();
HttpServletRequest httpRequest = (HttpServletRequest) request;
HttpServletResponse httpResponse = (HttpServletResponse) response;
HttpSession httpSession = httpRequest.getSession(false);

if (SessionMgr.isConcurrentSessionExpired(httpSession)) {
handleConcurrentSessionExpiredRequest(httpRequest, httpResponse);
return;
}

String authtype = PropertiesUtil.getProperty(RANGER_AUTH_TYPE);
Authentication existingAuth = SecurityContextHolder.getContext().getAuthentication();

if (isSpnegoEnable(authtype) && (existingAuth == null || !existingAuth.isAuthenticated())) {
KerberosName.setRules(PropertiesUtil.getProperty(NAME_RULES, "DEFAULT"));
Expand All @@ -482,7 +492,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha

try {
if (StringUtils.equals(httpRequest.getParameter("action"), RestUtil.TIMEOUT_ACTION)) {
handleTimeoutRequest(httpRequest, (HttpServletResponse) response);
handleTimeoutRequest(httpRequest, httpResponse);
} else {
super.doFilter(request, response, filterChain);
}
Expand All @@ -503,8 +513,6 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
}

if (allowTrustedProxy && StringUtils.isNotEmpty(doAsUser) && existingAuth != null && existingAuth.isAuthenticated() && StringUtils.equals(action, RestUtil.TIMEOUT_ACTION)) {
HttpServletResponse httpResponse = (HttpServletResponse) response;

handleTimeoutRequest(httpRequest, httpResponse);
} else {
filterChain.doFilter(request, response);
Expand Down Expand Up @@ -682,6 +690,38 @@ protected Configuration getProxyuserConfiguration() {
return conf;
}

private void handleConcurrentSessionExpiredRequest(HttpServletRequest httpRequest, HttpServletResponse httpResponse) throws IOException {
HttpSession httpSession = httpRequest.getSession(false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the non-SSO branch, this always does httpSession.invalidate() + httpResponse.sendRedirect(...) regardless of whether the request is a full page load or an XHR/fetch call from the React UI. That's inconsistent with the AJAX-aware convention already used elsewhere in this codebase RangerAuthenticationEntryPoint and RangerSSOAuthenticationFilter both check the X-Requested-With: XMLHttpRequest header and respond with RangerConstants.SC_AUTHENTICATION_TIMEOUT (419) + an X-Rngr-Redirect-Url header for AJAX calls, reserving sendRedirect for full-page navigations.

As written, an in-flight XHR call from the SPA that lands here will auto-follow the 302 and get login.jsp's HTML back where JSON was expected, which the frontend likely won't handle gracefully. Could this reuse the same XMLHttpRequest-aware pattern as RangerAuthenticationEntryPoint, instead of an unconditional sendRedirect?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Non-SSO concurrent expire now returns 419 + X-Rngr-Redirect-Url for XHR, and sendRedirect for full page loads.


if (SessionMgr.isConcurrentSessionExpiredSso(httpSession)) {
LOG.info("Concurrent session expired for SSO/Trusted Proxy user; redirecting to Knox login");
handleTimeoutRequest(httpRequest, httpResponse);
return;
}

LOG.info("Concurrent session expired; redirecting to Ranger login");

if (httpSession != null) {
try {
httpSession.invalidate();
} catch (IllegalStateException e) {
LOG.debug("Session already invalidated", e);
}
}

String loginPage = PropertiesUtil.getProperty("ranger.logout.success.page", "/login.jsp");
String redirectUrl = httpRequest.getContextPath() + loginPage;
String ajaxHeader = httpRequest.getHeader("X-Requested-With");

if ("XMLHttpRequest".equalsIgnoreCase(ajaxHeader)) {
httpResponse.setHeader("X-Frame-Options", "DENY");
httpResponse.setStatus(RangerConstants.SC_AUTHENTICATION_TIMEOUT);
httpResponse.setHeader("X-Rngr-Redirect-Url", redirectUrl);
} else {
httpResponse.sendRedirect(redirectUrl);
}
}

private void handleTimeoutRequest(HttpServletRequest httpRequest, HttpServletResponse httpResponse) throws IOException {
String xForwardedURL = RestUtil.constructForwardableURL(httpRequest);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,19 @@
<name>ranger.admin.login.autolock.maxfailure</name>
<value>5</value>
</property>
<property>
<name>ranger.session.limit.concurrency</name>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this description call out two things explicitly, so operators aren't surprised:

  1. This limit is enforced per Ranger Admin node's in-memory session list, not cluster-wide in an HA/load-balanced deployment a user can hold up to this many sessions on each node.
  2. Beyond the plugin/tag/role/policy download URLs, any authenticated request (including REST/API calls under the same login ID, e.g. scripted RangerClient usage) counts toward this limit, not just browser UI sessions.

Both are non-obvious from the property name (ranger.session.limit.concurrency) and could otherwise surprise someone tuning this in a cluster or with automation running under a shared account.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The description now states UI-only (REST/API excluded) and that enforcement is per Admin node, not cluster-wide.

<value>0</value>
<description>
Maximum number of concurrent Ranger Admin UI sessions per user.
When the limit is exceeded, the oldest UI session is expired so the new login succeeds.
0 or a negative value means no limit (default). Plugin policy/tag/role download
sessions and non-browser REST/API clients (for example scripted RangerClient or curl)
do not count toward this limit. Browser detection uses ranger.krb.browser-useragents-regex.
Enforcement is per Ranger Admin process using that node's in-memory session list.
In an HA or load-balanced deployment a user can hold up to this many UI sessions on each node.
</description>
</property>

<!-- # anonymous access -->
<property>
Expand Down
3 changes: 3 additions & 0 deletions security-admin/src/main/webapp/WEB-INF/web.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
<listener>
<listener-class>org.springframework.web.context.request.RequestContextListener</listener-class>
</listener>
<listener>
<listener-class>org.apache.ranger.security.listener.RangerHttpSessionListener</listener-class>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RangerHttpSessionListener is now registered twice: via this explicit <listener> entry and via the @weblistener annotation added in the same commit (RangerHttpSessionListener.java). web-app here doesn't set metadata-complete="true", so a Servlet 3.0+ container (Tomcat) will pick the class up through both annotation scanning and this XML declaration, instantiating two listener instances.

Since sessionCreated/sessionDestroyed both write into the same static CopyOnWriteArrayList listOfSession, every session create/destroy event fires twice, so each login adds the session to the list twice. That inflates the count enforceConcurrentSessionLimit() compares against ranger.session.limit.concurrency, making the limit trip early/incorrectly, and also affects the existing consumer of getActiveSessionOnServer() in SessionMgr.java.

Please pick one registration mechanism — either drop this <listener> block (the annotation alone is sufficient) or drop @WebListener and keep this explicit entry.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Kept the web.xml listener entry and removed @weblistener so the listener is registered once.

</listener>
<filter>
<filter-name>springSecurityFilterChain</filter-name>
<filter-class>org.springframework.web.filter.DelegatingFilterProxy</filter-class>
Expand Down
Loading