Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ALPN support in SSLSocket #640

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/jss.map
Original file line number Diff line number Diff line change
Expand Up @@ -499,3 +499,9 @@ Java_org_mozilla_jss_nss_SSLErrors_getBadCertDomain;
local:
*;
};
JSS_4.8.0 {
global:
Java_org_mozilla_jss_ssl_SSLSocket_getNegotiatedProtocol;
local:
*;
};
37 changes: 36 additions & 1 deletion org/mozilla/jss/ssl/SSLSocket.c
Original file line number Diff line number Diff line change
Expand Up @@ -572,9 +572,32 @@ Java_org_mozilla_jss_ssl_SSLSocket_getPort(JNIEnv *env,
}
}

JNIEXPORT jbyteArray JNICALL
Java_org_mozilla_jss_ssl_SSLSocket_getNegotiatedProtocol(JNIEnv *env, jobject self)
{
JSSL_SocketData *sock;
SECStatus status;
SSLNextProtoState npState;
unsigned char buf[255];
unsigned int bufLen = 0;

if (JSSL_getSockData(env, self, &sock) != PR_SUCCESS || sock == NULL) {
/* exception was thrown */
EXCEPTION_CHECK(env, sock);
return NULL;
}

status = SSL_GetNextProto(sock->fd, &npState, buf, &bufLen, sizeof(buf));
if (status == SECSuccess && bufLen > 0 && bufLen <= sizeof(buf)) {
return JSS_ToByteArray(env, buf, bufLen);
} else {
return NULL;
}
}

JNIEXPORT void JNICALL
Java_org_mozilla_jss_ssl_SSLSocket_socketConnect
(JNIEnv *env, jobject self, jbyteArray addrBA, jstring hostname, jint port)
(JNIEnv *env, jobject self, jbyteArray addrBA, jstring hostname, jint port, jbyteArray alpn)
{
JSSL_SocketData *sock;
PRNetAddr addr;
Expand All @@ -584,6 +607,9 @@ Java_org_mozilla_jss_ssl_SSLSocket_socketConnect
int stat;
const char *hostnameStr=NULL;

unsigned char *alpnData = NULL;
int alpnLength = 0;

jmethodID supportsIPV6ID;
jclass socketBaseClass;
jboolean supportsIPV6 = 0;
Expand Down Expand Up @@ -630,6 +656,14 @@ Java_org_mozilla_jss_ssl_SSLSocket_socketConnect
goto finish;
}

/* Set ALPN extension */
if (alpn != NULL) {
JSS_RefByteArray(env, alpn, (jbyte**)&alpnData, &alpnLength);
if (alpnLength > 0) {
SSL_SetNextProtoNego(sock->fd, alpnData, alpnLength);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

@cipherboy cipherboy Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cipherboy yep :D

}
}

if(addrBALen != 4 && addrBALen != 16) {
JSSL_throwSSLSocketException(env, "Invalid address in connect!");
goto finish;
Expand Down Expand Up @@ -672,6 +706,7 @@ Java_org_mozilla_jss_ssl_SSLSocket_socketConnect

JSS_DerefJString(env, hostname, hostnameStr);
JSS_DerefByteArray(env, addrBA, addrBAelems, JNI_ABORT);
JSS_DerefByteArray(env, alpn, alpnData, JNI_ABORT);
}

