[WIP] Add SNI support to OpenSSL::SSL::Context::Server. by bmmcginty · Pull Request #5755 · crystal-lang/crystal

bmmcginty · 2018-02-27T10:26:28Z

I'm having GC errors storing sni certificates inside their parent. I've tried making each SNI certificate a WeakRef, but no luck.
I'd love suggestions, because I'm stumped.
Specs should cover all cases, with and without the client providing SNI, and with and without SNI hardfail being set.

jhass · 2018-02-27T10:28:41Z

+Oe8UMl5dCuO/efCPpBy46cYtUhZel0Z0Tak1OycE6M/2asWoz7kgc9P3hdy6v1nZ
+wrYe8DyQ3DImnL9JVsUJk7lxewlTzOEWA4AnuianF5WWI8XkhP6v0CQufLtvKPtG
+O4TUQWXQy7upOFZWvBdSsNiAU/DAlHakXJ4fhSM=
+-----END CERTIFICATE REQUEST-----


We don't need the CSR in the repo I think.

jhass · 2018-02-27T10:30:02Z

+    getter ctxbox
+    getter! sni
+
+    def add_sni(hostname : String, context : Server)


I would go for some slightly more verbose add_sni_hostname. Documentation would be great too, especially since having to pass a Context::Server to a Context::Server quickly gets confusing.

Also how about a variant that takes multiple hostnames? Since at least the bare and the www hostname in the same certificate is very common.

…csr to generation notes

ysbaddaden

Nice implementation! I noted a bunch of pedantic enhancements to make it perfect.

I have a question thought: why do we have to pass context to the add_sni_hostname methods and store it? Isn't the context always self, or does SNI means we can have multiple contexts? But then, maybe the context should default to self or nil (don't bother changing the SSL context)?

One last pedantic: maybe you don't need to box the context, it shall be kept alive long enough for GC to not garbage it, and you may just context.as(Void*) and ctx = data.as(Server) (if I'm not mistaken).

ysbaddaden · 2018-02-27T11:12:09Z

+
+    property sni_fail_hard
+    getter ctxbox
+    getter! sni


You can group accessors, type and documentation in a single definition, and @sni is an internal detail (the public API is #add_sni_hostname):

# :nodoc: getter! sni : Hash(String, Server) # ... property sni_fail_hard = false

Furthermore you shouldn't expose @ctxbox, it's an unsafe internal detail. It probably doesn't have to be typed either.

Got it. Where should I place developer notes? If I come back to this in six months, I want to know that, say, child contexts shouldn't have @sni set.

ysbaddaden · 2018-02-27T11:13:14Z

+    getter! sni
+
+    def add_sni_hostname(hostname : String, context : Server)
+      unless @sni


Maybe set_tlsext_servername_callback itself could start with return if @sni?

ysbaddaden · 2018-02-27T11:18:42Z

+      cb = ->(ssl : LibSSL::SSL, cmd : LibC::Int, ctxptr : Void*) {
+        ctx = Box(Server).unbox(ctxptr)
+        if ctx.sni_fail_hard
+          ret = LibSSL::SSL_TLSEXT_ERR_OK


I'm not sure I understand sni_fail_hard. If true then it will accept any hostname, which is weird.

Should OK and NOACK be reversed or should OK be something else?

fixed in latest push. I wasn't initializing the contexts correctly throughout the spec, and that coupled with the sni_fail_ahrd issue let tests pass. sni_fail_hard, if set, should immediately throw an error to any SSL client connecting (see any of cloudflares sites).

ysbaddaden · 2018-02-27T11:19:58Z

+                   else
+                     nil
+                   end
+        if hostname


Maybe avoid a double check:

if hostname_ptr hostname = String.new(hostname_ptr) # ... end

ysbaddaden · 2018-02-27T11:26:30Z

+    # ctx.add_sni_hostname(["example.com","*.example.com"],sniCtx)
+    # If you want to require clients to present SNI hostnames, enable sni_fail_hard.
+    # ctx.sni_fail_hard=true
+    # Now pass ctx to your OpenSSL server, and clients will be handed the correct certificate for the SNI hostname they pass in.


The documentation will be rendered as one big paragraph. A bit a markdown information would help.

I hope this is replying to the right comment. Regarding having self as the default context, that's what the current functionality does. SNI just allows different hostnames to be added, and with each new hostname comes a new context that the client will be attached to. I can reexplain this if I've botched the explanation.

ysbaddaden · 2018-02-27T11:27:53Z

+      sni[hostname] = context
+    end
+
+    def add_sni_hostnames(hostnames : Array(String), context : Server)


Maybe use Enumerable(String) instead of Array(String) so the method would accept more collection types.

Sija · 2018-02-27T11:54:50Z

+# run an ssl server with the supplied server context and port
+def run_server(q, port, tls = nil)
+  q.send "start"
+  tcpServer = TCPServer.new port


please, use snake_case (ditto all below) instead of camelCase.

Thanks. I reread the style guide again. Don't know where I got this idea from.

jhass · 2018-02-27T12:14:49Z

+                ret = LibSSL::SSL_TLSEXT_ERR_OK
+                break
+              end
+            end


I'm not sure these two branches should be the same interface, that is for an interface that asks me for hostnames explicitly I would expect the certificate to only be presented for that exact hostname.

On the other hand being able to just give a bunch of certificates and have the valid hostnames figured out from that is quite useful, so a separate interface for that does make sense to me. That should then clearly document that in case of certificates with overlapping hostnames, say you add one for example.org and example.com and one for example.org and example.net, the first one that matches win.

Finally I think mixing both approaches should also work but still a certificate that was added via the "figure the hostnames from it" interface shouldn't be presented for an explicitly given hostname, likewise an explicitly given hostname's certificate should always win over the implicit one if present.

Now this starts to accumulate quite some logic to the point where I start to wonder whether a Context::Server subclass makes sense, perhaps with some nice self yielding constructor so you can do something like

server.context = OpenSSL::SSL::Context::SniServer.new do |ctx| ctx.add_certifcate cert_a, key_a ctx.add_certificate "example.org", cert_b, key_b ctx.add_certificate "*.example.org", cert_c, key_c # explicit hostnames with wildcard matching probably warrants its own PR already ctx.add_certificate "example.net", some_context ctx.add_certificate another_context # not sure about that one, would need to validate the context has a certificate etc. end

I don't want to ask you to do all of that now and I am mostly brainstorming here. But if there's some general agreement along those thoughts we should maybe figure out what the minimal thing is we can do here without introducing weird behaviours that we have to break later.

…ontexts

…to add_sni_hostnames

bmmcginty · 2018-02-28T03:57:24Z

@jhass mentioned ordering. I need to add a note on ordering, at the very least. If someone adds a wildcard hostname, then a specific hostname, the specific is going to win. If someone adds a second duplicate hostname, should it be ignored, or raised?

jhass · 2018-02-28T09:20:11Z

I would raise or have the last added one win actually, with a slight but not strong tendency towards the latter. We might need to stop raising once we support adding certificates without explicit hostnames given, in case the certificates overlap. Of course such behaviour needs clear documenation too.

Sija · 2018-02-28T10:15:13Z

+        end
+        ret
+      }
+      unless @sni


Isn't this branch redundant with return if @sni above?

bmmcginty · 2018-03-01T22:56:40Z

I think I've handled all of the comments/reviews presented. I'm still having GC issues though, and I don't have a clue how to fix those. If I'm understanding what's happening correctly, the child contexts are never GC'd.

straight-shoota · 2020-12-09T21:05:48Z

Closing stale PR.

Thank you anyways.

jhass reviewed Feb 27, 2018

View reviewed changes

working SNI, but with errors from finalizer and memory consumption

a1b181c

jhass reviewed Feb 27, 2018

View reviewed changes

bmmcginty force-pushed the ssl_server_sni branch from d738c43 to a1b181c Compare February 27, 2018 10:31

bmmcginty added 3 commits February 27, 2018 05:36

change add_sni to add_sni_hostname, remove server.csr, add rm server.…

d0986cb

…csr to generation notes

add add_sni_hostnames call to support multiple hostnames per certificate

36b426a

add documentation to add_sni_hostname

7c481e2

ysbaddaden requested changes Feb 27, 2018

View reviewed changes

Sija reviewed Feb 27, 2018

View reviewed changes

jhass reviewed Feb 27, 2018

View reviewed changes

bmmcginty added 2 commits February 27, 2018 22:19

remove extra property, fix camelcase, fix specs to always use valid c…

de10c99

…ontexts

cleanup set_tlsext_servername_callback calling, fix getters, add doc …

6b7ac43

…to add_sni_hostnames

Make docs more readable (hopefully).

62fb2c5

Sija reviewed Feb 28, 2018

View reviewed changes

remove dead code branch

ce0e24e

allow get_client_hello to be raised for older SSL versions

6ea2e25

straight-shoota closed this Dec 9, 2020

Uh oh!

Conversation

bmmcginty commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhass Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ysbaddaden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhass Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmmcginty commented Feb 28, 2018

Uh oh!

jhass commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmmcginty commented Mar 1, 2018

Uh oh!

straight-shoota commented Dec 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jhass Feb 27, 2018 •

edited

Loading

jhass Feb 27, 2018 •

edited

Loading