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

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

Merged
merged 3 commits into from Aug 6, 2018

Conversation

Projects
None yet
4 participants
@straight-shoota
Contributor

straight-shoota commented Apr 17, 2018

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

This comment has been minimized.

Member

RX14 commented Apr 17, 2018

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

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Apr 17, 2018

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

This comment has been minimized.

Member

RX14 commented Apr 17, 2018

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.

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

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 18, 2018

Member

Perhaps specs should always expect SSL?

This comment has been minimized.

@straight-shoota

straight-shoota Apr 18, 2018

Contributor

Maybe... I don't care.

This comment has been minimized.

@ysbaddaden

ysbaddaden Apr 18, 2018

Member

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

This comment has been minimized.

@straight-shoota

straight-shoota Apr 18, 2018

Contributor

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.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-server-ssl branch from 3f367f9 to 7f0045e Apr 18, 2018

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Apr 18, 2018

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.

  1. 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

This comment has been minimized.

Member

RX14 commented Apr 18, 2018

  1. 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.

  2. 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

This comment has been minimized.

Contributor

straight-shoota commented Apr 18, 2018

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.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-server-ssl branch from 7f0045e to f88c43b Apr 18, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Apr 18, 2018

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 straight-shoota force-pushed the straight-shoota:jm/feature/http-server-ssl branch from f88c43b to a2c868b Apr 18, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Apr 18, 2018

Rebased

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

@RX14

The socket library is a real mess, honestly.

@@ -260,7 +260,9 @@ class HTTP::Server
end
private def handle_client(io : IO)
io.sync = false
if io.is_a?(IO::Buffered)

This comment has been minimized.

@RX14

RX14 May 25, 2018

Member

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.
end
end
def accept? : OpenSSL::SSL::Socket::Server?

This comment has been minimized.

@RX14

RX14 May 25, 2018

Member

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.

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

This comment has been minimized.

@RX14

RX14 May 25, 2018

Member

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

This comment has been minimized.

@straight-shoota

straight-shoota May 25, 2018

Contributor

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 straight-shoota force-pushed the straight-shoota:jm/feature/http-server-ssl branch from a2c868b to 0527633 Jun 5, 2018

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-server-ssl branch from 0527633 to 259c64e Jun 22, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jun 22, 2018

Can this get a review?

@RX14

RX14 approved these changes Jun 25, 2018

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.

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

This comment has been minimized.

@RX14

RX14 Jun 25, 2018

Member

use datapath?

begin
yield server
ensure
server.close

This comment has been minimized.

@bcardiff

bcardiff Jun 25, 2018

Member

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 comment has been minimized.

@RX14

RX14 Jun 25, 2018

Member

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

This comment has been minimized.

@bcardiff

bcardiff Jul 4, 2018

Member

Yup, I miss read it.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-server-ssl branch from 259c64e to b3d8a98 Jun 27, 2018

# 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

This comment has been minimized.

@bcardiff

bcardiff Jul 4, 2018

Member

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

@RX14

RX14 approved these changes Jul 9, 2018

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

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

# 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

This comment has been minimized.

@bcardiff

bcardiff Aug 4, 2018

Member

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.

This comment has been minimized.

@straight-shoota

straight-shoota Aug 4, 2018

Contributor

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

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

This comment has been minimized.

@bcardiff

bcardiff Aug 4, 2018

Member

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

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-server-ssl branch from 5afb75b to 0fddf42 Aug 4, 2018

@bcardiff

Thanks!

@bcardiff

This comment has been minimized.

Member

bcardiff commented Aug 5, 2018

@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 added some commits Jun 5, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 5, 2018

Rebased and squashed 👍

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/http-server-ssl branch from 0fddf42 to a9e78f2 Aug 5, 2018

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

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@straight-shoota straight-shoota deleted the straight-shoota:jm/feature/http-server-ssl branch Aug 6, 2018

@bcardiff bcardiff referenced this pull request Aug 7, 2018

Closed

Update to crystal 0.26.0 #479

@miketheman miketheman referenced this pull request Sep 6, 2018

Closed

No shared cipher #6534

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment