-
Notifications
You must be signed in to change notification settings - Fork 30
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
Changes from all commits
6f5a0cf
465ac1d
555134d
ab29dd6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In your commit message you wrote:
But I only see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it should be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) { | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that we have any hope of making https://docs.oracle.com/javase/9/docs/api/javax/net/ssl/SSLSocket.html#getApplicationProtocol-- (return There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
*/ | ||
|
@@ -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; | ||
|
||
//////////////////////////////////////////////////////////////////// | ||
|
There was a problem hiding this comment.
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 aJSSSocketFactory
(which I think we have in ldap-jdk and/or PKI).There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
... So ... JSSParameters? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cipherboy yep :D