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/11/24 05:06:39 UTC
[PR] [SPARK-46058][CORE] Add separate flag for privateKeyPassword [spark]
hasnain-db opened a new pull request, #43998:
URL: https://github.com/apache/spark/pull/43998
### What changes were proposed in this pull request?
This PR adds a separate way of configuring the private key for RPC SSL support when using openssl.
### Why are the changes needed?
Right now with config inheritance we support:
* JKS with password A, PEM with password B
* JKS with no password, PEM with password A
* JKS and PEM with no password
But we do not support the case where JKS has a password and PEM does not. If we set `keyPassword` we will attempt to use it, and cannot set `spark.ssl.rpc.keyPassword` to null to override the password. So let's make it a separate flag as the easiest workaround.
This was noticed while migrating some existing deployments to the RPC SSL support where we use openssl support for RPC and use a key with no password
### Does this PR introduce _any_ user-facing change?
Yes, this affects how the (currently unreleased) RPC SSL feature is configured going forward
### How was this patch tested?
Updated test configs to match the issue I saw, which would fail `SSLFactory.init()` saying key was invalid. Tests now pass.
```
build/sbt
> project network-common
> testOnly
> project network-shuffle
> testOnly
> project core
> test *Ssl*
```
### 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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43998:
URL: https://github.com/apache/spark/pull/43998#issuecomment-1841803523
cc @mridulm in case you want to take a look.
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43998:
URL: https://github.com/apache/spark/pull/43998#issuecomment-1828329150
cc @mridulm @JoshRosen let me know if this makes sense or if there's a way to do this purely via existing configs (in which case I can abandon this PR and just update docs)
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "JoshRosen (via GitHub)" <gi...@apache.org>.
JoshRosen commented on code in PR #43998:
URL: https://github.com/apache/spark/pull/43998#discussion_r1406981160
##########
docs/security.md:
##########
@@ -636,6 +636,14 @@ replaced with one of the above namespaces.
</td>
<td>rpc</td>
</tr>
+ <tr>
+ <td><code>${ns}.keyPassword</code></td>
Review Comment:
```suggestion
<td><code>${ns}.privateKeyPassword</code></td>
```
?
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43998:
URL: https://github.com/apache/spark/pull/43998#issuecomment-1829034324
Thank you @JoshRosen ! I've fixed the typo and filed a ticket to track the additional item - you have shown me how to fix it (I'd gotten confused in my last look). Hope it's OK to punt that change to a different PR.
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43998:
URL: https://github.com/apache/spark/pull/43998#issuecomment-1830868251
CI failure is in generating docs but that looks unrelated to this PR
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "JoshRosen (via GitHub)" <gi...@apache.org>.
JoshRosen commented on code in PR #43998:
URL: https://github.com/apache/spark/pull/43998#discussion_r1406993411
##########
common/network-common/src/main/java/org/apache/spark/network/ssl/SSLFactory.java:
##########
@@ -106,7 +106,7 @@ private void initNettySslContexts(final Builder b)
.build();
nettyServerSslContext = SslContextBuilder
- .forServer(b.certChain, b.privateKey, b.keyPassword)
+ .forServer(b.certChain, b.privateKey, b.privateKeyPassword)
Review Comment:
This change makes sense and I agree that this seems like the right thing to do for PEM-based authentication.
After this change, though, I don't see any additional usages of `b.keyPassword` in RPL SSL code.
That code was added in https://github.com/apache/spark/commit/cfea30037ff4ac7e386a1478e7dce07ca3bb9072.
There, it looks like it influences both Akka and Jetty.
In the Jetty code, which hasn't changed to this day, it does
```
keyStorePassword.foreach(sslContextFactory.setKeyStorePassword)
keyPassword.foreach(sslContextFactory.setKeyManagerPassword)
```
and flows to the "key manager" password.
Within Jetty, that password is read at once place:
```
protected KeyManager[] getKeyManagers(KeyStore keyStore) throws Exception
{
KeyManager[] managers = null;
if (keyStore != null)
{
KeyManagerFactory keyManagerFactory = getKeyManagerFactoryInstance();
keyManagerFactory.init(keyStore, _keyManagerPassword == null ? (_keyStorePassword == null ? null : _keyStorePassword.toString().toCharArray()) : _keyManagerPassword.toString().toCharArray());
managers = keyManagerFactory.getKeyManagers();
```
So based this Jetty code, it looks like the right place to thread this password is to use it when initializing the KeyManagerFactory.
Given that, do you think we need to have similar logic in our own code at
```
private static KeyManager[] keyManagers(File keyStore, String keyStorePassword)
throws NoSuchAlgorithmException, CertificateException,
KeyStoreException, IOException, UnrecoverableKeyException {
KeyManagerFactory factory = KeyManagerFactory.getInstance(
KeyManagerFactory.getDefaultAlgorithm());
char[] passwordCharacters = keyStorePassword != null? keyStorePassword.toCharArray() : null;
factory.init(loadKeyStore(keyStore, passwordCharacters), passwordCharacters);
return factory.getKeyManagers();
}
```
to use the key password if provided and fall back on the key store password if empty?
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43998:
URL: https://github.com/apache/spark/pull/43998#issuecomment-1843956053
@mridulm CI is now green
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "JoshRosen (via GitHub)" <gi...@apache.org>.
JoshRosen commented on code in PR #43998:
URL: https://github.com/apache/spark/pull/43998#discussion_r1406993411
##########
common/network-common/src/main/java/org/apache/spark/network/ssl/SSLFactory.java:
##########
@@ -106,7 +106,7 @@ private void initNettySslContexts(final Builder b)
.build();
nettyServerSslContext = SslContextBuilder
- .forServer(b.certChain, b.privateKey, b.keyPassword)
+ .forServer(b.certChain, b.privateKey, b.privateKeyPassword)
Review Comment:
This change makes sense and I agree that this seems like the right thing to do for PEM-based authentication.
After this change, though, I don't see any additional usages of `b.keyPassword`. Even in the Web UI SSL code, I haven't managed to find any places where this key-specific `keyPassword` is actually used?
That code was added in https://github.com/apache/spark/commit/cfea30037ff4ac7e386a1478e7dce07ca3bb9072.
There, it looks like it influences both Akka and Jetty.
In the Jetty code, which hasn't changed to this day, it does
```
keyStorePassword.foreach(sslContextFactory.setKeyStorePassword)
keyPassword.foreach(sslContextFactory.setKeyManagerPassword)
```
and flows to the "key manager" password.
Within Jetty, that password is read at once place:
```
protected KeyManager[] getKeyManagers(KeyStore keyStore) throws Exception
{
KeyManager[] managers = null;
if (keyStore != null)
{
KeyManagerFactory keyManagerFactory = getKeyManagerFactoryInstance();
keyManagerFactory.init(keyStore, _keyManagerPassword == null ? (_keyStorePassword == null ? null : _keyStorePassword.toString().toCharArray()) : _keyManagerPassword.toString().toCharArray());
managers = keyManagerFactory.getKeyManagers();
```
So based this Jetty code, it looks like the right place to thread this password is to use it when initializing the KeyManagerFactory.
Given that, do you think we need to have similar logic in our own code at
```
private static KeyManager[] keyManagers(File keyStore, String keyStorePassword)
throws NoSuchAlgorithmException, CertificateException,
KeyStoreException, IOException, UnrecoverableKeyException {
KeyManagerFactory factory = KeyManagerFactory.getInstance(
KeyManagerFactory.getDefaultAlgorithm());
char[] passwordCharacters = keyStorePassword != null? keyStorePassword.toCharArray() : null;
factory.init(loadKeyStore(keyStore, passwordCharacters), passwordCharacters);
return factory.getKeyManagers();
}
```
to use the key password if provided and fall back on the key store password if empty?
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43998:
URL: https://github.com/apache/spark/pull/43998#discussion_r1408168103
##########
common/network-common/src/main/java/org/apache/spark/network/ssl/SSLFactory.java:
##########
@@ -215,16 +216,27 @@ public Builder privateKey(File privateKey) {
}
/**
- * Sets the Key password
+ * Sets the key password
*
- * @param keyPassword The password for the private key
+ * @param keyPassword The password for the private key in the key store
* @return The builder object
*/
public Builder keyPassword(String keyPassword) {
this.keyPassword = keyPassword;
return this;
}
+ /**
+ * Sets the private key password
+ *
+ * @param keyPassword The password for the private key
Review Comment:
this should be privateKeyPassword, will fix
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43998:
URL: https://github.com/apache/spark/pull/43998#issuecomment-1843412884
@mridulm the failure was a docs failure that was unrelated to this PR but it's been long enough that a rebase is warranted. I'll do that and re-request review once CI is green again.
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43998:
URL: https://github.com/apache/spark/pull/43998#issuecomment-1844054833
Merged to master.
Thanks for adding this @hasnain-db !
Thanks for the review @JoshRosen :-)
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43998:
URL: https://github.com/apache/spark/pull/43998#discussion_r1407053124
##########
common/network-common/src/main/java/org/apache/spark/network/ssl/SSLFactory.java:
##########
@@ -106,7 +106,7 @@ private void initNettySslContexts(final Builder b)
.build();
nettyServerSslContext = SslContextBuilder
- .forServer(b.certChain, b.privateKey, b.keyPassword)
+ .forServer(b.certChain, b.privateKey, b.privateKeyPassword)
Review Comment:
> Within Jetty, that password is read at once place:
Thanks!! This is what I had missed earlier and I wasn't sure how to pass the password.
To keep this change focused, and unblocked, though (I'll need to do more work to test this) I've filed https://issues.apache.org/jira/browse/SPARK-46132 - hope it's fine for me to put up a separate PR with just that change (+ additional tests)
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43998:
URL: https://github.com/apache/spark/pull/43998#issuecomment-1841908399
Can you check on the build failure @hasnain-db ? If it unrelated to the PR, it might be something which is fixed in master (in which case, a rebase should help).
--
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-46058][CORE] Add separate flag for privateKeyPassword [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #43998: [SPARK-46058][CORE] Add separate flag for privateKeyPassword
URL: https://github.com/apache/spark/pull/43998
--
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