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

Implement peer blacklist #149

Merged
merged 7 commits into from
Jan 17, 2019
Merged

Implement peer blacklist #149

merged 7 commits into from
Jan 17, 2019

Conversation

vyzo
Copy link
Collaborator

@vyzo vyzo commented Jan 15, 2019

Adds a peer blacklist, which is checked before doing any processing on an incoming message.

@vyzo vyzo force-pushed the feat/blacklist branch 4 times, most recently from 157a7dd to 7f6f0f1 Compare January 15, 2019 17:01
@vyzo
Copy link
Collaborator Author

vyzo commented Jan 15, 2019

sorry for the force pushing mess, i was trying to get the test coverage right... it should be ready for review now.

@vyzo
Copy link
Collaborator Author

vyzo commented Jan 16, 2019

there is a hole currently in that the incoming stream is not tracked; this allows a blacklisted peer to replay old valid messages from other peers.

@vyzo
Copy link
Collaborator Author

vyzo commented Jan 16, 2019

actually it's ok, we do the blacklist check on the node that forwarded the message. but we should also check the message origin as well, as it might be blacklisted.

@vyzo
Copy link
Collaborator Author

vyzo commented Jan 16, 2019

the issue is that the validator can't see the peer that forwarded the message, so it can't make blacklist decisions for that peer.

@vyzo
Copy link
Collaborator Author

vyzo commented Jan 16, 2019

follow up in #150.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

We should probably use an LRU to prevent bad peers from forcing us to leak memory.

pubsub.go Outdated
@@ -179,6 +183,8 @@ func NewPubSub(ctx context.Context, h host.Host, rt PubSubRouter, opts ...Option
topics: make(map[string]map[peer.ID]struct{}),
peers: make(map[peer.ID]chan *RPC),
topicVals: make(map[string]*topicVal),
blacklist: make(map[peer.ID]struct{}),
Copy link
Member

Choose a reason for hiding this comment

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

May want to use an LRU. This could turn into a DoS vector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. As we discussed with why, let's make it an interface the user can pass with a default implementation using a map.

@vyzo
Copy link
Collaborator Author

vyzo commented Jan 17, 2019

Added a blacklist type, with map and lru cache backed implementations.
The blacklist can be configured with the WithBlacklist option.

@vyzo vyzo merged commit 87e2f56 into master Jan 17, 2019
@ghost ghost removed the in progress label Jan 17, 2019
@vyzo vyzo deleted the feat/blacklist branch January 17, 2019 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants