Skip to content
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

Decentralization #4

Open
delucis opened this issue Oct 4, 2021 · 27 comments
Open

Decentralization #4

delucis opened this issue Oct 4, 2021 · 27 comments

Comments

@delucis
Copy link
Member

delucis commented Oct 4, 2021

Currently a single client acts as the host and all communications pass through them.

The host could provide connected clients with the peer IDs of all other clients so that they can also establish connections between each other (a “full-mesh topology”). At that point, various other possibilities arise: each client could be responsible for processing and emitting their own state updates instead of sending an action to be processed and emitted by the host. Then if the host drops out, the clients (via their existing connections) could agree a new host amongst themselves and keep playing.

A variation on this would be the host just transmitting a list of IDs for all connected peers to allow them to establish connections only in the case the host goes offline.

Decentralization would also mean storing match state in all clients, which would also be more robust.

This should probably be an optional feature.

@delucis
Copy link
Member Author

delucis commented Oct 4, 2021

For the full state to be present on all clients, the host would presumably have to send over the full database state to a connecting peer on connection. This would include credentials as suggested in #1, so would render that authentication approach more or less useless.

@jonaswre
Copy link
Contributor

jonaswre commented Nov 9, 2021

In a p2p scenario authentication would need to be handed through encryption. The host could store a public key from the client. And a client could then send something that's encrypted with the private key. Any peer could then verify the player.

@jonaswre
Copy link
Contributor

jonaswre commented Nov 9, 2021

This could also be used to provide a mechanism for clients private state being present on all clients.

@delucis
Copy link
Member Author

delucis commented Nov 10, 2021

Nice idea! Do you think we could implement that already?

Currently each client has a metadata object for itself:

p2p/src/index.ts

Lines 106 to 108 in 7066e90

private get metadata(): Client["metadata"] {
return { playerID: this.playerID, credentials: this.credentials };
}

Which we send as part of the initial connection to the host:

const host = this.peer.connect(this.hostID, { metadata: this.metadata });

(credentials in this context is a standard property that is set via the boardgame.io client.)

When the host receives the connection request, it checks if a client for the requested playerID previously connected, and if they did, then checks the credentials match what was previously provided:

p2p/src/host.ts

Lines 100 to 117 in 7066e90

// If no credentials exist yet for this player, store those
// provided by the connecting client and authenticate.
if (!existingCredentials && credentials) {
this.db.setMetadata(this.matchID, {
...metadata,
players: {
...metadata.players,
[+playerID]: { ...metadata.players[+playerID], credentials },
},
});
return true;
}
// If credentials are neither provided nor stored, authenticate.
if (!existingCredentials && !credentials) return true;
// If credentials match, authenticate.
return credentials === existingCredentials;

Would the two sides of this connection simply need an additional step locally to encrypt/decrypt the credentials? Or would an additional negotiation be needed where the client demonstrates it can encrypt a token provided by the host?

Happy to look at a PR for this!

@jonaswre
Copy link
Contributor

I've created a small prototyp. https://github.com/jonaswre/p2p/commit/e4249c96741c2b96df102f97cb5be37aec34957f.

I've not tested it in a webclient but made some unit tests. This should be as secure as your current authentification is. But if any other client gets access to the signed message (not persited anywhere) they could use that and replay it. But you have the same issue with your current model. To make the process more secure, one would need to sign every action that is send. And if we add a counter to the action and the server only accepts actions with lastCounter + 1 we could counter replay attacks.

But that would need a bit more restructuring.

@delucis
Copy link
Member Author

delucis commented Nov 10, 2021

Cool, thanks! I’ll try to read through those changes soon (I don’t know much about cryptography so I’m a bit slow).

If you want to try it out in a browser, you could download a copy of the example CodeSandbox: https://codesandbox.io/s/boardgame-io-p2p-demo-0loyd (File > Export to ZIP) and install your local fork:

# in repo for @boardgame.io/p2p
# compile Typescript to JS
npm run build
# create a tarball for the p2p module
npm pack

cd ../boardgameio-p2p-demo
# install dependencies
npm i
# install the local tarball instead of the remote dependency
npm i ../p2p/boardgame.io-p2p-0.3.0.tgz
# serve the demo app
npm start

@jonaswre
Copy link
Contributor

After a little adjustment my change works.

@delucis
Copy link
Member Author

delucis commented Nov 10, 2021

Great to hear! I should have time over the weekend to take a look at your changes — look forward to it 😄

@jonaswre
Copy link
Contributor

To be honest I think taking your approach to P2P will be very difficult because many features will be need to be implemented.
I think I will checkout libp2p, ipfs, orbitdb. This libraries will already implement many of the needed features.

OrbitDB is a Database that's already P2P with authentication. Might be a better fit then doing everything ourselves.

@delucis
Copy link
Member Author

delucis commented Nov 13, 2021

Hi @jonaswre, I’m just trying out your changes with the example code and I get a TypeError: invalid encoding thrown by the call to decodeBase64 in verifyMessage.

Did you see that while you were testing? Does the use of private/public keys impose certain restrictions on the formatting of credentials?

@jonaswre
Copy link
Contributor

Hi @delucis,

I've just checked to be sure. I dont get any error when running the demo. Do you get the errors in "npm run jest" The keys are returned as Uint8Array and then base64 encoded so the credentials just have to be able to store a base64.

@delucis
Copy link
Member Author

delucis commented Nov 13, 2021

The tests pass. I’m reading through in detail and now I see the keyPair option. I’m guessing I maybe have to provide that for this to work?

@delucis
Copy link
Member Author

delucis commented Nov 13, 2021

Oh, I see now that actually you provide a default in the constructor, so it’s not that…

My steps to reproduce are:

  1. Run npm start to serve the demo.
  2. Select the isHost checkbox.
  3. Click the Connect button.

I’ll try some debugging to see if verifyMessage is receiving some unexpected value.

@jonaswre
Copy link
Contributor

Ahh I see the Issue...

https://github.com/jonaswre/p2p/blob/798e8cc835fd8ebfe2d91ef1b44452fc6dab3202/src/index.ts#L118

If credentials are provided they will be sent to the server.
I removed the credentials field from the demo. Credentials need to be undefined in the constructor.

@delucis
Copy link
Member Author

delucis commented Nov 13, 2021

Ahh, I think I see the issue.

If credentials is set, that value gets sent (instead of the public key, which is only used when credentials isn’t provided):

p2p/src/index.ts

Lines 118 to 119 in 5241c4d

credentials:
this.credentials == undefined ? this.publicKey : this.credentials,

The host stores credentials and then uses them to try to verify the message:

verifyMessage(message, existingCredentials) !== null

In the demo, default credentials are generated using nanoid, which don’t match the expected encoding.

@delucis
Copy link
Member Author

delucis commented Nov 13, 2021

Ahh I see the Issue...

Exactly! I need to think what could be a better public API for this.

@jonaswre
Copy link
Contributor

I think best would be, that the client can provide the PrivateKey in the credentials.
Then tweetnacl uses that privatekey to generate the publickey an send that to the server.
That way the constructor doesn't get any new parameters. And client logic is basicly the same. - If you want to rejoin you need to remember the privatekey. -

I also had the idea to use the method generate keyPair from seed. The seed could be calculated from a user input, so that the user can basicly rejoin from any device because you only need to remember the input you gave when you joined, not some cryptographic keys. That would be way less secure but to be honest does it matter?

@jonaswre
Copy link
Contributor

@delucis check out my last commit. That might be a good way to allow user inputs to generate the private keys.

@delucis
Copy link
Member Author

delucis commented Nov 13, 2021

That looks good. I was trying something like this, but just using decodeUTF8 looks much nicer 🤣

