SslContextFactory certHosts map results in non-deterministic certificate selection

[mpkusnierz]

The _certHosts map keyed on hostname is populated in the load() method where it will put ALL certificates/aliases into this map based on the host(s) listed for each certificate; this will overwrite existing certificates in the _certHosts map that return the same host.
A keystore may contain multiple certificates for the same host; and this process of populating a _certHosts map and overriding previous entries makes this process unpredictable. The order of the certificates returned by the keystore doesn't appear to always be consistent.

This load step does NOT check the _certAlias attribute to see if an explicit alias has been configured; so there appears to be no way to control which certificate is actually used in the scenario that multiple certificates for the same host are present in the keystore.

The scenario I am facing has a self-signed (key-pair) certificate in the keystore alongside a properly signed certificate (with the associated chain to the root CA cert). Both certificates are for the same host. They have different aliases in the keystore and I am explicitly setting the certAlias attribute to specify the correct (fully signed+chain) cert from the keystore; but on some machines the correctly signed cert is selected, on other machines, the self-signed certificate is selected (which is obviously rejected by the browser as invalid).

I am using the 9.4.50.v20221201 version of jetty-util, but from what I can see the same issue would still occur in the v10 series.

Possibly related to this other issue: #6929, and at least overlap in the same problem space as this issue: #6034
Issue also raised as a StackOverflow question, although via spark-java which uses jetty under the covers:
https://stackoverflow.com/questions/75449183/spark-java-certificate-alias-not-selecting-correct-entry-from-keystore

[mpkusnierz]

from the load() method, line 362 (using line numbers fomr v9.4.50.v20221201); existing line:

_certHosts.put(h, x509);

I would suggest changing this to:

_certHosts.compute(h, (k, o) -> {
                                		if (o == null) {
                                			return x509;
                                		}
                            			X509 x = _certAlias != null && _certAlias.equalsIgnoreCase(o.getAlias()) ? o : x509;
                                		LOG.info("multiple certificates found for the same host {} - aliases: {}, and {} - adding {} to certHosts mapping", h, o.getAlias(), x509.getAlias(), x.getAlias());
                                		//Don't replace existing cert if that cert matches an explicitly specified alias 
										return x;
                                	});

[mpkusnierz]

There is a secondary issue; this org.eclipse.jetty.util.ssl.SslContextFactory$Server#sniSelect() method does not take into account an explicitly configured alias.

line 2426-2430 (using line numbers from v9.4.50.v20221201) selects an alias from a list of matching server aliases; but it just selects the first matching alias with the only preference based on exact hostname matches vs wildcard hostnames...
But if there are multiple exact-hostname matches, this results in unpredictable certificate selection.

The existing logic looks like this:

                        // Prefer strict matches over wildcard matches.
                        alias = matching.stream()
                            .min(Comparator.comparingInt(cert -> cert.getWilds().size()))
                            .map(X509::getAlias)
                            .orElse(alias);

I would suggest changing this to:

                    	//Use explicitly configured alias
                    	if (!alias.equalsIgnoreCase(super.getCertAlias())) {
                    		Optional<String> oa = super.getCertAlias() != null 
                    				? matching.stream().map(X509::getAlias).filter(a -> a.equalsIgnoreCase(super.getCertAlias())).findFirst()
            						: Optional.empty();
                    		alias = oa.orElse(
                    				// Prefer strict matches over wildcard matches.
                    				matching.stream()
			                            .min(Comparator.comparingInt(cert -> cert.getWilds().size()))
			                            .map(X509::getAlias)
			                            .orElse(alias));
                    	}

[sbordet]

You said you have a self-signed certificate and a signed certificate for the same host and said that you specify certAlias to the signed certificate.
In this way, the self-signed is never going to be used, so why keep it in the KeyStore at all, obviously causing confusion?

I am failing to see a use case where you want to have 2 different certificates for the same host?
Perhaps just install a newer certificate when the previous one is expiring?
But even in this case, why keep the old one?

[mpkusnierz]

We have an automated certificate renewal process, all the stages from self-signed through to fully signed cert are retained along the way in order to be able to a) restart the process part way along in the event of an error/interruption, and b) for debugging the process. I agree it might be a rare corner case, and probably a clean keystore with a single cert should be generated even if we do want to keep a second keystore with both certs. However, the cert alias can be specified, but is not always correctly selected, so I still think this is a bug.

…

On Sun, 30 Jul 2023, 10:29 Simone Bordet, ***@***.***> wrote: You said you have a self-signed certificate and a signed certificate for the same host and said that you specify certAlias to the signed certificate. In this way, the self-signed is never going to be used, so why keep it in the KeyStore at all, obviously causing confusion? I am failing to see a use case where you want to have 2 different certificates for the same host? Perhaps just install a newer certificate when the previous one is expiring? But even in this case, why keep the old one? — Reply to this email directly, view it on GitHub <#9435 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZHPAUW4ZOR2Y7TDZIJF3XSYSPBANCNFSM6AAAAAAVGYMT64> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[github-actions]

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.