Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proxy_protocol_filter: Add configuration to match only specific proxy protocol versions, new stats #32861

Merged
merged 36 commits into from
Mar 29, 2024
Merged

proxy_protocol_filter: Add configuration to match only specific proxy protocol versions, new stats #32861

merged 36 commits into from
Mar 29, 2024
+724 −38

Conversation

nareddyt
Copy link
Contributor

@nareddyt nareddyt commented Mar 12, 2024 •
edited
Loading

Commit Message:
proxy_protocol_filter: Configuration to match only specific proxy protocol versions, new stats

Additional Description:
Currently the Proxy Protocol Listener filter will try to match incoming connections against both proxy protocol v1 and v2 signatures. While this is convenient, it:

  • Increases the attack surface of the filter.
  • (when allow_requests_without_proxy_protocol is enabled) Increases the chance of signature conflicts between proxy protocol v1 requests and non-proxy protocol requests.

This change adds a new config option disallowed_versions that scopes down the set of proxy protocol versions that the filter matches. The configuration is optional and defaults to current behavior when not specified.

This change also adds new statistics per matched proxy protocol version. See doc update for details.

downstream_cx_proxy_proto.not_found_disallowed
downstream_cx_proxy_proto.not_found_allowed
downstream_cx_proxy_proto.v1.found
downstream_cx_proxy_proto.v1.disallowed
downstream_cx_proxy_proto.v1.error
downstream_cx_proxy_proto.v2.found
downstream_cx_proxy_proto.v2.disallowed
downstream_cx_proxy_proto.v2.error

Pre-existing stat downstream_cx_proxy_proto_error is kept at it's own scope for backwards-compatibility.

Risk Level: Low

  • All client-facing behavior changes are guarded by new filter config field.
  • Only default behavior change is incrementing new counters.

Testing:

  • Extensive unit tests
  • Some integration tests

Docs Changes:
Updated proto and filter docs

Release Notes:
Updated

Platform Specific Features:
N/A

Fixes #32425

@nareddyt
Code changes
787d32b 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Format
3010892 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Docs
c285452 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Docs fix
c846b2e 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Add release notes
0a8f787 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@repokitteh-read-only RepoKitteh - Read Only
Copy link

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: #32861 was opened by nareddyt.

see: more, trace.

@nareddyt
Fix typo
ebcb7dd 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Fix typo
cfb3be7 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Copy link
Contributor Author

/review @fzhong-connect

@repokitteh-read-only RepoKitteh - Read Only
Copy link

nareddyt is not a collaborator, thus allowed to assign users.

🐱

Caused by: a #32861 (comment) was created by @nareddyt.

see: more, trace.

@nareddyt
Copy link
Contributor Author

cc @fzhong-connect @jaysonjdmello

@nareddyt
review comments
ae9c3e3 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
update doc
5df93bb 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt nareddyt requested a review from ggreenway March 13, 2024 15:26
@nareddyt
Fix docs
03b353e 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Fix windows build
ed99e3c 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Attempt to fix test failure
c0614b3 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Copy link
Contributor Author

@markdroth or @envoyproxy/api-shepherds PTAL, we have a question for you in the comments.

@nareddyt
Copy link
Contributor Author

PTAL

@nareddyt
fix spelling
f9c33a1 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
Copy link
Contributor Author

Ready for review @ggreenway

@nareddyt
Copy link
Contributor Author

/retest

1 similar comment
@nareddyt
Copy link
Contributor Author

/retest

ggreenway
ggreenway requested changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/wait

docs/root/configuration/listeners/listener_filters/proxy_protocol.rst Outdated Show resolved Hide resolved
docs/root/configuration/listeners/listener_filters/proxy_protocol.rst Outdated Show resolved Hide resolved
@nareddyt
review comments: change root stats scope, fix docs
4a02c62 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt nareddyt requested a review from ggreenway March 26, 2024 22:21
@nareddyt
Copy link
Contributor Author

/retest

@nareddyt
Copy link
Contributor Author

Ready for review @ggreenway . Not sure how to re-run the flaky CI failure

ggreenway
ggreenway previously requested changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly LGTM, aside from these small details.

I'm going to be out for awhile starting tomorrow, so one of the other maintainers will need to finish review of this and get it merged.

/wait

source/common/config/well_known_names.cc Outdated Show resolved Hide resolved
source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc Outdated Show resolved Hide resolved
@ggreenway ggreenway removed their assignment Mar 27, 2024
@nareddyt
Merge branch 'main' into pp-allowed-versions-stats
2c0f33f 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
review comments: fix stats tag extraction
5ae0954 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt
review comments: stats prefix name
9674dbf 
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt nareddyt requested a review from ggreenway March 27, 2024 23:07
@nareddyt
Copy link
Contributor Author

This mostly LGTM, aside from these small details.

I'm going to be out for awhile starting tomorrow, so one of the other maintainers will need to finish review of this and get it merged.

Thanks @ggreenway . If you have time, feel free to review today. Slightly worried Github may not let other reviewers merge as the status checks say:

1 review requesting changes by reviewers with write access. Merging can be performed automatically once the requested changes are addressed.

But no worries if you have taken off already, I can ping other reviewers tomorrow. Thanks for the thorough feedback!

@nareddyt
Copy link
Contributor Author

/retest

@yanavlasov yanavlasov dismissed ggreenway’s stale review March 29, 2024 23:05

Comments were addressed

@yanavlasov yanavlasov merged commit 113e997 into envoyproxy:main Mar 29, 2024
54 checks passed
nareddyt added a commit to nareddyt/envoy that referenced this pull request Apr 1, 2024
@nareddyt
proxy_protocol_filter: Add configuration to match only specific proxy…
1ff7c1b 
… protocol versions, new stats (envoyproxy#32861)

---------

Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
nareddyt added a commit to nareddyt/envoy that referenced this pull request Apr 11, 2024
@nareddyt
proxy_protocol_filter: Add configuration to match only specific proxy…
91ada95 
… protocol versions, new stats (envoyproxy#32861)

---------

Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
alyssawilk pushed a commit to alyssawilk/envoy that referenced this pull request Apr 29, 2024
@nareddyt @alyssawilk
proxy_protocol_filter: Add configuration to match only specific proxy…
f99459a 
… protocol versions, new stats (envoyproxy#32861)


---------

Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
@nareddyt nareddyt mentioned this pull request May 29, 2024
Merged
jmarantz pushed a commit that referenced this pull request Jun 10, 2024
@nareddyt
proxy_protocol_filter: Add field stat_prefix to the filter configur…
69d4ef8 
…ation (#34414)

Commit Message:
proxy_protocol_filter: Add field stat_prefix to the filter configuration

Additional Description:
This field allows for differentiating statistics when multiple proxy protocol listener filters are configured.

This PR is a follow-up from previous conversation: #32861 (comment)

Risk Level: Low

All client-facing behavior changes are guarded by new filter config field.
Testing:

Stats unit tests
Proxy protocol listener filter integration tests
Docs Changes:
Done

Release Notes:
Done

Platform Specific Features:
None

Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
nareddyt added a commit to nareddyt/envoy that referenced this pull request Jun 10, 2024
@nareddyt
proxy_protocol_filter: Add field stat_prefix to the filter configur…
d81a16c 
…ation (envoyproxy#34414)

Commit Message:
proxy_protocol_filter: Add field stat_prefix to the filter configuration

Additional Description:
This field allows for differentiating statistics when multiple proxy protocol listener filters are configured.

This PR is a follow-up from previous conversation: envoyproxy#32861 (comment)

Risk Level: Low

All client-facing behavior changes are guarded by new filter config field.
Testing:

Stats unit tests
Proxy protocol listener filter integration tests
Docs Changes:
Done

Release Notes:
Done

Platform Specific Features:
None

Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch left review comments

@yanavlasov yanavlasov yanavlasov approved these changes

@soulxu soulxu Awaiting requested review from soulxu soulxu is a code owner

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@ggreenway ggreenway Awaiting requested review from ggreenway ggreenway is a code owner

Assignees

@soulxu soulxu

@markdroth markdroth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

proxy_protocol listener filter: Allow configuration for specific proxy protocol version
6 participants
@nareddyt @markdroth @soulxu @yanavlasov @ggreenway @htuch