[SPARK-46058][CORE] Add separate flag for privateKeyPassword

[hasnain-db]

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

[hasnain-db]

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)

[JoshRosen]

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 cfea300.

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?

[hasnain-db]

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)

[JoshRosen]

This makes sense overall, and from a compatibility perspective I think this is fine since this new feature has yet to ship in a Apache Spark release.

I left two comments, one about a doc typo and another about a potential bug related to keyPassword possibly being ignored in JKS SSL.

[hasnain-db]

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.

[hasnain-db]

CI failure is in generating docs but that looks unrelated to this PR

[JoshRosen]

LGTM.

I agree that the documentation build failure appears to be unrelated: it looks like we also hit this in another PR at #43978 (comment)

/cc @mridulm, in case you want to give this a review as well.

[hasnain-db]

cc @mridulm in case you want to take a look.

[mridulm]

Missed out on this, looks good to me.

[mridulm]

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).

[hasnain-db]

@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.

[hasnain-db]

@mridulm CI is now green

[mridulm]

Merged to master.
Thanks for adding this @hasnain-db !
Thanks for the review @JoshRosen :-)