Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Improved support for Server Name Indication (SNI) #349

Closed
wants to merge 2 commits into from

3 participants

@ptoomey3

Fixes JRUBY-6891, JRUBY-6944, and should fix JRUBY-6771 (unverified).

Java 7 and greater supports SNI when creating SSL Sockets.
JRuby's origional support performed unnecessary reverse DNS
lookups if the SSL destination was an IP address instead
of a hostname. This patch is more inline with how MRI
works, as it requires the developer to explicitly
set a hostname, thus avoiding any reverse lookups.

ptoomey3 added some commits
@ptoomey3 ptoomey3 Improved support for Server Name Indication (SNI)
Java 7 and greater supports SNI when creating SSL Sockets.
JRuby's origional support performed unnecessary reverse DNS
lookups if the SSL destination was an IP address instead
of a hostname.  This patch is more inline with how MRI
works, as it requires the developer to explicitly
set a hostname, thus avoiding any reverse lookups.
8fbe70c
@ptoomey3 ptoomey3 Merge remote-tracking branch 'upstream/master' 7da35ab
@BanzaiMan
Owner

Your patch enables a test in the MRI suite that fails. https://github.com/ruby/ruby/blob/v1_9_3_286/test/openssl/test_ssl.rb#L354-370

OpenSSL::SSL::SSContext needs servername_cb attribute: http://www.ruby-doc.org/stdlib-1.9.3/libdoc/openssl/rdoc/OpenSSL/SSL/SSLContext.html

I am not familiar with SSL, so I can't comment much further than this.

@ptoomey3

Hey,
So, I am also not terribly familiar with the openssl codebase. But, I did a bit of digging this morning, and as far as I can tell, the test is not ideal, as it requires that both client SNI and server SNI to be supported for that test to pass. However, the test is run anytime client SNI is detected (i.e. instance_methods.include?(:hostname)). It would probably make sense for that test to really check the following before proceeding:

return unless OpenSSL::SSL::SSLSocket.instance_methods.include?(:hostname) and OpenSSL::SSL::SSLContext.instance_methods.include?(:servername_cb)

I can see why the test was written that way, as it is difficult to validate that client SNI is working without being able to create a server to test against. But, SNI support in Java is new, as of JDK 7, and as far as I understand, it only support client SNI. So, as of now, I don't think it is possible/practical to implement server SNI (i.e. add support for servername_cb) until Java supports it.

In summary, I think it would make sense to augment the test conditions in test_ssl.rb such that one can still move forward with client support for SNI (probably the much more common use case). If that set of unit tests is kepy in sync with MRI, then it might make sense to ask upstream MRI to check for both client and server support for SNI before running that test.

@BanzaiMan
Owner

@ptoomey3 Thank you for looking into the fitness of the tests. As they come from MRI, do you mind dropping them a note at http://bugs.ruby-lang.org? If you have test cases in mind, that will help tremendously, too.

(cc @nahi)

@headius
Owner

I incorporated your change and masked out the failing test for now in the following commits. @ptoomey3 Will you follow up with ruby-core about fixing the test? We'd like to unmask it as soon as possible, so we know we're actually doing it right.

commit 1e9086f

Author: Charles Oliver Nutter <headius@headius.com>
Date:   Sat Oct 27 01:17:54 2012 -0500

    Mask newly-failing test enabled by previous commit.

commit 607207f

Author: Patrick Toomey <ptoomey3@biasedcoin.com>
Date:   Wed Oct 17 16:05:13 2012 -0700

    Improved support for Server Name Indication (SNI)

    Java 7 and greater supports SNI when creating SSL Sockets.
    JRuby's origional support performed unnecessary reverse DNS
    lookups if the SSL destination was an IP address instead
    of a hostname.  This patch is more inline with how MRI
    works, as it requires the developer to explicitly
    set a hostname, thus avoiding any reverse lookups.
@headius headius closed this
@ptoomey3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 18, 2012
  1. @ptoomey3

    Improved support for Server Name Indication (SNI)

    ptoomey3 authored
    Java 7 and greater supports SNI when creating SSL Sockets.
    JRuby's origional support performed unnecessary reverse DNS
    lookups if the SSL destination was an IP address instead
    of a hostname.  This patch is more inline with how MRI
    works, as it requires the developer to explicitly
    set a hostname, thus avoiding any reverse lookups.
  2. @ptoomey3
This page is out of date. Refresh to see the latest.
View
29 src/org/jruby/ext/openssl/SSLContext.java
@@ -12,7 +12,7 @@
* rights and limitations under the License.
*
* Copyright (C) 2006 Ola Bini <ola@ologix.com>
- *
+ *
* Alternatively, the contents of this file may be used under the terms of
* either of the GNU General Public License Version 2 or later (the "GPL"),
* or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
@@ -80,7 +80,7 @@
private final static Map<String, String> SSL_VERSION_OSSL2JSSE;
// Mapping table for JSEE's enabled protocols for the algorithm.
private final static Map<String, String[]> ENABLED_PROTOCOLS;
-
+
static {
SSL_VERSION_OSSL2JSSE = new HashMap<String, String>();
ENABLED_PROTOCOLS = new HashMap<String, String[]>();
@@ -106,7 +106,7 @@
ENABLED_PROTOCOLS.put("SSL", new String[] { "SSLv2", "SSLv3", "TLSv1" });
// Followings(TLS, TLSv1.1) are JSSE only methods at present. Let's allow user to use it.
-
+
SSL_VERSION_OSSL2JSSE.put("TLS", "TLS");
ENABLED_PROTOCOLS.put("TLS", new String[] { "TLSv1", "TLSv1.1" });
@@ -119,7 +119,7 @@ public IRubyObject allocate(Ruby runtime, RubyClass klass) {
return new SSLContext(runtime, klass);
}
};
-
+
public static void createSSLContext(Ruby runtime, RubyModule mSSL) {
RubyClass cSSLContext = mSSL.defineClassUnder("SSLContext",runtime.getObject(),SSLCONTEXT_ALLOCATOR);
for(int i=0;i<ctx_attrs.length;i++) {
@@ -159,7 +159,7 @@ public IRubyObject setup() {
return getRuntime().getNil();
}
this.freeze(getRuntime().getCurrentContext());
-
+
internalCtx = new InternalContext();
internalCtx.protocol = protocol;
internalCtx.protocolForServer = protocolForServer;
@@ -245,7 +245,7 @@ public IRubyObject setup() {
if (value != null && !value.isNil()) {
internalCtx.timeout = RubyNumeric.fix2int(value);
}
-
+
value = getInstanceVariable("@verify_depth");
if (value != null && !value.isNil()) {
internalCtx.store.setDepth(RubyNumeric.fix2int(value));
@@ -380,7 +380,16 @@ SSLEngine createDummySSLEngine() throws GeneralSecurityException {
// should keep SSLContext as a member for introducin SSLSession. later...
SSLEngine createSSLEngine(String peerHost, int peerPort) throws NoSuchAlgorithmException, KeyManagementException {
- SSLEngine engine = internalCtx.getSSLContext().createSSLEngine(peerHost, peerPort);
+ SSLEngine engine;
+ // an empty peerHost implies no SNI (RFC 3546) support requested
+ if (peerHost == null || peerHost.length() == 0) {
+ engine = internalCtx.getSSLContext().createSSLEngine();
+ }
+ // SNI is attempted for valid peerHost hostname on Java >= 7
+ // if peerHost is set to an IP address Java does not use SNI
+ else {
+ engine = internalCtx.getSSLContext().createSSLEngine(peerHost, peerPort);
+ }
engine.setEnabledCipherSuites(getCipherSuites(engine));
engine.setEnabledProtocols(getEnabledProtocols(engine));
return engine;
@@ -438,7 +447,7 @@ private String sslVersionString(long bits) {
}
return sb.toString();
}
-
+
private PKey getCallbackKey() {
if (t_key != null) {
return t_key;
@@ -505,7 +514,7 @@ private long getOptions() {
return 0;
}
}
-
+
private X509Cert[] convertToX509Certs(IRubyObject value) {
final ArrayList<X509Cert> result = new ArrayList<X509Cert>();
ThreadContext ctx = getRuntime().getCurrentContext();
@@ -582,7 +591,7 @@ StoreContext createStoreContext(String purpose) {
private static class KM extends javax.net.ssl.X509ExtendedKeyManager {
private final InternalContext ctx;
-
+
public KM(InternalContext ctx) {
super();
this.ctx = ctx;
View
32 src/org/jruby/ext/openssl/SSLSocket.java
@@ -12,7 +12,7 @@
* rights and limitations under the License.
*
* Copyright (C) 2006, 2007 Ola Bini <ola@ologix.com>
- *
+ *
* Alternatively, the contents of this file may be used under the terms of
* either of the GNU General Public License Version 2 or later (the "GPL"),
* or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
@@ -81,12 +81,13 @@ public IRubyObject allocate(Ruby runtime, RubyClass klass) {
};
private static RubyObjectAdapter api = JavaEmbedUtils.newObjectAdapter();
-
+
public static void createSSLSocket(Ruby runtime, RubyModule mSSL) {
RubyClass cSSLSocket = mSSL.defineClassUnder("SSLSocket",runtime.getObject(),SSLSOCKET_ALLOCATOR);
cSSLSocket.addReadWriteAttribute(runtime.getCurrentContext(), "io");
cSSLSocket.addReadWriteAttribute(runtime.getCurrentContext(), "context");
cSSLSocket.addReadWriteAttribute(runtime.getCurrentContext(), "sync_close");
+ cSSLSocket.addReadWriteAttribute(runtime.getCurrentContext(), "hostname");
cSSLSocket.defineAlias("to_io","io");
cSSLSocket.defineAnnotatedMethods(SSLSocket.class);
}
@@ -95,7 +96,7 @@ public SSLSocket(Ruby runtime, RubyClass type) {
super(runtime,type);
verifyResult = X509Utils.V_OK;
}
-
+
public static RaiseException newSSLError(Ruby runtime, String message) {
return Utils.newError(runtime, "OpenSSL::SSL::SSLError", message, false);
}
@@ -108,14 +109,14 @@ public static RaiseException newSSLError(Ruby runtime, String message) {
private ByteBuffer peerNetData;
private ByteBuffer netData;
private ByteBuffer dummy;
-
+
private boolean initialHandshake = false;
-
+
private SSLEngineResult.HandshakeStatus hsStatus;
private SSLEngineResult.Status status = null;
int verifyResult;
-
+
@JRubyMethod(name = "initialize", rest = true, frame = true)
public IRubyObject _initialize(IRubyObject[] args, Block unused) {
if (Arity.checkArgumentCount(getRuntime(), args, 1, 2) == 1) {
@@ -127,6 +128,7 @@ public IRubyObject _initialize(IRubyObject[] args, Block unused) {
Utils.checkKind(getRuntime(), args[0], "IO");
io = (RubyIO) args[0];
api.callMethod(this, "io=", io);
+ api.callMethod(this, "hostname=", getRuntime().newString(""));
// This is a bit of a hack: SSLSocket should share code with RubyBasicSocket, which always sets sync to true.
// Instead we set it here for now.
api.callMethod(io, "sync=", getRuntime().getTrue());
@@ -139,12 +141,14 @@ public IRubyObject _initialize(IRubyObject[] args, Block unused) {
private void ossl_ssl_setup() throws NoSuchAlgorithmException, KeyManagementException, IOException {
if(null == engine) {
Socket socket = getSocketChannel().socket();
- String peerHost = socket.getInetAddress().getHostName();
+ // Server Name Indication (SNI) RFC 3546
+ // SNI support will not be attempted unless hostname is explicitly set by the caller
+ String peerHost = api.callMethod(this,"hostname").convertToString().toString();
int peerPort = socket.getPort();
engine = rubyCtx.createSSLEngine(peerHost, peerPort);
SSLSession session = engine.getSession();
peerNetData = ByteBuffer.allocate(session.getPacketBufferSize());
- peerAppData = ByteBuffer.allocate(session.getApplicationBufferSize());
+ peerAppData = ByteBuffer.allocate(session.getApplicationBufferSize());
netData = ByteBuffer.allocate(session.getPacketBufferSize());
peerNetData.limit(0);
peerAppData.limit(0);
@@ -390,7 +394,7 @@ private void doTasks() {
verifyResult = rubyCtx.getLastVerifyResult();
}
- private boolean flushData() throws IOException {
+ private boolean flushData() throws IOException {
try {
writeToChannel(netData);
} catch (IOException ioe) {
@@ -403,7 +407,7 @@ private boolean flushData() throws IOException {
return true;
}
}
-
+
private int writeToChannel(ByteBuffer buffer) throws IOException {
int totalWritten = 0;
while (buffer.hasRemaining()) {
@@ -476,7 +480,7 @@ private int readAndUnwrap(boolean blocking) throws IOException {
if(res.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.FINISHED) {
finishInitialHandshake();
}
- if(peerAppData.position() == 0 &&
+ if(peerAppData.position() == 0 &&
res.getStatus() == SSLEngineResult.Status.OK &&
peerNetData.hasRemaining()) {
res = engine.unwrap(peerNetData, peerAppData);
@@ -528,7 +532,7 @@ private IRubyObject do_sysread(ThreadContext context, IRubyObject[] args, boolea
Ruby runtime = context.runtime;
int len = RubyNumeric.fix2int(args[0]);
RubyString str = null;
-
+
if (args.length == 2 && !args[1].isNil()) {
str = args[1].convertToString();
} else {
@@ -582,7 +586,7 @@ private IRubyObject do_sysread(ThreadContext context, IRubyObject[] args, boolea
public IRubyObject sysread(ThreadContext context, IRubyObject[] args) {
return do_sysread(context, args, false);
}
-
+
@JRubyMethod(rest = true, required = 1, optional = 1)
public IRubyObject sysread_nonblock(ThreadContext context, IRubyObject[] args) {
return do_sysread(context, args, true);
@@ -747,7 +751,7 @@ public IRubyObject session_reused_p() {
public synchronized IRubyObject session_set(IRubyObject aSession) {
throw new UnsupportedOperationException();
}
-
+
private SocketChannel getSocketChannel() {
return (SocketChannel) io.getChannel();
}
Something went wrong with that request. Please try again.