Add ALPN support in SSLSocket #640

frasertweedale · 2020-10-06T04:15:54Z

This is a prerequisite for ACME tls-alpn-01 challenge verification.
See https://gist.github.com/frasertweedale/dfac386579cb2d1fe892f39d22c2975d
for usage / testbed.

ab29dd6c (Fraser Tweedale, 7 minutes ago)
   alpn: support via new SSLSocket constructor

   Add a new SSLSocket constructor that allows the caller to specify ALPN
   protocols.

555134db (Fraser Tweedale, 2 hours ago)
   alpn: add SSLSocket.getNegotiatedProtocol()

   Add a method to retrieve the identifier of the negotiated protocol, if
   available.

465ac1d9 (Fraser Tweedale, 4 days ago)
   alpn: format and supply ALPN data if user provides it

   Update the private SSLSocket constructor to accept a String[] of ALPN
   protocol names in preferred order.  When given, it will prepare the ALPN
   extension data in the required format and pass it along to the
   socketConnect method.

   The change to publicly expose this capability will occur in a subsequent
   commit.

6f5a0cf8 (Fraser Tweedale, 4 days ago)
   alpn: add support in native socketConnect method

   Update the private native socketConnect method to receive optional ALPN
   data to include in handshake.  The value is performatted per 
   SSL_SetNextProtoNego().

   Support for library users to supply the ALPN data will follow in subsequent
   commits.

Update the private native socketConnect method to receive optional ALPN data to include in handshake. The value is performatted per SSL_SetNextProtoNego(). Support for library users to supply the ALPN data will follow in subsequent commits.

Update the private SSLSocket constructor to accept a String[] of ALPN protocol names in preferred order. When given, it will prepare the ALPN extension data in the required format and pass it along to the socketConnect method. The change to publicly expose this capability will occur in a subsequent commit.

Add a method to retrieve the identifier of the negotiated protocol, if available.

Add a new SSLSocket constructor that allows the caller to specify ALPN protocols.

frasertweedale · 2020-10-06T04:26:24Z

@cipherboy ahaha whups, I did not even see #526 :S

frasertweedale · 2020-10-06T05:16:22Z

@cipherboy ah, looks like #526 is about the server side anyway, whereas this PR is for the client side and is mostly orthogonal.

cipherboy

This is largely fine.

What's missing is a ALPN selection routine on the server. I'm not sure if you wanted to add that; this is purely client side support, as is mine in #526 (it doesn't do server selection at all--that requires a call to SSL_SetNextProtoCallback).

My only thing is... this is the legacy SSLSocket... We should really be adding features its replacement, org.mozilla.jss.ssl.javax.JSSSocket.

I guess, what's the use case for still using this one? The other one is compliant with standard JDK interfaces and I'd prefer to push #526 (and then a later one adding server-side ALPN support) rather than continuing to add new features to the old one.

It is on my TODO list to rip out all of this old SSLSocket, including from PKI.

My 2c. I'll merge if there's a compelling reason to not use the new one for something.

cipherboy · 2020-10-06T12:56:23Z

org/mozilla/jss/ssl/SSLSocket.java

+     * After the handshake, returns the negotiated protocol
+     * or null (e.g. when the server does not support ALPN).
+     */
+    public native byte[] getNegotiatedProtocol() throws SocketException;


Not that we have any hope of making org.mozilla.jss.ssl.SSLSocket comply with javax.net.ssl.SSLSocket at this point in time (I've tried, twice!)... But how about we at least comply when the functions do the same thing? :-)

https://docs.oracle.com/javase/9/docs/api/javax/net/ssl/SSLSocket.html#getApplicationProtocol--

(return String and getNegotiatedProtocol -> getApplicationProtocol since they do the same thing).

Change the name sure. Return String I am less sure about (discussed elsewhere).

cipherboy · 2020-10-06T13:00:15Z

org/mozilla/jss/ssl/SSLSocket.java

+     */
+    public SSLSocket(
+        InetAddress address, int port,
+        String hostname, byte[][]alpn,


In your commit message you wrote:

Update the private SSLSocket constructor to accept a String[] of ALPN

But I only see byte[][] of ALPNs. I'd prefer we stick with JCA-standard types here rather than introducing our own in general... But at minimum, the commit message should match the code. :-)

No, it should be byte[] - this is how ALPN protocol IDs are defined. See https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids and https://tools.ietf.org/html/rfc7301#section-6 .

Just saw your comment on the other PR. Bravo Java, bravo. What if the sequence of bytes is not valid UTF-8? What do you know, there are ALPN GREASE values that are not valid UTF-8 (see https://tools.ietf.org/html/rfc8701, e.g. {0xFA, 0xFA}. If you want to set them, you cannot. If you want to read them, you cannot (the UTF-8 reader converts them to ��.

cipherboy · 2020-10-06T13:04:27Z

org/mozilla/jss/ssl/SSLSocket.c

+    if (alpn != NULL) {
+        JSS_RefByteArray(env, alpn, (jbyte**)&alpnData, &alpnLength);
+        if (alpnLength > 0) {
+            SSL_SetNextProtoNego(sock->fd, alpnData, alpnLength);


ALPN data only really needs to be provided before the handshake is performed. Not that I mind the extra constructors, but should we add a separate setApplicationProtocols(...) function to add this after construction? This would let us set it after construction from a JSSSocketFactory (which I think we have in ldap-jdk and/or PKI).

(And IIRC, it can be used with TLSv1.2 and can be provided on later handshakes. Under TLSv1.3 there's only the initial one so I guess it doesn't matter too much.)

I don't think we need to worry about the renegotiation use case. But if it does turn out to be important we could add the extra function then.

Or now... I do prefer supplying as much "config" in the constructor rather than afterwards. Having to call extra functions to do more setup is something I find distasteful (I use that word in acknowledgement that it is indeed a matter of taste ^_^). Ideally there would be a single configuration record type that can supply SNI, ALPN and be extensible as new knobs are needed, so that there is not a combinatorial explosion in the number of possible constructors :)

It also turns out (I had forgotten until I made changes in my other PR) -- NSS doesn't support changing ALPN values on renegotiation. Even 0RTT assumes (and validates!) ALPN doesn't change any.

"single configuration record type"

... So ... JSSParameters? :-)

@cipherboy yep :D

frasertweedale · 2020-10-07T01:19:20Z

This is largely fine.

What's missing is a ALPN selection routine on the server. I'm not sure if you wanted to add that; this is purely client side support, as is mine in #526 (it doesn't do server selection at all--that requires a call to SSL_SetNextProtoCallback).

My only thing is... this is the legacy SSLSocket... We should really be adding features its replacement, org.mozilla.jss.ssl.javax.JSSSocket.

I guess, what's the use case for still using this one? The other one is compliant with standard JDK interfaces and I'd prefer to push #526 (and then a later one adding server-side ALPN support) rather than continuing to add new features to the old one.

It is on my TODO list to rip out all of this old SSLSocket, including from PKI.

My 2c. I'll merge if there's a compelling reason to not use the new one for something.

I'll have a closer review of #526. I don't mind to abandon this #526 satisfies the client side use case of ACME tls-alpn-01 challenge.

frasertweedale · 2020-10-08T01:14:45Z

Abandoning in favour of #526 .

frasertweedale added 4 commits October 2, 2020 15:00

alpn: add SSLSocket.getNegotiatedProtocol()

555134d

Add a method to retrieve the identifier of the negotiated protocol, if available.

alpn: support via new SSLSocket constructor

ab29dd6

Add a new SSLSocket constructor that allows the caller to specify ALPN protocols.

frasertweedale requested review from cipherboy and edewata October 6, 2020 04:16

frasertweedale mentioned this pull request Oct 6, 2020

JSSEngine - Initial ALPN support #526

Open

cipherboy requested changes Oct 6, 2020

View reviewed changes

frasertweedale closed this Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ALPN support in SSLSocket #640

Add ALPN support in SSLSocket #640

frasertweedale commented Oct 6, 2020

frasertweedale commented Oct 6, 2020

frasertweedale commented Oct 6, 2020

cipherboy left a comment

cipherboy Oct 6, 2020

frasertweedale Oct 7, 2020

cipherboy Oct 6, 2020

frasertweedale Oct 7, 2020

frasertweedale Oct 7, 2020

cipherboy Oct 7, 2020

cipherboy Oct 6, 2020

cipherboy Oct 6, 2020

frasertweedale Oct 7, 2020

cipherboy Oct 7, 2020 •

edited

Loading

frasertweedale Oct 8, 2020

frasertweedale commented Oct 7, 2020

frasertweedale commented Oct 8, 2020

Add ALPN support in SSLSocket #640

Add ALPN support in SSLSocket #640

Conversation

frasertweedale commented Oct 6, 2020

frasertweedale commented Oct 6, 2020

frasertweedale commented Oct 6, 2020

cipherboy 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

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

frasertweedale commented Oct 7, 2020

frasertweedale commented Oct 8, 2020

cipherboy Oct 7, 2020 •

edited

Loading