Next steps for Quinn support (cont'd) #1798
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1798 +/- ##
==========================================
- Coverage 95.95% 95.90% -0.05%
==========================================
Files 84 81 -3
Lines 18822 18717 -105
==========================================
- Hits 18060 17950 -110
- Misses 762 767 +5 ☔ View full report in Codecov by Sentry. |
cpu
force-pushed
the
cpu-quic-support
branch
from
704040c
to
796c37e
Compare
djc
reviewed
Feb 21, 2024
There was a problem hiding this comment.
Thanks for picking this up, @cpu!
I think we should definitely get the Quinn upgrade PR passing CI before we merge this, to avoid getting similar issues again.
@djc I'm thinking the best route for that is to update quinn#1715 to target this branch/Rustls main. From my perspective it seems like it might make the most sense to have Quinn jump over 0.22 straight to the soon-to-be-cut 0.23 that would roll in the changes from this branch. |
Yup, I agree that that makes sense. |
|
I guess one downside is that it wouldn't help the folks still working through upgrading to the 0.22 release (e.g. h3 for the reqwest/hyper ecosystem). They'll still be blocked, but now on a 0.23 update instead of a Quinn update w/ 0.22. There isn't really a fix for that other than backporting breaking changes to 0.22 and pretending they aren't :-/ |
cpu
force-pushed
the
cpu-quic-support
branch
from
796c37e
to
9d547f7
Compare
ctz
approved these changes
Feb 22, 2024
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
Yeah, I think it's okay to expect that everyone downstream of Quinn will just skip 0.22 and move straight to 0.23. |
The `append_hs` function of the `MessageDeframer` (used only by QUIC connections) mishandles the case where we were in the process of deframing a QUIC HS message that required joining. When copying a payload of the fragmented HS message into the deframer buffer the `DeframerBuffer<'a, ExternalPayload<'a>>` trait implementation for `DeframerVecBuffer` _already_ positioned the write into the unfilled section of the buffer, `self.unfilled()` (e.g. `self.buf[self.used..]`). However, the branch of `append_hs` that continues processing of joining a fragmented HS message was incorrectly further offsetting the copy position by `meta.payload.end`, which is equal to `self.used` at this point. In effect trying to write to `self.buf[self.used+self.used..]`. As a result, if we have buffered more than half the capacity of `self.buf` and then attempt to join in more payload bytes, the unfilled offset is outside the bounds of `buf` and an out-of-bounds indexing panic occurs. This commit adds a simple integration test, as well as a fix.
Previously the `CipherSuiteCommon` type had a `confidentiality_limit` and a `integrity_limit`. Recent refactoring for better downstream QUIC ergonomics has pulled these limits into the `quic::PacketKey` trait. To reduce duplication this commit adjusts our handling of these two limits. For the `integrity_limit`, it was already documented in `CipherSuiteCommon` as being specific to QUIC and irrelevant for TLS over TCP. For this reason we delete the field from `CipherSuiteCommon`, leaving it only in `quic::PacketKey` where it is actually useful. For the `confidentiality_limit` it was described imprecisely and erred on the side of caution, proposing a limit calculated based on QUIC overhead even for the TCP usecase. Now that we've split this field the `CipherSuiteCommon` version's documentation is updated to use a tighter bound for the TCP use-case, and the associated `PacketKey` field can be documented to use the QUIC bound.
The `pub(crate)` members should be below the `pub` members and above the `pub(super)` members.
When holding a `ClientConfig` or a `ServerConfig` it may be helpful to be able to access the `&Arc<CryptoProviver>` that will be used for the configuration. This commit adds accessor functions for this purpose.
This commit adds a `quic::Suite` struct for representing the combination of a `Tls13CipherSuite` and a `quic::Algorithm`. This can optionally be constructed from a `Tls13CipherSuite` that supports QUIC. Having this type helps downstream users that otherwise need to juggle the `Option<quic::Algorithm>` and `Option<Tls13CipherSuite>` from a `SupportedCipherSuite` separately.
cpu
force-pushed
the
cpu-quic-support
branch
from
9d547f7
to
9852c1d
Compare
djc
approved these changes
Feb 27, 2024
|
Sorry for taking so long -- I think it looks good! |
MOZGIII
reviewed
Apr 29, 2024
Comment on lines
-507
to
511
| fn copy(&mut self, payload: &ExternalPayload<'a>, at: usize) { | ||
| fn copy(&mut self, payload: &ExternalPayload<'a>, _at: usize) { | ||
| let len = payload.len(); | ||
| self.unfilled()[at..at + len].copy_from_slice(payload.0); | ||
| self.unfilled()[..len].copy_from_slice(payload.0); | ||
| self.advance(len); | ||
| } |
There was a problem hiding this comment.
Can someone explain this to me?
It looks like adding a debug_assert! would make sense here, instead of not using at completely. We'd compare the at with the self.used to see if they always match - and if they don't - oopsie we panic.
There was a problem hiding this comment.
Can someone explain this to me?
Can you be more specific about what part you're not understanding? The commit message has some additional context in the event you haven't seen it yet.
It looks like adding a debug_assert! would make sense here, instead of not using at completely.
Sounds reasonable. Do you want to open a PR?
There was a problem hiding this comment.
Can you be more specific about what part you're not understanding? The commit message has some additional context in the event you haven't seen it yet.
The motivation behind this change, the commit message explains it. It would be better capture as a code comment though.
There was a problem hiding this comment.
Sounds reasonable. Do you want to open a PR?
Maybe in a bit, once I collect all the feedback.
There was a problem hiding this comment.
This looks like an very odd implementation of a trait, should it be just remodelled in a different way - in other words, should we fix the trait itself maybe?
There was a problem hiding this comment.
in other words, should we fix the trait itself maybe?
I think we would be open to a refactoring if you see a path forward. Reviewing CONTRIBUTING.md might be helpful if so.
I understand there to be a general agreement on the team that the deframer and associated APIs have room for improvement. From my perspective it was challenging updating this area of the codebase to support the new unbuffered connection API. I've tried my hand at trying to clean up a bit and didn't arrive at anything that was a workable improvement in the time I had boxed for the task but that's likely more a product of my own relative inexperience.
There was a problem hiding this comment.
Well, yeah, tell me about it, this is the first PR I'm reviewing here. I do not think I'll be taking on refactoring this - but at least capturing this context for a comment would be helpful, thanks!
MOZGIII
reviewed
Apr 29, 2024
Comment on lines
+726
to
+728
| /// This is not relevant for TLS over TCP (which is implemented in this crate) | ||
| /// because a single failed decryption is fatal to the connection. However, | ||
| /// this quantity is used by QUIC. |
There was a problem hiding this comment.
This is a bit confusing - looks like a copy-paste from the older comment.
In particular - the which is implemented in this crate part. Would probably be better as which is also implemented in this crate (added also).
There was a problem hiding this comment.
Would probably be better as which is also implemented in this crate (added also).
👍 Want to open a PR with the change?
This was referenced Apr 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a continuation of #1741, with some adjustments/new work:
maininstead ofrel-0.22- I'm happy to backport this work if it would help with Quinn testing but in order to merge this to unblock 0.23 release preparation #1777 we'll need a version targettingmain.CryptoProvideraccessible from a[Client|Server]Config.AsRef<Arc<CryptoProvider>>to expose the provider, simpler accessor methods are used.main. With the fix proposed in this branch the test passes again.quic::InitalSuiteAPI improvements from the original branch are reworked based on Djc's later feedback, adding aquic::Suitetype and aTls13CipherSuitehelper.