You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hasnain-db (via GitHub)" <gi...@apache.org> on 2023/10/06 03:04:19 UTC
[PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
hasnain-db opened a new pull request, #43240:
URL: https://github.com/apache/spark/pull/43240
### What changes were proposed in this pull request?
Add some documentation to clarify the new flags being added and how this feature interacts with existing SSL and encryption support. It's a little confusing so feedback is welcome.
Note I am not sure if it's best practice to merge this after the feature is fully merged - thought I would put this up now since it's unblocked.
### Why are the changes needed?
New features require documentation so users can understand how to use them.
### Does this PR introduce _any_ user-facing change?
Yes, this is adding documentation for a new feature
### How was this patch tested?
Not applicable, this is documentation
### Was this patch authored or co-authored using generative AI tooling?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1376768939
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
Review Comment:
Documented. Updating the PR in a second..
> QQ: Would be interesting to see what would happen in case jks and openssl configs are not 'compatible' (if jks was being specified for UI) - how spark behaves: perhaps we should disable that fallback ?
Could you clarify what you mean?
I see a few cases:
1. OpenSSL requested, and privateKey/certChain not set in the configs. This is an error and will throw saying keys are missing.
2. OpenSSL not requested, and JKS keys are specified (keyStore/trustStore). The JKS keys will be used
3. OpenSSL requested, but not available at runtime. This uses the JDK SSL implementation, but with the keys specified in `certChain` and `privateKey`.
Was that what you were asking about?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1385259389
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
Review Comment:
@mridulm I might be missing something. We clearly document two things that I believe should mean we don't have a problem:
1. We say `privateKey` and `certChain` must be PEM files,
2. We say that `privateKey` and `certChain` are only supported for the rpc namespace.
If one wants to use `keyStore` and `trustStore` it *must* be a JKS file, and you then must use the JDK SSL provider (not openssl). In that case the keys will be the same across rpc and UI and things should work fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1375832463
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
Review Comment:
Call out that this takes precedence over JKS, and that unavailability of openssl at runtime will cause spark to fall back to jks ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43240:
URL: https://github.com/apache/spark/pull/43240#issuecomment-1784445658
@mridulm could you look at this one when you have a chance? (or recommend a different reviewer if someone else is more suited to review docs PRs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1375832463
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
Review Comment:
Call out that this takes precedence over JKS, and that unavailability of openssl at runtime will cause spark to fall back to jks ?
QQ: Would be interesting to see what would happen in case jks and openssl configs are not 'compatible' (if jks was being specified for UI) - how spark behaves.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #43240: [SPARK-45431][DOCS] Document new SSL RPC feature
URL: https://github.com/apache/spark/pull/43240
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43240:
URL: https://github.com/apache/spark/pull/43240#issuecomment-1784699307
+CC @JoshRosen as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1375828446
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
+ </td>
+ </tr>
+ <tr>
+ <td><code>${ns}.privateKey</code></td>
+ <td>None</td>
+ <td>
+ Path to the private key file in PEM format. The path can be absolute or relative to the
+ directory in which the process is started.
+ This setting is only applicable to the `rpc` namespace, and is required when using the
+ OpenSSL implementation.
+ </td>
+ </tr>
+ <tr>
+ <td><code>${ns}.certChain</code></td>
+ <td>None</td>
+ <td>
+ Path to the certificate chain file in PEM format. The path can be absolute or relative to the
+ directory in which the process is started.
+ This setting is only applicable to the `rpc` namespace, and is required when using the
+ OpenSSL implementation.
+ </td>
+ </tr>
+ <tr>
+ <td><code>${ns}.trustStoreReloadingEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether the trust store should be reloaded periodically.
+ This setting is only applicable to the `rpc` namespace.
+ </td>
+ </tr>
+ <tr>
+ <td><code>${ns}.trustStoreReloadIntervalMs</code></td>
+ <td>10000</td>
+ <td>
+ The interval at which the trust store should be reloaded (in milliseconds).
+ This setting is only applicable to the `rpc` namespace.
Review Comment:
This would be useful only in standalone, right ? Call that out ?
It is not useful for k8s or yarn iirc.
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
+ </td>
+ </tr>
+ <tr>
+ <td><code>${ns}.privateKey</code></td>
+ <td>None</td>
+ <td>
+ Path to the private key file in PEM format. The path can be absolute or relative to the
+ directory in which the process is started.
+ This setting is only applicable to the `rpc` namespace, and is required when using the
+ OpenSSL implementation.
+ </td>
+ </tr>
+ <tr>
+ <td><code>${ns}.certChain</code></td>
+ <td>None</td>
+ <td>
+ Path to the certificate chain file in PEM format. The path can be absolute or relative to the
+ directory in which the process is started.
+ This setting is only applicable to the `rpc` namespace, and is required when using the
+ OpenSSL implementation.
+ </td>
+ </tr>
+ <tr>
+ <td><code>${ns}.trustStoreReloadingEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether the trust store should be reloaded periodically.
+ This setting is only applicable to the `rpc` namespace.
+ </td>
+ </tr>
+ <tr>
+ <td><code>${ns}.trustStoreReloadIntervalMs</code></td>
+ <td>10000</td>
+ <td>
+ The interval at which the trust store should be reloaded (in milliseconds).
+ This setting is only applicable to the `rpc` namespace.
Review Comment:
This would be useful only in standalone, right ? Call that out ?
It is not useful for k8s or yarn imo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1385273938
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
Review Comment:
You are right, it is namespaced to rpc, and so won't use the keystore from other namespace ... sigh, should have reread that better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1375832463
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
Review Comment:
Call out that this takes precedence over JKS, and that unavailability of openssl at runtime will cause spark to fall back to jks ?
QQ: Would be interesting to see what would happen in case jks and openssl configs are not 'compatible' (if jks was being specified for UI) - how spark behaves: perhaps we should disable that fallback ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43240:
URL: https://github.com/apache/spark/pull/43240#issuecomment-1800946245
Merged to master.
Thanks for working on this @hasnain-db !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1375825668
##########
docs/security.md:
##########
@@ -528,7 +566,7 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.keyStoreType</code></td>
<td>JKS</td>
- <td>The type of the key store.</td>
+ <td>The type of the key store. This setting is not applicable to the `rpc` namespace.</td>
Review Comment:
Do we want to include a namespace column instead of adding it this way ?
To indicate which namespace a key is applicable to ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1385278632
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
Review Comment:
no worries! this is confusing and I am open to suggestions on how to make this clearer since if it's confusing to you it might be confusing for others. We can iterate on the docs if people have confusion going forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45431][DOCS] Document new SSL RPC feature [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43240:
URL: https://github.com/apache/spark/pull/43240#discussion_r1384439623
##########
docs/security.md:
##########
@@ -563,7 +604,52 @@ replaced with one of the above namespaces.
<tr>
<td><code>${ns}.trustStoreType</code></td>
<td>JKS</td>
- <td>The type of the trust store.</td>
+ <td>The type of the trust store. This setting is not applicable to the `rpc` namespace.</td>
+ </tr>
+ <tr>
+ <td><code>${ns}.openSSLEnabled</code></td>
+ <td>false</td>
+ <td>
+ Whether to use OpenSSL for cryptographic operations instead of the JDK SSL provider.
+ This setting is only applicable to the `rpc` namespace, and also requires the `certChain`
+ and `privateKey` settings to be set.
Review Comment:
I am referring to (3).
If openssl is requested but is not available at runtime, we will try to fallback to jks.
The openssl config for `privateKey`/`certChain` need not be compatible with the `certChain`/`privateKey` for jks - for example, openssl for rpc vs jks for ui.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org