Plumb header validator into HTTP/1 server codec by yanavlasov · Pull Request #21974 · envoyproxy/envoy

yanavlasov · 2022-07-01T02:39:17Z

Additional Description:
This PR plumbs header validator into HTTP/1 codec. The code is disabled in normal builds and is behind the ENVOY_ENABLE_UHV flag, which is tested during compile_time_options build only.

H/1 client and H/2 codecs are to be done in a follow up PR.

Risk Level: Low, no changes for normal Envoy builds.
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

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

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

mattklein123

LGTM, thanks. Very excited to see this work progressing.

yanavlasov · 2022-07-01T19:08:11Z

/retest

repokitteh-read-only · 2022-07-01T19:08:15Z

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

🐱

Caused by: a #21974 (comment) was created by @yanavlasov.

see: more, trace.

ggreenway

LGTM

alyssawilk

Exciting to see this underway!
/wait

alyssawilk · 2022-07-06T13:09:41Z

source/common/http/http1/codec_impl.cc

+  const absl::string_view InvalidMethod = "http1.invalid_method";
+  const absl::string_view InvalidStatus = "http1.invalid_status";
+  const absl::string_view InvalidHeader = "http1.invalid_header";
+  const absl::string_view InvalidHeaders = "http1.invalid_headers";


is there value in disambiguating 1 from 1+?

Removed after refactor

alyssawilk · 2022-07-06T13:11:32Z

source/common/http/http1/codec_impl.cc

@@ -507,6 +515,9 @@ Status ConnectionImpl::completeLastHeader() {
  ASSERT(dispatching_);


optional as it's not your code, but consider renaming completeLastHeader as I read it as completing the last header, not completing the most recent header to be processed

Will do in a separate PR

Done in PR #22408

alyssawilk · 2022-07-06T13:13:13Z

source/common/http/http1/codec_impl.cc

  // CONNECT "urls" are actually host:port so look like absolute URLs to the above checks.
  // Absolute URLS in CONNECT requests will be rejected below by the URL class validation.
  if (!codec_settings_.allow_absolute_url_ && !is_connect) {
+    RETURN_IF_ERROR(checkHeaderEntryAndSendError(path, active_request_->request_url_,


don't we need to know if the request is CONNECT to validate if the "path" is legal in this case?

alyssawilk · 2022-07-06T13:16:14Z

source/common/http/http1/codec_impl.cc

  // request-target. A proxy that forwards such a request MUST generate a
  // new Host field-value based on the received request-target rather than
  // forward the received Host field-value.
+  RETURN_IF_ERROR(checkHeaderEntryAndSendError(Http::HeaderString(header_values.Host),


I can imagine these errors being pretty hard to debug without trace logs - I think previously the headers would land in the access logs, so you could debug what the invalid host was, where now we're protecting access logs and response filters from bad header data. I think the loss of debugability is probably worse though so I'd be inclined to add the header and then check validity so we can debug or come up with some other workaround

alyssawilk · 2022-07-06T13:20:08Z

source/common/http/http1/codec_impl.cc

+      result = codecProtocolError("http/1.1 protocol error: invalid header map");
+      std::function<void(ResponseHeaderMap & headers)> modify_headers;
+      if (validation_result == Http::HeaderValidator::RequestHeaderMapValidationResult::Redirect &&
+          !Grpc::Common::hasGrpcContentType(header_map)) {


Also I think before we mostly did checks of each header after processing all, so if we had a gRPC request and a bad header before gRPC content type, response would follow gRPC path, and now we'll send a standard error reply for headers we would have done "in envoy" (same path for prior http_parser errors). I don't think it's deal breaker but we should consider documenting this wherever we document differences between the old and the new.

It's also a lot more work to do this in all three codecs. Given we're probably going to have to have a "check these headers for errors" function for headers transformed by WASM or ext_authz and such, I wonder if we should simply do a one time pass on all headers after we read them and before the HCM processes them, rather than doing all this invasive plumbing into client and server codecs for HTTP/1, HTTP/2 and HTTP/3. We're going to have to do a pass against all headers once we have all headers to make sure we catch things like 'no content length and transfer encoding' so I wonder if we should just do character validation there and skip all the work along the way. Thoughts?

I have moved header validation into the HCM and moved it out of the codec. I think if we need to do checks for individual headers as they are being parsed, I can plumb it back into codecs. It is however performance optimization only, that allows header parsing to be stopped early once bad header is found.

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

alyssawilk

Nice, I think this will be much simpler.
/wait

alyssawilk · 2022-07-26T13:45:28Z

source/common/http/conn_manager_impl.cc

+      auto response_details_opt = filter_manager_.streamInfo().responseCodeDetails();
+
+      sendLocalReply(response_code, "", modify_headers, grpc_status,
+                     response_details_opt


IMO details should not be optional, and we should ENVOY_BUG it's not empty.

sorry missed this but it still looks optional. We could add details to the function signature and ENVOY_BUG that it's not empty?

alyssawilk · 2022-07-26T13:47:34Z

source/common/http/conn_manager_impl.cc

    request_header_timer_.reset();
  }

+  if (!validateHeaders()) {


optional: move logging up
ENVOY_STREAM_LOG(debug, "request headers complete
so we get "request headers complete" then "sending local reply"

alyssawilk · 2022-07-26T13:48:41Z

source/common/http/conn_manager_impl.cc

    request_header_timer_.reset();
  }

+  if (!validateHeaders()) {


one of the gains of moving this to the HCM is making sure we handle connection close properly for say HTTP/1.0 header only requests. I encourage best-effort checks of the shouldCloseconnection before the validation and an integration test which future-proofs since apparently nothing failed.

You right, I missed that. Added tests

alyssawilk · 2022-07-26T13:50:52Z

source/common/http/conn_manager_impl.cc

  }

+  if (!validateHeaders()) {
+    ENVOY_STREAM_LOG(debug, "request headers validation failed:\n{}", *this, *request_headers_);


encouraged follow-up (TODO?) we could have a default header validator, and move most of Envoy's current HCM checks there since they should be redundant with UHV
if (!request_headers_->Host())
if ((!HeaderUtility::isConnect(*request_headers_) || request_headers_->Path()) &&
...

This is on my TODO list and PR with these changes will be out soon.

alyssawilk · 2022-07-26T13:52:48Z

source/common/http/conn_manager_impl.h

      return *tracing_custom_tags_;
    }

+    bool validateHeaders();


comment, especially as today it doesn't always validate

alyssawilk · 2022-07-26T13:53:33Z

source/extensions/filters/network/http_connection_manager/config.cc

+                             [[maybe_unused]] Server::Configuration::FactoryContext& context) {
+
+  Http::HeaderValidatorFactoryPtr header_validator_factory;
+#ifdef ENVOY_ENABLE_UHV


this is only ifdef'd out until we have runtime controls of underlying header validation correct?

Should we throw an exception if it's configured on but not supported due to compile time options?

alyssawilk · 2022-07-26T13:55:32Z

source/server/admin/admin.h

    return proxy_status_config_.get();
  }
+  Http::HeaderValidatorPtr makeHeaderValidator(Http::Protocol, StreamInfo::StreamInfo&) override {
+    // TODO(yanavlasov): admin interface should use the default validator


should this ENVOY_BUG? seems like it's pretty unsafe to do admin requests until this is fixed

Had to undo ENVOY_BUG as it crashed all integration tests with UHV enabled. This item is on my internal OKR tracker and will not be dropped.

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

alyssawilk

LGTM modulo one last nit and CI failure
/wait
/retest

alyssawilk · 2022-07-27T13:17:28Z

source/common/http/conn_manager_impl.cc

+      auto response_details_opt = filter_manager_.streamInfo().responseCodeDetails();
+
+      sendLocalReply(response_code, "", modify_headers, grpc_status,
+                     response_details_opt


sorry missed this but it still looks optional. We could add details to the function signature and ENVOY_BUG that it's not empty?

repokitteh-read-only · 2022-07-27T13:17:46Z

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

🐱

Caused by: a #21974 (review) was submitted by @alyssawilk.

see: more, trace.

alyssawilk

making that last comment optional :-)

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

yanavlasov · 2022-07-27T19:24:10Z

Changed the return type of header validator to be a tuple of action and details string and enforced non empty string at construction.

alyssawilk · 2022-07-27T20:59:31Z

envoy/http/header_validator.h

+    using Action = ActionType;
+
+    // Helper for constructing successful results
+    static Result success() { return Result(ActionType::Accept, absl::string_view()); }


This needs a TODO for removal since it encourages empty string views, no?

We only need details if header map was invalid, i.e. when result was either reject or redirect. If header map passed, the details are empty, which is what this method is used for.

ugh sorry yes, my bad.

Signed-off-by: Yan Avlasov <yavlasov@google.com> Signed-off-by: yanavlasov <yavlasov@google.com> Signed-off-by: Ryan Northey <ryan@synca.io>

Plumb header validator into HTTP/1 server codec

4c8ef95

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

yanavlasov requested review from alyssawilk and mattklein123 as code owners July 1, 2022 02:39

Fix clang-tidy

c1ff781

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

yanavlasov assigned snowp, mattklein123, ggreenway and alyssawilk Jul 1, 2022

Fix build problem

d2d2ecb

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

mattklein123 previously approved these changes Jul 1, 2022

View reviewed changes

ggreenway previously approved these changes Jul 5, 2022

View reviewed changes

alyssawilk reviewed Jul 6, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 6, 2022

Address comments

24982fd

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

yanavlasov dismissed stale reviews from ggreenway and mattklein123 via 24982fd July 25, 2022 20:15

repokitteh-read-only bot removed the waiting label Jul 25, 2022

yanavlasov added 3 commits July 25, 2022 16:21

Merge branch 'main' into uhv-hcm-plumbing

ef41dc9

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

Update comment

96ef80b

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

Fix clang-tidy

0c15e37

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

alyssawilk reviewed Jul 26, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 26, 2022

yanavlasov added 2 commits July 26, 2022 13:19

Fix compile time build

f9aa958

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

Address comments

35d4ea4

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

repokitteh-read-only bot removed the waiting label Jul 26, 2022

alyssawilk reviewed Jul 27, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 27, 2022

alyssawilk previously approved these changes Jul 27, 2022

View reviewed changes

Address comments

078da58

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

yanavlasov dismissed alyssawilk’s stale review via 078da58 July 27, 2022 19:18

repokitteh-read-only bot removed the waiting label Jul 27, 2022

Remove unused comment

3d56ddd

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

alyssawilk reviewed Jul 27, 2022

View reviewed changes

alyssawilk approved these changes Jul 28, 2022

View reviewed changes

yanavlasov merged commit e2c2a77 into envoyproxy:main Jul 28, 2022

yanavlasov deleted the uhv-hcm-plumbing branch July 28, 2022 14:44

ameily mentioned this pull request Aug 3, 2022

uhv: header validators for H1 and H2 codecs #22537

Merged

		@@ -507,6 +515,9 @@ Status ConnectionImpl::completeLastHeader() {
		ASSERT(dispatching_);

Conversation

yanavlasov commented Jul 1, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Jul 1, 2022

Uh oh!

repokitteh-read-only bot commented Jul 1, 2022

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk 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!

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!

alyssawilk 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!

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!

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!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jul 27, 2022

Uh oh!