Turn on TLS client certificate validation by default, expose OpenSSL::SSL::Context in HTTP::Client

[jhass]

Don't merge before #2671, includes redundant work that's better done there.

This is basically an alternative to #2343. Fixes #2073.

This does three things:

• Makes HTTP::Client#ssl a OpenSSL::SSL:Context::Client as well as all ssl options in HTTP::Client also take one.
• Turns on peer verification by default.
• Remove OpenSSL::SSL::Context.default, since it's not easy to copy a context, we should avoid global mutable defaults. More reasoning in the commit message.

This does not yet setup a strong cipher suite as #2671 does for the server side, which would fix #2674's second issue. We can look into what we can reuse from there in a follow up PR. It also does not yet set a sane default for enabled protocol versions, for largely the same reasons.

[jhass]

@ysbaddaden did you understand what this does in server mode? In particular I wonder if setting this in server mode will cause prompts in browsers etc.

SSL_VERIFY_PEER
Server mode: the server sends a client certificate request to the client. The certificate returned (if any) is checked. If the verification process
fails, the TLS/SSL handshake is immediately terminated with an alert message containing the reason for the verification failure.

[ysbaddaden]

Maybe for specific configurations where both the client and server share a certificate?

[jhass]

Oh no, it is for the server validating the certificate of the client during TLS client authentication. I just wonder if setting it already makes it mandatory or prompts the client to provide a certificate. The doc is clear that it does give the client the opportunity to send a certificate if this is set.

[ysbaddaden]

Maybe we should only set the verify mode from HTTP::Client (and other protocols)?

[jhass]

I do would like very much to centralize it, it's a good default to have it enabled. So first of all we should test with #2671 whether it's actually an issue at all.

If it is, how about a Context.new_for_server / Context.new_for_client?

[jhass]

Alright, the behavior in browsers is that it does prompt for a client certificate but does require it to proceed (one can cancel). I'll go with the constructor approach then.

[ysbaddaden]

Maybe Context.new(:client) and Context.new(:server) instead of distinct constructors?

[jhass]

That would be compile time vs runtime errors on typos, no?

[jhass]

Alright, rebased against #2671

[ysbaddaden]

Looks good overall.

I'm just not fond of the new_for_client and new_for_server naming. OpenSSL::SSL::Socket#new is passed a mode = :client | :server argument. Context could do the same, and the only difference between client and server is the verify mode value.

[ysbaddaden]

Apart from that: :+1+

[jhass]

the only difference between client and server is the verify mode value.

Yet, at some latter point we might want to configure different default ciphersuites and things like that there.

The issue is that passing the wrong thing to OpenSSL::SSL::Socket will break your program, it won't work, you'll notice. Passing the wrong thing to OpenSSL::SSL::Context, or worse using a potential default in the wrong situation, will make your program incorrect, it will still work but not verify certificates (client using server context) or unnecessarily prompt the user for a client certificate (server using client context). I would have no issue with unifying the APIs into the other direction though, OpenSSL::SSL::Socket.new_client, OpenSSL::SSL::Socket.new_server.

[ysbaddaden]

But does the compiler has any mean to detect that? Both cases (symbol or factories) return the same type and behave exactly the same for the compiler.

[jhass]

My point is that it's a lot harder easier to make the mistake with the symbol based API IMO, especially since unification with OpenSSL::SSL::Socket demands a default of :client.

[ysbaddaden]

We could make it mandatory?

@asterite what do you think? Do you have a better solution?

[asterite]

If an OpenSSL::SSL::Socket created as a client requires an OpenSSL::SSL::Context created as a client, we could maybe have subclasses of each (ClientSocket, ClientContext), and then to create a ClientSocket you would have to pass a ClientContext, and passing a ServerContext would not compile. Then Socket and Context would be abstract. That's the only way the compiler could help.

@jhass Sorry about without_openssl, once we find a way to make omnibus compile with openssl we'll remove that (and zlib too).

[jhass]

@asterite Yes that came to my mind too. Note that requires is not exactly right, it'll behave potentially unintentionally. The question however is, what's your opinion on the preferred route we should take here?

[asterite]

I see that new_for_client does:

