yahns Ruby server user/dev discussion
 help / color / mirror / code / Atom feed
* [PATCH 0/3] TLS fixes
@ 2016-02-12  1:47 Eric Wong
  2016-02-12  1:47 ` [PATCH 1/3] acceptor: all subclasses of TCPServer use TCP_INFO Eric Wong
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Eric Wong @ 2016-02-12  1:47 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] acceptor: all subclasses of TCPServer use TCP_INFO
  2016-02-12  1:47 [PATCH 0/3] TLS fixes Eric Wong
@ 2016-02-12  1:47 ` Eric Wong
  2016-02-12  1:47 ` [PATCH 2/3] properly emulate sendfile for OpenSSL sockets Eric Wong
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-02-12  1:47 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/3] properly emulate sendfile for OpenSSL sockets
  2016-02-12  1:47 [PATCH 0/3] TLS fixes Eric Wong
  2016-02-12  1:47 ` [PATCH 1/3] acceptor: all subclasses of TCPServer use TCP_INFO Eric Wong
@ 2016-02-12  1:47 ` Eric Wong
  2016-02-12  1:47 ` [PATCH 3/3] avoid race conditions in OpenSSL::SSL::SSLContext#setup Eric Wong
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-02-12  1:47 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/3] avoid race conditions in OpenSSL::SSL::SSLContext#setup
  2016-02-12  1:47 [PATCH 0/3] TLS fixes Eric Wong
  2016-02-12  1:47 ` [PATCH 1/3] acceptor: all subclasses of TCPServer use TCP_INFO Eric Wong
  2016-02-12  1:47 ` [PATCH 2/3] properly emulate sendfile for OpenSSL sockets Eric Wong
@ 2016-02-12  1:47 ` Eric Wong
  2016-02-12  4:05 ` [PATCH 4/3] set HTTPS and rack.url_scheme in Rack env as appropriate Eric Wong
  2016-02-13 22:50 ` [PATCH 5/3] proxy_pass: pass X-Forwarded-Proto through Eric Wong
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-02-12  1:47 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 4/3] set HTTPS and rack.url_scheme in Rack env as appropriate
  2016-02-12  1:47 [PATCH 0/3] TLS fixes Eric Wong
                   ` (2 preceding siblings ...)
  2016-02-12  1:47 ` [PATCH 3/3] avoid race conditions in OpenSSL::SSL::SSLContext#setup Eric Wong
@ 2016-02-12  4:05 ` Eric Wong
  2016-02-13 22:50 ` [PATCH 5/3] proxy_pass: pass X-Forwarded-Proto through Eric Wong
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-02-12  4:05 UTC (permalink / raw)
  To: yahns-public

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 5/3] proxy_pass: pass X-Forwarded-Proto through
  2016-02-12  1:47 [PATCH 0/3] TLS fixes Eric Wong
                   ` (3 preceding siblings ...)
  2016-02-12  4:05 ` [PATCH 4/3] set HTTPS and rack.url_scheme in Rack env as appropriate Eric Wong
@ 2016-02-13 22:50 ` Eric Wong
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-02-13 22:50 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-02-13 22:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-12  1:47 [PATCH 0/3] TLS fixes Eric Wong
2016-02-12  1:47 ` [PATCH 1/3] acceptor: all subclasses of TCPServer use TCP_INFO Eric Wong
2016-02-12  1:47 ` [PATCH 2/3] properly emulate sendfile for OpenSSL sockets Eric Wong
2016-02-12  1:47 ` [PATCH 3/3] avoid race conditions in OpenSSL::SSL::SSLContext#setup Eric Wong
2016-02-12  4:05 ` [PATCH 4/3] set HTTPS and rack.url_scheme in Rack env as appropriate Eric Wong
2016-02-13 22:50 ` [PATCH 5/3] proxy_pass: pass X-Forwarded-Proto through Eric Wong

Code repositories for project(s) associated with this inbox:

	../../../yahns.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).