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

[RFC] Caller-side managed buffers #1420

Closed
wants to merge 10 commits into from

Conversation

japaric
Copy link
Contributor

@japaric japaric commented Aug 25, 2023

Summary

This RFC proposes adding a new, lower-level {Client,Server}Connection API where TLS and plain-text buffers are managed by the caller / end-user.
The design goals of this new API are to reduce / minimize memcpys, be no-std compatible and allow async certificate verification.

Rendered version

This RFC addresses issue #1362

Also check out the mock implementation of the proposed API in the PR branch

allow async certificate verification.

This aspect still needs more work but I think the main part that addresses #1362 is ready for comments! this section is now complete

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #1420 (f2b87c4) into main (cc201f5) will increase coverage by 0.16%.
Report is 109 commits behind head on main.
The diff coverage is n/a.

❗ Current head f2b87c4 differs from pull request most recent head fbcdc20. Consider uploading reports for the commit fbcdc20 to get more accurate results

@@            Coverage Diff             @@
##             main    #1420      +/-   ##
==========================================
+ Coverage   96.34%   96.50%   +0.16%     
==========================================
  Files          64       71       +7     
  Lines       14808    15124     +316     
==========================================
+ Hits        14267    14596     +329     
+ Misses        541      528      -13     

see 51 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Stream API still needs to be updated
@japaric
Copy link
Contributor Author

japaric commented Aug 29, 2023

allow async certificate verification.

I have started working on revising the RFC to meet this design goal. the work is visible in this WIP branch. when I'm done there, I'll push the changes into this branch / PR

switch main proposal to use the enum alternative to a Status struct that
only contains boolean flags
@japaric
Copy link
Contributor Author

japaric commented Sep 1, 2023

The RFC has been updated:

  • the main proposal now uses the enum alternative to a Status struct that only contains boolean flags
  • the 'async certificate verification' section has been completed and now it's already for feedback / comments

@japaric japaric marked this pull request as ready for review September 5, 2023 15:45
@djc djc mentioned this pull request Sep 7, 2023
@japaric japaric mentioned this pull request Sep 5, 2023
rfcs/000-caller-side-managed-buffers.md Outdated Show resolved Hide resolved
}

/// Encrypts `app_data` into the `outgoing_tls` buffer
pub fn encrypt_outgoing(&self, app_data: &[u8], outgoing_tls: &mut Vec<u8>) { /* .. */ }

Choose a reason for hiding this comment

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

Ditto.

pub fn decrypt_incoming<'a, B>(
&self,
incoming_tls: &'a mut IncomingTls<B>,
) -> impl Iterator<Item = Result<&'a [u8]>> {

Choose a reason for hiding this comment

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

Should this be a GAT-based LendingIterator?

rfcs/000-caller-side-managed-buffers.md Outdated Show resolved Hide resolved
rfcs/000-caller-side-managed-buffers.md Outdated Show resolved Hide resolved
rfcs/000-caller-side-managed-buffers.md Show resolved Hide resolved
rfcs/000-caller-side-managed-buffers.md Outdated Show resolved Hide resolved
`State`, and thus `Status` as well, would have to gain a lifetime that ties it to the `incoming_tls` buffer.
Given how Rust works, even if `State` was not in the `ReceivedEarlyData` variant, the `status.state` field would still "freeze" `incoming_tls` meaning that it can't be mutated until `status.state` goes out of scope / is dropped.

The other issues is that if `process_tls_records` automatically decrypts early data, the end-user no longer has the option to discard the early data without decrypting it first (see snippet below) to save some CPU work.
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this could be offered as a method.

rfcs/000-caller-side-managed-buffers.md Outdated Show resolved Hide resolved
rfcs/000-caller-side-managed-buffers.md Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented Sep 8, 2023

BTW, one goal to keep in mind is code reuse across the high-level and low-level API. So ideally the high-level API can be implemented on top of the low-level API to avoid duplicating logic.

@japaric
Copy link
Contributor Author

japaric commented Sep 11, 2023

the sketch2 crate has been updated and now includes the certification validation API. two mocked examples / tests are included: one uses mocked async IO; the other one uses libstd's read_buf API, also mocked

the sketch2 implements this idea of making certain operations possible only when certain conditions are met thus making the API statically impossible to misuse.

I also went ahead and experimented with turning incoming_tls into a regular slice type. this pushes the task of removing already processed records from said buffer to the end user, which makes the API a bit more cumbersome to use. the inconvenience is traded off with increased flexibility as the API can now be used with seemingly any buffer type: Vec or BorrowedBuf is what I tested but I expected that a custom buffer type like a ring / circular buffer should also work.

@djc
Copy link
Member

djc commented Sep 12, 2023

I also went ahead and experimented with turning incoming_tls into a regular slice type. this pushes the task of removing already processed records from said buffer to the end user, which makes the API a bit more cumbersome to use. the inconvenience is traded off with increased flexibility as the API can now be used with seemingly any buffer type: Vec or BorrowedBuf is what I tested but I expected that a custom buffer type like a ring / circular buffer should also work.

Another important point to me at least is that the library cannot issue allocations (via resizing the &mut Vec internally) on the data path behind the caller's back. I definitely think we'll want to use BorrowedBuf for this in the future, after it stabilizes.

re-implement Stream on top of LlClientConnection for testing purposes
@japaric
Copy link
Contributor Author

japaric commented Sep 12, 2023

I've updated sketch2 and turned AppDataAvailable into an iterator. that seems to work fine. I also re-implemented Stream on top of LlClientConnection to see if that would work out. even without support for partially discarding TLS records; that's doable with some extra buffering.

what I realized while implementing Stream is that we may need another version of AppDataAvailable. the current version is meant to represent app-data records during the handshake; that variant does not provide an encrypt method because during handshake that's represented with a different enum: MayEncryptAppData. after handshake, however, it's possible to encrypt app-data into outgoing_tls while decrypting app-data records out of incoming_tls -- even while iterating; the buffers are separate so the borrow checker won't complain about encrypting in the middle of an iteration loop, though one may need to use while let instead of a for loop.

OR, if it's always possible to encrypt when encrypted app data has arrived then we can add encrypt to AppDataAvailable variant / type and no need to add another variant to State (and it may even be possible to remove MayEncryptAppData variant?). the two sequence diagrams I have, one of TLS 1.2 and one of TLS 1.3 (this one is in the README of the sketch directory), do indicate that encryption is possible at that point of the handshake / connection.

@japaric
Copy link
Contributor Author

japaric commented Sep 15, 2023

the RFC text has been update to use "sketch2" as the main proposal. the sketch project has been removed and sketch2 has been renamed to sketch.

this is ready for a new review / round of comments!

@ctz
Copy link
Member

ctz commented Mar 7, 2024

Going to close this. We have some follow-on work in other issues, but the initial version of this was delivered in 0.23.

@ctz ctz closed this Mar 7, 2024
@c410-f3r
Copy link

c410-f3r commented Mar 7, 2024

Is there an unified issue intended to track the remaining work of this feature?

#1362

@tshepang tshepang deleted the rfc-caller-side-buffers branch March 8, 2024 02:40
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.

None yet

5 participants