Reimplement library on top of rustls::kernel API (continuation of #61)
#62
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
===========================================
+ Coverage 24.74% 64.16% +39.42%
===========================================
Files 6 20 +14
Lines 1265 2227 +962
===========================================
+ Hits 313 1429 +1116
+ Misses 952 798 -154 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cxw620
force-pushed
the
use-rustls-kernel-connection
branch
from
e3d2016 to
a70ffc5
Compare
|
I have put this back to draft. Please work on making this reviewable, by (for example) minimising back-and-forth changes, making a sequence of logical commits, and perhaps slicing off some smaller PRs that are easier to review and land, but still make sense individually. |
cxw620
force-pushed
the
use-rustls-kernel-connection
branch
from
b02695e to
16d4a3c
Compare
Hi, I've cleaned up and sorted the commits. Since almost everything is rewritten, I think we can hardly break this PR into some smaller ones. |
If that's not possible I don't think any of the rustls maintainers will be able to review it. Maybe @fasterthanlime will be able to? |
We just need to review the following core changes, in fact. If the core developer of this crate @fasterthanlime doesn't have spare time to review this, and you, on behalf of rustls maintainers, still insist that this PR shall be sliced off, I will break this PR into separate ones at this weekend when I'm free :(.
The tests, examples and GHA CI workflows are rewritten accordingly 1cf2d16, while kernel compatibility test is newly introduced: cc2a11.
|
|
The point in making smaller commits is seeing how you are evolving the current code towards the new desired state. That is usually a lot easier then closely reviewing "here's a bunch of new code I wrote" after deleting all the old code. As such, the current state of the PR isn't any easier for me to review than the previous state. |
…export it [NOT COMPILED]
cxw620
force-pushed
the
use-rustls-kernel-connection
branch
from
afe0c97 to
449bf63
Compare
I attempted to refactor this crate incrementally, as you requested, while keeping the crate compilable at each step, preserving some of the old code to show "how I am evolving the current code towards the new desired state." However, I was unable to achieve this goal. Writing glue code for deprecated and soon-to-be-removed code is really frustrating. |
cxw620
force-pushed
the
use-rustls-kernel-connection
branch
from
4dd6436 to
0548809
Compare
cxw620
force-pushed
the
use-rustls-kernel-connection
branch
from
0548809 to
54af664
Compare
|
Sorry for disappearing on the last PR and thanks @cxw620 for picking this up! I'm glad this didn't get stalled completely due to me getting burned out over the summer and I like the new work you've done to finish it :) Here's some suggestions for things you can break into separate PRs that should help the maintainers review things more easily:
I'm going to take a crack at splitting out the Edit: I have a branch up here that just includes the |
| tls12 = ["rustls/tls12"] | ||
|
|
||
| # Expose some low-level APIs | ||
| raw-api = [] |
There was a problem hiding this comment.
What's the reason putting some of the APIs behind features? I would understand if this involved a lot a large amount of code or dependencies/features in dependencies but neither of those seem to be the case for this or probe-ktls-compatibility so I'm confused here.
There was a problem hiding this comment.
These APIs are not needed in most cases, but using them incorrectly could cause problems, so I gate them behind the feature raw-api to force users to read the documentation.
There was a problem hiding this comment.
IMO that's not a good enough reason to put them behind a feature, though obviously this is up to the maintainers for the final say. As far as I can tell, though, it looks like 70% of the crate API is behind the raw-api feature. I don't think this is worth it.
There was a problem hiding this comment.
If we implement Read / Write / AsyncRead / AsyncWrite for KtlsStream, KtlsStream can serve as a replacement of original TcpStream (or something else) in most cases. Exposing these raw APIs are just for something like splice(2) (this is also why I'm here trying to continue on your work).
Ah, setup_tls_* doesn't need to be exposed at all, and marking KtlsStream::as_raw as unsafe is enough. I'll remove this feature flag.
There was a problem hiding this comment.
I agree on making the setup_tls_ methods non-public. I don't think you should make KtlsStream::as_raw unsafe though because it really isn't (not in the rust sense).
As an extra note: you should be implementing AsRawFd for KtlsStream, which will give you access to the raw fd regardless.
There was a problem hiding this comment.
Alright, but to avoid misuse, I still think it should be gated behind a feature flag, disabled by default, guiding users to check the documentation.
| @@ -0,0 +1,18 @@ | |||
| [package] | |||
| name = "ktls-util" | |||
There was a problem hiding this comment.
Would it make more sense to call this crate tokio-ktls? That was originally what the ktls crate was but with the changes you've made here that is no longer the case.
There was a problem hiding this comment.
I think we shall contribute these codes to tokio-rustls instead of a new crate, and then implement the fallback mechanism, so than we can benifit from kTLS transparently.
This crate is here for examples and tests only (oh, maybe I should move the codes to ktls-test)
There was a problem hiding this comment.
Uhh, you should probably open an issue on the tokio-rustls repository to check that they are actually interested in that. Either way if you're not intending to submit it as part of this PR then it should be moved out to a separate branch. What's currently in the ktls-util crate is something that can very clearly be submitted in a separate PR.
Thanks for your advices! I will continue to work on this and cherry-pick your commits. This PR may be broke into these pieces:
|
| @@ -0,0 +1,119 @@ | |||
| name: CI | |||
There was a problem hiding this comment.
All of the CI changes should go into a separate PR.
| @@ -1,15 +1,17 @@ | |||
| [package] | |||
There was a problem hiding this comment.
All the changes to ktls-sys here can be easily pulled out into a separate PR. They are completely independent of the rest of this PR.
| @@ -1,451 +1,33 @@ | |||
| use ffi::{setup_tls_info, setup_ulp, KtlsCompatibilityError}; | |||
There was a problem hiding this comment.
One big comment on the new organization of the crate: you have a lot of unnecessary public modules. Looking through the rustdoc I counted at least 6 different public modules that are completely empty.
IMO I think we should just dump everything in crate root (not for the implementation but for the public API). The only exception I can think of is maybe the setup module.
| @@ -0,0 +1,308 @@ | |||
| //! See the [module-level documentation](crate::setup) for more details. | |||
There was a problem hiding this comment.
I think this module ends up being a bit overcomplicated. I don't think the extra generics actually introduce any extra safety. I took a shot at what I think it should look like here: swlynch99@122882e.
We can probably simplify it a bit further and drop the Direction enum I introduced there in favour of a set_tx and a set_rx on CryptoInfo
| pub(crate) fn take_buffer(&mut self) -> Option<Buffer> { | ||
| if self.state.has_buffered_data() { | ||
| self.state.set_has_buffered_data(false); | ||
|
|
||
| Some(mem::take(&mut self.buffer)) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm going to suggest this just be implemented using Vec::drain to remove the part of the buffer that has already been used. I expect most use cases will not have a partially drained buffer and so will have no overhead. I don't think the remaining cases are worth the extra API complexity here.
| #![warn( | ||
| unsafe_code, | ||
| unused_must_use, | ||
| clippy::alloc_instead_of_core, | ||
| clippy::exhaustive_enums, | ||
| clippy::exhaustive_structs, | ||
| clippy::manual_let_else, | ||
| clippy::use_self, | ||
| clippy::upper_case_acronyms, | ||
| elided_lifetimes_in_paths, | ||
| missing_docs, | ||
| trivial_casts, | ||
| trivial_numeric_casts, | ||
| unreachable_pub, | ||
| unused_import_braces, | ||
| unused_extern_crates, | ||
| unused_qualifications | ||
| )] |
There was a problem hiding this comment.
It's worth double-checking with the maintainers if they think this is worth it, especially since you're just suppressing them all over the place anyway.
edit: I see these are copied from ktls. I do think it's worth doing a pass over these to see which are worth keeping. #[warn(unsafe_code)] doesn't really make sense for a FFI wrapper crate like ktls. Others seem to also apply.
| mod protocol; | ||
| pub mod setup; | ||
| pub mod stream; | ||
| pub mod utils; |
There was a problem hiding this comment.
Instead of this being pub we should just export CompatibleCipherSuites in the crate root.
| pub mod impl_std; | ||
| #[cfg(feature = "async-io-tokio")] | ||
| pub mod impl_tokio; |
There was a problem hiding this comment.
Neither of these modules actually contain any public types.
There was a problem hiding this comment.
I moved them to separate modules just to make stream.rs looks cleaner 🤔
There was a problem hiding this comment.
The issue isn't that they are in separate modules (that's fine). It's that they are public but have no public types. They should just be private.
| @@ -0,0 +1,21 @@ | |||
| edition = "2021" | |||
There was a problem hiding this comment.
This change should definitely go in its own PR.
| #[non_exhaustive] | ||
| #[derive(Debug)] | ||
| /// The error type for `KtlsStream` and related operations. | ||
| pub enum KtlsStreamError { |
There was a problem hiding this comment.
This should really document that it is emitted within an io::Error from operations on KtlsStream.
| pub struct CompatibleCipherSuites { | ||
| suites: HashSet<u16>, | ||
|
|
||
| /// The supported protocol versions. | ||
| pub protocol_versions: &'static [&'static SupportedProtocolVersion], | ||
| } |
There was a problem hiding this comment.
I find this struct quite confusing.
- Why is
protocol_versionspublic? - Why are we using a HashSet to store a maximum of
612ish items?
I'm also mostly just confused as to why you chose this over the original definition? A big match statement in a method called something like is_supported(suite: SupportedCipherSuite) would be a lot less confusing to me.
|
I'm gonna suggest we try and work through some of my comments here before trying to split things off as separate PRs. The end goal should be to get everything in this PR merged in separate piecewise PRs that can be effectively reviewed by the existing maintainers to the point where there's nothing left in this PR. I don't think it is possible to effectively review this PR at once given its size. |
| let mut cmsgs = recv_msg | ||
| .cmsgs() | ||
| .expect("should have 1..24 control message received"); | ||
|
|
||
| match cmsgs.next().expect("should have at least one CMSG?") { | ||
| ControlMessageOwned::TlsGetRecordType(content_type) => { | ||
| // `recv` will never return data from mixed types of TLS records. | ||
| debug_assert!(cmsgs.all(|cmsg| { | ||
| matches!(cmsg, ControlMessageOwned::TlsGetRecordType(_)) | ||
| })); | ||
|
|
||
| (content_type, recv_msg.bytes) | ||
| } | ||
| value => { | ||
| abort_and_return_error!( | ||
| self, | ||
| socket, | ||
| io::Error::other(format!( | ||
| "unknown control message received: {value:?}" | ||
| )) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This code makes no sense to me. Why are we reading up to 24 control messages and throwing away all but the first?
| pub(crate) fn handle_io_result<S: AsFd, T>( | ||
| &mut self, | ||
| socket: &S, | ||
| ret: io::Result<T>, | ||
| ) -> io::Result<Option<T>> { |
There was a problem hiding this comment.
Instead of handle_io_result I think this should be either
fn handle_io_error<S: AsFd>(&mut self, socket: &S, err: io::Error) -> io::Result<()>;or possibly just
fn handle_control_message<S: AsFd>(&mut self, socket: &S) -> io::Result<()>;and then we tell the user to call it if they get EIO.
TBH this depends on whether we want to specifically handle EPIPE or error as if we got an unexpected hangup. What does tokio-rustls do in this case?
Updates repo metadata and formats the codes (part of #62)
This PR inherits from #61 (#61 has had no activities for nearly two months, but I could not wait for it :) )
This PR completely rewrites this crate using the
rustls::kernelAPI provided by rustls since rustls 0.23.27.Breaking changes
Almost everything.
The MSRV is now 1.77.0.
Questions
Is this crate a low-level crate, or a high-level crate (similar to
tokio-rustls)? Should the example implementations of (Ktls)Connector / (Ktls)Acceptor be merged into this crate, or just left them to be implemented bytokio-rustls(or any other crate)?My opinion is that we could create a new crate called
ktls-util, implementing KtlsConnector / KtlsAcceptor, provided as an optional feature. Let higher-level cratetokio-rustlsimplement the fallback mechanism to transparently provide best-effort kTLS offload support for applications.For detecting
CompatibleCiphers, should it be implemented within this crate?My opinion is that this could also be implemented in
ktls-util, provided as an optional feature.TODOs
Improves testing.
Due to the difficulty of enumerating all edge cases in real-world scenarios and the complexity of writing tests, more investigation is needed.
(Additionally, I will privately deploy this in actual environments for testing)
Reviews whether the current implementation complies with RFC 8446 and other relevant RFC document requirements.
More comprehensive documentation.DoneReplacesDone.slicur::Readerwithrustls::internal::msgs::codec::ReaderMisc, such as applying cargo clippy / rustfmt, etc.DoneLooking forward to feedback and code review.
Fixed: #59