forked from bitcoin/bitcoin
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 MNAUTH and allow unlimited inbound MN connections #2790
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
38eb76d
Sort evo/* source files in Makefile.am
codablock b8af5ff
Keep track of proRegTxHash in CConnman::masternodeQuorumNodes map
codablock 60225d1
Fix serialization of std::tuple with const rvalue elements
codablock 5ead24e
Implement MNAUTH message
codablock 6bb8958
Protect fresh incoming connections for a second from eviction
codablock fe811b9
Avoid connecting to masternodes if an incoming connection is from the…
codablock d58789f
Apply review suggestions
codablock File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,141 @@ | ||
// Copyright (c) 2019 The Dash Core developers | ||
// Distributed under the MIT software license, see the accompanying | ||
// file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
||
#include "mnauth.h" | ||
|
||
#include "activemasternode.h" | ||
#include "evo/deterministicmns.h" | ||
#include "masternode-sync.h" | ||
#include "net.h" | ||
#include "net_processing.h" | ||
#include "netmessagemaker.h" | ||
#include "validation.h" | ||
|
||
#include <unordered_set> | ||
|
||
void CMNAuth::PushMNAUTH(CNode* pnode, CConnman& connman) | ||
{ | ||
if (!fMasternodeMode || activeMasternodeInfo.proTxHash.IsNull()) { | ||
return; | ||
} | ||
|
||
uint256 signHash; | ||
{ | ||
LOCK(pnode->cs_mnauth); | ||
if (pnode->receivedMNAuthChallenge.IsNull()) { | ||
return; | ||
} | ||
// We include fInbound in signHash to forbid interchanging of challenges by a man in the middle. This way | ||
// we protect ourself against MITM in this form: | ||
// node1 <- Eve -> node2 | ||
// It does not protect against: | ||
// node1 -> Eve -> node2 | ||
// This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff | ||
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound)); | ||
} | ||
|
||
CMNAuth mnauth; | ||
mnauth.proRegTxHash = activeMasternodeInfo.proTxHash; | ||
mnauth.sig = activeMasternodeInfo.blsKeyOperator->Sign(signHash); | ||
|
||
LogPrint("net", "CMNAuth::%s -- Sending MNAUTH, peer=%d\n", __func__, pnode->id); | ||
|
||
connman.PushMessage(pnode, CNetMsgMaker(pnode->GetSendVersion()).Make(NetMsgType::MNAUTH, mnauth)); | ||
} | ||
|
||
void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataStream& vRecv, CConnman& connman) | ||
{ | ||
if (!fMasternodeMode) { | ||
return; | ||
} | ||
|
||
if (!masternodeSync.IsBlockchainSynced()) { | ||
// we can't really verify MNAUTH messages when we don't have the latest MN list | ||
return; | ||
} | ||
|
||
if (strCommand == NetMsgType::MNAUTH) { | ||
CMNAuth mnauth; | ||
vRecv >> mnauth; | ||
|
||
{ | ||
LOCK(pnode->cs_mnauth); | ||
// only one MNAUTH allowed | ||
if (!pnode->verifiedProRegTxHash.IsNull()) { | ||
LOCK(cs_main); | ||
Misbehaving(pnode->id, 100); | ||
return; | ||
} | ||
} | ||
|
||
if (mnauth.proRegTxHash.IsNull() || !mnauth.sig.IsValid()) { | ||
LOCK(cs_main); | ||
Misbehaving(pnode->id, 100); | ||
return; | ||
} | ||
|
||
auto mnList = deterministicMNManager->GetListAtChainTip(); | ||
auto dmn = mnList.GetValidMN(mnauth.proRegTxHash); | ||
if (!dmn) { | ||
LOCK(cs_main); | ||
Misbehaving(pnode->id, 10); | ||
// in case he was unlucky and not up to date, let him retry the whole verification process | ||
pnode->fDisconnect = true; | ||
return; | ||
} | ||
|
||
uint256 signHash; | ||
{ | ||
LOCK(pnode->cs_mnauth); | ||
// See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection) | ||
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->sentMNAuthChallenge, !pnode->fInbound)); | ||
} | ||
|
||
if (!mnauth.sig.VerifyInsecure(dmn->pdmnState->pubKeyOperator, signHash)) { | ||
LOCK(cs_main); | ||
Misbehaving(pnode->id, 10); | ||
// in case he was unlucky and not up to date, let him retry the whole verification process | ||
pnode->fDisconnect = true; | ||
return; | ||
} | ||
|
||
connman.ForEachNode([&](CNode* pnode2) { | ||
if (pnode2->verifiedProRegTxHash == mnauth.proRegTxHash) { | ||
LogPrint("net", "CMNAuth::ProcessMessage -- Masternode %s has already verified as peer %d, dropping old connection. peer=%d\n", | ||
mnauth.proRegTxHash.ToString(), pnode2->id, pnode->id); | ||
pnode2->fDisconnect = true; | ||
} | ||
}); | ||
|
||
{ | ||
LOCK(pnode->cs_mnauth); | ||
pnode->verifiedProRegTxHash = mnauth.proRegTxHash; | ||
pnode->verifiedPubKeyHash = dmn->pdmnState->pubKeyOperator.GetHash(); | ||
} | ||
|
||
LogPrint("net", "CMNAuth::%s -- Valid MNAUTH for %s, peer=%d\n", __func__, mnauth.proRegTxHash.ToString(), pnode->id); | ||
} | ||
} | ||
|
||
void CMNAuth::NotifyMasternodeListChanged(const CDeterministicMNList& newList) | ||
{ | ||
std::unordered_set<uint256> pubKeys; | ||
g_connman->ForEachNode([&](const CNode* pnode) { | ||
LOCK(pnode->cs_mnauth); | ||
if (!pnode->verifiedProRegTxHash.IsNull()) { | ||
pubKeys.emplace(pnode->verifiedPubKeyHash); | ||
} | ||
}); | ||
newList.ForEachMN(true, [&](const CDeterministicMNCPtr& dmn) { | ||
pubKeys.erase(dmn->pdmnState->pubKeyOperator.GetHash()); | ||
}); | ||
g_connman->ForEachNode([&](CNode* pnode) { | ||
LOCK(pnode->cs_mnauth); | ||
if (!pnode->verifiedProRegTxHash.IsNull() && pubKeys.count(pnode->verifiedPubKeyHash)) { | ||
LogPrint("net", "CMNAuth::NotifyMasternodeListChanged -- Disconnecting MN %s due to key changed/removed, peer=%d\n", | ||
pnode->verifiedProRegTxHash.ToString(), pnode->id); | ||
pnode->fDisconnect = true; | ||
} | ||
}); | ||
} |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
// Copyright (c) 2019 The Dash Core developers | ||
// Distributed under the MIT software license, see the accompanying | ||
// file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
||
#ifndef DASH_MNAUTH_H | ||
#define DASH_MNAUTH_H | ||
|
||
#include "bls/bls.h" | ||
#include "serialize.h" | ||
|
||
class CConnman; | ||
class CDataStream; | ||
class CDeterministicMN; | ||
class CDeterministicMNList; | ||
class CNode; | ||
class UniValue; | ||
|
||
/** | ||
* This class handles the p2p message MNAUTH. MNAUTH is sent directly after VERACK and authenticates the sender as a | ||
* masternode. It is only sent when the sender is actually a masternode. | ||
* | ||
* MNAUTH signs a challenge that was previously sent via VERSION. The challenge is signed differently depending on | ||
* the connection being an inbound or outbound connection, which avoids MITM of this form: | ||
* node1 <- Eve -> node2 | ||
* while still allowing: | ||
* node1 -> Eve -> node2 | ||
* | ||
* This is fine as we only use this mechanism for DoS protection. It allows us to keep masternode connections open for | ||
* a very long time without evicting the connections when inbound connection limits are hit (non-MNs will then be evicted). | ||
* | ||
* If we ever want to add transfer of sensible data, THIS AUTHENTICATION MECHANISM IS NOT ENOUGH!! We'd need to implement | ||
* proper encryption for these connections first. | ||
*/ | ||
|
||
class CMNAuth | ||
{ | ||
public: | ||
uint256 proRegTxHash; | ||
CBLSSignature sig; | ||
|
||
public: | ||
ADD_SERIALIZE_METHODS; | ||
|
||
template <typename Stream, typename Operation> | ||
inline void SerializationOp(Stream& s, Operation ser_action) | ||
{ | ||
READWRITE(proRegTxHash); | ||
READWRITE(sig); | ||
} | ||
|
||
static void PushMNAUTH(CNode* pnode, CConnman& connman); | ||
static void ProcessMessage(CNode* pnode, const std::string& strCommand, CDataStream& vRecv, CConnman& connman); | ||
static void NotifyMasternodeListChanged(const CDeterministicMNList& newList); | ||
}; | ||
|
||
|
||
#endif //DASH_MNAUTH_H |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why isn't it assumed this is the case? I would think this method is only called when that check is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