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.

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

feat: HTTP/3 support #3218

Merged
merged 4 commits into from Jul 17, 2023
Merged

feat: HTTP/3 support #3218

merged 4 commits into from Jul 17, 2023

Conversation

azurit
Copy link
Member

@azurit azurit commented May 20, 2023

Adding support for HTTP/3 protocol.

Fixes: #3246

@azurit azurit mentioned this pull request May 20, 2023
1 task
@Xhoenix
Copy link
Member

Xhoenix commented May 21, 2023

Just curious, are your servers running HTTP/3? 👀

@azurit
Copy link
Member Author

azurit commented May 21, 2023

All my servers are providing a production environment and HTTP/3 is still experimental technology, so no.

@Zoey2936
Copy link

Zoey2936 commented May 22, 2023

HTTP/3 is still experimental technology

Not sure, the rfc9000 is finished since May 2021 and nginx merged quic into theier default branch 3 days ago (https://hg.nginx.org/nginx/rev/235d482ef6bc), so it will probably be included in nginx v1.25, there is already documentation: https://nginx.org/en/docs/http/ngx_http_v3_module.html

@Zoey2936
Copy link

nginx published HTTP/3 today: https://nginx.org/en/CHANGES
can you already say, when this PR gets merged/finished (crs-setup.conf.example line 495/496 don't list HTPP/3)?

@azurit
Copy link
Member Author

azurit commented May 23, 2023

No but probably soon as it's really simple.

Copy link

@BurningDog BurningDog left a comment

Choose a reason for hiding this comment

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

I can confirm that these changes allow HTTP3 on my Caddy 2.6.4 setup using Coraza and this ruleset. Caddy has supported HTTP3 for ages - I only picked up the HTTP3 issue because 2 clients couldn't login on a test site (but it worked for me!).

Not making this as "approved" as I'm not sure about the test - it looks good to me, I just haven't run it.

@dune73
Copy link
Member

dune73 commented Jun 2, 2023

The CRS team is meeting on Monday. I've added this PR to the agenda. We could merge right away, but I think it worthwhile to discuss with the team since this is something of a big and official step.

Agenda: #3221

@RedXanadu
Copy link
Member

RedXanadu commented Jun 2, 2023

Is any client using HTTP/3.0? Was this ever a thing? Do we need it? Can we instead just use the official HTTP/3?

I understand that in the early days of HTTP/2, some early clients used HTTP/2.0 before the standardisation of HTTP/2, so 2.0 is needed for historical reasons, but do we need HTTP/3.0? Has that ever been a thing? It is not present in the RFC.

Apart from that query, this seems like a good change 🙂

@Zoey2936
Copy link

Zoey2936 commented Jun 2, 2023

Is any client using HTTP/3.0? Was this ever a thing? Do we need it? Can we instead just use the official HTTP/3?

firefox and chrome(-ium)

@RedXanadu
Copy link
Member

Is any client using HTTP/3.0? Was this ever a thing? Do we need it? Can we instead just use the official HTTP/3?

firefox and chrome(-ium)

Do you have a source for that? I see it as HTTP/3 not HTTP/3.0 in Firefox. Chromium I can't get an answer out of easily.

@azurit
Copy link
Member Author

azurit commented Jun 2, 2023

https://www.chromium.org/quic https://hacks.mozilla.org/2021/04/quic-and-http-3-support-now-in-firefox-nightly-and-beta

I think that @RedXanadu was NOT asking about which clients supports HTTP/3 protocol. He was asking if any of the clients is sending HTTP/3 instead of HTTP/3.0 as protocol identification during the communication with the server.

@RedXanadu
Copy link
Member

RedXanadu commented Jun 2, 2023

I tested this by building HAProxy and curl with HTTP/3.

Regardless of what the browsers report (they seem to prettify things and display HTTP/3 and HTTP/2), requests do indeed look like
GET / HTTP/3.0
on the server side, at least.

So I think I had it the wrong way around: 2.0 is correct and 2 was for legacy compatibility.

But, everything from HTTP/2 onwards doesn't have a real "request line" with an HTTP version number anyway, the request line is 'imaginary'…

So I'm still not sure if it's appropriate to accept both HTTP/3 and HTTP/3.0 😅 Maybe it depends on the server implementation and both need to be accepted, but I'm guessing at that.

@fzipi
Copy link
Member

fzipi commented Jun 5, 2023

So I'm still not sure if it's appropriate to accept both HTTP/3 and HTTP/3.0 😅 Maybe it depends on the server implementation and both need to be accepted, but I'm guessing at that.

I guess the RFC should be the SPOT on this.

@EsadCetiner
Copy link
Member

EsadCetiner commented Jun 27, 2023

a chained rule may need to be added to 920280 to check for HTTP/3, I'm currently testing QUIC with Nginx and ModSec and it looks like HTTP/3 requests don't contain the host header.
can anyone else reproduce?
edit: this might be a nginx specific issue, but I'd still like to make sure.

@jpds
Copy link

jpds commented Jul 1, 2023

The RFC states that the version is implicitly 3.0 and Caddy logs this as "proto":"HTTP/3.0" .

However it's probably the safest to simply accept both (as is already done with HTTP/2).

@EsadCetiner Caddy takes a host header for HTTP/3.

@jcchavezs
Copy link

Wondering the status of this.

@fzipi fzipi merged commit cdf3ebc into coreruleset:v4.0/dev Jul 17, 2023
5 checks passed
@fzipi
Copy link
Member

fzipi commented Jul 17, 2023

Merged after meeting decision.

@Zoey2936
Copy link

(crs-setup.conf.example line 495/496 don't list HTPP/3)

this is still wrong

@M4tteoP
Copy link
Member

M4tteoP commented Jul 18, 2023

Hey @Zoey2936, thanks for the heads up, I will follow up with a PR updating the rule description soon.

@fzipi
Copy link
Member

fzipi commented Jul 18, 2023

Thanks @Zoey2936 for your comment! Maybe next time you can add suggestions directly in the PR? That way it will prevent us from merging until all comments are solved 😄

@Zoey2936
Copy link

Thanks @Zoey2936 for your comment! Maybe next time you can add suggestions directly in the PR? That way it will prevent us from merging until all comments are solved 😄

yes

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.

Protocol enforcement blocks HTTP/3