Skip to content

Revert "extending propercasing to legacy clusters, header sanitization to all routes (#1621)"#1710

Merged
buildbreaker merged 4 commits intomainfrom
revert-extending-propercasing-to-legacy-clusters-header-sanitization-to-all-routes-1621
Aug 17, 2021
Merged

Revert "extending propercasing to legacy clusters, header sanitization to all routes (#1621)"#1710
buildbreaker merged 4 commits intomainfrom
revert-extending-propercasing-to-legacy-clusters-header-sanitization-to-all-routes-1621

Conversation

@buildbreaker
Copy link

This is causing some TLS errors:
upstream connect error or disconnect/reset before headers. reset reason: connection failure, transport failure reason: TLS error: 268435703:SSL routines:OPENSSL_internal:WRONG_VERSION_NUMBER

This reverts commit 6e507d3.

Signed-off-by: Alan Chiu achiu@lyft.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Revert "extending propercasing to legacy clusters, header sanitization to all routes (#1621)"
Risk Level: medium
Testing: manual testing
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

…n to all routes (#1621)"

This reverts commit 6e507d3.

Signed-off-by: Alan Chiu <achiu@lyft.com>
@mattklein123
Copy link
Member

cc @alyssawilk I can't see any scenario in which this config change causes a TLS wrong version error, but I'm actually able to repro this on Linux so I'm looking into it.

@mattklein123
Copy link
Member

So the issue is in the following code:

https://github.com/envoyproxy/envoy/blob/5e48a19b20c10f31750ce430721ea868e7b1c457/source/extensions/clusters/dynamic_forward_proxy/cluster.cc#L182-L193

We did not previously configure upstream_http_protocol_options. This meant that both auto_sni() and auto_san_validation() have been set to true. Note also that upstream_http_protocol_options is deprecated. When we configure typed_extension_protocol_options we actually overwrite the values of this deprecated field, thus ignoring it:

https://github.com/envoyproxy/envoy/blob/5e48a19b20c10f31750ce430721ea868e7b1c457/source/common/upstream/upstream_impl.cc#L683-L689

This change definitely needs to be reverted and a few action items I would recommend:

  1. Make sure we have tests that would have caught this problem.
  2. Make sure we are warning/failing on using deprecated fields in Envoy Mobile.
  3. Make sure that the DFP cluster checking code properly understands how to check for whether auto_sni and auto_san_validation are set.

mattklein123
mattklein123 previously approved these changes Aug 16, 2021
@buildbreaker
Copy link
Author

buildbreaker commented Aug 16, 2021

@carloseltuerto the //test/java/org/chromium/net:cronet_url_request_test is failing here. I will have to revert ignore it :(

Signed-off-by: Alan Chiu <achiu@lyft.com>
mattklein123
mattklein123 previously approved these changes Aug 16, 2021
goaway
goaway previously approved these changes Aug 16, 2021
Signed-off-by: Alan Chiu <achiu@lyft.com>
@buildbreaker buildbreaker dismissed stale reviews from goaway and mattklein123 via 643344b August 16, 2021 23:10
…ters-header-sanitization-to-all-routes-1621
@buildbreaker buildbreaker merged commit 6bfcbea into main Aug 17, 2021
@buildbreaker buildbreaker deleted the revert-extending-propercasing-to-legacy-clusters-header-sanitization-to-all-routes-1621 branch August 17, 2021 01:09
buildbreaker pushed a commit that referenced this pull request Aug 18, 2021
…n to all routes (#1621)" (#1710)

* Revert "extending propercasing to legacy clusters, header sanitization to all routes (#1621)"

This reverts commit 6e507d3.

Signed-off-by: Alan Chiu <achiu@lyft.com>

* ignore test

Signed-off-by: Alan Chiu <achiu@lyft.com>

* codespell

Signed-off-by: Alan Chiu <achiu@lyft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants