Skip to content
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

fix TLS resume with client certificates #79898

Merged
merged 8 commits into from
Jan 19, 2023
Merged

fix TLS resume with client certificates #79898

merged 8 commits into from
Jan 19, 2023

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Dec 22, 2022

This is 7.0 regression caused by combination of #64369 #63200 #63945.
The intention always was to disable TLS resume with client certificates.
However we also changed how client certificates are selected so the CertificateContext may not be set when the handle is created and it is updated later when server asks for it via UpdateClientCertiticate.

To make it simple, this change will also disable TLS resume if there are client certificate candidates e.g. the selection was not made yet. With guard for the callback, this hopefully should be complete.

I added few more tests to guard various combinations of how client certs can be selected.

fixes #79869
contributes to #75545

@wfurt wfurt added this to the 8.0.0 milestone Dec 22, 2022
@wfurt wfurt requested a review from a team December 22, 2022 02:57
@wfurt wfurt self-assigned this Dec 22, 2022
@ghost
Copy link

ghost commented Dec 22, 2022

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

This is 7.0 regression caused by combination of #64369 #63200 #63945.
The intention always was to disable TLS resume with client certificates.
However we also changed how client certificates are selected so the CertificateContext may not be set when the handle is created and it is updated later when server asks for it via UpdateClientCertiticate.

To make it simple, this change will also disable TLS resume if there are client certificate candidates e.g. the selection was not made yet. With guard for the callback, this hopefully should be complete.

I added few more tests to guard various combinations of how client certs can be selected.

fixes #79869
contributes to #75545

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security

Milestone: 8.0.0

@wfurt
Copy link
Member Author

wfurt commented Dec 29, 2022

This is getting more complicated. The real fix for #79869 is still one line in Interop.OpenSsl.cs. However the added tests were failing on Windows, showing that #79128 was not complete fix.
It seems like we cannot determine reliably usage of the client certificate in case of TLS resume. To fix that, we will now store copy of selected certificate into credentials. And we do that only when Schannel asks so we have confidence it is actually used. With that, we avoid issue I mentioned here. (reverted CertificateValidationClientServer_EndToEnd_Ok test workaround)

Changes around refreshCredentialNeeded are unrelated and it is just small optimization to keep the logic together and it allows to replace instance property with local variable.

contributes to #65563

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

It might be worth checking if this does not cause perf regression, there have been some sneaky ones in the past in this area.

@wfurt
Copy link
Member Author

wfurt commented Jan 4, 2023

We did not have any benchmarks so I opened dotnet/performance#2814
For Linux, it will drop back to 6.0 e.g. there will not be TLS resume. After experimenting with Windows, I think we may be able to resume with client certs in some situations but that would be new work for 8.0.

For Windows, the extra work is done only when somebody access IsMutuallyAuthenticated. That is probably rare and the cost of extra call is probably small compared to handshake crypto.

@wfurt
Copy link
Member Author

wfurt commented Jan 19, 2023

WASM & OS X failures seems unrelated. Strangely, the consoles look like success but it is still reported as failure.

@wfurt wfurt merged commit 8300e38 into dotnet:main Jan 19, 2023
@wfurt wfurt deleted the ccert branch January 19, 2023 20:10
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
* fix TLS resume with client certificates

* fix windows

* fix unused warning

* undo dotnet#79128 test change

* remove dead code

* fix resolve
@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET 7, Linux, SSL connection is being reused between independent HttpClients?
2 participants