dongjoon-hyun commented on code in PR #58640:
URL: https://github.com/apache/spark/pull/58640#discussion_r3985599330
##########
core/src/main/scala/org/apache/spark/internal/config/UI.scala:
##########
@@ -105,6 +105,21 @@ private[spark] object UI {
.booleanConf
.createWithDefault(true)
+ val UI_KILL_VIA_GET_ENABLED = ConfigBuilder("spark.ui.killViaGetEnabled")
Review Comment:
This new config needs `.withBindingPolicy(...)`. This is the cause of the
`SparkConfigBindingPolicySuite` failure (`Config enforcement for
bindingPolicy`) in the `hive - other tests` job. Like `UI_HOLD_ENABLED` above,
`.withBindingPolicy(ConfigBindingPolicy.NOT_APPLICABLE)` should work.
##########
core/src/main/scala/org/apache/spark/ui/SparkUI.scala:
##########
@@ -121,16 +124,24 @@ private[spark] class SparkUI private (
attachHandler(PrometheusResource.getServletHandler(this))
}
- // These should be POST only, but, the YARN AM proxy won't proxy POSTs
+ // These endpoints change state, so they require the per-UI CSRF token and
reject
+ // prefetch requests (see JettyUtils.createRedirectHandler). Kill also
accepts GET
+ // where a proxy in front of the UI cannot forward POST -- the token rides
in the
+ // request parameters, which such proxies do forward. See
SparkUI.killViaGetEnabled.
+ val killHttpMethods: Set[String] =
+ if (killViaGetEnabled) Set("GET", "POST") else Set("POST")
attachHandler(createRedirectHandler(
- "/jobs/job/kill", "/jobs/", jobsTab.handleKillRequest, httpMethods =
Set("GET", "POST")))
+ "/jobs/job/kill", "/jobs/", jobsTab.handleKillRequest, httpMethods =
killHttpMethods,
+ csrfToken = Some(csrfToken)))
attachHandler(createRedirectHandler(
"/stages/stage/kill", "/stages/", stagesTab.handleKillRequest,
- httpMethods = Set("GET", "POST")))
+ httpMethods = killHttpMethods, csrfToken = Some(csrfToken)))
attachHandler(createRedirectHandler(
- "/jobs/hold", "/jobs/", jobsTab.handleHoldRequest, httpMethods =
Set("GET", "POST")))
+ "/jobs/hold", "/jobs/", jobsTab.handleHoldRequest, httpMethods =
Set("GET", "POST"),
Review Comment:
`/jobs/hold` and `/jobs/resume` still accept GET regardless of
`spark.ui.killViaGetEnabled`, and `AllJobsPage` renders them as `<a
href=".../jobs/hold/?csrfToken=...">` links. Since the token is in the link, a
crawler or agent that follows the links of the jobs page without prefetch
headers can still hold the application outside YARN, which is the scenario this
PR aims to prevent. Shall we apply the same GET/POST policy (a POST form in
POST-only mode) to hold/resume? In that case, a config name that is not limited
to `kill` may fit better.
##########
core/src/main/scala/org/apache/spark/internal/config/UI.scala:
##########
@@ -105,6 +105,21 @@ private[spark] object UI {
.booleanConf
.createWithDefault(true)
+ val UI_KILL_VIA_GET_ENABLED = ConfigBuilder("spark.ui.killViaGetEnabled")
+ .doc("Whether the job/stage kill endpoints of the web UI accept HTTP GET
requests in " +
+ "addition to POST. Unset, this defaults to true when spark.master is
yarn, because " +
+ "the YARN ResourceManager/AM proxy does not forward POST requests
(SPARK-6846), and " +
+ "to false everywhere else. Either way the state-changing endpoints
require the " +
+ "random per-UI CSRF token embedded in the links and forms the UI
renders, and " +
+ "reject prefetch requests (Purpose/Sec-Purpose/X-Moz headers) and HEAD
requests, so " +
+ "forged cross-site requests and incidental link fetches cannot trigger
them; " +
+ "prefetch rejection relies on the prefetcher identifying itself via
those headers. " +
+ "Introduced in 4.3.0; also available in 3.5.10, 4.0.5, 4.1.4 and 4.2.1;
and in all " +
Review Comment:
Could you remove this sentence here and in `docs/configuration.md`? Spark
docs don't list backport versions in the description, and these releases don't
exist yet. In addition, this patch cannot be backported as-is:
`java.util.HexFormat` requires Java 17 (`branch-3.5` is on Java 8), and the
hold/resume endpoints exist only since 4.4.0.
##########
core/src/test/scala/org/apache/spark/ui/UISeleniumSuite.scala:
##########
@@ -593,28 +594,151 @@ class UISeleniumSuite extends SparkFunSuite with
WebBrowser with Matchers {
}
}
- test("kill stage POST/GET response is correct") {
+ // The state-changing endpoints require the per-UI token rendered into the
kill links
+ // (or the hidden form field in POST-only mode); scrape it the way a
scripted client
+ // would.
+ private def scrapeCsrfToken(sc: SparkContext): String = {
+ val html = Utils.tryWithResource(
+ Source.fromURL(sc.ui.get.webUrl.stripSuffix("/") + "/jobs/"))(_.mkString)
+ """(?:csrfToken=|name="csrfToken" value=")([0-9a-f]+)""".r
+ .findFirstMatchIn(html)
+ .map(_.group(1))
+ .getOrElse(fail("no CSRF token found on the jobs page"))
+ }
+
+ test("kill stage requires the CSRF token, rejecting prefetch and HEAD
requests") {
+ // GET mode is off by default outside YARN, and this test is about the
token rather than
+ // the default, so ask for GET explicitly.
+ withSpark(newSparkContext(killEnabled = true,
+ additionalConfs = Map(UI_KILL_VIA_GET_ENABLED.key -> "true"))) { sc =>
+ sc.parallelize(1 to 10).map{x => Thread.sleep(10000); x}.countAsync()
+ // java.net.HttpURLConnection silently drops some headers, so use
Jetty's client
+ // for requests that must carry specific prefetch headers.
+ val client = new HttpClient()
+ client.start()
+ try {
+ val base = sc.ui.get.webUrl.stripSuffix("/")
+ // Retry only until the kill link appears. Everything after this runs
once: a request
+ // the endpoint accepts kills the job, and the link is then gone, so
retrying the whole
+ // block could never succeed a second time -- it would fail in
scrapeCsrfToken and
+ // report a missing token rather than whatever actually went wrong.
+ val token = eventually(timeout(5.seconds),
interval(50.milliseconds))(scrapeCsrfToken(sc))
+ val noToken = new URI(base + "/stages/stage/kill/?id=0").toURL
+ val withToken = new URI(
+ base + s"/stages/stage/kill/?id=0&csrfToken=$token").toURL
+ // Forged or scripted requests without the token, or with a wrong one,
fail.
+ TestUtils.httpResponseCode(noToken, "GET") should be (403)
+ TestUtils.httpResponseCode(
+ new URI(base + "/stages/stage/kill/?id=0&csrfToken=bogus").toURL,
+ "GET") should be (403)
+ // HEAD must be safe (RFC 9110), so it is refused rather than
delegated to doGet.
+ TestUtils.httpResponseCode(withToken, "HEAD") should be (405)
+ // ...but HEAD still works on unguarded redirect handlers ("/" ->
"/jobs/").
+ TestUtils.httpResponseCode(new URI(base + "/").toURL, "HEAD") should
be (200)
+ // Browser link prefetchers identify themselves; a valid token must not
+ // save them, since the token rides in the link they prefetch.
+ client.newRequest(withToken.toURI).headers(
+ _.add("Sec-Purpose", "prefetch")).send().getStatus should be (403)
+ client.newRequest(withToken.toURI).headers(
+ _.add("Purpose", "prefetch")).send().getStatus should be (403)
+ client.newRequest(withToken.toURI).headers(
+ _.add("X-Moz", "prefetch")).send().getStatus should be (403)
+ // Last, because a deliberate click from the UI carries the token,
goes through, and
+ // kills the stage.
+ TestUtils.httpResponseCode(withToken, "GET") should be (200)
+ TestUtils.httpResponseCode(withToken, "POST") should be (200)
+ } finally {
+ client.stop()
+ }
+ }
+ }
+
+ test("kill job requires the CSRF token") {
+ withSpark(newSparkContext(killEnabled = true,
+ additionalConfs = Map(UI_KILL_VIA_GET_ENABLED.key -> "true"))) { sc =>
+ sc.parallelize(1 to 10).map{x => Thread.sleep(10000); x}.countAsync()
+ val base = sc.ui.get.webUrl.stripSuffix("/")
+ // Retry only until the kill link appears; the accepted requests below
kill the job.
+ val token = eventually(timeout(5.seconds),
interval(50.milliseconds))(scrapeCsrfToken(sc))
+ TestUtils.httpResponseCode(
+ new URI(base + "/jobs/job/kill/?id=0").toURL, "GET") should be (403)
+ TestUtils.httpResponseCode(
+ new URI(base + s"/jobs/job/kill/?id=0&csrfToken=$token").toURL,
+ "GET") should be (200)
+ TestUtils.httpResponseCode(
+ new URI(base + s"/jobs/job/kill/?id=0&csrfToken=$token").toURL,
+ "POST") should be (200)
+ }
+ }
+
+ test("kill stage is POST-only when spark.ui.killViaGetEnabled is disabled") {
+ withSpark(newSparkContext(killEnabled = true,
+ additionalConfs = Map(UI_KILL_VIA_GET_ENABLED.key -> "false"))) { sc =>
+ sc.parallelize(1 to 10).map{x => Thread.sleep(10000); x}.countAsync()
+ val client = new HttpClient()
+ client.start()
+ try {
+ eventually(timeout(5.seconds), interval(50.milliseconds)) {
+ val base = sc.ui.get.webUrl.stripSuffix("/")
+ val token = scrapeCsrfToken(sc)
+ val withToken = new URI(
+ base + s"/stages/stage/kill/?id=0&csrfToken=$token").toURL
+ TestUtils.httpResponseCode(withToken, "HEAD") should be (405)
+ TestUtils.httpResponseCode(withToken, "GET") should be (405)
+ TestUtils.httpResponseCode(withToken, "POST") should be (200)
+ // The browser path in this mode: the form posts id and token in the
body.
+ client.POST(new URI(base + "/stages/stage/kill/")).body(
+ new StringRequestContent("application/x-www-form-urlencoded",
+ s"id=0&csrfToken=$token")).send().getStatus should be (200)
+ }
+ } finally {
+ client.stop()
+ }
+ }
+ }
+
+ test("hold and resume endpoints require the CSRF token") {
withSpark(newSparkContext(killEnabled = true)) { sc =>
sc.parallelize(1 to 10).map{x => Thread.sleep(10000); x}.countAsync()
eventually(timeout(5.seconds), interval(50.milliseconds)) {
- val url = new URI(
- sc.ui.get.webUrl.stripSuffix("/") + "/stages/stage/kill/?id=0").toURL
- // SPARK-6846: should be POST only but YARN AM doesn't proxy POST
- TestUtils.httpResponseCode(url, "GET") should be (200)
- TestUtils.httpResponseCode(url, "POST") should be (200)
+ val base = sc.ui.get.webUrl.stripSuffix("/")
+ val token = scrapeCsrfToken(sc)
+ // holdEnabled is off in this context, so the actions themselves
no-op; what
Review Comment:
nit. `spark.ui.holdEnabled` is `true` by default. These requests are no-ops
because `executorHoldSupported` is `false` in `local` mode.
##########
core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala:
##########
@@ -613,11 +618,27 @@ private[ui] class JobPagedTable(
val job = jobTableRow.jobData
val killLink = if (killEnabled) {
- // SPARK-6846 this should be POST-only but YARN AM won't proxy POST
- val killLinkUri = s"$basePath/jobs/job/kill/?id=${job.jobId}"
- <a href={killLinkUri} role="button"
- data-kill-message={s"Are you sure you want to kill job ${job.jobId}
?"}
- class="btn btn-sm btn-outline-danger kill-link float-end">Kill</a>
+ val killMessage = s"Are you sure you want to kill job ${job.jobId} ?"
+ if (killViaGetEnabled) {
+ // Default: a plain GET link, which also works through proxies that do
not forward
Review Comment:
nit. This comment is stale because GET is no longer the default outside
YARN. The same comment exists in `StageTable.scala`.
##########
docs/configuration.md:
##########
@@ -1666,6 +1666,30 @@ Apart from these, the following properties are also
available, and may be useful
</td>
<td>1.0.0</td>
</tr>
+<tr>
+ <td><code>spark.ui.killViaGetEnabled</code></td>
+ <td><code>true</code> on YARN, <code>false</code> otherwise</td>
+ <td>
+ Whether the job/stage kill endpoints of the web UI accept HTTP GET
requests in
+ addition to POST. Left unset, this follows the cluster manager: GET is
accepted when
+ <code>spark.master</code> is <code>yarn</code>, because the YARN
ResourceManager/AM
+ proxy does not forward POST requests, and refused everywhere else.
+ Either way the state-changing endpoints require the random per-UI CSRF
token embedded
+ in the links and forms the UI renders, and reject prefetch requests
(identified by
+ the Purpose, Sec-Purpose, or X-Moz headers) and HEAD requests, so forged
cross-site
+ requests and incidental link fetches cannot trigger them. Scripted clients
can read
Review Comment:
`docs/monitoring.md` (`/applications/[app-id]/holdstatus`) still describes
`/jobs/hold/` and `/jobs/resume/` as POST endpoints which only require modify
permissions. Could you mention the `csrfToken` requirement there too?
##########
core/src/main/scala/org/apache/spark/internal/config/UI.scala:
##########
@@ -105,6 +105,21 @@ private[spark] object UI {
.booleanConf
.createWithDefault(true)
+ val UI_KILL_VIA_GET_ENABLED = ConfigBuilder("spark.ui.killViaGetEnabled")
+ .doc("Whether the job/stage kill endpoints of the web UI accept HTTP GET
requests in " +
+ "addition to POST. Unset, this defaults to true when spark.master is
yarn, because " +
+ "the YARN ResourceManager/AM proxy does not forward POST requests
(SPARK-6846), and " +
+ "to false everywhere else. Either way the state-changing endpoints
require the " +
+ "random per-UI CSRF token embedded in the links and forms the UI
renders, and " +
+ "reject prefetch requests (Purpose/Sec-Purpose/X-Moz headers) and HEAD
requests, so " +
+ "forged cross-site requests and incidental link fetches cannot trigger
them; " +
+ "prefetch rejection relies on the prefetcher identifying itself via
those headers. " +
+ "Introduced in 4.3.0; also available in 3.5.10, 4.0.5, 4.1.4 and 4.2.1;
and in all " +
+ "versions after 4.3.0.")
+ .version("4.3.0")
Review Comment:
`4.3.0` is not a valid version for a new config. `branch-4.3` already has
`v4.3.0-rc1`, and `branch-4.x` is `4.4.0`. Please use `4.4.0` here and in
`docs/configuration.md`.
##########
core/src/test/scala/org/apache/spark/ui/UISeleniumSuite.scala:
##########
@@ -593,28 +594,151 @@ class UISeleniumSuite extends SparkFunSuite with
WebBrowser with Matchers {
}
}
- test("kill stage POST/GET response is correct") {
+ // The state-changing endpoints require the per-UI token rendered into the
kill links
+ // (or the hidden form field in POST-only mode); scrape it the way a
scripted client
+ // would.
+ private def scrapeCsrfToken(sc: SparkContext): String = {
+ val html = Utils.tryWithResource(
+ Source.fromURL(sc.ui.get.webUrl.stripSuffix("/") + "/jobs/"))(_.mkString)
+ """(?:csrfToken=|name="csrfToken" value=")([0-9a-f]+)""".r
+ .findFirstMatchIn(html)
+ .map(_.group(1))
+ .getOrElse(fail("no CSRF token found on the jobs page"))
+ }
+
+ test("kill stage requires the CSRF token, rejecting prefetch and HEAD
requests") {
+ // GET mode is off by default outside YARN, and this test is about the
token rather than
+ // the default, so ask for GET explicitly.
+ withSpark(newSparkContext(killEnabled = true,
+ additionalConfs = Map(UI_KILL_VIA_GET_ENABLED.key -> "true"))) { sc =>
+ sc.parallelize(1 to 10).map{x => Thread.sleep(10000); x}.countAsync()
+ // java.net.HttpURLConnection silently drops some headers, so use
Jetty's client
+ // for requests that must carry specific prefetch headers.
+ val client = new HttpClient()
+ client.start()
+ try {
+ val base = sc.ui.get.webUrl.stripSuffix("/")
+ // Retry only until the kill link appears. Everything after this runs
once: a request
+ // the endpoint accepts kills the job, and the link is then gone, so
retrying the whole
+ // block could never succeed a second time -- it would fail in
scrapeCsrfToken and
+ // report a missing token rather than whatever actually went wrong.
+ val token = eventually(timeout(5.seconds),
interval(50.milliseconds))(scrapeCsrfToken(sc))
+ val noToken = new URI(base + "/stages/stage/kill/?id=0").toURL
+ val withToken = new URI(
+ base + s"/stages/stage/kill/?id=0&csrfToken=$token").toURL
+ // Forged or scripted requests without the token, or with a wrong one,
fail.
+ TestUtils.httpResponseCode(noToken, "GET") should be (403)
+ TestUtils.httpResponseCode(
+ new URI(base + "/stages/stage/kill/?id=0&csrfToken=bogus").toURL,
+ "GET") should be (403)
+ // HEAD must be safe (RFC 9110), so it is refused rather than
delegated to doGet.
+ TestUtils.httpResponseCode(withToken, "HEAD") should be (405)
+ // ...but HEAD still works on unguarded redirect handlers ("/" ->
"/jobs/").
+ TestUtils.httpResponseCode(new URI(base + "/").toURL, "HEAD") should
be (200)
+ // Browser link prefetchers identify themselves; a valid token must not
+ // save them, since the token rides in the link they prefetch.
+ client.newRequest(withToken.toURI).headers(
+ _.add("Sec-Purpose", "prefetch")).send().getStatus should be (403)
+ client.newRequest(withToken.toURI).headers(
+ _.add("Purpose", "prefetch")).send().getStatus should be (403)
+ client.newRequest(withToken.toURI).headers(
+ _.add("X-Moz", "prefetch")).send().getStatus should be (403)
+ // Last, because a deliberate click from the UI carries the token,
goes through, and
+ // kills the stage.
+ TestUtils.httpResponseCode(withToken, "GET") should be (200)
+ TestUtils.httpResponseCode(withToken, "POST") should be (200)
+ } finally {
+ client.stop()
+ }
+ }
+ }
+
+ test("kill job requires the CSRF token") {
+ withSpark(newSparkContext(killEnabled = true,
+ additionalConfs = Map(UI_KILL_VIA_GET_ENABLED.key -> "true"))) { sc =>
+ sc.parallelize(1 to 10).map{x => Thread.sleep(10000); x}.countAsync()
+ val base = sc.ui.get.webUrl.stripSuffix("/")
+ // Retry only until the kill link appears; the accepted requests below
kill the job.
+ val token = eventually(timeout(5.seconds),
interval(50.milliseconds))(scrapeCsrfToken(sc))
+ TestUtils.httpResponseCode(
+ new URI(base + "/jobs/job/kill/?id=0").toURL, "GET") should be (403)
+ TestUtils.httpResponseCode(
+ new URI(base + s"/jobs/job/kill/?id=0&csrfToken=$token").toURL,
+ "GET") should be (200)
+ TestUtils.httpResponseCode(
+ new URI(base + s"/jobs/job/kill/?id=0&csrfToken=$token").toURL,
+ "POST") should be (200)
+ }
+ }
+
+ test("kill stage is POST-only when spark.ui.killViaGetEnabled is disabled") {
+ withSpark(newSparkContext(killEnabled = true,
+ additionalConfs = Map(UI_KILL_VIA_GET_ENABLED.key -> "false"))) { sc =>
+ sc.parallelize(1 to 10).map{x => Thread.sleep(10000); x}.countAsync()
+ val client = new HttpClient()
+ client.start()
+ try {
+ eventually(timeout(5.seconds), interval(50.milliseconds)) {
Review Comment:
This test still runs the requests that kill the stage inside `eventually`,
which is the pattern fixed by the second commit for the other tests. If an
assertion fails after the first successful POST, every retry fails in
`scrapeCsrfToken` because the kill form is gone. Could you retry only the token
scraping here too?
--
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]
