Codecov Report

Attention: Patch coverage is 64.31535% with 86 lines in your changes missing coverage. Please review.

Project coverage is 95.57%. Comparing base (ffac73a) to head (e8c9bb0).
Report is 7 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2370      +/-   ##
==========================================
- Coverage   95.96%   95.57%   -0.39%     
==========================================
  Files          94       95       +1     
  Lines       22634    22843     +209     
==========================================
+ Hits        21721    21833     +112     
- Misses        913     1010      +97     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

First, I think the dangerous_into_external_connection() API is an excellent way to do this and maybe I'd go further to suggest that this PR could deprecate the dangerous_extract_secrets() API, and eviscerate its internal implementation (eg, by making dangerous_extract_secrets() just do let (secrets, _) = self.dangerous_into_external_connection()?; Ok(secrets)).

I think there is space here to reduce the public API scope of ExternalConnection quite a bit. Observing the idea that KTLS users are exchanging complexity for big performance and density improvements, I think that committing to keeping things like the peer's certificate around while servicing the connection is not in keeping with that goal. So I would suggest removing much of that API surface for now -- since the caller needs to ensure the handshake is complete before calling dangerous_into_external_connection(), if they need the peer certificate, handshake shape, ALPN, etc, they can obtain this before the conversion. (That doesn't stop us from having to store it internally, but gives us some future leeway in how we do that to improve density.)

Agreed. Also, a bit of bikeshedding: I don't love External as it feels a little generic. Was thinking maybe Traffic would be a good prefix for this that is a little more meaningful in communicating how it's different?

I'm going to make changes as new commits for now since it is easier to see what is changing. I'll rework things into a more coherent commit history before this gets merged.

Quick summary of the new changes before I respond to individual comments:

• I have made ExternalConnection generic over Data so that users with a server-side connection cannot call handle_new_session_ticket. This is currently just a facade using PhantomData over non-generic internals but it gives freedom in the future if something needs access to Data. My main motivation for this, though, is disallowing calls to handle_new_session_ticket on server side connections.
• I have removed a bunch of the public API methods from ExternalConnection.
• I have reimplemented dangerous_into_secrets on top of dangerous_into_external_connection. The one change in behaviour is that it now emits an error if there are messages stored in sendable_tls instead of giving unusable secrets.

I'd go further to suggest that this PR could deprecate the dangerous_extract_secrets() API, and eviscerate its internal implementation.

I have done all of this except the deprecation. I'll add a commit for the deprecation bit once we've decided on the final name for the type.

I think there is space here to reduce the public API scope of ExternalConnection quite a bit.

I have reduced the API surface down to just the negotiated protocol version and the chosen cipher suite. The protocol version needs to be kept anyways and the cipher suite is the only way to get the confidentiality limit (and is needed for kTLS besides). This has not appreciably reduced the size of ExternalConnection, unfortunately, since the quic data is ~600B and everything else is ~80B.

I don't love External as it feels a little generic. Was thinking maybe Traffic would be a good prefix for this that is a little more meaningful in communicating how it's different?

Up front, I don't have a strong preference here. This is your bikeshed so ultimately you get the final say :)

My thought process behind ExternalConnection was something like "it's a connection that's managed externally to rustls" -> ExternalConnection. Not sure if that makes it better or worse.

TrafficConnection sounds a little off to me. It might be because all the other prefixes are adjectives?. As an alternative, what if we just called it Connection like in the quic module? Then you have external::Connection, external::ClientConnection, and external::ServerConnection. I'm not sure if that's better, on second thought, but I'll leave it here as an alternative.

TrafficConnection sounds a little off to me. It might be because all the other prefixes are adjectives?. As an alternative, what if we just called it Connection like in the quic module? Then you have external::Connection, external::ClientConnection, and external::ServerConnection. I'm not sure if that's better, on second thought, but I'll leave it here as an alternative.

That's fair. I think my beef with "external" is that it leaves open the question of "external to what"?

@djc I've been thinking on and off about the naming for the last few days and have not been able to come up with a better name. So I'm putting this back on your side of the court: if you want me to rename type+module to TrafficConnection and traffic then I'll do so, if you think it is ok staying as ExternalConnection + external, then I'll leave it as is. I don't have a strong enough opinion on this to stall the PR over it :)

I will say that when I named it my thought process was that it is a connection that is "external to rustls". i.e. that the user has to implement the connection themselves, whereas for the other connection types the user just provides/receives bytes and rustls handles the rest.

I think I have addressed all the points brought up so far. The only thing left is deprecating dangerous_external_secrets which I will do after naming is settled. Let me know if there is anything I can do to make the review easier, as I realize this is a big PR. Otherwise, I'll leave this until you guys have bandwidth to review.

I don't mind the ExternalConnection naming, but WDYT about KernelConnection?

I don't have an issue with KernelConnection. I suspect the only user of it is going to be kTLS so I'm happy with that. If you guys are ok with it as well then I'll go ahead and change it.

I have gone ahead and done the rename. I have also deprecated dangerous_extract_secrets in favour of the new dangerous_into_kernel_connection method. Finally, I have cleaned up the commit history into something a bit more coherent.

I think this is generally the right direction. I'm on the fence about the example code -- while it's nice to have a worked example, it seems to spend a lot of code on the details of the kernel API, while the API surface we have to maintain in order to make it work appears to be substantially smaller.

Pinging on this. I think I have addressed and/or responded to all the comments, so I'm leaving this comment mostly to mark that I consider this PR to be waiting on a reviewer.

I think that's everything resolved. I have also rebased on main, so everything should be up to date.

Benchmark results

Instruction counts

Significant differences

⚠️ There are significant instruction count differences

Other differences

Wall-time

Significant differences

There are no significant wall-time differences

Other differences

Additional information

Historical results

Checkout details:

• Base repo: https://github.com/rustls/rustls.git
• Base branch: main (ffac73a)
• Candidate repo: https://github.com/swlynch99/rustls.git
• Candidate branch: external-connection (cf4e9a4)