JSSEngine - Initial ALPN support #526

cipherboy · 2020-04-29T14:03:54Z

This pull requests introduces support for ALPN negotiation via a fixed list of next protocol identifiers.

Technically this support was introduced in the native SSLEngine in JDK 9, but support in Tomcat exists for older versions when advertised.

ALPN is a method of negotiating the next protocol after TLS. Most of the time this will be http/1.1, but could be h2 or spdy/2 depending on browser and server support. Without this, Tomcat is limited to http/1.1 and cannot speak e.g., http/2.

--

Also, having a ALPN-capable implementation means we can go implement TLS-ALPN-01 / RFC 8737 if we wanted to.

org/mozilla/jss/ssl/javax/JSSEngineReferenceImpl.java

cipherboy · 2020-05-01T15:59:41Z

@edewata Note that ALPN support also gives us the ability to do TLS-ALPN-01 challenges with ACME if we wanted to.

edewata

Please see my comments below.

org/mozilla/jss/ssl/javax/JSSEngine.java

org/mozilla/jss/ssl/javax/JSSParameters.java

org/mozilla/jss/util/JDKCompat.java

cipherboy · 2020-09-23T14:03:45Z

I've rebased this to drop the JDK compatibility interface. Hopefully a high enough version of the JDK has shipped everywhere we need it to for this feature.

cipherboy · 2020-09-23T14:08:04Z

@edewata Looks like Fedora 32 still doesn't have ALPN support in JDK8. Thoughts? We might need to re-introduce compatibility testing. This would also affect RHEL...

edewata

Not familiar with ALPN, but it looks fine to me, just some minor comments. Feel free to merge.

org/mozilla/jss/ssl/javax/JSSEngineReferenceImpl.java

edewata · 2020-09-23T15:51:30Z

org/mozilla/jss/ssl/javax/JSSEngine.java

+    public byte[] getALPNWireData() {
+        int length = 0;
+        for (String protocol : alpn_protocols) {
+            length += 1 + protocol.getBytes().length;


Do we need to use UTF-8 specifically? Otherwise getBytes() will use the system's default encoding. Or if the protocol names are ASCII anyway then it doesn't matter.

Officially an ALPN protocol ID is a sequence of bytes and they are registered with IANA as such; see https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids. Therefore I recommend the JSS interface deal with protocol IDs as byte[] the whole way through, and defer conversion to/from other types (such as String) to the library user. This is what I did in my other ALPN PR #640.

This isn't actually possible. This PR is against the JCA-standard javax.net.ssl implementation that I've added to JSS. It resides under the org.mozilla.jss.ssl.javax package namespace to avoid confusing it with the custom, legacy "SSLSocket" implementation under org.mozilla.jss.ssl.

In particular, looking at the interface we're given:

We get the output as a String: SSLSocket.getApplicationProtocol(), SSLEngine.getApplicationProtocol()

We get the "input"/protocol choices as a String: SSLEngine.setHandshakeApplicationProtocolSelector(...)

So we either choose our own encoding and force it throughout, or let the user control it via system default.

It looks like the JDK11 always forces UTF-8:
https://github.com/openjdk/jdk11u-dev/blob/master/src/java.base/share/classes/sun/security/ssl/AlpnExtension.java#L100

So perhaps this will be good enough for us.

It will be good enough for us. But it is seriously flawed w.r.t. how the ALPN extension and protocol ID is actually specified. Java's fault, we just have to put up with it.

I mean, I don't disagree:

byte[] a = new byte[]{ (byte) 0xFA, (byte) 0xFA }; String b = new String(a, StandardCharsets.UTF_8); byte[] c = b.getBytes(StandardCharsets.UTF_8); System.out.println(a.length + " " + a[0] + " " +a[1]); System.out.println(b); System.out.println(c.length + " " + c[0] + " " + c[1]);

Outputs:

2 -6 -6 �� 6 -17 -65 ...

But also.. If you and the server both negotiate GREASE'd values... What does that even mean? What protocol are you actually negotiating there? It really isn't well defined. So they just hope the rest of the (valid) IANA protocol values are otherwise valid.

(I say negotiate because NSS and the protocol doesn't give the client a way to see what the server has for ALPN data priorities, and for that matter, the String comparison all happens on the server-side anyways, so we should probably coerce byte -> String and use String.equals(...), rather than trying to go from String -> Byte and compare byte arrays... That ensure we probably have the same encoded representation rather than dealing with differences there...)

Well, there's nothing we can do about it but to descend slightly further into this rabbit hole:

You don't negotiate GREASE (servers MUST ignore GREASE values). But if you want to send GREASE in your ClientHello, 15 of the 16 defined values are impossible to send.

And if any other ALPN values get defined that are not the UTF-8 encoding of some string, then you can't send those either.

On your second comment, no, the string comparison is essential on the client side too: the client will usually need to check what protocol was negotiated.

As for how to compare, given the interface imposed on us I have no objection to String.equals().

I suppose... This is why we have JSSEngine and JSSParameters after all... We could add a custom getter/setter which returns the raw byte values... Thoughts?

@frasertweedale -- I've brought this up for discussion on the OpenJDK security-dev mailing list and CC'd you. That was the approach suggested by #java.

Yes, we could implement our own methods that deal with byte[], if the requirement arises.

I don't think the need is there right now (I do not need to send grease or handle a non-UTF-8 protocol ID).

I'm just annoyed at such a mistake in a foundational API. So easliy avoided, if they had just read the spec and implemented what is says.

org/mozilla/jss/ssl/javax/JSSParameters.java

org/mozilla/jss/tests/TestSSLEngine.java

edewata

I don't have any objection, as long as it passes CI feel free to merge it.
Is there a real application that can use this now, or do we need to
implement tls-alpn-01 ACME challenge in order to truly test it?

cipherboy · 2020-10-06T21:53:12Z

We have two options. I'm inclined to leave it open a little longer and implement server support. Or we could point a small client socket at something like google.com which supports HTTP/2 and see if it negotiates it correctly. :)

org/mozilla/jss/ssl/javax/JSSEngine.java

cipherboy · 2020-10-08T14:24:23Z

Overall I am at a bit of a loss about the JSSEngine class in general (which implements the abstract SSLEngine class). It is not at all clear to me how to use that class to establish a TLS-secured network connection. Surely we also want to enhance JSSSocket with ALPN capability?

This is done now. Technically this wasn't necessary if you had provided a JSSParameters to the JSSSocket. But easy enough to add the other methods. :-)

Thanks @cipherboy. It is important to have JSSSocket.getApplicationProtocol() so the client or server can find out what (if any) protocol was negotiated.

JSSSocket.getSession().getNextProtocol() works too :-)

And note that this is again, only client-side checking. The server does not use SSL_SetNextProtoNego() and instead only uses SSL_SetNextProtoCallback, which internally maintains its own list and/or is passed one on arg depending on how the user writes their callback handler. In a language more pythonic, the callback function signature is:

def SSLNextProtoCallback(caller_arg: Any, fd: NSPR.PRFileDesc<NSS.SSL>, peer_protos: List[str], max_ret_length: int) -> str:
    pass

and there are no other helper functions which return the server-side configured list of protocols. So there is no way for the server to take that into account, if set. Additionally, it is in the wrong format: we have to encode the list of protocols ourselves, so we'd have to re-parse that into a useful value in order to make sense of them in the callback handler. However, NSS already does this for us with the client's list of protocols.

Indeed, you can see that in the gtests here:

https://github.com/nss-dev/nss/blob/master/gtests/ssl_gtest/tls_connect.cc#L627

It does its own parsing of the server-provided "arg" value (which in turn is its own list of values) and uses that to determine which of the protos to return.

frasertweedale · 2020-10-09T07:17:24Z

And note that this is again, only client-side checking. The server does not use SSL_SetNextProtoNego() and instead only uses SSL_SetNextProtoCallback, which internally maintains its own list and/or is passed one on arg depending on how the user writes their callback handler. In a language more pythonic, the callback function signature is:
def SSLNextProtoCallback(caller_arg: Any, fd: NSPR.PRFileDesc<NSS.SSL>, peer_protos: List[str], max_ret_length: int) -> str:
    pass
and there are no other helper functions which return the server-side configured list of protocols. So there is no way for the server to take that into account, if set. Additionally, it is in the wrong format: we have to encode the list of protocols ourselves, so we'd have to re-parse that into a useful value in order to make sense of them in the callback handler. However, NSS already does this for us with the client's list of protocols.

Indeed, you can see that in the gtests here:

https://github.com/nss-dev/nss/blob/master/gtests/ssl_gtest/tls_connect.cc#L627

It does its own parsing of the server-provided "arg" value (which in turn is its own list of values) and uses that to determine which of the protos to return.

I admit I haven't tested it, but let me refer to the NSS code that leads me to the opposite conclusion:
https://github.com/nss-dev/nss/blob/master/lib/ssl/sslsock.c#L2224-L2254

As you can see, this sets the protocol list data ss->opt.nextProtoNego.data which a client will transmit to server. It also calls SSL_SetNextProtoCallback(fd, ssl_NextProtoNegoCallback, NULL).
The callback function ssl_NextProtoNegoCallback is a server-side callback that refers to the protocol list data just set (ss->opt.nextProtoNego.data).

In this way, SSL_SetNextProtoNego satisfies both the client side requirements, and the common case for the server side too.

cipherboy · 2020-10-09T13:36:25Z

Aha, that's interesting. Then there must be some other bug in this code, I'll have to take a look more closely.

I was looking here:

https://github.com/nss-dev/nss/blob/master/lib/ssl/ssl3exthandle.c#L325-L334
https://github.com/nss-dev/nss/blob/master/lib/ssl/ssl3exthandle.c#L388-L396

I missed that SSL_SetNextProtoNego had its own custom handler. I gotcha now.

org/mozilla/jss/ssl/javax/JSSParameters.java

frasertweedale · 2020-10-13T13:09:16Z

org/mozilla/jss/ssl/javax/JSSEngineReferenceImpl.java

@@ -347,8 +347,17 @@ private void createBufferFD() throws SSLException {
            throw new SSLException("Unable to enable SSL Handshake Callback on this SSLFDProxy instance.");
        }

-        // Pass this ssl_fd to the session object so that we can use
-        // SSL methods to invalidate the session.
+        if (alpn_protocols != null) {


@cipherboy I've tested this PR and it's almost there. We really do need this length check though.
Feel free to just apply this patch or modify the relevant commit: whichever you prefer:

commit 5e09241e923b426733c444ccb1ed9295726c3080 Author: Fraser Tweedale <[email protected]> Date: Tue Oct 13 12:45:46 2020 +1100 ALPN: guard against empty array of protocol IDs javax.net.ssl.SSLParameters sets default ALPN protocol IDs to empty array. When downcasting to JSSParameters, we take the value as-is, so we could also end up with an empty array. Therefore when deciding whether to create the ALPN extension the non-null check is not sufficient. Also check that the array is non-empty. diff --git a/org/mozilla/jss/ssl/javax/JSSEngineReferenceImpl.java b/org/mozilla/jss/ssl/javax/JSSEngineReferenceImpl.java index 93fe703c..f84ae800 100644 --- a/org/mozilla/jss/ssl/javax/JSSEngineReferenceImpl.java +++ b/org/mozilla/jss/ssl/javax/JSSEngineReferenceImpl.java @@ -347,7 +347,7 @@ public class JSSEngineReferenceImpl extends JSSEngine { throw new SSLException("Unable to enable SSL Handshake Callback on this SSLFDProxy instance."); } - if (alpn_protocols != null) { + if (alpn_protocols != null && alpn_protocols.length > 0) { byte[] wire_data = getALPNWireData(); if (wire_data == null) { throw new RuntimeException("JSSEngine.init(): ALPN wire data is NULL but alpn_protocols is non-NULL.");

cipherboy · 2020-10-14T22:14:22Z

@frasertweedale If you have cycles, do you mind looking at jdk8 failure? This started recently and didn't happen on the initial code, so I'm suspicious it was something simple.

If you patch the Dockerfile:

diff --git a/tools/Dockerfiles/ubuntu_jdk8 b/tools/Dockerfiles/ubuntu_jdk8
index 48cc3720..64a561fe 100644
--- a/tools/Dockerfiles/ubuntu_jdk8
+++ b/tools/Dockerfiles/ubuntu_jdk8
@@ -24,11 +24,12 @@ COPY . /home/sandbox/jss
 
 # Perform the actual build
 WORKDIR /home/sandbox/jss
-CMD true \
+RUN true \
         && rm -rf build \
         && mkdir build \
         && cd build \
         && CFLAGS="-Wall -Wextra -Werror" cmake .. \
         && make all \
-        && ctest --output-on-failure \
         && true
+
+CMD /bin/bash

It will let you run but I don't know how to enable gdb inside a container. Perhaps I just need to build and run it as root.

frasertweedale · 2020-10-15T05:23:49Z

@frasertweedale If you have cycles, do you mind looking at jdk8 failure? This started recently and didn't happen on the initial code, so I'm suspicious it was something simple.

I don't have cycles to deeply investigate. I created another PR to test this one alongside the PR #642. Note the container is a based on a rolling release so maybe something changed in the base image? We could also "bisect" the commits in this PR to narrow down the change that actually triggers the failure.

frasertweedale · 2020-10-15T07:53:20Z

@cipherboy hm, yeah it sure is weird. Only Ubuntu, only FIPS, only when ALPN is used. I note that ubuntu "focal" is at NSS v3.49 - https://packages.ubuntu.com/source/focal/nss. But Centos 7 is at 3.44 and f31 and f32 are at 3.53 and 3.55 respectively, so the version doesn't seem to be the issue.

Looking further, it is the server unwrap at NEED_UNWRAP that is failing. But you would really have to get more logging to work out why it is failing. Rather than trying to debug inside the container, maybe just make an Ubuntu VM and try and repro there?

I'd have a look at what is enabled/disabled in FIPS mode on Ubuntu; might give a clue.

frasertweedale · 2020-10-15T07:55:57Z

Traceback:

INFO: [JSS Server for ALPN] JSSEngine.unwrap() - Got SSLException: javax.net.ssl.SSLException: Unexpected return from PR.Read(): SSL_ERROR_HANDSHAKE_FAILED (-12116)
Exception in thread "main" javax.net.ssl.SSLException: Unexpected return from PR.Read(): SSL_ERROR_HANDSHAKE_FAILED (-12116)
	at org.mozilla.jss.ssl.javax.JSSEngineReferenceImpl.unwrap(JSSEngineReferenceImpl.java:1257)
	at java.base/javax.net.ssl.SSLEngine.unwrap(SSLEngine.java:637)
	at org.mozilla.jss.tests.TestSSLEngine.testHandshake(TestSSLEngine.java:468)
	at org.mozilla.jss.tests.TestSSLEngine.testInitialHandshake(TestSSLEngine.java:786)
	at org.mozilla.jss.tests.TestSSLEngine.testALPNHandshake(TestSSLEngine.java:1005)
	at org.mozilla.jss.tests.TestSSLEngine.testNativeClientServer(TestSSLEngine.java:1039)
	at org.mozilla.jss.tests.TestSSLEngine.main(TestSSLEngine.java:1095)

frasertweedale · 2020-10-15T08:15:47Z

Possibly relevant: Ubuntu patches nss to always return FALSE from NSS "is fips enabled?" subroutines: https://git.launchpad.net/ubuntu/+source/nss/tree/debian/patches/disable_fips_enabled_read.patch?id=af745d02d46dd21c52abe39093188ac6e0918df3

cipherboy · 2020-10-15T14:33:51Z

Hmmm ok. I'll try the bisect method later. Curious. :-)

Signed-off-by: Alexander Scheel <[email protected]>

ALPN support merged in JDK9 and will eventually be backported into JDK8. However, not every consumer of JSS will necessarily upgrade to a newer JDK8 version. While support has landed upstream, Fedora 32 and Fedora 33 haven't yet seen a backport yet. This likely means that RHEL also won't see a backport. Introduce a small reflection-based compatibility layer to see if the class supports ALPN and if so, use the value from it. Signed-off-by: Alexander Scheel <[email protected]>

It looks like UTF-8 is the desired encoding for ALPN data for compatibility with what the JCA and SunJSSE does: https://github.com/openjdk/jdk11u-dev/blob/master/src/java.base/share/classes/sun/security/ssl/AlpnExtension.java#L100 Therefore we should make sure we encode our protocols the same way. Signed-off-by: Alexander Scheel <[email protected]>

Since setting parameters via JSSParameters was already supported, technically no work is necessary to support ALPN in JSSSocket. However, we should extend JSSSocket with the new methods added to JSSEngine and implement them via calls to the underlying engine. Signed-off-by: Alexander Scheel <[email protected]>

This will let GREASE'd ALPN values be handled correctly in JSS, despite the JDK not working well. Additionally, handle NSS's quirks w.r.t. ALPN protocol preference. Signed-off-by: Alexander Scheel <[email protected]>

Signed-off-by: Alexander Scheel <[email protected]>

cipherboy added enhancement New feature or request jdk9+ Changes related to JDK9+ support javax Work to support javax.net.ssl interfaces labels Apr 29, 2020

cipherboy added this to the 4.7.0 milestone Apr 29, 2020

cipherboy self-assigned this Apr 29, 2020

cipherboy commented Apr 29, 2020

View reviewed changes

org/mozilla/jss/ssl/javax/JSSEngineReferenceImpl.java Outdated Show resolved Hide resolved

cipherboy force-pushed the sslengine-alpn branch 3 times, most recently from 209030d to 971ffdd Compare May 1, 2020 15:14

cipherboy requested a review from edewata May 1, 2020 15:14

cipherboy mentioned this pull request May 1, 2020

JSSEngine: Add getSSLParameters #535

Merged

edewata reviewed May 1, 2020

View reviewed changes

org/mozilla/jss/ssl/javax/JSSEngine.java Show resolved Hide resolved

org/mozilla/jss/ssl/javax/JSSParameters.java Show resolved Hide resolved

org/mozilla/jss/ssl/javax/JSSParameters.java Show resolved Hide resolved

edewata reviewed May 1, 2020

View reviewed changes

org/mozilla/jss/util/JDKCompat.java Outdated Show resolved Hide resolved

cipherboy force-pushed the sslengine-alpn branch from 971ffdd to 8cc0119 Compare May 4, 2020 19:59

cipherboy removed this from the 4.7.0 milestone Jun 23, 2020

cipherboy added this to the 4.8.0 milestone Aug 3, 2020

cipherboy force-pushed the sslengine-alpn branch 3 times, most recently from 3bfccd3 to b159a1d Compare September 23, 2020 14:03

edewata approved these changes Sep 23, 2020

View reviewed changes

frasertweedale mentioned this pull request Oct 6, 2020

Add ALPN support in SSLSocket #640

Closed

cipherboy commented Oct 6, 2020

View reviewed changes

org/mozilla/jss/tests/TestSSLEngine.java Outdated Show resolved Hide resolved

cipherboy force-pushed the sslengine-alpn branch from b159a1d to 9d336c7 Compare October 6, 2020 13:41

cipherboy requested review from edewata and frasertweedale October 6, 2020 13:41

edewata approved these changes Oct 6, 2020

View reviewed changes

frasertweedale reviewed Oct 8, 2020

View reviewed changes

org/mozilla/jss/ssl/javax/JSSEngine.java Show resolved Hide resolved

cipherboy force-pushed the sslengine-alpn branch from f181c23 to 9c3f195 Compare October 9, 2020 16:04

cipherboy changed the title ~~JSSEngine - Initial Client ALPN support~~ JSSEngine - Initial ALPN support Oct 9, 2020

frasertweedale reviewed Oct 13, 2020

View reviewed changes

org/mozilla/jss/ssl/javax/JSSParameters.java Show resolved Hide resolved

frasertweedale requested changes Oct 13, 2020

View reviewed changes

frasertweedale mentioned this pull request Oct 15, 2020

[test] PR 526 + 642 to see if CI issues go away #644

Closed

cipherboy modified the milestones: 4.8.0, 4.9.0 Nov 5, 2020

cipherboy added 11 commits December 16, 2020 09:23

Add SSL.SetNextProtoNeg()

12e4bc5

Signed-off-by: Alexander Scheel <[email protected]>

Add SSLNextProtoState enum

326dc83

Signed-off-by: Alexander Scheel <[email protected]>

Add SSL.GetNextProto()

d38661f

Signed-off-by: Alexander Scheel <[email protected]>

Implement intial ALPN support in JSSEngine

1f5f18a

Signed-off-by: Alexander Scheel <[email protected]>

Add test for JSSEngine.getALPNWireData()

784501a

Signed-off-by: Alexander Scheel <[email protected]>

Add test for ALPN negotiation during handshake

31257d1

Signed-off-by: Alexander Scheel <[email protected]>

Support byte-encoded application protocols

5998295

This will let GREASE'd ALPN values be handled correctly in JSS, despite the JDK not working well. Additionally, handle NSS's quirks w.r.t. ALPN protocol preference. Signed-off-by: Alexander Scheel <[email protected]>

Explicitly test null and zero-length ALPN protos

f50bf1c

Signed-off-by: Alexander Scheel <[email protected]>

cipherboy force-pushed the sslengine-alpn branch from a6c2f8c to f50bf1c Compare December 16, 2020 14:23

cipherboy removed their assignment Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSSEngine - Initial ALPN support #526

JSSEngine - Initial ALPN support #526

cipherboy commented Apr 29, 2020 •

edited

Loading

cipherboy commented May 1, 2020

edewata left a comment

cipherboy commented Sep 23, 2020

cipherboy commented Sep 23, 2020 •

edited

Loading

edewata left a comment

edewata Sep 23, 2020

frasertweedale Oct 6, 2020

cipherboy Oct 6, 2020

frasertweedale Oct 7, 2020

cipherboy Oct 7, 2020 •

edited

Loading

cipherboy Oct 7, 2020

frasertweedale Oct 8, 2020

cipherboy Oct 8, 2020

cipherboy Oct 8, 2020

frasertweedale Oct 9, 2020

edewata left a comment

cipherboy commented Oct 6, 2020

cipherboy commented Oct 8, 2020

frasertweedale commented Oct 9, 2020

cipherboy commented Oct 9, 2020

frasertweedale Oct 13, 2020

cipherboy commented Oct 14, 2020

frasertweedale commented Oct 15, 2020

frasertweedale commented Oct 15, 2020

frasertweedale commented Oct 15, 2020

frasertweedale commented Oct 15, 2020

cipherboy commented Oct 15, 2020

JSSEngine - Initial ALPN support #526

Are you sure you want to change the base?

JSSEngine - Initial ALPN support #526

Conversation

cipherboy commented Apr 29, 2020 • edited Loading

cipherboy commented May 1, 2020

edewata left a comment

Choose a reason for hiding this comment

cipherboy commented Sep 23, 2020

cipherboy commented Sep 23, 2020 • edited Loading

edewata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edewata left a comment

Choose a reason for hiding this comment

cipherboy commented Oct 6, 2020

cipherboy commented Oct 8, 2020

frasertweedale commented Oct 9, 2020

cipherboy commented Oct 9, 2020

Choose a reason for hiding this comment

cipherboy commented Oct 14, 2020

frasertweedale commented Oct 15, 2020

frasertweedale commented Oct 15, 2020

frasertweedale commented Oct 15, 2020

frasertweedale commented Oct 15, 2020

cipherboy commented Oct 15, 2020

cipherboy commented Apr 29, 2020 •

edited

Loading

cipherboy commented Sep 23, 2020 •

edited

Loading

cipherboy Oct 7, 2020 •

edited

Loading