H2 Support

[kenichi]

this has been awhile in coming, but with the recent release of http-2 0.8.4, it is time to get this PR going and see if we can make reel officially support h2.

all input is very welcome.

[rfestag]

@kenichi: I pulled down this branch, but am having issues running the examples. I'm getting an SSL_VERSION_OR_CIPHER_MISMATCH error. I'm not entirely sure why, because I'm not overriding the SSL version (TLS 1.2), and I should be accepting the default cipher suite. Those same settings worked on my hacked version from https://github.com/igrigorik/http-2/issues/101.

I got ssldump working, and both are trying to use TLS 1.2. The weird thing is, if I print out the ciphers on the SSL context, the list of ciphers from Reel::H2 are a superset of those from my application (implying it should be able to negotiate it).

In my case, the working example negotiates cipher TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f). Interestingly, Firefox is sending a different cipher list when hitting my example vs. Reel, but both lists did contain 0xc02f. I modified the reel example to use my own self-signed certificates (we may want to add some fake self-signed certs to the repo for the example, or instructions on how to generate them manually), but still no joy.

[rfestag]

After a little more investigating, it seems like the SNI callback isn't being run when the hostname I hit matches a key in the sni parameter. I added some puts statements to the callback, and it only runs when I hit a host not in the list (localhost vs 127.0.0.1). I thought that callback had to be execute at some point in order to actually initialize certs/keys.

Does that sound right? Any thoughts as to why it wouldn't be called?

[rfestag]

Alright, so it looks like it is related to SNI. For whatever reason, the servername_cb is not being called when I try to hit a host listed in the sni hash (but it IS called when hit one that isn't...say, when I try localhost vs 127.0.0.1). Additionally, the constructor calls create_ssl_context without any arguments, meaning the default SSL context for the socket is basically invalid (no key/cert). I would assume that, if I wasn't having issues with SNI, this wouldn't be a huge deal.

In light of all of this, what is the reason for SNI being mandatory? Would it be reasonable to make the sni hash an optional parameter? When I passed the options hash to the create_ssl_context method, along with key/cert arguments, things seem to work as expected.

If we can make SNI optional, we would need to: 1) Make sni an optional named argument 2) Pass the options hash to the create_ssl_context method in the constructor. This would allow users who don't need SNI to just create a single context. 3) Do we need to memoize the context for each certificate? If it were being called as I would have expected, it seems like it will rebuild the context every time there is a new connection. That seems like a bit of waste (reading/parsing key/cert).

If SNI is mandatory, then we probably shouldn't fall back to the default socket context, because it can't be valid anyways with a key/cert. In that case, perhaps this is a better exception that can be thrown, making it more obvious to the system owner what is going on.

I can submit a PR with the changes if they actually make sense. They do work around the issue I'm having. That said, I haven't ever actually used SNI before, so I may be misunderstanding something.

[kenichi]

@rfestag thank you very much for trying this out and reporting your findings.

For whatever reason, the servername_cb is not being called when I try to hit a host listed in the sni hash (but it IS called when hit one that isn't...say, when I try localhost vs 127.0.0.1).

i have found that browsers and clients like nghttp do not set a servername on the request when the host part of the URL is an IP address instead of a domain name. if that is not set, servername_cb is not called on the server side.

i mistakenly have '127.0.0.1' in the SNI hashes in my examples. i will fix and update that. i was testing with my client, which i also mistakenly let set that value regardless of what is passed in.

what is the reason for SNI being mandatory? Would it be reasonable to make the sni hash an optional parameter?

you're correct: there should be a way to configure the default context for clients that don't set a servername or services that won't handle requests for more than one domain.

Pass the options hash to the create_ssl_context method in the constructor.

i like this suggestion!

Do we need to memoize the context for each certificate?

another good call.

I can submit a PR with the changes if they actually make sense.

thank you! in reviewing your comments and testing things, i've got a commit ready to go, so if you'd review that, i'd be very grateful.

[rfestag]

Great, everything seems to be working as expected, at least in Firefox. For some reason, Chrome doesn't like your fake certs. I was able to get the examples working with my own self-signed, so it may simply be because I created exemptions for my own fake certs. I don't think it is an issue with your code.

[kenichi]

@rfestag awesome. thank you so much. the certs in tmp/certs are created by https://github.com/celluloid/reel/blob/master/spec/support/create_certs.rb and there's a PR #237 to update it but i haven't looked too closely into what's going on. i generally use valid let's encrypt certs and test on a publicly accessible domain.

[rfestag]

I've been working on a Webmachine-ruby adapter for this, and it seems to be working. I haven't fully tested yet, but the only feature I'm seeing missing so far is the ability to do streaming responses. The HTTP1.x reel response supports standard string responses, IO, and enumerables (for chunked encoding). As I understand, there is no need for chunked encoding in HTTP2 because of the data frames, but I think supporting enumerables could be useful. I was thinking of implementing both IO and enumarble basically the same (IO would do a series of readpartial calls to create the data frames, while enumerable would send each element as a frame). Thoughts?

[kenichi]

@rfestag interesting idea, sorry for the late reply. i definitely think there is a case for that feature, also thinking about SSE over H2. i attempted to sort of copy the response interface from existing Reel::Response, but left that part out. i'm not super happy with either decision. i'm also wavering between wanting to get this merged before adding more or after.

@digitalextremist thoughts on this PR? reel's future in general? 😸

[kenichi]

Alright, I finally got some time to get back to this and got CI passing after rebase but had to switch which jruby versions were allowed to fail. @tarcieri @digitalextremist any thoughts? I know it's a big PR, would be happy to walk anyone through changes.

I realize that the general path forward in the community seems to be to use a reverse proxy that supports h2, and translates to h1 requests for the app service (nginx does this quite nicely). Also, 103 early hints is suggested as a way to make push promises work in this situation. However, I still think it would be cool to have Reel be the first ruby webserver to "natively" support H2.

Eventually, I'd like to update Angelo to have some H2 DSL support/helpers as well.

[rfestag]

I personally would like to see this as well.

[kenichi]

@tarcieri @digitalextremist with the latest release of puma supporting 103 early hints, and rails 5.2 embracing that as well, i can understand how interest in a ruby h2 server wanes.

i think i will close this PR and make this code into an "add-on" gem. if anyone has any input on the matter, speak up soon.

[kenichi]

https://github.com/kenichi/h2/pull/3 finally got around to it 😺