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

Improved support for Server Name Indication (SNI) #349

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@ptoomey3
Copy link
Contributor

ptoomey3 commented Oct 18, 2012

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 Oct 17, 2012

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.
@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Oct 19, 2012

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

This comment has been minimized.

Copy link
Contributor Author

ptoomey3 commented Oct 19, 2012

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

This comment has been minimized.

Copy link
Member

BanzaiMan commented Oct 20, 2012

@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

This comment has been minimized.

Copy link
Member

headius commented Oct 27, 2012

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 Oct 27, 2012

@ptoomey3

This comment has been minimized.

Copy link
Contributor Author

ptoomey3 commented Oct 27, 2012

Hey
I submitted a patch to ruby-core, but I haven't heard anything back. I'll
check to see if I received any response this weekend.

eregon added a commit that referenced this pull request Dec 21, 2016

Squashed 'spec/ruby/' changes from 852254a..f4a4f0e
f4a4f0e Spec cloning of tainted frozen objects
8838ec2 Check that Hash can accept Bignum #hash values
1904cad Specify that Hash#compare_by_identity rehashes internally
ca082dc Fix typo
429285e Check that flatten on Array subclasses produces the right result
7169e98 Avoid race in abort_on_exception spec
1b569bd Add a spec for rescue clauses that combine a splatted list of exceptions with a literal list of exceptions.
d9e6b95 Add spec for passing an array to Kernel#warn
6ac5012 Add some sanity test for integers of different representations
10b3bf6 Simplify Kernel#lambda spec
1e363ce Add proper specs for Kernel#proc and Kernel#lambda
7662bab Remove bad Proc#hash spec
3ebc007 Fix Random#rand spec with a Bignum
c21f437 Use defined? in Logger fixtures to avoid issues with --unguarded
3e8a0a9 [Truffle] .bc rather than .ll now.
03e816c Add specs testing zsuper + multiple _ parameters
2bd9131 Also pass $HOME in the minimal environment in spawn :unsetenv_others specs
281e6e3 Allow Errno::EPIPE during WEBrick concurrent shutdown
19d01a7 Merge pull request #370 from odaira/myContribution
d49b52e Make paragraph mode consistent (#372)
f8fa622 AIX requires EXTDLDFLAGS to specify an entry point
e6b8bdc Revert "Maybe AIX requires LIBRUBYARG_SHARED?"
b6f3eae Skip specs using getsockopt, which is known to return a wrong length in AIX
65b29eb Maybe AIX requires LIBRUBYARG_SHARED?
c484d97 fix typo
e0ccea5 Run execerror to show detaled reason on LoadError
513164d Check raw data to show the error detail
944edc9 AIX 7.1 doesn't raise error
2a62f37 Fix for bigdecimal version 1.3.0.pre.2
8570c4a Fix for the change of the default rounding mode in ruby 2.4
5254fba Allow EOPNOTSUPP and EINVAL for File::TMPFILE spec
bdb599c Make the File::TMPFILE spec more robust and precise
8efa9e8 Fix PRN keyword argument tests
38a6a1e Add new behaviors of dup/clone
b271639 Do not rely on readline on echoing or not the input
99295fa Update ruby versions
5743edb Remove duplicate spec
9969daf Revert "Fix UDPSocket.new spec, Solaris reports a different error on unknown protocol family."
7fdb170 Merge pull request #349 from nobu/bug/readline-feature-fix
62747a0 Disable readline specs if rb-readline is used
15ed183 Add a guard for blocking readline specs on Windows
f0d5c14 Simplify Readline.readline spec by delegating to an external process
715b7d1 Fix TCPSocket#recv_nonblock spec
2a0bcb8 Add spec for StringIO#set_encoding on a frozen string
6064faa Use another example than UTF-8 for StringIO#external_encoding
e000de6 Spec the new output buffer argument for recv/recv_nonblock
600d305 Specify exception: false behavior on existing _nonblock specs
ed926452 Add spec for rb_timespec_now
4cb0765 Add spec for the new :flags keyword argument for File.open
a4002ea Fix portabililty bugs
15f050b Check readline feature by require

git-subtree-dir: spec/ruby
git-subtree-split: f4a4f0ec51c8cf60f75128e3aa825fba6ef6f3d0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.