[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/19419 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144880059 --- Diff: docs/security.md --- @@ -186,7 +186,54 @@ configure those ports. +### HTTP Security Headers + +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning + + spark.ui.xXssProtection + None + +Value for HTTP X-XSS-Protection response header. You can choose appropriate value +from below: + + 0 (Disables XSS filtering) --- End diff -- @srowen Thanks for the excellent tip. It looks more readable now. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144816893 --- Diff: docs/security.md --- @@ -186,7 +186,54 @@ configure those ports. +### HTTP Security Headers + +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning + + spark.ui.xXssProtection + None + +Value for HTTP X-XSS-Protection response header. You can choose appropriate value +from below: + + 0 (Disables XSS filtering) --- End diff -- Now you have text outside the list item. This won't render correctly. This should be for example: ``` 0 (Disables XSS filtering) ``` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144787904 --- Diff: docs/security.md --- @@ -186,7 +186,54 @@ configure those ports. +### HTTP Security Headers + +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning --- End diff -- @jerryshao Fixed the indentation issue as per other Spark docs. Thanks for pointing that out. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user jerryshao commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144775941 --- Diff: docs/security.md --- @@ -186,7 +186,54 @@ configure those ports. +### HTTP Security Headers + +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning --- End diff -- I think in Spark we follow 2 space indent for html code. You could refer to other docs. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144768513 --- Diff: docs/configuration.md --- @@ -2013,7 +2013,6 @@ Apart from these, the following properties are also available, and may be useful - --- End diff -- Restored the change. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144768206 --- Diff: docs/security.md --- @@ -186,7 +186,52 @@ configure those ports. +### HTTP Security Headers + +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning + +spark.ui.xXssProtection +None + +Value for HTTP X-XSS-Protection response header. You can choose appropriate value +from below: + --- End diff -- @srowen Made them bulleted list and closed the tag as well. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144689872 --- Diff: docs/configuration.md --- @@ -2013,7 +2013,6 @@ Apart from these, the following properties are also available, and may be useful - --- End diff -- If you have to change the pull request again, I'd revert this, but no need to change it only for this --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144689866 --- Diff: docs/security.md --- @@ -186,7 +186,52 @@ configure those ports. +### HTTP Security Headers + +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning + +spark.ui.xXssProtection +None + +Value for HTTP X-XSS-Protection response header. You can choose appropriate value +from below: + --- End diff -- Why not just leave this as a bulleted list? Not a big deal I guess just less conventional for HTML --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r144565082
--- Diff:
core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -420,6 +420,25 @@ package object config {
.toSequence
.createWithDefault(Nil)
+
+ private[spark] val UI_X_XSS_PROTECTION =
+ConfigBuilder("spark.ui.xXssProtection")
+ .doc("Value for HTTP X-XSS-Protection response header")
+ .stringConf
+ .createOptional
+
+ private[spark] val UI_X_CONTENT_TYPE_OPTIONS =
+ConfigBuilder("spark.ui.xContentTypeOptions.enabled")
+ .doc("Set to 'true' for setting X-Content-Type-Options HTTP response
header to 'nosniff'")
+ .stringConf
--- End diff --
Making it to Boolean.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144495051 --- Diff: docs/configuration.md --- @@ -2013,7 +2013,62 @@ Apart from these, the following properties are also available, and may be useful +### HTTP Security Headers +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning + +spark.ui.xXssProtection +None + +Value for HTTP X-XSS-Protection response header. You can choose appropriate value +from below: + + 0 (Disables XSS filtering) --- End diff -- Removed the lists, however it is being used in many places in existing configuration.md --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144495102 --- Diff: docs/configuration.md --- @@ -2013,7 +2013,62 @@ Apart from these, the following properties are also available, and may be useful +### HTTP Security Headers +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning + +spark.ui.xXssProtection +None + +Value for HTTP X-XSS-Protection response header. You can choose appropriate value +from below: + + 0 (Disables XSS filtering) + 1 (Enables XSS filtering. If a cross-site scripting attack is detected, +the browser will sanitize the page.) + 1; mode=block (Enables XSS filtering. The browser will prevent rendering +of the page if an attack is detected.) + + + + +spark.ui.allowFramingFrom +SAMEORIGIN + +Value for X-Frame-Options HTTP response header +You can provide the "website uri" which can only be displayed in a frame on +the specified origin. + --- End diff -- Done. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user jerryshao commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144488504 --- Diff: docs/configuration.md --- @@ -2013,7 +2013,62 @@ Apart from these, the following properties are also available, and may be useful +### HTTP Security Headers --- End diff -- I think this could move to `security.md` as a security related advanced configurations. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144483296 --- Diff: docs/configuration.md --- @@ -2013,7 +2013,62 @@ Apart from these, the following properties are also available, and may be useful +### HTTP Security Headers +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning + +spark.ui.xXssProtection +None + +Value for HTTP X-XSS-Protection response header. You can choose appropriate value +from below: + + 0 (Disables XSS filtering) --- End diff -- Markdown won't work here? I think it won't. Close the `` tags though. It's not clear below what the values the users sets though. The values are `0`, `1`, or `1; mode=block` and that should be clear. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144483342 --- Diff: docs/configuration.md --- @@ -2013,7 +2013,62 @@ Apart from these, the following properties are also available, and may be useful +### HTTP Security Headers +Apache Spark can be configured to include HTTP Headers which aids in preventing Cross +Site Scripting (XSS), Cross-Frame Scripting (XFS), MIME-Sniffing and also enforces HTTP +Strict Transport Security. + + +Property NameDefaultMeaning + +spark.ui.xXssProtection +None + +Value for HTTP X-XSS-Protection response header. You can choose appropriate value +from below: + + 0 (Disables XSS filtering) + 1 (Enables XSS filtering. If a cross-site scripting attack is detected, +the browser will sanitize the page.) + 1; mode=block (Enables XSS filtering. The browser will prevent rendering +of the page if an attack is detected.) + + + + +spark.ui.allowFramingFrom +SAMEORIGIN + +Value for X-Frame-Options HTTP response header +You can provide the "website uri" which can only be displayed in a frame on +the specified origin. + --- End diff -- Remove this --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r144483070
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +90,14 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+
conf.get(UI_X_XSS_PROTECTION).foreach(response.setHeader("X-XSS-Protection", _))
+if (conf.get(UI_X_CONTENT_TYPE_OPTIONS.key).equals("true")) {
--- End diff --
At the moment this would fail if unset
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r144482977
--- Diff:
core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -420,6 +420,25 @@ package object config {
.toSequence
.createWithDefault(Nil)
+
+ private[spark] val UI_X_XSS_PROTECTION =
+ConfigBuilder("spark.ui.xXssProtection")
+ .doc("Value for HTTP X-XSS-Protection response header")
+ .stringConf
+ .createOptional
+
+ private[spark] val UI_X_CONTENT_TYPE_OPTIONS =
+ConfigBuilder("spark.ui.xContentTypeOptions.enabled")
+ .doc("Set to 'true' for setting X-Content-Type-Options HTTP response
header to 'nosniff'")
+ .stringConf
--- End diff --
This should be a boolean option then?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r144348004
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,13 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+
xXssProtectionValue.foreach(response.setHeader("X-XSS-Protection", _))
+if (xContentTypeOptionsValue.get.equalsIgnoreCase("true")) {
+ response.setHeader("X-Content-Type-Options", "nosniff")
+}
+if (conf.get("spark.ssl.enabled").equalsIgnoreCase("true")) {
--- End diff --
@jerryshao Thanks for the tip. I will do that.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r144286844
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -79,6 +79,9 @@ private[spark] object JettyUtils extends Logging {
val allowFramingFrom = conf.getOption("spark.ui.allowFramingFrom")
val xFrameOptionsValue =
allowFramingFrom.map(uri => s"ALLOW-FROM
$uri").getOrElse("SAMEORIGIN")
+val xXssProtectionValue = conf.getOption("spark.ui.xXssProtection")
--- End diff --
It follows new code convention, newly added configurations is better to
change to that way.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r144283398
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,13 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+
xXssProtectionValue.foreach(response.setHeader("X-XSS-Protection", _))
+if (xContentTypeOptionsValue.get.equalsIgnoreCase("true")) {
+ response.setHeader("X-Content-Type-Options", "nosniff")
+}
+if (conf.get("spark.ssl.enabled").equalsIgnoreCase("true")) {
--- End diff --
I think you can check `request.scheme` if it is "https" or not?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r144281816
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,9 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+
xXssProtectionValue.foreach(response.setHeader("X-XSS-Protection", _))
+
xContentTypeOptionsValue.foreach(response.setHeader("X-Content-Type-Options",
_))
+
strictTransportSecurityValue.foreach(response.setHeader("Strict-Transport-Security",
_))
--- End diff --
@jerryshao I added a check to set the STS header, only if SSL is enabled.
However, STS header was not stopping Browser from rendering the content even
when it was set and Spark was running on plain HTTP. Need to get expected
behaviour here.
https://user-images.githubusercontent.com/6433184/31497151-ba7f7dac-af7b-11e7-99ea-88b64c778b51.png;>
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r144262853
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -79,6 +79,9 @@ private[spark] object JettyUtils extends Logging {
val allowFramingFrom = conf.getOption("spark.ui.allowFramingFrom")
val xFrameOptionsValue =
allowFramingFrom.map(uri => s"ALLOW-FROM
$uri").getOrElse("SAMEORIGIN")
+val xXssProtectionValue = conf.getOption("spark.ui.xXssProtection")
--- End diff --
@jerryshao I am not sure if that will add much value in this context apart
from following best practices.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144262264 --- Diff: conf/spark-defaults.conf.template --- @@ -25,3 +25,10 @@ # spark.serializer org.apache.spark.serializer.KryoSerializer # spark.driver.memory 5g # spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +# spark.ui.allowFramingFrom https://www.example.com/ --- End diff -- @srowen , @jerryshao Removed the entries from the config template and put it to configuration doc as suggested. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144204531 --- Diff: conf/spark-defaults.conf.template --- @@ -25,3 +25,10 @@ # spark.serializer org.apache.spark.serializer.KryoSerializer # spark.driver.memory 5g # spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +# spark.ui.allowFramingFrom https://www.example.com/ +# spark.ui.xXssProtection 1; mode=block +# spark.ui.xContentType.options nosniff + +# Enable below only when Spark is running on HTTPS +# spark.ui.strictTransportSecurity max-age=31536000 --- End diff -- The REQUIRED "max-age" directive specifies the number of seconds, after the reception of the STS header field, during which the UA regards the host (from whom the message was received) as a Known HSTS Host. Here the value is equal to 365 days. More at https://tools.ietf.org/html/rfc6797#section-6.1.1 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user jerryshao commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r144186220 --- Diff: conf/spark-defaults.conf.template --- @@ -25,3 +25,10 @@ # spark.serializer org.apache.spark.serializer.KryoSerializer # spark.driver.memory 5g # spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +# spark.ui.allowFramingFrom https://www.example.com/ +# spark.ui.xXssProtection 1; mode=block +# spark.ui.xContentType.options nosniff + +# Enable below only when Spark is running on HTTPS +# spark.ui.strictTransportSecurity max-age=31536000 --- End diff -- What's the meaning of this specific number "31536000"? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r143427428
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,9 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+
xXssProtectionValue.foreach(response.setHeader("X-XSS-Protection", _))
+
xContentTypeOptionsValue.foreach(response.setHeader("X-Content-Type-Options",
_))
+
strictTransportSecurityValue.foreach(response.setHeader("Strict-Transport-Security",
_))
--- End diff --
@jerryshao Out of these three, Strict Transport Security header makes
sense, when SSL/TLS is enabled.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r143377794
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -79,6 +79,9 @@ private[spark] object JettyUtils extends Logging {
val allowFramingFrom = conf.getOption("spark.ui.allowFramingFrom")
val xFrameOptionsValue =
allowFramingFrom.map(uri => s"ALLOW-FROM
$uri").getOrElse("SAMEORIGIN")
+val xXssProtectionValue = conf.getOption("spark.ui.xXssProtection")
--- End diff --
Please use `ConfigEntry` for newly added configurations, you could refer to
`org.apache.spark.internal.config`.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r143377976
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,9 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+
xXssProtectionValue.foreach(response.setHeader("X-XSS-Protection", _))
+
xContentTypeOptionsValue.foreach(response.setHeader("X-Content-Type-Options",
_))
+
strictTransportSecurityValue.foreach(response.setHeader("Strict-Transport-Security",
_))
--- End diff --
The changes here will also affect HTTP request, is that OK?
Also is it enough to only change the GET request?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user jerryshao commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r143377740 --- Diff: conf/spark-defaults.conf.template --- @@ -25,3 +25,10 @@ # spark.serializer org.apache.spark.serializer.KryoSerializer # spark.driver.memory 5g # spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +# spark.ui.allowFramingFrom https://www.example.com/ --- End diff -- Agree with @srowen , we should remove the configurations here in template, since they're not common configurations. Also add them to `docs/configuration.md`. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r143349235 --- Diff: conf/spark-defaults.conf.template --- @@ -25,3 +25,10 @@ # spark.serializer org.apache.spark.serializer.KryoSerializer # spark.driver.memory 5g # spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +# spark.ui.allowFramingFrom https://www.example.com/ --- End diff -- Hm, my last thought on this is that these are undocumented options, and maybe rightly so as they're kind of advanced options. But if so they don't really have a place in the default props template; what do they mean to a user? I think they should actually be removed here, on second glance, but, wouldn't be a big deal to document them in the configuration doc as they're arguably non-internal, important functionality for some users. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user dongjoon-hyun commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r143113850 --- Diff: conf/spark-defaults.conf.template --- @@ -25,3 +25,10 @@ # spark.serializer org.apache.spark.serializer.KryoSerializer # spark.driver.memory 5g # spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +#spark.ui.allowFramingFrom https://www.example.com/ +#spark.ui.xXssProtection 1; mode=block +#spark.ui.xContentType.options nosniff + +#Enable below only when Spark is running on HTTPS +#spark.ui.strictTransportSecurity max-age=31536000 --- End diff -- Could you add a space after `#` and an empty line at the end of file? ``` # spark.ui.allowFramingFrom https://www.example.com/ # spark.ui.xXssProtection 1; mode=block # spark.ui.xContentType.options nosniff # Enable below only when Spark is running on HTTPS # spark.ui.strictTransportSecurity max-age=31536000 ``` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r143048963
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,15 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+if (xXssProtectionValue.isDefined) {
+ response.setHeader("X-XSS-Protection",
xXssProtectionValue.get)
+}
+if (xContentTypeOptionsValue.isDefined) {
+ response.setHeader("X-Content-Type-Options",
xContentTypeOptionsValue.get)
+}
+if (strictTransportSecurityValue.isDefined) {
+ response.setHeader("Strict-Transport-Security",
strictTransportSecurityValue.get)
+}
--- End diff --
Can we simplify line 95~103 into the following?
```scala
xXssProtectionValue.foreach(response.setHeader("X-XSS-Protection", _))
xContentTypeOptionsValue.foreach(response.setHeader("X-Content-Type-Options",
_))
strictTransportSecurityValue.foreach(response.setHeader("Strict-Transport-Security",
_))
```
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user dongjoon-hyun commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r143043339 --- Diff: conf/spark-defaults.conf.template --- @@ -19,9 +19,16 @@ # This is useful for setting default environmental settings. # Example: -# spark.master spark://master:7077 -# spark.eventLog.enabled true -# spark.eventLog.dir hdfs://namenode:8021/directory -# spark.serializer org.apache.spark.serializer.KryoSerializer -# spark.driver.memory 5g -# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" +# spark.master spark://master:7077 +# spark.eventLog.enabledtrue +# spark.eventLog.dirhdfs://namenode:8021/directory +# spark.serializer org.apache.spark.serializer.KryoSerializer +# spark.driver.memory 5g +# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" --- End diff -- Hi, @krishna-pandey . It seems that you can revert the change on line 22 ~ 27. Could you re-adjust the spacing? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r142708896 --- Diff: conf/spark-defaults.conf.template --- @@ -19,9 +19,16 @@ # This is useful for setting default environmental settings. # Example: -# spark.master spark://master:7077 -# spark.eventLog.enabled true -# spark.eventLog.dir hdfs://namenode:8021/directory -# spark.serializer org.apache.spark.serializer.KryoSerializer -# spark.driver.memory 5g -# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" +# spark.master spark://master:7077 +# spark.eventLog.enabledtrue +# spark.eventLog.dirhdfs://namenode:8021/directory +# spark.serializer org.apache.spark.serializer.KryoSerializer +# spark.driver.memory 5g +# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +#spark.ui.allowFramingFrom https://example.com/ +#spark.ui.xXssProtection.enabled1; mode=block --- End diff -- @srowen renamed the keys as suggested. Thanks again for the review. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r142701588 --- Diff: conf/spark-defaults.conf.template --- @@ -19,9 +19,16 @@ # This is useful for setting default environmental settings. # Example: -# spark.master spark://master:7077 -# spark.eventLog.enabled true -# spark.eventLog.dir hdfs://namenode:8021/directory -# spark.serializer org.apache.spark.serializer.KryoSerializer -# spark.driver.memory 5g -# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" +# spark.master spark://master:7077 +# spark.eventLog.enabledtrue +# spark.eventLog.dirhdfs://namenode:8021/directory +# spark.serializer org.apache.spark.serializer.KryoSerializer +# spark.driver.memory 5g +# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +#spark.ui.allowFramingFrom https://example.com/ +#spark.ui.xXssProtection.enabled1; mode=block +#spark.ui.xContentType.options nosniff + +#Enable below only when Spark is running on HTTPS +#spark.ui.strictTransportSecurity.age max-age=31536000 --- End diff -- I understand that the "max-age" part is common across all values but don't want to tamper the value part for ease and any future compatibility. I will rather remove ".age" to avoid confusion arising out of this. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r142634158 --- Diff: conf/spark-defaults.conf.template --- @@ -19,9 +19,16 @@ # This is useful for setting default environmental settings. # Example: -# spark.master spark://master:7077 -# spark.eventLog.enabled true -# spark.eventLog.dir hdfs://namenode:8021/directory -# spark.serializer org.apache.spark.serializer.KryoSerializer -# spark.driver.memory 5g -# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" +# spark.master spark://master:7077 +# spark.eventLog.enabledtrue +# spark.eventLog.dirhdfs://namenode:8021/directory +# spark.serializer org.apache.spark.serializer.KryoSerializer +# spark.driver.memory 5g +# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +#spark.ui.allowFramingFrom https://example.com/ +#spark.ui.xXssProtection.enabled1; mode=block --- End diff -- I would remove '.enabled' from the name then --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r142634239 --- Diff: conf/spark-defaults.conf.template --- @@ -19,9 +19,16 @@ # This is useful for setting default environmental settings. # Example: -# spark.master spark://master:7077 -# spark.eventLog.enabled true -# spark.eventLog.dir hdfs://namenode:8021/directory -# spark.serializer org.apache.spark.serializer.KryoSerializer -# spark.driver.memory 5g -# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" +# spark.master spark://master:7077 +# spark.eventLog.enabledtrue +# spark.eventLog.dirhdfs://namenode:8021/directory +# spark.serializer org.apache.spark.serializer.KryoSerializer +# spark.driver.memory 5g +# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +#spark.ui.allowFramingFrom https://example.com/ +#spark.ui.xXssProtection.enabled1; mode=block +#spark.ui.xContentType.options nosniff + +#Enable below only when Spark is running on HTTPS +#spark.ui.strictTransportSecurity.age max-age=31536000 --- End diff -- I'd just remove '.age' then, likewise. It's the very value of the STS header --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r142578623
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,9 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+response.setHeader("X-XSS-Protection", xXssProtectionValue.get)
--- End diff --
@srowen Added the check for if Option exists then set and tested locally.
Thanks for the review.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r142578622
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,9 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+response.setHeader("X-XSS-Protection", xXssProtectionValue.get)
--- End diff --
@srowen Added the check for if Option exists then set and tested locally.
Thanks for the review.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r142446016
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,9 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+response.setHeader("X-XSS-Protection", xXssProtectionValue.get)
--- End diff --
Yes, I will add a check for that.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r142445649
--- Diff: conf/spark-defaults.conf.template ---
@@ -19,9 +19,16 @@
# This is useful for setting default environmental settings.
# Example:
-# spark.master spark://master:7077
-# spark.eventLog.enabled true
-# spark.eventLog.dir hdfs://namenode:8021/directory
-# spark.serializer
org.apache.spark.serializer.KryoSerializer
-# spark.driver.memory 5g
-# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value
-Dnumbers="one two three"
+# spark.master spark://master:7077
+# spark.eventLog.enabledtrue
+# spark.eventLog.dirhdfs://namenode:8021/directory
+# spark.serializer
org.apache.spark.serializer.KryoSerializer
+# spark.driver.memory 5g
+# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value
-Dnumbers="one two three"
+
+#spark.ui.allowFramingFrom https://example.com/
+#spark.ui.xXssProtection.enabled1; mode=block
+#spark.ui.xContentType.options nosniff
+
+#Enable below only when Spark is running on HTTPS
+#spark.ui.strictTransportSecurity.age max-age=31536000
--- End diff --
Yes, "max-age" need to be part of user-supplied string. Possible values can
be one from the below set, default one mentioned in template is for optimum
secure value commonly used.
Strict-Transport-Security: {max-age=\ |
max-age=\; includeSubDomains | max-age=\; preload}
Value is in delta-seconds. More here
https://tools.ietf.org/html/rfc6797#section-6.1.1
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user krishna-pandey commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r142442356
--- Diff: conf/spark-defaults.conf.template ---
@@ -19,9 +19,16 @@
# This is useful for setting default environmental settings.
# Example:
-# spark.master spark://master:7077
-# spark.eventLog.enabled true
-# spark.eventLog.dir hdfs://namenode:8021/directory
-# spark.serializer
org.apache.spark.serializer.KryoSerializer
-# spark.driver.memory 5g
-# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value
-Dnumbers="one two three"
+# spark.master spark://master:7077
+# spark.eventLog.enabledtrue
+# spark.eventLog.dirhdfs://namenode:8021/directory
+# spark.serializer
org.apache.spark.serializer.KryoSerializer
+# spark.driver.memory 5g
+# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value
-Dnumbers="one two three"
+
+#spark.ui.allowFramingFrom https://example.com/
+#spark.ui.xXssProtection.enabled1; mode=block
--- End diff --
Possible values can be one from the below set, default one mentioned in
template is commonly-used and provides effective protection.
X-XSS-Protection: { 0 | 1 |1; mode=block | 1; report= }
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19419#discussion_r142409896
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -89,6 +92,9 @@ private[spark] object JettyUtils extends Logging {
val result = servletParams.responder(request)
response.setHeader("Cache-Control", "no-cache, no-store,
must-revalidate")
response.setHeader("X-Frame-Options", xFrameOptionsValue)
+response.setHeader("X-XSS-Protection", xXssProtectionValue.get)
--- End diff --
This will fail if these aren't set. Only set the headers if the Option
exists
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r142409684 --- Diff: conf/spark-defaults.conf.template --- @@ -19,9 +19,16 @@ # This is useful for setting default environmental settings. # Example: -# spark.master spark://master:7077 -# spark.eventLog.enabled true -# spark.eventLog.dir hdfs://namenode:8021/directory -# spark.serializer org.apache.spark.serializer.KryoSerializer -# spark.driver.memory 5g -# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" +# spark.master spark://master:7077 +# spark.eventLog.enabledtrue +# spark.eventLog.dirhdfs://namenode:8021/directory +# spark.serializer org.apache.spark.serializer.KryoSerializer +# spark.driver.memory 5g +# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +#spark.ui.allowFramingFrom https://example.com/ +#spark.ui.xXssProtection.enabled1; mode=block --- End diff -- This sounds like a boolean flag but its value isn't boolean; if it has only one reasonable value then it can be a boolean that sets that value. Otherwise maybe call the property something else --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19419: [SPARK-22188] [CORE] Adding security headers for ...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/19419#discussion_r142409795 --- Diff: conf/spark-defaults.conf.template --- @@ -19,9 +19,16 @@ # This is useful for setting default environmental settings. # Example: -# spark.master spark://master:7077 -# spark.eventLog.enabled true -# spark.eventLog.dir hdfs://namenode:8021/directory -# spark.serializer org.apache.spark.serializer.KryoSerializer -# spark.driver.memory 5g -# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" +# spark.master spark://master:7077 +# spark.eventLog.enabledtrue +# spark.eventLog.dirhdfs://namenode:8021/directory +# spark.serializer org.apache.spark.serializer.KryoSerializer +# spark.driver.memory 5g +# spark.executor.extraJavaOptions -XX:+PrintGCDetails -Dkey=value -Dnumbers="one two three" + +#spark.ui.allowFramingFrom https://example.com/ +#spark.ui.xXssProtection.enabled1; mode=block +#spark.ui.xContentType.options nosniff + +#Enable below only when Spark is running on HTTPS +#spark.ui.strictTransportSecurity.age max-age=31536000 --- End diff -- On a similar note does max-age need to be part of the user-supplied string or just the age? in what units? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
