From 0ef3f8bb8f0fd06c037ea11fcd2c8a40f6ab7852 Mon Sep 17 00:00:00 2001 From: Gabor Somogyi Date: Wed, 20 Mar 2019 17:11:29 +0100 Subject: [PATCH 1/8] [SPARK-26998][CORE] Add secure alternatives of ssl password parameters --- .../scala/org/apache/spark/SSLOptions.scala | 59 +++++++++-- core/src/test/resources/key-password | 1 + core/src/test/resources/keystore-password | 1 + core/src/test/resources/truststore-password | 1 + .../org/apache/spark/SSLOptionsSuite.scala | 99 ++++++++++++++++--- docs/security.md | 35 ++++++- 6 files changed, 169 insertions(+), 27 deletions(-) create mode 100644 core/src/test/resources/key-password create mode 100644 core/src/test/resources/keystore-password create mode 100644 core/src/test/resources/truststore-password diff --git a/core/src/main/scala/org/apache/spark/SSLOptions.scala b/core/src/main/scala/org/apache/spark/SSLOptions.scala index 1632e0c69eef..8cdac1382897 100644 --- a/core/src/main/scala/org/apache/spark/SSLOptions.scala +++ b/core/src/main/scala/org/apache/spark/SSLOptions.scala @@ -18,6 +18,7 @@ package org.apache.spark import java.io.File +import java.nio.file.Files import java.security.NoSuchAlgorithmException import javax.net.ssl.SSLContext @@ -131,8 +132,9 @@ private[spark] case class SSLOptions( /** Returns a string representation of this SSLOptions with all the passwords masked. */ override def toString: String = s"SSLOptions{enabled=$enabled, port=$port, " + s"keyStore=$keyStore, keyStorePassword=${keyStorePassword.map(_ => "xxx")}, " + + s"keyPassword=${keyPassword.map(_ => "xxx")}, keyStoreType=$keyStoreType, " + s"trustStore=$trustStore, trustStorePassword=${trustStorePassword.map(_ => "xxx")}, " + - s"protocol=$protocol, enabledAlgorithms=$enabledAlgorithms}" + s"trustStoreType=$trustStoreType, protocol=$protocol, enabledAlgorithms=$enabledAlgorithms}" } @@ -184,13 +186,11 @@ private[spark] object SSLOptions extends Logging { val keyStore = conf.getWithSubstitution(s"$ns.keyStore").map(new File(_)) .orElse(defaults.flatMap(_.keyStore)) - val keyStorePassword = conf.getWithSubstitution(s"$ns.keyStorePassword") - .orElse(Option(hadoopConf.getPassword(s"$ns.keyStorePassword")).map(new String(_))) - .orElse(defaults.flatMap(_.keyStorePassword)) + val keyStorePassword = + getPassword(conf, hadoopConf, ns, "keyStorePassword", defaults.flatMap(_.keyStorePassword)) - val keyPassword = conf.getWithSubstitution(s"$ns.keyPassword") - .orElse(Option(hadoopConf.getPassword(s"$ns.keyPassword")).map(new String(_))) - .orElse(defaults.flatMap(_.keyPassword)) + val keyPassword = + getPassword(conf, hadoopConf, ns, "keyPassword", defaults.flatMap(_.keyPassword)) val keyStoreType = conf.getWithSubstitution(s"$ns.keyStoreType") .orElse(defaults.flatMap(_.keyStoreType)) @@ -201,9 +201,8 @@ private[spark] object SSLOptions extends Logging { val trustStore = conf.getWithSubstitution(s"$ns.trustStore").map(new File(_)) .orElse(defaults.flatMap(_.trustStore)) - val trustStorePassword = conf.getWithSubstitution(s"$ns.trustStorePassword") - .orElse(Option(hadoopConf.getPassword(s"$ns.trustStorePassword")).map(new String(_))) - .orElse(defaults.flatMap(_.trustStorePassword)) + val trustStorePassword = getPassword( + conf, hadoopConf, ns, "trustStorePassword", defaults.flatMap(_.trustStorePassword)) val trustStoreType = conf.getWithSubstitution(s"$ns.trustStoreType") .orElse(defaults.flatMap(_.trustStoreType)) @@ -231,5 +230,45 @@ private[spark] object SSLOptions extends Logging { enabledAlgorithms) } + private def getPassword( + conf: SparkConf, + hadoopConf: Configuration, + ns: String, + parameter: String, + default: Option[String]): Option[String] = { + var parameterValue = conf.getWithSubstitution(s"$ns.$parameter") + .orElse(Option(hadoopConf.getPassword(s"$ns.$parameter")).map(new String(_))) + .orElse(default) + if (parameterValue.isDefined && default.isDefined && parameterValue.get != default.get) { + logWarning( + s"$ns.$parameter configuration parameter defined which may cause security problems. When " + + "its configured as command line argument then plain text password can be dumped by " + + "listing the process command line arguments. The more secure alternative solution is to " + + s"use $ns.${parameter}File." + ) + } + + val parameterFileValue = conf.getWithSubstitution(s"$ns.${parameter}File") + .orElse(Option(hadoopConf.getPassword(s"$ns.${parameter}File")).map(new String(_))) + if (parameterFileValue.isDefined) { + val parameterFileContent = readPasswordFile(parameterFileValue.get) + if (parameterValue.isDefined && parameterValue.get != parameterFileContent) { + throw new IllegalArgumentException(s"Both $ns.$parameter and $ns.${parameter}File " + + "parameters defined but the they differ.") + } + parameterValue = Some(parameterFileContent) + } + + parameterValue + } + + private def readPasswordFile(passwordFilePath: String): String = { + val passwordFile = new File(passwordFilePath) + require(passwordFile.isFile, s"No file found containing the password at $passwordFilePath.") + val content = new String(Files.readAllBytes(passwordFile.toPath)).trim + require(!content.isEmpty, s"Password from file located at $passwordFilePath is empty.") + content + } + } diff --git a/core/src/test/resources/key-password b/core/src/test/resources/key-password new file mode 100644 index 000000000000..7aa311adf93f --- /dev/null +++ b/core/src/test/resources/key-password @@ -0,0 +1 @@ +password \ No newline at end of file diff --git a/core/src/test/resources/keystore-password b/core/src/test/resources/keystore-password new file mode 100644 index 000000000000..7aa311adf93f --- /dev/null +++ b/core/src/test/resources/keystore-password @@ -0,0 +1 @@ +password \ No newline at end of file diff --git a/core/src/test/resources/truststore-password b/core/src/test/resources/truststore-password new file mode 100644 index 000000000000..7aa311adf93f --- /dev/null +++ b/core/src/test/resources/truststore-password @@ -0,0 +1 @@ +password \ No newline at end of file diff --git a/core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala b/core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala index 57d33971a57c..f88f010d3b5c 100644 --- a/core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala +++ b/core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala @@ -29,7 +29,72 @@ import org.apache.spark.util.SparkConfWithEnv class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll { - test("test resolving property file as spark conf ") { + test("missing password file") { + val hadoopConf = new Configuration() + + def testMissingPasswordFile(param: String): Unit = { + val missingFile = "/intentionally/missing/file" + val conf = getPasswordFilesSparkConf + conf.set(param, missingFile) + val msg = intercept[IllegalArgumentException] { + SSLOptions.parse(conf, hadoopConf, "spark.ssl") + }.getMessage + assert(msg.contains("No file found") && msg.contains(missingFile)) + } + + testMissingPasswordFile("spark.ssl.keyStorePasswordFile") + testMissingPasswordFile("spark.ssl.keyPasswordFile") + testMissingPasswordFile("spark.ssl.trustStorePasswordFile") + } + + test("both plain text password and file provided but differs") { + val hadoopConf = new Configuration() + + def testPasswordDiffers(param: String): Unit = { + val conf = getPasswordFilesSparkConf + conf.set(param, "12345") + val msg = intercept[IllegalArgumentException] { + SSLOptions.parse(conf, hadoopConf, "spark.ssl") + }.getMessage + assert(msg.contains(param) && msg.contains("they differ")) + } + + testPasswordDiffers("spark.ssl.keyStorePassword") + testPasswordDiffers("spark.ssl.keyPassword") + testPasswordDiffers("spark.ssl.trustStorePassword") + } + + test("test resolving property file as spark conf and plain passwords") { + testResolvingWithSparkConf(getPlainTextPasswordSparkConf) + } + + test("test resolving property file as spark conf and password files") { + testResolvingWithSparkConf(getPasswordFilesSparkConf) + } + + private def getPlainTextPasswordSparkConf: SparkConf = { + val conf = new SparkConf + conf.set("spark.ssl.keyStorePassword", "password") + conf.set("spark.ssl.keyPassword", "password") + conf.set("spark.ssl.trustStorePassword", "password") + conf + } + + private def getPasswordFilesSparkConf: SparkConf = { + val keyStorePasswordPath = + new File(this.getClass.getResource("/keystore-password").toURI).getAbsolutePath + val keyPasswordPath = + new File(this.getClass.getResource("/key-password").toURI).getAbsolutePath + val trustStorePasswordPath = + new File(this.getClass.getResource("/truststore-password").toURI).getAbsolutePath + + val conf = new SparkConf + conf.set("spark.ssl.keyStorePasswordFile", keyStorePasswordPath) + conf.set("spark.ssl.keyPasswordFile", keyPasswordPath) + conf.set("spark.ssl.trustStorePasswordFile", trustStorePasswordPath) + } + + private def testResolvingWithSparkConf(conf: SparkConf) { val keyStorePath = new File(this.getClass.getResource("/keystore").toURI).getAbsolutePath val trustStorePath = new File(this.getClass.getResource("/truststore").toURI).getAbsolutePath @@ -42,14 +107,10 @@ class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll { .take(2) .toSet - val conf = new SparkConf val hadoopConf = new Configuration() conf.set("spark.ssl.enabled", "true") conf.set("spark.ssl.keyStore", keyStorePath) - conf.set("spark.ssl.keyStorePassword", "password") - conf.set("spark.ssl.keyPassword", "password") conf.set("spark.ssl.trustStore", trustStorePath) - conf.set("spark.ssl.trustStorePassword", "password") conf.set("spark.ssl.enabledAlgorithms", algorithms.mkString(",")) conf.set("spark.ssl.protocol", "TLSv1.2") @@ -69,18 +130,22 @@ class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll { assert(opts.enabledAlgorithms === algorithms) } - test("test resolving property with defaults specified ") { + test("test resolving property with defaults specified and plain passwords") { + testResolvingWithDefaults(getPlainTextPasswordSparkConf) + } + + test("test resolving property with defaults specified and password files") { + testResolvingWithDefaults(getPasswordFilesSparkConf) + } + + private def testResolvingWithDefaults(conf: SparkConf) { val keyStorePath = new File(this.getClass.getResource("/keystore").toURI).getAbsolutePath val trustStorePath = new File(this.getClass.getResource("/truststore").toURI).getAbsolutePath - val conf = new SparkConf val hadoopConf = new Configuration() conf.set("spark.ssl.enabled", "true") conf.set("spark.ssl.keyStore", keyStorePath) - conf.set("spark.ssl.keyStorePassword", "password") - conf.set("spark.ssl.keyPassword", "password") conf.set("spark.ssl.trustStore", trustStorePath) - conf.set("spark.ssl.trustStorePassword", "password") conf.set("spark.ssl.enabledAlgorithms", "TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA") conf.set("spark.ssl.protocol", "SSLv3") @@ -103,21 +168,25 @@ class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll { Set("TLS_RSA_WITH_AES_128_CBC_SHA", "TLS_RSA_WITH_AES_256_CBC_SHA")) } - test("test whether defaults can be overridden ") { + test("test whether defaults can be overridden with plain passwords") { + testResolvingWithDefaultsAndOverwrite(getPlainTextPasswordSparkConf) + } + + test("test whether defaults can be overridden with password files") { + testResolvingWithDefaultsAndOverwrite(getPasswordFilesSparkConf) + } + + private def testResolvingWithDefaultsAndOverwrite(conf: SparkConf): Unit = { val keyStorePath = new File(this.getClass.getResource("/keystore").toURI).getAbsolutePath val trustStorePath = new File(this.getClass.getResource("/truststore").toURI).getAbsolutePath - val conf = new SparkConf val hadoopConf = new Configuration() conf.set("spark.ssl.enabled", "true") conf.set("spark.ssl.ui.enabled", "false") conf.set("spark.ssl.ui.port", "4242") conf.set("spark.ssl.keyStore", keyStorePath) - conf.set("spark.ssl.keyStorePassword", "password") conf.set("spark.ssl.ui.keyStorePassword", "12345") - conf.set("spark.ssl.keyPassword", "password") conf.set("spark.ssl.trustStore", trustStorePath) - conf.set("spark.ssl.trustStorePassword", "password") conf.set("spark.ssl.enabledAlgorithms", "TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA") conf.set("spark.ssl.ui.enabledAlgorithms", "ABC, DEF") diff --git a/docs/security.md b/docs/security.md index 20492d871b1a..9022c9b3e5af 100644 --- a/docs/security.md +++ b/docs/security.md @@ -454,11 +454,20 @@ replaced with one of the above namespaces.
Note: If not set, the default cipher suite for the JRE will be used. + + ${ns}.keyPasswordFile + None + + File which contains the password to the private key in the key store. + The file content will be read, trimmed leading/tailing whitespaces and internally set as ${ns}.keyPassword. + + ${ns}.keyPassword None The password to the private key in the key store. + The more secure solution to provide passoword is ${ns}.keyPasswordFile, please use it instead. @@ -469,10 +478,21 @@ replaced with one of the above namespaces. process is started. + + ${ns}.keyStorePasswordFile + None + + File which contains the password to the key store. + The file content will be read, trimmed leading/tailing whitespaces and internally set as ${ns}.keyStorePassword. + + ${ns}.keyStorePassword None - Password to the key store. + + Password to the key store. + The more secure solution to provide passoword is ${ns}.keyStorePasswordFile, please use it instead. + ${ns}.keyStoreType @@ -504,10 +524,21 @@ replaced with one of the above namespaces. the process is started. + + ${ns}.trustStorePasswordFile + None + + File which contains the password for the trust store. + The file content will be read, trimmed leading/tailing whitespaces and internally set as ${ns}.keyStorePassword. + + ${ns}.trustStorePassword None - Password for the trust store. + + Password for the trust store. + The more secure solution to provide passoword is ${ns}.trustStorePasswordFile, please use it instead. + ${ns}.trustStoreType From f6174a5e65acfa1f80fe34805c6ab1182445726b Mon Sep 17 00:00:00 2001 From: Gabor Somogyi Date: Thu, 21 Mar 2019 21:33:50 +0100 Subject: [PATCH 2/8] Add RAT excludes --- dev/.rat-excludes | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dev/.rat-excludes b/dev/.rat-excludes index 59e619ba109b..72786e44e8d9 100644 --- a/dev/.rat-excludes +++ b/dev/.rat-excludes @@ -115,3 +115,6 @@ structured-streaming/* kafka-source-initial-offset-version-2.1.0.bin kafka-source-initial-offset-future-version.bin vote.tmpl +key-password +keystore-password +truststore-password From f26288d1209ed0a3cd3d804901d7cd6a1476d99e Mon Sep 17 00:00:00 2001 From: Gabor Somogyi Date: Fri, 22 Mar 2019 10:37:42 +0100 Subject: [PATCH 3/8] Review fixes --- core/src/main/scala/org/apache/spark/SSLOptions.scala | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/SSLOptions.scala b/core/src/main/scala/org/apache/spark/SSLOptions.scala index 8cdac1382897..ddc54e607fd1 100644 --- a/core/src/main/scala/org/apache/spark/SSLOptions.scala +++ b/core/src/main/scala/org/apache/spark/SSLOptions.scala @@ -133,7 +133,8 @@ private[spark] case class SSLOptions( override def toString: String = s"SSLOptions{enabled=$enabled, port=$port, " + s"keyStore=$keyStore, keyStorePassword=${keyStorePassword.map(_ => "xxx")}, " + s"keyPassword=${keyPassword.map(_ => "xxx")}, keyStoreType=$keyStoreType, " + - s"trustStore=$trustStore, trustStorePassword=${trustStorePassword.map(_ => "xxx")}, " + + s"needClientAuth=$needClientAuth, trustStore=$trustStore, " + s"trustStorePassword=${trustStorePassword.map(_ => "xxx")}, " + s"trustStoreType=$trustStoreType, protocol=$protocol, enabledAlgorithms=$enabledAlgorithms}" } @@ -239,7 +240,7 @@ private[spark] object SSLOptions extends Logging { var parameterValue = conf.getWithSubstitution(s"$ns.$parameter") .orElse(Option(hadoopConf.getPassword(s"$ns.$parameter")).map(new String(_))) .orElse(default) - if (parameterValue.isDefined && default.isDefined && parameterValue.get != default.get) { + if (parameterValue.isDefined && (!default.isDefined || (parameterValue.get != default.get))) { logWarning( s"$ns.$parameter configuration parameter defined which may cause security problems. When " + "its configured as command line argument then plain text password can be dumped by " + @@ -254,7 +255,7 @@ private[spark] object SSLOptions extends Logging { val parameterFileContent = readPasswordFile(parameterFileValue.get) if (parameterValue.isDefined && parameterValue.get != parameterFileContent) { throw new IllegalArgumentException(s"Both $ns.$parameter and $ns.${parameter}File " + - "parameters defined but the they differ.") + "parameters defined but they differ.") } parameterValue = Some(parameterFileContent) } From cfc46d673d9e646b5403cf5f536602c39be7d3a4 Mon Sep 17 00:00:00 2001 From: Gabor Somogyi Date: Tue, 26 Mar 2019 13:14:41 +0100 Subject: [PATCH 4/8] Revert "Review fixes" This reverts commit f26288d1209ed0a3cd3d804901d7cd6a1476d99e. --- core/src/main/scala/org/apache/spark/SSLOptions.scala | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/SSLOptions.scala b/core/src/main/scala/org/apache/spark/SSLOptions.scala index ddc54e607fd1..8cdac1382897 100644 --- a/core/src/main/scala/org/apache/spark/SSLOptions.scala +++ b/core/src/main/scala/org/apache/spark/SSLOptions.scala @@ -133,8 +133,7 @@ private[spark] case class SSLOptions( override def toString: String = s"SSLOptions{enabled=$enabled, port=$port, " + s"keyStore=$keyStore, keyStorePassword=${keyStorePassword.map(_ => "xxx")}, " + s"keyPassword=${keyPassword.map(_ => "xxx")}, keyStoreType=$keyStoreType, " + - s"needClientAuth=$needClientAuth, trustStore=$trustStore, " - s"trustStorePassword=${trustStorePassword.map(_ => "xxx")}, " + + s"trustStore=$trustStore, trustStorePassword=${trustStorePassword.map(_ => "xxx")}, " + s"trustStoreType=$trustStoreType, protocol=$protocol, enabledAlgorithms=$enabledAlgorithms}" } @@ -240,7 +239,7 @@ private[spark] object SSLOptions extends Logging { var parameterValue = conf.getWithSubstitution(s"$ns.$parameter") .orElse(Option(hadoopConf.getPassword(s"$ns.$parameter")).map(new String(_))) .orElse(default) - if (parameterValue.isDefined && (!default.isDefined || (parameterValue.get != default.get))) { + if (parameterValue.isDefined && default.isDefined && parameterValue.get != default.get) { logWarning( s"$ns.$parameter configuration parameter defined which may cause security problems. When " + "its configured as command line argument then plain text password can be dumped by " + @@ -255,7 +254,7 @@ private[spark] object SSLOptions extends Logging { val parameterFileContent = readPasswordFile(parameterFileValue.get) if (parameterValue.isDefined && parameterValue.get != parameterFileContent) { throw new IllegalArgumentException(s"Both $ns.$parameter and $ns.${parameter}File " + - "parameters defined but they differ.") + "parameters defined but the they differ.") } parameterValue = Some(parameterFileContent) } From c1157aaad1d4f5d8477a9fe30c0323835634bc9a Mon Sep 17 00:00:00 2001 From: Gabor Somogyi Date: Tue, 26 Mar 2019 13:14:47 +0100 Subject: [PATCH 5/8] Revert "Add RAT excludes" This reverts commit f6174a5e65acfa1f80fe34805c6ab1182445726b. --- dev/.rat-excludes | 3 --- 1 file changed, 3 deletions(-) diff --git a/dev/.rat-excludes b/dev/.rat-excludes index 72786e44e8d9..59e619ba109b 100644 --- a/dev/.rat-excludes +++ b/dev/.rat-excludes @@ -115,6 +115,3 @@ structured-streaming/* kafka-source-initial-offset-version-2.1.0.bin kafka-source-initial-offset-future-version.bin vote.tmpl -key-password -keystore-password -truststore-password From 23e0908d3d21c86bbbef884ce949562ebf2b1229 Mon Sep 17 00:00:00 2001 From: Gabor Somogyi Date: Tue, 26 Mar 2019 13:14:48 +0100 Subject: [PATCH 6/8] Revert "[SPARK-26998][CORE] Add secure alternatives of ssl password parameters" This reverts commit 0ef3f8bb8f0fd06c037ea11fcd2c8a40f6ab7852. --- .../scala/org/apache/spark/SSLOptions.scala | 59 ++--------- core/src/test/resources/key-password | 1 - core/src/test/resources/keystore-password | 1 - core/src/test/resources/truststore-password | 1 - .../org/apache/spark/SSLOptionsSuite.scala | 99 +++---------------- docs/security.md | 35 +------ 6 files changed, 27 insertions(+), 169 deletions(-) delete mode 100644 core/src/test/resources/key-password delete mode 100644 core/src/test/resources/keystore-password delete mode 100644 core/src/test/resources/truststore-password diff --git a/core/src/main/scala/org/apache/spark/SSLOptions.scala b/core/src/main/scala/org/apache/spark/SSLOptions.scala index 8cdac1382897..1632e0c69eef 100644 --- a/core/src/main/scala/org/apache/spark/SSLOptions.scala +++ b/core/src/main/scala/org/apache/spark/SSLOptions.scala @@ -18,7 +18,6 @@ package org.apache.spark import java.io.File -import java.nio.file.Files import java.security.NoSuchAlgorithmException import javax.net.ssl.SSLContext @@ -132,9 +131,8 @@ private[spark] case class SSLOptions( /** Returns a string representation of this SSLOptions with all the passwords masked. */ override def toString: String = s"SSLOptions{enabled=$enabled, port=$port, " + s"keyStore=$keyStore, keyStorePassword=${keyStorePassword.map(_ => "xxx")}, " + - s"keyPassword=${keyPassword.map(_ => "xxx")}, keyStoreType=$keyStoreType, " + s"trustStore=$trustStore, trustStorePassword=${trustStorePassword.map(_ => "xxx")}, " + - s"trustStoreType=$trustStoreType, protocol=$protocol, enabledAlgorithms=$enabledAlgorithms}" + s"protocol=$protocol, enabledAlgorithms=$enabledAlgorithms}" } @@ -186,11 +184,13 @@ private[spark] object SSLOptions extends Logging { val keyStore = conf.getWithSubstitution(s"$ns.keyStore").map(new File(_)) .orElse(defaults.flatMap(_.keyStore)) - val keyStorePassword = - getPassword(conf, hadoopConf, ns, "keyStorePassword", defaults.flatMap(_.keyStorePassword)) + val keyStorePassword = conf.getWithSubstitution(s"$ns.keyStorePassword") + .orElse(Option(hadoopConf.getPassword(s"$ns.keyStorePassword")).map(new String(_))) + .orElse(defaults.flatMap(_.keyStorePassword)) - val keyPassword = - getPassword(conf, hadoopConf, ns, "keyPassword", defaults.flatMap(_.keyPassword)) + val keyPassword = conf.getWithSubstitution(s"$ns.keyPassword") + .orElse(Option(hadoopConf.getPassword(s"$ns.keyPassword")).map(new String(_))) + .orElse(defaults.flatMap(_.keyPassword)) val keyStoreType = conf.getWithSubstitution(s"$ns.keyStoreType") .orElse(defaults.flatMap(_.keyStoreType)) @@ -201,8 +201,9 @@ private[spark] object SSLOptions extends Logging { val trustStore = conf.getWithSubstitution(s"$ns.trustStore").map(new File(_)) .orElse(defaults.flatMap(_.trustStore)) - val trustStorePassword = getPassword( - conf, hadoopConf, ns, "trustStorePassword", defaults.flatMap(_.trustStorePassword)) + val trustStorePassword = conf.getWithSubstitution(s"$ns.trustStorePassword") + .orElse(Option(hadoopConf.getPassword(s"$ns.trustStorePassword")).map(new String(_))) + .orElse(defaults.flatMap(_.trustStorePassword)) val trustStoreType = conf.getWithSubstitution(s"$ns.trustStoreType") .orElse(defaults.flatMap(_.trustStoreType)) @@ -230,45 +231,5 @@ private[spark] object SSLOptions extends Logging { enabledAlgorithms) } - private def getPassword( - conf: SparkConf, - hadoopConf: Configuration, - ns: String, - parameter: String, - default: Option[String]): Option[String] = { - var parameterValue = conf.getWithSubstitution(s"$ns.$parameter") - .orElse(Option(hadoopConf.getPassword(s"$ns.$parameter")).map(new String(_))) - .orElse(default) - if (parameterValue.isDefined && default.isDefined && parameterValue.get != default.get) { - logWarning( - s"$ns.$parameter configuration parameter defined which may cause security problems. When " + - "its configured as command line argument then plain text password can be dumped by " + - "listing the process command line arguments. The more secure alternative solution is to " + - s"use $ns.${parameter}File." - ) - } - - val parameterFileValue = conf.getWithSubstitution(s"$ns.${parameter}File") - .orElse(Option(hadoopConf.getPassword(s"$ns.${parameter}File")).map(new String(_))) - if (parameterFileValue.isDefined) { - val parameterFileContent = readPasswordFile(parameterFileValue.get) - if (parameterValue.isDefined && parameterValue.get != parameterFileContent) { - throw new IllegalArgumentException(s"Both $ns.$parameter and $ns.${parameter}File " + - "parameters defined but the they differ.") - } - parameterValue = Some(parameterFileContent) - } - - parameterValue - } - - private def readPasswordFile(passwordFilePath: String): String = { - val passwordFile = new File(passwordFilePath) - require(passwordFile.isFile, s"No file found containing the password at $passwordFilePath.") - val content = new String(Files.readAllBytes(passwordFile.toPath)).trim - require(!content.isEmpty, s"Password from file located at $passwordFilePath is empty.") - content - } - } diff --git a/core/src/test/resources/key-password b/core/src/test/resources/key-password deleted file mode 100644 index 7aa311adf93f..000000000000 --- a/core/src/test/resources/key-password +++ /dev/null @@ -1 +0,0 @@ -password \ No newline at end of file diff --git a/core/src/test/resources/keystore-password b/core/src/test/resources/keystore-password deleted file mode 100644 index 7aa311adf93f..000000000000 --- a/core/src/test/resources/keystore-password +++ /dev/null @@ -1 +0,0 @@ -password \ No newline at end of file diff --git a/core/src/test/resources/truststore-password b/core/src/test/resources/truststore-password deleted file mode 100644 index 7aa311adf93f..000000000000 --- a/core/src/test/resources/truststore-password +++ /dev/null @@ -1 +0,0 @@ -password \ No newline at end of file diff --git a/core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala b/core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala index f88f010d3b5c..57d33971a57c 100644 --- a/core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala +++ b/core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala @@ -29,72 +29,7 @@ import org.apache.spark.util.SparkConfWithEnv class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll { - test("missing password file") { - val hadoopConf = new Configuration() - - def testMissingPasswordFile(param: String): Unit = { - val missingFile = "/intentionally/missing/file" - val conf = getPasswordFilesSparkConf - conf.set(param, missingFile) - val msg = intercept[IllegalArgumentException] { - SSLOptions.parse(conf, hadoopConf, "spark.ssl") - }.getMessage - assert(msg.contains("No file found") && msg.contains(missingFile)) - } - - testMissingPasswordFile("spark.ssl.keyStorePasswordFile") - testMissingPasswordFile("spark.ssl.keyPasswordFile") - testMissingPasswordFile("spark.ssl.trustStorePasswordFile") - } - - test("both plain text password and file provided but differs") { - val hadoopConf = new Configuration() - - def testPasswordDiffers(param: String): Unit = { - val conf = getPasswordFilesSparkConf - conf.set(param, "12345") - val msg = intercept[IllegalArgumentException] { - SSLOptions.parse(conf, hadoopConf, "spark.ssl") - }.getMessage - assert(msg.contains(param) && msg.contains("they differ")) - } - - testPasswordDiffers("spark.ssl.keyStorePassword") - testPasswordDiffers("spark.ssl.keyPassword") - testPasswordDiffers("spark.ssl.trustStorePassword") - } - - test("test resolving property file as spark conf and plain passwords") { - testResolvingWithSparkConf(getPlainTextPasswordSparkConf) - } - - test("test resolving property file as spark conf and password files") { - testResolvingWithSparkConf(getPasswordFilesSparkConf) - } - - private def getPlainTextPasswordSparkConf: SparkConf = { - val conf = new SparkConf - conf.set("spark.ssl.keyStorePassword", "password") - conf.set("spark.ssl.keyPassword", "password") - conf.set("spark.ssl.trustStorePassword", "password") - conf - } - - private def getPasswordFilesSparkConf: SparkConf = { - val keyStorePasswordPath = - new File(this.getClass.getResource("/keystore-password").toURI).getAbsolutePath - val keyPasswordPath = - new File(this.getClass.getResource("/key-password").toURI).getAbsolutePath - val trustStorePasswordPath = - new File(this.getClass.getResource("/truststore-password").toURI).getAbsolutePath - - val conf = new SparkConf - conf.set("spark.ssl.keyStorePasswordFile", keyStorePasswordPath) - conf.set("spark.ssl.keyPasswordFile", keyPasswordPath) - conf.set("spark.ssl.trustStorePasswordFile", trustStorePasswordPath) - } - - private def testResolvingWithSparkConf(conf: SparkConf) { + test("test resolving property file as spark conf ") { val keyStorePath = new File(this.getClass.getResource("/keystore").toURI).getAbsolutePath val trustStorePath = new File(this.getClass.getResource("/truststore").toURI).getAbsolutePath @@ -107,10 +42,14 @@ class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll { .take(2) .toSet + val conf = new SparkConf val hadoopConf = new Configuration() conf.set("spark.ssl.enabled", "true") conf.set("spark.ssl.keyStore", keyStorePath) + conf.set("spark.ssl.keyStorePassword", "password") + conf.set("spark.ssl.keyPassword", "password") conf.set("spark.ssl.trustStore", trustStorePath) + conf.set("spark.ssl.trustStorePassword", "password") conf.set("spark.ssl.enabledAlgorithms", algorithms.mkString(",")) conf.set("spark.ssl.protocol", "TLSv1.2") @@ -130,22 +69,18 @@ class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll { assert(opts.enabledAlgorithms === algorithms) } - test("test resolving property with defaults specified and plain passwords") { - testResolvingWithDefaults(getPlainTextPasswordSparkConf) - } - - test("test resolving property with defaults specified and password files") { - testResolvingWithDefaults(getPasswordFilesSparkConf) - } - - private def testResolvingWithDefaults(conf: SparkConf) { + test("test resolving property with defaults specified ") { val keyStorePath = new File(this.getClass.getResource("/keystore").toURI).getAbsolutePath val trustStorePath = new File(this.getClass.getResource("/truststore").toURI).getAbsolutePath + val conf = new SparkConf val hadoopConf = new Configuration() conf.set("spark.ssl.enabled", "true") conf.set("spark.ssl.keyStore", keyStorePath) + conf.set("spark.ssl.keyStorePassword", "password") + conf.set("spark.ssl.keyPassword", "password") conf.set("spark.ssl.trustStore", trustStorePath) + conf.set("spark.ssl.trustStorePassword", "password") conf.set("spark.ssl.enabledAlgorithms", "TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA") conf.set("spark.ssl.protocol", "SSLv3") @@ -168,25 +103,21 @@ class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll { Set("TLS_RSA_WITH_AES_128_CBC_SHA", "TLS_RSA_WITH_AES_256_CBC_SHA")) } - test("test whether defaults can be overridden with plain passwords") { - testResolvingWithDefaultsAndOverwrite(getPlainTextPasswordSparkConf) - } - - test("test whether defaults can be overridden with password files") { - testResolvingWithDefaultsAndOverwrite(getPasswordFilesSparkConf) - } - - private def testResolvingWithDefaultsAndOverwrite(conf: SparkConf): Unit = { + test("test whether defaults can be overridden ") { val keyStorePath = new File(this.getClass.getResource("/keystore").toURI).getAbsolutePath val trustStorePath = new File(this.getClass.getResource("/truststore").toURI).getAbsolutePath + val conf = new SparkConf val hadoopConf = new Configuration() conf.set("spark.ssl.enabled", "true") conf.set("spark.ssl.ui.enabled", "false") conf.set("spark.ssl.ui.port", "4242") conf.set("spark.ssl.keyStore", keyStorePath) + conf.set("spark.ssl.keyStorePassword", "password") conf.set("spark.ssl.ui.keyStorePassword", "12345") + conf.set("spark.ssl.keyPassword", "password") conf.set("spark.ssl.trustStore", trustStorePath) + conf.set("spark.ssl.trustStorePassword", "password") conf.set("spark.ssl.enabledAlgorithms", "TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA") conf.set("spark.ssl.ui.enabledAlgorithms", "ABC, DEF") diff --git a/docs/security.md b/docs/security.md index 9022c9b3e5af..20492d871b1a 100644 --- a/docs/security.md +++ b/docs/security.md @@ -454,20 +454,11 @@ replaced with one of the above namespaces.
Note: If not set, the default cipher suite for the JRE will be used. - - ${ns}.keyPasswordFile - None - - File which contains the password to the private key in the key store. - The file content will be read, trimmed leading/tailing whitespaces and internally set as ${ns}.keyPassword. - - ${ns}.keyPassword None The password to the private key in the key store. - The more secure solution to provide passoword is ${ns}.keyPasswordFile, please use it instead. @@ -478,21 +469,10 @@ replaced with one of the above namespaces. process is started. - - ${ns}.keyStorePasswordFile - None - - File which contains the password to the key store. - The file content will be read, trimmed leading/tailing whitespaces and internally set as ${ns}.keyStorePassword. - - ${ns}.keyStorePassword None - - Password to the key store. - The more secure solution to provide passoword is ${ns}.keyStorePasswordFile, please use it instead. - + Password to the key store. ${ns}.keyStoreType @@ -524,21 +504,10 @@ replaced with one of the above namespaces. the process is started. - - ${ns}.trustStorePasswordFile - None - - File which contains the password for the trust store. - The file content will be read, trimmed leading/tailing whitespaces and internally set as ${ns}.keyStorePassword. - - ${ns}.trustStorePassword None - - Password for the trust store. - The more secure solution to provide passoword is ${ns}.trustStorePasswordFile, please use it instead. - + Password for the trust store. ${ns}.trustStoreType From a1aa23c4484c86d7cd3ddcbc798f2fa6a6eb74fe Mon Sep 17 00:00:00 2001 From: Gabor Somogyi Date: Tue, 26 Mar 2019 14:24:26 +0100 Subject: [PATCH 7/8] [SPARK-26998][CORE] Remove SSL configuration from executors --- .../main/scala/org/apache/spark/SparkConf.scala | 1 - .../scala/org/apache/spark/SparkConfSuite.scala | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/org/apache/spark/SparkConf.scala b/core/src/main/scala/org/apache/spark/SparkConf.scala index 529ca3faac13..be152477cb34 100644 --- a/core/src/main/scala/org/apache/spark/SparkConf.scala +++ b/core/src/main/scala/org/apache/spark/SparkConf.scala @@ -740,7 +740,6 @@ private[spark] object SparkConf extends Logging { */ def isExecutorStartupConf(name: String): Boolean = { (name.startsWith("spark.auth") && name != SecurityManager.SPARK_AUTH_SECRET_CONF) || - name.startsWith("spark.ssl") || name.startsWith("spark.rpc") || name.startsWith("spark.network") || isSparkPortConf(name) diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala index 4ba8a3ab1c85..a0263b9a6e85 100644 --- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala +++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala @@ -354,6 +354,20 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst } } + test("SPARK-26998: SSL configuration not needed on executors") { + val conf = new SparkConf(false) + conf.validateSettings() + + conf.set("spark.ssl.enabled", "true") + conf.set("spark.ssl.keyPassword", "password") + conf.set("spark.ssl.keyStorePassword", "password") + conf.set("spark.ssl.trustStorePassword", "password") + conf.validateSettings() + + val filtered = conf.getAll.filter { case (k, _) => SparkConf.isExecutorStartupConf(k) } + assert(filtered.isEmpty) + } + val defaultIllegalValue = "SomeIllegalValue" val illegalValueTests : Map[String, (SparkConf, String) => Any] = Map( "getTimeAsSeconds" -> (_.getTimeAsSeconds(_)), From fa8cdcd6b716ea1cd1784efa9167d3ee76631224 Mon Sep 17 00:00:00 2001 From: Gabor Somogyi Date: Mon, 1 Apr 2019 10:57:30 +0200 Subject: [PATCH 8/8] Remove unnecessary validateSettings calls --- core/src/test/scala/org/apache/spark/SparkConfSuite.scala | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala index 041fe3633a9f..02aaf4af6932 100644 --- a/core/src/test/scala/org/apache/spark/SparkConfSuite.scala +++ b/core/src/test/scala/org/apache/spark/SparkConfSuite.scala @@ -356,13 +356,10 @@ class SparkConfSuite extends SparkFunSuite with LocalSparkContext with ResetSyst test("SPARK-26998: SSL configuration not needed on executors") { val conf = new SparkConf(false) - conf.validateSettings() - conf.set("spark.ssl.enabled", "true") conf.set("spark.ssl.keyPassword", "password") conf.set("spark.ssl.keyStorePassword", "password") conf.set("spark.ssl.trustStorePassword", "password") - conf.validateSettings() val filtered = conf.getAll.filter { case (k, _) => SparkConf.isExecutorStartupConf(k) } assert(filtered.isEmpty)