uhv: header validators for H1 and H2 codecs by ameily · Pull Request #22537 · envoyproxy/envoy

ameily · 2022-08-03T12:33:31Z

Commit Message: uhv: header validators for H1 and H2 codecs
Additional Description: This PR builds off the foundation laid in #21974 to add validators for H1 and H2. H3 and path normalization are intentionally out of scope for this PR and will be completed later.
Risk Level: Low
Testing: Build Envoy with --define uhv=enabled, configure a service with UHV enabled, and then test a valid and invalid request.

# config.yml, in the HCM section
typed_header_validation_config:
  name: envoy.http.header_validators.envoy_default
  typed_config:
      "@type": type.googleapis.com/envoy.extensions.http.header_validators.envoy_default.v3.HeaderValidatorConfig
      #
      # UHV configuration
      # uncomment any of the following:
      #
      # restrict_http_methods: true
      # reject_headers_with_underscores: true
      # http1_protocol_options: {allow_chunked_length: true}
      #

# run tests
# valid request
http GET http://localhost:10000 X-Valid:ok
# invalid request
http GET http://localhost:10000 "X Invalid":bad

Fixes #19753
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

Moving the call to validateHeaders fixes a segfault when validation fails. Prior to this change, the stream would be reset after the local reply was sent, which caused a crash. Now, the request is marked as completed prior to sending the local reply when UHV fails. Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

repokitteh-read-only · 2022-08-03T12:33:45Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #22537 was opened by ameily.

see: more, trace.

alyssawilk · 2022-08-03T18:58:28Z

/wait on CI

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

ameily · 2022-08-05T15:02:44Z

@alyssawilk Can you rerun the failed CI job? CI is passing for me locally and I think the current failure is because a package couldn't be downloaded.

alyssawilk · 2022-08-08T13:57:08Z

you should be able to yourself with
/retest

repokitteh-read-only · 2022-08-08T13:57:13Z

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

🐱

Caused by: a #22537 (comment) was created by @alyssawilk.

see: more, trace.

kyessenov · 2022-08-08T18:58:12Z

/assign-from @envoyproxy/api-shepherds

repokitteh-read-only · 2022-08-08T18:58:16Z

@envoyproxy/api-shepherds assignee is @markdroth

🐱

Caused by: a #22537 (comment) was created by @kyessenov.

see: more, trace.

yanavlasov

/wait

source/extensions/http/header_validators/envoy_default/character_tables.h

source/extensions/http/header_validators/envoy_default/header_validator.cc

source/extensions/http/header_validators/envoy_default/header_validator_factory.cc

source/extensions/http/header_validators/envoy_default/http1_header_validator.cc

source/extensions/http/header_validators/envoy_default/http2_header_validator.cc

source/extensions/http/header_validators/envoy_default/header_validator_factory.cc

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

jmarantz · 2022-08-17T13:14:19Z

@yanavlasov @adisuissa this looks ready for another round of review.

wrowe · 2022-09-19T17:09:43Z

@lizan could you give this an API stamp for the docs update?

alyssawilk

Looking great! Sorry for the delay
/wait

source/extensions/http/header_validators/envoy_default/header_validator.cc

source/extensions/http/header_validators/envoy_default/header_validator.h

source/extensions/http/header_validators/envoy_default/http1_header_validator.cc

source/extensions/http/header_validators/envoy_default/http2_header_validator.cc

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

source/extensions/http/header_validators/envoy_default/header_validator.cc

source/extensions/http/header_validators/envoy_default/header_validator.h

source/extensions/http/header_validators/envoy_default/http1_header_validator.cc

test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

alyssawilk

Awesome. Thanks for the fantastic addition and all your patience with the review

wrowe · 2022-10-06T15:56:31Z

@envoyproxy/api-shepherds - This optional validator (default, not enabled) has been approved by Yan, but who is willing to review this for an api stamp of approval?

yanavlasov · 2022-10-06T16:02:33Z

I'm submitting this PR so we can proceed with follow-up work. If you have any more comments, please open Issues.

adisuissa

Thanks, left some API comments.

api/envoy/extensions/http/header_validators/envoy_default/v3/header_validator.proto

adisuissa · 2022-10-06T16:10:05Z

api/envoy/extensions/http/header_validators/envoy_default/v3/header_validator.proto

+
+  // Action to take when a client request with a header name containing underscore characters is received.
+  // If this setting is not specified, the value defaults to ALLOW.
+  HeadersWithUnderscoresAction headers_with_underscores_action = 4;


High-level question: do you think there may be a need in the future for action-specific-settings.
For example, will there be something like "REPLACE_UNDERSCORE" that will need the replacement string.
If so, I would suggest using a oneof, and messages for each option.

All these settings are for existing behavior. In the future if people want to modify it, they can add a new extension with their own config.

lizan · 2022-10-06T18:13:51Z

