Reimplement library on top of `rustls::kernel` API (continuation of #61)
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::kernel API 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.~~ Done
-
~~Replaces
slicur::Readerwithrustls::internal::msgs::codec::Reader~~ Done. -
~~Misc, such as applying cargo clippy / rustfmt, etc.~~ Done
Looking forward to feedback and code review.
Fixed: #59
~~I think most works are done and can be reviewed.~~
CI pass: https://github.com/cxw620/ktls/actions/runs/17178153092
Update: https://github.com/cxw620/ktls/actions/runs/17185892230
TBD:
- ~~Checking supported cipher suites on different versions of Linux kernel (reference: https://github.com/tokio-rs/io-uring/blob/master/.github/workflows/kernel-version-test.yml).~~
- Refine exposed APIs, such as name, docs, etc?
Known issues:
- Read early data failed.
Codecov Report
:x: Patch coverage is 63.63636% with 580 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.16%. Comparing base (571d49a) to head (4dd6436).
:warning: Report is 10 commits behind head on main.
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
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.
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.
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.
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?
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 :(.
- Setting ULP and TLS params on the socket, commit 1da6207
- Implement
KtlsStream, managing kTLS context (contains therustls::kernel::KernelConnection), this is newly introduced, commit c5c229f - Implement Read / Write / AsyncRead / AsyncWrite for
KtlsStream, see commit e59aba8, 225f0b4
The tests, examples and GHA CI workflows are rewritten accordingly 1cf2d16, while kernel compatibility test is newly introduced: cc2a11.
ktls-util is currently for tests only and need not be published (I think it's crate tokio-rustls that should implement so, not this crate). It also serves as examples, so that anyone may just copy and paste the codes and create their own crate; ktls_util::suites::CompatibleCipherSuites can be moved into ktls, gated with a feature flag, since without adjusting supported cipher suites the client example still works, according to the kernel compatibility test.
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.
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.
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.
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:
- Create a separate PR with mostly just the crate reorganization changes (e.g. creating the
setup) module. Possibly multiple if there are multiple independent reorganizations. - The rewrite of the code to probe for supported protocols can be moved to its own PR. The original
KtlsStreamstruct doesn't interact with this in any way so it should be possible to do this without affecting much else. - I'm not sure that there's a good way to break
KtlsStreamapart so this might be one big PR, but it may be possible to put it in as a skeleton and build it up using multiple PRs.
I'm going to take a crack at splitting out the setup module and see if I can come up with something. @cxw620 if you would rather I don't do this lmk and I'm happy to let you have control here :)
Edit: I have a branch up here that just includes the setup and error modules that could be submitted independently.
https://github.com/swlynch99/tokio-ktls/tree/extract-setup
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:
* Create a separate PR with mostly just the crate reorganization changes (e.g. creating the `setup`) module. Possibly multiple if there are multiple independent reorganizations. * The rewrite of the code to probe for supported protocols can be moved to its own PR. The original `KtlsStream` struct doesn't interact with this in any way so it should be possible to do this without affecting much else. * I'm not sure that there's a good way to break `KtlsStream` apart so this might be one big PR, but it may be possible to put it in as a skeleton and build it up using multiple PRs.I'm going to take a crack at splitting out the
setupmodule and see if I can come up with something. @cxw620 if you would rather I don't do this lmk and I'm happy to let you have control here :)Edit: I have a branch up here that just includes the
setupanderrormodules that could be submitted independently. https://github.com/swlynch99/tokio-ktls/tree/extract-setup
Thanks for your advices! I will continue to work on this and cherry-pick your commits.
This PR may be broke into these pieces:
- Refactors of mod
setup - Refactors of mod
utils::probe - Implements
KtlsStream. I think we can hardly break this into smaller PRs.- Implements Tokio's
AsyncRead/AsyncWritesupport forKtlsStream.
- Implements Tokio's
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.