const credentials = "my-wordy-credentials";
const seed = Uint8Array.from(
    [...credentials]
      .slice(0, 32) // Trim length to 32
      .map((char) => char.charCodeAt(0)) // Map characters to integers
      .concat(Array.from({ length: Math.max(0, 32 - credentials.length) })) // Pad length up to 32
  );
const keyPair = nacl.sign.keyPair.fromSeed(seed);

The tweetnacl docs say

“The seed must contain enough entropy to be secure. This method is not recommended for general use”

But it would be nice this way because it allows us to keep the public API the same as for other boardgame.io uses. Maybe we could export a helper method to generate sufficiently random credentials?

@jonaswre
Copy link
Contributor

Thats what I meant with its way less secure. I think the question is what is our threat model. I pretty sure if we intruduce some entropy through for example sha-256(this.matchId+this.playername+this.credentials) that might be enough entropy for us.

If we want the more entroy just leave credentials undefined and use nac.sign.keyPair() which generates a random keyPair. If players dont change devices that keyPair can just be cached and all is well.

@delucis
Copy link
Member Author

delucis commented Nov 13, 2021

I was thinking an app developer might want to be able to generate and cache the credentials somehow.

const { generateCredentials } from '@boardgame.io/p2p';

let credentials = fetchStoredCredentials();

if (!credentials) {
  credentials = generateCredentials();
  storeCredenitals(credentials);
}

// pass credentials to boardgame.io

So providing a simple helper for generating a string with sufficient entropy would be helpful, e.g.:

export function generateCredentials(): string {
  return encodeUTF8(nacl.sign.keyPair().secretKey);
}

@jonaswre
Copy link
Contributor

So no key generation inside of boardgame.io?
Rather then generate a full keyPair just for randombytes we can just use https://github.com/dchest/tweetnacl-js/blob/master/README.md#naclrandombyteslength.

@delucis
Copy link
Member Author

delucis commented Nov 13, 2021

We would still generate keys inside this package, like in your commit above, using credentials as the seed.

The helper would just be a method for generating sufficiently random credentials that a developer could use. Or am I misunderstanding? Is credentials: 'user-generated-string' just as secure as credentials: encodeUTF8(nacl.sign.keyPair().secretKey) (or equivalent using the random bytes method)?

@jonaswre
Copy link
Contributor

I think the following two functions are equaly secure.

export function generateCredentials(): string { return encodeUTF8(nacl.sign.keyPair().secretKey); }

export function generateCredentials() { return encodeBase64(nacl.randomBytes(64)); }

But im pretty sure the first is more expensive because it does the generation of random bytes plus all the crypto between private and public key and all the rest. But we only use the random bytes.

And regarding generation inside of boardgame.io I meant the else part of the if statemante from the last commit. What do we do when there are no credentials given? throw an error or just dont send any credentials in the client server communication?

Latest commit fixed the bad size seed. But this all still just prototyping

@delucis
Copy link
Member Author

delucis commented Nov 14, 2021

For now I would not generate keys if no credentials are provided. In that case, things are just unauthenticated and insecure like they are currently.

@jonaswre
Copy link
Contributor

jonaswre commented Nov 14, 2021

@delucis
Have not tested the latest commit. But it should work now as we talked about.

But I still think if your (and defently my) dream is full-mesh. We should look for another tech stack then peerjs thats just too barebone. But I think there is still value in this p2p package.

@delucis
Copy link
Member Author

delucis commented Nov 14, 2021

I’ve merged your changes into my own branch for PR #10 and I can confirm it’s working 🎉

Yeah — I’d be happy to look at any other changes for heading in that direction. One of the challenges is that boardgame.io is not designed with P2P in mind and another is that this is my first time playing with a P2P set-up, so there’s plenty I don’t know. PeerJS was nice because it is simple to use. Plus they provide a free handshake server so you can get up and running really quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants