Fix AliasX509ExtendedKeyManager to process both server and client aliases properly

[ecnabogs]

SpringBoot provides a default implementation of the org.springframework.boot.ssl.SslManagerBundle interface (DefaultSslManagerBundle) that allows to specify a SslBundleKey (referencing the private key within the associated KeyStore) to be used for establishing an SSL connection. This feature, added in SpringBoot 3.1.0, is really appreciable as it simplifies a lot the selection of the private key to be used during SSL handshake.
However, the thing is that the underlying implementation (AliasX509ExtendedKeyManager) only considers the server-side of an SSL connection. If a client has to communicate with multiple partners through mTLS protocol and has a single KeyStore containing its private keys, it cannot select the appropriate key alias to be used with the related partner.

My proposal is to support this use case and to make AliasX509ExtendedKeyManager working similarly for both server and client sides. That's the first point.

Then, looking more in details at the implementation, I noticed that the selected alias was returned unconditionally at the server side. I think it is not correct as SSL handshake is expected to reach an agreement between client and server, especially regarding the key algorithm to be used. Here, this algorithm is not taken into account meaning that although the key with the specified alias could exist in the underlying KeyStore, it could also be of the wrong type.
Thus, my implementation gets inspired by the JDK default implementation (sun.security.ssl.SunX509KeyManagerImpl) in which the alias selection is made by calling the getServerAliases(keyType, ...) or getClientAliases(keyType, ...) for restricting the choice to compatible keys only.

Finally, the AliasX509ExtendedKeyManager current implementation also only considers the transport-independent SSLEngine but not the Socket-specific one. To cover all use cases, the implementation should also apply the same alias selection logic to the Socket-specific methods. That's what I did actually and factorized this logic for all contexts.

[ecnabogs]

Hi,
Would it be possible to review this PR please ?
I published it a week ago and it seems that nobody went through it. There is no posts here while all other PRs, even the latest ones, are commented. The approving workflow has not even been approved. Did you or I miss something ?
Thanks

[wilkinsona]

Did you or I miss something ?

I don't think anybody has missed anything. We're just a small team with plenty to do. Some PRs are easier to digest than others which can lead to them being processed more quickly as they're easier to fit in when only a small chunk of time is available.

The approving workflow has not even been approved

I've approved the workflow. Thanks for the nudge on that.

We'll review this in more detail as soon as we can. Thanks for your patience in the meantime.

[ecnabogs]

No problem Andy. I understand you are overwhelmed with lots of other stuff. I only wanted to be sure that my PR would be considered.
Thanks and have a good day.

[ecnabogs]

I have just rebased my branch on the main with last changes. Waiting for the workflow to be re-rerun.

[philwebb]

@ecnabogs Thanks, workflows should run automatically now the first one has been approved.

[wilkinsona]

I'm not sure that this is the behavior that we want. If the user's configured a particular alias, I would not expect another alias to be used. The same applies to the other …WhenTestAliasIsUnknown tests below but I may well be missing something and we don't need to have the same discussion multiple times.

[ecnabogs]

Since all chooseXXXAlias(...) methods in the current Spring Boot implementation, except chooseEngineServerAlias(...) one, are delegating their behavior to the internal delegate factory, I was assuming that it was a fallback choice. If we consider that we should return our configured alias (if available and compatible) or nothing, I can change the implementation and the tests. The JDK default implementation returns the first alias whose associated key's algorithm and issuer match the ones requested. Then, the key point now is to choose between the strategy selecting an exclusive configured key alias or the one selecting a preferred one and falling back to another one if unavailable. I have no preference regarding this choice. Maybe it could also be parameterized, couldn't it ?

[wilkinsona]

Thus far, we'd only really considered the server and SSLEngine side of things and it, as you noted above, always returns the user's configured alias.

Then, the key point now is to choose between the strategy selecting an exclusive configured key alias or the one selecting a preferred one and falling back to another one if unavailable

My preference is for the former. I think it could be considered a regression if we changed the behaviour to return an alias that isn't the user-configured alias.

I have no preference regarding this choice. Maybe it could also be parameterized, couldn't it ?

If there's demand for it, that's certainly something that we could consider. I could also imagine some users may want to be able to configure multiple aliases which can then be part of the negotiation to determine the preferred alias. I'd wait for some demand before adding that complexity too, though.

[ecnabogs]

You are right, I had not considered regression issues. I agree not to return delegate choice in this case. However, in the current implementation, key's algorithm and issuer are not taken into account while selecting the alias. If we keep returning the configured alias unconditionally, it might also be considered as a bug. If the configured key's algorithm and issuer do not match the passed parameters, we should return null, right ?

[wilkinsona]

If the configured key's algorithm and issuer do not match the passed parameters, we should return null, right?

Yes, I think so. It's returned too broadly now and it should be fine to narrow things down. AIUI, with the current implementation we may return an alias that can't actually be used which violates the contract of the choose… methods.

[ecnabogs]

Ok, fine. I will update the code and the tests accordingly.

[ecnabogs]

Fix done. Awaiting for approval to run the worklow.