Skip to content

[release/5.0] rebuild certificate context if we use client cert from credential cache#48042

Merged
Anipik merged 1 commit intodotnet:release/5.0from
wfurt:50clientCert_47580
Feb 10, 2021
Merged

[release/5.0] rebuild certificate context if we use client cert from credential cache#48042
Anipik merged 1 commit intodotnet:release/5.0from
wfurt:50clientCert_47580

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Feb 9, 2021

Fixes #47580

Customer Impact

Heavily upvoted customer report of regression in 5.0.

TLS handshake can sometimes fail when client certificates are used for authentication.
This depends on server configuration as well on internal SslStream caching.
In general, this is difficult to predict and diagnose in the field.

Cause: in 5.0 we added options to provide full certificate chain to avoid work on every connection. That also removed certificate chain building from PAL and moved it up so it is done only when needed. That logic missed one place when the chain is not rebuilt when credential cache is used for client certificate. In such case we fail to include intermediate certificates in TLS handshake and that can lead to handshake failure. It works on first attempt (covered by tests) but it may fail on subsequent attempts when cache is used. (missed by current tests)

Regression?

yes. same scenarios work with 3.1 and got broken in 5.0.

Risk

very low. This is minimal change to get on par with 3.1

Testing

We did not have any tests to cover cases when credential cache is used. This changes adds basic to cover the scenario e.g. try client auth few times while creating conditions for cache lookup.

@wfurt wfurt added Servicing-consider Issue for next servicing release review area-System.Net.Security labels Feb 9, 2021
@wfurt wfurt added this to the 5.0.x milestone Feb 9, 2021
@wfurt wfurt requested review from a team and bartonjs February 9, 2021 07:24
@wfurt wfurt self-assigned this Feb 9, 2021
@ghost
Copy link

ghost commented Feb 9, 2021

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

Issue Details

Customer Impact

TLS handshake can sometimes fail when client certificates are used for authentication.
This depend on server configuration as well on internal SslStream caching.
In general, this is difficult to predict and diagnose in the field.

In 5.0 we added options to provide full certificate chain to avoid work on every connection. That also removed certificate chain building from PAL and moved it up so it is done only when needed. That logic missed one place when the chain is not rebuilt when credential cache is used for client certificate. In such case we fail to include intermediate certificates in TLS handshake and that can lead to handshake failure. It works on first attempt (covered by tests) but it may fail on subsequent attempts when cache is used. (missed by current tests)

Regression?

yes. same scenarios work with 3.1 and got broken in 5.0.

Risk

very low. This is minimal change to get on par with 3.1

Testing

We did not have any tests to cover cases when credential cache is used. This changes adds basic to cover the scenario e.g. try client auth few times while creating conditions for cache lookup.

Author: wfurt
Assignees: wfurt
Labels:

Servicing-consider, area-System.Net.Security

Milestone: 5.0.x

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 9, 2021
@leecow leecow modified the milestones: 5.0.x, 5.0.4 Feb 9, 2021
@danmoseley
Copy link
Member

danmoseley commented Feb 9, 2021

@wfurt tactics says approved if @bartonjs signs off.

BTW I see some possibly related test failures.

@danmoseley
Copy link
Member

BTW I see some possibly related test failures

I tried to rerun the legs that appeared to have unrelated test failures, but ended up rerunning them all. But you can still see the test failure in Azdo.

@wfurt
Copy link
Member Author

wfurt commented Feb 9, 2021

Thanks @danmosemsft. I will take a look.

@Anipik Anipik merged commit cda1bdc into dotnet:release/5.0 Feb 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 12, 2021
@wfurt wfurt deleted the 50clientCert_47580 branch January 7, 2026 20:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants