You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "aajisaka (via GitHub)" <gi...@apache.org> on 2026/05/29 07:34:35 UTC
[PR] Restrict Engine UI proxy targets [kyuubi]
aajisaka opened a new pull request, #7483:
URL: https://github.com/apache/kyuubi/pull/7483
### Why are the changes needed?
Restrict Engine UI proxy targets to only the registered Kyuubi Engine URLs. User can disable the filter by setting `kyuubi.frontend.rest.engine.ui.proxy.filter.enabled` to false (Default: true).
### How was this patch tested?
Built Kyuubi and verified locally
```
# Build Kyuubi
./build/dist --web-ui
# Run Kyuubi Server
./dist/bin/kyuubi start
# Start Kyuubi Engine
./dist/bin/kyuubi-beeline -u 'jdbc:hive2://<your local IP address>:10009/' -n anonymous
# Browse Web UI (http://localhost:10009)
```
Also, added unit tests.
### Was this patch authored or co-authored using generative AI tooling?
Assisted-by: OpenCode (GPT-5.5)
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "wangzhigang1999 (via GitHub)" <gi...@apache.org>.
wangzhigang1999 commented on PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#issuecomment-4590028032
Looks good! Since this flips the proxy default, might be worth a quick migration-guide note for existing users.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "wangzhigang1999 (via GitHub)" <gi...@apache.org>.
wangzhigang1999 commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3324031625
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -18,51 +18,92 @@
package org.apache.kyuubi.server.api
import java.net.URL
+import java.util.Locale
+import java.util.regex.Pattern
import javax.servlet.http.HttpServletRequest
import org.apache.commons.lang3.StringUtils
import org.eclipse.jetty.client.api.Request
import org.eclipse.jetty.proxy.ProxyServlet
import org.apache.kyuubi.Logging
+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.server.KyuubiRestFrontendService
-private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
+private[api] class EngineUIProxyServlet(
+ proxyEnabled: Boolean,
+ allowedHosts: Set[String]) extends ProxyServlet with Logging {
+
+ private val allowedHostPatterns =
+ allowedHosts.map(EngineUIProxyServlet.normalizeHost).filter(_.nonEmpty).map(
+ EngineUIProxyServlet.wildcardHostPattern)
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
- var targetURL = "/no-ui-error"
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
- // for some reason, the proxy can not handle redirect well, as a workaround,
- // we simulate the Spark UI redirection behavior and forcibly rewrite the
- // empty URI to the Spark Jobs page.
- case "" | "/" => "/jobs/"
- case path => path
- }
- val targetQueryString =
- Option(request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
- targetURL = new URL("http", host, port, targetURI + targetQueryString).toString
- }
+ // If proxying is enabled and the request URI host is not allowed, return null so Jetty
+ // responds with 403 Forbidden.
+ val targetURL = allowedTarget(requestURI).map {
+ case (host, port) =>
+ val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
+ // for some reason, the proxy can not handle redirect well, as a workaround,
+ // we simulate the Spark UI redirection behavior and forcibly rewrite the
+ // empty URI to the Spark Jobs page.
+ case "" | "/" => "/jobs/"
+ case path => path
+ }
+ val targetQueryString =
+ Option(
+ request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
+ new URL("http", host, port, targetURI + targetQueryString).toString
+ }.orNull
debug(s"rewrite $requestURL => $targetURL")
targetURL
}
override def addXForwardedHeaders(
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
- val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
- proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
+ allowedTarget(clientRequest.getRequestURI).foreach {
+ case (host, port) =>
+ // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
+ proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isAllowedHost(host: String): Boolean =
+ proxyEnabled && {
+ val normalizedHost = EngineUIProxyServlet.normalizeHost(host)
+ allowedHostPatterns.exists(_.matcher(normalizedHost).matches())
+ }
+
+ private def allowedTarget(requestURI: String): Option[(String, Int)] =
+ extractTargetAddress(requestURI).filter(target => isAllowedHost(target._1))
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
Review Comment:
Parsing host and port directly with this regex is insecure — the host can be spoofed. I did a quick verification, and perhaps we should use a more robust method to parse the host and port.
```java
import java.net.URI;
import java.net.URL;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
public class EngineUIProxyRegexPoc {
public static void main(String[] a) throws Exception {
// same regex as EngineUIProxyServlet.scala:84
Pattern r = Pattern.compile("^/engine-ui/([^/:]+):(\\d+)/?.*");
// attacker URI: smuggle the real target after '@'
String uri = "/engine-ui/spark-x@100.100.100.200:80/latest/meta-data/";
Matcher m = r.matcher(uri);
m.matches();
String host = m.group(1); // host checked against allowlist
String target = new URL("http", host, 80, "/").toString(); // same as PR
String realHost = URI.create(target).getHost(); // host Jetty actually connects to
System.out.println("checked host : " + host); // spark-x@100.100.100.200
System.out.println("connect host : " + realHost); // 100.100.100.200
System.out.println("bypass? : " + !host.equals(realHost)); // true
}
}
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3323221492
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -52,17 +70,125 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
+ extractTargetAddress(requestURI).filter(isRegistered).foreach { case (host, port) =>
// SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isRegistered(target: (String, Int)): Boolean = {
+ !filterEnabled || {
+ try {
+ currentRegisteredEngineUrls().contains(normalizeHostPort(target))
+ } catch {
+ case NonFatal(e) =>
+ warn(s"Failed to validate Engine UI target ${target._1}:${target._2}", e)
+ false
+ }
+ }
+ }
+
+ private def currentRegisteredEngineUrls(): Set[(String, Int)] = {
+ val now = System.nanoTime()
+ val cacheTtlNanos = EngineUIProxyServlet.RegisteredEngineUrlsCacheTtlNanos
+ if (now - cachedAtNanos > cacheTtlNanos) {
+ synchronized {
+ val current = System.nanoTime()
+ if (current - cachedAtNanos > cacheTtlNanos) {
+ cachedRegisteredEngineUrls = registeredEngineUrls()
+ cachedAtNanos = current
+ }
+ }
+ }
+ cachedRegisteredEngineUrls
+ }
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
- private def extractTargetAddress(requestURI: String): Option[(String, Int)] =
+ private[api] def extractTargetAddress(requestURI: String): Option[(String, Int)] =
requestURI match {
case r(host, port) => Some(host -> port.toInt)
case _ => None
}
+
+ private def normalizeHostPort(hostPort: (String, Int)): (String, Int) = {
+ hostPort._1.toLowerCase(Locale.ROOT) -> hostPort._2
+ }
+}
+
+private[api] object EngineUIProxyServlet extends Logging {
+
+ // Engine UI pages load many assets through the proxy. Avoid scanning discovery for every request.
+ private val RegisteredEngineUrlsCacheTtlNanos = TimeUnit.SECONDS.toNanos(10)
+
+ def apply(fe: KyuubiRestFrontendService): EngineUIProxyServlet = {
+ new EngineUIProxyServlet(
+ fe.getConf.get(KyuubiConf.FRONTEND_REST_ENGINE_UI_PROXY_FILTER_ENABLED),
+ () => registeredEngineUrls(fe))
+ }
+
+ private[api] def registeredEngineUrls(fe: KyuubiRestFrontendService): Set[(String, Int)] = {
+ DiscoveryClientProvider.withDiscoveryClient(fe.getConf) { discoveryClient =>
+ registeredEngineUrls(discoveryClient, fe.getConf.get(HA_NAMESPACE))
+ }
+ }
+
+ private[api] def registeredEngineUrls(
Review Comment:
I think we should have two configs, breaking changes are acceptable as long as we document them in the migration guide.
```
kyuubi.frontend.rest.engine.ui.proxy.enabled -- disabled by default
kyuubi.frontend.rest.engine.ui.proxy.hosts -- none by default
```
1. do you think host restriction is sufficient? is port, path necessary?
2. we should at least allow a wildcard for values, e.g., `*`(admins take their own risk to set it), `hadoop*.example.org`, not sure if regex should be used (not much readable, but much more powerful)
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3322859961
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -52,17 +70,125 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
+ extractTargetAddress(requestURI).filter(isRegistered).foreach { case (host, port) =>
// SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isRegistered(target: (String, Int)): Boolean = {
+ !filterEnabled || {
+ try {
+ currentRegisteredEngineUrls().contains(normalizeHostPort(target))
+ } catch {
+ case NonFatal(e) =>
+ warn(s"Failed to validate Engine UI target ${target._1}:${target._2}", e)
+ false
+ }
+ }
+ }
+
+ private def currentRegisteredEngineUrls(): Set[(String, Int)] = {
+ val now = System.nanoTime()
+ val cacheTtlNanos = EngineUIProxyServlet.RegisteredEngineUrlsCacheTtlNanos
+ if (now - cachedAtNanos > cacheTtlNanos) {
+ synchronized {
+ val current = System.nanoTime()
+ if (current - cachedAtNanos > cacheTtlNanos) {
+ cachedRegisteredEngineUrls = registeredEngineUrls()
+ cachedAtNanos = current
+ }
+ }
+ }
+ cachedRegisteredEngineUrls
+ }
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
- private def extractTargetAddress(requestURI: String): Option[(String, Int)] =
+ private[api] def extractTargetAddress(requestURI: String): Option[(String, Int)] =
requestURI match {
case r(host, port) => Some(host -> port.toInt)
case _ => None
}
+
+ private def normalizeHostPort(hostPort: (String, Int)): (String, Int) = {
+ hostPort._1.toLowerCase(Locale.ROOT) -> hostPort._2
+ }
+}
+
+private[api] object EngineUIProxyServlet extends Logging {
+
+ // Engine UI pages load many assets through the proxy. Avoid scanning discovery for every request.
+ private val RegisteredEngineUrlsCacheTtlNanos = TimeUnit.SECONDS.toNanos(10)
+
+ def apply(fe: KyuubiRestFrontendService): EngineUIProxyServlet = {
+ new EngineUIProxyServlet(
+ fe.getConf.get(KyuubiConf.FRONTEND_REST_ENGINE_UI_PROXY_FILTER_ENABLED),
+ () => registeredEngineUrls(fe))
+ }
+
+ private[api] def registeredEngineUrls(fe: KyuubiRestFrontendService): Set[(String, Int)] = {
+ DiscoveryClientProvider.withDiscoveryClient(fe.getConf) { discoveryClient =>
+ registeredEngineUrls(discoveryClient, fe.getConf.get(HA_NAMESPACE))
+ }
+ }
+
+ private[api] def registeredEngineUrls(
Review Comment:
So the idea is to store the registered engine URL on Zookeeper? Sounds too heavy, maybe have a configurable allow list? something similar to `hadoop.proxyuser.foo.hosts`
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "wangzhigang1999 (via GitHub)" <gi...@apache.org>.
wangzhigang1999 commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3324046243
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -18,51 +18,92 @@
package org.apache.kyuubi.server.api
import java.net.URL
+import java.util.Locale
+import java.util.regex.Pattern
import javax.servlet.http.HttpServletRequest
import org.apache.commons.lang3.StringUtils
import org.eclipse.jetty.client.api.Request
import org.eclipse.jetty.proxy.ProxyServlet
import org.apache.kyuubi.Logging
+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.server.KyuubiRestFrontendService
-private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
+private[api] class EngineUIProxyServlet(
+ proxyEnabled: Boolean,
+ allowedHosts: Set[String]) extends ProxyServlet with Logging {
+
+ private val allowedHostPatterns =
+ allowedHosts.map(EngineUIProxyServlet.normalizeHost).filter(_.nonEmpty).map(
+ EngineUIProxyServlet.wildcardHostPattern)
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
- var targetURL = "/no-ui-error"
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
- // for some reason, the proxy can not handle redirect well, as a workaround,
- // we simulate the Spark UI redirection behavior and forcibly rewrite the
- // empty URI to the Spark Jobs page.
- case "" | "/" => "/jobs/"
- case path => path
- }
- val targetQueryString =
- Option(request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
- targetURL = new URL("http", host, port, targetURI + targetQueryString).toString
- }
+ // If proxying is enabled and the request URI host is not allowed, return null so Jetty
+ // responds with 403 Forbidden.
+ val targetURL = allowedTarget(requestURI).map {
+ case (host, port) =>
+ val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
+ // for some reason, the proxy can not handle redirect well, as a workaround,
+ // we simulate the Spark UI redirection behavior and forcibly rewrite the
+ // empty URI to the Spark Jobs page.
+ case "" | "/" => "/jobs/"
+ case path => path
+ }
+ val targetQueryString =
+ Option(
+ request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
+ new URL("http", host, port, targetURI + targetQueryString).toString
+ }.orNull
debug(s"rewrite $requestURL => $targetURL")
targetURL
}
override def addXForwardedHeaders(
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
- val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
- proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
+ allowedTarget(clientRequest.getRequestURI).foreach {
+ case (host, port) =>
+ // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
+ proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isAllowedHost(host: String): Boolean =
+ proxyEnabled && {
+ val normalizedHost = EngineUIProxyServlet.normalizeHost(host)
+ allowedHostPatterns.exists(_.matcher(normalizedHost).matches())
+ }
+
+ private def allowedTarget(requestURI: String): Option[(String, Int)] =
+ extractTargetAddress(requestURI).filter(target => isAllowedHost(target._1))
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
Review Comment:
Parsing host and port directly with this regex is insecure — the host can be spoofed. I did a quick verification, and perhaps we should use a more robust method to parse the host and port.
```java
import java.net.URI;
import java.net.URL;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
public class EngineUIProxyRegexPoc {
public static void main(String[] a) throws Exception {
// same regex as EngineUIProxyServlet.scala:84
Pattern r = Pattern.compile("^/engine-ui/([^/:]+):(\\d+)/?.*");
// attacker URI: smuggle the real target after '@'
String uri = "/engine-ui/spark-x@100.100.100.200:80/latest/meta-data/";
Matcher m = r.matcher(uri);
m.matches();
String host = m.group(1); // host checked against allowlist
String target = new URL("http", host, 80, "/").toString(); // same as PR
String realHost = URI.create(target).getHost(); // host Jetty actually connects to
System.out.println("checked host : " + host); // spark-x@100.100.100.200
System.out.println("connect host : " + realHost); // 100.100.100.200
System.out.println("bypass? : " + !host.equals(realHost)); // true
}
}
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3331298165
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -18,51 +18,92 @@
package org.apache.kyuubi.server.api
import java.net.URL
+import java.util.Locale
+import java.util.regex.Pattern
import javax.servlet.http.HttpServletRequest
import org.apache.commons.lang3.StringUtils
import org.eclipse.jetty.client.api.Request
import org.eclipse.jetty.proxy.ProxyServlet
import org.apache.kyuubi.Logging
+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.server.KyuubiRestFrontendService
-private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
+private[api] class EngineUIProxyServlet(
+ proxyEnabled: Boolean,
+ allowedHosts: Set[String]) extends ProxyServlet with Logging {
+
+ private val allowedHostPatterns =
+ allowedHosts.map(EngineUIProxyServlet.normalizeHost).filter(_.nonEmpty).map(
+ EngineUIProxyServlet.wildcardHostPattern)
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
- var targetURL = "/no-ui-error"
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
- // for some reason, the proxy can not handle redirect well, as a workaround,
- // we simulate the Spark UI redirection behavior and forcibly rewrite the
- // empty URI to the Spark Jobs page.
- case "" | "/" => "/jobs/"
- case path => path
- }
- val targetQueryString =
- Option(request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
- targetURL = new URL("http", host, port, targetURI + targetQueryString).toString
- }
+ // If proxying is enabled and the request URI host is not allowed, return null so Jetty
+ // responds with 403 Forbidden.
+ val targetURL = allowedTarget(requestURI).map {
+ case (host, port) =>
+ val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
+ // for some reason, the proxy can not handle redirect well, as a workaround,
+ // we simulate the Spark UI redirection behavior and forcibly rewrite the
+ // empty URI to the Spark Jobs page.
+ case "" | "/" => "/jobs/"
+ case path => path
+ }
+ val targetQueryString =
+ Option(
+ request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
+ new URL("http", host, port, targetURI + targetQueryString).toString
+ }.orNull
debug(s"rewrite $requestURL => $targetURL")
targetURL
}
override def addXForwardedHeaders(
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
- val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
- proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
+ allowedTarget(clientRequest.getRequestURI).foreach {
+ case (host, port) =>
+ // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
+ proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isAllowedHost(host: String): Boolean =
+ proxyEnabled && {
+ val normalizedHost = EngineUIProxyServlet.normalizeHost(host)
+ allowedHostPatterns.exists(_.matcher(normalizedHost).matches())
+ }
+
+ private def allowedTarget(requestURI: String): Option[(String, Int)] =
+ extractTargetAddress(requestURI).filter(target => isAllowedHost(target._1))
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
Review Comment:
Nice catch! I dropped the regex pattern and let `URI` own the parse.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3323221492
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -52,17 +70,125 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
+ extractTargetAddress(requestURI).filter(isRegistered).foreach { case (host, port) =>
// SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isRegistered(target: (String, Int)): Boolean = {
+ !filterEnabled || {
+ try {
+ currentRegisteredEngineUrls().contains(normalizeHostPort(target))
+ } catch {
+ case NonFatal(e) =>
+ warn(s"Failed to validate Engine UI target ${target._1}:${target._2}", e)
+ false
+ }
+ }
+ }
+
+ private def currentRegisteredEngineUrls(): Set[(String, Int)] = {
+ val now = System.nanoTime()
+ val cacheTtlNanos = EngineUIProxyServlet.RegisteredEngineUrlsCacheTtlNanos
+ if (now - cachedAtNanos > cacheTtlNanos) {
+ synchronized {
+ val current = System.nanoTime()
+ if (current - cachedAtNanos > cacheTtlNanos) {
+ cachedRegisteredEngineUrls = registeredEngineUrls()
+ cachedAtNanos = current
+ }
+ }
+ }
+ cachedRegisteredEngineUrls
+ }
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
- private def extractTargetAddress(requestURI: String): Option[(String, Int)] =
+ private[api] def extractTargetAddress(requestURI: String): Option[(String, Int)] =
requestURI match {
case r(host, port) => Some(host -> port.toInt)
case _ => None
}
+
+ private def normalizeHostPort(hostPort: (String, Int)): (String, Int) = {
+ hostPort._1.toLowerCase(Locale.ROOT) -> hostPort._2
+ }
+}
+
+private[api] object EngineUIProxyServlet extends Logging {
+
+ // Engine UI pages load many assets through the proxy. Avoid scanning discovery for every request.
+ private val RegisteredEngineUrlsCacheTtlNanos = TimeUnit.SECONDS.toNanos(10)
+
+ def apply(fe: KyuubiRestFrontendService): EngineUIProxyServlet = {
+ new EngineUIProxyServlet(
+ fe.getConf.get(KyuubiConf.FRONTEND_REST_ENGINE_UI_PROXY_FILTER_ENABLED),
+ () => registeredEngineUrls(fe))
+ }
+
+ private[api] def registeredEngineUrls(fe: KyuubiRestFrontendService): Set[(String, Int)] = {
+ DiscoveryClientProvider.withDiscoveryClient(fe.getConf) { discoveryClient =>
+ registeredEngineUrls(discoveryClient, fe.getConf.get(HA_NAMESPACE))
+ }
+ }
+
+ private[api] def registeredEngineUrls(
Review Comment:
I think we should have two configs, breaking changes are acceptable as long as we document them in the migration guide.
```
kyuubi.frontend.rest.engine.ui.proxy.enabled -- disabled by default
kyuubi.frontend.rest.engine.ui.proxy.hosts -- none by default
```
1. do you think host restriction is sufficient? are port, path necessary?
2. we should at least allow a wildcard for values, e.g., `*`(admins take their own risk to set it), `hadoop*.example.org`, not sure if regex should be used (not much readable, but much more powerful)
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#issuecomment-4590146243
Thank you! Added a note in the migration guide.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3323124519
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -52,17 +70,125 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
+ extractTargetAddress(requestURI).filter(isRegistered).foreach { case (host, port) =>
// SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isRegistered(target: (String, Int)): Boolean = {
+ !filterEnabled || {
+ try {
+ currentRegisteredEngineUrls().contains(normalizeHostPort(target))
+ } catch {
+ case NonFatal(e) =>
+ warn(s"Failed to validate Engine UI target ${target._1}:${target._2}", e)
+ false
+ }
+ }
+ }
+
+ private def currentRegisteredEngineUrls(): Set[(String, Int)] = {
+ val now = System.nanoTime()
+ val cacheTtlNanos = EngineUIProxyServlet.RegisteredEngineUrlsCacheTtlNanos
+ if (now - cachedAtNanos > cacheTtlNanos) {
+ synchronized {
+ val current = System.nanoTime()
+ if (current - cachedAtNanos > cacheTtlNanos) {
+ cachedRegisteredEngineUrls = registeredEngineUrls()
+ cachedAtNanos = current
+ }
+ }
+ }
+ cachedRegisteredEngineUrls
+ }
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
- private def extractTargetAddress(requestURI: String): Option[(String, Int)] =
+ private[api] def extractTargetAddress(requestURI: String): Option[(String, Int)] =
requestURI match {
case r(host, port) => Some(host -> port.toInt)
case _ => None
}
+
+ private def normalizeHostPort(hostPort: (String, Int)): (String, Int) = {
+ hostPort._1.toLowerCase(Locale.ROOT) -> hostPort._2
+ }
+}
+
+private[api] object EngineUIProxyServlet extends Logging {
+
+ // Engine UI pages load many assets through the proxy. Avoid scanning discovery for every request.
+ private val RegisteredEngineUrlsCacheTtlNanos = TimeUnit.SECONDS.toNanos(10)
+
+ def apply(fe: KyuubiRestFrontendService): EngineUIProxyServlet = {
+ new EngineUIProxyServlet(
+ fe.getConf.get(KyuubiConf.FRONTEND_REST_ENGINE_UI_PROXY_FILTER_ENABLED),
+ () => registeredEngineUrls(fe))
+ }
+
+ private[api] def registeredEngineUrls(fe: KyuubiRestFrontendService): Set[(String, Int)] = {
+ DiscoveryClientProvider.withDiscoveryClient(fe.getConf) { discoveryClient =>
+ registeredEngineUrls(discoveryClient, fe.getConf.get(HA_NAMESPACE))
+ }
+ }
+
+ private[api] def registeredEngineUrls(
Review Comment:
Probably we might need to set `*` by default for compatibility, but I feel it doesn't change the situation. Probably almost all the users rely on the default value.
Instead, can we set the default to none? and if the host is not in the allowlist, just redirect it. The traffic directly goes to the engine URL. What do you think?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "wangzhigang1999 (via GitHub)" <gi...@apache.org>.
wangzhigang1999 commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3324031625
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -18,51 +18,92 @@
package org.apache.kyuubi.server.api
import java.net.URL
+import java.util.Locale
+import java.util.regex.Pattern
import javax.servlet.http.HttpServletRequest
import org.apache.commons.lang3.StringUtils
import org.eclipse.jetty.client.api.Request
import org.eclipse.jetty.proxy.ProxyServlet
import org.apache.kyuubi.Logging
+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.server.KyuubiRestFrontendService
-private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
+private[api] class EngineUIProxyServlet(
+ proxyEnabled: Boolean,
+ allowedHosts: Set[String]) extends ProxyServlet with Logging {
+
+ private val allowedHostPatterns =
+ allowedHosts.map(EngineUIProxyServlet.normalizeHost).filter(_.nonEmpty).map(
+ EngineUIProxyServlet.wildcardHostPattern)
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
- var targetURL = "/no-ui-error"
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
- // for some reason, the proxy can not handle redirect well, as a workaround,
- // we simulate the Spark UI redirection behavior and forcibly rewrite the
- // empty URI to the Spark Jobs page.
- case "" | "/" => "/jobs/"
- case path => path
- }
- val targetQueryString =
- Option(request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
- targetURL = new URL("http", host, port, targetURI + targetQueryString).toString
- }
+ // If proxying is enabled and the request URI host is not allowed, return null so Jetty
+ // responds with 403 Forbidden.
+ val targetURL = allowedTarget(requestURI).map {
+ case (host, port) =>
+ val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
+ // for some reason, the proxy can not handle redirect well, as a workaround,
+ // we simulate the Spark UI redirection behavior and forcibly rewrite the
+ // empty URI to the Spark Jobs page.
+ case "" | "/" => "/jobs/"
+ case path => path
+ }
+ val targetQueryString =
+ Option(
+ request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
+ new URL("http", host, port, targetURI + targetQueryString).toString
+ }.orNull
debug(s"rewrite $requestURL => $targetURL")
targetURL
}
override def addXForwardedHeaders(
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
- val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
- proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
+ allowedTarget(clientRequest.getRequestURI).foreach {
+ case (host, port) =>
+ // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
+ proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isAllowedHost(host: String): Boolean =
+ proxyEnabled && {
+ val normalizedHost = EngineUIProxyServlet.normalizeHost(host)
+ allowedHostPatterns.exists(_.matcher(normalizedHost).matches())
+ }
+
+ private def allowedTarget(requestURI: String): Option[(String, Int)] =
+ extractTargetAddress(requestURI).filter(target => isAllowedHost(target._1))
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
Review Comment:
Parsing host and port directly with this regex is insecure — the host can be spoofed. I did a quick verification, and perhaps we should use a more robust method to parse the host and port.
```java
import java.net.URI;
import java.net.URL;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
public class EngineUIProxyRegexPoc {
public static void main(String[] a) throws Exception {
// same regex as EngineUIProxyServlet.scala:84
Pattern r = Pattern.compile("^/engine-ui/([^/:]+):(\\d+)/?.*");
// attacker URI: smuggle the real target after '@'
String uri = "/engine-ui/spark-x@100.100.100.200:80/latest/meta-data/";
Matcher m = r.matcher(uri);
m.matches();
String host = m.group(1); // host checked against allowlist
String target = new URL("http", host, 80, "/").toString(); // same as PR
String realHost = URI.create(target).getHost(); // host Jetty actually connects to
System.out.println("checked host : " + host); // spark-x@100.100.100.200
System.out.println("connect host : " + realHost); // 100.100.100.200
System.out.println("bypass? : " + !host.equals(realHost)); // true
}
}
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3323388168
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -52,17 +70,125 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
+ extractTargetAddress(requestURI).filter(isRegistered).foreach { case (host, port) =>
// SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isRegistered(target: (String, Int)): Boolean = {
+ !filterEnabled || {
+ try {
+ currentRegisteredEngineUrls().contains(normalizeHostPort(target))
+ } catch {
+ case NonFatal(e) =>
+ warn(s"Failed to validate Engine UI target ${target._1}:${target._2}", e)
+ false
+ }
+ }
+ }
+
+ private def currentRegisteredEngineUrls(): Set[(String, Int)] = {
+ val now = System.nanoTime()
+ val cacheTtlNanos = EngineUIProxyServlet.RegisteredEngineUrlsCacheTtlNanos
+ if (now - cachedAtNanos > cacheTtlNanos) {
+ synchronized {
+ val current = System.nanoTime()
+ if (current - cachedAtNanos > cacheTtlNanos) {
+ cachedRegisteredEngineUrls = registeredEngineUrls()
+ cachedAtNanos = current
+ }
+ }
+ }
+ cachedRegisteredEngineUrls
+ }
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
- private def extractTargetAddress(requestURI: String): Option[(String, Int)] =
+ private[api] def extractTargetAddress(requestURI: String): Option[(String, Int)] =
requestURI match {
case r(host, port) => Some(host -> port.toInt)
case _ => None
}
+
+ private def normalizeHostPort(hostPort: (String, Int)): (String, Int) = {
+ hostPort._1.toLowerCase(Locale.ROOT) -> hostPort._2
+ }
+}
+
+private[api] object EngineUIProxyServlet extends Logging {
+
+ // Engine UI pages load many assets through the proxy. Avoid scanning discovery for every request.
+ private val RegisteredEngineUrlsCacheTtlNanos = TimeUnit.SECONDS.toNanos(10)
+
+ def apply(fe: KyuubiRestFrontendService): EngineUIProxyServlet = {
+ new EngineUIProxyServlet(
+ fe.getConf.get(KyuubiConf.FRONTEND_REST_ENGINE_UI_PROXY_FILTER_ENABLED),
+ () => registeredEngineUrls(fe))
+ }
+
+ private[api] def registeredEngineUrls(fe: KyuubiRestFrontendService): Set[(String, Int)] = {
+ DiscoveryClientProvider.withDiscoveryClient(fe.getConf) { discoveryClient =>
+ registeredEngineUrls(discoveryClient, fe.getConf.get(HA_NAMESPACE))
+ }
+ }
+
+ private[api] def registeredEngineUrls(
Review Comment:
> breaking changes are acceptable as long as we document them in the migration guide.
Thanks. Good to know.
> do you think host restriction is sufficient? are port, path necessary?
I think host restriction is sufficient.
> we should at least allow a wildcard for values
+1
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3322988984
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -52,17 +70,125 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
+ extractTargetAddress(requestURI).filter(isRegistered).foreach { case (host, port) =>
// SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isRegistered(target: (String, Int)): Boolean = {
+ !filterEnabled || {
+ try {
+ currentRegisteredEngineUrls().contains(normalizeHostPort(target))
+ } catch {
+ case NonFatal(e) =>
+ warn(s"Failed to validate Engine UI target ${target._1}:${target._2}", e)
+ false
+ }
+ }
+ }
+
+ private def currentRegisteredEngineUrls(): Set[(String, Int)] = {
+ val now = System.nanoTime()
+ val cacheTtlNanos = EngineUIProxyServlet.RegisteredEngineUrlsCacheTtlNanos
+ if (now - cachedAtNanos > cacheTtlNanos) {
+ synchronized {
+ val current = System.nanoTime()
+ if (current - cachedAtNanos > cacheTtlNanos) {
+ cachedRegisteredEngineUrls = registeredEngineUrls()
+ cachedAtNanos = current
+ }
+ }
+ }
+ cachedRegisteredEngineUrls
+ }
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
- private def extractTargetAddress(requestURI: String): Option[(String, Int)] =
+ private[api] def extractTargetAddress(requestURI: String): Option[(String, Int)] =
requestURI match {
case r(host, port) => Some(host -> port.toInt)
case _ => None
}
+
+ private def normalizeHostPort(hostPort: (String, Int)): (String, Int) = {
+ hostPort._1.toLowerCase(Locale.ROOT) -> hostPort._2
+ }
+}
+
+private[api] object EngineUIProxyServlet extends Logging {
+
+ // Engine UI pages load many assets through the proxy. Avoid scanning discovery for every request.
+ private val RegisteredEngineUrlsCacheTtlNanos = TimeUnit.SECONDS.toNanos(10)
+
+ def apply(fe: KyuubiRestFrontendService): EngineUIProxyServlet = {
+ new EngineUIProxyServlet(
+ fe.getConf.get(KyuubiConf.FRONTEND_REST_ENGINE_UI_PROXY_FILTER_ENABLED),
+ () => registeredEngineUrls(fe))
+ }
+
+ private[api] def registeredEngineUrls(fe: KyuubiRestFrontendService): Set[(String, Int)] = {
+ DiscoveryClientProvider.withDiscoveryClient(fe.getConf) { discoveryClient =>
+ registeredEngineUrls(discoveryClient, fe.getConf.get(HA_NAMESPACE))
+ }
+ }
+
+ private[api] def registeredEngineUrls(
Review Comment:
> So the idea is to store the registered engine URL on Zookeeper?
In HA setup, yes.
> Sounds too heavy, maybe have a configurable allow list? something similar to hadoop.proxyuser.foo.hosts
Agreed. I'll update the patch to pass a configurable allowlist.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "wangzhigang1999 (via GitHub)" <gi...@apache.org>.
wangzhigang1999 commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3324153514
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -18,51 +18,92 @@
package org.apache.kyuubi.server.api
import java.net.URL
+import java.util.Locale
+import java.util.regex.Pattern
import javax.servlet.http.HttpServletRequest
import org.apache.commons.lang3.StringUtils
import org.eclipse.jetty.client.api.Request
import org.eclipse.jetty.proxy.ProxyServlet
import org.apache.kyuubi.Logging
+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.server.KyuubiRestFrontendService
-private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
+private[api] class EngineUIProxyServlet(
+ proxyEnabled: Boolean,
+ allowedHosts: Set[String]) extends ProxyServlet with Logging {
+
+ private val allowedHostPatterns =
+ allowedHosts.map(EngineUIProxyServlet.normalizeHost).filter(_.nonEmpty).map(
+ EngineUIProxyServlet.wildcardHostPattern)
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
- var targetURL = "/no-ui-error"
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
- // for some reason, the proxy can not handle redirect well, as a workaround,
- // we simulate the Spark UI redirection behavior and forcibly rewrite the
- // empty URI to the Spark Jobs page.
- case "" | "/" => "/jobs/"
- case path => path
- }
- val targetQueryString =
- Option(request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
- targetURL = new URL("http", host, port, targetURI + targetQueryString).toString
- }
+ // If proxying is enabled and the request URI host is not allowed, return null so Jetty
+ // responds with 403 Forbidden.
+ val targetURL = allowedTarget(requestURI).map {
+ case (host, port) =>
+ val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
+ // for some reason, the proxy can not handle redirect well, as a workaround,
+ // we simulate the Spark UI redirection behavior and forcibly rewrite the
+ // empty URI to the Spark Jobs page.
+ case "" | "/" => "/jobs/"
+ case path => path
+ }
+ val targetQueryString =
+ Option(
+ request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
+ new URL("http", host, port, targetURI + targetQueryString).toString
+ }.orNull
debug(s"rewrite $requestURL => $targetURL")
targetURL
}
override def addXForwardedHeaders(
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
- val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
- proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
+ allowedTarget(clientRequest.getRequestURI).foreach {
+ case (host, port) =>
+ // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
+ proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isAllowedHost(host: String): Boolean =
+ proxyEnabled && {
+ val normalizedHost = EngineUIProxyServlet.normalizeHost(host)
+ allowedHostPatterns.exists(_.matcher(normalizedHost).matches())
+ }
+
+ private def allowedTarget(requestURI: String): Option[(String, Int)] =
+ extractTargetAddress(requestURI).filter(target => isAllowedHost(target._1))
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
Review Comment:
Idea: drop the regex and let `URI` own the parse, so the validated host is exactly the one Jetty connects to. Roughly:
```scala
val rest = requestURI.stripPrefix("/engine-ui/")
val authority = rest.indexOf('/') match { case -1 => rest; case i => rest.substring(0, i) }
val u = URI.create(s"http://$authority") // may throw -> deny
// reject smuggling / invalid targets
if (u.getUserInfo != null || u.getHost == null || u.getPort < 0) return denied
// build the target ONLY from the validated host/port, never from the raw string
// (getHost already brackets IPv6 literals; avoids the deprecated java.net.URL ctor)
val target = s"http://${u.getHost}:${u.getPort}$path$query"
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3322859961
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -52,17 +70,125 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
+ extractTargetAddress(requestURI).filter(isRegistered).foreach { case (host, port) =>
// SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isRegistered(target: (String, Int)): Boolean = {
+ !filterEnabled || {
+ try {
+ currentRegisteredEngineUrls().contains(normalizeHostPort(target))
+ } catch {
+ case NonFatal(e) =>
+ warn(s"Failed to validate Engine UI target ${target._1}:${target._2}", e)
+ false
+ }
+ }
+ }
+
+ private def currentRegisteredEngineUrls(): Set[(String, Int)] = {
+ val now = System.nanoTime()
+ val cacheTtlNanos = EngineUIProxyServlet.RegisteredEngineUrlsCacheTtlNanos
+ if (now - cachedAtNanos > cacheTtlNanos) {
+ synchronized {
+ val current = System.nanoTime()
+ if (current - cachedAtNanos > cacheTtlNanos) {
+ cachedRegisteredEngineUrls = registeredEngineUrls()
+ cachedAtNanos = current
+ }
+ }
+ }
+ cachedRegisteredEngineUrls
+ }
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
- private def extractTargetAddress(requestURI: String): Option[(String, Int)] =
+ private[api] def extractTargetAddress(requestURI: String): Option[(String, Int)] =
requestURI match {
case r(host, port) => Some(host -> port.toInt)
case _ => None
}
+
+ private def normalizeHostPort(hostPort: (String, Int)): (String, Int) = {
+ hostPort._1.toLowerCase(Locale.ROOT) -> hostPort._2
+ }
+}
+
+private[api] object EngineUIProxyServlet extends Logging {
+
+ // Engine UI pages load many assets through the proxy. Avoid scanning discovery for every request.
+ private val RegisteredEngineUrlsCacheTtlNanos = TimeUnit.SECONDS.toNanos(10)
+
+ def apply(fe: KyuubiRestFrontendService): EngineUIProxyServlet = {
+ new EngineUIProxyServlet(
+ fe.getConf.get(KyuubiConf.FRONTEND_REST_ENGINE_UI_PROXY_FILTER_ENABLED),
+ () => registeredEngineUrls(fe))
+ }
+
+ private[api] def registeredEngineUrls(fe: KyuubiRestFrontendService): Set[(String, Int)] = {
+ DiscoveryClientProvider.withDiscoveryClient(fe.getConf) { discoveryClient =>
+ registeredEngineUrls(discoveryClient, fe.getConf.get(HA_NAMESPACE))
+ }
+ }
+
+ private[api] def registeredEngineUrls(
Review Comment:
So the idea is to store the registered engine URL on Zookeeper? Sounds too heavy, may have a configurable allow list? something similar to `hadoop.proxyuser.foo.hosts`
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "wangzhigang1999 (via GitHub)" <gi...@apache.org>.
wangzhigang1999 commented on code in PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#discussion_r3324052632
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -18,51 +18,92 @@
package org.apache.kyuubi.server.api
import java.net.URL
+import java.util.Locale
+import java.util.regex.Pattern
import javax.servlet.http.HttpServletRequest
import org.apache.commons.lang3.StringUtils
import org.eclipse.jetty.client.api.Request
import org.eclipse.jetty.proxy.ProxyServlet
import org.apache.kyuubi.Logging
+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.server.KyuubiRestFrontendService
-private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
+private[api] class EngineUIProxyServlet(
+ proxyEnabled: Boolean,
+ allowedHosts: Set[String]) extends ProxyServlet with Logging {
+
+ private val allowedHostPatterns =
+ allowedHosts.map(EngineUIProxyServlet.normalizeHost).filter(_.nonEmpty).map(
+ EngineUIProxyServlet.wildcardHostPattern)
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
- var targetURL = "/no-ui-error"
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
- // for some reason, the proxy can not handle redirect well, as a workaround,
- // we simulate the Spark UI redirection behavior and forcibly rewrite the
- // empty URI to the Spark Jobs page.
- case "" | "/" => "/jobs/"
- case path => path
- }
- val targetQueryString =
- Option(request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
- targetURL = new URL("http", host, port, targetURI + targetQueryString).toString
- }
+ // If proxying is enabled and the request URI host is not allowed, return null so Jetty
+ // responds with 403 Forbidden.
+ val targetURL = allowedTarget(requestURI).map {
+ case (host, port) =>
+ val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
+ // for some reason, the proxy can not handle redirect well, as a workaround,
+ // we simulate the Spark UI redirection behavior and forcibly rewrite the
+ // empty URI to the Spark Jobs page.
+ case "" | "/" => "/jobs/"
+ case path => path
+ }
+ val targetQueryString =
+ Option(
+ request.getQueryString).filter(StringUtils.isNotEmpty).map(q => s"?$q").getOrElse("")
+ new URL("http", host, port, targetURI + targetQueryString).toString
+ }.orNull
debug(s"rewrite $requestURL => $targetURL")
targetURL
}
override def addXForwardedHeaders(
clientRequest: HttpServletRequest,
proxyRequest: Request): Unit = {
- val requestURI = clientRequest.getRequestURI
- extractTargetAddress(requestURI).foreach { case (host, port) =>
- // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
- proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
+ allowedTarget(clientRequest.getRequestURI).foreach {
+ case (host, port) =>
+ // SPARK-24209: Knox uses X-Forwarded-Context to notify the application the base path
+ proxyRequest.header("X-Forwarded-Context", s"/engine-ui/$host:$port")
}
super.addXForwardedHeaders(clientRequest, proxyRequest)
}
+ private def isAllowedHost(host: String): Boolean =
+ proxyEnabled && {
+ val normalizedHost = EngineUIProxyServlet.normalizeHost(host)
+ allowedHostPatterns.exists(_.matcher(normalizedHost).matches())
+ }
+
+ private def allowedTarget(requestURI: String): Option[(String, Int)] =
+ extractTargetAddress(requestURI).filter(target => isAllowedHost(target._1))
+
private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
Review Comment:
> private val r = "^/engine-ui/([^/:]+):(\\d+)/?.*".r
In fact, I want to refer to this line of code.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on PR #7483:
URL: https://github.com/apache/kyuubi/pull/7483#issuecomment-4599353554
Merged. Thank you @pan3793 and @wangzhigang1999 !
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] Restrict Engine UI proxy targets [kyuubi]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka closed pull request #7483: Restrict Engine UI proxy targets
URL: https://github.com/apache/kyuubi/pull/7483
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org