http: processing of headers with underscores in names by yanavlasov · Pull Request #10611 · envoyproxy/envoy

yanavlasov · 2020-04-01T16:59:58Z

add config option to either allow, drop header or reject request
applies to client request headers only

Risk Level: Low (default behavior unchanged)
Testing: Unit+Integration Tests
Docs Changes: Yes
Release Notes: Yes

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

* add config option to either allow, drop header or reject request * applies to client request headers only Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only · 2020-04-01T17:00:05Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10611 was opened by yanavlasov.

see: more, trace.

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

mattklein123

Awesome work, thanks. Just a few small comments.

/wait

source/common/http/http1/codec_impl.h

mattklein123 · 2020-04-01T18:00:04Z

test/extensions/filters/http/jwt_authn/filter_integration_test.cc

    return [](Http::FilterChainFactoryCallbacks& callbacks) -> void {
      callbacks.addStreamDecoderFilter(
-          std::make_shared<HeaderToFilterStateFilter>("jwt_selector", "jwt_selector"));
+          std::make_shared<HeaderToFilterStateFilter>("jwt-selector", "jwt-selector"));


Given that we decided to keep the default as allow, maybe revert all of these test changes that we don't need?

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

mattklein123

Awesome, thank you. Can you do me a favor and open a tech debt issue to follow up on unifying all of the checks between the codecs into a single piece of code somehow? We need to also get this implemented for HTTP/3 before we call that finished so that would be a good opportunity to do that unification. cc @danzh2010

mattklein123 · 2020-04-02T21:46:43Z

Oops compile error looks legit.

/wait

mattklein123 · 2020-04-02T23:18:33Z

/azp run envoy-presubmit

azure-pipelines · 2020-04-02T23:18:41Z

Azure Pipelines successfully started running 1 pipeline(s).

yanavlasov · 2020-04-03T13:37:29Z

clang-tidy error seems to be bogus. It looks like include search directories may be not set correctly.

https://dev.azure.com/cncf/envoy/_build/results?buildId=36689&view=logs&j=3bf5ec37-bb6f-5fb6-2cf1-5134dc6106d6&t=41d571fe-3811-578d-07de-68468f467dae&l=1570

mattklein123 · 2020-04-03T18:08:18Z

@yanavlasov can you make sure that the bazel deps are correct for that target? Sometimes there are issues with that. Also can you merge master and open the follow up issue re: unification and QUIC? Thank you!

/wait

yanavlasov · 2020-04-03T23:48:40Z

Will do. Header validation issue: #10646

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

mattklein123 · 2020-04-05T16:14:57Z

I will go ahead and merge this as we need to do a full master pass over clang-tidy anyway. Thank you!

PiotrSikora · 2020-04-06T15:53:02Z

/backport

PiotrSikora · 2020-04-06T15:53:31Z

@mattklein123 @yanavlasov @lizan any thoughts on whether we should backport this to stable branches or not?

mattklein123 · 2020-04-06T15:55:37Z

@PiotrSikora I would defer to you. Given that we ultimately agreed with your original assessment that it is a feature request, I think it makes sense either way.

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

http: processing of headers with undderscores in names

341c163

* add config option to either allow, drop header or reject request * applies to client request headers only Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov requested review from alyssawilk, lizan and mattklein123 as code owners April 1, 2020 16:59

repokitteh-read-only bot added the api label Apr 1, 2020

yanavlasov changed the title ~~http: processing of headers with undderscores in names~~ http: processing of headers with underscores in names Apr 1, 2020

Merge branch 'master' into underscores

7fe7c29

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

mattklein123 self-assigned this Apr 1, 2020

mattklein123 requested changes Apr 1, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 1, 2020

yanavlasov added 3 commits April 2, 2020 16:01

Address comments

9c663f3

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

Merge branch 'master' into underscores

7c0529f

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

Change defaul action to ALLOW in tests

773a5c1

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

repokitteh-read-only bot removed the waiting label Apr 2, 2020

mattklein123 previously approved these changes Apr 2, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Apr 2, 2020

repokitteh-read-only bot added the waiting label Apr 2, 2020

mattklein123 removed the waiting label Apr 2, 2020

repokitteh-read-only bot added the waiting label Apr 3, 2020

Merge branch 'master' into underscores

c582ec3

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

yanavlasov dismissed mattklein123’s stale review via c582ec3 April 4, 2020 01:10

repokitteh-read-only bot added api and removed waiting labels Apr 4, 2020

mattklein123 approved these changes Apr 5, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Apr 5, 2020

mattklein123 merged commit 4449939 into envoyproxy:master Apr 5, 2020

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

PiotrSikora pushed a commit to PiotrSikora/envoy that referenced this pull request Jun 5, 2020

http: processing of headers with underscores in names (envoyproxy#10611)

0a3c1fe

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

PiotrSikora pushed a commit to PiotrSikora/envoy that referenced this pull request Jun 5, 2020

http: processing of headers with underscores in names (envoyproxy#10611)

d160ca4

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

PiotrSikora added a commit that referenced this pull request Jun 6, 2020

http: processing of headers with underscores in names (#10611) (#11464)

7c3231e

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

PiotrSikora added a commit that referenced this pull request Jun 6, 2020

http: processing of headers with underscores in names (#10611) (#11465)

1547f8b

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

PiotrSikora added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Jul 18, 2020

yanavlasov deleted the underscores branch July 30, 2020 14:52

Conversation

yanavlasov commented Apr 1, 2020

Uh oh!

repokitteh-read-only bot commented Apr 1, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Apr 2, 2020

Uh oh!

mattklein123 commented Apr 2, 2020

Uh oh!

azure-pipelines bot commented Apr 2, 2020

Uh oh!

yanavlasov commented Apr 3, 2020

Uh oh!

mattklein123 commented Apr 3, 2020

Uh oh!

yanavlasov commented Apr 3, 2020

Uh oh!

mattklein123 commented Apr 5, 2020

Uh oh!

PiotrSikora commented Apr 6, 2020

Uh oh!

PiotrSikora commented Apr 6, 2020

Uh oh!

mattklein123 commented Apr 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants