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.

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

add validation of allowed CrdsValues in gossip #4764

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

alexpyattaev
Copy link

Protocol enum variants can now be pruned better in Sanitize impl

Problem

Sanitize implementation allowed certain invalid PullRequest variants to travel through the code

Summary of Changes

  • Expand Sanitize impl to eliminate values that should not be present in valid traffic.

Copy link

@gregcusack gregcusack left a comment

Choose a reason for hiding this comment

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

i know its just a draft pr rn. big question is if we can actually get rid of LegacyVersion

gossip/src/protocol.rs Outdated Show resolved Hide resolved
gossip/src/protocol.rs Outdated Show resolved Hide resolved
gossip/src/protocol.rs Outdated Show resolved Hide resolved
@alexpyattaev alexpyattaev marked this pull request as ready for review February 4, 2025 07:14
@behzadnouri
Copy link

This can amplify traffic if someone pushes LegacyVersion to the cluster.
Your node sends pull requests out, other nodes respond with LegacyVersions that you are missing (because they don't drop them yet), then your node drops all LegacyVersions received immediately. Next pull request, same thing happens again.

So your node keeps receiving same LegacyVersion in pull responses repeatedly and drops them every time.

We need to wait for #3653 to hit mainnet, so that nodes do not return deprecated values in pull responses.
Once the cluster has upgraded to #3653 we can drop all deprecated values during deserializiation or sanity checks.

Alex Pyattaev and others added 4 commits February 4, 2025 22:18
Protocol enum variants can now be pruned better in Sanitize impl
Style change for PullRequest sanitize.

Co-authored-by: Greg Cusack <[email protected]>
@alexpyattaev
Copy link
Author

We need to wait for #3653 to hit mainnet, so that nodes do not return deprecated values in pull responses. Once the cluster has upgraded to #3653 we can drop all deprecated values during deserializiation or sanity checks.

So we allow it in PullResponses for now, got it. Fixed in 814abfa

@behzadnouri
Copy link

We need to wait for #3653 to hit mainnet, so that nodes do not return deprecated values in pull responses. Once the cluster has upgraded to #3653 we can drop all deprecated values during deserializiation or sanity checks.

So we allow it in PullResponses for now, got it. Fixed in 814abfa

We need to allow it for push messages as well.
Otherwise you receive the push message, and you drop it.
So your crds table misses that entry, and you will receive an extra pull response next time around you send the pull request; even though you had already received the message from push before (but you dropped it).
If someone spams with LegacyVersion, this can double the traffic.

@alexpyattaev
Copy link
Author

We need to wait for #3653 to hit mainnet, so that nodes do not return deprecated values in pull responses. Once the cluster has upgraded to #3653 we can drop all deprecated values during deserializiation or sanity checks.

So we allow it in PullResponses for now, got it. Fixed in 814abfa

We need to allow it for push messages as well.

Done in e4fc336, also added comment so others do not forget about this caveat.

behzadnouri
behzadnouri previously approved these changes Feb 4, 2025
val.sanitize()
}
Protocol::PushMessage(_, val) => {
// PullMessage is allowed to carry anything in its CrdsData, including deprecated Crds

Choose a reason for hiding this comment

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

PushMessage instead of "PullMessage"

Copy link
Author

Choose a reason for hiding this comment

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

oops... corrected.

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