UHV: Fix response status checks #27125

yanavlasov · 2023-05-02T19:03:12Z

Commit Message:
Change the code in H/2 and H/3 codecs that needs valid :status value before the header map was validated by UHV to not expect the :status to be valid. If :status is invalid the response is rejected when it is checked by UHV in the CodecClient.
For H/1 codec the status validation remains in the low level parser as it is a much larger change to move it to UHV. This reduces visibility into protocol errors due to malformed status (i.e. it is impossible to see the specific status value that caused the failure), however it will be mitigated once parser error reasons are plumbed into the reset reason strings that are logged in the access log.

Additional Description:
For H/1 codec it is a lot more difficult to move status validation in the response line out of the low level parser (http-parser and Balsa), as it will require changing parser API and a few other spots where parser itself is using the code. This may become more tractable once http-parser is decommissioned.

I think keeping status validation in H/1 parser for now is ok, as it just reduces visibility into protocol errors if status is malformed. When the plumbing for the parser error codes into reset reason strings is finished it will reduce this shortcoming.

Risk Level: Low. Build flag protected
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

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

alyssawilk · 2023-05-03T12:54:45Z

source/common/http/http2/codec_impl.cc

+    // Non-informational headers are non-1xx OR 101-SwitchingProtocols, since 101 implies that
+    // further proxying is on an upgrade path.
+    received_noninformational_headers_ =
+        !CodeUtility::is1xx(status) || status == enumToInt(Http::Code::SwitchingProtocols);


why don't we have he same problem for the other codecs?
would it makes more sense to have isSpecial1xx handle these cases?

Good point. Both balsa and EnvoyQuicClientStream validate status, so it does not reach UHV. I will work on fixing this. Balsa fix will need to wait, since Bence is OOO.

I have moved the :status header checks for H/2 and H/3 codecs into UHV when it is enabled. For H/1 codec it is a lot more difficult to move status validation in the response line out of the low level parser (http-parser and Balsa), as it will require changing parser API and a few other spots where parser itself is using the code. This may become more tractable once http-parser is decommissioned.

I think keeping status validation in H/1 parser for now is ok, as it just reduces visibility into protocol errors if status is malformed. When the plumbing for the parser error codes into reset reason strings is finished it will reduce this shortcoming.

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

RyanTheOptimist

Looks good. A couple of small comments.

RyanTheOptimist · 2023-08-15T23:02:23Z

source/common/http/http2/codec_impl.cc

+    } else {
+      response_decoder_.decodeHeaders(std::move(headers), sendEndStream());
+    }
+  } else {


nit: Let's use early return to save a level of nesting:

if (!status_opt.has_value()) { // In case the status is invalid or missing, the response_decoder_.decodeHeaders() will fail the // request response_decoder_.decodeHeaders(std::move(headers), sendEndStream()); return; } const uint64_t status = status_opt.value(); ...

RyanTheOptimist · 2023-08-15T23:04:37Z

test/integration/protocol_integration_test.cc

@@ -1879,13 +1879,6 @@ TEST_P(ProtocolIntegrationTest, 304WithBody) {
 }

 TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) {
-#ifdef ENVOY_ENABLE_UHV


RyanTheOptimist · 2023-08-15T23:04:58Z

source/common/http/http2/codec_impl.cc

@@ -547,6 +546,31 @@ void ConnectionImpl::ClientStreamImpl::decodeHeaders() {
  } else {
    response_decoder_.decodeHeaders(std::move(headers), sendEndStream());
  }
+#else
+  // in UHV mode the :status header at this point can be malformed, as it is validated


nit: "in" => "In"

RyanTheOptimist · 2023-08-15T23:05:18Z

source/common/quic/envoy_quic_client_stream.cc

+  // represented as their HTTP/1 forms, regardless of the HTTP version used.
+  // Therefore, these need to be transformed into their HTTP/1 form.
+
+  // in UHV mode the :status header at this point can be malformed, as it is validated


nit: "in" => "In"

RyanTheOptimist · 2023-08-15T23:06:21Z

source/common/quic/envoy_quic_client_stream.cc

+  if (optional_status.has_value()) {
+    const uint64_t status = optional_status.value();
+    if (Http::CodeUtility::is1xx(status)) {
+      // These are Informational 1xx headers, not the actual response headers.


Does this need to check enumToInt(Http::Code::SwitchingProtocols), too?

101 Switching protocols is invalid for H/2 and H/3, since they do not support upgrades, and have the extended CONNECT which uses 200 status.

Yeah, agreed. But I'm wondering if we need to detect this code and explicitly error out, since it's forbidden. Do we have an integration test for 101 Switching protocols over H3 (with and without UHV)?

This is a good question. The standard does not say that it is malformed, only that it is not supported https://www.rfc-editor.org/rfc/rfc9113#name-the-upgrade-header-field . I'm unsure if it needs to be treated as a protocol error by Envoy. Also not sure what effect it will have on H/3 by calling set_headers_decompressed(false);

Thanks for pointing this out. I've filed #29071 (and left comment in code) since it is a bigger issue that needs separate look.

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

Fix response status checks with UHV

a07abe6

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

yanavlasov assigned alyssawilk May 2, 2023

alyssawilk reviewed May 3, 2023

View reviewed changes

alyssawilk added the waiting label May 9, 2023

yanavlasov added the no stalebot Disables stalebot from closing an issue label Jun 8, 2023

alyssawilk removed their assignment Jun 15, 2023

yanavlasov added 2 commits August 8, 2023 20:21

Merge branch 'main' into uhv-status-check

21705a9

Change :status validation for H/3 in UHV mode

2b024c7

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

yanavlasov removed the no stalebot Disables stalebot from closing an issue label Aug 15, 2023

yanavlasov assigned RyanTheOptimist Aug 15, 2023

yanavlasov removed the waiting label Aug 15, 2023

RyanTheOptimist reviewed Aug 15, 2023

View reviewed changes

yanavlasov added 2 commits August 16, 2023 14:54

Address comments

6940338

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

Address comments

c12fc22

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

RyanTheOptimist approved these changes Aug 16, 2023

View reviewed changes

yanavlasov enabled auto-merge (squash) August 16, 2023 20:10

Merge branch 'main' into uhv-status-check

3524186

yanavlasov disabled auto-merge August 17, 2023 20:24

Merge branch 'main' into uhv-status-check

38d892e

yanavlasov enabled auto-merge (squash) August 21, 2023 14:19

Merge branch 'main' into uhv-status-check

98a495d

yanavlasov merged commit 37ab22a into envoyproxy:main Aug 21, 2023
114 of 115 checks passed

yanavlasov deleted the uhv-status-check branch August 21, 2023 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UHV: Fix response status checks #27125

UHV: Fix response status checks #27125

yanavlasov commented May 2, 2023 •

edited

Loading

alyssawilk May 3, 2023

yanavlasov May 4, 2023

yanavlasov Aug 15, 2023

RyanTheOptimist left a comment

RyanTheOptimist Aug 15, 2023

yanavlasov Aug 16, 2023

RyanTheOptimist Aug 15, 2023

RyanTheOptimist Aug 15, 2023

yanavlasov Aug 16, 2023

RyanTheOptimist Aug 15, 2023

yanavlasov Aug 16, 2023

RyanTheOptimist Aug 15, 2023

yanavlasov Aug 16, 2023

RyanTheOptimist Aug 16, 2023

yanavlasov Aug 16, 2023

UHV: Fix response status checks #27125

UHV: Fix response status checks #27125

Conversation

yanavlasov commented May 2, 2023 • edited Loading

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

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

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

yanavlasov commented May 2, 2023 •

edited

Loading