Limit new UnsupportedOperationException to direct base class SslContextFactory usage

[joakime]

This change seems to have broken creation of a HTTPS connector for those using SslContextFactory directly instead of its Server subclass, like https://github.com/dropwizard/dropwizard/blob/release/1.3.x/dropwizard-jetty/src/main/java/io/dropwizard/jetty/HttpsConnectorFactory.java#L599

For a codepath that is merely "deprecated", this seems unexpected.

Originally posted by @bentmann in #4326

[joakime]

Opened new PR #4386

[sbordet]

The change is on purpose throwing to get users to update to use SslContextFactory.Server.

Having a keystore that needs SNI functionalities needs proper wrapping of the KeyManagers, so we deemed better to throw at startup than having a non-working SNI only discovered when a request hits the server.

We had to err on the safer side on this one, and yes there were some behavior incompatibilities, but the change to make is trivial (just use SslContextFactory.Server).

[joakime]

As pointed out at #4326 (comment)

@bentmann looking at the code you linked, https://github.com/dropwizard/dropwizard/blob/release/1.3.x/dropwizard-jetty/src/main/java/io/dropwizard/jetty/HttpsConnectorFactory.java#L599

That would not work for Server side SNI based certificates / keystore as it sits right now.
Jetty version 9.4.16.v20190411 is the one that introduced the split for Server vs Client (which fixed client configs impacting server, and server configs impacting client, among which was SNI behaviors and EndpointIdentificationAlgorithm behaviors)

[bentmann]

but the change to make is trivial (just use SslContextFactory.Server).

Yes, "trivial" if one's app makes direct use of Jetty, not so much if one relies on external libraries for that and now has to juggle incompatibilities among patch versions.

[sbordet]

@bentmann If you don't use Jetty directly, don't you just need to upgrade your external library (which would have upgraded to a correct use of Jetty)?

[bentmann]

External libraries (like Dropwizard in our particular case) don't always consume newer versions of their dependencies in a timely manner. E.g. latest stable release of DW is 1.3.17 which natively depends on Jetty 9.4.18 (cf. https://github.com/dropwizard/dropwizard/blob/v1.3.17/dropwizard-bom/pom.xml#L25).

[sbordet]

I'd rather help DW to update their code to working SNI with SslContextFactory.Server on their 1.3.18 release, rather than code monkeying Jetty 9.4.25 so that DW 1.3.18 with Jetty 9.4.25 will have broken SNI, only to avoid exceptions at startup.

@bentmann did you raise a DW issue yet that we can link?

[bentmann]

@bentmann did you raise a DW issue yet that we can link?

No, not yet.

[bentmann]

@bentmann did you raise a DW issue yet that we can link?

dropwizard/dropwizard#3050

[sbordet]

@gregw I don't think #4386 should be merged.
IIUC, all those using SslContextFactory and not SslContextFactory.Server have now SNI broken, but they can only figure it out on the first request, rather than at startup.

[gregw]

The warning is issued at start up not at first request? What is the proposed alternative?

[sbordet]

@gregw with the changes introduced in #3863 we introduced a regression so that if a user used the base class SslContextFactory instead of SslContextFactory.Server, it would get a NPE during the TLS handshake.

#4325 aimed to fix that by throwing an exception earlier (at startup), rather than later (at runtime). I think this is the correct way to go.

The changes in this issue avoid the early error (we know how much users care about warnings at startup - not!) and disable completely SNI processing when using the base class, leading to hard to debug issues for users that rely on SNI (that is now not working at all).

The exception at startup has triggered an issue in libraries that were still using the base class, causing them to update to the new Server subclass (e.g. DropWizard) for a better solution in the long run.

I think we should revert the changes introduced in this issue and keep the "throw at startup" behavior.

[gregw]

@sbordet, I can't see how a NPE on startup is better than a clear warning on startup?
How about we keep #4386 but instead of warning throw an exception with the same text?

[sbordet]

@gregw it's not an NPE at startup, it's a NPE at runtime.

[gregw]

Somebody is not describing something properly??? This PR changes getKeyManagers which is called during startup and now produces a warning at startup if the wrong type of SslContextFactory is used. How can this affect the first request? The unit tests in this PR don't even make requests but check the behaviour of start()! So again - would it be OK if the warning was changed to a fatal error? If not then I just don't understand the problem you are describing?

[ttaranov]

solr is having startup issues due to this change - https://issues.apache.org/jira/browse/SOLR-14105 - since multi domain certs are no longer supported by jetty it seems.

[sbordet]

@ttaranov multi domain certs are supported.

Can you please open a new issue? Describe whether this is a client or server problem in Solr. Thanks!

[ttaranov]

@sbordet well, it seems to be a strange issue where Solr calls itself internally on the server side at startup - https://github.com/apache/lucene-solr/blob/master/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java#L316 - and uses jetty's SslContextFactory.Client to setup ssl connection to itself, which seems like no longer allows using multi domain certs that actual server is setup with.

[sbordet]

@ttaranov please open a new issue - I'll tackle it tomorrow.

[ttaranov]

@sbordet #4874