ALPN API should allow throwing IOException or SSLException

[ejona86]

On its foundation, the problem is simple: in the event that protocol selection fails, an ALPN.Provider wants to throw an exception. An SSLException is attractive as existing error handling can appropriately deal with the exception.

Netty

Netty is currently throwing SSLHandshakeException if protocol selection fails within ServerProvider.select() or ClientProvider.selected(). I initially thought that Netty was doing something unreasonable, as it uses Generic tricks/Unsafe to throw the checked exception from select().

However, due to the Sun SSL Handler all exceptions thrown are wrapped, but a select few (including SSLHandshakeException) maintain their type.

Jetty

Jetty has a filter that only exposes "system classes" from the bootstrap class loader. Although it is supposed to be possible to configure what classes are "system classes," we had considerable trouble getting it to work. Even if we figured out a recipe to get Jetty working, Jetty is not the only container performing such tricks. Most importantly, the failure mode is that the ALPN-using application code loads a different ALPN class than the one that exists in the bootstrap class loader. This means that registering the ALPN provider appears to work and we can verify that the ALPNExtension is loaded, but nothing functions because ALPN is no longer a true singleton.

A much more reliable method for users that Just Works™ (although uglier for library developers) is to use reflection to get the ALPN Class from the bootstrap class loader (by passing "null" as the ClassLoader). This will succeed if and only if ALPN boot is in the bootclasspath. Since ALPN.ServerProvider and similar may be hidden just like ALPN, you have to use java.lang.reflect.Proxy to implement them.

Netty + Jetty

Netty's current hack works today, but it is unable to be used with Proxy, and thus is harder to run within Jetty and other containers. Proxy notices that a checked exception was thrown from a method not throwing checked exceptions and wraps the SSLHandshakeException in UndeclaredThrowableException. The UndeclaredThrowableException is then subsequently wrapped in a RuntimeException by the SSL code.

I have investigated fixing Netty to no longer throw a checked exception and no longer throw an exception at all, but these did not prove very feasible. I have investigated further hacks on top of the current set, and although there are some options, they are highly unpalatable.

I've already confirmed that ALPN boot would need no changes to support checked exceptions from select()/selected() Provider methods. Given the other constraints involved it seems the best approach is to change the ALPN API to allow throwing SSLException.

[Scottmitch]

+1 for checked/explicit exceptions. IIRC there were some reservations about this when the API was updated to support failures however this seems like a legitimate motivator.

@sbordet @joakime WDYT?

[sbordet]

I do not understand what the problem is ?

It's not clear what is the "existing error handling" you refer to is, what features it has ?

The classloading issues are only due to mistakes in adding the libraries to the application classpath. They should not be added to the application classpath, and at that point you don't need any "Just Works™" solutions.

[ejona86]

That would still require configuring Jetty (or any other container) to expose the ALPN classes. @lesv

The "existing error handling" wants to abort the connection if ALPN protocol negotiation fails. Per the ALPN spec, the server should send no_application_protocol alert. With alpn-boot this is done by throwing an exception. Although that works well for informing the remote side about what sort of failure occurred, it isn't very helpful for the local side as the exception will be wrapped by SSL in an unhelpful way for any RuntimeException. Using an SSLException is able to communicate more and re-use normal SSL error handling.

[sbordet]

I don't understand what you mean by "expose the ALPN classes". We do ALPN in Jetty and we don't expose them, and we don't need any "Just Works™" solutions.
I still don't understand what the problem is.

As for existing error handling on the local side, can you show a snippet of real code of how you handle an SSLException that wraps a RuntimeException differently from a SSLHandshakeException, and what benefits you have in handling them differently ?

Also, how do you know that SSLHandshakeException is treated differently ? How can we be sure that this implementation detail you seem to rely on won't be changed in the future ?

[ejona86]

Running with Jetty as the server and ALPN server support works. But not if you are trying to use ALPN from within the container, for example, as a client.

The wrapped exception will be a RuntimeException if wrapping a RuntimeException, a SSLHandshakeException if wrapping a SSLHandshakeException, and a SSLException if wrapping an SSLException. It is common when calling SSL code to only catch SSLExceptions, since that's what the checked exception is for.

I expect SSLHandshakeException is being treated the same as SSLException today. However, SSLHandshakeException would be propagated if the code needed to distinguish handshake failures which signal incompatibility. The distinction between SSLHandshakeException and SSLException is more minor than the distinction between SSLException and RuntimeException.

[sbordet]

