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

Run all HTTP SSL tests against both HTTP versions, too #699

Closed DerGuteMoritz closed 7 months ago

DerGuteMoritz commented 7 months ago

To that end, introduce a new with-http-ssl-servers macro which works like with-http-servers but with SSL enabled for both versions. It also sets *use-tls* true by default.

This also fixes some tests which were passing http1-ssl-server-options to with-http-servers. This resulted in them running only against HTTP/1 (twice) because the :ssl-context of with-http2-server would get overridden.

With this change, test-ssl-session-access is failing in the HTTP/2 case. This is because it calls http.core/ring-request-ssl-session on the ring request. However, this fails because the HTTP/2 server implementation currently doesn't wrap the ring request in aleph.http.core/NettyRequest. @KingMob Can we just change it to do that or is there a good reason for not doing so?

Note: To avoid conflicts, this PR is based on https://github.com/clj-commons/aleph/pull/698

DerGuteMoritz commented 7 months ago

It also sets *use-tls* true by default.

Oh yeah, this variable got me confused for a moment since I thoguht it would enable TLS in the servers, too. How about renaming it to *use-tls-requests* or perhaps *use-tls-request-urls*?

KingMob commented 7 months ago

Unfortunately NettyRequest is based on Netty's HTTP1 classes. IIRC, those SSL tests were fragile because they exploited the underlying fields of NettyRequest, instead of just treating it like a map, which is why I made them HTTP1 for the time being.

I agree that functionality should be tested, though.

Can you merge #698, or do you need someone else to do it?

Also, are you trying one of the stack tools out? I noticed this is a branch off a branch.

DerGuteMoritz commented 7 months ago

Unfortunately NettyRequest is based on Netty's HTTP1 classes. IIRC, those SSL tests were fragile because they exploited the underlying fields of NettyRequest, instead of just treating it like a map, which is why I made them HTTP1 for the time being.

I see! How about exposing the channel via :aleph/netty-channel or so in the request map then?

Can you merge #698, or do you need someone else to do it?

Ah I thought you didn't already merge it because you wanted @arnaudgeiser to confirm it, too (which he did in the meantime :smile:). Merged now!

Also, are you trying one of the stack tools out? I noticed this is a branch off a branch.

Nope, in this case I just manually did it. And note what GH did after merging the other branch:

Base automatically changed from cleanup-http-tests to master 1 minute ago

Pretty good!

DerGuteMoritz commented 7 months ago

I see! How about exposing the channel via :aleph/netty-channel or so in the request map then?

Just realized there is precedence of exposing it via :aleph/channel in the stream metadata in aleph.tcp and aleph.udp as well as the websocket streams. Since we don't necessarily have a stream here and we already have precedence of adding such keys to the ring request map directly (e.g. :aleph/request-arrived), I suggest using :aleph/channel here too but in the request map.

DerGuteMoritz commented 7 months ago

How about renaming it to *use-tls-requests*

I suggest using :aleph/channel here too but in the request map.

Just pushed commits for both of these!