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

PSK and DTLS support #202

Merged
merged 11 commits into from
Oct 6, 2022
Merged

PSK and DTLS support #202

merged 11 commits into from
Oct 6, 2022

Conversation

DrTobe
Copy link
Contributor

@DrTobe DrTobe commented Jun 9, 2022

I have added PSK (only the interface to mbedtls_ssl_conf_psk(), so mainly useful for clients) and DTLS support. Because using either of those is a little different from the default, I have also added two new examples.

Using DTLS strictly requires setting a timer with ssl_set_timer_cb so I added an appropriate interface. Because it is relatively straightforward to supply one in a std-environment, I did so, too.

Additionally, I needed another IoCallback which works over UDP. The previously available impl based on the Write and Read traits is clearly designed to be used with std::net::TcpStream which is a good choice for TLS connections but inappropriate for DTLS. std::net::UdpSocket does not implement those traits but I thought that a default implementation to use in std-environments would be good. So I ended up creating a new type which wraps a UDP socket and enforces that connect is properly called before. Please let me know what you think about that design.

The more I worked on the UDP/DTLS stuff, the more I got the feeling that maybe, the current API design is insufficient for TLS and DTLS usage (although the current API design with the mbedtls::ssl::config::Transport parameter for mbedtls::ssl::Config::new suggests that this should be possible). Some things I noticed while working on this:

  • We can try to open a DTLS connection over TCP or a TLS connection over UDP which is both nonsense.
  • UDP is unreliable and the naive implementation could easily hang forever if no response is received. I do not know how TCP connections behave if the connection is lost. Maybe, it would be nicer if we would use mbedtls_ssl_recv_timeout_t instead of mbedtls_ssl_recv_t to prevent getting stuck? Unfortunately, this would require constant calls to TcpStream::set_read_timeout and UdpSocket::set_read_timeout and would prevent the IoCallback implementation based on the Write and Read traits.
  • At some points, the code looks as if asynchronous operation was supported (Error::SslWantRead and Error::SslWantWrite are handled differently) but the handshake can not be completed because Context::handshake is not pub.

Nevertheless, unless we can quickly agree on how to resolve any of those issues, I would like to see these changes merged soon so that we can base our future work onto them. If there is anything obvious which can be fixed, please let me know.


#[cfg(feature = "std")]
impl ConnectedUdpSocket {
pub fn connect<A: std::net::ToSocketAddrs>(socket: std::net::UdpSocket, addr: A) -> std::result::Result<Self, (std::io::Error, std::net::UdpSocket)> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

We already use core::result::Result as StdResult in this file, and since here we have std in this block let's keep with StdResult.

Similarly, we can import std::io::Error as IoError and use that.

(note: https://github.com/rust-lang/rust/blob/5807fbbfde3ad04820f6fa0269711c81538057ec/src/libstd/lib.rs#L332-L333)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just giving evidence of the fact that std exports core.

mbedtls/Cargo.toml Show resolved Hide resolved
@zugzwang
Copy link
Contributor

Hey @DrTobe, would you mind adding some tests that you find appropriate? For instance I'd like to see mbedtls complaining when the identity is not valid UTF-8, if it makes sense.
After that we can go ahead. Thanks!

@DrTobe
Copy link
Contributor Author

DrTobe commented Sep 26, 2022

For instance I'd like to see mbedtls complaining when the identity is not valid UTF-8, if it makes sense.

That specific test won't be necessary because the type of the psk_identity is &str which is guaranteed to be valid UTF-8 in Rust. If this was not the case here, the user would have screwed up terribly.

I will have a look if I find some things to test. If you have any other specific idea, please let me know.

mbedtls/src/ssl/config.rs Outdated Show resolved Hide resolved
…propriate tests

This requires to make the handshake method public because it needs to be
called again after the initial handshake attempt has failed due to the server
responding with a HelloVerifyRequest.
@DrTobe
Copy link
Contributor Author

DrTobe commented Sep 28, 2022

I have added the whole DTLS server side (cookies and client ID) which requires some dodging around some current design decisions. On the server side, the first connection setup attempt will always fail if the cookie-based DoS protection is activated (which it should be). So it must be possible to reset the context and try the handshake again. I have considered various possibilities to achieve that, the solution I chose was the one which required, in my opinion, the most acceptable changes.

I started with a separate client_server_dtls test but I merged it into the TLS test to avoid code duplication. This makes the client_server test a little bit more complicated but I think it is the right thing to do.

I guess I would add another PSK test, too. I do not know yet if I prefer a separate test or merge it, too.

@DrTobe
Copy link
Contributor Author

DrTobe commented Sep 29, 2022

With the recent changes, everything should be properly tested by now. Again, to avoid code duplication, I have merged the PSK-based tests into the client_server test. Like this, all four combinations TLS/DTLS x certificates/PSK are tested.

Unfortunately, I had to add a short thread::sleep which was apparently necessary to reuse the same UDP sockets. I hope the comment above is enough to accept this.

@zugzwang
Copy link
Contributor

zugzwang commented Oct 6, 2022

bors +r

@bors
Copy link
Contributor

bors bot commented Oct 6, 2022

Did you mean "r+"?

@zugzwang
Copy link
Contributor

zugzwang commented Oct 6, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 6, 2022

Build succeeded:

@bors bors bot merged commit 610cc77 into fortanix:master Oct 6, 2022
@zugzwang zugzwang mentioned this pull request Oct 20, 2022
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.

2 participants