Skip to content

RANGER-5749: Limit concurrent Ranger Admin UI sessions per user - #1200

Open
eoinmcdonnell113 wants to merge 4 commits into
apache:masterfrom
eoinmcdonnell113:RANGER-5749
Open

RANGER-5749: Limit concurrent Ranger Admin UI sessions per user#1200
eoinmcdonnell113 wants to merge 4 commits into
apache:masterfrom
eoinmcdonnell113:RANGER-5749

Conversation

@eoinmcdonnell113

Copy link
Copy Markdown
Contributor

Expire the oldest UI session when ranger.session.limit.concurrency is exceeded so a new login succeeds. Default 0 means unlimited.

What changes were proposed in this pull request?

RANGER-5749: Limit concurrent Ranger Admin UI sessions per user.

Adds ranger.session.limit.concurrency (default 0 = no limit). When the limit is exceeded, the oldest UI session for that user is expired so the new login succeeds. Plugin policy/tag/role download sessions do not count.

Form-login sessions are invalidated and sent to the Ranger login page. Knox SSO / Trusted Proxy sessions are marked expired and redirected to Knox login using the existing inactivity-timeout path.

JIRA: https://issues.apache.org/jira/browse/RANGER-5749

How was this patch tested?

Unit tests: TestSessionMgr, TestRangerHttpSessionListener, TestRangerKRBAuthenticationFilter (46 tests, 0 failures, 2 skipped).
Manual test on Ranger Admin Docker/UI with ranger.session.limit.concurrency=1: a second browser login as the same user expires the first session. The first browser is sent back to the Ranger login page.

Expire the oldest UI session when ranger.session.limit.concurrency is exceeded so a new login succeeds. Default 0 means unlimited.
<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.


import java.util.concurrent.CopyOnWriteArrayList;

@WebListener

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.

See my comment on web.xml — this annotation plus the explicit <listener> entry added there for the same class will cause double registration under Tomcat (no metadata-complete="true" on web.xml), doubling every session-created/destroyed event. Please remove one of the two registration paths.

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.

|| uri.contains("/gds/download/");
}

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.

}
}

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.

* 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.
*/
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.

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().

}

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.

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

}

@Test
public void testIsPluginOrSecureDownloadRequest() {

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.

Nice coverage of the core cases. A few gaps worth adding given the logic being tested:

  • limit=2 with three existing sessions for the user — verify only the single oldest is expired (proves the toExpire count and sort-by-creation-time ordering for N > 1, not just the N=1 case currently tested).
  • Two different users at limit=1 verify user B's login doesn't expire user A's session (proves the loginId filter in findActiveUiSessionsForUser).
  • A concurrent-login scenario (even a simple two-thread test) against limit=1, since enforceConcurrentSessionLimit isn't currently synchronized per loginId.
  • If REST/API sessions end up excluded per the discussion elsewhere in this PR, a test proving a /service/public/v2/api/...-style login doesn't consume a UI quota slot.

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. Added tests for limit=2 (only oldest expired), user B not kicking user A, a two-thread race, and REST /service/public/v2/api not consuming a UI slot.

Keep a single session listener, count only browser UI sessions, return 419 for AJAX kicks, and serialize find-and-expire per user.
}

for (String agentPrefix : agents.split(",")) {
if (StringUtils.isNotBlank(agentPrefix) && userAgent.toLowerCase().startsWith(agentPrefix.trim().toLowerCase())) {

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.

userAgent.toLowerCase() can be sent this method instead of looping and converting it again and again.

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. User-Agent is lowercased once before the prefix loop.

Avoid converting the user-agent on every prefix comparison.

@pradeepagrawal8184 pradeepagrawal8184 left a comment

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.

Optional follow-ups (non-blocking):

  • Clean up CONCURRENT_SESSION_LOCKS entries after use.
  • Add a brief release-note entry about per-node limits in HA case.
  • Consider renaming or documenting that RangerKRBAuthenticationFilter handles concurrent-session expiry for all auth types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants