[SPARK-542] Add and document Driver<->Executor TLS support. #188

ArtRand · 2017-09-22T23:49:42Z

TLS in Spark

This PR is about 2 things.

Update documentation and expectations surrounding TLS in Spark right now.
Propose a temporary solution.

How it works

Spark has native support for TLS with java KeyStores. It requires the KeyStore and optionally the TrustStore (hereafter referred to as Stores) to be in the sandbox of the Driver and the Executors.

The solution implemented here uses environment based secrets. The user base64 encodes their Stores and puts them in the DC/OS secret store. Then you use add the appropriate configuration (either on the command line or with a properties file) and the base64 string is injected in the Driver and executor environments. The pre-start script spark-env.sh notices these environment variables and decodes them to files.

The question.

This solution is similar to the older Kerberos implementation, which is why I asked @triclambert and @vishnu2kmohan to review this PR. My question is: is this solution inherently insecure and thus we should not even allow/advise customers to use it? Is exposing the Stores files in the sandbox a common practice?

The better alternative is to use file-based secrets as we do for Kerberos. Unfortunately, the MesosCoarseGrainedSchedulerBackend (the Driver) does not have secrets support which it needs to to start the executors with secrets. I recently added secret support to the Dispatcher (in Spark) and it would be a trivial change to add it to the Driver also (but would require the ASF review process).

Thanks for the input.

skonto · 2017-09-25T08:24:45Z

conf/spark-env.sh

+  else
+      BASE64_D="base64 -D" # BSD
+fi
+


How about base64 --decode, should be the same:
https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man1/base64.1.html
http://manpages.ubuntu.com/manpages/zesty/man1/base64.1.html

skonto · 2017-09-25T09:06:30Z

@ArtRand I think sandbox access is protected, yet depending on different envs it would be good to support both options. For example with DC/OS you have better handling of env secrets using the secret store. Pure mesos does not provide such support out of the box. Would it be possible that the end-user would like to store all credentials in his key store? Hope I don't miss anything here.

sascala

little edits

sascala · 2017-09-25T16:19:16Z

docs/security.md

@@ -37,26 +40,35 @@ Both stores must be base64 encoded, for example:

 **Note:** The base64 string of the keystore will probably be much longer than the snippet above, spanning 50 lines or so.

-With this and the password `secret` for the keystore and the private key, your JSON options file will look like this:
+Add the stores to your secrets in the DC/OS Secret store, for example if your base64 encoded keystores and truststores are server.jks.base64 and trust.jks.base64, respectively then do the following: 


Add the stores to your secrets in the DC/OS secret store. For example, if your base64-encoded keystores and truststores are server.jks.base64 and trust.jks.base64, respectively, then use the following commands to add them to the secret store:

sascala · 2017-09-25T16:19:46Z

docs/security.md

-        }
-      }
-    }
+In this case you're adding two secrets `/truststore` and `/keystore` that you will need to pass to the Spark Driver and Executors. You will need to add the following configurations to your `dcos spark run ` command:


In this case, you are adding....

You must add the following configurations...

sascala · 2017-09-25T16:22:47Z

docs/security.md


-Make sure to connect the DC/OS cluster only using an SSL connection (i.e. by using an `https://<dcos-url>`). Use the following command to set your DC/OS URL:
+Importantly the `spark.mesos.driver.labels` and `spark.mesos.task.labels` must be set as shown. If you upload your secret with another path (e.g. not `/keystore` and `/truststore`) then change the `name` in the value accordingly. Lastly, `spark.mesos.task.lables` must have the `DCOS_SPACE:<dcos_space>` label as well, to have access to the secret. See the [Secrets Documentation about SPACES][13] for more details about Spaces, but usually you want `/spark` as shown.


Note: The spark.mesos.driver.labels and spark.mesos.task.labels must be set as shown. If you upload your secret with another path (e.g. not /keystore and /truststore), then change the name in the value accordingly. Lastly, spark.mesos.task.lables must have also have the DCOS_SPACE:<dcos_space> label in order to access the secret. See the [Secrets Documentation about spaces][13] for more details about spaces. Usually, you will want to set the space label to /spark, as shown.

