Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tooptoop4 wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
380d97d
[SPARK-22860] [Core] - hide key password from linux ps listing
Jun 8, 2018
f38de2b
SPARK-24621
Jun 30, 2018
825380a
updated according to review
Jul 1, 2018
050b226
style guide <100 char line of code
Jul 16, 2018
bc8a833
added UT and changed 4 lines to 1 line
Jul 16, 2018
39bb2a0
style comments
Jul 17, 2018
d4de123
nit pickin'
Jul 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion core/src/main/scala/org/apache/spark/deploy/master/Master.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,13 @@ 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)
Copy link
Contributor

@mgaido91 mgaido91 Jul 17, 2018

Choose a reason for hiding this comment

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

nit: rename to sslEnabled

Copy link
Contributor Author

@tooptoop4 tooptoop4 Jul 17, 2018

Choose a reason for hiding this comment

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

done

val uriScheme = "http://"
if (SSL_ENABLED) {
uriScheme = "https://"
Copy link
Contributor

@mgaido91 mgaido91 Jun 30, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@pjfanning pjfanning Jul 1, 2018

Choose a reason for hiding this comment

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

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://"

Copy link
Contributor Author

@tooptoop4 tooptoop4 Jul 1, 2018

Choose a reason for hiding this comment

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

woops, i just updated val to var

Copy link
Contributor

@mgaido91 mgaido91 Jul 1, 2018

Choose a reason for hiding this comment

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

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

}
masterWebUiUrl = uriScheme + masterPublicAddress + ":" + webUi.boundPort
//masterWebUiUrl = "http://" + masterPublicAddress + ":" + webUi.boundPort
Copy link
Contributor

@mgaido91 mgaido91 Jun 30, 2018

Choose a reason for hiding this comment

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

please remove this comment

Copy link
Contributor Author

@tooptoop4 tooptoop4 Jul 1, 2018

Choose a reason for hiding this comment

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

update in latest commit

if (reverseProxy) {
masterWebUiUrl = conf.get("spark.ui.reverseProxyUrl", masterWebUiUrl)
webUi.addProxy()
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala/org/apache/spark/scheduler/cluster/StandaloneSchedulerBackend.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")))
Copy link
Contributor

@jiangxb1987 jiangxb1987 Jun 11, 2018

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

@jerryshao jerryshao Jun 11, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tooptoop4 tooptoop4 Jul 1, 2018

Choose a reason for hiding this comment

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

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

val webUrl = sc.ui.map(_.webUrl).getOrElse("")
val coresPerExecutor = conf.getOption("spark.executor.cores").map(_.toInt)
// If we're using dynamic allocation, set our initial executor limit to 0 for now.
Expand Down