context.verify_mode = OpenSSL::SSL::VerifyMode::PEER

Can the socket check this and raise a runtime error if creating a client socket and the context's verify mode is not peer? Or are there additional checks to do?

It this can be done, then it's simpler to introduce a class hierarchy. This error will be raised as soon as the code is used wrong, so it's almost as good as a compile-time check.

[jhass]

My current plans go into setting the default ciphersuite and enabled protocol versions there too, though likely for both server and client. But in the future we might want to have different defaults in those settings for server and client too.

We can't do sanity checks in the socket, as passing a context with those defaults changed to any other value is a perfectly valid usecase. It should just happen very explicitly and intentionally by the user.

[asterite]

So new_for_client doesn't add type safety, it just makes things more obvious to a reader/programmer that they are using the API correctly?

[jhass]

Yes, I'll toy a bit with a inheritance based approach, that already seems to cleanup SSL::Socket quite a bit. @ysbaddaden would you be okay with an inheritance based approach, before I push this too far?

[jhass]

Alright, that was easier than expected and I think I like it more. See the last commit for the changes, all others remained unchanged.

[jhass]

@asterite btw isn't having to cast here a bug?

[asterite]

I don't get any error if I leave that case out. What's a code to reproduce the bug?

[jhass]

Mmh, must have been a fluke in some intermediate state.

[asterite]

Unrelated, but this in master now fails:

require "http/client"

puts HTTP::Client.get("https://www.google.com").body

Maybe one of the previous changes to SSL broke this?

[asterite]

Seems to happen after merging #2676

[ysbaddaden]

After more thinking, I'm not sure OpenSSL::SSL::Context should assume anything about what will use the SSL context. Especially setting ciphers, but even setting the verify mode. This should be controlled by whatever will use the context, namely HTTP::Server, HTTP::Client, LDAP, IMAP...

I think there should be a single OpenSSL::SSL::Context.new constructor that merely calls SSL_CTX_new and then have specific factories like LDAP.default_ssl_context, HTTP::Server.default_ssl_context or HTTP::Client.default_ssl_context that will return a configured context for their own specific needs and requirements. That may end up with some repetition, but this may not be a bad thing.

👍 for distinguishing the Server and Client SSL sockets!

[jhass]

Sorry but I'm not following that thinking at all. Say I write an IRC client that supports connecting to TLS servers, I'll do socket = OpenSSL::Socket::Client.new(socket) if tls?, why shouldn't I get sane defaults? Why should I have to redo the work of defining a proper ciphersuite or scrap a default context from a seemingly completely unrelated domain like HTTP?

[jhass]

@ysbaddaden I would really like to see this in, as I'm not understanding it could you please elaborate your stance as to why the defaults for a generic encryption protocol, which TLS is, it's independent of the application protocol it encrypts, should be specific to said application protocol and not reside with the TLS implementation? Put differently, why does a HTTP server need different defaults than a IMAP4 server? Or a IRC client than a SIP client? Sure, the specific endpoints on the other side you want to support might need differing configurations, but that's the application developers concern and doesn't warrant to make finding good defaults a concern of the protocol library developer. What am I missing here that could allow us to find a compromise?

[ysbaddaden]

Indeed OpenSSL::SSL::Socket::Client.new(socket) (btw: it's getting quite long to type) should use good defaults.

Yet, I don't think it's a good idea to create an artificial difference between a client context and a server context when there isn't any in OpenSSL; and it would be bad to enforce a specific client or server context class at the SSL socket level, since it will necessarily come preconfigured, and one would have to reset everything before reconfiguring the context to its needs...

Ruby for example only sets the SSL_OP_ALL option on the context by default, and does nothing more. This is up to OpenSSL::SSL::SSLSocket and OpenSSL::SSL::SSLServer to setup the context (with provided default ciphers, modes and options).

Maybe I'm just picky, or I doesn't know SSL/TLS enough and I believe that different protocols may need different SSL/TLS modes, options, ciphers, ECDH / DH configurations, when there is no such thing.

What about:

• 👍 for OpenSSL::SSL::Socket::Client and OpenSSL::SSL::Socket::Server
• 👎 for OpenSSL::SSL::Context::Client and OpenSSL::SSL::Context::Server
• 🆗 for OpenSSL::SSL::Context.new_for_client and OpenSSL::SSL::Context.new_for_server (or OpenSSL::SSL.default_client_context?).

Sorry for being so stubborn 🙇

[jhass]

Indeed OpenSSL::SSL::Socket::Client.new(socket) (btw: it's getting quite long to type) should use good defaults.

And the current approach does that very cleanly by just instantiating the right context.

Maybe I'm just picky, or I doesn't know SSL/TLS enough and I believe that different protocols may need different SSL/TLS modes, options, ciphers, ECDH / DH configurations, when there is no such thing.

I use the same TLS config for all services on my server, HTTP, SMTP, IMAP4, POP3, Sieve, XMPP, probably something I forgot. I just can't imagine a reason as to why good defaults should differ.

The point is that it should be hard to undo the defaults, because we should consider any non default less secure, else we shouldn't pick it as a default.

Ruby for example only sets the SSL_OP_ALL option on the context by default, and does nothing more. This is up to OpenSSL::SSL::SSLSocket and OpenSSL::SSL::SSLServer to setup the context (with provided default ciphers, modes and options).

That approach makes it hard to get a context with good defaults and change just what you need. It means that either

context = OpenSSL::SSL::Context.new
context.certificate_chain = chain
context.private_key = key
client = HTTP::Client.new(host, tls: context)

will be less secure than

client = HTTP::Client.new(host)

Or that each and every user of OpenSSL:SSL::Socket has to pollute its interface with context specific options. See Net::HTTP#cert, Net::HTTP#cert_store and so on in Ruby.

Now going back to new_for_* would alleviate that pain somewhat, but there's still the risk from using an unconfigured context all too easily and in a non-obvious way, simply because you're not aware that .new returns bad defaults.

While Ruby has a quite extensive OpenSSL interface that's easy to map to the manpages, I think we can still vastly improve over it and it shouldn't be our single role model.

What about a OpenSSL::SSL::Context::Server.unconfigured / OpenSSL::SSL::Context::Client.unconfigured?

Sorry for being stubborn too, but to be honest I'm trying to come up with arguments for my position while your three arguments I could find so far, and that just might be my failure at reading(!), are:

• "OpenSSL does it this way" (to which I reply, it's often cited as a role model for a bad API and that doesn't mean we can't improve upon it).
• "Ruby does it this way" (to which I reply, only because it defers to OpenSSL for its interface design).
• "I have some feeling TLS consumers need different defaults but no facts to prove so." (Sorry for saying that so directly, I just want to make sure I don't miss something).

[jhass]

Oh and btw I agree on the long to type stuff, but I would like to keep that out of the discussion for this PR, adding convenience methods at other places is always something we can easily do later, but shouldn't influence the core design.

[ysbaddaden]

I use the same TLS config for all services on my server

Good to know!

Huum, what about making new private and have an insecure constructor? This way it's obvious the context is BAD by default.

class OpenSSL::SSL::Context
  DEFAULT_METHOD = LibSSL.sslv23_method
  DEFAULT_CIPHERS = %w( ... ).join(' ')
  DEFAULT_OPTIONS = LibSSL::Options::ALL | LibSSL::Options::NO_SSLV2 | LibSSL::Options::NO_SSLV3
  DEFAULT_MODES = LibSSL::Modes::AUTO_RETRY | LibSSL::Modes::RELEASE_BUFFERS

  private def initialize(method : LibSSL::Method)
  end

  # Returns a secure context configuration for client SSL sockets.
  #
  # The context is configured per the intermediate recommendation
  # from https://wiki.mozilla.org/Security/Server_Side_TLS with peer
  # certificate verification enabled.
  def self.client(method = DEFAULT_METHOD, host = nil)
  end

  # Returns a secure context configuration for server SSL sockets.
  #
  # The context is configured per the intermediate recommendation
  # from https://wiki.mozilla.org/Security/Server_Side_TLS
  def self.server(method = DEFAULT_METHOD)
  end

  # Returns a blank, insecure, context.
  #
  # A blank context is highly INSECURE by default and enables
  # deprecated and broken protocols and ciphers. The context MUST
  # be configured properly before use. Please consider the default
  # `client` or `server` contexts instead that are preconfigured
  # with better defaults.
  def self.insecure(method = DEFAULT_METHOD)
  end
end

[jhass]

That's where I was going with unconfigured and insecure is indeed a better name. However that still makes it easier to pass a context with server defaults (only relevant so far seems the default verify mode here) to a client and vice versa. I quite like that it's impossible with Context subclasses, but willing to settle on that as a compromise. Though tbh I don't understand why Context::Client.insecure/Context::Server.insecure is that much worse.

[ysbaddaden]

I'm willing to compromise too! All your changes are great, and we're almost at a good TLS integration in Crystal. Definitely more secure and less buggy!

I guess unconfigured reminded me of ActiveRecord's suppress method: why configure something to unconfigure it? I didn't get that it was the same than insecure: return a blank context. I'm not sure just a single configuration parameter is enough to create distinct types, especially with insecure around that shouldn't care about client or server, but maybe it's better? As long as it allows those use cases that are better off with blank contexts:

• We only supply the intermediate Mozilla TLS configuration which is good for a default, but I want the modern configuration which is stronger.
• I want to allow users of my application to configure the TLS context (like nginx or apache do).
• I want to develop a cipherscan (hey, why not?)

[jhass]

We only supply the intermediate Mozilla TLS configuration which is good for a default, but I want the modern configuration which is stronger.

# Server
context = OpenSSL::SSL::Context::Server.new # we still want SSLv2/3 disabled as a default
context.ciphers = MODERN_CIPHERS
Server.new(tls: context)
# Client
context = OpenSSL::SSL::Context::Client.new # we still want SSLv2/3 disabled as a default and peer verification enabled
context.ciphers = MODERN_CIPHERS
Client.new(tls: context)

I want to allow users of my application to configure the TLS context (like nginx or apache do).

# Server
# I also don't want to replicate good defaults in case my users didn't supply any configuration for an option
context = OpenSSL::SSL::Context::Server.new
context.ciphers = config.ciphers if config.ciphers?
context.verify_mode = config.verify_mode if config.verify_mode?
context.methods = config.methods if config.methods?
context.ca_certificates = config.ca_certificates if config.ca_certificates?
# .... you get the pattern
Server.new(tls: context)

# Client
# I also don't want to replicate good defaults in case my users didn't supply any configuration for an option
context = OpenSSL::SSL::Context::Client.new
context.ciphers = config.ciphers if config.ciphers?
context.verify_mode = config.verify_mode if config.verify_mode?
context.methods = config.methods if config.methods?
context.ca_certificates = config.ca_certificates if config.ca_certificates?
# .... you get the pattern
Client.new(tls: context)

I want to develop a cipherscan (hey, why not?)

# Only makes sense in the client role
CIPHERS.each do |cipher|
  context = OpenSSL::SSL::Context::Client.insecure
  # ...
  begin
    OpenSSL::SSL::Socket::Client.new(sock, context: context, sync_close: false)
  rescue e : OpenSSL::SSL::Error
    # ...
  end
end

Makes sense?

[jhass]

Added OpensSSL::SSL::Context::Client.insecure/OpensSSL::SSL::Context::Server.insecure

[ysbaddaden]

👌

Then all that's left is to remove HTTP::SSL_CIPHERS and HTTP::Server.default_ssl_context and to move the logic to both OpenSSL::SSL::Context classes, doesn't it?

[jhass]

Yes, I'd like to do that in a follow up PR, this is already big enough :)

[asterite]

I would avoid using allocate, as it basically bypasses some compiler type checks. How about an initialize method with a required named argument insecure? We would just ignore that parameter's value, it's just a marker to allow an extra overload.

[jhass]

I tried that but couldn't get it to work without getting public. Feel free to checkout the branch and try to find a way, the requirement is that the caller cannot pass it but has to use insecure and the context_spec.cr file still passes.

[asterite]

Hm, yeah, we'd need to duplicate that initialize in the subclasses. I tried that but I think I found a bug in the compiler, let's keep it like this for now :-)

[jhass]

So, ready from my side @asterite @ysbaddaden

[ysbaddaden]

Done!