rebuild certificate context if we use client cert from credential cache

[wfurt]

This regression caused by #38364. We used to call X509Chain.Build inside of pal for each connection. We don't want to do that for the SslStreamCertificateContext so the change would also create context for client certificates to keep the pal simple. However, the context is not part of are cache so when get client certificate from cache it would not have the associated certificates and it would send only leave certificate. And some servers are sensitive to it.

This is mini fix to get on par with 3.1.
Longer term fix may be to make context part of the cache ... and possibly measure how much the cache actually saves and re-consider usefulness. That can be done later as per fix instead of functional change.

The most digital part of this change is the test. Since the client side has no API to take certificate chain, only one way is to add intermediates to the CA store. With that, the server side can always construct full chain regardless of what client actually provided. Also intermediate certificate can be cached behind the scenes so the verification may be tricky.

I eventually realized that VerifyRemoteCertificate add certificates from wire chain.ChainPolicy.ExtraStore.AddRange(remoteCertificateStore). So I use that for fix validation.
I verified that without the fix only first round would work and that subsequent handshakes would have no extra certs - just like described in the linked issue.

If somebody has better idea for validation please let me know.

fixed #47580

[ghost]

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

Issue Details

---

This regression caused by #38364. We used to call X509Chain.Build inside of pal for each connection. We don't want to do that for the SslStreamCertificateContext so the change would also create context for client certificates to keep the pal simple. However, the context is not part of are cache so when get client certificate from cache it would not have the associated certificates and it would send only leave certificate. And some servers are sensitive to it.

This is mini fix to get on par with 3.1.
Longer term fix may be to make context part of the cache ... and possibly measure how much the cache actually saves and re-consider usefulness. That can be done later as per fix instead of functional change.

The most digital part of this change is the test. Since the client side has no API to take certificate chain, only one way is to add intermediates to the CA store. With that, the server side can always construct full chain regardless of what client actually provided. Also intermediate certificate can be cached behind the scenes so the verification may be tricky.

I eventually realized that VerifyRemoteCertificate add certificates from wire chain.ChainPolicy.ExtraStore.AddRange(remoteCertificateStore). So I use that for fix validation.
I verified that without the fix only first round would work and that subsequent handshakes would have no extra certs - just like described in the linked issue.

If somebody has better idea for validation please let me know.

fixed #47580

Author:	wfurt
Assignees:	wfurt
Labels:	area-System.Net.Security
Milestone:	-

[geoffkizer]

Pardon my ignorance here, but does this change affect when you are using SslStreamCertificateContext, or not, or both?

[wfurt]

Publicly, SslStreamCertificateContext is used only for server as added to SslServerAuthenticationOptions
Internally, we converted the code to use the context in all cases - client & server in order to keep the PAL simpler.
However that change missed one place when the client certificate is pulled from credential cache.
In the "normal" case we would do the same just few lines bellow:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs

Lines 595 to 601 in 5b41e82

	else
	{
	if (selectedCert != null)
	{
	_sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert!);
	}

As I mentioned in the text, I think better long term fix would be to refactor the cache to hold the context. I also noticed that some parameters passed to PAL are essentially duplicates as there is already access to _sslAuthenticationOptions inside PAL.

I did not want to do the re-factor here as I'm hoping to get approval for servicing and I wanted to keep the product change as simple as possible. I plan to follow-up with some refactor-cleanup for master. Let me know if that make sense @geoffkizer

BTW we could also add SslStreamCertificateContext to the client side to be consistent. But there was no compelling use case so that did not happened in 5.0

[wfurt]

I look at the test failures and I have semi-reliable local repro. It seems like sometimes X509Chain succeeds (e.g. returns true) but the chain.ChainElements has only one item instead of three inside of SslStreamCertificateContext(). With that, client does not send the chain and the test fails.
I'll keep digging but if you have some idea let me know @bartonjs. For one, how do we synchronize certificates after adding to StoreName.CertificateAuthority? If this would be root case, the build chain should fail, right?

[wfurt]

I open #48017 to track the chain unreliability. merging so I can start servicing request for the product change.

[wfurt]

/backport to release/5.0

[github-actions]

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/550366727

[github-actions]

@wfurt backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: rebuild context if we use client cert from cache
error: sha1 information is lacking or useless (src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 rebuild context if we use client cert from cache
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!