Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 7 commits into from
Nov 23, 2023
Merged

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

merged 7 commits into from
Nov 23, 2023

Conversation

DerGuteMoritz
Copy link
Collaborator

@DerGuteMoritz DerGuteMoritz commented Nov 23, 2023 •
edited
Loading

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
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
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
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
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.

@DerGuteMoritz
Run all HTTP SSL tests against both HTTP versions, too
7309170 
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.
@DerGuteMoritz
Add :aleph/channel to server's ring request map
4fe4e63 
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.
@DerGuteMoritz
Rename *use-tls?* to *use-tls-requests?*
74c93c8 
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
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!

KingMob
KingMob reviewed Nov 23, 2023
View reviewed changes
src/aleph/http/http2.clj Outdated Show resolved Hide resolved
KingMob
KingMob reviewed Nov 23, 2023
View reviewed changes
src/aleph/netty.clj Outdated Show resolved Hide resolved
KingMob
KingMob reviewed Nov 23, 2023
View reviewed changes
src/aleph/http/http2.clj Outdated Show resolved Hide resolved
KingMob
KingMob approved these changes Nov 23, 2023
View reviewed changes
Copy link
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

@DerGuteMoritz
Move comment about :aleph/ keys being internal to only cover the newl…
0674f4f 
…y introduced ones

Might later be refined e.g. by namespacing them with :aleph.internal/ instead. See
#700 for future reconsideration.
@DerGuteMoritz DerGuteMoritz mentioned this pull request Nov 23, 2023
Open
@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
Reviewers

@KingMob KingMob KingMob approved these changes

@arnaudgeiser arnaudgeiser Awaiting requested review from arnaudgeiser arnaudgeiser is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DerGuteMoritz @KingMob