Skip to content

Add a new IO abstraction #237

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

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Add a new IO abstraction #237

merged 1 commit into from
Mar 22, 2023

Conversation

jethrogb
Copy link
Member

Implement an IO abstraction as previously suggested to replace Read+Write if you don't have that available. This allows you to layer any Io types like you would Read+Write.

cc @DrTobe hope you can live with the API change.

@jethrogb jethrogb requested a review from Taowyoo March 20, 2023 22:45
Copy link
Collaborator

@Taowyoo Taowyoo left a comment

Choose a reason for hiding this comment

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

LGTM.
Thank you for this PR for provide an better unified interface of IO. Learn a lot
It make it easier to add the async support.
Will refactor #266 based on this. It seems I could follow same path to add maker type of Async AnyIo, Steam and implement related traits

@DrTobe
Copy link
Contributor

DrTobe commented Mar 21, 2023

If I understand it correctly, you did two things at once in this PR:

  1. IoCallback + IoCallbackUnsafe as a safe interface

That is a nice and safe abstraction over the C interface and removes the necessity to manually implement a C callback interface. This part is a huge step forward 👍

  1. Introducing the Io trait

to layer any Io types like you would Read+Write

It took me a while to wrap my head around this but I guess it is meant to be an interface which the SSL context can wrap transparently like Io + ssl::Context = Io in the same way as Read+Write + ssl::Context = Read+Write, right?

I see the motivation here because it would theoretically allow consumers of this interface to work independently from the underlying transport layer but since this trait is defined in this library, this would only work for consumers which explicitly have mbedtls in mind. So it would only work for users which want to use mbedtls anyways. For this idea to work in a generic way, the trait would need to be defined outside of this library but this would just be like the start of another core_io library which we just got rid of. In general, I would really like to see a generic interface like that but I do not think that this belongs into rust-mbedtls.

Regarding the fact that there are now three similarly named traits, I think there is a loss of readibility. Additionally, I suspect that it might get harder to use ssl::Context now because there are multiple methods with the same name from different traits (and the private version in the Context itself) and it is not immediately clear what trait bounds should be used or which traits need to be implemented when. So to me, the possible advantage is less than the drawbacks here.

If you stick to the Io trait approach, I would like to see the "to layer any Io types like you would Read+Write" idea to be better documented for new users to grasp what these different traits are about.

@jethrogb
Copy link
Member Author

jethrogb commented Mar 21, 2023

This part is a huge step forward 👍

Thanks

It took me a while to wrap my head around this but I guess it is meant to be an interface which the SSL context can wrap transparently like Io + ssl::Context = Io in the same way as Read+Write + ssl::Context = Read+Write, right?

Right.

I see the motivation here because it would theoretically allow consumers of this interface to work independently from the underlying transport layer but since this trait is defined in this library, this would only work for consumers which explicitly have mbedtls in mind. So it would only work for users which want to use mbedtls anyways. For this idea to work in a generic way, the trait would need to be defined outside of this library but this would just be like the start of another core_io library which we just got rid of. In general, I would really like to see a generic interface like that but I do not think that this belongs into rust-mbedtls.

Agreed but the ecosystem hasn't really developed something like this. Especially also considering the datagram-based abstraction. My thought was that by providing this trait, users can always interface their own traits with it as they see fit.

@DrTobe
Copy link
Contributor

DrTobe commented Mar 21, 2023

Agreed but the ecosystem hasn't really developed something like this. Especially also considering the datagram-based abstraction.

Yes, that's right. Personally, I would not do it like this but I do not object too strongly here. So feel free to merge this.

@jethrogb jethrogb force-pushed the jb/io-abstraction branch from f4a6a5d to 3bde9bb Compare March 22, 2023 13:20
@jethrogb
Copy link
Member Author

Added some more documentation on the io module and Io trait.

bors r=Taowyoo

@bors
Copy link
Contributor

bors bot commented Mar 22, 2023

Build succeeded:

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 74dbd1b into master Mar 22, 2023
bors bot added a commit that referenced this pull request Apr 19, 2023
239: Add async IO &  async support for TLS r=jethrogb a=Taowyoo

# TLDR
This PR added:
- Async version of IO abstraction ( only stream part, IO abstraction is introduced on PR: [Add a new IO abstraction #237](237) )
- Implementation for TLS 

# Detailed Explanation

## Background

We use `mbedtls` for crypto and SSL handshake in rust. `mbedtls::ssl` module contains most FFI wrapper for c mbedtls API.

### How `mbedtls::ssl` works

To setup a TLS client or a TLS server with `mbedtls`, mainly you need to provide two things to `mbedtls`:
- [Config](https://github.com/fortanix/rust-mbedtls/blob/c39d93a19d043587dc2346fb4c54501e71190162/mbedtls/src/ssl/config.rs#L148): a config object containing settings and parameters needed for TLS handshake
- [`BIO`][1]: similar to `openssl`, `mbedtls` need you to provide a `BIO` to it. `BIO` stands for **Basic I/O abstraction**, at here you could simply treat them as source & destination of data such as a TCP socket or file i/o. In code level, it represent as two callbacks in rust:
	```rust
	// representation of the `mbedtls_ssl_send_t` and `mbedtls_ssl_recv_t` callback function pointers.
	unsafe extern "C" fn call_recv(user_data: *mut c_void, data: *mut c_uchar, len: size_t) -> c_int where Self: Sized;
	unsafe extern "C" fn call_send(user_data: *mut c_void, data: *const c_uchar, len: size_t) -> c_int where Self: Sized;
	```
In `mbedtls`, we pass function pointers and user_data pointer to c mbedtls lib through FFI function `ssl_set_bio`.
During handshake and data transfer, the c mbedtls lib will utilize those two callbacks to send or receive data from underlying IO.

### IO abstraction

The IO abstraction PR #237 created and implemented safe rust abstraction of `BIO`:

<details>
<summary>Details</summary>

- Introduce unsafe trait [`IoCallbackUnsafe`](https://github.com/fortanix/rust-mbedtls/blob/c39d93a19d043587dc2346fb4c54501e71190162/mbedtls/src/ssl/io.rs#L36) to represent `mbedtls_ssl_send_t` and `mbedtls_ssl_recv_t` callback function pointers.
- Introduce safe trait [`IoCallback`](https://github.com/fortanix/rust-mbedtls/blob/c39d93a19d043587dc2346fb4c54501e71190162/mbedtls/src/ssl/io.rs#L48) : A safe representation of above trait. Also provides generic implementation of `IoCallback` for `IoCallbackUnsafe`. So user only need to implement this trait.
- Introduce trait [`Io`](https://github.com/fortanix/rust-mbedtls/blob/c39d93a19d043587dc2346fb4c54501e71190162/mbedtls/src/ssl/io.rs#L102) to represent a duplex socket or file descriptor that can be read from or written to in case you cannot use `std::io::Read` and `std::io::Write`.
- To integrate TCP, `std::net::TcpStream` implemented `std::io::Read` and `std::io::Write`:
	- Implemented `IoCallback` with Marker types:  `Stream` for `T: std::io::Read + std::io::Write`
	- Implemented `IoCallbackUnsafe` with Marker types:  `Stream` for `Contex<T>`
	- So you can pass `std::net::TcpStream` directly to [`mbedtls::ssl::context::Context::establish`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/context.rs#L234)
- To integrate UDP, since `std::net::UdpSocket` does not implement and reader/writer trait:
	- Introduced `ConnectedUdpSocket` as wrapper of `std::net::UdpSocket` and implemented `Io` for it
	- Implemented `IoCallback` with Marker types:  `AnyIo` for `T: Io`
	- Implemented `IoCallbackUnsafe` with Marker types:  `AnyIo` for `Contex<T>`
	- So you can pass `ConnectedUdpSocket` directly to [`mbedtls::ssl::context::Context::establish`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/context.rs#L234)

</details>

##  Async Support for TLS

###  Follow same path 

To add async support, we could use `tokio::net::TcpStream` and traits `tokio::io::Read` and `tokio::io::Write` and follow the same path of what we did for TCP in [IO abstraction](###IO_abstraction):

<details>
<summary>Details</summary>

- (new) Add marker type for async Steam IO: `AsyncStream`
- (new) Add [`establish_async`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/async_io.rs#L144) for `Context<T>`, this async function is implemented by wrapping the original blocking function [`handshake`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/context.rs#L271) in a custom `Future` implementation.
- Implemented `IoCallback` with Marker types:  `AsyncStream` for `T: tokio::io::Read + tokio::io::Write`
- Implemented `IoCallbackUnsafe` with Marker types:  `AsyncStream` for `Contex<T>`
- So you can pass `tokio::net::TcpStream` directly to [`Context::establish_async`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/async_io.rs#L144)

</details>

### Async need Context

But the key obstacle here is async code need context.
The functions needed by [`tokio::io::Read`](https://docs.rs/tokio/latest/tokio/io/trait.AsyncRead.html) and [`tokio::io::Write` ](https://docs.rs/tokio/latest/tokio/io/trait.AsyncWrite.html) both need a parameter: `cx` whose type is `std::task::Context`.

To address this, implementation need to adjust:
- `IoCallback` `impl`'s now are for a tuple `(&'a mut TaskContext<'b>, &'c mut T)` so that `Context` is accessible in the callback
- Introduced function [`with_bio_async`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/context.rs#L199) to use a callback function to wrap the `Context` and callback functions' data pointer together and pass it to `mbedtls` through `set_bio_raw`

**Note**:
The async support for DTLS will be added in another PR: #241

[1]: https://www.openssl.org/docs/man1.1.1/man7/bio.html

Co-authored-by: Yuxiang Cao <yuxiang.cao@fortanix.com>
bors bot added a commit that referenced this pull request Apr 19, 2023
239: Add async IO &  async support for TLS r=[jethrogb] a=Taowyoo

# TLDR
This PR added:
- Async version of IO abstraction ( only stream part, IO abstraction is introduced on PR: [Add a new IO abstraction #237](237) )
- Implementation for TLS 

# Detailed Explanation

## Background

We use `mbedtls` for crypto and SSL handshake in rust. `mbedtls::ssl` module contains most FFI wrapper for c mbedtls API.

### How `mbedtls::ssl` works

To setup a TLS client or a TLS server with `mbedtls`, mainly you need to provide two things to `mbedtls`:
- [Config](https://github.com/fortanix/rust-mbedtls/blob/c39d93a19d043587dc2346fb4c54501e71190162/mbedtls/src/ssl/config.rs#L148): a config object containing settings and parameters needed for TLS handshake
- [`BIO`][1]: similar to `openssl`, `mbedtls` need you to provide a `BIO` to it. `BIO` stands for **Basic I/O abstraction**, at here you could simply treat them as source & destination of data such as a TCP socket or file i/o. In code level, it represent as two callbacks in rust:
	```rust
	// representation of the `mbedtls_ssl_send_t` and `mbedtls_ssl_recv_t` callback function pointers.
	unsafe extern "C" fn call_recv(user_data: *mut c_void, data: *mut c_uchar, len: size_t) -> c_int where Self: Sized;
	unsafe extern "C" fn call_send(user_data: *mut c_void, data: *const c_uchar, len: size_t) -> c_int where Self: Sized;
	```
In `mbedtls`, we pass function pointers and user_data pointer to c mbedtls lib through FFI function `ssl_set_bio`.
During handshake and data transfer, the c mbedtls lib will utilize those two callbacks to send or receive data from underlying IO.

### IO abstraction

The IO abstraction PR #237 created and implemented safe rust abstraction of `BIO`:

<details>
<summary>Details</summary>

- Introduce unsafe trait [`IoCallbackUnsafe`](https://github.com/fortanix/rust-mbedtls/blob/c39d93a19d043587dc2346fb4c54501e71190162/mbedtls/src/ssl/io.rs#L36) to represent `mbedtls_ssl_send_t` and `mbedtls_ssl_recv_t` callback function pointers.
- Introduce safe trait [`IoCallback`](https://github.com/fortanix/rust-mbedtls/blob/c39d93a19d043587dc2346fb4c54501e71190162/mbedtls/src/ssl/io.rs#L48) : A safe representation of above trait. Also provides generic implementation of `IoCallback` for `IoCallbackUnsafe`. So user only need to implement this trait.
- Introduce trait [`Io`](https://github.com/fortanix/rust-mbedtls/blob/c39d93a19d043587dc2346fb4c54501e71190162/mbedtls/src/ssl/io.rs#L102) to represent a duplex socket or file descriptor that can be read from or written to in case you cannot use `std::io::Read` and `std::io::Write`.
- To integrate TCP, `std::net::TcpStream` implemented `std::io::Read` and `std::io::Write`:
	- Implemented `IoCallback` with Marker types:  `Stream` for `T: std::io::Read + std::io::Write`
	- Implemented `IoCallbackUnsafe` with Marker types:  `Stream` for `Contex<T>`
	- So you can pass `std::net::TcpStream` directly to [`mbedtls::ssl::context::Context::establish`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/context.rs#L234)
- To integrate UDP, since `std::net::UdpSocket` does not implement and reader/writer trait:
	- Introduced `ConnectedUdpSocket` as wrapper of `std::net::UdpSocket` and implemented `Io` for it
	- Implemented `IoCallback` with Marker types:  `AnyIo` for `T: Io`
	- Implemented `IoCallbackUnsafe` with Marker types:  `AnyIo` for `Contex<T>`
	- So you can pass `ConnectedUdpSocket` directly to [`mbedtls::ssl::context::Context::establish`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/context.rs#L234)

</details>

##  Async Support for TLS

###  Follow same path 

To add async support, we could use `tokio::net::TcpStream` and traits `tokio::io::Read` and `tokio::io::Write` and follow the same path of what we did for TCP in [IO abstraction](###IO_abstraction):

<details>
<summary>Details</summary>

- (new) Add marker type for async Steam IO: `AsyncStream`
- (new) Add [`establish_async`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/async_io.rs#L144) for `Context<T>`, this async function is implemented by wrapping the original blocking function [`handshake`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/context.rs#L271) in a custom `Future` implementation.
- Implemented `IoCallback` with Marker types:  `AsyncStream` for `T: tokio::io::Read + tokio::io::Write`
- Implemented `IoCallbackUnsafe` with Marker types:  `AsyncStream` for `Contex<T>`
- So you can pass `tokio::net::TcpStream` directly to [`Context::establish_async`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/async_io.rs#L144)

</details>

### Async need Context

But the key obstacle here is async code need context.
The functions needed by [`tokio::io::Read`](https://docs.rs/tokio/latest/tokio/io/trait.AsyncRead.html) and [`tokio::io::Write` ](https://docs.rs/tokio/latest/tokio/io/trait.AsyncWrite.html) both need a parameter: `cx` whose type is `std::task::Context`.

To address this, implementation need to adjust:
- `IoCallback` `impl`'s now are for a tuple `(&'a mut TaskContext<'b>, &'c mut T)` so that `Context` is accessible in the callback
- Introduced function [`with_bio_async`](https://github.com/fortanix/rust-mbedtls/blob/yx/async-support_master/mbedtls/src/ssl/context.rs#L199) to use a callback function to wrap the `Context` and callback functions' data pointer together and pass it to `mbedtls` through `set_bio_raw`

**Note**:
The async support for DTLS will be added in another PR: #241

[1]: https://www.openssl.org/docs/man1.1.1/man7/bio.html

Co-authored-by: Yuxiang Cao <yuxiang.cao@fortanix.com>
@Taowyoo Taowyoo deleted the jb/io-abstraction branch October 20, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants