[PATCH 0/3] TLS fixes

[PATCH 0/3] TLS fixes

[Eric Wong]

The big thing is sendfile(2) got mis-called when using TLS-wrapped
sockets.  This still needs to be documented better, but
https://yhbt.net/ is up-and-running for now.

Things like rack.url_scheme, SERVER_NAME, SERVER_PORT, etc...
will all need to be set properly.

 lib/yahns/acceptor.rb        |  2 +-
 lib/yahns/sendfile_compat.rb |  4 ----
 lib/yahns/server.rb          |  9 ++++++++-
 lib/yahns/wbuf_common.rb     |  1 +
 test/test_ssl.rb             | 25 ++++++++++++++++++++++++-
 5 files changed, 34 insertions(+), 7 deletions(-)

[PATCH 1/3] acceptor: all subclasses of TCPServer use TCP_INFO

[Eric Wong]

This will allow Yahns::OpenSSLServer instances to take advantage
of TCP_INFO under Linux, saving us the overhead of method
invocations.
---
 lib/yahns/acceptor.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/yahns/acceptor.rb b/lib/yahns/acceptor.rb
index 8650c50..7ab9f60 100644
--- a/lib/yahns/acceptor.rb
+++ b/lib/yahns/acceptor.rb
@@ -70,7 +70,7 @@ def spawn_acceptor(nr, logger, client_class)
   end
 
   def expire_mod
-    (Yahns::TCPServer === self && Yahns.const_defined?(:ClientExpireTCPI)) ?
+    (TCPServer === self && Yahns.const_defined?(:ClientExpireTCPI)) ?
      Yahns::ClientExpireTCPI : Yahns::ClientExpireGeneric
   end
 end
-- 
EW

[PATCH 2/3] properly emulate sendfile for OpenSSL sockets

[Eric Wong]

We cannot use the sendfile(2) syscall when serving static files
to TLS clients without breaking them.  We currently rely on
OpenSSL to encrypt the data before it hits the socket, so it
must be read into userspace buffers before being written to the
socket.
---
 lib/yahns/sendfile_compat.rb |  4 ----
 lib/yahns/wbuf_common.rb     |  1 +
 test/test_ssl.rb             | 25 ++++++++++++++++++++++++-
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/lib/yahns/sendfile_compat.rb b/lib/yahns/sendfile_compat.rb
index cdd2d7b..8bd4622 100644
--- a/lib/yahns/sendfile_compat.rb
+++ b/lib/yahns/sendfile_compat.rb
@@ -22,7 +22,3 @@ def trysendfile(io, offset, count)
     end while true
   end
 end
-
-class IO # :nodoc:
-  include Yahns::SendfileCompat
-end
diff --git a/lib/yahns/wbuf_common.rb b/lib/yahns/wbuf_common.rb
index 21e9b3a..c51050b 100644
--- a/lib/yahns/wbuf_common.rb
+++ b/lib/yahns/wbuf_common.rb
@@ -7,6 +7,7 @@
   require 'sendfile'
 rescue LoadError
   require_relative 'sendfile_compat'
+  IO.__send__ :include, Yahns::SendfileCompat
 end
 
 module Yahns::WbufCommon # :nodoc:
diff --git a/test/test_ssl.rb b/test/test_ssl.rb
index a8e3bea..172d8e4 100644
--- a/test/test_ssl.rb
+++ b/test/test_ssl.rb
@@ -64,9 +64,22 @@ def srv_ctx
   def test_ssl_basic
     err, cfg, host, port = @err, Yahns::Config.new, @srv.addr[3], @srv.addr[1]
     ctx = srv_ctx
+    raw = File.read(__FILE__)
     pid = mkserver(cfg) do
       cfg.instance_eval do
-        ru = lambda { |_| [ 200, {'Content-Length'=>'2'}, ['HI'] ] }
+        ru = lambda do |env|
+          case env['PATH_INFO']
+          when '/static'
+            f = File.open(__FILE__)
+            [ 200, {
+                'Content-Length' => f.size.to_s,
+                'Content-Type'=>'text/plain',
+              },
+              f ]
+          else
+            [ 200, {'Content-Length'=>'2'}, ['HI'] ]
+          end
+        end
         app(:rack, ru) { listen "#{host}:#{port}", ssl_ctx: ctx }
         logger(Logger.new(err.path))
       end
@@ -81,6 +94,16 @@ def test_ssl_basic
     assert_equal "HI", body
     assert_match %r{\AHTTP/1\.\d 200 OK\r\n}, head
 
+    # read static file
+    client.write("GET /static HTTP/1.1\r\nHost: example.com\r\n\r\n")
+    buf.clear
+    Timeout.timeout(60) do
+      buf << client.readpartial(8192) until buf.include?(raw)
+    end
+    head, body = buf.split("\r\n\r\n", 2)
+    assert_match %r{\AHTTP/1\.\d 200 OK\r\n}, head
+    assert_equal raw, body
+
     client.write("GET / HTTP/1.0\r\n\r\n")
     head, body = client.read.split("\r\n\r\n", 2)
     assert_equal "HI", body
-- 
EW

[PATCH 3/3] avoid race conditions in OpenSSL::SSL::SSLContext#setup

[Eric Wong]

By explicitly calling OpenSSL::SSL::SSLContext#setup before
accepting connections.  We cannot rely on "setup" being called
implicitly because any callbacks configured or objects
configured by the client may not be thread-safe.

We also avoid calling "setup" in the master process (if yahns is
configured to use worker processeses) in case the setup code
starts any TCP connections (e.g. to memcached for session
caching).
---
 lib/yahns/server.rb | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/yahns/server.rb b/lib/yahns/server.rb
index b7a7554..09ddbef 100644
--- a/lib/yahns/server.rb
+++ b/lib/yahns/server.rb
@@ -380,7 +380,14 @@ def fdmap_init
       ctx.queue = queues[qegg] ||= qegg_vivify(qegg, fdmap)
       ctx = ctx.dup
       ctx.__send__(:include, l.expire_mod)
-      ctx.__send__(:include, Yahns::OpenSSLClient) if opts[:ssl_ctx]
+      if ssl_ctx = opts[:ssl_ctx]
+        ctx.__send__(:include, Yahns::OpenSSLClient)
+
+        # call OpenSSL::SSL::SSLContext#setup explicitly here to detect
+        # errors and avoid race conditions.  We avoid calling this in the
+        # parent process since
+        ssl_ctx.setup
+      end
       ctx_list << ctx
       # acceptors feed the the queues
       l.spawn_acceptor(opts[:threads] || 1, @logger, ctx)
-- 
EW

[PATCH 4/3] set HTTPS and rack.url_scheme in Rack env as appropriate

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> Things like rack.url_scheme, SERVER_NAME, SERVER_PORT, etc...
> will all need to be set properly.

SERVER_NAME and SERVER_PORT will be set inside the unicorn HTTP
parser based on the value of the Host: header

-----------8<-----------
Subject: [PATCH] set HTTPS and rack.url_scheme in Rack env as appropriate

env['HTTPS'] is not documented in rack SPEC, but appears to be
used by Rack::Request since 2010[*].  Also, set rack.url_scheme
as documented by rack SPEC.

[*] - commit 4defbe5d7c07b3ba721ff34a8ff59fde480a4a9f
      ("Improves performance by lazy loading the session.")
---
 lib/yahns/http_context.rb |  2 +-
 lib/yahns/server.rb       |  3 +++
 test/test_ssl.rb          | 43 +++++++++++++++++++++++++++++++++++--------
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/lib/yahns/http_context.rb b/lib/yahns/http_context.rb
index 10be062..c02eefd 100644
--- a/lib/yahns/http_context.rb
+++ b/lib/yahns/http_context.rb
@@ -17,7 +17,7 @@ module Yahns::HttpContext # :nodoc:
   attr_accessor :qegg
   attr_accessor :queue # set right before spawning acceptors
   attr_reader :app
-  attr_reader :app_defaults
+  attr_accessor :app_defaults
   attr_writer :input_buffer_tmpdir
   attr_accessor :output_buffer_tmpdir
 
diff --git a/lib/yahns/server.rb b/lib/yahns/server.rb
index 09ddbef..d6a03f3 100644
--- a/lib/yahns/server.rb
+++ b/lib/yahns/server.rb
@@ -382,6 +382,9 @@ def fdmap_init
       ctx.__send__(:include, l.expire_mod)
       if ssl_ctx = opts[:ssl_ctx]
         ctx.__send__(:include, Yahns::OpenSSLClient)
+        env = ctx.app_defaults = ctx.app_defaults.dup
+        env['HTTPS'] = 'on' # undocumented, but Rack::Request uses this
+        env['rack.url_scheme'] = 'https'
 
         # call OpenSSL::SSL::SSLContext#setup explicitly here to detect
         # errors and avoid race conditions.  We avoid calling this in the