JNIEXPORT jobject JNICALL
Expand Down
88 changes: 83 additions & 5 deletions org/mozilla/jss/ssl/SSLSocket.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.ByteArrayOutputStream;
import java.net.InetAddress;
import java.net.SocketException;
import java.net.SocketTimeoutException;
Expand Down Expand Up @@ -547,16 +548,76 @@ public SSLSocket(InetAddress address, int port,
SSLClientCertificateSelectionCallback clientCertSelectionCallback)
throws IOException
{
this(address, address.getHostName(), port, localAddr, localPort,
this(address, address.getHostName(), port, localAddr, localPort, null /* alpn */,
certApprovalCallback, clientCertSelectionCallback);
}

/**
* Create an SSL client socket and connects to the specified
* address and port. Installs the given callbacks for
* certificate approval and client certificate selection.
* Supports Application Layer Protocol Negotiation (ALPN)
* extension.
*
* @param address The IP address to connect to.
* @param port The port to connect to.
* @param hostname Hostname to use for SNI
* @param alpn Array of ALPN protocol IDS. If empty, extension is not
* added. Each protocol ID must have a length between 1 and 255
* bytes.
* @param certApprovalCallback A callback that can be used to override
* approval of the peer's certificate.
* @param clientCertSelectionCallback A callback to select the client
* certificate to present to the peer.
*/
public SSLSocket(
InetAddress address, int port,
String hostname, byte[][]alpn,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ��.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-)

SSLCertificateApprovalCallback certApprovalCallback,
SSLClientCertificateSelectionCallback clientCertSelectionCallback)
throws IOException {
this(
address, hostname, port, null, 0, alpn,
certApprovalCallback, clientCertSelectionCallback);
}


/** Write an ALPN protocol name to the OutputStream. The protocol name
* will be prefixed by its length (as a single byte).
*
* @throws IllegalArgumentException if length of protocol name is
* zero or greater than 255.
*/
private static void writeALPNProtocol(OutputStream out, byte[] s)
throws IOException {
if (s.length < 1) throw new IllegalArgumentException("ALPN protocol cannot be empty");
if (s.length > 255) throw new IllegalArgumentException("ALPN protocol is too long");
out.write(s.length);
out.write(s);
}

private SSLSocket(InetAddress address, String hostname, int port,
InetAddress localAddr,
int localPort, SSLCertificateApprovalCallback certApprovalCallback,
InetAddress localAddr, int localPort,
byte[][] alpn,
SSLCertificateApprovalCallback certApprovalCallback,
SSLClientCertificateSelectionCallback clientCertSelectionCallback)
throws IOException
{
/* Create ALPN data
*
* Due to an historical quirk, SSL_SetNextProtoNego moves the first
* protocol to the end. Therefore we put the last protocol at the
* beginning of the formatted string.
*/
byte[] alpnData = null;
if (alpn != null && alpn.length > 0) {
ByteArrayOutputStream out = new ByteArrayOutputStream();
writeALPNProtocol(out, alpn[alpn.length - 1]);
for (int i = 0; i < alpn.length - 1; i++) {
writeALPNProtocol(out, alpn[i]);
}
alpnData = out.toByteArray();
}

int socketFamily = SocketBase.SSL_AF_INET;
if(SocketBase.supportsIPV6()) {
Expand All @@ -581,7 +642,7 @@ private SSLSocket(InetAddress address, String hostname, int port,
}

/* connect to the remote socket */
socketConnect(address.getAddress(), hostname, port);
socketConnect(address.getAddress(), hostname, port, alpnData);
}

/**
Expand Down Expand Up @@ -613,6 +674,14 @@ public SSLSocket(java.net.Socket s, String host,
resetHandshake();
}

/* Get result of ALPN negotiation.
*
* Prior to the handshake this will always return null.
* After the handshake, returns the negotiated protocol
* or null (e.g. when the server does not support ALPN).
*/
public native byte[] getNegotiatedProtocol() throws SocketException;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* @return The remote peer's IP address or null if the SSLSocket is closed.
*/
Expand Down Expand Up @@ -789,7 +858,16 @@ public void close() throws IOException {
}
}

private native void socketConnect(byte[] addr, String hostname, int port)
/**
* Connect socket
*
* @param addr IPv4 or IPv6 address
* @param hostname server name (used for SNI)
* @param port port number
* @param alpn *formatted* ALPN extension data (see SSL_SetNextProtoNego),
* or null if ALPN extension is not to be sent
*/
private native void socketConnect(byte[] addr, String hostname, int port, byte[] alpn)
throws SocketException;

////////////////////////////////////////////////////////////////////
Expand Down