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/[email protected]: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); //
[email protected]
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: [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]
