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

Unify HTTP/1 req->host and HTTP/2 :authority processing #1630

Closed
krizhanovsky opened this issue May 24, 2022 · 5 comments
Closed

Unify HTTP/1 req->host and HTTP/2 :authority processing #1630

krizhanovsky opened this issue May 24, 2022 · 5 comments
Assignees
Labels
good to start Start form this tasks if you're new in Tempesta FW h2 low priority
Milestone

Comments

@krizhanovsky
Copy link
Contributor

krizhanovsky commented May 24, 2022

At the moment we store URI authority part in req->host for HTTP/1, but we have a designated TFW_HTTP_HDR_H2_AUTHORITY special header for HTTP/2 for :authority. This complicates the logic (e.g. see #1629 (comment) discussion). Need to store URI authority for HTTP/1 in the special header, just as we do this for HTTP/2, and adjust all the places processing req->host.

No need any special testing - these changes are well covered by the existing test suite.

@krizhanovsky krizhanovsky added low priority good to start Start form this tasks if you're new in Tempesta FW labels May 24, 2022
@krizhanovsky krizhanovsky added this to the 0.8 - HTTP normalization milestone May 24, 2022
@const-t
Copy link
Contributor

const-t commented Aug 10, 2022

Current issue may contribute to #1667

@const-t
Copy link
Contributor

const-t commented Aug 25, 2022

Http2 pseudo-headers must be matched by HTTP Tables rules e.g hdr :authority == "test.com" -> test_host;. After unifying the processing logic need to ensure, that Authority(Authority from URI for HTTP), Host, Forwarded headers are processes in such order for H2 and H1.

Also we should to consider to rework our Host header parsing logic to satisfy this rule from RFC 7230 5.4:

If the target URI includes an authority component, then a client MUST send a field-value for Host that is identical to that authority component

@RomanBelozerov
Copy link
Contributor

RomanBelozerov commented Nov 8, 2022

If send h2 request with host: (empty), we will receive response status code - 400 and Warning: failed to parse request. #1705 hides warning.
But http 1.1 request will be processed successfully.

@krizhanovsky
Copy link
Contributor Author

krizhanovsky commented Apr 7, 2023

Please also fix https://github.com/tempesta-tech/tempesta/wiki/HTTP-tables

host - the host part from URI in HTTP request line, or the value of "Host" header field or value of "host" parameter from "Forwarded" header field. The Host part in URI takes priority over the Host header field value and "host" parameter from "Forwarded" header.

Which makes false statement about URI priority over Host header value and does not consider http2. See better description for http_host_required in https://github.com/tempesta-tech/tempesta/wiki/HTTP-security

Also please make Frang loaded by default to enforce default enabled http_host_required. Probably we need to update all frang tests for this.

@dmpetroff
Copy link
Contributor

Fixed in #1862

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good to start Start form this tasks if you're new in Tempesta FW h2 low priority
Projects
None yet
Development

No branches or pull requests

5 participants