Implement MNAUTH and allow unlimited inbound MN connections#2790
Merged
codablock merged 7 commits intodashpay:developfrom Mar 22, 2019
Merged
Implement MNAUTH and allow unlimited inbound MN connections#2790codablock merged 7 commits intodashpay:developfrom
codablock merged 7 commits intodashpay:developfrom
Conversation
UdjinM6
reviewed
Mar 21, 2019
UdjinM6
left a comment
There was a problem hiding this comment.
Few things:
- let's keep
Hashsuffix to avoid confusion (I pointed just at few places the issue occurs, not to be committed from suggestions directly); - probably a copy/paste mistake when filling
pubKeysset.
We will later need the proRegTxHash
Having serialization and deserialization in the same specialized template results in compilation failures due to the "if(for_read)" branch.
This allows masternodes to authenticate themself.
Give fresh connections some time to do the VERSION/VERACK handshake and an optional MNAUTH when it's a masternode. When an MNAUTH happened, the incoming connection is then forever protected against eviction. If a timeout of 1 second occurs or the first message after VERACK is not MNAUTH, the node is not protected anymore and becomes eligable for eviction.
… same one Now that incoming connections from MNs authenticate them self, we can avoid connecting to the same MNs through intra-quorum connections.
Author
|
Rebased on develop to fix merge conflicts. Needs re-ACK |
| return; | ||
| } | ||
|
|
||
| if (strCommand == NetMsgType::MNAUTH) { |
Member
There was a problem hiding this comment.
why isn't it assumed this is the case? I would think this method is only called when that check is true
Author
There was a problem hiding this comment.
Nope, it's called for every incoming message. This follows the convention that we use for all Dash specific message handling. For example, everything related to DKGs is handled in CDKGSessionManager::ProcessMessage and it by itself checks which message it actually is. This might look a bit strange in the cases where it's only one message that we're interested in, but I prefer to use the same style/convention for all cases.
UdjinM6
added a commit
to UdjinM6/dash
that referenced
this pull request
Jan 31, 2020
We could be reading multiple messages from a socket buffer at once _without actually processing them yet_ which means that `fSuccessfullyConnected` might not be switched to `true` at the time we already parsed `VERACK` message and started to parse the next one. This is basically a false positive and we drop a legit node as a result even though the order of messages sent by this node was completely fine. To fix this I partially reverted dashpay#2790 (where the issue was initially introduced) and moved the logic for tracking the first message into ProcessMessage instead.
UdjinM6
added a commit
that referenced
this pull request
Feb 1, 2020
We could be reading multiple messages from a socket buffer at once _without actually processing them yet_ which means that `fSuccessfullyConnected` might not be switched to `true` at the time we already parsed `VERACK` message and started to parse the next one. This is basically a false positive and we drop a legit node as a result even though the order of messages sent by this node was completely fine. To fix this I partially reverted #2790 (where the issue was initially introduced) and moved the logic for tracking the first message into ProcessMessage instead.
UdjinM6
added a commit
to UdjinM6/dash
that referenced
this pull request
Feb 3, 2020
We could be reading multiple messages from a socket buffer at once _without actually processing them yet_ which means that `fSuccessfullyConnected` might not be switched to `true` at the time we already parsed `VERACK` message and started to parse the next one. This is basically a false positive and we drop a legit node as a result even though the order of messages sent by this node was completely fine. To fix this I partially reverted dashpay#2790 (where the issue was initially introduced) and moved the logic for tracking the first message into ProcessMessage instead.
FornaxA
pushed a commit
to ioncoincore/ion
that referenced
this pull request
Jul 6, 2020
We could be reading multiple messages from a socket buffer at once _without actually processing them yet_ which means that `fSuccessfullyConnected` might not be switched to `true` at the time we already parsed `VERACK` message and started to parse the next one. This is basically a false positive and we drop a legit node as a result even though the order of messages sent by this node was completely fine. To fix this I partially reverted dashpay#2790 (where the issue was initially introduced) and moved the logic for tracking the first message into ProcessMessage instead. Signed-off-by: cevap <dev@i2pmail.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implements MNAUTH, which allows us to verify that an incoming connection is indeed from another MN. This in turn allows us to exclude inbound MN connections from limit checks and at the same time protect these connections from eviction.
This is required to ensure that intra-quorum connections stay active as long as they are needed and never get evicted due to MNs being spammed with inbound connections.
The reason we can exclude authenticated MN connections from eviction is because these are naturally limited. MNAUTH will drop any previous connections that were authenticated with the same MN, which means that each MN can cause only one incoming authenticated (non-evictable) connection.
MNAUTH is also sent/verified for outgoing connections, this is however not of interest when it comes to the inbound connection eviction and DoS (or network partition) protection. It is however useful to avoid unnecessarily connecting to a MN that already connected to us when ensuring intra-quorum connections.