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

Conversation

Projects
None yet
3 participants
@ilammy
Copy link
Member

ilammy commented Feb 11, 2019

And one more usability improvement for Secure Session. Currently it simply ignores and loses any TransportErrors that happen inside the transport callbacks. This was fine when errors were simply () but now they can be rich, detailed errors. The users are likely to be interested in the reason why their transport callbacks fail. Let's thread the error through the C call stack and return it the caller.

This behavior is currently not tested. I'll add tests in a separate PR which will also improve existing tests for Secure Session. (And note all the breaking changes in the changelog as well.)

Add TransportError to themis::Error

We will need to return TransportErrors from Secure Session methods on failures to extend the ErrorKind enumeration to actually contain the transport error inside its ErrorKind::SessionTransportError variant.

This change has a number of implications. First of all, TransportError is not copyable. Therefore we have to let go of the Clone and Copy implementations on the ErrorKind as well as the Clone impl on Error. One should not usually copy errors anyway so this should be okay. Note that Error::kind() now returns a reference to the stored error kind instead of its copy.

Another important thing is that TransportError does not implement PartialEq as it is not possible to compare abstract errors in any meaningful way. This implies that we cannot automatically derive implementation of PartialEq for ErrorKind. We have to implement it manually. We need to compare kinds of errors here so it is okay to simply ignore the details of transport errors and treat them as equivalent.

Forward transport errors from Secure Session callbacks

Now that we are able to store TransportError inside of a themis::Error we can actually forward the error from the callback to the method call. With this the user can actually see why the transport layer has failed.

Use the provided SecureSessionContext to temporarily store the error while the control is still in the C code. Then we extract the error and return it. Such implementation does not support concurrent usage of Secure Session from multiple threads but it's not thread-safe anyway so it is fine to use this approach.

We can use negative return values to indicate implementation-specific errors in Secure Session callbacks. The C code returns them as is for us to inspect. While we're here, stop using bare constant -1 and give it a name. Also, handle the overflow with a different error kind.

ilammy added some commits Feb 11, 2019

Add TransportError to themis::Error
We will need to return TransportErrors from Secure Session methods on
failures to extend the ErrorKind enumeration to actually contain the
transport error inside its ErrorKind::SessionTransportError variant.

This change has a number of implications. First of all, TransportError
is not copyable. Therefore we have to let go of the Clone and Copy
implementations on the ErrorKind as well as the Clone impl on Error.
One should not usually copy errors anyway so this should be okay.
Note that Error::kind() now returns a reference to the stored error
kind instead of its copy.

Another important thing is that TransportError does not implement
PartialEq as it is not possible to compare abstract errors in any
meaningful way. This implies that we cannot automatically derive
implementation of PartialEq for ErrorKind. We have to implement it
manually. We need to compare *kinds* of errors here so it is okay
to simply ignore the details of transport errors and treat them
as equivalent.
Forward transport errors from Secure Session callbacks
Now that we are able to store TransportError inside of a themis::Error
we can actually forward the error from the callback to the method call.
With this the user can actually see why the transport layer has failed.

Use the provided SecureSessionContext to temporarily store the error
while the control is still in the C code. Then we extract the error
and return it.

Such implementation does not support concurrent usage of Secure Session
from multiple threads but it's not thread-safe anyway so it is fine to
use this approach.

We can use negative return values to indicate implementation-specific
errors in Secure Session callbacks. The C code returns them as is for
us to inspect. While we're here, stop using bare constant "-1" and give
it a name. Also, handle the overflow with a different error kind.

@ilammy ilammy added the rust label Feb 11, 2019

@ilammy ilammy requested review from vixentael and Lagovas Feb 11, 2019

}
if length == TRANSPORT_OVERFLOW {
return Err(Error::with_kind(ErrorKind::BufferTooSmall));
}
if length <= 21 {

This comment has been minimized.

@Lagovas

Lagovas Feb 11, 2019

Contributor

what mean 21?

This comment has been minimized.

@ilammy

ilammy Feb 11, 2019

Author Member

A magic number Actually that's the maximum numerical value of themis_status_t — THEMIS_SSESSION_GET_PUB_FOR_ID_CALLBACK_ERROR.

It is a necessary hack because the current callback API of Secure Session is not able to distinguish between errors and short reads/writes. For example, if secure_session_receive() returns 15 then it may mean that 15 bytes of data has been received, or it can also mean a THEMIS_DATA_CORRUPT error. There is an outstanding issue for that somewhere...

I guess this constant deserves a name as well.

Ok(received_bytes) => as_isize(received_bytes).unwrap_or(TRANSPORT_OVERFLOW),
Err(error) => {
context.last_error = Some(error);
TRANSPORT_FAILURE

This comment has been minimized.

@Lagovas

Lagovas Feb 11, 2019

Contributor

looks better with constants usage

}
}

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

This comment has been minimized.

@Lagovas

Lagovas Feb 11, 2019

Contributor

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

This comment has been minimized.

@ilammy

ilammy Feb 11, 2019

Author Member

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.

Forward errors during channel negotiation
There's another secure_session_receive() call here in this method. It
too can return special error codes.
@ilammy

This comment has been minimized.

Copy link
Member Author

ilammy commented Feb 12, 2019

Writing tests for every single method and use case of Secure Session turned out to be useful (surprise-surpise). Then have uncovered that I have missed another place where a special error code may be returned. I guess this should be it.

@ilammy ilammy merged commit a7dee76 into cossacklabs:master Feb 12, 2019

6 checks passed

ci/circleci: android Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: php5 Your tests passed on CircleCI!
Details
ci/circleci: php70 Your tests passed on CircleCI!
Details
ci/circleci: php71 Your tests passed on CircleCI!
Details
ci/circleci: x86_64 Your tests passed on CircleCI!
Details

@ilammy ilammy deleted the ilammy:forward-transport-error branch Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment