Add OpenSSL::SSL::Server and HTTP::Server#bind_ssl #5960

straight-shoota · 2018-04-17T20:47:35Z

This adds a reusable SSLServer class which wraps another Socket::Server in an SSL layer.

With HTTP::Server#bind_ssl SSL context can be configured for every server socket individually instead of using the instance variable tls for all sockets.

Followup on #5776

RX14 · 2018-04-17T21:19:55Z

Doesn't this make requiring socket require openssl now? That's certainly something I don't like.

straight-shoota · 2018-04-17T21:30:08Z

Yes. But we can remove require "./socket/*" and make require "socket/ssl_server" opt-in. It needs to be included in http/server, though.

RX14 · 2018-04-17T22:13:17Z

If we already have OpenSSL::SSL::Socket (wraps IO), then surely this should be OpenSSL::SSL::Server (wraps Socket::Server)...

Which avoids the whole require issue entirely.

ysbaddaden · 2018-04-18T08:45:28Z

spec/std/http/server/server_spec.cr

@@ -1,5 +1,8 @@
 require "spec"
 require "http/server"
+{% unless flag?(:without_openssl) %}
+require "../../../support/ssl"
+{% end %}


Perhaps specs should always expect SSL?

Maybe... I don't care.

Well, I don't understand why specs should sometimes test something but sometimes shouldn't.

True. I don't know if there is any specific use case for speccing without openssl.
If removed, it should be removed from http client specs as well.

ysbaddaden · 2018-04-18T09:01:51Z

In Ruby this is OpenSSL::SSL::SSLServer but if we aim to abstract SSL implementations we should introduce something more generic. SSLServer was nicer than the ugly OpenSSL::SSL.

An SSLServer should create the underlying TCPServer, otherwise it's usefulness sounds quite limited (delegate everything but accept to wrap the accepted socket...). Are there any reason it shouldn't?

RX14 · 2018-04-18T09:51:08Z

We either abstract ssl from the openssl module or not. We currently don't, so we shouldn't change the design. We haven't even discussed a new design for tls.
Because you want to be able to do ssl over a unix sockets server. This class should definitely wrap. We can implement special constructors for tcp and unix servers that always return wrapped servers, but that's just constructors.

straight-shoota · 2018-04-18T12:37:44Z

I agree with @ysbaddaden that having SSLServer as an abstraction would be preferable, that's why I had it like this initially. But in the current state it seems to make sense to align the naming to the existing OpenSSL::SSL::Socket and refactor the entire design in a later PR.

On the other hand, there is a small conceptual difference, as OpenSSL::SSL::Socket really doesn't depend on Socket but actually works on any IO while the SSL server strictly depends on Socket::Server. But, at the moment I think we should keep it in the OpenSSL namespace.

It probably doesn't make much sense to to SSL over Unix sockets, but it should be flexible enough to be able to do that (or possibly any other Socket implementation).

But I could add some initializer overloads to match those of TCPServer and delegate to that to make using SSLServer easier.

RX14 · 2018-04-18T14:25:24Z

On the other hand, there is a small conceptual difference, as OpenSSL::SSL::Socket really doesn't depend on Socket but actually works on any IO while the SSL server strictly depends on Socket::Server.

This is simply because we have no abstraction for a "IO producer". If we did have one, SSL::Server should use it. I don't think that it's a particularly common abstraction though, so that difference is fine.

It probably doesn't make much sense to to SSL over Unix sockets

Yes it does - client certificates can be useful for a system daemon. It's not common though.

I'm fine with adding constructors to shorten OpenSSL::SSL::Server.new(TCPServer.new(...)) to OpenSSL::SSL::Server.tcp(...), but it's not a big deal either way.

This needs a rebase.

straight-shoota · 2018-04-18T15:35:24Z

Rebased

RX14

The socket library is a real mess, honestly.

RX14 · 2018-05-25T16:59:23Z

src/http/server.cr

@@ -260,7 +260,9 @@ class HTTP::Server
  end

  private def handle_client(io : IO)
-    io.sync = false
+    if io.is_a?(IO::Buffered)


This highlights some pretty bad design:

sync should always be false by default, it's currently not for sockets

sync should always be a property on IO, it should be ignored if the IO isn't buffered.

the previous means wrapper IOs can correctly pass through sync to their wrapped IOs.

RX14 · 2018-05-25T17:14:00Z

src/openssl/ssl/server.cr

+    end
+  end
+
+  def accept? : OpenSSL::SSL::Socket::Server?


