[SPARK-45427][CORE] Add RPC SSL settings to SSLOptions and SparkTransportConf #43238

hasnain-db · 2023-10-06T02:17:35Z

What changes were proposed in this pull request?

This PR adds the options added in #43220 to SSLOptions and SparkTransportConf.

By adding it to the SSLOptions we can support inheritance of options, so settings for the UI and RPC SSL settings can be shared as much as possible. The SparkTransportConf changes are needed to support propagating these settings.

I also make some changes to SecurityManager to log when this feature is enabled, and make the existing spark.network.crypto options mutually exclusive with this new settings (it would just involve double encryption then).

Lastly, make these flags propagate down to when executors are launched, and allow the passwords to be sent via environment variables (similar to how it's done for an existing secret). This ensures they are not visible in plaintext, but also ensures they are available at executor startup (otherwise it can never talk to the driver/worker)

Why are the changes needed?

The propagation of these options are needed for the RPC functionality to work

Does this PR introduce any user-facing change?

No

How was this patch tested?

CI

Added some unit tests which I verified passed:

build/sbt
> project core
> testOnly org.apache.spark.SparkConfSuite org.apache.spark.SSLOptionsSuite org.apache.spark.SecurityManagerSuite org.apache.spark.deploy.worker.CommandUtilsSuite

The rest of the changes and integration were tested as part of #42685

Was this patch authored or co-authored using generative AI tooling?

No

hasnain-db · 2023-10-06T03:15:28Z

cc: @mridulm @JoshRosen

mridulm

w.r.t the config namespace, we currently have:

spark.ssl for UI related SSL configs.
rpc IO is currently under spark.network.crypto
spark.rpc. for various rpc related configs, but they dont deal with security.
spark.io.encryption which deals with block encryption.

IMO we should namespace to spark.network and add ssl there, instead of moving these configs for rpc to spark.ssl.

Thoughts @JoshRosen, @hasnain-db ?

core/src/main/scala/org/apache/spark/SSLOptions.scala

core/src/main/scala/org/apache/spark/network/netty/SparkTransportConf.scala

core/src/main/scala/org/apache/spark/SecurityManager.scala

core/src/main/scala/org/apache/spark/SparkConf.scala

core/src/main/scala/org/apache/spark/SSLOptions.scala

hasnain-db · 2023-10-08T05:45:36Z

w.r.t the config namespace, we currently have ...
IMO we should namespace to spark.network and add ssl there, instead of moving these configs for rpc to spark.ssl.

My main preference here was to use the existing spark.ssl settings because there is a nice interop with how SSL works for the UI: we can reuse the same keystore and trustore and protocol settings and have things be consistent. We can also reuse the existing code for option inheritance then without much hassle. We could make this reuse existing settings under a difference namespace (if we want to offer the same UX) but then the code gets a bit messier (which is probably still fine if we feel strongly about prioritizing the UX and using the spark.network namespace).

I agree it's a little confusing as it's not under the network settings, but I don't see it being a stretch for a user to expect SSL settings to be under spark.ssl - that's naively where I checked initially when looking for RPC SSL support.

What do you think? I'm happy to defer to you and @JoshRosen for the final call here.

hasnain-db · 2023-10-11T05:29:26Z

addressed part of the feedback. Still waiting for @JoshRosen to also chime in with his thoughts re: which config namespace to use.

JoshRosen

I don't have super strong feelings about the configuration namespacing: I'm okay with any option as long as our docs help users to chart a clear course for common deployment scenarios.

Some assorted thoughts:

As we think through this, it might be useful to also review some of the proposed documentation updates from the main PR at https://github.com/apache/spark/pull/42685/files#diff-2c506cc756a5333f8fb5acb850d443980706602048f9ddb3dc515abacbfbce6a (in security.md) to see if we think these scenarios make sense.
- Re-reading this now, I think we might want to re-work the page's introduction to offer more clarity on the different options for encryption so users have a clearer understanding of which options are mutually-exclusive. For example, we should clarify whether the existing shared secret authentication can be used with SSL.
- We might want to re-work the intro to more clearly state that there are two mutually-exclusive choices for RPC encryption, and might want to leave some breadcrums cross-linking some sub-sections to reiterate this point.
There may be some advantages to the hierarchical namespacing of SSL configurations for the sake of giving users a single place to configure enabledAlgorithms (e.g. if a user has a compliance protocol that allows only certain algorithms then they shouldn't have to repeat themselves and keep multiple configurations in sync (although I suppose we could have a separate configuration that just inherits its default from the main SSL options)).
In the long run, do we expect that new users would choose to use non-SSL RPC encryption mechanisms? Or would they configure SSL everywhere? Do we expect the existing non-UI encryption configurations to eventually be deprecated (SASL already is)? I'm wondering if we use some long-term vision of a future end state to guide us here.

hasnain-db · 2023-10-11T16:03:39Z

FYI The open PR for the docs is here: #43240 -- happy to address and make changes there.

hasnain-db · 2023-10-12T04:36:32Z

@JoshRosen I updated the docs PR to hopefully add more clarity around the various encryption options (and, to your request, clarified how they overlap with the authentication setting).

There may be some advantages to the hierarchical namespacing of SSL configurations...

Agreed. If we reuse the spark.ssl.rpc namespace as is done in the PR the implementation is also much easier -- but we can also make the settings inheritance work if we use a different namespace for it - implementation complexity shouldn't hinder UX IMO.

I'm wondering if we use some long-term vision of a future end state to guide us here.

I don't have enough context to make a call here and defer to @mridulm . I will say that as a random observer here who hasn't had to manage too many spark deployments just yet, I feel like the current encryption mechanism has a strong point in its favor: it's quite simple to configure just a shared secret. SSL requires provisioning keys and certs which is always more work.

mridulm · 2023-10-12T20:58:53Z

I was trying to see if spark.network.ssl made more sense, and why we have spark.ssl and not spark.ui.ssl (given current use is for UI).

We used to have more ssl related configs in the past under that namespace - spark.ssl.akka, spark.ssl.fs (file server), etc ... with the intent to keep spark.ssl as the namespace for various module's ssl config.

Given this, even though today it is just UI, it does make more sense to go back to that approach.
Let us keep it as proposed in this PR.

mridulm

Just a couple of nits, looks good to me

core/src/main/scala/org/apache/spark/network/netty/SparkTransportConf.scala

core/src/main/scala/org/apache/spark/SecurityManager.scala

hasnain-db · 2023-10-13T21:33:20Z

test failure looks unrelated, retrying for the third time

hasnain-db · 2023-10-14T03:35:51Z

org.apache.spark.sql.kafka010.KafkaSourceStressSuite is failing on this and the other outstanding PR, and seems to be unrelated to the changes here. I wonder if that is related to the new kafka version bump since it looks like it also failed on https://github.com/apache/spark/actions/runs/6497942586/job/17648192318

mridulm · 2023-10-14T07:36:58Z

The failures are not related.
Merging to master.
Thanks for working on this @hasnain-db !
Thanks for the review @JoshRosen :-)

…rtConf ### What changes were proposed in this pull request? This change ensures that RPC SSL options settings inheritance works properly after #43238 - we pass `sslOptions` wherever we call `fromSparkConf`. In addition to that minor mechanical change, duplicate/add tests for every place that calls this method, to add a test case that runs with SSL support in the config. ### Why are the changes needed? These changes are needed to ensure that the RPC SSL functionality can work properly with settings inheritance. In addition, through these tests we can ensure that any changes to these modules are also tested with SSL support and avoid regressions in the future. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Full integration testing also done as part of #42685 Added some tests and ran them: ``` build/sbt > project core > testOnly org.apache.spark.*Ssl* > testOnly org.apache.spark.network.netty.NettyBlockTransferSecuritySuite ``` and ``` build/sbt -Pyarn > project yarn > testOnly org.apache.spark.network.yarn.SslYarnShuffleServiceWithRocksDBBackendSuite ``` ### Was this patch authored or co-authored using generative AI tooling? No Closes #43387 from hasnain-db/spark-tls-integrate-everywhere. Authored-by: Hasnain Lakhani <hasnain.lakhani@databricks.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>

github-actions bot added the CORE label Oct 6, 2023

mridulm reviewed Oct 8, 2023

View reviewed changes

beliefer mentioned this pull request Oct 8, 2023

[SPARK-45408][CORE] Add RPC SSL settings to TransportConf #43220

Closed

beliefer reviewed Oct 8, 2023

View reviewed changes

core/src/main/scala/org/apache/spark/SSLOptions.scala Outdated Show resolved Hide resolved

hasnain-db added 2 commits October 10, 2023 22:09

working

0d64bc4

address feedback

91ba3a8

hasnain-db force-pushed the spark-tls-ssloptions branch from ed3dea0 to 91ba3a8 Compare October 11, 2023 05:28

JoshRosen reviewed Oct 11, 2023

View reviewed changes

fix mima

1b00748

github-actions bot added the BUILD label Oct 11, 2023

hasnain-db requested review from JoshRosen and mridulm October 12, 2023 04:36

mridulm approved these changes Oct 12, 2023

View reviewed changes

core/src/main/scala/org/apache/spark/network/netty/SparkTransportConf.scala Show resolved Hide resolved

core/src/main/scala/org/apache/spark/SecurityManager.scala Outdated Show resolved Hide resolved

comments

fd08012

mridulm closed this in 26aaf1c Oct 14, 2023

hasnain-db mentioned this pull request Oct 16, 2023

[SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf #43387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-45427][CORE] Add RPC SSL settings to SSLOptions and SparkTransportConf #43238

[SPARK-45427][CORE] Add RPC SSL settings to SSLOptions and SparkTransportConf #43238

hasnain-db commented Oct 6, 2023 •

edited

Loading

hasnain-db commented Oct 6, 2023

mridulm left a comment

hasnain-db commented Oct 8, 2023 •

edited

Loading

hasnain-db commented Oct 11, 2023

JoshRosen left a comment •

edited

Loading

hasnain-db commented Oct 11, 2023

hasnain-db commented Oct 12, 2023

mridulm commented Oct 12, 2023

mridulm left a comment

hasnain-db commented Oct 13, 2023

hasnain-db commented Oct 14, 2023

mridulm commented Oct 14, 2023

[SPARK-45427][CORE] Add RPC SSL settings to SSLOptions and SparkTransportConf #43238

[SPARK-45427][CORE] Add RPC SSL settings to SSLOptions and SparkTransportConf #43238

Conversation

hasnain-db commented Oct 6, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

hasnain-db commented Oct 6, 2023

mridulm left a comment

Choose a reason for hiding this comment

hasnain-db commented Oct 8, 2023 • edited Loading

hasnain-db commented Oct 11, 2023

JoshRosen left a comment • edited Loading

Choose a reason for hiding this comment

hasnain-db commented Oct 11, 2023

hasnain-db commented Oct 12, 2023

mridulm commented Oct 12, 2023

mridulm left a comment

Choose a reason for hiding this comment

hasnain-db commented Oct 13, 2023

hasnain-db commented Oct 14, 2023

mridulm commented Oct 14, 2023

hasnain-db commented Oct 6, 2023 •

edited

Loading

hasnain-db commented Oct 8, 2023 •

edited

Loading

JoshRosen left a comment •

edited

Loading