Repository navigation
Investigate means of authenticating a node ID when adding to NodeGraph #322
Description
Specification
With #344 more or less solved we need a way of authenticating when adding forward connections. this includes authenticating the old connections when a bucket fills up. In both cases we can so a pingNode to check if a node is alive. However the current implementation of pingNode is insufficient for the job as it only checks if a node is alive.
NodeManager.pingNode needs to be updated to match the following criteria;
- It needs to establish a proxy connection to the target. if a connection can be established then the target is considered alive. if the connection is established then we can consider it authenticated as well.
- It needs to take an optional address information for checking if direct connections without the nat bashing proceedure can be done. This will be useful for ICE
The NodeManager.setNode needs to be updated to do the following.
- when a bucket is full the oldest node needs to be verified with
pingNode. This is already done, just need to update ping for this. This will need to concurrently check alpha of the oldest connections. - Needs an optional force parameter to garentee adding the node.
NodeConnectionManager needs to be updated to;
- takes
NodeManageras part of start. - establishing a connection should trigger
NM.qeueSetNode
NodeManager needs to be updated to;
- startStop asyinc init pattern.
- implement a async queue for adding nodes to the nodeGraph.
After that we need to make sure all instance where we need to add a node just calls NodeManager.setNode. Where this is done still needs to be speced out but this may be outside the scope of this issue. Create a new issue for this?
Additional context
- another very relevant issue: Expunge any added "invalid" nodes from k-buckets #226
- Related Seed node not adding details of connecting node to its
NodeGraph#344 - Related Testnet Deployment #326
- Related Authenticate the sender of a hole-punching signalling message #148
Tasks
- update
NodeManager.pingNodeto match the above specification. - update
NodeManager.setNodeto match the above specification. - implement setnode queue. this is similar to the queue used by discovery.
- seperate pingNode out of setNode
- concurrently check alpha old nodes if a bucket is full when setting node.
- convert NCM and NM to startstop, they rely on each other now with NCM calling NM.qeueSetNode.
- establishing a NodeConnection should trigger setting a node in the nodeGraph by callong NM.qeueSetNode.
- Creating a new connection should not be blocked by setting node.
- Integrate into
pk nodes set(with--no-pingand--force) andpk nodes ping
Activity
tegefaulkes commentedon Mar 17, 2022
There are 4 cases where we add a node to the node graph.
Manually adding a node should ping the node before adding it to the
NodeGraph. In fact I think this is functionally identical to pinging a node in outcome except that manually adding the node should guarantee that the node is added to the graph by dropping an older node.Pinging a node will add the node to the graph but this should be done in the CLI ping command's GRPC handler. having the
NodeManager.pingNodeis called to verify a node's 'aliveness' during ansetNodeso we can't have that attempting to set a node.Adding a node after connecting to it can be handled by the nodeConnectionManager in the
withcontext functions? or possible triggered by the forward proxy? need to explore this more. it would be nice to have this handled neatly in one place.receiving the connection will be verified by the reverse proxy and then added to the nodeGraph. this is addressed in #344
tegefaulkes commentedon Mar 23, 2022
WIth the changes in commit 7cc74d0 I added a check for if a node was alive with
pingNode. currently this is blocking and will timeout based on the proxies timeout which is usually 20 seconds. This will need to be fixed by letting us set a time out in the ping command. I think there was an issue planed for this? #353? but that's for the node connections, we will need a similar solution for the proxies stuff. New issue?In any case we ill need to connect to a node and check it's nodeId and certificates data. maybe we can create a new agent-agent RPC for getting a remote node's certificates and nodeId? isn't this already what
GRPCClientAgent.nodesChainDataGetdoes? we can verify the node id and MTLS on the client side as well.CMCDragonkai commentedon Mar 23, 2022
Yea the closest issue is #353, by creating a timeout for
NodeConnectionManager.I think however the proxies
openConnectionForwardandopenConnectionReverseshould also have timeouts that override the default timeout. This is already supported with thetimerparameter.Also relevant is #297 as background theory into the future.
CMCDragonkai commentedon Mar 23, 2022
Furthermore solving #353 would just reduce the amount of time waiting for a ping. To really solve the problem of blocking, is to remove blocking entirely. That is we wouldn't want to block a request to contact a Node or do operations against another Node due to pinging old nodes in order to maintain bucket limit.
I suggest that this logic should be non-blocking, and to do so, we can either do non-blocking in-memory by queuing up a job using JS's own event loop, or by queuing it up into the DB and run this as part of a background work as suggested by the longer-kademlia trie paper at the end.
CMCDragonkai commentedon Mar 23, 2022
Suggest creating a new issue for this non-blocking problem as it relates to #353.
tegefaulkes commentedon Mar 24, 2022
Ah I just remembered that the feature added in 7cc74d0 just checks that the old node is still alive. as mentioned above authenticating it still needs to be done at this stage and that's only really needed to make sure that the node hasn't changed identities. it should've been authenticated before it was added to the graph.
For adding a new node I think the expectation is that the authentication is done when connecting to it just before adding it to the nodeGraph. We could use the same method we use to authenticate the old nodes but we would be doubling up the action of connecting to the node since the action of adding a node is triggered by a connection to a node in the first place.
As to how the node is authenticated we would need to obtain the connection info from the newly combined
Proxyclass. Based on this comment #344 (comment) we will add a callback to theProxyclass that will cause a connection event in theEventBussystem. This will trigger adding the node to the node graph. Assuming that the proxy authenticates the connection before triggering the event then the only information the event needs is a tuple of(remoteNodeId, remoteHost, remotePort).tegefaulkes commentedon Mar 24, 2022
Ignore this, moved old spec back.
CMCDragonkai commentedon Mar 24, 2022
I think the proxy open connection already does authentication by checking the certificate. Therefore the needing to authenticate a node would only be when one is explicitly adding a node to the node graph.
Any authentication would need to be done prior to adding the node to the node graph.
One question is whether we should allow users to explicitly add nodes to the node manager/graph straight from the command line and therefore not have to be authenticated ahead of time. I think we should allow this as this is would primarily be used for special situations and debugging.
The node manager's add node method can schedule an authentication by scheduling a ping node.
I think the ping node should not just establish a proxy connection but also check the certificates which I believe the proxy does already. But you should check @tegefaulkes
CMCDragonkai commentedon Mar 24, 2022
We should re-use the ping node as the basis of "authenticating a node". It would then be used when refreshing buckets and dealing with filled buckets.
We could add a boolean flag during the add node method which controls whether to authenticate the node prior to adding.
tegefaulkes commentedon Mar 24, 2022
Right now the
setNodeassumes the node was already authenticated prior to callingsetNodesince this will be triggered by an already authenticated connection by theProxy.tegefaulkes commentedon Mar 24, 2022
Hmm.. the spec I created here relates more to issue #344 but it still relates this one partly. The scope this issue relates more to authenticating and adding nodes when connecting TO a node. Whereas 344 relates to authenticating and adding a node when receiving a connection FROM a node.
I hoped that I could handle adding nodes for
ForwardConnections the same way as theReverseConnections. But if we automatically try to add the old node when we establish the connection for a ping then that could lead to issues or it's too side effect-ful.So adding nodes based on reverse connections can be automatic but we may have to handle forward connections separately.
In any case, this Issue should focus more on the authenticating the forward connections and pinging the old nodes when we need to add a new node to a full bucket.
146 remaining items
feat: `NodeManager.setNode` authenticates the added node
fix: squash into ping node changes.
fix: setNode now does not ping the new node
feat: setNode concurrently pings multiple nodes
feat: nodeManager is now startStop
feat: async queueing for setting nodes
feat: establishing a `NodeConnection` adds the node to the nodeGraph
fix: `setNode` no longer adds node twice when `force` is true
feat: generic `SetNodeQueue` class for queuing `setNode` operations
fix: `syncNodeGraph` during agent startup is now non-blocking
fix: `NodeConnectionManager.syncNodeGraph` now pings nodes