search

clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Enable TLS endpoint identification by default (breaking) #689

Closed DerGuteMoritz closed 9 months ago

DerGuteMoritz commented 9 months ago

A few review notes:

  1. See commit message for overall intent
  2. I would have preferred to make the option part of :ssl-context instead having it sit next to it since conceptually, it belongs there. The reason for not doing this is that we also support passing in an SslContext object. Since Netty doesn't offer an API for setting an endpoint identification algorithm on the context, this would make it impossible to reach for users doing so, forcing them to convert their SslContext into an options map (which for some reason or another they might not be able to do). Feel free to disagree on this tradeoff, of course.
  3. In the tests, I had to give up on netty/self-signed-ssl-context because I needed to be able to reach the certificate to properly configure the client's trust store (which in turn allowed me to remove the :insecure? true). Unfortunately, SslContext has no API for that. I guess we could still keep netty/self-signed-ssl-context around since it's exposed as public API. However, perhaps we should also offer an arity for passing in a hostname now?
  4. Also in the tests you'll notice that I changed the default connection options to :http-versions [:http1]. This is because I wanted to override (only) the :trust-store option in :ssl-context which means I don't get the "good default" :application-protocol-config provided in aleph.http.client/client-ssl-context anymore. Perhaps we could lift that default a bit higher so that it also applies when passing in an :ssl-context map without :application-protocol-config?
KingMob commented 9 months ago
  • I would have preferred to make the option part of :ssl-context instead having it sit next to it since conceptually, it belongs there. The reason for not doing this is that we also support passing in an SslContext object. Since Netty doesn't offer an API for setting an endpoint identification algorithm on the context, this would make it impossible to reach for users doing so, forcing them to convert their SslContext into an options map (which for some reason or another they might not be able to do). Feel free to disagree on this tradeoff, of course.

I agree. It's not ideal, but it makes sense.

  • In the tests, I had to give up on netty/self-signed-ssl-context because I needed to be able to reach the certificate to properly configure the client's trust store (which in turn allowed me to remove the :insecure? true). Unfortunately, SslContext has no API for that. I guess we could still keep netty/self-signed-ssl-context around since it's exposed as public API. However, perhaps we should also offer an arity for passing in a hostname now?

What does "reach the certificate" mean here? You can access the certs in test/aleph/ssl.clj for testing purposes. Is that what you need?

We should definitely keep netty/self-signed-ssl-context around for users and testing. Especially with HTTP2, where TLS is on by default, and wyou have to jump through hoops not to use it.

I was also thinking about an arity for passing in a hostname. That would be nice.

KingMob commented 9 months ago
  • Also in the tests you'll notice that I changed the default connection options to :http-versions [:http1].

See my inline comments on this.

This is because I wanted to override (only) the :trust-store option in :ssl-context which means I don't get the "good default" :application-protocol-config provided in aleph.http.client/client-ssl-context anymore. Perhaps we could lift that default a bit higher so that it also applies when passing in an :ssl-context map without :application-protocol-config?

Feel free to lift that default ApplicationProtocolConfig out for reuse.

And adding ALPN when missing should be pretty harmless, afaict.

DerGuteMoritz commented 9 months ago

What does "reach the certificate" mean here? You can access the certs in test/aleph/ssl.clj for testing purposes. Is that what you need?

As in: extract the certificate from the SslContext object so that I can also use it in the client's :trust-store option. For the purpose of testing endpoint identifications, I couldn't use the certificates present in aleph.ssl as those are for the correct hostname (i.e. localhost). However, taking a closer look at aleph.ssl was a good idea (see my other comments).

We should definitely keep netty/self-signed-ssl-context around for users and testing. Especially with HTTP2, where TLS is on by default, and wyou have to jump through hoops not to use it.

I was also thinking about an arity for passing in a hostname. That would be nice.

OK, had that change still lying around anyway, pushed :slightly_smiling_face:

Feel free to lift that default ApplicationProtocolConfig out for reuse.

With the switch to aleph.ssl this ended up not being an issue anymore because client-ssl-context-opts has :application-protocol-config properly configured. Lifting the default for reuse would still be convenient but is now out of scope for this PR - will file a ticket!

DerGuteMoritz commented 9 months ago

Hm I think the failing test is a flake - it passes for me locally. It seems I am not allowed to trigger a retry, though!

DerGuteMoritz commented 9 months ago

Hm I think the failing test is a flake - it passes for me locally. It seems I am not allowed to trigger a retry, though!

Never mind, turns out I wasn't logged into CircleCI :facepalm:

KingMob commented 9 months ago

Hm I think the failing test is a flake - it passes for me locally. It seems I am not allowed to trigger a retry, though!

Never mind, turns out I wasn't logged into CircleCI 🤦

We ran into similar issues with pyr. CircleCI really wants people to log in...