Update Rust binding to v0.0.3 #349

ilammy · 2019-01-17T21:45:20Z

This is a cumulative update of rust-themis to version 0.0.3 from here:

https://github.com/ilammy/rust-themis

You can see git diff v0.0.2..v0.0.3 there for detailed changes. Obviously, the diff does not apply as is and has been manually merged into this repository, taking into account previous changes and the new code layout.

Some notable changes since v0.0.2:

Secure zeroing of key material is included into this update.
Various changes related to Rust 2018 edition support and API documentation updates introduce a lot of diff clutter. Deal with it.
Vendored build is back. It is mostly intended as a hack for docs.rs support rather than an actual use-case. Absolutely worth it if you ask me, but I'm slightly biased ;)

This brings a new libthemis-src crate to contain the whole source code of Themis. It is implemented with a symbolic link to the top of the repository. Hopefully, this loop does not break anything anywhere...

For one, symlink usage has broken libthemis-src compilation (due to a bug in 3rd-party crate) so vendored build is currently failing. We'll fix this issue later.
There are some other interesting TODOs added and removed during the development. Just grep for them in you're interested.

These changes do not integrate rust-themis into the main build system. It is possible to build rust-themis alone with Cargo, but the Makefile does not know about us yet. This will be added soon.

This is a cumulative update of rust-themis to version 0.0.3 from here: https://github.com/ilammy/rust-themis You can see `git diff v0.0.2..v0.0.3` there for detailed changes. Obviously, the diff does not apply as is and has been manually merged into this repository, taking into account previous changes and the new code layout. Some notable changes since v0.0.2: - Secure zeroing of key material is included into this update. - Rust 2018 edition support and API documentation updates introduce a lot of diff clutter. Deal with it. - Vendored build is back. It is mostly intended as a hack for docs.rs support rather than an actual use-case. This brings a new `libthemis-src` crate to contain the whole source code of Themis. It is implemented with a symbolic link to the top of the repository. Hopefully, this loop does not break anything anywhere... For one, symlink usage *has* broken `libthemis-src` compilation (due to a bug in 3rd-party crate) so vendored build is currently failing. We'll fix this issue later. - There are some other interesting TODOs added and removed during the development. Just grep for them in you're interested. These changes do not integrate rust-themis into the main build system. It is possible to build rust-themis alone with Cargo, but the Makefile does not know about us yet. This will be added later.

Lagovas · 2019-01-18T10:14:27Z

src/wrappers/themis/rust/src/secure_cell.rs

+    /// ```
+    /// use themis::secure_cell::SecureCell;
+    ///
+    /// SecureCell::with_key_and_context(&[], b"byte string");


can you change examples without usage of empty keys? themis doesn't allow to use empty keys:

themis/src/soter/openssl/soter_sym.c

Line 132 in 8b87705

SOTER_CHECK_PARAM_(key_length!=0);

themis/src/soter/boringssl/soter_sym.c

Line 132 in efe288e

SOTER_CHECK_PARAM_(key_length!=0);

plus constructor with context as parameter force to use one context for all encryptions but more securely when user will use different contexts for separate encryptions. all other our wrappers don't provide to initialize secure cell component with pre-defined context. better to force using different contexts with extra parameter in encrypt/seal method

If one tries to use an empty key with Secure Cell they will get an error from encrypt() and friends. However, that will happen later, not at the construction site where we do not perform any checks currently. (Though we probably should...)

These examples serve more as a compilation checks to ensure that the type bounds on the generic constructor functions allow using these various data types as input.

I fully agree with you that using incorrect keys makes a bad example, especially in documentation. I will update the keys in examples in this pull request.

As for your comment on the user context, please see the reply below. I agree that this needs to be changed, but I'd like to do it in a different pull request.

Thank you, I see a commit with updated keys in examples.

Regarding context – ok, will wait for next PR.

vixentael

user_context is required in SecureCell context_imprint mode

let's change comment to

/// Encrypts `message` with `master_key` including required `context`.

https://github.com/cossacklabs/themis/pull/349/files#diff-9357a1d77e8235f09ab09538579d776aL393

and check for context != null in encrypt_context_imprint
https://github.com/cossacklabs/themis/pull/349/files#diff-9357a1d77e8235f09ab09538579d776aL397

Same for decrypt
https://github.com/cossacklabs/themis/pull/349/files#diff-9357a1d77e8235f09ab09538579d776aL444

vixentael · 2019-01-18T16:53:23Z

src/wrappers/themis/rust/libthemis-src/src/lib.rs

+use std::path::{Path, PathBuf};
+use std::process::{Command, Stdio};
+
+/// A builder (literally!) for Themis, produces [`Library`].


vixentael · 2019-01-18T17:01:32Z

src/wrappers/themis/rust/src/secure_cell.rs

-/// * `token_protect()`
-/// * `context_imprint()`
+/// This is modeless, basic cell. First you create a `SecureCell` object with a key and an optional
+/// context then you select the desired operation mode and your Secure Cell is ready to go.
 pub struct SecureCell {


as @Lagovas mentioned, from security point of view, SecureCell shouldn't take context as init param. Context is an additional parameter in encryption function which increase security guarantees for messages encrypted with same key.

Yeah... I thought about that when I started working on integration tests and studied other wrappers and their API more carefully. I doesn't make much sense to keep the user context the same for all operations – then it can be a part of the key. However, changing the API looked like a hassle so I never got to that (just left some TODOs in my WIP branches).

I will unify the API to be the same as the other language wrappers, but I would like to do this in a separate pull request, after this one is merged and Rust wrapper is integrated into the build system. That way we'd be able to see that the changes do not break anything, the code still compiles and the tests pass.

Please rest assured, this change is on my (private) task list.

ok, makes sense

vixentael · 2019-01-18T17:05:29Z

src/wrappers/themis/rust/src/secure_cell.rs

+    ///
+    /// assert!(cell.decrypt(&corrupted_data, &auth_token).is_err());
+    /// assert!(cell.decrypt(&encrypted, &corrupted_token).is_err());
+    /// ```


awesome examples!

vixentael · 2019-01-18T17:06:44Z

src/wrappers/themis/rust/src/secure_cell.rs

 pub struct SecureCellContextImprint(SecureCell);

+// TODO: maybe panic if a SecureCell with an empty context is switched into context imprint mode


Will do together with other API changes to Secure Cell. And we'll probably need some unit-tests and documentation updates ("Panics" sections) to communicate clearly that passing an empty context or a key is big no-no and you should never ever do this (or your application will crash).

We could leave the behavior as it is now (encrypt/decrypt returns an error when called for a Secure Cell with an empty key or context), but I think this warrants an early check and a loud failure. This is a programmer error and a data structure contract breach, like using an invalid index when addressing an array (which does panic in Rust).

Though I'll update the documentation for context imprint mode in this PR.

it totally makes sense to add panic at the early stage

vixentael · 2019-01-18T17:13:06Z

Overall I really like the changes, the documentation looks fantastic and it's very easy to read!

Secure Cell does not allow empty keys. Encryption and decryption will fail if an empty key is used, but this will be detected later, not at the construction site. Currently we don't do anything about it (so just update the API docs), but later this will become an enforced assertion.

Secure Cell *requires* non-empty user context when in context imprint mode. It fails to operate if the provided context is empty. Currently this is noted only in the very last example in the module-level documentation. Let's be more explicit about this. Later we'll get even more serious and add some assertions in the code as well as a "Panics" section in the docs.

vixentael · 2019-01-20T20:12:57Z

@ilammy summarizing, are there any other changes you would like to add in this PR?
(except update SecureCell API and panic checks for context which will be part of next PR)

ilammy · 2019-01-20T20:25:56Z

@vixentael nope. I don't have anything else cooking for this PR. I believe it can be merged as is now if you and @Lagovas are okay with it.

ilammy added the W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates label Jan 17, 2019

ilammy requested review from vixentael, ignatk, Lagovas and shadinua January 17, 2019 21:45

ilammy mentioned this pull request Jan 17, 2019

RustThemis #339

Closed

11 tasks

Lagovas requested changes Jan 18, 2019

View reviewed changes

vixentael reviewed Jan 18, 2019

View reviewed changes

ilammy added 2 commits January 19, 2019 17:21

vixentael approved these changes Jan 20, 2019

View reviewed changes

Lagovas approved these changes Jan 21, 2019

View reviewed changes

vixentael merged commit b86ba8e into cossacklabs:master Jan 21, 2019

ilammy deleted the rust-themis-0.0.3 branch January 27, 2019 22:53

ilammy mentioned this pull request Aug 6, 2020

Drop libthemis-src support #691

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Rust binding to v0.0.3 #349

Update Rust binding to v0.0.3 #349

ilammy commented Jan 17, 2019

Lagovas Jan 18, 2019

ilammy Jan 19, 2019

vixentael Jan 20, 2019

vixentael left a comment

vixentael Jan 18, 2019

vixentael Jan 18, 2019

ilammy Jan 19, 2019

vixentael Jan 20, 2019

vixentael Jan 18, 2019

vixentael Jan 18, 2019

ilammy Jan 19, 2019

vixentael Jan 20, 2019

vixentael commented Jan 18, 2019

vixentael commented Jan 20, 2019

ilammy commented Jan 20, 2019

		pub struct SecureCellContextImprint(SecureCell);

		// TODO: maybe panic if a SecureCell with an empty context is switched into context imprint mode

Update Rust binding to v0.0.3 #349

Update Rust binding to v0.0.3 #349

Conversation

ilammy commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vixentael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vixentael commented Jan 18, 2019

vixentael commented Jan 20, 2019

ilammy commented Jan 20, 2019