Few problems with SslContextFactory behavior

[nigredo-tori]

These are some of the issues I've encountered while digging into the SslContextFactory implementation. As an example I use CryptoPro JCP - one of the few crypto providers certified in Russia.

1. Key manager password falls back to key store password: https://github.com/eclipse/jetty.project/blob/820ccd7bd16c44549169dfad3a48b5a1a351985c/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#L1152 This means we can't init the key manager factory with null password if the key store password is not null. CryptoPro JCP accepts different passwords for a key store and a key manager, and allows the key manager password to be null.

2. Кey managers are only filtered by alias/SNI if they implement X509ExtendedKeyManager: https://github.com/eclipse/jetty.project/blob/820ccd7bd16c44549169dfad3a48b5a1a351985c/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#L1158-L1165 This is a bit silly:

   • For the alias filter we only need the stronger X509ExtendedKeyManager interface if the provided alias is null - which never happens in existing Jetty code! https://github.com/eclipse/jetty.project/blob/820ccd7bd16c44549169dfad3a48b5a1a351985c/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/AliasedX509ExtendedKeyManager.java#L53-L54
   • For the SNI filter we only use X509ExtendedKeyManager as the final fallback , so it shouldn't be mandatory. https://github.com/eclipse/jetty.project/blob/820ccd7bd16c44549169dfad3a48b5a1a351985c/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SniX509ExtendedKeyManager.java#L115-L116

   CryptoPro JCP doesn't provide extended managers - meaning I can't properly filter them by alias.

3. Key store is always loaded using .load(InputStream, char[]) method. CryptoPro JCP also supports a "default" store (.load(null)) - but I can't use it since OpenSslFactory and CertificateStreamUtils don't support that scenario

[gregw]

@nigredo-tori thanks for this feedback. We might take a bit of time to get around to fixing this and checking it works with CryptoPro. So if you wanted to prepare a pull request with some suggested changes to SslContextFactory, then that would move this along faster as we would review your changes!

[nigredo-tori]

I'll try and make a PR once I have some breathing space at work. I'm not sure when that'll be, though.

Looking back at it, the third problem is absolutely avoidable. I've somehow missed the most glaringly obvious solution - setKeyStore ^_^. Still, it's a bit confusing that SslContextFactory includes custom KeyStore initialization logic. It seems that it should be enough to pass KeyStore.Builder instances for key and trust stores, and move all the song and dance with KeyStore initialization, etc. into a custom KeyStore.Builder subclass. Would you be interested in a PR with this against jetty-10.0.x branch?

[gregw]

Any more radical API changes would be best against the 10.0.x branch, as we try to keep the API stable in 9.4.x (which is kind of responsible for a lot of the strangeness in this 15 year old class).

[nigredo-tori]

Regarding filtering aliases - I was wrong. JCP does indeed produce X509ExtendedKeyManagers. The reason I failed to properly filter these by alias is because of two wrappers interfering with each other:

1. If we set a non-null cert alias, each X509ExtendedKeyManager is wrapped in an AliasedX509ExtendedKeyManager that makes sure chooseEngineServerAlias etc. yield an alias we want, or nothing at all. This is the functionality I needed. However, this doesn't filter getServerAliases result.
2. If there are few SNI-eligible certificates or hosts, that is further wrapped in an SniX509ExtendedKeyManager. This also has its own chooseEngineServerAlias logic, and only delegates to the AliasedX509ExtendedKeyManager.chooseEngineServerAlias if it fails to match any certificate. So even with a set cert alias we can get another alias!

Seems like the the only way to get the behavior I want would be to override/reimplement SslContextFactory.getKeyManagers.

[stale[bot]]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been closed due to it having no activity.