Copilot commented on code in PR #7319:
URL: https://github.com/apache/kyuubi/pull/7319#discussion_r2893823055
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/ApiUtils.scala:
##########
@@ -22,13 +22,16 @@ 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
import org.apache.kyuubi.ha.client.ServiceNodeInfo
import org.apache.kyuubi.operation.KyuubiOperation
import org.apache.kyuubi.session.KyuubiSession
object ApiUtils extends Logging {
def sessionEvent(session: KyuubiSession): dto.KyuubiSessionEvent = {
- session.getSessionEvent.map(event =>
+ session.getSessionEvent.map { event =>
+ val redactionPattern =
session.sessionManager.getConf.get(SERVER_SECRET_REDACTION_PATTERN)
+ val redactedConf = Utils.redact(Option(redactionPattern),
event.conf.toSeq).toMap.asJava
Review Comment:
The call `Utils.redact(Option(redactionPattern), ...)` has a type error.
`session.sessionManager.getConf.get(SERVER_SECRET_REDACTION_PATTERN)` already
returns `Option[Regex]` because `OptionalConfigEntry[Regex]` extends
`ConfigEntry[Option[Regex]]`. Wrapping it in another `Option(...)` produces
`Option[Option[Regex]]`, which is a type mismatch with `Utils.redact`'s first
parameter type `Option[Regex]`. This would cause a compile-time error.
The correct call should pass `redactionPattern` directly (without
`Option()`), since it's already an `Option[Regex]`.
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/SessionsResourceSuite.scala:
##########
@@ -389,4 +389,27 @@ class SessionsResourceSuite extends KyuubiFunSuite with
RestFrontendTestHelper {
assert(operations.size == 1)
assert(sessionHandle.toString.equals(operations.head.getSessionId))
}
+
+ test("get /sessions returns redacted spark confs") {
Review Comment:
The test does not configure `SERVER_SECRET_REDACTION_PATTERN` (via
`conf.set(SERVER_SECRET_REDACTION_PATTERN, ...)`) before asserting that the
sensitive config value is redacted. Since this config entry has no default
value (`createOptional`), the redaction mechanism will not activate, and
`sensitiveValue` will be returned unredacted. The test needs to set the
redaction pattern (e.g., `"(?i)password".r`) as part of the test setup or
override the server config in `WithKyuubiServer`.
```suggestion
test("get /sessions returns redacted spark confs") {
// Configure redaction so that sensitive configs like spark.password are
masked
conf.set(KyuubiConf.SERVER_SECRET_REDACTION_PATTERN, "(?i)password".r)
```
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/ApiUtils.scala:
##########
@@ -48,17 +51,20 @@ 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 redactionPattern =
session.sessionManager.getConf.get(SERVER_SECRET_REDACTION_PATTERN)
+ val redactedConf = Utils.redact(Option(redactionPattern),
session.conf.toSeq).toMap.asJava
Review Comment:
Same type error as in `sessionEvent`:
`Utils.redact(Option(redactionPattern), ...)` passes `Option[Option[Regex]]`
where `Option[Regex]` is required. The `redactionPattern` variable is already
`Option[Regex]` (from `conf.get(SERVER_SECRET_REDACTION_PATTERN)`), so it
should be passed directly without the `Option()` wrapping.
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/SessionsResource.scala:
##########
@@ -55,8 +55,12 @@ private[v1] class SessionsResource extends ApiRequestContext
with Logging {
description = "get the list of all live sessions")
Review Comment:
The `@ApiResponse` description still says "get the list of all live
sessions" but the implementation now filters sessions to only return those
belonging to the currently authenticated user. The description should be
updated to reflect this behavior change, e.g., "get the list of live sessions
for the current user".
```suggestion
description = "get the list of live sessions for the current user")
```
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/SessionsResourceSuite.scala:
##########
@@ -389,4 +389,27 @@ class SessionsResourceSuite extends KyuubiFunSuite with
RestFrontendTestHelper {
assert(operations.size == 1)
assert(sessionHandle.toString.equals(operations.head.getSessionId))
}
+
+ test("get /sessions returns redacted spark confs") {
+ val sensitiveKey = "spark.password"
+ val sensitiveValue = "superSecret123"
+ val requestObj = new SessionOpenRequest(Map(sensitiveKey ->
sensitiveValue).asJava)
+
+ val response = webTarget.path("api/v1/sessions")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(requestObj, MediaType.APPLICATION_JSON_TYPE))
+ assert(200 == response.getStatus)
+ val sessionHandle =
response.readEntity(classOf[SessionHandle]).getIdentifier
+
+ val response2 = webTarget.path("api/v1/sessions").request().get()
+ assert(200 == response2.getStatus)
+ val sessions = response2.readEntity(new GenericType[Seq[SessionData]]() {})
+ val sessionConf = sessions.find(_.getIdentifier ==
sessionHandle).get.getConf
+
+ assert(sessionConf.get(sensitiveKey) != sensitiveValue)
+ assert(sessionConf.get(sensitiveKey).forall(_ == '*'))
Review Comment:
The test asserts `sessionConf.get(sensitiveKey).forall(_ == '*')`, but the
actual redaction replacement text in Kyuubi is `"*********(redacted)"` (defined
as `REDACTION_REPLACEMENT_TEXT` in `CommandLineUtils`), which contains
characters other than `'*'`. This assertion would fail even when redaction
works correctly. The assertion should instead compare against
`REDACTION_REPLACEMENT_TEXT` or at minimum check that the value is not equal to
the original sensitive value and not null.
```suggestion
assert(sessionConf.get(sensitiveKey) != null)
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