We need to provide def accept too. In TCPServer this comes from Socket (which we don't inherit).

::Socket::Server also needs to gain the correct abstract defs.

RX14 · 2018-05-25T17:15:09Z

src/openssl/ssl/server.cr

+    @wrapped.close if @sync_close
+  end
+
+  delegate local_address, remote_address, to: @wrapped


If you're going to add these delegations you might as well just make wrapped a TCPSocket | UNIXSocket instead of a Socket::Server.

Actually, remote_address is incorrect, a server doesn't have one. Only sockets do.
But every server should have a local_address (see my proposal in #5778). So I think @wrapped's type should stay Socket::Server. And this way you can wrap SSL socket servers ;)

straight-shoota · 2018-06-22T20:13:58Z

Can this get a review?

RX14

This is a breaking change though, so should be merged for 0.26.0.

I still think we should have a OpenSSL::SSL::Server.tcp constructor.

RX14 · 2018-06-25T10:46:58Z

spec/support/ssl.cr

+
+def ssl_context_pair
+  server_context = OpenSSL::SSL::Context::Server.new
+  server_context.certificate_chain = File.join("spec", "std", "openssl", "ssl", "openssl.crt")


use datapath?

bcardiff · 2018-06-25T12:48:47Z

src/openssl/ssl/server.cr

+    begin
+      yield server
+    ensure
+      server.close


Maybe we should bubble exceptions in general. There might be a specific kind of exception that make sense to not bubble, but usually that is not the case. Other than that LGTM.

This does bubble exceptions, it's ensure not rescue?

Yup, I miss read it.

bcardiff · 2018-07-04T17:53:43Z

src/http/server.cr

+  # context.private_key = "openssl.key"
+  # server.bind_ssl "127.0.0.1", 8080, context
+  # ```
+  def bind_ssl(host : String, port : Int32, context : OpenSSL::SSL::Context::Server = OpenSSL::SSL::Context::Server.new, reuse_port : Bool = false) : Socket::IPAddress


Shouldn't bind_ssl methods be wrapped between {% unless flag?(:without_openssl) %}?
Otherwise there is a reference to a type that is not declared.

bcardiff

@straight-shoota I had two minor questions regarding docs and a request for a rebase of the PR so the CI can be happy before merging.

bcardiff · 2018-08-04T19:55:47Z

src/http/server.cr

+    # context.private_key = "openssl.key"
+    # server.bind_ssl "127.0.0.1", 8080, context
+    # ```
+    def bind_ssl(host : String, port : Int32, context : OpenSSL::SSL::Context::Server = OpenSSL::SSL::Context::Server.new, reuse_port : Bool = false) : Socket::IPAddress


Can the context be accessed and changed after bind_ssl is called?
Or does the default OpenSSL::SSL::Context::Server.new load some system information?

Otherwise I don't see why a default OpenSSL::SSL::Context::Server is defined.

Yeah, it probably doesn't make much sense. The context can't be accessed afterwards.

bcardiff · 2018-08-04T19:56:58Z

src/openssl/ssl/server.cr

@@ -0,0 +1,46 @@
+require "socket"
+
+class OpenSSL::SSL::Server


There is doc missing in the OpenSSL::SSL::Server, #initialize and .open. Or there is a :nodoc: missing.

bcardiff

Thanks!

bcardiff · 2018-08-05T02:17:14Z

@straight-shoota I would rather merge this without squashing, but since there are fixup! commits, should I ask you to rebase? I don't think that when commit-merging from GitHub directly the fixup will be applied when intended.

straight-shoota · 2018-08-05T08:16:04Z

Rebased and squashed 👍

straight-shoota mentioned this pull request Apr 17, 2018

Refactor HTTP::Server to bind to multiple addresses #5776

Merged

ysbaddaden reviewed Apr 18, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/http-server-ssl branch from 3f367f9 to 7f0045e Compare April 18, 2018 08:50

straight-shoota force-pushed the jm/feature/http-server-ssl branch from 7f0045e to f88c43b Compare April 18, 2018 12:48

straight-shoota force-pushed the jm/feature/http-server-ssl branch from f88c43b to a2c868b Compare April 18, 2018 15:25

straight-shoota changed the title ~~Add SSLServer and HTTP::Server#bind_ssl~~ Add OpenSSL::SSL::Server and HTTP::Server#bind_ssl Apr 22, 2018

RX14 requested changes May 25, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/http-server-ssl branch from a2c868b to 0527633 Compare June 5, 2018 09:40

straight-shoota force-pushed the jm/feature/http-server-ssl branch from 0527633 to 259c64e Compare June 22, 2018 20:10

RX14 approved these changes Jun 25, 2018

View reviewed changes

RX14 added kind:feature topic:stdlib breaking-change labels Jun 25, 2018

RX14 added this to the 0.26.0 milestone Jun 25, 2018

bcardiff reviewed Jun 25, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/http-server-ssl branch from 259c64e to b3d8a98 Compare June 27, 2018 08:03

bcardiff reviewed Jul 4, 2018

View reviewed changes

RX14 approved these changes Jul 9, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/http-server-ssl branch 2 times, most recently from f042096 to 5afb75b Compare July 9, 2018 13:24

bcardiff requested changes Aug 4, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/http-server-ssl branch from 5afb75b to 0fddf42 Compare August 4, 2018 21:02

bcardiff approved these changes Aug 4, 2018

View reviewed changes

straight-shoota added 3 commits August 5, 2018 10:15

Add abstract methods accept and accept? to Socket::Server

eeca6e1

Add OpenSSL::SSL::Server

0bc15c4

Replace HTTP::Server#tls with HTTP::Server#bind_ssl

a9e78f2

straight-shoota force-pushed the jm/feature/http-server-ssl branch from 0fddf42 to a9e78f2 Compare August 5, 2018 08:16

bcardiff merged commit 34e8aeb into crystal-lang:master Aug 6, 2018

straight-shoota deleted the jm/feature/http-server-ssl branch August 6, 2018 15:07

This was referenced Aug 6, 2018

Update to use HTTP::Server#bind_ssl kemalcr/kemal#477

Merged

Add HTTP::Server#bind(URI) #6500

Merged

bcardiff mentioned this pull request Aug 7, 2018

Update to crystal 0.26.0 kemalcr/kemal#479

Closed

ashmckenzie mentioned this pull request Aug 15, 2018

undefined method 'bind_ssl' for HTTP::Server kemalcr/kemal#482

Closed

miketheman mentioned this pull request Sep 6, 2018

No shared cipher #6534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenSSL::SSL::Server and HTTP::Server#bind_ssl #5960

Add OpenSSL::SSL::Server and HTTP::Server#bind_ssl #5960

straight-shoota commented Apr 17, 2018

RX14 commented Apr 17, 2018

straight-shoota commented Apr 17, 2018

RX14 commented Apr 17, 2018 •

edited

Loading

ysbaddaden Apr 18, 2018

straight-shoota Apr 18, 2018

ysbaddaden Apr 18, 2018

straight-shoota Apr 18, 2018

ysbaddaden commented Apr 18, 2018

RX14 commented Apr 18, 2018

straight-shoota commented Apr 18, 2018

RX14 commented Apr 18, 2018 •

edited

Loading

straight-shoota commented Apr 18, 2018

RX14 left a comment

RX14 May 25, 2018

RX14 May 25, 2018

RX14 May 25, 2018 •

edited

Loading

straight-shoota May 25, 2018 •

edited

Loading

straight-shoota commented Jun 22, 2018

RX14 left a comment

RX14 Jun 25, 2018

bcardiff Jun 25, 2018

RX14 Jun 25, 2018

bcardiff Jul 4, 2018

bcardiff Jul 4, 2018

bcardiff left a comment

bcardiff Aug 4, 2018

straight-shoota Aug 4, 2018

bcardiff Aug 4, 2018

bcardiff left a comment

bcardiff commented Aug 5, 2018

straight-shoota commented Aug 5, 2018

Add OpenSSL::SSL::Server and HTTP::Server#bind_ssl #5960

Add OpenSSL::SSL::Server and HTTP::Server#bind_ssl #5960

Conversation

straight-shoota commented Apr 17, 2018

RX14 commented Apr 17, 2018

straight-shoota commented Apr 17, 2018

RX14 commented Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ysbaddaden commented Apr 18, 2018

RX14 commented Apr 18, 2018

straight-shoota commented Apr 18, 2018

RX14 commented Apr 18, 2018 • edited Loading

straight-shoota commented Apr 18, 2018

RX14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 May 25, 2018 • edited Loading

Choose a reason for hiding this comment

straight-shoota May 25, 2018 • edited Loading

Choose a reason for hiding this comment

straight-shoota commented Jun 22, 2018

RX14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardiff left a comment

Choose a reason for hiding this comment

bcardiff commented Aug 5, 2018

straight-shoota commented Aug 5, 2018

RX14 commented Apr 17, 2018 •

edited

Loading

RX14 commented Apr 18, 2018 •

edited

Loading

RX14 May 25, 2018 •

edited

Loading

straight-shoota May 25, 2018 •

edited

Loading