fix: don't delegate client alias choosing for ssl bundles

[MezinK]

Currently, the .key.alias specified inside of an SSL Bundle is not taken into account when dealing with client certificates.
This causes the delegate (usually SunX509KeyManagerImpl) to just pick the first alias it finds.
For requests requiring mTLS, this can cause issues if the certificates are inside of a shared keystore, as you may not get the certificate that you would want.

I have created an example repo demonstrating this behavior:
https://github.com/MezinK/spring-boot-mtls-demo

[wilkinsona]

We should consider the discussion in #44629 when reviewing this proposal.

[wilkinsona]

This causes the delegate (usually SunX509KeyManagerImpl) to just pick the first alias it finds.

I don't think it picks the first alias. AIUI, it picks the first matching alias.

Regardless, when looking at #44629, I had this concern:

Previously, the alias only affected the server side. Applying it to the client side as well could be a breaking change, particularly if someone's sharing an SSL bundle between client and server components and they're used to the alias only affecting the server side.

I still have that concern and the change proposed here does not address it. If we want to provide some control over the alias that's used on the client-side rather than delegating to the matching performed by SunX509KeyManagerImpl, I think we'll need to make it opt-in some how.

[MezinK]

I still have that concern and the change proposed here does not address it. If we want to provide some control over the alias that's used on the client-side rather than delegating to the matching performed by SunX509KeyManagerImpl, I think we'll need to make it opt-in some how.

Perhaps, this can be added under SslOptions?
As per documentation: "Configuration options that should be applied when establishing an SSL connection."

What do you think?

[wilkinsona]

That would allow users to opt in but I wonder if some may need separate aliases for the client side and for the server side. I'm not sure how likely it is that someone would have an SSL setup that requires that configurability but perhaps we should deprecate alias and introduce serverAlias and clientAlias? The new serverAlias would behave as alias does today and clientAlias would be returned from the chooseEngineClientAlias and chooseClientAlias methods. I'll discuss it with the rest of the team.

[MezinK]

I've debugged the flow of the SunX509KeyManagerImpl and understood the following:

1. Check if keyTypes were specified
2. For every keyType specified, check if the underlying certificates public key uses that keyType
3. If true -> put the alias of that certificate in a list, if none found -> skip and continue with the next keyType
4. If alias(es) have been found, the first alias in the array is returned.

See this screenshot:

Here, we are in the CertificateATest.java class, but you can see that aliases[0] is equal to "certificate-b", therefore the test has the unexpected results.

[wilkinsona]

We discussed this today. We'd like to deprecate alias and replace it with server-alias and introduce a new client-alias property. @MezinK, would you like to update your PR to that effect?

[MezinK]

Sure, I can do that

[MezinK]

@wilkinsona I have yet to add tests but I caught a case where I got a bit confused. I marked it with "TODO" in the code.
https://github.com/MezinK/spring-boot/blob/29dbc32194e56575b53990b412e335d31bc43e70/core/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/ssl/PropertiesSslBundle.java#L104
What do we do here? We have the option of clientAlias and serverAlias, but what if we have both AND they're different?

because this would currently be valid:

spring:
  ssl:
    bundle:
      jks:
        certificate:
          key:
            client-alias: "client-alias" # what happens here? there may only be one certificate at a time if the aliases are different
            server-alias: "server-alias"
          keystore:
            location: classpath:ssl/shared.p12
            password: ...

I assume, we want to load either a client alias or a server alias, but never both (unless they have the same value)
do we throw an error in this case telling the user they may not have different client and server aliases inside one ssl-bundle?

if this is the case, does it really make sense to branch it out into client-alias and server-alias?
because either way, it's only one certificate that is being loaded in one ssl bundle, which is going to be used for the server or the client, depending on what the user uses it in their code.

[wilkinsona]

When both are configured, my expectation is that we'd pass both into AliasKeyManagerFactory and down into AliasX509ExtendedKeyManager and then the choose(Engine)ClientAlias methods would return the value of client-alias and the choose(Engine)ServerAlias methods would return the value of server-alias.