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_proto: fixing hashing bug #13768

Merged
merged 9 commits into from
Nov 4, 2020
Merged

proxy_proto: fixing hashing bug #13768

merged 9 commits into from
Nov 4, 2020

Conversation

alyssawilk
Copy link
Contributor

Fix a bug where the transport socket options for the first downstream got reused for subsequent upstream connections.

Risk Level: low
Testing: new integration test
Docs Changes: n/a
Release Notes:
Platform Specific Features:
Fixes #13659

@alyssawilk
proxy_proto: fixing hashing bug
e7d8f9d 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk added the backport/review Request to backport to stable releases label Oct 26, 2020
@alyssawilk
Copy link
Contributor Author

Also per @rchernobelskiy 's comment on the issue, can anyone think of a better way to avoid bugs of this type? I really dislike sizeof tests, but the best I can think of is a sizeof() test which best-effort catches new proxy protocol additions and suggests that we update the hash before adjusting the size. Any better ideas?

@alyssawilk
fix
028b270 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
lizan
lizan reviewed Oct 27, 2020
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

LGTM

test/integration/integration_tcp_client.h Outdated
const Network::ConnectionSocket::OptionsSharedPtr& options);
const Network::ConnectionSocket::OptionsSharedPtr& options,
Network::Address::InstanceConstSharedPtr source_address =
Network::Address::InstanceConstSharedPtr());
Copy link
Member

Choose a reason for hiding this comment

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

style nit: this is nullptr?

lizan
lizan reviewed Oct 27, 2020
View reviewed changes
include/envoy/network/transport_socket.h Outdated
/**
* @return bool whether the transport socket will use proxy protocol options.
*/
virtual bool usesProxyProtocolOptions() const { return false; }
Copy link
Member

Choose a reason for hiding this comment

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

tap transport socket would need return underlying transport socket value instead of false.

@alyssawilk
comments
bc51d65 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Merge branch 'master' into proxy_proto_final
16a444f 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
coverage
7df67cc 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
more coverage
af7d989 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Merge branch 'master' into proxy_proto_final
e13672f 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Merge branch 'master' into proxy_proto_final
d8c9cc6 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Contributor Author

Sorry, I think I need another stamp after merge

lizan
lizan previously approved these changes Nov 3, 2020
View reviewed changes
@alyssawilk
Merge branch 'master' into proxy_proto_final
5f25541 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@lizan lizan merged commit c4e9c4c into envoyproxy:master Nov 4, 2020
@rchernobelskiy
Copy link

Thanks you guys for the fix 🎉
Do you know if it's planned for 1.16.1 soon or not for a while?

@alyssawilk
Copy link
Contributor Author

@envoyproxy/stable-maintainers haven't reviewed the backport yet, but there's also not a set schedule for backport releases other than when we cut a CVE release.
@envoyproxy/maintainers were just discussing cadence here but currently I'm not sure we have enough folks signed on for stable release work to justify more than cutting a release every few months.

@cpakulski
Copy link
Contributor

I think it should be backported to 1.16 - starting this work.

@cpakulski cpakulski added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Nov 6, 2020
cpakulski pushed a commit to cpakulski/envoy that referenced this pull request Nov 10, 2020
@alyssawilk @cpakulski
proxy_proto: fixing hashing bug (envoyproxy#13768)
dc80aed 
Fix a bug where the transport socket options for the first downstream got reused for subsequent upstream connections.

Risk Level: low
Testing: new integration test
Docs Changes: n/a
Release Notes:
Platform Specific Features:
Fixes envoyproxy#13659

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
lizan pushed a commit that referenced this pull request Nov 11, 2020
@cpakulski
backport to rel-1.16: proxy_proto - fixing hashing bug #13768 (#13966)
cb4c1af 
Fix a bug where the transport socket options for the first downstream got reused for subsequent upstream connections.

Risk Level: low
Testing: new integration test
Docs Changes: n/a
Release Notes:
Platform Specific Features:
Fixes #13659

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>
guyco-redis added a commit to RedisLabs/envoy that referenced this pull request Nov 11, 2020
@guyco-redis @cpakulski
sync from envoyproxy/envoy (#3)
719e1ea 
* backport: Prevent SEGFAULT when disabling listener (envoyproxy#13515) (envoyproxy#13882)

* Prevent SEGFAULT when disabling listener (envoyproxy#13515)

This prevents the stop_listening overload action from causing
segmentation faults that can occur if the action is enabled after the
listener has already shut down.

Signed-off-by: Alex Konradi <akonradi@google.com>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

* backport to rel-1.16: proxy_proto - fixing hashing bug envoyproxy#13768 (envoyproxy#13966)

Fix a bug where the transport socket options for the first downstream got reused for subsequent upstream connections.

Risk Level: low
Testing: new integration test
Docs Changes: n/a
Release Notes:
Platform Specific Features:
Fixes envoyproxy#13659

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Co-authored-by: Christoph Pakulski <christoph@tetrate.io>
@cpakulski
Copy link
Contributor

Backported to 1.16. Removing backport/approved label.

@cpakulski cpakulski removed the backport/approved Approved backports to stable releases label Nov 13, 2020
@alyssawilk alyssawilk deleted the proxy_proto_final branch June 10, 2021 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizan lizan lizan approved these changes

@asraa asraa Awaiting requested review from asraa

@htuch htuch Awaiting requested review from htuch

@mattklein123 mattklein123 Awaiting requested review from mattklein123

@PiotrSikora PiotrSikora Awaiting requested review from PiotrSikora

Assignees

@lizan lizan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proxy protocol transport socket intermittently injects wrong addresses
4 participants
@alyssawilk @rchernobelskiy @cpakulski @lizan