Why won't ALPN work with as a client from within the container ?
What issues do you have ?

[ejona86]

Jetty has a white list of classes that propagate to applications. Note that org.eclipse.jetty.alpn is not in the list. Although the list can be modified, we had trouble getting it to work, and it adds yet another thing necessary for each application using a library that needs ALPN.

[sbordet]

Please log an issue about the ALPN classes to be whitelisted.

[joakime]

@ejona86 the wiki.eclipse.org is for Older Jetty 7 and Jetty 8 documentation.

Current documentation can be found at the documentation hub: https://www.eclipse.org/jetty/documentation/

As for that whitelist, the code is the canonical source.
And even that list isn't hardcoded and/or static.

Libraries, and even our own modules, add/remove/replace those lists on a regular basis.

[lesv]

I think what Eric is asking for is an explicit or implied:
addSystemClass("org.eclipse.jetty.alpn.");

When ALPN is installed. This is so Netty can be used easily, which is required for gRPC to be used.

[ejona86]

@lesv, what are your opinions on revisiting the ALPN visibility issue in Jetty via configuration (maybe by including a WEB-INF/jetty-env.xml in on of our JARs/WARs).

The doc is for the older version, but the newer version is basically unchanged and says:

This page contains content that we have migrated from Jetty 7 or Jetty 8 documentation into the correct format, but we have not yet audited it for technical accuracy in Jetty 9

I originally found the white list via the code and then back-tracked to the documentation. The core point still stands: alpn is not in the list by default.

My initial assumption was that Jetty was unlikely to be the only project doing such things, and getting things working for [random framework/container] would be a constant pain point. After some searching projects that I know would have custom ClassLoaders, I found Tomcat, GlassFish, WebLogic, and WebSphere don't appear to have a whitelist. WildFly does though, and it is very easy to miss in the documentation. So it may still be a pain point, but maybe a lesser one than I imagined.

[lesv]

Actually - take another look - the Servlet specification requires it. I'm fairly certain Tomcat has it. Others may as well.

Given that ALPN is developed by the Jetty project - it would seem quite reasonable from them to include it in the whitelist.

[ejona86]

@lesv, you mean due to:

The container should not allow applications to override or access the container’s implementation classes.

That just means Tomcat would need to filter org.apache.catalina and similar (via a blacklist). It wouldn't necessitate a whitelist. It doesn't look like Tomcat would filter ALPN.

[ejona86]

I filed a Jetty bug. It still seems like throwing SSLExceptions is necessary to use alpn boot reasonably, because of the no_application_protocol alert handling. Without the API being fixed, the only reasonable solution is to abuse the language and throw a checked exception from a method not marked as throwing checked exceptions.

[sbordet]

@ejona86 sorry but I am really slow on this.
What has the bug you filed to do with the ALPN API declaring to throw SSLException or not ?
There are easy workarounds to the bug you filed.
I still don't understand what exact problem do you have ?

[ejona86]

My exact problem was the intersection of multiple problems and pre-existing workarounds preventing using ALPN in a client-provided library running in Jetty. I originally worried that it was not a Jetty-specific problem, but now believe it may only impact Jetty and WildFly.

The Jetty bug I filed would eventually take care of Jetty. In the mean time it seems we'll recommend our users to use a WEB-INF/jetty-env.xml. WildFly would require a jboss-deployment-structure.xml for now as well.

This entire exercise exposed that the ALPN API should allow throwing an SSLException, as there aren't really any alternatives when ALPN protocol negotiation fails such that you can trigger no_application_protocol and communicate the error locally. I've not found a bug tracker for alpn-api (e.g., no component in Eclipse), but it did seem that such a change would impact alpn_boot most directly.

[sbordet]

Released alpn-api-1.1.2.v20150522 with methods ServerProvider.select(List) and ClientProvider.selected(String) now throwing SSLException.

[Scottmitch]

@sbordet - Should we also expect a new version of alpn_boot as a result of this change? I noticed a few minor commits to https://github.com/jetty-project/jetty-alpn since this change.

[sbordet]

@Scottmitch There is no plan to update the alpn-boot artifact because of this change. My tests have shown that since checked exception enforcing is a javac thing only, applications will work fine.

If that is not the case, let's have a test case, discuss it and fix the issue.

The plan is to incorporate these changes in alpn-boot when the next OpenJDK 8 version will be out.

[nmittler]

FYI, it looks like we can configure a Jetty webapp to allow access to ALPN: grpc/grpc-java#180 (comment)