Add NAT traversal utilities #84
Conversation
|
STUN actually looks quite complicated, so I suggest we leave it as an enhancement and just go with TCP hole punching and TURN functionality for now. And I should say that STUN doesn't always find the type of NAT the private node is behind. |
robert-cronin
force-pushed
the
nat-traversal
branch
9 times, most recently
from
c33bfac
to
4edd2f3
Compare
robert-cronin
force-pushed
the
nat-traversal
branch
3 times, most recently
from
9ded781
to
493210d
Compare
|
Forgoing the QEMU testing for now, I believe this PR is ready for review. |
|
I remember you said your implementation deviates from the standard. Can you describe how your implementation deviates from STUN and TURN standards? What features are missing or differently implemented? With reference to pieces on the code too. |
|
There's a lot of files here. Can you generate a call graph diagram of the new modules and isolate to the NAT functionality. I think I mentioned things like: Before. But there's also xstate which you need to use to create protocol modelling. Can you also post the diagrams/visualizations in these in the PRs as comments. When I create such massive PRs, I always provide visual documentation along with it. Useful for later refactoring. |
|
There are some diagrams and explanation in the related issues, but I will copy them here for convenience and elaborate on the points of difference: Here is the general process for peer communication over a public TURN node: Stage 2: peerA opens a connection to the git server and relays all packets to and from peerC's TURN server Final stage: peerB wants to connect to peerA so it queries the known, public intermediary: peerC. PeerC responds with peerA's relayed address and peerB can communicate with peerA now via TURN server on peer B just as it normally could. |
|
Ok, but please generate the call graphs as well. I need to map it to the files as well. |
robert-cronin
force-pushed
the
nat-traversal
branch
3 times, most recently
from
5941ff8
to
2f08dc6
Compare
robert-cronin
force-pushed
the
nat-traversal
branch
2 times, most recently
from
a9f73ff
to
dd26034
Compare
|
Ok after going through the codebase. I have 131 comments on various parts of it. One part I didn't review which was the meat of this PR is the actual turn server and hole punching logic. I want to go through that in detail with you together. The other comments above are all Quality of Life improvements and abstraction/modelling improvements and design improvements and standardisation and Quality Assurance stuff. All stuff to be focused on to reduce the entropy of the codebase before moving ahead to add more features. Let's schedule another time to go through it. Perhaps Monday or later. First I need that UI case flow diagrams and then we go through it. The very next PR I want to focus on:
All in all, this is amazing amount of work. |
|
Oh right yea, testing `npm build`?
…On 14/09/2020 1:47 pm, Robbie Cronin wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In .gitlab-ci.yml
<#84 (comment)>:
>
+build_npm:
+ image: registry.gitlab.com/matrixai/engineering/maintenance/gitlab-runner
+ stage: build
+ script:
+ - nix-shell --packages nodejs --run "npm install"
this is not to test |npm install|, this is to test building via npm,
this can pick up a lot of errors in the code actually, including
import errors/type errors. But like you said, it probably will be
picked up in |nix-build| anyway, so I guess we could remove it.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHJ6OPRMVMFVTAX54XTSFWG6RANCNFSM4QCTOA4Q>.
|
yeah thats right, but I agree that the errors will just be picked up in the nix build anyway, so npm build is kind of redundant. |
|
Usually we "multiplex channels" on a connection. So there's one
connection from 1 place to another place. But there may be multiple
channels.
This is arbitrary but that's how I've seen it been used.
So `PeerConnection` is fine for this actually. But we need to
differentiate client objects as containers of RPC methods versus client
interfaces which mostly contain types. If you have a client object
wrapping another client object, then it should be differentiated as
something as a lower level client vs a higher level client. Usually I
have shorter names for higher level clients, and longer names for lower
level clients if there's a nameclash.
…On 9/14/20 4:39 PM, Robbie Cronin wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/lib/peers/peer-connection/PeerConnection.ts
<#84 (comment)>:
> + this.peerManager = peerManager;
+
+ const pkiInfo = keyManager.PKIInfo;
+ if (pkiInfo.caCert && pkiInfo.cert && pkiInfo.key) {
+ this.credentials = grpc.credentials.createSsl(pkiInfo.caCert, pkiInfo.key, pkiInfo.cert);
+ } else {
+ this.credentials = grpc.credentials.createInsecure();
+ }
+ }
+
+ private async connectDirectly(): Promise<PeerClient> {
+ // try to create a direct connection
+ if (this.getPeerInfo().connectedAddress) {
+ // direct connection attempt
+ const address = this.getPeerInfo().connectedAddress!;
+ const peerClient = new PeerClient(address.toString(), this.credentials);
Just for clarity here, |PeerClient| is the client that is
auto-generated by |protoc|, i.e. the actual grpc client.
|PeerConnection| is the class abstracted over the top that contains
the logic to select which of the 3 channels to use for connection:
direct connection/relayed connection/UDP hole punched connection.
Perhaps we could rename |PeerConnection| to something more appropriate
or even add it into the |PeerManager| class but I think it is a good
separation of concerns. The |PeerServer| is another abstraction over
the grpc server stub. I will add a note to the domain modelling issue
about renaming of these classes to something less confusing.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHMR6ZYZCWGY3NRAD6DSFW3BPANCNFSM4QCTOA4Q>.
|
|
Surely there's a promise combinator for this?
…On 9/14/20 4:44 PM, Robbie Cronin wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/lib/peers/peer-connection/PeerConnection.ts
<#84 (comment)>:
> + this.connected = true;
+ return peerClient;
+ } else if (!this.getPeerInfo().relayPublicKey) {
+ throw Error('peer does not have relay public key specified');
+ } else {
+ throw Error('peer is already connected');
+ }
+ }
+
+ async connectFirstChannel() {
+ return await new Promise<PeerClient>((resolve, reject) => {
+ const promiseList = [this.connectDirectly(), this.connectHolePunch(), this.connectRelay()];
+
+ const errorList: Error[] = [];
+ for (const promise of promiseList) {
+ promise
well part of this script is a promise utility: resolving on the first
promise and rejecting if all reject. This is an attempt to try and
connect to the first successful address and only reject if none work.
This could probably be brought out into a utility function. As for
async await, I can see a way to do that now with trycatch blocks so I
will try that now to see if it achieves the same thing.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHJA5PAJRKVDZLEVNULSFW3TBANCNFSM4QCTOA4Q>.
|
|
Then call it `promiseAny` then!
…On 9/14/20 4:56 PM, Robbie Cronin wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/lib/peers/peer-connection/PeerConnection.ts
<#84 (comment)>:
> + this.connected = true;
+ return peerClient;
+ } else if (!this.getPeerInfo().relayPublicKey) {
+ throw Error('peer does not have relay public key specified');
+ } else {
+ throw Error('peer is already connected');
+ }
+ }
+
+ async connectFirstChannel() {
+ return await new Promise<PeerClient>((resolve, reject) => {
+ const promiseList = [this.connectDirectly(), this.connectHolePunch(), this.connectRelay()];
+
+ const errorList: Error[] = [];
+ for (const promise of promiseList) {
+ promise
I remember now, async/await doesn't work here. The point is you don't
want to wait for any particular promise, you want them to race as in
|Promise.race|. The only issue with that convenience function is that
it rejects when any promise rejects as well, here we want a resolve
race but not a reject race.
The |bluebird| promise library has this functionality in the form of
|Promise.any(..)| but I decided to just implement this ourselves to
save a dependency.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHJ35CSRQITYNYE3SXLSFW5CRANCNFSM4QCTOA4Q>.
|
|
HMAC can be useful for maintaining authenticity of a message. TLS is
more for encryption. But certs does ensure authenticity too. But HMAC
can be useful for delay-tolerance.
…On 9/14/20 5:06 PM, Robbie Cronin wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/lib/peers/peer-connection/PeerConnection.ts
<#84 (comment)>:
> + }
+ return this.peerManager.getPeer(this.publicKey)!;
+ }
+
+ private async sendPingRequest(timeout?: number): Promise<boolean> {
+ // eslint-disable-next-line
+ return await new Promise<boolean>(async (resolve, reject) => {
+ try {
+ if (timeout) {
+ setTimeout(() => reject('ping timed out'), timeout);
+ }
+
+ const challenge = randomBytes(16).toString('base64');
+
+ // encode request
+ const peerRequest = await this.encodeRequest(SubServiceType.PING_PEER, stringToProtobuf(challenge));
I think TLS might use HMAC under the hood? but it's a good idea to
confirm this.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHPNFAJUTJWZN5Y25CTSFW6JBANCNFSM4QCTOA4Q>.
|
|
Yeah so perhaps the names can be Yeah channels are definitely multiplexed over a single
|
|
Done!
|
|
Sonar ping is not a scan.
Sonar ping is a one way thing. A ping. The response is not guaranteed.
It's closer to "broadcast".
If you want to check liveness, use the "heartbeat" analogy. Check
heartbeat or check liveness.
…On 9/14/20 5:12 PM, Robbie Cronin wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/lib/peers/peer-connection/PeerServer.ts
<#84 (comment)>:
> + Buffer.from(publickey),
+ );
+ const signedResponse = await this.keyManager.signData(encryptedResponse);
+ const subMessage = signedResponse.toString();
+
+ // composes peer message
+ const peerResponse = new PeerMessage();
+ peerResponse.setPublickey(this.peerManager.peerInfo.publicKey);
+ peerResponse.setType(type);
+ peerResponse.setSubmessage(subMessage);
+
+ // return peer response
+ callback(null, peerResponse);
+ }
+
+ private async handlePing(request: Uint8Array): Promise<Uint8Array> {
yeah you're right, this is more involved in checking if the peer node
is alive which could probably be done other ways with gRPC. As opposed
to our sonar ping which is more like scanning the multicast broadcast
address for broadcasting peers right?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHORCCND6JP2RHR7RW3SFW66NANCNFSM4QCTOA4Q>.
|
|
isn't sonar ping a kind of scan? to follow the analogy, you're sending out interrogation packets (e.g. sound waves) and listening to what comes back which tells you what is out there. In our broadcast situation, we're skipping the 'sending out' phase and just listening to what is out there on the multicast address. I guess you could say its more akin to ADS-B since the broadcast is active and the listening is passive. So when we talk about our sonar ping, that function as far as the user is concerned is listening to the multicast address to see what/who is out there on the LAN, right? |
|
Listening just means listening on the multicast address.
Broadcasting can still be done on the multicast address.
Which is why we can retain the idea of ping for this feature.
…On 9/14/20 5:28 PM, Robbie Cronin wrote:
isn't sonar ping a kind of |discovery|? to follow the analogy, you're
sending out interrogation packets (e.g. sound waves) and listening to
what comes back which tells you what is out there. In our broadcast
situation, we're skipping the 'sending out' phase and just listening
to what is out there on the multicast address.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHJ5NHLZZPRSC7GM3VDSFXAYFANCNFSM4QCTOA4Q>.
|
|
The `bin` is fairly standard name for folders containing executables.
Nothing to do with whether the files are binary are not. It's more
standard than `cli`. Nix uses this as well as do all the other projects.
…On 9/15/20 3:45 PM, Robbie Cronin wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/cli/Peers.ts
<#84 (comment)>:
> +
+function makeAddPeerCommand() {
+ return new commander.Command('add')
+ .description('add a new peer to the store')
+ .option('-b, --base64 <base64>', 'decode the peer info from a base64 string')
+ .option('-p, --public-key <publicKey>', 'path to the file which contains the public key')
+ .option('-a, --connected-address <connectedAddress>', 'public address on which the node can be contacted')
+ .option('-r, --relay-key <relayKey>', 'path to the file which contains the public key of the relay peer')
+ .option('--node-path <nodePath>', 'node path')
+ .option('-v, --verbose', 'increase verbosity level by one')
+ .action(
+ actionRunner(async (options) => {
+ const client = PolykeyAgent.connectToAgent();
+ const status = await client.getAgentStatus();
+ if (status != 'online') {
+ throw Error(`agent status is: ${status}`);
oh okay, I just figured since they are source files and not really
binary files that bin isn't really accurate.
Yeah I guess the file name shouldn't really be uppercase since it's
not exporting a class but I think the cli and the lib are different
things, it doesn't make sense to me to have the |peer| command inside
the library as it doesn't compile any command like that.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#84 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHN75HCIO22XWSKOKO3SF35ODANCNFSM4QCTOA4Q>.
|
|
Reviewed the entire NAT stuff. Lots of things to work on. Please summarize the notes and things to do next with a new issue linked to #84.
|
|
The final function taking options can be modularised.
…On 16 September 2020 1:12:57 pm AEST, Robbie Cronin ***@***.***> wrote:
@robert-cronin commented on this pull request.
> @@ -0,0 +1,387 @@
+import fs from 'fs';
+import commander from 'commander';
+import { PolykeyAgent, PeerInfo } from '../lib/Polykey';
+import {} from '../../tmp/hole-punch-test/publicserver';
+import { actionRunner, pkLogger, PKMessageType, determineNodePath }
from '.';
+
+function makeAddPeerCommand() {
+ return new commander.Command('add')
I am not sure I follow, as far as I can tell you mean something like
this?
```
function makeAddPeerCommand() {
return new commander.Command('add')
.description('add a new peer to the store')
}
function addHandlersToAddPeerCommand(command: Command) {
return command
.option('-b, --base64 <base64>', 'decode the peer info from a base64
string')
.option('-p, --public-key <publicKey>', 'path to the file which
contains the public key')
.action(...)
}
function makePeersCommand() {
return new commander.Command('peers')
.description('peer operations')
.addCommand(addHandlersToAddPeerCommand(makeAddPeerCommand()))
}
```
It seems like it complicates things, the handlers are linked only to
one command and its kind of a self contained unit.
--
You are receiving this because your review was requested.
Reply to this email directly or view it on GitHub:
#84 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
|
I think from the previous discussion you cannot expect the user to know what relay to use. All polykeys on the network are relays that are synchronising the DHT. This includes the bootstrap nodes. Therefore one shouldn't even bother setting the relay dhts unless one explicitly is limiting the relay usage for security reasons.
…On 16 September 2020 1:56:33 pm AEST, Robbie Cronin ***@***.***> wrote:
@robert-cronin commented on this pull request.
> @@ -0,0 +1,387 @@
+import fs from 'fs';
+import commander from 'commander';
+import { PolykeyAgent, PeerInfo } from '../lib/Polykey';
+import {} from '../../tmp/hole-punch-test/publicserver';
+import { actionRunner, pkLogger, PKMessageType, determineNodePath }
from '.';
+
+function makeAddPeerCommand() {
+ return new commander.Command('add')
+ .description('add a new peer to the store')
+ .option('-b, --base64 <base64>', 'decode the peer info from a
base64 string')
+ .option('-p, --public-key <publicKey>', 'path to the file which
contains the public key')
+ .option('-a, --connected-address <connectedAddress>', 'public
address on which the node can be contacted')
+ .option('-r, --relay-key <relayKey>', 'path to the file which
contains the public key of the relay peer')
Well the relay key is supposed to tell polykey which intermediary node
to use as a relay when asking for a relayed connection to this peer. I
am not sure what else to call this, perhaps `--relay-peer-public-key`?
I can understand to the user it is probably a little confusing. I hope
polykey can do this all under the hood (i.e. there is just a bunch of
peers in the store that have been marked that they can support a relay
connection and polykey just tries them all when trying to connect to a
private peer)
--
You are receiving this because your review was requested.
Reply to this email directly or view it on GitHub:
#84 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
|
Should be done for any case where you find yourself using magic strings.
…On 16 September 2020 2:12:12 pm AEST, Robbie Cronin ***@***.***> wrote:
@robert-cronin commented on this pull request.
> +function makeUpdatePeerInfoCommand() {
+ return new commander.Command('update')
+ .description('update the peer info for a particular public key')
+ .option('--node-path <nodePath>', 'node path')
+ .option('-c, --current-node', 'only list the peer information for
the current node, useful for sharing')
+ .option('-p, --public-key <publicKey>', 'path to the file which
contains the public key')
+ .option('-b, --base64 <base64>', 'decode the peer info from a
base64 string')
+ .option('-ch, --connected-host <connectedHost>', 'update the
connected addresss host')
+ .option('-cp, --connected-port <connectedPort>', 'update the
connected addresss port')
+ .option('-r, --relay-key <relayKey>', 'path to the file which
contains the public key of the relay peer')
+ .option('-v, --verbose', 'increase verbosity level by one')
+ .action(
+ actionRunner(async (options) => {
+ const client = PolykeyAgent.connectToAgent();
+ const status = await client.getAgentStatus();
+ if (status != 'online') {
okay sure thing, I will create an enum for this 👍
--
You are receiving this because your review was requested.
Reply to this email directly or view it on GitHub:
#84 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
|
Is it possible to do the fit over grpc or is it an entirely different TCP port?
…On 16 September 2020 6:59:09 pm AEST, Robbie Cronin ***@***.***> wrote:
@robert-cronin commented on this pull request.
> @@ -4,13 +4,16 @@
class GitRequest {
Git is not only acting as a data structure and way to create packfiles,
but also as a protocol for how to send those packfiles over network
requests (e.g. sideband protocols). So I can't really see a way to
separate git from the network code.
--
You are receiving this because your review was requested.
Reply to this email directly or view it on GitHub:
#84 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
|
Kademlia would naturally be multihop.
…On 17 September 2020 12:47:47 pm AEST, Robbie Cronin ***@***.***> wrote:
@robert-cronin commented on this pull request.
> + fs.rmdirSync(tempDirPeerC, { recursive: true })
+ })
+
+ describe('Peer Relay Sharing', () => {
+ test('can clone a vault through a peer relay connection', async
done => {
+ // ==== Pull Vault B to C ==== //
+ const vaultName = `Vault-${randomString()}`
+ const vault = await peerB.vaultManager.createVault(vaultName)
+
+ const clonedVault = await
peerC.vaultManager.cloneVault(vault.name,
peerB.peerManager.peerInfo.publicKey)
+ expect(vault.name).toEqual(clonedVault.name)
+
+ done()
+ })
+
+ test('can clone many vaults through a peer relay connection',
async done => {
yeah this is only one hop. I guess when we implement out DHT, it might
be a good opportunity to implement multi hop functionality with
something like a multi hop relay. Or do you think pk doesnt really need
to be multi-hop?
I kind of think whatever MatrixOS can do with relay, polykey should be
able to do as well.
--
You are receiving this because your review was requested.
Reply to this email directly or view it on GitHub:
#84 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
|
Should have a crypto model. For specific utilities of encrypting and decrypting and signing and verifying things. Sort of like gpg.
…On 17 September 2020 12:31:58 pm AEST, Robbie Cronin ***@***.***> wrote:
@robert-cronin commented on this pull request.
> }
/**
* Encrypts the given data for a specific public key
* @param data The data to be encrypted
* @param publicKey The key to encrypt for
*/
- async encryptData(data: Buffer, publicKey?: Buffer): Promise<string>
{
+ async encryptData(data: Buffer, publicKey?: Buffer): Promise<Buffer>
{
yeah we can probably put them somewhere else, perhaps in our vault
model? but I just thought since we don't have any specific
`cryptography` domain yet they should just be closest to the keys.
--
You are receiving this because your review was requested.
Reply to this email directly or view it on GitHub:
#84 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
robert-cronin
force-pushed
the
nat-traversal
branch
from
0bd7f64
to
fbf8189
Compare
|
Okay I might go ahead and merge this now. |
This PR is essential for making polykey more available to more users and increasing the p2p aspect of secret sharing. I feel this should be part of MVP too as it peer sharing is essential to the whole polykey idea.
The bulk of this PR is adding functionality for a public keynode that is run on a machine that is exposed to the public internet for relay purposes. This can be broken down into 3 main functions that this public keynode can provide to private keynodes behind NAT layers and other obscurities:
The above 3 points are also in order of which are tried first in order to establish connectivity between two private nodes.
Fixes #37
Fixes #86
Fixes #92