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

Rust implementation #8

Closed
vvanders opened this issue Mar 2, 2017 · 89 comments
Closed

Rust implementation #8

vvanders opened this issue Mar 2, 2017 · 89 comments

Comments

@vvanders
Copy link
Contributor

vvanders commented Mar 2, 2017

I've started on a wrapper of netcode for Rust located at https://github.com/vvanders/netcode-rust (until we can figure out best way to merge).

Right now only client functions are hooked up and nothing is tested but it's a bit of a start. Currently it bootstraps netcode via "gcc" crate which shells out to msvc pretty cleanly on windows. Still need to sort out linux.

I'll update this issue once I've got something more stable, this is mostly a placeholder to discuss ongoing integration and related items.

@gafferongames
Copy link
Contributor

Sounds fantastic. Thanks!

I'm restructuring the project right now so there is a rust directory you can work under, and a go directory, in case somebody else wants to do that.

I've moved all my work under "c"

@gafferongames
Copy link
Contributor

Oh wait, it's a wrapper or an implementation in Rust? I'm confused. If it's a wrapper, I think it's best to keep it as a separate project.

But if somebody wants to create an actual implementation in rust, yes I would accept that in this repository.

cheers

  • Glenn

@vvanders
Copy link
Contributor Author

vvanders commented Mar 2, 2017

Yeah, it's a wrapper for now, sorry if I wasn't clear on it up front. Totally fine with keeping it in a separate repo.

My plan was to wrap the c impl and make sure to nail down a solid API + docs. Once that's done then migrate things piecewise over to a Rust impl. Probably server first followed by client. That way anyone can pick up Rust bindings right away since it's going to take me a while to get a proper implementation off the ground.

@gafferongames
Copy link
Contributor

Sounds pretty good, let me know when you get to the implementation point. Meanwhile, I'll continue to work on the spec.

@vvanders
Copy link
Contributor Author

vvanders commented Mar 3, 2017

So I've got everything wrapped now. Started to take a crack at unit tests but Rust's multithread runner + actually binding sockets doesn't play nice, which is pretty much what I'd expect. That said VSCode + C/C++ extension actually lets me debug Rust + C inline together which is nifty.

I think I'm going to work on the API bits next, I want to get something a bit more in line with Rust's patterns.

Also had a quick question on the server C API. There's a check for it a client is connected but I can't seem to find a way to distinguish them, from what I could tell there could be a case where we have a disconnect + new connection that could look like the same client, ideas?

I was also wondering what the netcode_client_index() was for? I can't seem to find any usages or reference to client_id in the code.

Anyway, that's probably where I'll leave it for tonight, overall I like the spec. Simple and straightforward, was pretty easy to pick up and connection tokens are a nice idea.

@gafferongames
Copy link
Contributor

gafferongames commented Mar 3, 2017

netcode_client_index returns the index of the client on the server. which is basically the slot number, so on a 4 player game, you could have slot index 0, and be player 1 effectively. This is used by games to assign who is which player.

On the server there is a check to see if a server is connected, and then possibly a short time later, yes a different client could be connected in that slot. Would you like the ability to get the client id for a client slot as a public API? This is a globally unique identifier for clients, so it would be different if a different client is in the slot.

cheers

@gafferongames
Copy link
Contributor

added server_client_id function. returns uint64_t unique client id for that slot so you can distinguish different clients in a slot before vs. after. -- cheers

@vvanders
Copy link
Contributor Author

vvanders commented Mar 3, 2017

That totally makes sense on the client_index, I was thinking of some cases where we didn't use fixed slots and so my head wasn't in the right space.

Thanks for the client_id, should make it trivial to generate connect/disconnect events in addition to packet events. If I can get some time tomorrow I think I've got a good idea on what the Rust side will look API-wise. After that it's time to dig into libsodium and get started on a proper implementation.

@gafferongames
Copy link
Contributor

I'm excited. Thanks for your work!

@vvanders
Copy link
Contributor Author

vvanders commented Mar 4, 2017

Yeah, me too. I've been looking for a non-trivial Rust project that's small enough that I can complete it in a month or two of coding on the side. Doesn't hurt that the domain is interesting too.

I'm still working on the API but I started digging into libsodium/netcode internals and was wondering why it looks like the server sends down two full keys once a channel is established rather than using asymmetric key pair with the client sending up their public key? More for my education than anything else since I haven't worked with sodium before. Both approaches seem like they'd work but I would think that it would be a tad more secure to not share the key with the client in case of interception(I know sequence numbers help here too)?

@gafferongames
Copy link
Contributor

I send down two keys because each packet needs to be encrypted with a nonce (eg. 64bit sequence) number. So there is one key for the client -> server direction, and another key for the server -> client direction. I felt this was more elegant than for example, having the high bit set on the sequence number for one of those directions, because you can never use the same nonce value more than once, or you risk exposing the private key.

@gafferongames
Copy link
Contributor

ps. The symmetric keys are sent down to the client, but these keys are only the keys for encryption between the client and the server over UDP. The other key, the private key that generates the connect token, is known only to the web backend and the dedicated server instances.

The reason this works is that the connect token is sent down to the client over HTTPS (public and private portion), then only the private portion of that connect token (encrypted with shared private key), is sent over UDP as part of connection handshake.

cheers

  • Glenn

@vvanders
Copy link
Contributor Author

vvanders commented Mar 4, 2017

Yeah, that makes complete sense. I was just thinking it'd be slightly more secure to have asym from client->server and server->client in case the client was ever compromised they couldn't use it to generate fake packets(with new nonce) spoofed from the server. Principals of least access and all that but it's a pretty theoretical attack anyway.

Either way libsodium is pretty nice, the Rust bindings are a bit behind and don't use ChaCha20 so I'll probably just have to use raw FFI which is fine, it'll be easier to closely mirror what netcode.c already does.

@gafferongames
Copy link
Contributor

gafferongames commented Mar 4, 2017

Yeah, that makes complete sense. I was just thinking it'd be slightly more secure to have asym from client->server and server->client in case the client was ever compromised they couldn't use it to generate fake packets(with new nonce) spoofed from the server. Principals of least access and all that but it's a pretty theoretical attack anyway.

Oh I see what you mean here. I hadn't considered this as a viable attack. I consider the client fully compromised (running on PC), so I would expect an attacker on client would be able to modify code or memory locally to insert arbitrary packets or data into its queue anyway.

cheers

@vvanders
Copy link
Contributor Author

vvanders commented Mar 4, 2017

Yeah, that's totally fair. Like I said it's a pretty rare attack angle.

I have to say, I'm really impressed with libsodium. Seems very well put together and tons of awesome auth/crypto tools.

@gafferongames
Copy link
Contributor

I'm a huge fan of libsodium. It's a great library. Somebody should port it to Rust :)

@vvanders
Copy link
Contributor Author

vvanders commented Mar 5, 2017

Yeah, there's a set of wrappers but they're still a bit lacking(no ChaCha20 secret box for instance).

So I'm starting to dig into the implementation, a couple questions about connection tokens if you don't mind:

timeout_seconds - this looks fixed, any reason to encode rather than control server-side?
create_timestamp - Can't we just use UTC here and only store expire time? It looks like it's ignored in netcode.c:2608
userdata - Is this for future use or just to have a unique set of data for the challenge token?
sequence - Is this something that you'd expect users to have domain knowledge around and specify? I saw in libsodium that they say that random_bytes is sufficient for nonce since the collision chance is small? I'm wondering if this would be better to not let the client specify in case they accidentally reuse a number.

Also let me know if any of this feedback is unwarranted. I'm mostly just trying to get context on the parts of the framework but I'm happy to also provide notes on things I as I encounter them if you think that'd be useful.

@vvanders
Copy link
Contributor Author

vvanders commented Mar 5, 2017

Also, one last thing and then I promise I'll shut-up and get back to implementing. I see a bunch of ifdefs around endianess but I only see it used in some of the test suites. Is this transport considered endian specific?

@gafferongames
Copy link
Contributor

gafferongames commented Mar 5, 2017

The transport has little endian byte order. On a big endian machine, you should convert to/from little endian. See the functions to netcode_read_uint32 and so on, they're written in an endian agnostic way (they could be optimized to use the endian #ifdefs...) but currently they are just coded slowly so that the uint16_t/uint32_t values written/read are in little endian order.

For the other questions:

timeout seconds is fixed in the C reference implementation for simplicity, can be a server-side variable set to whatever you want. This is why it's in the connect token, so it can vary. I recommend a default of 5 seconds.

create timestamp for the token is used on the client so it knows when to give up, eg. if client connect goes on longer than expire timestamp - create timestamp in the connect token, the connect cannot possibly succeed, because the connect token has expired, so give up. This is a safety measure in case the client gets a long list of servers that are all going to timeout (eg. worst case timeout per-server before client realizes it needs to go onto the next one).

userdata is user defined data, 256 bytes large, that allows communication from the web backend to the dedicated server instance per-connecting client.

64 bits is too small to just roll random and have secure crypto, hence sequence # that increases with each connect token generated. The user should be aware of this, as they have to save that sequence # and restore if the server goes down and back up, eg. not start back up again at sequence 0 or they risk exposing the private key.

@vvanders
Copy link
Contributor Author

vvanders commented Mar 5, 2017

Thanks for the details, not sure how I missed the endian thing, makes sense now.

On the nonce I though the point of those was to defeat replay attacks rather than leaking the key? The docs specifically reference randombytes_buf as a reasonable source:

https://download.libsodium.org/doc/secret-key_cryptography/authenticated_encryption.html
The nonce doesn't have to be confidential, but it should never ever be reused with the same key. The easiest way to generate a nonce is to use randombytes_buf().

That said I don't think it's going to matter much in implementation terms just trying to see if it's worth making some simplifying assumptions. I'll go ahead and keep it in on the Rust implementation. I think I've got enough to sort out the token generation and connection handshake. It'll probably take me a few days to get something talking to each other but I'll keep you updated on my progress.

I think the Rust stuff is going to land really nice, I've already got a clean abstraction for swapping socket implementation for testing(that's totally zero-cost) and I think I can provide a simple API that'll be optionally plugable into futures/tokio to help drive adoption for people who are looking to use high performance IO.

@gafferongames
Copy link
Contributor

gafferongames commented Mar 5, 2017

To my knowledge it's generally accepted that random nonces at 64bits aren't safe.

From the libsodium docs:

The XChaCha20 variant, introduced in Libsodium 1.0.12. It can safely encrypt a practically unlimited number of messages of any sizes, and random nonces are safe to use.

I agree that a random nonce for the connect token would be much nicer from a user point of view. Something to consider in the future, and maybe worth upgrading to libsodium 1.0.12 to obtain, vs. the current sequence number, which is error prone and easy to screw up.

Back to rust:

I think the key thing that rust can provide is better safety, more performant I/O (C implementation uses sendto/recvfrom), and multithreading. I'm very interested to see where it lands.

cheers

@gafferongames
Copy link
Contributor

Some more info on short nonces here:

https://download.libsodium.org/doc/key_derivation/index.html#nonce-extension

@vvanders
Copy link
Contributor Author

vvanders commented Mar 9, 2017

Huh, not sure why they contradict themselves in the docs, better to err on the side of caution I guess.