susanxhuynh

Thanks, @ArtRand Should we add a regression test for this?

susanxhuynh · 2017-09-25T23:40:22Z

docs/security.md


 [11]: https://docs.mesosphere.com/1.9/overview/architecture/components/
 [12]: http://docs.oracle.com/javase/8/docs/technotes/tools/unix/keytool.html
+ [13]: https://docs.mesosphere.com/service-docs/spark/v2.0.1-2.2.0-1/run-job/


As referenced above, this link is supposed to point to "Secrets Documentation about SPACES".

susanxhuynh · 2017-09-25T23:44:59Z

docs/security.md

+| Variable                         | Description                                     |
+|----------------------------------|-------------------------------------------------|
+| `spark.ssl.enabled`              | Whether to enable SSL (default: `false`).       |
+| `spark.ssl.keyStoreBase64`       | Base64 encoded blob containing a Java keystore. |


I think this is not required anymore, if using secrets.

susanxhuynh · 2017-09-25T23:53:05Z

docs/security.md

+--conf spark.ssl.trustStore=trust.jks \  # this MUST be set this way
+--conf spark.ssl.trustStorePassword=<truststore password> \
+--conf spark.mesos.driver.labels=DCOS_SECRETS_DIRECTIVE:[{\"name\"\:\"/keystore\"\,\"type\"\:\"ENVIRONMENT\"\,\"environment\"\:{\"name\"\:\"KEYSTORE_BASE64\"}}\,{\"name\"\:\"/truststore\"\,\"type\"\:\"ENVIRONMENT\"\,\"environment\"\:{\"name\"\:\"TRUSTSTORE_BASE64\"}}] \
+--conf spark.mesos.task.labels=DCOS_SECRETS_DIRECTIVE:[{\"name\"\:\"/keystore\"\,\"type\"\:\"ENVIRONMENT\"\,\"environment\"\:{\"name\"\:\"KEYSTORE_BASE64\"}}\,{\"name\"\:\"/truststore\"\,\"type\"\:\"ENVIRONMENT\"\,\"environment\"\:{\"name\"\:\"TRUSTSTORE_BASE64\"}}],DCOS_SPACE:/spark, \


This is a pretty long command. Did you consider setting some of these configs automatically in the CLI? Or, do you want the user to be aware of each config being set?

mhrabovcin · 2017-09-27T11:20:59Z

conf/spark-env.sh

    echo "spark-env: Copying krb config from $KRB5_CONFIG_BASE64 to /etc/" >&2
    echo "${KRB5_CONFIG_BASE64}" | ${BASE64_D} > /etc/krb5.conf
 else
    echo "spark-env: No kerberos KDC config found" >&2
 fi

+if [[ -n "${KEYSTORE_BASE64}" ]]; then
+    echo "spark-env: Copying keystore to SANDBOX" >&2
+    echo "${KEYSTORE_BASE64}" | ${BASE64_D} > ${MESOS_SANDBOX}/server.jks


I'd suggest using file based secrets that are more secure than writing contents of keystore to mesos sandbox.

susanxhuynh · 2017-09-27T15:57:58Z

This will be replaced with file-based secrets.

susanxhuynh · 2017-10-09T22:21:58Z

I'm going to reopen this PR and replace the env-based secrets with file-based secrets.

susanxhuynh · 2017-10-16T15:19:29Z

I have replaced the env-based secrets with file-based secrets, pending this Apache Spark PR: apache/spark#19437
This PR contains the related documentation and an integration test.
cc @ArtRand @skonto @mhrabovcin @sascala

ArtRand

Just a few nits. Thanks!

ArtRand · 2017-10-27T16:15:04Z

tests/test_spark.py

+    truststore_path = os.path.join(resources_folder, '{}.base64'.format(truststore_file))
+    keystore_secret = '__dcos_base64__keystore'
+    truststore_secret = '__dcos_base64__truststore'
+    shakedown.run_dcos_command('security secrets create /{} --value-file {}'.format(keystore_secret, keystore_path))


how come this works now? I remember having to jump through some hoops to get the secrets API to work (https://github.com/mesosphere/spark-build/blob/master/tests/utils.py#L179). Could you homogenize the way secrets are added in the tests by either using the SecretsHandler or changing/removing it?

ArtRand · 2017-10-27T16:17:35Z

tests/test_spark.py

@@ -318,6 +318,53 @@ def test_secrets():
        LOGGER.warn("Error when deleting secret, {}".format(r.content))


+# Skip DC/OS < 1.10, because it doesn't have support for file-based secrets.
+@pytest.mark.skip("Enable when SPARK-22131 is merged and released in DC/OS Spark")


I assume you've tested this locally. How about we test with a specific docker image (that has the patch, built from master) instead of the universe package. I don't like the idea of merging "skipped tests"

ArtRand · 2017-10-27T16:21:44Z

docs/security.md

+--conf spark.ssl.protocol=TLS \
+--conf spark.ssl.trustStore=trust.jks \  # this MUST be set this way
+--conf spark.ssl.trustStorePassword=<truststore password> \
+--conf spark.mesos.driver.secret.names=__dcos_base64__keystore,__dcos_base64__truststore \


@keithchambers should probably review this. We could add some sugar to the CLI so these extra configs aren't required (like we do with Kerberos), but it becomes the same thing.

ArtRand · 2017-10-27T16:25:09Z

docs/security.md

+--conf spark.mesos.driver.secret.filenames=server.jks,trust.jks \
+--conf spark.mesos.executor.secret.names=__dcos_base64__keystore,__dcos_base64__truststore \
+--conf spark.mesos.executor.secret.filenames=server.jks,trust.jks \
+--conf spark.mesos.task.labels=DCOS_SPACE:/spark \


we should probably add this via the CLI automatically.

Would it be more intuitive for the secret:file pairs to be together?
e.g., conf spark.mesos.executor.secrets=secret1:file1,secret2:file2

susanxhuynh · 2017-11-03T16:47:18Z

docs/security.md

-    }
+In this case, you are adding two secrets `/truststore` and `/keystore`. 
+You must add the following configurations to your `dcos spark run ` command.
+The ones in parentheses are optional.:


Good point, @keithchambers . Actually, that was the old verbose command. Here is the simplified one. What do you think? --

susanxhuynh · 2017-11-20T22:50:44Z

@ArtRand I've simplified the interface a bit. There are only three required properties now, and some optional ones. PTAL. cc @skonto

ArtRand · 2017-11-21T18:25:00Z

cli/dcos-spark/submit_builder.go

+		paths = append(paths, args.truststoreSecretPath)
+		filenames = append(filenames, trustStoreFileName)
+	}
+	args.properties["spark.mesos.driver.secret.names"] = strings.Join(paths, ",")


will this overwrite someone's specified secrets if they've already specified some?
i.e.

docs spark run --submit-args="\ ... --conf spark.mesos.driver.secret.names=awsaccess \ --conf spark.mesos.driver.secret.filenames=awsaccess \ ...

ArtRand · 2017-11-21T19:04:01Z

cli/dcos-spark/submit_builder.go

@@ -43,6 +43,8 @@ type sparkArgs struct {
 	keytabSecretPath	string
 	tgtSecretPath		string
 	tgtSecretValue		string
+	keystoreSecretPath	string


what about adding a password because the other "magic" configs are here also?

ArtRand · 2017-11-21T19:05:32Z

cli/dcos-spark/submit_builder.go

+		// Make sure passwords are set
+		if _, ok := args.properties["spark.ssl.keyPassword"]; !ok {
+			return "", errors.New("Need to provide key password with keystore: " +
+				"--conf spark.ssl.keyPassword=<password>")


again, I think we should add a DC/OS specific flag here otherwise your TLS args are both in the DC/OS-specific args and the submit-args

ArtRand · 2017-11-21T19:06:06Z

docs/security.md

+| Variable                         | Description                                     | Default Value |
+|----------------------------------|-------------------------------------------------|---------------|
+| `spark.ssl.enabledAlgorithms`    | Allowed cyphers                                 | JVM defaults  |
+| `spark.ssl.keyPassword`          | The password for the private key                | None          |


Change default value to REQUIRED? @sascala ?

…upport

…isabled).

…onfig properties.

susanxhuynh · 2017-11-22T03:34:58Z

@ArtRand

Decided to go with "magic" options for passwords. I figured since any user will have to figure out our DC/OS-specific --keystore-secret-path option, it's not too much trouble to add the password ones also.
Made sure any additional user-defined secrets are preserved, added placeholders for env-based secrets.
Waiting for tests to pass one last time.

ArtRand · 2017-11-22T16:51:02Z

cli/dcos-spark/submit_builder.go

+		if args.keystorePassword == "" || args.privateKeyPassword == "" {
+			return "", errors.New("Need to provide keystore password and key password with keystore")
+		} else {
+			args.properties["spark.ssl.keyStorePassword"] = args.keystorePassword


nit/question: any reason the password configs are set here instead of in `setupTLSArgs?

No particular reason. I'll move the setting into setupTLSArgs and just leave the validation check here.

ArtRand · 2017-11-22T16:57:44Z

tests/jobs/python/pi_with_secret.py

+        Then calculates the value of pi.
+    """
+
+    check_secret(sys.argv[2], sys.argv[3])


does this also check the secret in the executors or just the driver?

I didn't bother adding a test for executor secrets because the CLI code (wrt TLS) is the same for driver and executor secrets.

ArtRand · 2017-11-22T18:34:23Z

docs/security.md


-    dcos package install --options=options.json spark
+**Note:** If you have specified a space for your secrets other than the default value,


I think you can specify this with a "magic" flag? d2iq-archive/dcos-commons#1872

Which magic flag? The link ^^ doesn't seem relevant?

ArtRand · 2017-11-22T18:35:54Z

docs/security.md

-Make sure to connect the DC/OS cluster only using an SSL connection (i.e. by using an `https://<dcos-url>`). Use the following command to set your DC/OS URL:
+**Note:** If you specify environment-based secrets with `spark.mesos.[driver|executor].secret.envkeys`,
+the keystore and truststore secrets will also show up as environment-based secrets,
+due to the way secrets are implemented. You can ignore these extra environment variables.


This should also be mentioned in Limitations (https://docs.mesosphere.com/service-docs/spark/2.1.0-2.2.0-1/limitations/)

ArtRand · 2017-11-22T18:38:27Z

tests/test_spark.py

@@ -286,5 +290,51 @@ def test_cli_multiple_spaces():
                          " --class  ", "org.apache.spark.examples.SparkPi"])


+# Skip DC/OS < 1.10, because it doesn't have support for file-based secrets.
+@pytest.mark.skipif('shakedown.dcos_version_less_than("1.10")')


Need to skip on open also.

ArtRand · 2017-11-22T18:40:42Z

tests/test_spark.py

+# Skip DC/OS < 1.10, because it doesn't have support for file-based secrets.
+@pytest.mark.skipif('shakedown.dcos_version_less_than("1.10")')
+@pytest.mark.sanity
+def test_driver_executor_tls():


Could we make the job rely on a file being served by the driver (i.e. the channel effected by TLS)? This may be more work than necessary because we'd need to enable/check the --files flag.

ArtRand · 2017-11-22T18:42:50Z

cli/dcos-spark/submit_builder.go

+	for _, taskType := range taskTypes {
+		appendToProperty(fmt.Sprintf("spark.mesos.%s.secret.names", taskType), joinedPaths, args)
+		appendToProperty(fmt.Sprintf("spark.mesos.%s.secret.filenames", taskType), joinedFilenames, args)
+		appendToPropertyIfSet(fmt.Sprintf("spark.mesos.%s.secret.envkeys", taskType), joinedEnvkeys, args)


ArtRand · 2017-11-22T18:45:16Z

tests/jobs/python/pi_with_secret.py

+
+def check_secret(secret_name, secret_content):
+    '''
+    Make sure the extra secret envvar and secret file show up in driver.


check driver and executor?

…nfigs.

ArtRand requested review from vishnu2kmohan, sascala, triclambert and susanxhuynh September 22, 2017 23:49

skonto reviewed Sep 25, 2017

View reviewed changes

sascala reviewed Sep 25, 2017

View reviewed changes

susanxhuynh reviewed Sep 25, 2017

View reviewed changes

mhrabovcin reviewed Sep 27, 2017

View reviewed changes

susanxhuynh closed this Sep 27, 2017

susanxhuynh reopened this Oct 9, 2017

susanxhuynh force-pushed the spark-542-tls-support-and-docs branch from ca8ae07 to 2a7a40a Compare October 15, 2017 19:52

ArtRand commented Oct 27, 2017

View reviewed changes

susanxhuynh force-pushed the spark-542-tls-support-and-docs branch from 24d75c8 to e223ddf Compare November 2, 2017 18:56

susanxhuynh reviewed Nov 3, 2017

View reviewed changes

susanxhuynh force-pushed the spark-542-tls-support-and-docs branch 2 times, most recently from 6c5246b to e7b869b Compare November 20, 2017 19:07

ArtRand commented Nov 21, 2017

View reviewed changes

ArtRand and others added 8 commits November 21, 2017 19:28

add env var decoding and documentation about driver-to-executor tls s…

e4828ee

…upport

Switched to file-based secrets.

90889bd

Reverted spark-env.sh change. Added TLS integration test (currently d…

85483d6

…isabled).

Updated docs, incorporating suggestions from Suzanne.

60cb154

Removed mention of driver labels in docs.

4d677f0

Consolidated secrets creation code, removed SecretHandler.

cc89073

Simplified the options in the run command.

4008a4a

Simplified the interface by automatically inferring some of the TLS c…

0a21adc

…onfig properties.

susanxhuynh added 3 commits November 21, 2017 19:28

Better --help and error messages, updated docs.

56cda6b

Make sure user-defined secrets are preserved. Updated test and docs.

103d825

Converted passwords to "magic" options. Updated test and docs.

17bccac

susanxhuynh force-pushed the spark-542-tls-support-and-docs branch from 904a4a0 to 17bccac Compare November 22, 2017 03:30

ArtRand commented Nov 22, 2017

View reviewed changes

Added blurb in limitations docs section, moved setting of password co…

ea4267c

…nfigs.

susanxhuynh merged commit 022f332 into master Nov 22, 2017

susanxhuynh deleted the spark-542-tls-support-and-docs branch November 22, 2017 21:13


		Make sure to connect the DC/OS cluster only using an SSL connection (i.e. by using an `https://<dcos-url>`). Use the following command to set your DC/OS URL:
		Importantly the `spark.mesos.driver.labels` and `spark.mesos.task.labels` must be set as shown. If you upload your secret with another path (e.g. not `/keystore` and `/truststore`) then change the `name` in the value accordingly. Lastly, `spark.mesos.task.lables` must have the `DCOS_SPACE:<dcos_space>` label as well, to have access to the secret. See the [Secrets Documentation about SPACES][13] for more details about Spaces, but usually you want `/spark` as shown.


		dcos package install --options=options.json spark
		Note: If you have specified a space for your secrets other than the default value,

[SPARK-542] Add and document Driver<->Executor TLS support. #188

[SPARK-542] Add and document Driver<->Executor TLS support. #188

Uh oh!

Conversation

ArtRand commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TLS in Spark

How it works

The question.

Uh oh!

skonto Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sascala left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susanxhuynh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susanxhuynh commented Sep 27, 2017

Uh oh!

susanxhuynh commented Oct 9, 2017

Uh oh!

susanxhuynh commented Oct 16, 2017

Uh oh!

ArtRand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtRand Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtRand Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susanxhuynh commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtRand Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susanxhuynh commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ArtRand commented Sep 22, 2017 •

edited

Loading

skonto Sep 25, 2017 •

edited

Loading

skonto commented Sep 25, 2017 •

edited

Loading

ArtRand Oct 27, 2017 •

edited

Loading

ArtRand Oct 27, 2017 •

edited

Loading

susanxhuynh commented Nov 20, 2017 •

edited

Loading

ArtRand Nov 21, 2017 •

edited

Loading