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

Set up ALPN configuration based on http-versions when possible #701

Closed DerGuteMoritz closed 7 months ago

DerGuteMoritz commented 7 months ago

This also adds an http-versions option to aleph.http/start-server. Its value will be used to automatically set up a matching ALPN config when ssl-context is a map and doesn't contain a :application-protocol-config key, yet. Support for the latter is retained so that users can still override the default in case any more interesting settings will become available in the future.

If an :application-protocol-config is present, we assert that the configured protocols match the desired HTTP versions. We need to have that validation code path anyway for the case when an io.netty.handler.ssl.SslContext instance is passed, which made the decision to retain the :application-protocol-config option easier.

Closes #696.

Some review notes:

DerGuteMoritz commented 7 months ago
  • Is it too strict to not allow more HTTP versions to be present in a custom ALPN config than specified via :http-versions? At first I thought so but then it would allow silly things like :http-versions [] or might lead to accidental misconfiguration.

Having thought about it some more, I think we should be even more strict. As you can see, we only check for HTTP protocols that we support in the ALPN config and ignore anything else. That means if a user enabled HTTP/3 for example, we would just let that pass. Clients could then negotiate HTTP/3 and run into an error right away because the server is not actually able to handle it! So in conclusion, I will change it to check that the protocols in the ALPN config match the exactly the desired HTTP versions.

KingMob commented 7 months ago

Generally speaking, if we can't eliminate potentially-conflicting options, we should throw errors when they conflict.

I added a test case which starts a server for :http2 without an ssl-context. I expected it to fail to start the server but it currently succeeds. Should we make this an error?

This should be an error unless use-h2c? is true.

Another test case enabled use-h2c? but also passes an ssl-context in which case the latter wins. Should this be an error? And if not, should use-h2c? perhaps trump ssl-context? The docstring words it as "set use-h2c? to force HTTP/2 cleartext" which somewhat implies it should.

Setting use-h2c? and passing an SslContext should also be an error. However, if use-h2c? is true, setting http-versions [:http1 :http2] should be OK, as long as there's no SslContext.

DerGuteMoritz commented 7 months ago
  1. Can you keep do on its own line?
  2. Can you add some whitespace between the testing macros, so it's not a wall of text?

Should both be taken care of, too!