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

hasnain-db · 2023-10-04T18:20:24Z

What changes were proposed in this pull request?

This change adds new settings to TransportConf which are needed for the RPC SSL functionality to work. Additionally, add some sample configurations which are used by tests in follow up PRs (see #42685 for the full context)

Why are the changes needed?

These changes are needed so that other modules can easily access configurations, and that the sample configurations are easily accessible for tests.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added a test, then ran:

./build/sbt
> project network-common
> testOnly org.apache.spark.network.TransportConfSuite

There are more follow up tests coming (see #42685)

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

No

hasnain-db · 2023-10-04T18:20:59Z

I am waiting on a few more CI retries to get a clear CI signal, but this should be ready to review (had to rebase to fix python lint issues but otherwise the original build was mostly good)

hasnain-db · 2023-10-04T22:42:47Z

CI looks green so this should be good to review. cc @JoshRosen @mridulm

mridulm · 2023-10-05T05:16:04Z

common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java

+   */
+  public boolean sslRpcEnabled() {
+    return conf.getBoolean("spark.ssl.rpc.enabled", false);
+  }


Typically, there are two cases -
a) Use of signed certificates, which can be validated,
b) Use of self-signed certificates.

Given the configs documented here, how are we planning to support these ?

We support both of these.

If you like to use the JDK SSL provider, we require a keyStore argument which takes in the key being used for the server, and the trustStore is used to verify the peer we talk to.

If the openSSL provider is used, we require the privateKey and certChain arguments which have similar purposes.

You can look at the SSLFactory in the reference PR #42685 (I was going to put it up after this PR and another one) to see how it's used.

The tests in the reference PR #42685 use self signed certificates and work fine.

Does that answer your question? Happy to clarify as needed

Ok, so we are relying on the fact the if truststore is not specified, it implies self signed cert as we are going to bypass cert check.
That sounds fine to me given this is different from 'typical' usage where client is accepting server's cert and does not know if it will be trusted, or should bypass trust (if cert is trusted, use that - but if it no, should it still be trusted).

Actually, looking at it again, I want to make sure I am not missing something here.

Currently, sslRpcEnabledAndKeysAreValid will return false when key store is specified, but trust store is not - and so ssl factory will be null.

Within ssl factory, we do have code to handle the case of trust store being null resulting in accepting all server certs - but this will never get triggered since factory is null.

What am I missing here ?

You are not missing anything. Thank you for catching this. In my testing, I was using a trust store that accepts the self signed certificate and copied this check to make the branches similar. But in the JDK SSL provider case, we do not always need the trust store as you rightly pointed out, so I'm going to remove the check

updated, PTAL

mridulm

Added a comment

mridulm

Thanks for addressing the queries @hasnain-db !

mridulm · 2023-10-05T16:41:16Z

Merged to master.
Thanks for working on this @hasnain-db !

### What changes were proposed in this pull request? This change adds new settings to `TransportConf` which are needed for the RPC SSL functionality to work. Additionally, add some sample configurations which are used by tests in follow up PRs (see apache#42685 for the full context) ### Why are the changes needed? These changes are needed so that other modules can easily access configurations, and that the sample configurations are easily accessible for tests. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added a test, then ran: ``` ./build/sbt > project network-common > testOnly org.apache.spark.network.TransportConfSuite ``` There are more follow up tests coming (see apache#42685) ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#43220 from hasnain-db/spark-tls-configs-low. Authored-by: Hasnain Lakhani <hasnain.lakhani@databricks.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>

beliefer · 2023-10-08T02:47:07Z

common/network-common/src/test/java/org/apache/spark/network/ssl/SslSampleConfigs.java

+
+/**
+ *
+ */


Please supply the comment or remove it.
@hasnain-db You can do the job at #43238.

I missed this, thanks for flagging it @beliefer !

will do, thanks for catching this!

addressing in #43249

…portConf ### 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 Closes #43238 from hasnain-db/spark-tls-ssloptions. 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 4, 2023

mridulm reviewed Oct 5, 2023

View reviewed changes

mridulm approved these changes Oct 5, 2023

View reviewed changes

mridulm reviewed Oct 5, 2023

View reviewed changes

mridulm self-requested a review October 5, 2023 07:15

hasnain-db added 2 commits October 5, 2023 08:58

working

131d8e1

remove unneeded null check

e323867

hasnain-db force-pushed the spark-tls-configs-low branch from 3e11ff8 to e323867 Compare October 5, 2023 16:06

mridulm approved these changes Oct 5, 2023

View reviewed changes

mridulm closed this in f1e9dc2 Oct 5, 2023

hasnain-db mentioned this pull request Oct 6, 2023

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

Closed

beliefer reviewed Oct 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hasnain-db commented Oct 4, 2023

hasnain-db commented Oct 4, 2023

hasnain-db commented Oct 4, 2023

mridulm Oct 5, 2023

hasnain-db Oct 5, 2023

mridulm Oct 5, 2023

mridulm Oct 5, 2023

hasnain-db Oct 5, 2023

hasnain-db Oct 5, 2023

mridulm left a comment

mridulm left a comment

mridulm commented Oct 5, 2023

beliefer Oct 8, 2023

mridulm Oct 8, 2023

hasnain-db Oct 8, 2023

hasnain-db Oct 11, 2023

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

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

Conversation

hasnain-db commented Oct 4, 2023

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 4, 2023

hasnain-db commented Oct 4, 2023

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

mridulm left a comment

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

mridulm commented Oct 5, 2023

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