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

Peers aren't banned for forwarding empty encrypted messages #5132

Closed
AaronFeickert opened this issue Jan 23, 2023 · 1 comment · Fixed by #5130
Closed

Peers aren't banned for forwarding empty encrypted messages #5132

AaronFeickert opened this issue Jan 23, 2023 · 1 comment · Fixed by #5130

Comments

@AaronFeickert
Copy link
Collaborator

AaronFeickert commented Jan 23, 2023 •
edited
Loading

Empty encrypted messages sent by peers are not allowed. However, sending an empty message is not a banned offence.

Because all nodes can detect this, forwarding such a message should result in a ban.
@AaronFeickert AaronFeickert mentioned this issue Jan 23, 2023
Merged
@AaronFeickert AaronFeickert changed the title Peers aren't banned for forwarding empty messages Peers aren't banned for forwarding empty encrypted messages Jan 24, 2023
@AaronFeickert
Copy link
Collaborator Author

After discussion with @sdbondi, it was decided that forwarding an empty non-encrypted message is not a banned offence. The relevant RFC should be updated to reflect this.

stringhandler pushed a commit that referenced this issue Jan 31, 2023
@AaronFeickert
fix(dht/test): ban peers who send empty encrypted messages (#5130)
86a9eaf 
Description
---
Bans peers who send empty encrypted messages. Significantly updates tests to check for more failure modes and assert ban status for each.

Closes [issue 5132](#5132).

Motivation and Context
---
An [earlier PR](#5123) introduces an error when a peer sends an empty encrypted message, which is not allowed. However, the peer was not banned.

Further, [another PR](#5129) updates the handling of unsigned encrypted messages to ensure that bans are done correctly, but does not update tests to check for the bug that led to it.

This PR updates the banning logic to ban a peer who forwards an empty encrypted message, which is always detectable.

It also significantly refactors and updates tests. For each relevant high-level message failure mode, we test for proper error detection. We also check for the proper ban status of the forwarding peer.

How Has This Been Tested?
---
[Who tests the testers?](https://en.wikipedia.org/wiki/Quis_custodiet_ipsos_custodes%3F)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(dht/test): ban peers who send empty encrypted messages AaronFeickert/tari
1 participant
@AaronFeickert