Anyway, I've made some small progress on things. I've got ConnectToken completely written in native rust(aside from libsodium-sys but I don't expect a pure rust implementation yet) with this commit. The wrappers worked out kinda nice since it means I'm able to check confirmation with the C API inside the unit tests.

A couple more things came up as I was implementing the token:

  1. It looks like write_u* just does an endian-swap instead of a conversion, I had to use big endian serialization on x86 to be able to read the tokens from the C API cleanly.
  2. If you don't mind I might look into splitting out the network sim/token functions on the C side a bit more so that I can call them from the Rust unit tests. I really like the idea of the Rust tests validating that we can talk to C API based servers.
  3. Is there any reason for going with IPs instead of host names for the connection token? There's some cases where you'll put a geographic load-balancing DNS in front of a host name and when the server has to do the resolution you won't get an IP that's geographically close(think CDNs and the like).

Anyway, I'm trying to carve out time for this when I can. Next big thing is probably going to be getting the server socket setup + decode right. I'll probably see if I can talk to the C Client so I can verify my side of things before implementing a client stack as well.

@gafferongames
Copy link
Contributor

gafferongames commented Mar 9, 2017

Thanks.

#1 - Sounds like a bug. Network order is supposed to be little endian. I'll fix this.

#2 - No problem as long as the functions split up but remain in the c file only. setup a pull request and i'll review and accept. good idea to be able to communicate between rust and c for tests like this.

#3 - Size mostly. The connect token needs to fit inside a conservative 1200 byte packet (MTU). I think max servers is set to 32, and I don't see 32 fqdns fitting easily inside the token (with the rest of the data), in all cases. Maybe the token has a list of IPs or one FQDN instead? That's doable.

Sounds good and thanks for working on this. I like how you can test the server, and run it against the C client to make sure it works. Helps avoid the chicken and egg (is the server broken, or the client?) which is always the painful thing when writing a client and server at the same time.

I guess fixing up the endian order to little endian is important, so I'll work on that now.

cheers

  • Glenn

@gafferongames
Copy link
Contributor

gafferongames commented Mar 9, 2017

I've worked out why the docs are different:

This page describes the secret key box encryption:

https://download.libsodium.org/doc/secret-key_cryptography/authenticated_encryption.html

And it has crypto_secretbox_NONCEBYTES, which is 24 bytes, this is large enough for a random

The other pages describe the AEAD primitives, which have 64, 96, and the extended nonce. (chacha20). These are the crypto primitives used for the connect tokens.

That's why there is a difference in the docs. Not safe to use random nonce for the 64 or 96 bit AEAD, but safe with the new chacha20 primitive.

cheers

@vvanders
Copy link
Contributor Author

vvanders commented Mar 9, 2017

Ah, that explains the difference. I wonder if there's not a big perf hit if it makes sense to move to XChaCha.

Sounds good, the endian thing is pretty easy to switch(Rust has a nice zero-cost ByteOrder crate that saves the trouble of all the bit-flipping).

I like the idea of one FQDN or raw IPs, seems like enough flexibility(and if they need more they can handle it when they give out the token). How would you feel about keeping everything as-is right now and then once I've got a working server/client we could consider a v2 token/etc? There might be a couple other design changes that come up which would be easy to do as just one large item.

@gafferongames
Copy link
Contributor

gafferongames commented Mar 10, 2017

Lets keep a list of things to improve, but keep these for a 1.1 version. Lets just get the current version fixed up, documented as a standard, and working across a number of languages. It's functional right now, and users will let us know what are the most important things to consider for 1.1.

@gafferongames
Copy link
Contributor

OK. Everything should be little endian now.

@vvanders
Copy link
Contributor Author

Awesome, sounds like a plan.

I've been digging into mio but they don't provide send()/recv() for UDP. I was thinking of using connected sockets so I can use completion IO to be able to parse work out to other threads. I just got one PR merged in miow and hopefully should be able to get the rest into mio from there and whatnot.

How strongly do you feel about being able to decode/process off-thread if users want to? It shouldn't be too tricky to work out as an opt-in feature. I'm thinking some sort of future.rs based API where the base just handles it in-line but can handle callbacks on completion for threadpools or the like.

@gafferongames
Copy link
Contributor

gafferongames commented Mar 11, 2017

I don't have strong opinions. Please implement the way you see as best fit for the language

@gafferongames
Copy link
Contributor

gafferongames commented Mar 31, 2017

It's historic.

Previously the packets were encrypted with this simplebox primitive too, but then I realized they needed the additional data for versioning and safety (eg. to make it impossible to change a packet type in the header and so on).

The connect token needs additional data because it has the private and public portions.

But a challenge token is only private data, so it has no additional data. Hence it stayed with the simplebox primitive.

cheers

@gafferongames
Copy link
Contributor

Would you like to create a pull request for your latest work into this repo? Having it in here would get it a lot more visibility. I'd be happy to accept pull requests as you reach milestones or when you have made progress you want to share.

@gafferongames
Copy link
Contributor

ps. There seems to be somebody working on a C# implementation as well in parallel with you, so you should have company shortly.

@vvanders
Copy link
Contributor Author

Yeah, that'd be great. Let me mark all the in-progress stuff as private and I'll get something put together tomorrow.

Any chance to unifying the crytpo? crypto_aead_chacha20poly1305_encrypt() lets you have the additional data as optional(I have it mapped to Option<&[u8]> on the Rust side) so we could use a single function for both paths.

Glad to hear there's a C# implementation starting up, who knows they might even beat me to completion :).

@gafferongames
Copy link
Contributor

I think we can unify the crypto. I'll work on that today.

@gafferongames
Copy link
Contributor

Closing this issue out. Creating a project for discussion on the Rust implementation

@gafferongames
Copy link
Contributor

Encryption is unified in reference implementation, standard updated.

@vvanders
Copy link
Contributor Author

vvanders commented Apr 1, 2017

Huh, no idea why I didn't see the project stuff before, neat!

Thanks for unifying the crypto. I'll update to latest and continue onwards towards getting the server running.

@vvanders
Copy link
Contributor Author

vvanders commented Apr 1, 2017

Also looks like I don't have permissions to access the project board, if you want to give me access I'll throw up a few tasks for the stuff I've been looking at.

@gafferongames
Copy link
Contributor

I'll see if I can find out how to give you access to that

@gafferongames
Copy link
Contributor

Until, then re-opening!

@gafferongames gafferongames reopened this Apr 1, 2017
@gafferongames
Copy link
Contributor

I've tried adding TODO and In Progress columns, but I'm not sure if you can edit them or not.

Not sure if I like the projects feature or not yet...

@gafferongames gafferongames changed the title Rust wrapper for netcode Rust implementation Apr 1, 2017
@gafferongames
Copy link
Contributor

Very small change made as I did another pass over the standard today:

8920fde

If the connect token create timestamp is greater than the expire timestamp, it should fail to read.

cheers

@vvanders
Copy link
Contributor Author

vvanders commented Apr 1, 2017

Yeah, I can't edit or anything yet. I saw the collab invite but didn't seem to work.

Not an issue, I'm happy to keep using this and submitting PRs.

One quick Q, I've got cargo + doc links in the from the old rust readme, mind if I merge that over into the main readme here?

@gafferongames
Copy link
Contributor

Hello, I have recently made some small changes to the C implementation to support custom allocators, but it has broken the Rust impl. Also, in my own testing with another project, I found that if UDP sockets are created with IPv4 rather than IPv6, then they will work when run under Travis.

@gafferongames gafferongames reopened this Jun 1, 2017
@gafferongames
Copy link
Contributor

Sorry, I added const support to the C API (see NETCODE_CONST) but it seems to have dorked up Rust again. On the positive side, you now have a way to mark bits you want const as const, I remember these were causing issues for Rust type safety...

@gafferongames
Copy link
Contributor

I've had a crack and fixed most issues, but one test is still disabled (needs to be updated to pass in a null allocator context and function...), plus there is some weird stuff that looks like the private_key passed in is never used. Also, bunch of complaints about variables, functions and structs that aren't used. Would be nice to clean those up, but I don't want to be a bull in a china shop here ;)

@vvanders
Copy link
Contributor Author

By all means feel free to edit/modify/improve, I don't mind at all. In fact I'm going to be pretty slammed at work for the next few months so I don't think I'll be able to get it anytime soon.

@gafferongames
Copy link
Contributor

BRB learning Rust...

@gafferongames
Copy link
Contributor

gafferongames commented Aug 14, 2017

Heads up, I just don't have the headspace to maintain Rust, Golang and C versions when I make small changes. Because of this think it's best that Rust lives as it's own repo from now on. I will continue to keep golang and C impl in the main repo because I can actually maintain these myself.

cheers

  • Glenn

@rohel01
Copy link

rohel01 commented Aug 28, 2017

Hello, the rust implementation repository seems gone :(

@gafferongames
Copy link
Contributor

Yes, we have decided to keep this repository for the C reference implementation only.

You can find the Rust implementation in its own repository now:

https://github.com/vvanders/netcode.io

If somebody would like to fork this and continue development, that would be great. It doesn't seem to be maintained anymore.

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

No branches or pull requests

3 participants