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

Forward transport errors from Secure Session callbacks #375

Merged
merged 5 commits into from Feb 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
89 changes: 82 additions & 7 deletions src/wrappers/themis/rust/src/error.rs
Expand Up @@ -26,6 +26,8 @@ use bindings::{
THEMIS_SSESSION_SEND_OUTPUT_TO_PEER, THEMIS_SSESSION_TRANSPORT_ERROR, THEMIS_SUCCESS,
};

use crate::secure_session::TransportError;

/// Themis status code.
pub(crate) use bindings::themis_status_t;

Expand All @@ -39,7 +41,7 @@ pub type Result<T> = result::Result<T, Error>;
/// details.
///
/// [`ErrorKind`]: enum.ErrorKind.html
#[derive(Debug, Clone, Eq, PartialEq)]
#[derive(Debug, Eq, PartialEq)]
pub struct Error {
kind: ErrorKind,
}
Expand Down Expand Up @@ -71,7 +73,9 @@ impl Error {
let kind = match status as u32 {
THEMIS_SSESSION_SEND_OUTPUT_TO_PEER => ErrorKind::SessionSendOutputToPeer,
THEMIS_SSESSION_KA_NOT_FINISHED => ErrorKind::SessionKeyAgreementNotFinished,
THEMIS_SSESSION_TRANSPORT_ERROR => ErrorKind::SessionTransportError,
THEMIS_SSESSION_TRANSPORT_ERROR => {
ErrorKind::SessionTransportError(TransportError::unspecified())
}
THEMIS_SSESSION_GET_PUB_FOR_ID_CALLBACK_ERROR => {
ErrorKind::SessionGetPublicKeyForIdError
}
Expand All @@ -80,6 +84,12 @@ impl Error {
Error { kind }
}

/// Wraps a transport error reported by Secure Session.
pub(crate) fn from_transport_error(error: TransportError) -> Error {
let kind = ErrorKind::SessionTransportError(error);
Error { kind }
}

/// Converts status codes returned by Secure Comparator data exchange.
pub(crate) fn from_compare_status(status: themis_status_t) -> Error {
let kind = match status as u32 {
Expand All @@ -101,8 +111,8 @@ impl Error {
}

/// Returns the corresponding `ErrorKind` for this error.
pub fn kind(&self) -> ErrorKind {
self.kind
pub fn kind(&self) -> &ErrorKind {
&self.kind
}
}

Expand All @@ -124,7 +134,9 @@ impl fmt::Display for Error {

ErrorKind::SessionSendOutputToPeer => write!(f, "send key agreement data to peer"),
ErrorKind::SessionKeyAgreementNotFinished => write!(f, "key agreement not finished"),
ErrorKind::SessionTransportError => write!(f, "transport layer error"),
ErrorKind::SessionTransportError(ref details) => {
write!(f, "transport layer error: {}", details)
}
ErrorKind::SessionGetPublicKeyForIdError => {
write!(f, "failed to get public key for ID")
}
Expand All @@ -143,8 +155,12 @@ impl fmt::Display for Error {
/// are specific to particular functions, and some are used internally by the library.
///
/// [`Error`]: struct.Error.html
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
#[derive(Debug)]
pub enum ErrorKind {
/*
* If you add a new error kind then please add it to the error_kinds_equal() function below
* as well. Unfortunately, we cannot derive PartialEq implementation automatically.
*/
/// Catch-all generic error.
///
/// If you encounter this error kind then the Themis binding is likely to be out of sync with
Expand Down Expand Up @@ -181,7 +197,7 @@ pub enum ErrorKind {
/// Attempt to use Secure Session before completing key exchange.
SessionKeyAgreementNotFinished,
/// Transport layer returned error.
SessionTransportError,
SessionTransportError(TransportError),
/// Could not retrieve a public key corresponding to peer ID.
SessionGetPublicKeyForIdError,

Expand All @@ -203,3 +219,62 @@ pub enum ErrorKind {
/// Attempt to use Secure Comparator before completing nonce exchange.
CompareNotReady,
}

// TransportError does not implement PartialEq therefore it's not possible to derive the
// implementation for ErrorKind automatically. TransportErrors cannot be compared because
// they either contain arbitrary human-readable strings or abstract boxed errors which
// may have different types. However, we really need to be able to compare ErrorKinds.
// Implement comparison by ignoring differences in TransportError details, comparing
// just *kinds* of errors, not their values. This is a full equivalence relationship.

impl Eq for ErrorKind {}

impl PartialEq for ErrorKind {
fn eq(&self, other: &ErrorKind) -> bool {
error_kinds_equal(self, other)
}
}

impl PartialEq<&ErrorKind> for ErrorKind {
fn eq(&self, other: &&ErrorKind) -> bool {
error_kinds_equal(self, other)
}
}

impl PartialEq<ErrorKind> for &ErrorKind {
fn eq(&self, other: &ErrorKind) -> bool {
error_kinds_equal(self, other)
}
}

fn error_kinds_equal(lhs: &ErrorKind, rhs: &ErrorKind) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

will be good to add a comment to ErrorKind definition that we should update error_kinds_equal if some new values will be added

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah... That's unfortunate but the catch-all branch in this match makes it possible to add a new variant to ErrorKind without adding a corresponding line to the comparison implementation.

I guess there's no other way around, I'll add a comment and let's hope it will be noticed.

match (lhs, rhs) {
(ErrorKind::UnknownError(lhs), ErrorKind::UnknownError(rhs)) => (lhs == rhs),
(ErrorKind::Success, ErrorKind::Success) => true,

(ErrorKind::Fail, ErrorKind::Fail) => true,
(ErrorKind::InvalidParameter, ErrorKind::InvalidParameter) => true,
(ErrorKind::NoMemory, ErrorKind::NoMemory) => true,
(ErrorKind::BufferTooSmall, ErrorKind::BufferTooSmall) => true,
(ErrorKind::DataCorrupt, ErrorKind::DataCorrupt) => true,
(ErrorKind::InvalidSignature, ErrorKind::InvalidSignature) => true,
(ErrorKind::NotSupported, ErrorKind::NotSupported) => true,

(ErrorKind::SessionSendOutputToPeer, ErrorKind::SessionSendOutputToPeer) => true,
(ErrorKind::SessionKeyAgreementNotFinished, ErrorKind::SessionKeyAgreementNotFinished) => {
true
}
// Ignore transport error details.
(ErrorKind::SessionTransportError(_), ErrorKind::SessionTransportError(_)) => true,
(ErrorKind::SessionGetPublicKeyForIdError, ErrorKind::SessionGetPublicKeyForIdError) => {
true
}

(ErrorKind::CompareSendOutputToPeer, ErrorKind::CompareSendOutputToPeer) => true,
(ErrorKind::CompareMatch, ErrorKind::CompareMatch) => true,
(ErrorKind::CompareNoMatch, ErrorKind::CompareNoMatch) => true,
(ErrorKind::CompareNotReady, ErrorKind::CompareNotReady) => true,

_ => false,
}
}
59 changes: 45 additions & 14 deletions src/wrappers/themis/rust/src/secure_session.rs
Expand Up @@ -48,6 +48,7 @@ pub struct SecureSession {
struct SecureSessionContext {
callbacks: secure_session_user_callbacks_t,
transport: Box<dyn SecureSessionTransport>,
last_error: Option<TransportError>,
}

/// Transport delegate for Secure Session.
Expand Down Expand Up @@ -265,6 +266,7 @@ impl SecureSession {
user_data: std::ptr::null_mut(),
},
transport: Box::new(transport),
last_error: None,
});
context.callbacks.user_data = context_as_user_data(&context);

Expand Down Expand Up @@ -567,6 +569,7 @@ impl SecureSession {
// Furthermore, Themis expects the send callback to send the whole message so it is kinda
// pointless to return the amount of bytes send. The receive callback returns accurate number
// of bytes, but I do not really like the Rust interface this implies. It could be made better.
const THEMIX_MAX_ERROR: isize = 21;

/// Sends a message to the remote peer.
///
Expand All @@ -582,7 +585,14 @@ impl SecureSession {
unsafe {
let length =
secure_session_send(self.session, message_ptr as *const c_void, message_len);
if length <= 21 {
if length == TRANSPORT_FAILURE {
let error = self.context.last_error.take().expect("missing error");
return Err(Error::from_transport_error(error));
}
if length == TRANSPORT_OVERFLOW {
return Err(Error::with_kind(ErrorKind::BufferTooSmall));
}
if length <= Self::THEMIX_MAX_ERROR {
return Err(Error::from_session_status(length as themis_status_t));
}
}
Expand Down Expand Up @@ -610,7 +620,14 @@ impl SecureSession {
message.as_mut_ptr() as *mut c_void,
message.capacity(),
);
if length <= 21 {
if length == TRANSPORT_FAILURE {
let error = self.context.last_error.take().expect("missing error");
return Err(Error::from_transport_error(error));
}
if length == TRANSPORT_OVERFLOW {
return Err(Error::with_kind(ErrorKind::BufferTooSmall));
}
if length <= Self::THEMIX_MAX_ERROR {
return Err(Error::from_session_status(length as themis_status_t));
}
debug_assert!(length as usize <= message.capacity());
Expand All @@ -636,6 +653,13 @@ impl SecureSession {
pub fn negotiate_transport(&mut self) -> Result<()> {
unsafe {
let result = secure_session_receive(self.session, ptr::null_mut(), 0);
if result == TRANSPORT_FAILURE {
let error = self.context.last_error.take().expect("missing error");
return Err(Error::from_transport_error(error));
}
if result == TRANSPORT_OVERFLOW {
return Err(Error::with_kind(ErrorKind::BufferTooSmall));
}
let error = Error::from_session_status(result as themis_status_t);
if error.kind() != ErrorKind::Success {
return Err(error);
Expand All @@ -659,19 +683,24 @@ unsafe fn user_data_as_context<'a>(ptr: *mut c_void) -> &'a mut SecureSessionCon
&mut *(ptr as *mut SecureSessionContext)
}

const TRANSPORT_FAILURE: isize = -1;
const TRANSPORT_OVERFLOW: isize = -2;

unsafe extern "C" fn send_data(
data_ptr: *const u8,
data_len: usize,
user_data: *mut c_void,
) -> isize {
let data = byte_slice_from_ptr(data_ptr, data_len);
let transport = &mut user_data_as_context(user_data).transport;
let context = user_data_as_context(user_data);

transport
.send_data(data)
.ok()
.and_then(as_isize)
.unwrap_or(-1)
match context.transport.send_data(data) {
Ok(sent_bytes) => as_isize(sent_bytes).unwrap_or(TRANSPORT_OVERFLOW),
Err(error) => {
context.last_error = Some(error);
TRANSPORT_FAILURE
}
}
}

unsafe extern "C" fn receive_data(
Expand All @@ -680,13 +709,15 @@ unsafe extern "C" fn receive_data(
user_data: *mut c_void,
) -> isize {
let data = byte_slice_from_ptr_mut(data_ptr, data_len);
let transport = &mut user_data_as_context(user_data).transport;
let context = user_data_as_context(user_data);

transport
.receive_data(data)
.ok()
.and_then(as_isize)
.unwrap_or(-1)
match context.transport.receive_data(data) {
Ok(received_bytes) => as_isize(received_bytes).unwrap_or(TRANSPORT_OVERFLOW),
Err(error) => {
context.last_error = Some(error);
TRANSPORT_FAILURE
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks better with constants usage

}
}
}

unsafe extern "C" fn state_changed(event: c_int, user_data: *mut c_void) {
Expand Down