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

Add new(), connect(), accept() and handshake() to SslStream #153

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

eaufavor
Copy link
Member

These APIs allow more SslStream to be used more flexibly

These APIs allow more SslStream to be used more flexibly
@nox
Copy link
Collaborator

nox commented Aug 25, 2023

Just wondering, why do we need more flexibility around those functions? I would rather we reduce the number of ways to do the same thing in that crate, than increase it.

What edge does SslStream::new followed by SslStream::connect gives us over Ssl::connect?

@nox
Copy link
Collaborator

nox commented Aug 25, 2023

Btw, have you seen this other PR that touches the handshake API entrypoints? #142

@eaufavor
Copy link
Member Author

My intention is to bring in the APIs openssl-rs provides. With these, it is easier for openssl-rs users to migrate to boring-rs (and vise versa).

Also I think this PR might might help simplify the async implementation too, as claimed by sfackler/rust-openssl#1397.

See more: tokio-rs/tokio-openssl@56f6618

@nox
Copy link
Collaborator

nox commented Aug 28, 2023

I really don't like how tokio-openssl lets you mix up poll_connect and poll_accept calls, which is something #142 completely prevents.

I think #142 is overall better than sfackler/rust-openssl#1397 but I'll let other people decide about that, as I can see value in API parity with the openssl stack, even if it is worse.

@eaufavor
Copy link
Member Author

Thanks.
There are two things I care about this change

  1. The ssl_set_fd (essentially unconnected SslStream) code patten (example https://wiki.openssl.org/index.php/Simple_TLS_Server) is the de facto way of using openssl/boringssl. It is fine that this crate provides a better interface but since the goal of this crate is "BoringSSL bindings for Rust", it should allow the said code pattern to be used.
  2. I think the new unconnected SslStream is more ergonomic than its counterpart without this change
enum MySslStream<S> {
   PreConnect((Ssl, S))
   MidHandshake(MidHandshakeSslStream<S>)
   Connected<Ssltream<S>>
}

@nox
Copy link
Collaborator

nox commented Sep 4, 2023

I think the new unconnected SslStream is more ergonomic than its counterpart without this change

I don't understand this. Why do you need MySslStream?

@eaufavor
Copy link
Member Author

eaufavor commented Sep 6, 2023

This just an artificial example, say, I want my custom stream struct to log the TCP handshake time.

MyStream<S> {
  tcp_handshaket_time: Duration,
  MySslStream<S>
}

enum MySslStream<S> {
   PreConnect((Ssl, S))
   MidHandshake(MidHandshakeSslStream<S>)
   Connected<Ssltream<S>>
}

vs

MyStream<S> {
  tcp_handshaket_time: Duration,
  SslStream<S>
}

It is just very annoying that I cannot have a single object to store the ssl stream before and after TLS handshake.

The point I try to make is that the current interface is unnecessarily complicated for some use case, which can be greatly simplified with the APIs in this PR.

@nox
Copy link
Collaborator

nox commented Sep 6, 2023

Wouldn't you store the TCP handshake time in the inner S of SslStream<S>, thus completely avoiding that specific issue? Why hoist it in your own MySslStream<S> wrapper instead?

@eaufavor
Copy link
Member Author

Maybe that is just a bad example. Maybe I have something that doesn't belong to S, say the TLS handshake time or maybe some preferences/settings I'd like to store for the TLS handshake itself. The goal of this PR is not about enabling certain things that is impossible to do without it. The goal is to provide a style of API interaction that is more familiar to openssl C users and openssl-rs users.

@nox nox merged commit a3cdf87 into cloudflare:master Sep 21, 2023
21 checks passed
This pull request was closed.
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