Added small hacks to work around tests disabled by #1630 #1862

dmpetroff · 2023-04-07T13:54:19Z

Current (27.04.2023) state of PR:
req->host aka picked_authority is selected from request data in the following priority:

from absoluteURI (HTTP/1.1 and earlier versions)
from :authority header (HTTP/2)
from Host header (all version)

req->host is now used for cache key calculation.

Hard-coded checks (check_authority_correctness()):

Host header MUST be present even when request comes in form of an absolute-URI (HTTP/1.1)
authority information MUST be supplied (ie non-empty) for HTTP/1.1 and HTTP/2

Frang check had been update accordingly:

:authority == Host (HTTP/2) or (host_from_absolute_uri == Host) (HTTP/1.1)
host_from_forwarded_header == picked_authority (both HTTP/1.1 and HTTP/2)

krizhanovsky

There are several questions and comments, which probably are good to be discussed

fw/http.c

fw/http_match.c

fw/http.c

fw/http.h

krizhanovsky

There are still things to fix and improve

fw/http.c

krizhanovsky · 2023-04-24T13:02:05Z

fw/http.c

+	return TFW_STR_EMPTY(&authority) || TFW_STR_EMPTY(&host)
+		|| tfw_strcmp(&authority, &host) == 0;
+}
+


Let's do these validations in extract_req_host():

tfw_http_req_process() is already too big

there is no sense to check the request version twice and spread the logic among many places (we already have the frang check)

fw/http.c

fw/http_match.c

fw/http.h

const-t

Please fix using TFW_HTTP_HDR_HOST in cache.c

krizhanovsky

There are still couple of not addressed comments and couple of a new comments to fix.

The logic looks good for me and I'd vote to fix it in the wiki (@const-t approve is required though).

fw/http.c

fw/http_limits.c

fw/http.c

const-t · 2023-04-28T13:43:19Z

fw/http_limits.c

+					  " from Host header",
+					&FRANG_ACC2CLI(ra)->addr, "\n");


&FRANG_ACC2CLI(ra)->addr not properly aligned. Also we may do not break string argument even if it wider than 80.

const-t · 2023-05-01T14:04:25Z

fw/http_limits.c

+				frang_msg("Request host from absolute URI differs"
+					  " from Host header",
+					&FRANG_ACC2CLI(ra)->addr, "\n");


Suggested change

frang_msg("Request host from absolute URI differs"

" from Host header",

&FRANG_ACC2CLI(ra)->addr, "\n");

frang_msg("Request host from absolute URI differs"

" from Host header",

&FRANG_ACC2CLI(ra)->addr, "\n");

krizhanovsky

Just couple of unresolved comments from the previous review.

fw/http.c

krizhanovsky · 2023-05-04T18:50:58Z

etc/tempesta_fw.conf

@@ -1025,7 +1025,7 @@
 #       http_header_cnt NUM;
 #       http_header_chunk_cnt NUM;
 #       http_body_chunk_cnt NUM;
-#       http_host_required true|false;
+#       http_strict_host_checking true|false;


Please fix all the Wiki docs by

tempesta.wiki$ grep http_host_required * HTTP-security.md:`http_host_required [true|false]` HTTP-security.md~:`http_host_required [true|false]` HTTP-security.md.orig:`http_host_required [true|false]` grep: scripts: Is a directory grep: _static: Is a directory grep: _templates: Is a directory Vhost-Confusion.md: http_host_required; Virtual-hosts-and-locations.md:* http_host_required

const-t

LGTM

const-t · 2023-05-05T10:46:29Z

fw/http_limits.c

+		 * HTTP/2 because it cannot have an absolute URI.
+		 * Also this MUST be removed after #1870 is complete*/
+		if (test_bit(TFW_HTTP_B_ABSOLUTE_URI, req->flags)) {
+			TfwStr host;


Please add new line after host declaration.

krizhanovsky

Just a minor cleanup

fw/http.c

krizhanovsky

LGTM

- fixed panic when host/authority headers are empty - moved absolute uri/host headers equality check to frang

dmpetroff requested review from krizhanovsky and const-t April 7, 2023 13:54

krizhanovsky marked this pull request as draft April 7, 2023 15:38

dmpetroff force-pushed the dp-1630-h2-host-processing branch from 0453105 to de8d891 Compare April 11, 2023 14:23

dmpetroff marked this pull request as ready for review April 11, 2023 15:37

dmpetroff force-pushed the dp-1630-h2-host-processing branch 2 times, most recently from bc27332 to a54930c Compare April 14, 2023 11:49

krizhanovsky requested changes Apr 18, 2023

View reviewed changes

fw/http.c Outdated Show resolved Hide resolved

fw/http.c Outdated Show resolved Hide resolved

fw/http_match.c Outdated Show resolved Hide resolved

fw/http.c Show resolved Hide resolved

fw/http.h Outdated Show resolved Hide resolved

dmpetroff requested a review from krizhanovsky April 20, 2023 08:27

dmpetroff force-pushed the dp-1630-h2-host-processing branch from 60b42f0 to f4e58d3 Compare April 20, 2023 13:22

dmpetroff mentioned this pull request Apr 24, 2023

Fix tests disabled after PR 1862 tempesta-tech/tempesta-test#441

Closed

dmpetroff closed this Apr 24, 2023

dmpetroff reopened this Apr 24, 2023

krizhanovsky requested changes Apr 24, 2023

View reviewed changes

const-t requested changes Apr 26, 2023

View reviewed changes

dmpetroff requested review from krizhanovsky and const-t April 27, 2023 07:47

krizhanovsky requested changes Apr 27, 2023

View reviewed changes

fw/http.c Outdated Show resolved Hide resolved

fw/http_limits.c Show resolved Hide resolved

fw/http.c Show resolved Hide resolved

dmpetroff force-pushed the dp-1630-h2-host-processing branch from e722ea3 to 5dde7e8 Compare April 28, 2023 13:22

const-t reviewed Apr 28, 2023

View reviewed changes

const-t reviewed May 1, 2023

View reviewed changes

dmpetroff requested review from const-t and krizhanovsky May 3, 2023 11:14

RomanBelozerov mentioned this pull request May 4, 2023

host parsing tests and fixed tests for PR 1862 tempesta-tech/tempesta-test#446

Merged

krizhanovsky reviewed May 4, 2023

View reviewed changes

const-t approved these changes May 5, 2023

View reviewed changes

krizhanovsky reviewed May 5, 2023

View reviewed changes

fw/http.c Outdated Show resolved Hide resolved

krizhanovsky approved these changes May 5, 2023

View reviewed changes

dmpetroff force-pushed the dp-1630-h2-host-processing branch from ba0d303 to 03f9d96 Compare May 10, 2023 09:32

Second attempt on req->host changes

60ce8ee

Dmitry Petrov added 22 commits May 10, 2023 17:17

WIP: trying to find appropriate place to set req->host

64b843f

Fixed storing HTTP/1.x Host header into req->host

13bed82

Fix http tables matching

c88bf17

Fixes related to host name selection in HTTP tables

be81437

Fix tempesta sending RST in response to empty authority request

53f4865

Review fixes #1

c722231

Cite RFC 9112 instead of obsolete RFC 7230

d233cdb

Enforce authority constraints for HTTP/1.1 and HTTP/2

b906b87

Fix error in validate_authority_rfc9113()

9e3cf23

Remove rfc-related authority checks from frang

c246ad8

Make bit flag from req->uri_host.

6003bc2

Multiple fixes.

94ca2a8

- fixed panic when host/authority headers are empty - moved absolute uri/host headers equality check to frang

Fine-tune frang/hardcoded authority correctness validation

1ba6bfa

Use req->host for cache key calculation.

5f1ba18

Fix H2 authority check condition

57d118c

Fix missing braces in frang which caused a lot of bugs

b21e7e5

Fix one more place to use req->host rather than HDR_HOST

ed3da9b

Rename http_host_required -> http_strict_host_checking

056c8f8

Change response in case of invalid authority.

850e0a2

Review fixes #2

a171701

Remove duplicating authority checks from tfw_h2_adjust_req

49fc35e

Codestyle tuning

ea83be6

dmpetroff force-pushed the dp-1630-h2-host-processing branch from eb3c9b9 to ea83be6 Compare May 10, 2023 14:17

Fix compilation error

f05e5c1

dmpetroff merged commit 7670e87 into master May 10, 2023

dmpetroff deleted the dp-1630-h2-host-processing branch May 10, 2023 16:27

This was referenced May 11, 2023

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

Closed

Wrong cached response with fake host #1667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added small hacks to work around tests disabled by #1630 #1862

Added small hacks to work around tests disabled by #1630 #1862

dmpetroff commented Apr 7, 2023 •

edited

Loading

krizhanovsky left a comment

krizhanovsky left a comment

krizhanovsky Apr 24, 2023

const-t left a comment

krizhanovsky left a comment

const-t Apr 28, 2023

const-t May 1, 2023

krizhanovsky left a comment

krizhanovsky May 4, 2023

const-t left a comment

const-t May 5, 2023

krizhanovsky left a comment

krizhanovsky left a comment

Added small hacks to work around tests disabled by #1630 #1862

Added small hacks to work around tests disabled by #1630 #1862

Conversation

dmpetroff commented Apr 7, 2023 • edited Loading

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky Apr 24, 2023

Choose a reason for hiding this comment

const-t left a comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

const-t Apr 28, 2023

Choose a reason for hiding this comment

const-t May 1, 2023

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky May 4, 2023

Choose a reason for hiding this comment

const-t left a comment

Choose a reason for hiding this comment

const-t May 5, 2023

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

dmpetroff commented Apr 7, 2023 •

edited

Loading