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

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

Merged
merged 1 commit into from
Feb 10, 2021

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 bartonjs and a team 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.

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