[SPARK-16796] [Web UI] Visible passwords on Spark environment page #14409

Devian-ua · 2016-07-29T20:19:15Z

What changes were proposed in this pull request?

Mask spark.ssl.keyPassword, spark.ssl.keyStorePassword, spark.ssl.trustStorePassword in Web UI environment page.
(Changes their values to ***** in env. page)

How was this patch tested?

I've built spark, run spark shell and checked that this values have been masked with *****.

Also run tests:
./dev/run-tests

[info] ScalaTest
[info] Run completed in 1 hour, 9 minutes, 5 seconds.
[info] Total number of tests run: 2166
[info] Suites: completed 65, aborted 0
[info] Tests: succeeded 2166, failed 0, canceled 0, ignored 590, pending 0
[info] All tests passed.

dongjoon-hyun · 2016-07-29T20:27:24Z

core/src/main/scala/org/apache/spark/ui/env/EnvironmentPage.scala

Could we use Set for this?

Devian-ua · 2016-07-29T20:41:39Z

@ajbozarth What if some property will be intoruduced containing "password" in its name, but not having password in its value?

@dongjoon-hyun I can change it to Set.

ajbozarth · 2016-07-29T20:45:18Z

I'd say that that a property like that was poorly named, but it is still a valid possibility so leaving it as you have it is probably better

ajbozarth · 2016-07-29T20:47:35Z

All in all I think this looks good

rxin · 2016-07-31T06:06:39Z

Perhaps everything with the name "password" we should just hide it?

srowen · 2016-07-31T12:56:57Z

core/src/main/scala/org/apache/spark/ui/env/EnvironmentPage.scala

+  val passwordProperties: Set[String] = Set("spark.ssl.keyPassword",
+    "spark.ssl.keyStorePassword", "spark.ssl.trustStorePassword")
+
+  def removePass(kv: (String, String)): (String, String) = {


private? agree with filtering anything whose key has "password" (not case sensitive). Check for other instances where listener.sparkProperties is accessed too?

Devian-ua · 2016-07-31T14:35:15Z

@srowen Made removePass private. Also now it filters all properties containing "password"

srowen · 2016-07-31T18:11:23Z

core/src/main/scala/org/apache/spark/ui/env/EnvironmentPage.scala

      propertyHeader, jvmRow, listener.jvmInformation, fixedWidth = true)
    val sparkPropertiesTable = UIUtils.listingTable(
-      propertyHeader, propertyRow, listener.sparkProperties, fixedWidth = true)
+      propertyHeader, propertyRow, listener.sparkProperties.map(removePass), fixedWidth = true)


This may be just taste, but isn't this easier inline?

val redactedProperties = listener.sparkProperties.map { case (key, value) => (key, if (key.toLowerCase.contains("password")) "******" else value) }

Are there any other instances where properties are displayed that need redacting?

Devian-ua · 2016-07-31T18:24:15Z

@srowen I think it looks cleaner when it is not inline.
On Web UI this is the only place they show up. Also they won't be masked in logs.

srowen · 2016-08-02T16:31:29Z

Jenkins test this please

srowen · 2016-08-02T17:36:58Z

@Devian-ua what is your ASF JIRA handle?

Devian-ua · 2016-08-02T17:58:34Z

@srowen You mean username? It is asukhenko

SparkQA · 2016-08-02T18:44:40Z

Test build #63128 has finished for PR 14409 at commit e4b617c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-08-02T23:13:23Z

Merged to master

a-roberts · 2016-08-03T12:58:42Z

@Devian-ua
I think that while we're in the web UI we should look at masking the shared secret too (so potentially another JIRA/contribution to follow? At least something I'm interested in pursuing)

Also curious if you have any plans to backport this small change to 1.5.x and 1.6.x

srowen · 2016-08-03T13:11:59Z

I'm OK back-porting further. It's minor and a potential security improvement. I don't think the 1.5 branch is active though.

## What changes were proposed in this pull request? Mask spark.ssl.keyPassword, spark.ssl.keyStorePassword, spark.ssl.trustStorePassword in Web UI environment page. (Changes their values to ***** in env. page) ## How was this patch tested? I've built spark, run spark shell and checked that this values have been masked with *****. Also run tests: ./dev/run-tests [info] ScalaTest [info] Run completed in 1 hour, 9 minutes, 5 seconds. [info] Total number of tests run: 2166 [info] Suites: completed 65, aborted 0 [info] Tests: succeeded 2166, failed 0, canceled 0, ignored 590, pending 0 [info] All tests passed. ![mask](https://cloud.githubusercontent.com/assets/15244468/17262154/7641e132-55e2-11e6-8a6c-30ead77c7372.png) Author: Artur Sukhenko <[email protected]> Closes #14409 from Devian-ua/maskpass. (cherry picked from commit 3861273) Signed-off-by: Sean Owen <[email protected]>

Devian-ua · 2016-08-03T16:11:20Z

@srowen @a-roberts So what should we do with spark.authenticate.secret. Do we want to mask it as well?
Something like this?

 // Spark Properties (lowercase). Their values will be changed to ***** in WebUI
  private val propertiesToMask = Set("spark.authenticate.secret")

  private def removePass(kv: (String, String)): (String, String) = {
    if (kv._1.toLowerCase.contains("password") || propertiesToMask.contains(kv._1.toLowerCase)) {
      (kv._1, "******")
    } else kv
  }

ajbozarth · 2016-08-03T19:21:57Z

Lets open a follow up JIRA on this, but I would say that similar to password, secret could be masked for all instances

srowen · 2016-08-03T19:28:34Z

If it's truly just an addendum to this issue, tacked on, just attach to the existing issue.

Devian-ua · 2016-08-03T19:52:35Z

@srowen So what should I do?

srowen · 2016-08-03T19:53:46Z

Just another PR for the same JIRA

Devian-ua · 2016-08-03T19:54:28Z

@srowen Different branch?

srowen · 2016-08-03T20:01:00Z

Yes, this is merged now.

Devian-ua · 2016-08-03T20:34:28Z

@srowen I've opened #14484

## What changes were proposed in this pull request? Mask spark.ssl.keyPassword, spark.ssl.keyStorePassword, spark.ssl.trustStorePassword in Web UI environment page. (Changes their values to ***** in env. page) ## How was this patch tested? I've built spark, run spark shell and checked that this values have been masked with *****. Also run tests: ./dev/run-tests [info] ScalaTest [info] Run completed in 1 hour, 9 minutes, 5 seconds. [info] Total number of tests run: 2166 [info] Suites: completed 65, aborted 0 [info] Tests: succeeded 2166, failed 0, canceled 0, ignored 590, pending 0 [info] All tests passed. ![mask](https://cloud.githubusercontent.com/assets/15244468/17262154/7641e132-55e2-11e6-8a6c-30ead77c7372.png) Author: Artur Sukhenko <[email protected]> Closes apache#14409 from Devian-ua/maskpass. (cherry picked from commit 3861273) Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 52d8837)

## What changes were proposed in this pull request? Mask `spark.authenticate.secret` on Spark environment page (Web UI). This is addition to #14409 ## How was this patch tested? `./dev/run-tests` [info] ScalaTest [info] Run completed in 1 hour, 8 minutes, 38 seconds. [info] Total number of tests run: 2166 [info] Suites: completed 65, aborted 0 [info] Tests: succeeded 2166, failed 0, canceled 0, ignored 590, pending 0 [info] All tests passed. Author: Artur Sukhenko <[email protected]> Closes #14484 from Devian-ua/SPARK-16796.

n-marion · 2016-11-02T13:20:07Z

A related commit #14484 hasn't been added to Spark 2.0 for 2.0.1 or 2.0.2

srowen · 2016-11-02T13:29:14Z

This was all merged to master, meaning 2.1.x

n-marion · 2016-11-02T17:34:17Z

Thanks for the quick response. I did see this in 2.1.X branch; but #14409 made it into 2.0.X, which was merged on August 3rd, while #14484 didn't which was merged on August 5th.

Although its a small change, providing sensitive information on a web UI isn't ideal, it is IMO important enough to make it into Spark 2.0.2.

srowen · 2016-11-02T18:05:23Z

Aha I see it now. Yes I think it's fair to put in 2.0 as well. It may or may not make 2.0.2.

n-marion · 2016-11-02T18:08:13Z

Appreciate that. Thank you.

## What changes were proposed in this pull request? Mask `spark.authenticate.secret` on Spark environment page (Web UI). This is addition to #14409 ## How was this patch tested? `./dev/run-tests` [info] ScalaTest [info] Run completed in 1 hour, 8 minutes, 38 seconds. [info] Total number of tests run: 2166 [info] Suites: completed 65, aborted 0 [info] Tests: succeeded 2166, failed 0, canceled 0, ignored 590, pending 0 [info] All tests passed. Author: Artur Sukhenko <[email protected]> Closes #14484 from Devian-ua/SPARK-16796. (cherry picked from commit 14dba45) Signed-off-by: Sean Owen <[email protected]>

[SPARK-16796] [Web UI] Visible passwords on Spark environment page

0f17ce1

dongjoon-hyun reviewed Jul 29, 2016
View reviewed changes

core/src/main/scala/org/apache/spark/ui/env/EnvironmentPage.scala Outdated

Copy link

Member

dongjoon-hyun Jul 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use Set for this?

Change Seq to Set for passwordProperties

e97f894

Devian-ua force-pushed the maskpass branch from e41b7d5 to e97f894 Compare July 29, 2016 21:02

srowen reviewed Jul 31, 2016
View reviewed changes

Devian-ua force-pushed the maskpass branch 2 times, most recently from 37b11ca to e97f894 Compare July 31, 2016 14:10

[SPARK-16796][Web UI] Visible passwords on Spark environment page

1bee0da

srowen reviewed Jul 31, 2016
View reviewed changes

Refactor removePass to follow scala style

e4b617c

asfgit closed this in 3861273 Aug 2, 2016

Devian-ua mentioned this pull request Aug 3, 2016

[SPARK-16796][Web UI] Mask spark.authenticate.secret on Spark environ… #14484

Closed

[SPARK-16796] [Web UI] Visible passwords on Spark environment page #14409

[SPARK-16796] [Web UI] Visible passwords on Spark environment page #14409

Uh oh!

Conversation

Devian-ua commented Jul 29, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dongjoon-hyun Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

Devian-ua commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajbozarth commented Jul 29, 2016

Uh oh!

ajbozarth commented Jul 29, 2016

Uh oh!

rxin commented Jul 31, 2016

Uh oh!

srowen Jul 31, 2016

Choose a reason for hiding this comment

Uh oh!

Devian-ua commented Jul 31, 2016

Uh oh!

srowen Jul 31, 2016

Choose a reason for hiding this comment

Uh oh!

Devian-ua commented Jul 31, 2016

Uh oh!

srowen commented Aug 2, 2016

Uh oh!

srowen commented Aug 2, 2016

Uh oh!

Devian-ua commented Aug 2, 2016

Uh oh!

SparkQA commented Aug 2, 2016

Uh oh!

srowen commented Aug 2, 2016

Uh oh!

a-roberts commented Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented Aug 3, 2016

Uh oh!

Devian-ua commented Aug 3, 2016

Uh oh!

ajbozarth commented Aug 3, 2016

Uh oh!

srowen commented Aug 3, 2016

Uh oh!

Devian-ua commented Aug 3, 2016

Uh oh!

srowen commented Aug 3, 2016

Uh oh!

Devian-ua commented Aug 3, 2016

Uh oh!

srowen commented Aug 3, 2016

Uh oh!

Devian-ua commented Aug 3, 2016

Uh oh!

n-marion commented Nov 2, 2016

Uh oh!

srowen commented Nov 2, 2016

Uh oh!

n-marion commented Nov 2, 2016

Uh oh!

srowen commented Nov 2, 2016

Uh oh!

n-marion commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Devian-ua commented Jul 29, 2016 •

edited

Loading

a-roberts commented Aug 3, 2016 •

edited

Loading