-
Notifications
You must be signed in to change notification settings - Fork 635
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
Don't add empty certificate_authorities
extension
#1729
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1729 +/- ##
==========================================
- Coverage 96.16% 96.16% -0.01%
==========================================
Files 80 80
Lines 17345 17342 -3
==========================================
- Hits 16680 16677 -3
Misses 665 665 ☔ View full report in Codecov by Sentry. |
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesClick to expand
Other differencesClick to expand
Additional informationCheckout details:
|
If there is already a test case, how come it didn't fail? It must not be a good test case for this particular failure mode? |
The existing test case is rustls/rustls/src/client/common.rs Lines 75 to 76 in b3913a5
With that said, I just took a second look, and:
|
Okay, nice. Why don't we support that in the bogo shim? Would it be hard to add support? |
This acts as a regression test for the previous commit. This also enables: - TLS12-Server-CertReq-CA-List - TLS13-Server-CertReq-CA-List - Null-Client-CA-List
c990b62
to
2dad93c
Compare
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.
Thank you. This looks good to me 👍
I feel like we should backport this to 0.22, do you agree? If so, maybe we should pick up #1706 while we're at it?
There's already a test case for this and the API-level behaviour is unchanged.
fixes #1727