diff --git a/test/test_ssl.rb b/test/test_ssl.rb
index 172d8e4..fe7e09e 100644
--- a/test/test_ssl.rb
+++ b/test/test_ssl.rb
@@ -63,12 +63,21 @@ def srv_ctx
 
   def test_ssl_basic
     err, cfg, host, port = @err, Yahns::Config.new, @srv.addr[3], @srv.addr[1]
+    insecure = TCPServer.new(ENV["TEST_HOST"] || "127.0.0.1", 0)
     ctx = srv_ctx
     raw = File.read(__FILE__)
     pid = mkserver(cfg) do
+      ENV["YAHNS_FD"] += ",#{insecure.fileno.to_s}"
       cfg.instance_eval do
         ru = lambda do |env|
-          case env['PATH_INFO']
+          case path_info = env['PATH_INFO']
+          when '/rack.url_scheme', '/HTTPS'
+            s = env[path_info[1..-1]] # remove leading slash
+            s = s.inspect if s.nil?
+            [ 200, {
+                'Content-Length' => s.bytesize.to_s,
+                'Content-Type'=>'text/plain',
+              }, [ s ] ]
           when '/static'
             f = File.open(__FILE__)
             [ 200, {
@@ -80,19 +89,36 @@ def test_ssl_basic
             [ 200, {'Content-Length'=>'2'}, ['HI'] ]
           end
         end
-        app(:rack, ru) { listen "#{host}:#{port}", ssl_ctx: ctx }
+        app(:rack, ru) {
+          listen "#{host}:#{port}", ssl_ctx: ctx
+          listen "#{insecure.addr[3]}:#{insecure.addr[1]}"
+        }
         logger(Logger.new(err.path))
       end
     end
     client = ssl_client(host, port)
-    client.write("GET / HTTP/1.1\r\nHost: example.com\r\n\r\n")
     buf = ''.dup
-    Timeout.timeout(60) do
-      buf << client.readpartial(111) until buf =~ /HI\z/
+    { '/' => 'HI',
+      '/rack.url_scheme' => 'https',
+      '/HTTPS' => 'on'
+    }.each do |path, exp|
+      client.write("GET #{path} HTTP/1.1\r\nHost: example.com\r\n\r\n")
+      buf.clear
+      re = /#{Regexp.escape(exp)}\z/
+      Timeout.timeout(60) do
+        buf << client.readpartial(111) until buf =~ re
+      end
+      head, body = buf.split("\r\n\r\n", 2)
+      assert_equal exp, body
+      assert_match %r{\AHTTP/1\.\d 200 OK\r\n}, head
+    end
+
+    Net::HTTP.start(insecure.addr[3], insecure.addr[1]) do |h|
+      res = h.get('/rack.url_scheme')
+      assert_equal 'http', res.body
+      res = h.get('/HTTPS')
+      assert_equal 'nil', res.body
     end
-    head, body = buf.split("\r\n\r\n", 2)
-    assert_equal "HI", body
-    assert_match %r{\AHTTP/1\.\d 200 OK\r\n}, head
 
     # read static file
     client.write("GET /static HTTP/1.1\r\nHost: example.com\r\n\r\n")
@@ -109,6 +135,7 @@ def test_ssl_basic
     assert_equal "HI", body
     assert_match %r{\AHTTP/1\.\d 200 OK\r\n}, head
   ensure
+    insecure.close if insecure
     client.close if client
     quit_wait(pid)
   end
-- 
EW

[PATCH 5/3] proxy_pass: pass X-Forwarded-Proto through

[Eric Wong]

This allows backend application servers to set "rack.url_scheme"
as appropriate using Rack::Request#scheme.

Plack/PSGI users can also take advantage of this using
Plack::Middleware::ReverseProxy
---
  Eric Wong <e@80x24.org> wrote:
  > Things like rack.url_scheme, SERVER_NAME, SERVER_PORT, etc...
  > will all need to be set properly.

 lib/yahns/proxy_pass.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/yahns/proxy_pass.rb b/lib/yahns/proxy_pass.rb
index 3b68f01..511db02 100644
--- a/lib/yahns/proxy_pass.rb
+++ b/lib/yahns/proxy_pass.rb
@@ -226,6 +226,7 @@ def call(env)
     end
 
     req = "#{env['REQUEST_METHOD']} #{req} #{ver}\r\n" \
+          "X-Forwarded-Proto: #{env['rack.url_scheme']}\r\n" \
           "X-Forwarded-For: #{env["REMOTE_ADDR"]}\r\n".dup
 
     # pass most HTTP_* headers through as-is
-- 
EW