Fix failure to initialise Netty channel when using :manual-ssl? option #752
Conversation
| @@ -641,7 +641,7 @@ | |||
| :handler handler | |||
| :server? true | |||
| :pipeline pipeline)] | |||
| (cond ssl? | |||
| (cond (and ssl? ssl-context) | |||
There was a problem hiding this comment.
While the fix would work, I think it's better if it's failing (maybe more gracefully with a clearer error message) because you don't expect a server to start without TLS when the ssl? parameter is passed with true.
There was a problem hiding this comment.
Note that ssl? isn't a parameter that the user can pass to aleph.http/start-server. Instead, it is derived from manual-ssl? and ssl-context.
FTR: I haven't yet reviewed the patch but intend to do so in the course of the week. Meanwhile, thanks a lot for your contribution, @scramjet! One thing I would like is to have a regression test for this but I can also take care of that if you don't have the bandwidth 🙏
There was a problem hiding this comment.
(Off-topic: At first I accidentally linked to a Jitsi meeting room there because I failed to copy the intended URL - fixed now. Please ignore the other URL in case you got it via email notification 😅)
There was a problem hiding this comment.
Thanks for looking at this.
While the fix would work, I think it's better if it's failing (maybe more gracefully with a clearer error message) because you don't expect a server to start without TLS when the
ssl?parameter is passed withtrue
:manual-ssl? = true indicates "I'm going to add the SSL handler(s) myself, trust me I know what I'm doing", and failing like it does breaks the ability to use it at all. As @DerGuteMoritz points out, ssl? is a handy internal flag to consult to see if SSL will be used at all.
I wish I had the time to write a test right now. In fact I wish I had made one in the original PR (#423) back when I had more time. Would have saved some grief now!
There was a problem hiding this comment.
OK I was finally able to look into this a bit deeper today and coded up a test case. And of course, it uncovered a problem right away 😅 Specifically, :manual-ssl? is somewhat at odds with HTTP/2: To determine whether to set up an HTTP/1 or HTTP/2 pipeline, we need ALPN which in turn requires TLS (h2s notwithstanding). However, custom pipeline transformations are only applied after the protocol negotiation has taken place - this is because they work differently depending on the chosen HTTP version (see here for the HTTP/1 options and here for HTTP/2; also note how :pipeline-transform is deprecated since 0.7.0). The way I see it, we have two choices now:
- Introduce a new pipeline transform option which is applied before ALPN (but what should it be called?) - could be useful for other purposes, too.
- Only support
:manual-ssl?for HTTP/1 and also skip the default ALPN handler when enabled.
Other ideas? Any opinions on which path to take?
There was a problem hiding this comment.
FWIW, here's an attempt at 1 in which I introduced a new :initial-pipeline-transform option which appears to do the trick. @scramjet Would you be able to give this patch a try to see if it also works in your case?
There was a problem hiding this comment.
Would you be able to give this patch a try to see if it also works in your case?
@DerGuteMoritz I've tested using :initial-pipeline-transform to set up SNI, and :http1-pipeline-transform for the rest of the transformations that used to be in :pipeline-transform, and all is good!
As well as fixing this problem, :initial-pipeline-transform seems like a generally useful and uncontroversial addition, so that would get my vote.
There was a problem hiding this comment.
@DerGuteMoritz I've tested using
:initial-pipeline-transformto set up SNI, and:http1-pipeline-transformfor the rest of the transformations that used to be in:pipeline-transform, and all is good!
Great, thanks for the quick feedback!
As well as fixing this problem,
:initial-pipeline-transformseems like a generally useful and uncontroversial addition, so that would get my vote.
Yep, having slept over it once more now, I agree. It merely fills the gap left by the deprecation of :pipeline-transform and it's pretty obvious what it does. I'll brush up the commit and open a follow-up PR for it then (GitHub doesn't allow me to push it to this PR alas ...). Will keep your original commit in there for attribution purposes 🙏
Fixes #751