http2: replaces nghttp2 functions with equivalents from QUICHE. #24943

birenroy · 2023-01-13T17:18:59Z

Signed-off-by: Biren Roy birenroy@google.com

Commit Message: Replaces nghttp2 functions with equivalents from QUICHE.
Additional Description: Cleanup related to #23596
Risk Level: Low
Testing: ran unit and integration tests locally
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Biren Roy <birenroy@google.com>

birenroy · 2023-01-13T17:19:16Z

/assign @diannahu
/assign @RyanTheOptimist

RyanTheOptimist · 2023-01-13T19:28:02Z

source/common/http/header_utility.cc

@@ -190,17 +190,16 @@ bool HeaderUtility::schemeIsValid(const absl::string_view scheme) {
 }

 bool HeaderUtility::headerValueIsValid(const absl::string_view header_value) {
-  return nghttp2_check_header_value(reinterpret_cast<const uint8_t*>(header_value.data()),
-                                    header_value.size()) != 0;
+  return http2::adapter::HeaderValidator::IsValidHeaderValue(header_value,


Hm, do we need a runtime guard for this? I would imagine it's possible that this is a behavior change in the event that QUICHE and nghttp2 do something different?

What do you think about a runtime guard for this change?

Let me see if I can write a unit test that demonstrates whether there is/is not a behavior change.

With ObsTextOption::kAllow, the set of characters allowed by the oghttp2 method is exactly the same as the nghttp2 function.

Hm. Can we write a unit test then where we loop through all the possible characters and verify we get the same output? I just want to make 100% sure that this change doesn't result in some crazy corner case behavior change which isn't protected by a runtime guard? Or alternatively, if it obvious from inspection can you point me at the two methods? (Sorry to be pedantic here!)

Thanks for the offline discussion and the pointer to the test.

RyanTheOptimist

Should we link to the GitHub issues in the PR description?

birenroy · 2023-01-13T20:05:58Z

Should we link to the GitHub issues in the PR description?

Done, thanks.

RyanTheOptimist · 2023-01-19T19:43:33Z

/wait

Signed-off-by: Biren Roy <birenroy@google.com>

birenroy · 2023-01-23T22:33:17Z

/retest

repokitteh-read-only · 2023-01-23T22:33:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #24943 (comment) was created by @birenroy.

see: more, trace.

birenroy · 2023-01-24T15:14:13Z

@RyanTheOptimist I think this is ready for another look. I verified that the behavior is unchanged with the new method.

RyanTheOptimist · 2023-01-24T20:10:06Z

/assign @zuercher

RyanTheOptimist · 2023-01-24T20:10:18Z

@zuercher for cross-company review.

Signed-off-by: Biren Roy <birenroy@google.com>

zuercher · 2023-01-26T17:16:15Z

source/common/http/header_utility.cc

 }

 bool HeaderUtility::headerNameContainsUnderscore(const absl::string_view header_name) {
  return header_name.find('_') != absl::string_view::npos;
 }

 bool HeaderUtility::authorityIsValid(const absl::string_view header_value) {
-  return nghttp2_check_authority(reinterpret_cast<const uint8_t*>(header_value.data()),
-                                 header_value.size()) != 0;
+  return http2::adapter::HeaderValidator::IsValidAuthority(header_value);


We've confirmed this is identical to nghttp as well?

Looks like the QUICHE method rejects "@", which nghttp2 allows. I think this is slightly more adherent to RFC 3986 section 3.2.2.

Because it's stricter, I think that means we should have a runtime guard for this behavior so that someone can revert it.

Agreed. @birenroy can you add a guard for this?

RyanTheOptimist · 2023-02-02T05:28:01Z

/wait

Signed-off-by: Biren Roy <birenroy@google.com>

repokitteh-read-only · 2023-02-03T00:20:31Z

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

🐱

Caused by: #24943 was synchronize by birenroy.

see: more, trace.

wbpcode · 2023-02-08T12:32:44Z

Please merge main and resolve the ci first, thanks.

wbpcode · 2023-02-08T12:32:57Z

/wait

Signed-off-by: Biren Roy <birenroy@google.com>

wbpcode · 2023-02-09T01:38:57Z

cc @zuercher could you give a review again when you have free time? Thanks.

zuercher · 2023-02-10T01:13:41Z

source/common/http/BUILD

@@ -510,7 +510,7 @@ envoy_cc_library(
    srcs = ["header_utility.cc"],
    hdrs = ["header_utility.h"],
    external_deps = [
-        "nghttp2",
+        "quiche_http2_adapter",


I'm by no means a bazel expert, but should this continue to reference nghttp2 until the runtime flag is removed?

zuercher

One question about deps, but otherwise lgtm.

Signed-off-by: Biren Roy <birenroy@google.com>

birenroy · 2023-02-10T01:52:15Z

One question about deps, but otherwise lgtm.

Done.

birenroy · 2023-02-10T16:44:30Z

@yanavlasov @KBaichoo I think this PR is ready for merge.

Envoy uses the `authorityIsValid()` to validate the authority header for both H/1 and H/2 codecs. Previously Envoy used the nghttp2 validator and in #24943 this was changed to oghttp2's implementation. The two implementations differ in the way they handle the "@" character (nghttp2 allows it, and oghttp2 doesn't). According to the H/2 spec, the "@" character is not allowed as part of the authority header. However, for H/1 it is allowed as part of the "user-info@host:port" structure of the authority header. This PR changes the validator to be similar to the nghttp2 implemenation. The change can be temporarily dis In the future, when Envoy fully supports UHV (#10646), the H/1 and H/2 validation parts should be decoupled, and the oghttp2 authority validation can be used for H/2. --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com>

) Envoy uses the `authorityIsValid()` to validate the authority header for both H/1 and H/2 codecs. Previously Envoy used the nghttp2 validator and in envoyproxy#24943 this was changed to oghttp2's implementation. The two implementations differ in the way they handle the "@" character (nghttp2 allows it, and oghttp2 doesn't). According to the H/2 spec, the "@" character is not allowed as part of the authority header. However, for H/1 it is allowed as part of the "user-info@host:port" structure of the authority header. This PR changes the validator to be similar to the nghttp2 implemenation. The change can be temporarily dis In the future, when Envoy fully supports UHV (envoyproxy#10646), the H/1 and H/2 validation parts should be decoupled, and the oghttp2 authority validation can be used for H/2. --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com> Signed-off-by: duxin40 <duxin40@gamil.com>

Replaces nghttp2 functions with equivalents from QUICHE.

f17c493

Signed-off-by: Biren Roy <birenroy@google.com>

repokitteh-read-only bot assigned diannahu and RyanTheOptimist Jan 13, 2023

diannahu previously approved these changes Jan 13, 2023

View reviewed changes

RyanTheOptimist reviewed Jan 13, 2023

View reviewed changes

Merge branch 'main' into replace-nghttp2-functions

69036d4

Signed-off-by: Biren Roy <birenroy@google.com>

repokitteh-read-only bot assigned zuercher Jan 24, 2023

Merge branch 'main' into replace-nghttp2-functions

651d775

Signed-off-by: Biren Roy <birenroy@google.com>

zuercher reviewed Jan 26, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 2, 2023

birenroy added 2 commits February 2, 2023 19:19

Added runtime guard.

21a51b8

Signed-off-by: Biren Roy <birenroy@google.com>

Merge branch 'main' into replace-nghttp2-functions

e5641f6

Signed-off-by: Biren Roy <birenroy@google.com>

birenroy dismissed diannahu’s stale review via e5641f6 February 3, 2023 00:20

repokitteh-read-only bot removed the waiting label Feb 3, 2023

repokitteh-read-only bot added the waiting label Feb 8, 2023

birenroy added 2 commits February 8, 2023 13:53

Merge branch 'main' into replace-nghttp2-functions

443c519

Signed-off-by: Biren Roy <birenroy@google.com>

Added a changelog note.

ddec137

Signed-off-by: Biren Roy <birenroy@google.com>

repokitteh-read-only bot removed the waiting label Feb 8, 2023

zuercher reviewed Feb 10, 2023

View reviewed changes

Restored nghttp2 to bazel dependencies.

c0ab09d

Signed-off-by: Biren Roy <birenroy@google.com>

KBaichoo approved these changes Feb 10, 2023

View reviewed changes

KBaichoo merged commit 9ddd06d into envoyproxy:main Feb 10, 2023

alyssawilk mentioned this pull request Oct 23, 2023

envoy_reloadable_features_http2_validate_authority_with_quiche deprecation #30419

Closed

birenroy deleted the replace-nghttp2-functions branch January 18, 2024 15:21

adisuissa mentioned this pull request Aug 6, 2024

http: modify authority validation to allow @ character #35602

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: replaces nghttp2 functions with equivalents from QUICHE. #24943

http2: replaces nghttp2 functions with equivalents from QUICHE. #24943

birenroy commented Jan 13, 2023 •

edited

Loading

birenroy commented Jan 13, 2023

RyanTheOptimist Jan 13, 2023

RyanTheOptimist Jan 16, 2023

birenroy Jan 19, 2023

birenroy Jan 23, 2023

RyanTheOptimist Jan 24, 2023

RyanTheOptimist Jan 24, 2023

RyanTheOptimist left a comment

birenroy commented Jan 13, 2023

RyanTheOptimist commented Jan 19, 2023

birenroy commented Jan 23, 2023

repokitteh-read-only bot commented Jan 23, 2023

birenroy commented Jan 24, 2023

RyanTheOptimist commented Jan 24, 2023

RyanTheOptimist commented Jan 24, 2023

zuercher Jan 26, 2023

birenroy Jan 27, 2023

zuercher Jan 31, 2023

RyanTheOptimist Feb 2, 2023

birenroy Feb 3, 2023

RyanTheOptimist commented Feb 2, 2023

repokitteh-read-only bot commented Feb 3, 2023

wbpcode commented Feb 8, 2023

wbpcode commented Feb 8, 2023

wbpcode commented Feb 9, 2023

zuercher Feb 10, 2023

zuercher left a comment

birenroy commented Feb 10, 2023

birenroy commented Feb 10, 2023

http2: replaces nghttp2 functions with equivalents from QUICHE. #24943

http2: replaces nghttp2 functions with equivalents from QUICHE. #24943

Conversation

birenroy commented Jan 13, 2023 • edited Loading

birenroy commented Jan 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist left a comment

Choose a reason for hiding this comment

birenroy commented Jan 13, 2023

RyanTheOptimist commented Jan 19, 2023

birenroy commented Jan 23, 2023

repokitteh-read-only bot commented Jan 23, 2023

birenroy commented Jan 24, 2023

RyanTheOptimist commented Jan 24, 2023

RyanTheOptimist commented Jan 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist commented Feb 2, 2023

repokitteh-read-only bot commented Feb 3, 2023

wbpcode commented Feb 8, 2023

wbpcode commented Feb 8, 2023

wbpcode commented Feb 9, 2023

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment

birenroy commented Feb 10, 2023

birenroy commented Feb 10, 2023

birenroy commented Jan 13, 2023 •

edited

Loading