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

Impl symmetric AES encryption on top of TcpStream #111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Impl symmetric AES encryption on top of TcpStream #111

wants to merge 2 commits into from

Conversation

s1gtrap
Copy link

@s1gtrap s1gtrap commented Jan 6, 2016

  • The server will now generate a 1024-bit certificate and corresponding private key at startup which is used during the encryption handshake (the certificates public key is sent to the client after which the private key is used to decrypt the shared secret).
  • Upon success the TcpStream is wrapped in the SymmStream type, after which all subsequent reads and writes are passed through two separate 128-bit AES ciphers (decrypter and encrypter respectively.)

The clients response is not verified atm, since the login::clientbound::Disconnect packet isn't implemented (guessing it won't be long, since @toqueteos has made efforts towards encoding ChatJson?)

The nonce isn't randomly generated either, which I suppose isn't strictly necessary, but would be nice to have. It's also how cuberite does it (kind of.)

I'm also not happy with overruling the compiler in terms of Send and Sync safety (see src/vanilla/server.rs), but I'm under the impression that the openssl crate is as thread safe as it can get, and that marking PKeys as such is a non-issue (I could totally be wrong, I'm not particularly well versed in Rust.)

Still to do:

  • Verify verify_token
  • Generate verify_token as opposed to hardcoding it
  • Resolve thread safety issues (if any?)

@s1gtrap
Copy link
Author

s1gtrap commented Jan 6, 2016

I'll get on this as soon as I can. read_exact can easily be replaced with read_exactly and clone_from_slices implementation is also fairly straight forward.

@toqueteos
Copy link
Contributor

Welcom to hematite_server @bheart and thanks for sending in this PR!

You are right, login::clientbound::Disconnect is not implemented on master. It is implemented on #97 #106 which hasn't been merged yet 😭

Apparently io::copy can be used with slices too. Would that help?

We probably should use take instead of read_exactly this is something I always forget to open an issue for...

@toqueteos
Copy link
Contributor

Oh, I forgot! It'll would be a good thing to enable encryption iff online-mode is true (#44).

@s1gtrap
Copy link
Author

s1gtrap commented Jan 6, 2016

Wasn't aware of io::copy! Thanks! The thing about read_exact/read_exactly is that they both error if they couldn't read the specified amount (right?), whereas an io::Take (when read_to_end) will happily return less. Not sure if that's desirable tbh.

@toqueteos
Copy link
Contributor

Just to be sure, when you say read_exactly are you referencing the one that's implemented on hematite_server/util?

@s1gtrap
Copy link
Author

s1gtrap commented Jan 6, 2016

Yea, don't read_exact and read_exactly work in pretty much the same way?

Regardless, using an io::Take does make more sense, all things considered. There are valid reasons for why the bytes read can be less than the output buffer's size after all.

@toqueteos
Copy link
Contributor

@bheart Yeah, read_exactly is inspired/copied so we can use it on stable. io::Take makes sense but maybe for some specific cases we'll need the exact version.

@@ -68,7 +68,7 @@ impl World {
}

#[allow(unreachable_code)]
pub fn handle_player(&self, mut stream: TcpStream) -> io::Result<()> {
pub fn handle_player(&self, mut stream: SymmStream) -> io::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change this to be a S: Read + Write instead of TcpStream/SymmStream so we can easily switch authenticated mode on/off. /cc @fenhl

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will be difficult to merge with your single-player branch, because of the try_clone call.

I was thinking about introducing a TryClone trait and implementing it for the SymmStream, plus an additional PlainStream which would do nothing but wrap the underlying TcpStream.

So two structs, each with an inner TcpStream, one for plaintext communication and one for encrypted communication.

Then S: Read + Write + TryClone would work.

@fenhl fenhl added the feature label Jan 7, 2016
* The server generates a 1024-bit certificate and corresponding private
key at startup which is used during the encryption handshake.
* All connections are wrapped in the `SymmStream` type during login,
after which all r/w's are passed through separate 128-bit AES ciphers
(known as `Crypter`s in `openssl` crate.)
Uses rust-openssl as released on crates.io instead of pulling the
revision from Github.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants