Open delucis opened 3 years ago
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.
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.
This could also be used to provide a mechanism for clients private state being present on all clients.
Nice idea! Do you think we could implement that already?
Currently each client has a metadata object for itself:
Which we send as part of the initial connection to the host:
https://github.com/boardgameio/p2p/blob/7066e90777c10a5eea85ce261f57579714de1711/src/index.ts#L156
(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:
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!
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.
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
After a little adjustment my change works.
Great to hear! I should have time over the weekend to take a look at your changes — look forward to it 😄
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.
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
?
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.
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?
Oh, I see now that actually you provide a default in the constructor, so it’s not that…
My steps to reproduce are:
npm start
to serve the demo.isHost
checkbox.I’ll try some debugging to see if verifyMessage
is receiving some unexpected value.
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.
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):
The host stores credentials
and then uses them to try to verify the message:
In the demo, default credentials are generated using nanoid
, which don’t match the expected encoding.
Ahh I see the Issue...
Exactly! I need to think what could be a better public API for this.
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?
@delucis check out my last commit. That might be a good way to allow user inputs to generate the private keys.
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?
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.
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);
}
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.
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)?
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
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.
@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.
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.
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.