NAT Signalling (Hole Punching) should be Fire & Forget, Coalesced, and Secured with Signatures

[tegefaulkes]

Description

Currently the way NAT signalling is implemented is pretty simplistic. Some optimisations and security improvements needs to be made. There are 3 main improvements that needs to be made.

1. fire and forget
2. Signal attempt coalescing.
3. Secure signal requests

fire and forget

Currently, when a signalling request is made, the RPC call remains active for the duration of the signalling request. This forms a chain of active RPC requests between the requester and the relay, as well as between the relay and the target. We want to refactor this so that the RPC request is only active while initiating the relay request.

To this end, signalling and hole punching attempts need to be back-grounded. any RPC request to relay a signal needs to return immediately and attempt the signal relay in the background.

In the same vein, when the target receives the request, it needs to return and do the hole punch attempts in the background.

Signal attempt coalescing

If multiple requests are made for the same hole punch attempt, then these attempts need to be coalesced together. This can be applied to both the signalling and hole punching attempts.

For each stage we need to track the signalling and punching attempts in a map. The map can be uniquely keyed with the nodeIds of the requester and requestee node concatinated together as a string. If the domain is shut down then these need to be cancelled.

Secure signal requests

Related: #148

We need a way to securely verify that the information of the request is correct and that the target of the hole punch messages made the request in the first place. There has been some discussion on this in #148.

Issues Fixed

• Fixes Authenticate the sender of a hole-punching signalling message #148

Tasks

• 1. Make RPC requests for signalling and hole punch requests non-blocking
• 2. Coalesce signalling and hole punching requests to prevent duplication of work
• 3. Manage back-grounded requests, Allow cancellation
• 4. Apply authentication to requests. Authenticate the sender of a hole-punching signalling message #148

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[ghost]

👇 Click on the image for a new way to code review

Legend

[tegefaulkes]

I'm thinking of breaking up the NodesHolePunchMessageSendHandler into two separate handlers. A signal request and a hole punch request. Right now both kinds of request are done in the same handler. The action it takes switches based on the message contents.

Right now these requests are only made through a single hop on a seed node. Later on with the #182 plan a hole punch will only be done with a single hop. So splitting the logic into two kinds of requests should simplify things.

[tegefaulkes]

Mostly done, Just need to fix some tests and then work on the authentication logic. I think this may need to be speced out a little more.

[tegefaulkes]

Looking into authentication now.

[tegefaulkes]

New issue at #556 for proof of work limiting.

[tegefaulkes]

Pretty much done now, I'm going to do a review.

[tegefaulkes]

General review for reference. Just added some notes for changes.

[tegefaulkes]

There are some tmp errors that need to be cleaned up.

[tegefaulkes]

Pending CI and merge. I'm considering this done now.

I'll move on to the configuration PR next. Then MDNS, node graph changes and final codebase cleanup. From there its testnet testing time.

[CMCDragonkai]

Tests pass but there's a resource leak in PK. This is probably the problem that occurred before. Resource leaks is a critical problem you need to have a look into this at a deeper level.

[CMCDragonkai]

I think this PR requires more work.

1. I don't understand how your fire and forget is achieved here. Where is this background processing happening if even required?
2. I think the resource leak needs to solved before committing any more code to staging.
3. I don't see any authentication happening right now so Authenticate the sender of a hole-punching signalling message #148 is not being solved.
4. Make the rate limiter a directory with 2 files so it can be used generically.

[CMCDragonkai]

Names of the 2 separate handlers:

• nodesConnectionSignalingInitial
• nodesConnectionSignalingFinal

Best names we could come up for now, and fits the SOV order.

[CMCDragonkai]

Changed to:

• nodesConnectionSignalInitial
• nodesConnectionSignalFinal

More verby.

[tegefaulkes]

I just need to fix one of the tests files not exiting the process. Besides that the rest that needs to be done like the message signing, authentication and handling background promises can be addressed later. We're putting this PR on hold until we work some things out.

[tegefaulkes]

Ok, this PR is on hold for now.

[CMCDragonkai]

This needs to be rebased and work should start on this.

I'm getting wary of all the stop-gap solutions that are introducing resource leaks and memory leaks here. I think it's important any stop-gap solution isn't resulting a resource leak for quic and ws.

Also here too if there are background processes running. Make sure if you're not using the tasks domain that you have proper tracking of these promises plus signal cancellation.

We should make sure to have tests that test the signal cancellations/abrupt aborts of the code.

[CMCDragonkai]

Needs rebase. What's the status of this?

[tegefaulkes]

Rebased on staging now. It was tricky given the number of changes to staging since this was worked on. I had to branch off of staging and pull the changes needed from a diff.

I haven't fully tested it yet and it likely still needs some polish.

Currently it isn't actually doing any authentication because at the time I concluded that it's too easy to work around if you're a bad actor. But if we still want that I need to apply signing and verification to the requests.

[CMCDragonkai]

Currently it isn't actually doing any authentication because at the time I concluded that it's too easy to work around if you're a bad actor. But if we still want that I need to apply signing and verification to the requests.

We still need auditability here so you should add in the signed requests. The signed requests is a quick add, and should be important for our metrics and observability.

[tegefaulkes]

Finished cleaning up and testing. I'll add signing and verification now.

[tegefaulkes]

Everything is done now. I need to go over the existing review comments to see if they're still relevant.

[tegefaulkes]

Ok, this is ready to merge.

[CMCDragonkai]

We need an explanation preferably a diagram on how all of the rate limiting, semaphores, active promise tracking actually work together, this code is quite tricky and will easily tech debt if we don't understand it.

[CMCDragonkai]

@tegefaulkes diagram?

[tegefaulkes]

I've been meaning to get to this. Just been side-tracked with other issues at the moment.