Update System.Net.* unit tests to react to SYSLIB0057 obsoletion

[vcsjones]

In #104165 most X509Certificate and X509Certificate2 constructors were obsoleted. Since this affected a large number of unit tests across the entire code base, unit tests initially suppressed the obsoletion warning.

The pull request changes the unit tests for System.Net.* to use the new certificate loader, where appropriate. Some places suppress the obsoletion as they appeared to be specifically testing behaviors and classes, like an X509Certificate instead of an X509Certificate2.

[dotnet-policy-service]

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

[rzikm]

LGTM

[wfurt]

LGTM

[wfurt]

I think the test failure is related to this change.

System.Security.Authentication.AuthenticationException : Authentication failed, see inner exception.
---- System.ComponentModel.Win32Exception : The Local Security Authority cannot be contacted

we used to have this problem and @bartonjs suggested to regenerate PFX file and that fixed it.
Is there chance that the new loader behaves different way than the old one?

[wfurt]

dotnet/runtime-assets#234

[wfurt]

and the original runtime issue #58927

[bartonjs]

Is there chance that the new loader behaves different way than the old one?

Most definitely... but in ways that avoid the need to regenerate PFXes.

System.ComponentModel.Win32Exception : The Local Security Authority cannot be contacted

I've heard reports elsewhere of CI machines having issues talking to the LSA, which seems to be an OS bug (that should have been fixed this week... maybe we don't have patches installed yet?)

[wfurt]

If this is LSA problem we should see failures in other branches. AFAK we did not see failures like this on main, right @rzikm?

[rzikm]

If this is LSA problem we should see failures in other branches. AFAK we did not see failures like this on main, right @rzikm?

No, we don't see those. I wanted to look into these but I did not yet found time to do it.

[rzikm]

There is seems to be a distinct difference in behavior when using one loading method over the other, the failure is pretty reproducible on my dev machine, but only when I run entire test suite, when I run affected tests (even test classes) in isolation, the issue disappears.

Schannel traces suggest that something unexpected happens during InitializeSecurityContext which leads to ERROR_INTERNAL_ERROR being returned internally, which gets translated as the error we see in logs.

I will see if I can reduce the repro to something manageable which we can pass to Schannel folks.

[bartonjs]

There is seems to be a distinct difference in behavior when using one loading method over the other, the failure is pretty reproducible on my dev machine

Huh. I'm really curious as to what's going on here. Private key lifetime did get simplified/normalized during the change, but that would be "before/after", not "old way/new way" (the older APIs still have the new key lifetime changes).

When loading new X509Certificate2(pfx) instead of X509CertificateLoader.LoadPkcs12(pfx) the only differences are:

• We respect the name of the key storage provider when using the old API
• We respect the persisted name of the key when using the old API
  • The thing in the past of rewriting the PFX was to strip the name out. The new loader strips it out at load time (unless the caller requests to preserve it)
• We preserve any cert.FriendlyName values when using the old API
• When using the old API, we pass along any attributes we don't understand... but I've never seen an attribute we don't understand, so that shouldn't be the problem.

None of those differences make sense to me as a long-term problem... they'd either be a problem, or not. A long-term problem sounds like lifetime management; which could be that somewhere you're doing new X509Certificate2(otherCert) or new X509Certificate2(otherCert.Handle) or X509Certificate2Collection.Find, and you're not holding a reference to the original certificate (and so when it finally gets finalized the private key gets erased). But, again, that would be the same between the old and new approaches. -- The one difference would be new X509Certificate2(path) vs X509CertificateLoader.LoadPkcs12FromFile, in that the older API will just pass the PFX directly to Windows, creating fewer objects in the GC heap... and so it might change how long it takes to trigger a finalization.

[rzikm]

A long-term problem sounds like lifetime management; which could be that somewhere you're doing new X509Certificate2(otherCert) or new X509Certificate2(otherCert.Handle) or X509Certificate2Collection.Find, and you're not holding a reference to the original certificate (and so when it finally gets finalized the private key gets erased).

That shouldn't be the case, the common test code keeps static references to the certificates once leaded, and gives out cloned handles.

What I stumbled upon on the Schanel logs are suspicious nullrefs when calculating the CertificateVerify message

both provider and key handle being null is unique to the failing case.

[wfurt]

I suspect this is related to the provider... at least that was the case last time. And selecting "wrong" one TLS 1.3 fails to load algorithms it needs.

[bartonjs]

When using the new certificate loader (and not specifying PreserveProviderName), the private key should always be loaded into the Microsoft Software KSP in CNG; which should be the best supported of all key providers.

[rzikm]

When using the new certificate loader (and not specifying PreserveProviderName), the private key should always be loaded into the Microsoft Software KSP in CNG; which should be the best supported of all key providers.

I don't see any member called PreserveProviderName, but also looking at the code

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Windows.cs

Lines 280 to 308 in d4fe4a7

	private static Interop.Crypt32.PfxCertStoreFlags MapKeyStorageFlags(X509KeyStorageFlags keyStorageFlags)
	{
	Debug.Assert((keyStorageFlags & KeyStorageFlagsAll) == keyStorageFlags);
	Interop.Crypt32.PfxCertStoreFlags pfxCertStoreFlags = 0;
	if ((keyStorageFlags & X509KeyStorageFlags.UserKeySet) == X509KeyStorageFlags.UserKeySet)
	pfxCertStoreFlags |= Interop.Crypt32.PfxCertStoreFlags.CRYPT_USER_KEYSET;
	else if ((keyStorageFlags & X509KeyStorageFlags.MachineKeySet) == X509KeyStorageFlags.MachineKeySet)
	pfxCertStoreFlags |= Interop.Crypt32.PfxCertStoreFlags.CRYPT_MACHINE_KEYSET;
	if ((keyStorageFlags & X509KeyStorageFlags.Exportable) == X509KeyStorageFlags.Exportable)
	pfxCertStoreFlags |= Interop.Crypt32.PfxCertStoreFlags.CRYPT_EXPORTABLE;
	if ((keyStorageFlags & X509KeyStorageFlags.UserProtected) == X509KeyStorageFlags.UserProtected)
	pfxCertStoreFlags |= Interop.Crypt32.PfxCertStoreFlags.CRYPT_USER_PROTECTED;
	// If a user is asking for an Ephemeral key they should be willing to test their code to find out
	// that it will no longer import into CAPI. This solves problems of legacy CSPs being
	// difficult to do SHA-2 RSA signatures with, simplifies the story for UWP, and reduces the
	// complexity of pointer interpretation.
	if ((keyStorageFlags & X509KeyStorageFlags.EphemeralKeySet) == X509KeyStorageFlags.EphemeralKeySet)
	pfxCertStoreFlags |= Interop.Crypt32.PfxCertStoreFlags.PKCS12_NO_PERSIST_KEY | Interop.Crypt32.PfxCertStoreFlags.PKCS12_ALWAYS_CNG_KSP;
	// In .NET Framework loading a PFX then adding the key to the Windows Certificate Store would
	// enable a native application compiled against CAPI to find that private key and interoperate with it.
	//
	// For .NET Core this behavior is being retained.
	return pfxCertStoreFlags;
	}

and https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-pfximportcertstore documentation, it may be the case that we are missing PKCS12_PREFER_CNG_KSP?

@bartonjs, When I add the flag, the test error seems to disappear. Should it be present always or just under specific conditions? I don't se corresponding member in X509KeyStorageFlags.

[bartonjs]

I don't see any member called PreserveProviderName

Sorry, PreserveStorageProvider

it may be the case that we are missing PKCS12_PREFER_CNG_KSP?

Based on testing in the past, a key with no provider name gets loaded into CNG. But, if you're seeing a behavioral change, perhaps something has changed, or it was more nuanced than I thought.

It's definitely not a thing we want to expose through X509KeyStorageFlags. It reads like it's a fine thing to always pass, so let's just always pass it.

[wfurt]

We should be able to get the provider from the kay, right @bartonjs? e.g. we can dump it for the failed test cases and see where the key was loaded.

[bartonjs]

((RSACng)cert.GetRSAPrivateKey()).Key.Provider.Provider, I believe. (Hard cast will of course only work on Windows). If there's not already a test for this for the new loader, I'll write one up today.

[bartonjs]

Added the test, and, yep, I'm wrong. It looks like the "improved" loader is loading things into the CAPI "Microsoft Base Cryptographic Provider v1.0". I'm surprised the universe isn't exploding.

[rzikm]

No known failures anymore, we can ship it.