[SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide key password from linux ps listing and masterwebui point to https if ssl enabled #21514

tooptoop4 · 2018-06-08T23:29:46Z

What changes were proposed in this pull request?

hide password from 'ps' linux command
AND
masterwebui point to https if ssl enabled

How was this patch tested?

Existing tests

Please review http://spark.apache.org/contributing.html before opening a pull request.
The contribution is my own original work and I license the work to the project under the project’s open source license.

pjfanning · 2018-06-08T23:33:59Z

Instead of filtering out the passwords, would it be more useful to blank the passwords or replace them with ****** in the output?

tooptoop4 · 2018-06-08T23:39:22Z

I just went through the process of getting 'security approval' in my organisation to use Spark in Production. They ran a Qualsys Vulnerability Assessment scanner that among other things picked up a vulnerability called 'presence of string keyStorePassword in process list' so the scan would still fail if the string keyStorePassword appeared at all.

tooptoop4 · 2018-06-09T01:44:43Z

@cloud-fan can u commit?

jiangxb1987 · 2018-06-11T05:38:03Z

Have you tried the config "spark.redaction.regex" ?

jiangxb1987 · 2018-06-11T05:41:05Z

core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala

@@ -100,7 +100,7 @@ private[spark] class StandaloneSchedulerBackend(
    val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf)
    val javaOpts = sparkJavaOpts ++ extraJavaOpts
    val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend",
-      args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts)
+      args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts.filterNot(_.startsWith("-Dspark.ssl.keyStorePassword")).filterNot(_.startsWith("-Dspark.ssl.keyPassword")))


If you really have to do this, I'd have:

javaOpts.filterNot { opt => opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword") }

What about *storePassword?

Actually I'm thinking of using Hadoop credential provider to store password (https://hadoop.apache.org/docs/current/hadoop-project-dist/hadoop-common/CredentialProviderAPI.html) to avoid plaintext password. I have a local PR for this.

does your PR still end up having the literal 'storePassword' in ps output?

maropu · 2018-06-18T05:14:20Z

Is this only the place where we need to hide the password? e.g., how about logging about properties in SparkSubmitArguments

mgaido91

can you add UTs for your changes? Or at least post some pictures with your tests of the changes?

mgaido91 · 2018-06-30T08:54:00Z

core/src/main/scala/org/apache/spark/deploy/master/Master.scala

+    val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false)
+    val uriScheme = "http://"
+    if (SSL_ENABLED) {
+      uriScheme = "https://"


have you tested this? This is a val, I doubt this can even compile...

Could you try this? It's a bit closer to the usual scala style.

val uriScheme = if (conf.getBoolean("spark.ssl.enabled", false)) "https://" else "http://"

woops, i just updated val to var

yes, but still please add tests for this, in order to enforce the correctness of your solution. PS I agree with @pjfanning's suggestion..

mgaido91 · 2018-06-30T08:54:10Z

core/src/main/scala/org/apache/spark/deploy/master/Master.scala

+      uriScheme = "https://"
+    }
+    masterWebUiUrl = uriScheme + masterPublicAddress + ":" + webUi.boundPort
+    //masterWebUiUrl = "http://" + masterPublicAddress + ":" + webUi.boundPort


please remove this comment

update in latest commit

HyukjinKwon · 2018-07-16T02:43:22Z

ok to test

SparkQA · 2018-07-16T02:51:19Z

Test build #93065 has finished for PR 21514 at commit 825380a.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

tooptoop4 · 2018-07-16T12:54:00Z

ok to test

tooptoop4 · 2018-07-16T12:54:44Z

@HyukjinKwon can u commit this? i fixed the Scala style

mgaido91

please if you cannot add UT, at least provide some pictures showing the behavior before and after your PR.

mgaido91 · 2018-07-16T13:08:18Z

core/src/main/scala/org/apache/spark/deploy/master/Master.scala

@@ -130,7 +130,12 @@ private[deploy] class Master(
    logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}")
    webUi = new MasterWebUI(this, webUiPort)
    webUi.bind()
