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

Improve and extend Secure Session API docs #380

Merged
merged 8 commits into from
Feb 15, 2019

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Feb 14, 2019

I'm closing in on the API of Secure Session which looks okay for me. This PR adds more information on how to use Secure Session to API docs, and updates the interface. You can view the docs by running

cargo doc --open

Some notable changes:

  • constructor is now called SecureSession::new() and it panics instead of returning a Result
  • some other methods have been renamed for consistency with other language wrappers:
    • callback API: connect, negotiate, send, receive
    • buffer-oriented API: connect_request, negotiate_reply, wrap, unwrap
  • impl Traits are used instead of explicit generics

The docs currently refer to nonexistent examples of Secure Session usage. They are currently cooking in a separate branch and will be added later in a separate pull request.

Write module-level docs based on description on Wiki. Describe API
flavors of Secure Session and update the method descriptions as well.
Rename the constructor to just "new()" and make it panic instead of
returning a Result.

There are no user-preventable errors which can be avoided to not cause
a panic. A construction failure usually means either an out-of-memory
condition, or some serious failure in Secure Session invariants or
crypto backend usage by Secure Session. We'd better abort in this case.

As you can see in the tests, we never handle the error and just expect
the construction to always succeed. The user will not get any value out
of error checks like that.
Group callback API and buffer-oriented API together in rustdoc output.
This makes navigation a bit easier.
Use the same naming as other language wrappers do:

- connect_request, wrap, unwrap for buffer-oriented API
- connect, send, receive for callback API

Other wrappers do not have a separate method for negotiation so we have
to invent something here. Keep the naming consistent.
Replace explicit templates with impl Trait which makes code a bit
easier to read.
@ilammy ilammy added the W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates label Feb 14, 2019
Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

LGTM

@Lagovas
Copy link
Collaborator

Lagovas commented Feb 14, 2019

  • constructor is now called SecureSession::new() and it panics instead of returning a Result

why panic? usually, secure session will be created in runtime (not at compile time). how users can to handle errors from constructor without abort of program?

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 14, 2019

@Lagovas, I have decided to use panics to not bother users with errors which they are unlikely to be able to handle in any meaningful way.

Rust wrapper uses Rust type system to prevent a number of errors possible with the underlying C API:

  • null pointers
  • invalid or malformed private key
  • invalid transport implementation

It is not possible for the user of Secure Session to compile a program which passes invalid arguments to the constructor.

Basically, the only errors that left are:

  • insufficient memory available
  • violations of Secure Session invariants
  • failures in underlying crypto backend

OOM is (currently) indicated by panics in Rust and there is no way to handle it. The other two cases are programming bugs in Secure Session. The users of Secure Session cannot handle these conditions. There is no meaningful action to perform for an error indication returned from the constructor other than cease usage of Secure Session in the whole application.

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 15, 2019

It turns out that I was mistaken about errors during Secure Session construction. There is one possible error which is not prevented by the type system: the client ID must be non-empty.

Therefore I'll (partially) revert the commit which replaced Result with panic. The constructor will still return a Result.

This partially reverts commit aa4b50b.

It turned out that Secure Sesssion construction can fail due to
preventable errors. On of them is invalid client ID which must be
non-empty.

Revert back to returning an Err instead of panicking, but keep the new
name of the constructor.
Secure Session is not able to negotiate connection if the client ID
is empty so do an early check and return an error to the user if the
provided client ID is invalid.

Using expect_err() with Result revealed tha SecureSession does not have
a Debug impl. All public types should have one so invent something.
@ilammy ilammy merged commit 27d9402 into cossacklabs:master Feb 15, 2019
@ilammy ilammy deleted the ssession-api branch February 19, 2019 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants