Conversation
|
One concern on the user-based filtering for GET /api/v1/sessions — we already have GET /api/v1/admin/sessions in AdminResource with user filtering support, and BatchesResource doesn't enforce user filtering either, so this would be inconsistent. Also without authentication enabled, anyone can claim any identity, so the filtering provides no real security. |
On auth, I agree that if auth isn’t on, you can’t really trust “current user,” so filtering isn’t a strong security fix by itself. The important part for everyone is redacting sensitive config in responses. When auth is on, listing only that user’s sessions still matches what we’d expect. On consistency with admin/sessions and batches, I treated GET /api/v1/sessions as the normal client API (not admin), so limiting the list to the logged-in user should be fine there. I get that other endpoints don’t all behave the same yet, if you prefer not changing session listing, I’m fine dropping it. We can figure out filtering for sessions/batches in a follow-up once there’s a clean plan. |
|
can we make the returned configs content configurable? e.g., |
Are you suggesting the conf display behavior should depend on the caller's role? For example, admins see the full config while regular users see redacted or nothing? or uses a single global mode kyuubi.session.conf.display.mode to set defult is REDACTED |
|
@Yutong-1424, does the global server-side config meet your requirements? if so, this is fine. if you want fine-grained control capabilities, we can allow client-side config override and check it on the server-side, the check rules might be flexible, and we can discuss details during implementation. |
…L/NONE)" This reverts commit 1b3e81e.
…ility in REST API and add tests cover three modes
|
I implemented |
| .createOptional | ||
|
|
||
| val SESSION_CONF_DISPLAY_MODE: ConfigEntry[String] = | ||
| buildConf("kyuubi.session.conf.display.mode") |
There was a problem hiding this comment.
how about "kyuubi.server.conf.retrieveMode"? we might extend the effective scope to batch, operation, server conf display, etc.
| session: KyuubiSession): java.util.Map[String, String] = { | ||
| session.sessionManager.getConf.get(SESSION_CONF_DISPLAY_MODE) match { | ||
| case "NONE" => Map.empty[String, String].asJava | ||
| case "ORIGINAL" => rawConf.asJava |
| val userName = fe.getSessionUser(Map.empty[String, String]) | ||
| sessionManager | ||
| .allSessions() | ||
| .filter(session => session.user == userName) |
There was a problem hiding this comment.
it's reasonable for security purposes, but it's a breaking change.
I would suggest making this change independent, with an internal legacy config switch (something like kyuubi.frontend.rest.legacy.v1.sessionsReturnAllUsers), and adding an item in the migration guide.
4 participants
Why are the changes needed?
Sensitive settings in session APIs
Session APIs were returning full session configs as-is. Some keys (passwords, tokens, etc.) should not be shown to clients or in traces. This patch applies the same redaction pattern the server already uses elsewhere (SERVER_SECRET_REDACTION_PATTERN + Utils.redact) so list/detail responses don’t expose those values in plain text.
Over-broad session listing
GET /api/v1/sessions returned every session on the server. With auth turned on, it should only return sessions for the logged-in user, not everyone else’s
How was this patch tested?
Added a REST test that opens a session with a sensitive Spark key, calls GET /api/v1/sessions, and checks the value is redacted.
Updated the session control CLI test so the session is opened with the same user the test client uses for auth, so the “list sessions” output matches real usage.
Verified tests run locally
Was this patch authored or co-authored using generative AI tooling?
No