Skip to content

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

Merged
DerGuteMoritz merged 7 commits intomasterfrom
run-http-ssl-tests-against-both-http-versions
Nov 23, 2023
Merged

Run all HTTP SSL tests against both HTTP versions, too#699
DerGuteMoritz merged 7 commits intomasterfrom
run-http-ssl-tests-against-both-http-versions

Conversation

@DerGuteMoritz
Copy link
Copy Markdown
Collaborator

@DerGuteMoritz DerGuteMoritz commented Nov 23, 2023

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 #698

@DerGuteMoritz DerGuteMoritz marked this pull request as draft November 23, 2023 10:53
@DerGuteMoritz DerGuteMoritz marked this pull request as ready for review November 23, 2023 10:54
@DerGuteMoritz DerGuteMoritz changed the title Draft: Run all HTTP SSL tests against both HTTP versions, too Run all HTTP SSL tests against both HTTP versions, too Nov 23, 2023
@DerGuteMoritz
Copy link
Copy Markdown
Collaborator Author

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
Copy link
Copy Markdown
Collaborator

KingMob commented Nov 23, 2023

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.

Base automatically changed from cleanup-http-tests to master November 23, 2023 14:36
@DerGuteMoritz
Copy link
Copy Markdown
Collaborator Author

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 😄). 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
Copy link
Copy Markdown
Collaborator Author

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.

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.
This makes `aleph.http.core/ring-request-ssl-session` work with HTTP/2 requests, too, since we no
longer have to rely on the HTTP/1-only NettyRequest's `ch` field. However, since for HTTP/2 the
`SslHandler` lives on the connection channel rather than the request's stream channel, we have to
change `channel-ssl-session` to recur the channel ancestor chain to find it now.
Should make its purpose a bit more self-explanatory.
@DerGuteMoritz DerGuteMoritz force-pushed the run-http-ssl-tests-against-both-http-versions branch from 82a9a2b to 74c93c8 Compare November 23, 2023 14:54
@DerGuteMoritz
Copy link
Copy Markdown
Collaborator Author

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!

Comment thread src/aleph/http/http2.clj Outdated
Comment thread src/aleph/netty.clj Outdated
Comment thread src/aleph/http/http2.clj Outdated
Copy link
Copy Markdown
Collaborator

@KingMob KingMob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me

…y introduced ones

Might later be refined e.g. by namespacing them with :aleph.internal/ instead. See
#700 for future reconsideration.
@DerGuteMoritz DerGuteMoritz merged commit 51f320d into master Nov 23, 2023
@DerGuteMoritz DerGuteMoritz deleted the run-http-ssl-tests-against-both-http-versions branch November 23, 2023 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants