H/2: discard Host header when :authority is present by yanavlasov · Pull Request #30005 · envoyproxy/envoy

yanavlasov · 2023-10-06T19:07:42Z

Commit Message:
Discard the Host header if the :authority header was received to bring Envoy into compliance with https://www.rfc-editor.org/rfc/rfc9113#section-8.3.1

This behavioral change can be reverted by setting runtime flag envoy.reloadable_features.http2_discard_host_header to false.

Risk Level: Low
Testing: Unit Tests
Fixes #31118
Docs Changes: N/A
Release Notes: Yes
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.http2_discard_host_header

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only · 2023-10-06T19:07:48Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #30005 was opened by yanavlasov.

see: more, trace.

adisuissa

Code looks good!
I've got a high-level question about the desired behavior.

adisuissa · 2023-10-06T20:33:33Z

+      // Check if there is already the :authority header
+      auto result = stream->headers().get(Http::Headers::get().Host);
+      if (!result.empty()) {
+        // Discard the host header value


My understanding of https://www.rfc-editor.org/rfc/rfc9113#section-8.3.1 is that discarding host is not necessary, but it must have the same value as :authority. Should the discarding be done only if the host differs from :authority?

Here are the statements related to the server:

The recipient of an HTTP/2 request MUST NOT use the Host header field to determine the target URI if ":authority" is present.

A server SHOULD treat a request as malformed if it contains a Host header field that identifies an entity that differs from the entity in the ":authority" pseudo-header field. (Envoy does not need to reject request with Host different from :authority)

An intermediary that forwards a request over HTTP/2 MAY retain any Host header field. (Envoy does not need to retain the Host if it was present).

Note that Envoy can not have both :authority and Host headers in the header map, they are treated as one and the same. As a result discarding the Host header is compliant with RFC.

You got me convinced. I agree it makes sense to remove the header and protect it with a runtime flag.

adisuissa · 2023-10-06T20:33:51Z

+    StreamImpl* stream = getStreamUnchecked(frame->hd.stream_id);
+    if (stream && name == static_cast<absl::string_view>(Http::Headers::get().HostLegacy)) {
+      // Check if there is already the :authority header
+      auto result = stream->headers().get(Http::Headers::get().Host);


jmarantz · 2023-10-12T13:25:11Z

I think this deserves a cross-company review

zuercher · 2023-10-12T17:26:35Z

+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_discard_host_header")) {
+    StreamImpl* stream = getStreamUnchecked(frame->hd.stream_id);
+    if (stream && name == static_cast<absl::string_view>(Http::Headers::get().HostLegacy)) {
+      // Check if there is already the :authority header


The http2 spec says that :authority must come before regular header fields and that reversing the order should be considered a malformed request. As long as the h2 parser actually rejects the reverse order, this looks good.

This is correct. I have added a test to validate this.

soulxu · 2023-10-13T07:20:45Z

  ASSERT(frame->headers.cat == NGHTTP2_HCAT_REQUEST || frame->headers.cat == NGHTTP2_HCAT_HEADERS);
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http2_discard_host_header")) {
+    StreamImpl* stream = getStreamUnchecked(frame->hd.stream_id);
+    if (stream && name == static_cast<absl::string_view>(Http::Headers::get().HostLegacy)) {


nit, maybe move this check above the line 2131

envoy/source/common/http/http2/codec_impl.cc

Lines 1474 to 1484 in 518c582

StreamImpl* stream = getStreamUnchecked(frame->hd.stream_id);

if (!stream) {

// We have seen 1 or 2 crashes where we get a headers callback but there is no associated

// stream data. I honestly am not sure how this can happen. However, from reading the nghttp2

// code it looks possible that inflate_header_block() can safely inflate headers for an already

// closed stream, but will still call the headers callback. Since that seems possible, we should

// ignore this case here.

// TODO(mattklein123): Figure out a test case that can hit this.

stats_.headers_cb_no_stream_.inc();

return 0;

}

although not sure that check still available for now.

saveHeader is called from two places. I would need to duplicate this check. Maybe it is is better to keep it in one place only.

alyssawilk · 2023-10-23T18:37:29Z

/wait on comments and main merge

github-actions · 2023-11-22T20:01:13Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2023-11-29T20:01:15Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov · 2023-12-07T15:03:43Z

Addressed comments. @zuercher @soulxu please take a look.

soulxu

LGTM, thanks!

SeanKilleen · 2023-12-28T18:35:48Z

@jmarantz @zuercher @soulxu -- quick question as I try to understand the flow of envoy releases. I see this PR is merged but is not associated with a milestone. Am I correct in assuming that this would go out with 1.28.1 and possibly be back-ported to 1.27.3?

Apologies for tagging you all; I looked through the various docs but if there was anything about release flow & cadence, I missed it.

soulxu · 2024-01-02T01:14:03Z

@jmarantz @zuercher @soulxu -- quick question as I try to understand the flow of envoy releases. I see this PR is merged but is not associated with a milestone. Am I correct in assuming that this would go out with 1.28.1 and possibly be back-ported to 1.27.3?

Apologies for tagging you all; I looked through the various docs but if there was anything about release flow & cadence, I missed it.

@SeanKilleen this is going to 1.29 release. If you want this in 1.28 or 1.27, you need to backport it.

SeanKilleen · 2024-04-03T02:04:58Z

Per @zirain at the Istio project, the Istio team would appreciate this change being back-ported to v1.28.x.

you can try to ping @phlax or @alyssawilk (I'm not sure who is the right person own backport) at the original PR, request to backport to v1.28.

CCing @yanavlasov in case this is something they'd be able to take on as well. I would typically be willing to attempt it myself but I'm out of my depth from a language perspective.

Sorry for the time gap on this -- a miscommunication elsewhere gave me the impression it already had been backported, despite @soulxu's comments above to the contrary. Apologies!

phlax · 2024-04-03T07:35:28Z

@SeanKilleen can you raise PRs against the other supported branches - ie:

release/v1.28
release/v1.27
release/v1.26

it should be backported to all supported (unless they dont have the issue it addresses)

/backport

SeanKilleen · 2024-04-03T10:45:01Z

@phlax sure, I can try. I have no experience in this codebase or C++; I just happened to stumble upon an issue which was luckily fixed in this PR by @yanavlasov. But I'll check the contributing docs etc. and do my best.

phlax · 2024-04-03T10:53:45Z

great thanks @SeanKilleen

dont worry about c++ skills (mine are rudimentary at best)

the steps are basically this ...

$ git checkout release/v1.28
$ git checkout -b 1.28-discard-fix
$ git cherry-pick $COMMIT_ID_FROM_MAIN
$ git  push --set-upstream origin 1.28-discard-fix

this assumes you have a remote origin that you use to create PRs from

the $COMMIT_ID_FROM_MAIN can be gotten from the linked commit when the PR landed - in this case from here:

a71c76d

ie a71c76da8e6e925dd4006c7696911ddf149c0c72

when you create the PRs make sure the target branch is set to the relevant branch - in the example case release/v1.28

we should probs add this to docs somewhere

phlax · 2024-04-03T10:55:10Z

you will most likely have to resolve some conflict - if you are lucky just the changelog

SeanKilleen · 2024-04-03T10:55:42Z

@phlax ah, no problem. I'll add it to the docs too :)

Discard the Host header if the :authority header was received to bring Envoy into compliance with https://www.rfc-editor.org/rfc/rfc9113#section-8.3.1 This behavioral change can be reverted by setting runtime flag envoy.reloadable_features.http2_discard_host_header to false. --------- Signed-off-by: Yan Avlasov <yavlasov@google.com> Signed-off-by: Sean Killeen <SeanKilleen@gmail.com>

Discard the Host header if the :authority header was received to bring Envoy into compliance with https://www.rfc-editor.org/rfc/rfc9113#section-8.3.1 This behavioral change can be reverted by setting runtime flag envoy.reloadable_features.http2_discard_host_header to false. --------- Signed-off-by: Yan Avlasov <yavlasov@google.com>

Discard the Host header if the :authority header was received to bring Envoy into compliance with https://www.rfc-editor.org/rfc/rfc9113#section-8.3.1 This behavioral change can be reverted by setting runtime flag envoy.reloadable_features.http2_discard_host_header to false. --------- Signed-off-by: Yan Avlasov <yavlasov@google.com> Signed-off-by: Sean Killeen <SeanKilleen@gmail.com>

H/2: discard Host header when :authority is present

dfcac15

Signed-off-by: Yan Avlasov <yavlasov@google.com>

adisuissa reviewed Oct 6, 2023

View reviewed changes

jmarantz assigned zuercher Oct 12, 2023

zuercher reviewed Oct 12, 2023

View reviewed changes

soulxu reviewed Oct 13, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 23, 2023

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 22, 2023

github-actions bot closed this Nov 29, 2023

yanavlasov mentioned this pull request Nov 30, 2023

Some HTTP/2 requests fail with duplicate value in envoy authority request pseudo-header #31118

Closed

Merge branch 'main' into http2-discard-host-header

c3961cc

yanavlasov reopened this Dec 6, 2023

Add test to validate header order

f54855b

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot removed the waiting label Dec 6, 2023

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 6, 2023

Fix UHV build

393ec3f

Signed-off-by: Yan Avlasov <yavlasov@google.com>

soulxu approved these changes Dec 7, 2023

View reviewed changes

yanavlasov merged commit a71c76d into envoyproxy:main Dec 8, 2023

yanavlasov deleted the http2-discard-host-header branch December 8, 2023 19:35

SeanKilleen mentioned this pull request Dec 9, 2023

Some HTTP/2 requests fail with duplicate value in envoy authority request header istio/istio#48088

Closed

2 tasks

repokitteh-read-only bot added the backport/review Request to backport to stable releases label Apr 3, 2024

SeanKilleen mentioned this pull request Apr 3, 2024

1.28 Backport H/2 header discard fix #33296

Closed

SeanKilleen mentioned this pull request Apr 3, 2024

1.27 Backport H/2 header discard fix #33297

Closed

This was referenced Apr 3, 2024

1.28 Backport H/2 header discard fix #33300

Closed

1.28 Backport H/2 header discard fix #33301

Merged

SeanKilleen mentioned this pull request Apr 3, 2024

1.27 Backport H/2 header discard fix #33302

Merged

alyssawilk mentioned this pull request Jul 23, 2024

envoy_reloadable_features_http2_discard_host_header deprecation #35370

Open

ggreenway mentioned this pull request Jul 24, 2024

QUIC rejects streams with Host header #35415

Closed

phlax removed the backport/review Request to backport to stable releases label Jul 9, 2025

	StreamImpl* stream = getStreamUnchecked(frame->hd.stream_id);
	if (!stream) {
	// We have seen 1 or 2 crashes where we get a headers callback but there is no associated
	// stream data. I honestly am not sure how this can happen. However, from reading the nghttp2
	// code it looks possible that inflate_header_block() can safely inflate headers for an already
	// closed stream, but will still call the headers callback. Since that seems possible, we should
	// ignore this case here.
	// TODO(mattklein123): Figure out a test case that can hit this.
	stats_.headers_cb_no_stream_.inc();
	return 0;
	}

Conversation

yanavlasov commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 6, 2023

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Oct 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Oct 23, 2023

Uh oh!

github-actions bot commented Nov 22, 2023

Uh oh!

github-actions bot commented Nov 29, 2023

Uh oh!

yanavlasov commented Dec 7, 2023

Uh oh!

soulxu left a comment

Choose a reason for hiding this comment

Uh oh!

SeanKilleen commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulxu commented Jan 2, 2024

Uh oh!

SeanKilleen commented Apr 3, 2024

Uh oh!

phlax commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeanKilleen commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phlax commented Apr 3, 2024

Uh oh!

phlax commented Apr 3, 2024

Uh oh!

SeanKilleen commented Apr 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yanavlasov commented Oct 6, 2023 •

edited

Loading

SeanKilleen commented Dec 28, 2023 •

edited

Loading

phlax commented Apr 3, 2024 •

edited

Loading

SeanKilleen commented Apr 3, 2024 •

edited

Loading