-    masterWebUiUrl = "http://" + masterPublicAddress + ":" + webUi.boundPort
+    val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false)
+    var uriScheme = "http://"


this can be:

val uriScheme = if (SSL_ENABLED) { "https://" } else { "http://" }

SparkQA · 2018-07-16T16:36:32Z

Test build #93104 has finished for PR 21514 at commit 050b226.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

tooptoop4 · 2018-07-16T22:05:48Z

ok to test

SparkQA · 2018-07-16T22:10:18Z

Test build #93142 has finished for PR 21514 at commit bc8a833.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2018-07-16T22:09:55Z

core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala

@@ -100,7 +100,9 @@ private[spark] class StandaloneSchedulerBackend(
    val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf)
    val javaOpts = sparkJavaOpts ++ extraJavaOpts
    val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend",
-      args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts)
+      args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries


please put the comma at the end of this line instead of the beginning of the next one.

mgaido91 · 2018-07-16T22:12:05Z

core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala

@@ -100,7 +100,9 @@ private[spark] class StandaloneSchedulerBackend(
    val sparkJavaOpts = Utils.sparkJavaOpts(conf, SparkConf.isExecutorStartupConf)
    val javaOpts = sparkJavaOpts ++ extraJavaOpts
    val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend",
-      args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts)
+      args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries
+      , javaOpts.filterNot { opt =>


we don't put long code inline to function argument usually. Please move this before and store it to a local val used here.

mgaido91 · 2018-07-16T22:14:22Z

core/src/test/scala/org/apache/spark/deploy/master/MasterSuite.scala

@@ -282,6 +282,40 @@ class MasterSuite extends SparkFunSuite
    }
  }

+  test("SPARK-24621 https urls when ssl enabled") {


nit: we usually put a : after the JIRA number, eg. SPARK-24621:

tooptoop4 · 2018-07-16T23:33:38Z

@mgaido91 - do you know what caused this?
Scalastyle checks failed at following occurrences:
[error] /home/jenkins/workspace/SparkPullRequestBuilder@2/core/src/test/scala/org/apache/spark/deploy/master/MasterSuite.scala: Expected token RPAREN but got Token(RBRACE,},10480,})

mgaido91 · 2018-07-17T08:57:15Z

@tooptoop4 the only thing I can see is that you are suing ' instead of " for string. Anyway you can run the scala linter on your local machine too and you can check which part of the code you added to the MasterSuite file causes the failure

tooptoop4 · 2018-07-17T09:57:55Z

ok to test

SparkQA · 2018-07-17T10:00:24Z

Test build #93166 has finished for PR 21514 at commit 39bb2a0.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2018-07-17T10:52:51Z

core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala

-    val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend",
-      args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts)
+    val javaOptsFiltered = javaOpts.filterNot { opt =>
+    opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword")}


wrong indentation. This should look like:

val javaOptsFiltered = javaOpts.filterNot { opt => opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword") }

mgaido91 · 2018-07-17T10:53:22Z

core/src/main/scala/org/apache/spark/deploy/master/Master.scala

@@ -130,7 +130,9 @@ private[deploy] class Master(
    logInfo(s"Running Spark version ${org.apache.spark.SPARK_VERSION}")
    webUi = new MasterWebUI(this, webUiPort)
    webUi.bind()
-    masterWebUiUrl = "http://" + masterPublicAddress + ":" + webUi.boundPort
+    val SSL_ENABLED = conf.getBoolean("spark.ssl.enabled", false)


nit: rename to sslEnabled

tooptoop4 · 2018-07-17T11:47:16Z

ok to test

SparkQA · 2018-07-17T11:50:28Z

Test build #93172 has finished for PR 21514 at commit d4de123.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2018-07-17T11:51:54Z

core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala

-    val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend",
-      args, sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOpts)
+    val javaOptsFiltered = javaOpts.filterNot { opt =>
+    opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword")


wrong indentation: missing 2 spaces.

mgaido91

left some other comments. Please verify locally and fix the scala style error. You can run the linter also locally. Thanks.

mgaido91 · 2018-07-17T11:53:15Z

core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala

+    opt.startsWith("-Dspark.ssl.keyStorePassword") || opt.startsWith("-Dspark.ssl.keyPassword")
+    }
+    val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", args,
+    sc.executorEnvs, classPathEntries ++ testingClassPath, libraryPathEntries, javaOptsFiltered)


wrong indentation here too, missing 2 spaces. Moreover, in such cases, we usually put one argument per line, so:

val command = Command("org.apache.spark.executor.CoarseGrainedExecutorBackend", args, sc.executorEnvs, ...)

mgaido91 · 2018-07-17T11:53:38Z

core/src/test/scala/org/apache/spark/deploy/master/MasterSuite.scala

+      eventually(timeout(5 seconds), interval(100 milliseconds)) {
+        val json = Source.fromURL(s"https://localhost:${localCluster.masterWebUIPort}/json")
+          .getLines().mkString("\n")
+        assert(json.contains('<a href="https://'))


" instead of '

mgaido91 · 2018-07-17T11:53:44Z

core/src/test/scala/org/apache/spark/deploy/master/MasterSuite.scala

+      eventually(timeout(5 seconds), interval(100 milliseconds)) {
+        val json = Source.fromURL(s"http://localhost:${localCluster.masterWebUIPort}/json")
+          .getLines().mkString("\n")
+        assert(!json.contains('<a href="https://'))


HyukjinKwon · 2018-11-11T03:33:22Z

ping @tooptoop4

HyukjinKwon · 2018-11-11T03:34:57Z

I'm going to suggest to close this. The review comments were not addressed more then few months and there's not quite a great point to keep inactive PRs.

Feel free to take over this if any of you here is interested in this. Or @tooptoop4, please recreate a PR after addressing review commnets here.

Thanks.

[SPARK-22860] [Core] - hide key password from linux ps listing

380d97d

jiangxb1987 reviewed Jun 11, 2018

View reviewed changes

SPARK-24621

f38de2b

tooptoop4 changed the title ~~[SPARK-22860] [Core] - hide key password from linux ps listing~~ [SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide key password from linux ps listing and masterwebui point to https if ssl enabled Jun 30, 2018

mgaido91 reviewed Jun 30, 2018

View reviewed changes

updated according to review

825380a

style guide <100 char line of code

050b226

mgaido91 reviewed Jul 16, 2018

View reviewed changes

added UT and changed 4 lines to 1 line

bc8a833

mgaido91 reviewed Jul 16, 2018

View reviewed changes

style comments

39bb2a0

mgaido91 reviewed Jul 17, 2018

View reviewed changes

nit pickin'

d4de123

mgaido91 reviewed Jul 17, 2018

View reviewed changes

HyukjinKwon mentioned this pull request Nov 11, 2018

[INFRA] Close stale PRs #23001

Closed

asfgit closed this in a3ba3a8 Nov 11, 2018

[SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide key password from linux ps listing and masterwebui point to https if ssl enabled #21514

[SPARK-22860] [SPARK-24621] [Core] [WebUI] - hide key password from linux ps listing and masterwebui point to https if ssl enabled #21514

Conversation

tooptoop4 commented Jun 8, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

pjfanning commented Jun 8, 2018

tooptoop4 commented Jun 8, 2018

tooptoop4 commented Jun 9, 2018 • edited Loading

jiangxb1987 commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu commented Jun 18, 2018

mgaido91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Jul 16, 2018

SparkQA commented Jul 16, 2018

tooptoop4 commented Jul 16, 2018

tooptoop4 commented Jul 16, 2018

mgaido91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 16, 2018

tooptoop4 commented Jul 16, 2018

SparkQA commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tooptoop4 commented Jul 16, 2018

mgaido91 commented Jul 17, 2018

tooptoop4 commented Jul 17, 2018

SparkQA commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tooptoop4 commented Jul 17, 2018

SparkQA commented Jul 17, 2018

Choose a reason for hiding this comment

mgaido91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Nov 11, 2018

HyukjinKwon commented Nov 11, 2018

tooptoop4 commented Jun 8, 2018 •

edited

Loading

tooptoop4 commented Jun 9, 2018 •

edited

Loading