-
Notifications
You must be signed in to change notification settings - Fork 994
Improve session REST security #7391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
c5a1b50
1b3e81e
52f9032
14de1e1
d068832
14d6253
18a9b3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3336,6 +3336,21 @@ object KyuubiConf { | |
| .regexConf | ||
| .createOptional | ||
|
|
||
| val SESSION_CONF_DISPLAY_MODE: ConfigEntry[String] = | ||
| buildConf("kyuubi.session.conf.display.mode") | ||
| .serverOnly | ||
| .doc("Controls how session configurations are returned in REST API responses. " + | ||
| "Supported values: " + | ||
| "<ul>" + | ||
| "<li>REDACTED: Mask values that match kyuubi.server.redaction.regex (default).</li>" + | ||
| "<li>ORIGINAL: Return the raw config values as-is.</li>" + | ||
| "<li>NONE: Omit the conf map from responses entirely.</li>" + | ||
| "</ul>") | ||
| .version("1.12.0") | ||
| .stringConf | ||
| .checkValues(Set("REDACTED", "ORIGINAL", "NONE")) | ||
| .createWithDefault("REDACTED") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a user-facing change, also needs to be mentioned in migration guide |
||
|
|
||
| val SERVER_PERIODIC_GC_INTERVAL: ConfigEntry[Long] = | ||
| buildConf("kyuubi.server.periodicGC.interval") | ||
| .doc("How often to trigger the periodic garbage collection. 0 will disable it.") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,13 +22,28 @@ import scala.collection.JavaConverters._ | |
| import org.apache.kyuubi.{Logging, Utils} | ||
| import org.apache.kyuubi.client.api.v1.dto | ||
| import org.apache.kyuubi.client.api.v1.dto.{OperationData, OperationProgress, ServerData, SessionData} | ||
| import org.apache.kyuubi.config.KyuubiConf.{SERVER_SECRET_REDACTION_PATTERN, SESSION_CONF_DISPLAY_MODE} | ||
| import org.apache.kyuubi.ha.client.ServiceNodeInfo | ||
| import org.apache.kyuubi.operation.KyuubiOperation | ||
| import org.apache.kyuubi.session.KyuubiSession | ||
|
|
||
| object ApiUtils extends Logging { | ||
|
|
||
| private def buildConf( | ||
| rawConf: Map[String, String], | ||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use enum |
||
| case _ => | ||
| val pattern = session.sessionManager.getConf.get(SERVER_SECRET_REDACTION_PATTERN) | ||
| Utils.redact(pattern, rawConf.toSeq).toMap.asJava | ||
| } | ||
| } | ||
|
|
||
| def sessionEvent(session: KyuubiSession): dto.KyuubiSessionEvent = { | ||
| session.getSessionEvent.map(event => | ||
| session.getSessionEvent.map { event => | ||
| val conf = buildConf(event.conf, session) | ||
| dto.KyuubiSessionEvent.builder() | ||
| .sessionId(event.sessionId) | ||
| .clientVersion(event.clientVersion) | ||
|
|
@@ -37,7 +52,7 @@ object ApiUtils extends Logging { | |
| .user(event.user) | ||
| .clientIp(event.clientIP) | ||
| .serverIp(event.serverIP) | ||
| .conf(event.conf.asJava) | ||
| .conf(conf) | ||
| .remoteSessionId(event.remoteSessionId) | ||
| .engineId(event.engineId) | ||
| .engineName(event.engineName) | ||
|
|
@@ -48,17 +63,19 @@ object ApiUtils extends Logging { | |
| .endTime(event.endTime) | ||
| .totalOperations(event.totalOperations) | ||
| .exception(event.exception.orNull) | ||
| .build()).orNull | ||
| .build() | ||
| }.orNull | ||
| } | ||
|
|
||
| def sessionData(session: KyuubiSession): SessionData = { | ||
| val sessionEvent = session.getSessionEvent | ||
| val conf = buildConf(session.conf, session) | ||
| new SessionData( | ||
| session.handle.identifier.toString, | ||
| sessionEvent.map(_.remoteSessionId).getOrElse(""), | ||
| session.user, | ||
| session.ipAddress, | ||
| session.conf.asJava, | ||
| conf, | ||
| session.createTime, | ||
| session.lastAccessTime - session.createTime, | ||
| session.getNoOperationTime, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,11 +52,15 @@ private[v1] class SessionsResource extends ApiRequestContext with Logging { | |
| content = Array(new Content( | ||
| mediaType = MediaType.APPLICATION_JSON, | ||
| array = new ArraySchema(schema = new Schema(implementation = classOf[SessionData])))), | ||
| description = "get the list of all live sessions") | ||
| description = "get the list of live sessions for the current user") | ||
| @GET | ||
| def sessions(): Seq[SessionData] = { | ||
| sessionManager.allSessions() | ||
| .map(session => ApiUtils.sessionData(session.asInstanceOf[KyuubiSession])).toSeq | ||
| val userName = fe.getSessionUser(Map.empty[String, String]) | ||
| sessionManager | ||
| .allSessions() | ||
| .filter(session => session.user == userName) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your comments, all comments addressed:
|
||
| .map(session => ApiUtils.sessionData(session.asInstanceOf[KyuubiSession])) | ||
| .toSeq | ||
| } | ||
|
|
||
| @ApiResponse( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about "kyuubi.server.conf.retrieveMode"? we might extend the effective scope to batch, operation, server conf display, etc.