api/envoy/extensions/http/header_validators/envoy_default/v3/header_validator.proto

+
+  // Action to take when a client request with a header name containing underscore characters is received.
+  // If this setting is not specified, the value defaults to ALLOW.
+  HeadersWithUnderscoresAction headers_with_underscores_action = 4;


Does this field need an Envoy build with uhv=enabled to take effect? If so please document.

UHV is enabled in HCM

which is currently marked as hidden.

The uhv=enabled is really a temporary build flag to safely transition to the new verification code. Do you think we need to document it? It will not be added into the generated documentation anyway because of the hidden flag.

api/envoy/extensions/http/header_validators/envoy_default/v3/header_validator.proto

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

adisuissa

/lgtm api

yanavlasov · 2022-10-11T20:42:29Z

Looks like all comments were addressed. If we missed anything, please file an Issue and we will address in a followup PR.

Signed-off-by: Adam Meily <adam.meily@trailofbits.com> Signed-off-by: Adam Meily <ameily@users.noreply.github.com> Signed-off-by: Ryan Northey <ryan@synca.io>

ameily added 4 commits July 29, 2022 12:10

add h1 and h2 unified header validators (manual rebase on main)

9ef13f5

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

fix uhv error handling

ba72f0b

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

remove debug print

5e45db0

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

ameily requested review from alyssawilk and yanavlasov as code owners August 3, 2022 12:33

repokitteh-read-only bot added the api label Aug 3, 2022

repokitteh-read-only bot assigned markdroth Aug 3, 2022

alyssawilk assigned yanavlasov Aug 3, 2022

repokitteh-read-only bot added the waiting label Aug 3, 2022

ameily added 3 commits August 4, 2022 07:59

clang-tidy fixes

a72e5df

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

update testChar function name

42a0898

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

fix string_view reference errors

03c6a53

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

repokitteh-read-only bot removed the waiting label Aug 4, 2022

kyessenov assigned lizan and unassigned markdroth Aug 8, 2022

yanavlasov requested changes Aug 11, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 11, 2022

ameily added 2 commits August 12, 2022 10:11

address PR feedback

289282a

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

clang-format

b698ccf

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

repokitteh-read-only bot removed the waiting label Aug 12, 2022

ameily mentioned this pull request Aug 15, 2022

uhv: path normalization #22707

Merged

alyssawilk reviewed Sep 22, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 22, 2022

address PR feedback

57055bb

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

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

fix spelling

a8b0d56

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

alyssawilk reviewed Sep 28, 2022

View reviewed changes

test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc Outdated Show resolved Hide resolved

ameily mentioned this pull request Sep 28, 2022

uhv: unify Transfer-Encoding validation behavior #23289

Closed

ameily added 3 commits September 28, 2022 14:25

update TODOs with issue references

337d961

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

address PR feedback

32c1917

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

address PR feedback

66bb212

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

alyssawilk previously approved these changes Oct 3, 2022

View reviewed changes

yanavlasov previously approved these changes Oct 5, 2022

View reviewed changes

adisuissa reviewed Oct 6, 2022

View reviewed changes

lizan reviewed Oct 6, 2022

View reviewed changes

adisuissa reviewed Oct 7, 2022

View reviewed changes

api/envoy/extensions/http/header_validators/envoy_default/v3/header_validator.proto Outdated Show resolved Hide resolved

api/envoy/extensions/http/header_validators/envoy_default/v3/header_validator.proto Outdated Show resolved Hide resolved

add references to counters

42251ef

Signed-off-by: Adam Meily <adam.meily@trailofbits.com>

ameily dismissed stale reviews from yanavlasov and alyssawilk via 42251ef October 11, 2022 17:13

adisuissa reviewed Oct 11, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Oct 11, 2022

yanavlasov approved these changes Oct 11, 2022

View reviewed changes

yanavlasov merged commit 05360a8 into envoyproxy:main Oct 11, 2022

adisuissa mentioned this pull request Oct 13, 2022

New header validation invalid parsing of integers #23471

Closed

Conversation

ameily commented Aug 3, 2022

Uh oh!

repokitteh-read-only bot commented Aug 3, 2022

Uh oh!

alyssawilk commented Aug 3, 2022

Uh oh!

ameily commented Aug 5, 2022

Uh oh!

alyssawilk commented Aug 8, 2022

Uh oh!

repokitteh-read-only bot commented Aug 8, 2022

Uh oh!

kyessenov commented Aug 8, 2022

Uh oh!

repokitteh-read-only bot commented Aug 8, 2022

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz commented Aug 17, 2022

Uh oh!

wrowe commented Sep 19, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

wrowe commented Oct 6, 2022

Uh oh!

yanavlasov commented Oct 6, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adisuissa Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

yanavlasov Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

lizan Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

yanavlasov Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Oct 11, 2022

Uh oh!