-
Notifications
You must be signed in to change notification settings - Fork 619
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
implement the caller-side managed buffers (AKA LlConnection
) API
#1578
Conversation
`LlCommonState` is a version of `CommonState` that lacks the `ChunkVecBuffer`s and will be used with the upcoming `LlConnection` API `CommonState2` is an enum that abstreacts over both `CommonState` and `LlCommonState` finally, `Context` has been changed to use `CommonState2`. this way, the existing `State` machinery can support the `LlConnection` API
this is a version of `MessageDeframer` that does not contain an internal buffer and instead operates on a user provided buffer
Codecov Report
@@ Coverage Diff @@
## main #1578 +/- ##
==========================================
- Coverage 96.41% 92.43% -3.98%
==========================================
Files 75 76 +1
Lines 15523 16335 +812
==========================================
+ Hits 14967 15100 +133
- Misses 556 1235 +679
... and 4 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an initial round of feedback. Feels like this doesn't yet hit on the right abstractions.
// Are we done? i.e., have we processed all received messages, and received a | ||
// close_notify to indicate that no new messages will arrive? | ||
peer_cleanly_closed: common.has_received_close_notify | ||
&& !self.core.message_deframer.has_pending(), | ||
has_seen_eof: common.has_seen_eof, | ||
received_plaintext: &mut common.received_plaintext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer not to have this unrelated change in this commit.
inner: CommonStateInner, | ||
} | ||
|
||
impl ops::DerefMut for CommonState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't love extending the deref tower with another layer. What do you think about moving in the other direction, wrapping up received_plaintext
, sendable_plaintext
and sendable_tls
into a CommonBuffers
type and sticking that in the current Connection
types as a sibling of common
?
@@ -359,6 +359,7 @@ mod builder; | |||
mod enums; | |||
mod key_log; | |||
mod key_log_file; | |||
mod ll; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is important that we come up with a better name for this stuff... Maybe something like unbuffered
?
@@ -25,6 +26,202 @@ use alloc::vec::Vec; | |||
|
|||
use pki_types::CertificateDer; | |||
|
|||
#[allow(dead_code)] | |||
pub(crate) enum CommonState2<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CommonState2
definitely needs a new name. Maybe it should not exist at all and we could pass around a CommonState
and an Option<CommonBuffers>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My interpretation of this is it's a incremental migration to a new replacement type? ie
- add a new type with a temporary name
- replace instances of an old type with the new one incrementally, in successive commits
- delete the old type now it is unused
- rename the new type to the old name
Once you're done, all that can get squashed together and the temporary name disappears.
@@ -379,6 +379,273 @@ impl MessageDeframer { | |||
} | |||
} | |||
|
|||
/// A low-level version of `MessageDeframer` that does not maintains an internal buffer. instead, it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to merge all this duplicate code.
@@ -39,6 +39,55 @@ pub(crate) enum CommonState2<'a> { | |||
} | |||
|
|||
impl CommonState2<'_> { | |||
fn reborrow(&mut self) -> CommonState2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go below fn process_main_protocol()
.
@@ -149,6 +200,42 @@ impl CommonState2<'_> { | |||
} | |||
} | |||
|
|||
fn send_warning_alert(&mut self, desc: AlertDescription) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't move and change this in the same commit.
@@ -621,13 +678,111 @@ pub(crate) struct LlCommonState { | |||
inner: CommonStateInner, | |||
} | |||
|
|||
impl ops::DerefMut for LlCommonState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's consistently order DerefMut
after Deref
?
Is it still the plan to have the buffering |
that's on my list of things to try once I have expanded the unbuffered API to support early data and have tested it doing the server side of things. that'd be one way to go: have the current API be implemented as the unbuffered one plus some ChunkVecBuffers then the current tests would exercise the unbuffered API. the |
added |
I've pushed early-data support for both the client and the server and updated the examples to exercise that functionality.
I gave this some idle thought and I think the buffered version could call the unbuffered version's pub struct BufferedClientConnection {
sendable_plaintext: ChunkVecBuffer,
sendable_tls: ChunkVecBuffer,
unbuffered: UnbufferedClientConnection,
}
// actually this should be implemented for the `Writer` but to keep this short
impl io::Write for BufferedClientConnection {
fn write(&mut self, plaintext: &[u8]) -> io::Result<usize> {
// .. state machine stuff omitted ..
if self.unbuffered.may_send_application_data {
// probe phase
let err = self.unbuffered.encrypt(plaintext, &mut []).unwrap_err();
let mut buf =
if let EncryptError::InsufficientSize(InsufficientSizeError { required_size }) =
&err
{
vec![0; *required_size]
} else {
return Err(map_err(err)); // legitimate error (e.g. encrypter exhausted)
};
// execute phase
let written = self
.unbuffered
.encrypt(plaintext, &mut buf)
.map_err(map_err)?;
buf.truncate(written);
self.sendable_tls.append(buf);
} else {
self.sendable_plaintext.append(plaintext.to_vec());
}
Ok(plaintext.len())
}
// ... I'll give that a closer look |
closing in favor of #1583 |
this is a WIP implementation of RFC #1420
currently only the
LlClientConnection
API has been minimally implemented but it's able to interact with, for example,example.com
using either TLS 1.2 or TLS 1.3.I'm putting this up now because I'd like feedback on the approach. I have aimed for maximal code reuse (though there a few places that could use a bit more code reuse) and my approach has been to reuse the existing
State::handle
machinery to implement theLlConnection
API.Arguably that's not the most performant option because, for example, the current implementation does memcpy and allocations when encoding / decoding TLS records (see
Codec
) so theLlConnection
API ends up inheriting those.However, it is, I believe, the fastest approach towards landing the public API. Further performance improvements can be done without breaking the public API (I believe the
#[non_exhaustive]
attribute onLlState
should let us extend it without incurring in semver breaking changes and any required change in traits likeCodec
andState
won't be public facing).This PR is best reviewed on a commit-by-commit basis
TODO list
encrypt
method (depends on PR addencrypted_payload_len
toMessageEncrypter
#1579 )LlMessageDeframer::append_hs
LlServerConnection