Clojurians Log v2

I see remarks like: > This is not a vulnerability. It’s just about properly using the Netty and JSSE APIs. Should be flagged as a false positive in your vuln scan tool. (https://github.com/netty/netty/issues/8537#issuecomment-1527896917) I'm hoping somebody who knows the details of Aleph better than myself can judge whether this is applicable.

Aleph doesn't do anything different as far as security goes. It hands all of that off to Netty. If it's a config issue with Netty, the same is probably true for Aleph. If it's a real vuln in Netty, it's probably a real vuln in Aleph. FWIW, there's the the manual-ssl? param, but I've never personally used it.

I also looked into this a bit yesterday. The issue you linked to is about enabling hostname verification by default in Netty itself which would indeed be the reasonable (and safe) thing to do. I don't quite get the comment you linked to in that context. It's akin to accepting self-signed certificates by default and requiring users to enable CA checks. I would consider this a vulnerability, too. During research I also found that https://github.com/julianladisch/vert.x/commit/8b2eef4f47301cfca8238ca4f4456691cb142ced. I think we could/should do the same in Aleph and provide an option for reverting back to the old (unsafe) behavior. I have a workaround in our codebase to enable it via :pipeline-transform which should be pretty easy to port to Aleph itself. If you agree, I will prepare a PR!

Thanks @dergutemoritz for looking into this! It sounds to me like a good idea to make a PR, and I would also be very grateful if you could share some of the setup code that you are using, so that we can do the same!

(But the PR thing is not my call to make 😉 )

(Just a heads-up, pipeline-transform can't work the same way in http2 (since there's now 1+N pipelines), so we need to come up with new params. See the HTTP/2 PR)

@kingmob Ah is this also the case for the client?

Yes. It's the nature of http2 multiplexing. I think pipeline-transform is going to be one of the few truly breaking changes, and I haven't figured out a good way around it.

Right, makes sense that it would affect both ends, of course

Anyway, when making it part of Aleph proper, we won't have to go through :pipeline-transform, of course

@stefan.van.den.oord snippet coming up!

Netty handles http2 multiplexing by creating 1 connection-level channel, and then multiple stream-level channels for each HTTP2 stream. Sometimes you'd wat pipeline-transform to alter the conn pipeline, other times you'd want it to alter the stream pipelines, and sometimes both! There's no way to know which without knowing what the transform fn wants to do

Right, and the SslHandler is of course a connection-level thing. Will try to think about this, too and get back to it on the new PR!

@kingmob WDYT about enabling hostname verification by default in Aleph?

The good news is pipeline-transform usage is rare, and anyone doing it should already be prepared to muck around with netty

> WDYT about enabling hostname verification by default in Aleph? Seems reasonable, but we should look into it first

@stefan.van.den.oord https://gist.github.com/DerGuteMoritz/da01ff0bcb0611fad2a3562a81dc1750

@kingmob Anything in particular you'd like me to look into beyond what I already did? Or do you want to familiarize yourself with the issue a bit more first?

If it's as simple as the gist, we don't have to worry about maintenance impact or prioritization. Only real question is whether it will break legitimate stuff. Of course, we'll need a flag to disable it. (I already know I may want to turn it off so I can MITM myself when debugging with Wireshark)

Maybe create a GH issue? It's not a priority at the moment, but we don't want to forget

Yup, it is a breaking change for sure

Aye, will file one

Question is, how much breaking?

Assuming that people have usually set up their TLS certificates properly, I would think not too much

Thanks @dergutemoritz I really appreciate that!

YW!

My suspicion is there'll be some internal use with out-of-date certs with old names, and deploying this change will break that

Yep, agreed

(gotta run, brb)

OK finally got around to it: https://github.com/clj-commons/aleph/issues/688

@dergutemoritz This should also work server-side then, right? Like this:

(reset! server (http/start-server (handler env)
                                  {:port port
                                   :pipeline-transform
                                   (fn [^ChannelPipeline pipeline]
                                       (when-let [ssl-engine (-> pipeline ^SslHandler (.get SslHandler) .engine)]
                                                 (let [ssl-params (.getSSLParameters ssl-engine)]
                                                      (.setEndpointIdentificationAlgorithm ssl-params "HTTPS")
                                                      (.setSSLParameters ssl-engine ssl-params))))}))

@stefan.van.den.oord That's not necessary, this only affects the client's behavior. If you're using client certificates, there is no standard way of verifying their CN on the server-side, that would be part of your application logic.

(at least in the context of HTTPS; there might be other standards which I'm not aware of)

Clear, thank you 🙏

@stefan.van.den.oord Headsup - I just updated the snippet, there was a small bug in there which made it unfit for use with non-HTTPS requests

@dergutemoritz Thank you so much for letting me now! I updated my notes 🙂