Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Rust API Guidelines compliance (#383)
* Use impl Trait for Secure Comparator

* Drop outdated TODOs

We already handle errors in Secure Cell in acceptable way, there is no
need to panic. Similarly, Secure Comparator has good enough usage
pattern with "is_complete()" so we don't have to handle completion
differently.

* Use impl Trait for key types

* Fix typos in API docs

* Reorder Secure Comparator methods for better API docs

* Add Debug implementation for all public types

* Remove Clone implementation from Secure Message types

They do not really need to be copyable (like Secure Cells). If you need
to have use Secure Messages with the same keys then construct it twice.
However, this should not be necessary in the first place.

* Rename getter for remote peer ID

Rust generally does not use "get" prefixes for getters. Furthermore,
at some distant point we may want to add a getter for our own ID
so give this method a better name.

While we're here, I have noticed that the documentation comment lies.
This method should properly handle the case where peer ID is not known.
Peer IDs can't be empty so an empty result from underlying function
secure_session_get_remote_id() means that there is no peer ID stored
(i.e., the connection has not been negotiated yet).

* Rename getter for comparison result

Similarly, rename the getter without a "get" prefix because Rust APIs
prefer such naming.

* Unsafe impl Send for pointer types

Rust cannot automatically infer thread-safety traits for types which
use raw pointers. Secure Session and Secure Comparator are actually
movable across threads, but not safe for concurrent usage. Reflect
this by implementing Send but not Sync.

* Use ? operator instead of unwrap in API examples

Code snippets from documentation tend to be copy-pasted verbatim so
take care to avoid unexpected panics.
  • Loading branch information
ilammy committed Feb 15, 2019
1 parent 9b4725f commit 463f105
Show file tree
Hide file tree
Showing 8 changed files with 91 additions and 76 deletions.
2 changes: 1 addition & 1 deletion docs/examples/rust/secure_compare.rs
Expand Up @@ -83,7 +83,7 @@ fn main() {
}
}

if comparison.get_result().expect("result") {
if comparison.result().expect("result") {
println!("[+] match OK");
} else {
println!("[-] no match");
Expand Down
21 changes: 10 additions & 11 deletions src/wrappers/themis/rust/src/keys.rs
Expand Up @@ -355,11 +355,10 @@ impl KeyPair {
/// However, it does verify that _the kinds_ of the keys match: i.e., that they are both
/// either RSA or ECDSA keys. An error is returned if that’s not the case. You can check
/// the kind of the key beforehand via its `kind()` method.
pub fn try_join<S, P>(private_key: S, public_key: P) -> Result<KeyPair>
where
S: Into<PrivateKey>,
P: Into<PublicKey>,
{
pub fn try_join(
private_key: impl Into<PrivateKey>,
public_key: impl Into<PublicKey>,
) -> Result<KeyPair> {
let (private_key, public_key) = (private_key.into(), public_key.into());
match (private_key.kind(), public_key.kind()) {
(KeyKind::RsaPrivate, KeyKind::RsaPublic) => {}
Expand All @@ -383,7 +382,7 @@ impl RsaPrivateKey {
/// Parses a key from a byte slice.
///
/// Returns an error if the slice does not contain a valid RSA private key.
pub fn try_from_slice<T: AsRef<[u8]>>(bytes: T) -> Result<Self> {
pub fn try_from_slice(bytes: impl AsRef<[u8]>) -> Result<Self> {
let key = KeyBytes::copy_slice(bytes.as_ref())?;
match get_key_kind(&key)? {
KeyKind::RsaPrivate => Ok(Self { inner: key }),
Expand All @@ -403,7 +402,7 @@ impl RsaPublicKey {
/// Parses a key from a byte slice.
///
/// Returns an error if the slice does not contain a valid RSA public key.
pub fn try_from_slice<T: AsRef<[u8]>>(bytes: T) -> Result<Self> {
pub fn try_from_slice(bytes: impl AsRef<[u8]>) -> Result<Self> {
let key = KeyBytes::copy_slice(bytes.as_ref())?;
match get_key_kind(&key)? {
KeyKind::RsaPublic => Ok(Self { inner: key }),
Expand All @@ -423,7 +422,7 @@ impl EcdsaPrivateKey {
/// Parses a key from a byte slice.
///
/// Returns an error if the slice does not contain a valid ECDSA private key.
pub fn try_from_slice<T: AsRef<[u8]>>(bytes: T) -> Result<Self> {
pub fn try_from_slice(bytes: impl AsRef<[u8]>) -> Result<Self> {
let key = KeyBytes::copy_slice(bytes.as_ref())?;
match get_key_kind(&key)? {
KeyKind::EcdsaPrivate => Ok(Self { inner: key }),
Expand All @@ -443,7 +442,7 @@ impl EcdsaPublicKey {
/// Parses a key from a byte slice.
///
/// Returns an error if the slice does not contain a valid ECDSA public key.
pub fn try_from_slice<T: AsRef<[u8]>>(bytes: T) -> Result<Self> {
pub fn try_from_slice(bytes: impl AsRef<[u8]>) -> Result<Self> {
let key = KeyBytes::copy_slice(bytes.as_ref())?;
match get_key_kind(&key)? {
KeyKind::EcdsaPublic => Ok(Self { inner: key }),
Expand All @@ -468,7 +467,7 @@ impl PrivateKey {
/// Parses a key from a byte slice.
///
/// Returns an error if the slice does not contain a valid RSA or ECDSA private key.
pub fn try_from_slice<T: AsRef<[u8]>>(bytes: T) -> Result<Self> {
pub fn try_from_slice(bytes: impl AsRef<[u8]>) -> Result<Self> {
let key = KeyBytes::copy_slice(bytes.as_ref())?;
match get_key_kind(&key)? {
KeyKind::RsaPrivate => Ok(Self { inner: key }),
Expand All @@ -487,7 +486,7 @@ impl PublicKey {
/// Parses a key from a byte slice.
///
/// Returns an error if the slice does not contain a valid RSA or ECDSA public key.
pub fn try_from_slice<T: AsRef<[u8]>>(bytes: T) -> Result<Self> {
pub fn try_from_slice(bytes: impl AsRef<[u8]>) -> Result<Self> {
let key = KeyBytes::copy_slice(bytes.as_ref())?;
match get_key_kind(&key)? {
KeyKind::RsaPublic => Ok(Self { inner: key }),
Expand Down
8 changes: 5 additions & 3 deletions src/wrappers/themis/rust/src/secure_cell.rs
Expand Up @@ -100,6 +100,7 @@ use crate::utils::into_raw_parts;
///
/// This is modeless, basic cell. First you provide the master key to a new `SecureCell` object
/// then you select the desired operation mode and your Secure Cell is ready to go.
#[derive(Debug)]
pub struct SecureCell {
master_key: KeyBytes,
}
Expand Down Expand Up @@ -169,6 +170,7 @@ impl SecureCell {
/// # Ok(())
/// # }
/// ```
#[derive(Debug)]
pub struct SecureCellSeal(SecureCell);

impl SecureCellSeal {
Expand Down Expand Up @@ -488,6 +490,7 @@ fn decrypt_seal(master_key: &[u8], user_context: &[u8], message: &[u8]) -> Resul
/// # Ok(())
/// # }
/// ```
#[derive(Debug)]
pub struct SecureCellTokenProtect(SecureCell);

impl SecureCellTokenProtect {
Expand Down Expand Up @@ -544,7 +547,7 @@ impl SecureCellTokenProtect {
/// # fn main() -> Result<(), themis::Error> {
/// use themis::secure_cell::SecureCell;
///
/// let cell = SecureCell::with_key(b"password").unwrap().token_protect();
/// let cell = SecureCell::with_key(b"password")?.token_protect();
///
/// cell.encrypt_with_context(b"byte string", format!("owned string"))?;
/// cell.encrypt_with_context(&[1, 2, 3, 4, 5], vec![6, 7, 8, 9, 10])?;
Expand Down Expand Up @@ -884,10 +887,9 @@ fn decrypt_token_protect(
/// Note that in context imprint mode you *must* provide non-empty context. Also keep in mind that
/// Secure Cell cannot verify integrity and correctness of the decrypted data so you have to have
/// some other means in place to validate the output.
#[derive(Debug)]
pub struct SecureCellContextImprint(SecureCell);

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

impl SecureCellContextImprint {
/// Encrypts the provided message, combining it with provided user context, and returns
/// the encrypted data.
Expand Down
82 changes: 43 additions & 39 deletions src/wrappers/themis/rust/src/secure_comparator.rs
Expand Up @@ -81,7 +81,7 @@
//! request = comparison.proceed_compare(&reply)?;
//! }
//!
//! if !comparison.get_result()? {
//! if !comparison.result()? {
//! unimplemented!("handle failed comparison here");
//! }
//! # Ok(())
Expand Down Expand Up @@ -121,17 +121,17 @@
//! send(&reply); // This function should send the `reply` to the client.
//! }
//!
//! if !comparison.get_result()? {
//! if !comparison.result()? {
//! unimplemented!("handle failed comparison here");
//! }
//! # Ok(())
//! # }
//! ```
//!
//! Both the server and the client use [`get_result`] to get the comparison result
//! Both the server and the client use [`result`] to get the comparison result
//! after it [`is_complete`]:
//!
//! [`get_result`]: struct.SecureComparator.html#method.get_result
//! [`result`]: struct.SecureComparator.html#method.result
//! [`is_complete`]: struct.SecureComparator.html#method.is_complete

use std::os::raw::c_void;
Expand All @@ -151,10 +151,15 @@ use crate::utils::into_raw_parts;
/// Please see [module-level documentation][secure_comparator] for examples.
///
/// [secure_comparator]: index.html
#[derive(Debug)]
pub struct SecureComparator {
comp_ctx: *mut secure_comparator_t,
}

// It safe to move secure_comparator_t to another thread, it does not depend on any thread-local
// state. However, it needs external synchronization for safe concurrent usage (hence no Sync).
unsafe impl Send for SecureComparator {}

impl SecureComparator {
/// Prepares a new comparison.
///
Expand Down Expand Up @@ -188,7 +193,7 @@ impl SecureComparator {
/// a `SecureComparator` to make a new comparison.
///
/// You can use this method only before the comparison has been started. That is,
/// [`append_secret`] is safe call only before [`begin_compare`] or [`proceed_compare`].
/// [`append_secret`] is safe to call only before [`begin_compare`] or [`proceed_compare`].
/// It will fail with an error if you try to append more data when you’re in the middle of
/// a comparison or after it has been completed.
///
Expand All @@ -214,7 +219,7 @@ impl SecureComparator {
/// # Ok(())
/// # }
/// ```
pub fn append_secret<S: AsRef<[u8]>>(&mut self, secret: S) -> Result<()> {
pub fn append_secret(&mut self, secret: impl AsRef<[u8]>) -> Result<()> {
let (secret_ptr, secret_len) = into_raw_parts(secret.as_ref());

unsafe {
Expand Down Expand Up @@ -302,7 +307,7 @@ impl SecureComparator {
/// Please see [module-level documentation][secure_comparator] for examples.
///
/// [secure_comparator]: index.html
pub fn proceed_compare<D: AsRef<[u8]>>(&mut self, peer_data: D) -> Result<Vec<u8>> {
pub fn proceed_compare(&mut self, peer_data: impl AsRef<[u8]>) -> Result<Vec<u8>> {
let (peer_compare_data_ptr, peer_compare_data_len) = into_raw_parts(peer_data.as_ref());

let mut compare_data = Vec::new();
Expand Down Expand Up @@ -335,7 +340,6 @@ impl SecureComparator {
let error = Error::from_compare_status(status);
match error.kind() {
ErrorKind::CompareSendOutputToPeer => {}
// TODO: signal that this does not need to be sent
ErrorKind::Success => {}
_ => {
return Err(error);
Expand All @@ -348,6 +352,34 @@ impl SecureComparator {
Ok(compare_data)
}

/// Checks if this comparison is complete.
///
/// Comparison that failed irrecoverably due to an error is also considered complete.
///
/// # Examples
///
/// Typically you would use this method to terminate the comparison loop. Please see
/// [module-level documentation][secure_comparator] for examples.
///
/// [secure_comparator]: index.html
///
/// It is safe to call this method at any point, even if the comparison has not been initiated
/// yet (in which case it is obviously not complete):
///
/// ```
/// use themis::secure_comparator::SecureComparator;
///
/// let mut comparison = SecureComparator::new();
///
/// assert!(!comparison.is_complete());
/// ```
pub fn is_complete(&self) -> bool {
match self.result() {
Err(ref e) if e.kind() == ErrorKind::CompareNotReady => false,
_ => true,
}
}

/// Returns the result of comparison.
///
/// Let it be a surprise: `true` if data has been found equal on both peers, `false` otherwise.
Expand All @@ -370,7 +402,7 @@ impl SecureComparator {
/// comparison.append_secret(b"999-04-1234")?;
/// # other_peer.append_secret(b"999-04-1234")?;
///
/// assert!(comparison.get_result().is_err());
/// assert!(comparison.result().is_err());
///
/// // Perform comparison
/// #
Expand All @@ -382,11 +414,11 @@ impl SecureComparator {
/// # request = comparison.proceed_compare(&reply)?;
/// }
///
/// assert!(comparison.get_result().is_ok());
/// assert!(comparison.result().is_ok());
/// # Ok(())
/// # }
/// ```
pub fn get_result(&self) -> Result<bool> {
pub fn result(&self) -> Result<bool> {
let status = unsafe { secure_comparator_get_result(self.comp_ctx) };
let error = Error::from_match_status(status);
match error.kind() {
Expand All @@ -395,34 +427,6 @@ impl SecureComparator {
_ => Err(error),
}
}

/// Checks if this comparison is complete.
///
/// Comparison that failed irrecoverably due to an error is also considered complete.
///
/// # Examples
///
/// Typically you would use this method to terminate the comparison loop. Please see
/// [module-level documentation][secure_comparator] for examples.
///
/// [secure_comparator]: index.html
///
/// It is safe to call this method at any point, even if the comparison has not been initiated
/// yet (in which case it is obviously not complete):
///
/// ```
/// use themis::secure_comparator::SecureComparator;
///
/// let mut comparison = SecureComparator::new();
///
/// assert!(!comparison.is_complete());
/// ```
pub fn is_complete(&self) -> bool {
match self.get_result() {
Err(ref e) if e.kind() == ErrorKind::CompareNotReady => false,
_ => true,
}
}
}

impl Default for SecureComparator {
Expand Down
6 changes: 3 additions & 3 deletions src/wrappers/themis/rust/src/secure_message.rs
Expand Up @@ -101,7 +101,7 @@ use crate::utils::into_raw_parts;
/// # Ok(())
/// # }
/// ```
#[derive(Clone)]
#[derive(Debug)]
pub struct SecureMessage {
key_pair: KeyPair,
}
Expand Down Expand Up @@ -206,7 +206,7 @@ impl SecureMessage {
///
/// assert!(signed_message.windows(message.len()).any(|subslice| subslice == message));
/// ```
#[derive(Clone)]
#[derive(Debug)]
pub struct SecureSign {
private_key: PrivateKey,
}
Expand Down Expand Up @@ -314,7 +314,7 @@ impl SecureSign {
///
/// assert!(secure_c.verify(&signed_message).is_err());
/// ```
#[derive(Clone)]
#[derive(Debug)]
pub struct SecureVerify {
public_key: PublicKey,
}
Expand Down
10 changes: 7 additions & 3 deletions src/wrappers/themis/rust/src/secure_session.rs
Expand Up @@ -143,6 +143,10 @@ struct SecureSessionContext {
last_error: Option<TransportError>,
}

// It safe to move secure_session_t to another thread, it does not depend on any thread-local
// state. However, it needs external synchronization for safe concurrent usage (hence no Sync).
unsafe impl Send for SecureSession {}

/// Transport delegate for Secure Session.
///
/// This is an interface you need to provide for Secure Session operation.
Expand Down Expand Up @@ -412,8 +416,8 @@ impl SecureSession {

/// Returns ID of the remote peer.
///
/// This method will return an error if the connection has not been established yet.
pub fn get_remote_id(&self) -> Result<Vec<u8>> {
/// Returns `None` if the connection has not been established yet and there is no peer.
pub fn remote_peer_id(&self) -> Result<Option<Vec<u8>>> {
let mut id = Vec::new();
let mut id_len = 0;

Expand All @@ -437,7 +441,7 @@ impl SecureSession {
id.set_len(id_len);
}

Ok(id)
Ok(if id.is_empty() { None } else { Some(id) })
}

/// Initiates connection to the remote peer.
Expand Down

0 comments on commit 463f105

Please sign in to comment.