Cache dynamically generated RSA keys in tests

[bartonjs]

Many of our tests want to use generated keys to avoid any notion that the test pass/fail is tied to using a specific key, but they very rarely care that they're using a completely fresh key.

With this change, the tests that don't involve mutating key objects (via Import*, Dispose, or calling set_KeySize), and otherwise were generating a new key every time, will now share keys across tests.

OuterLoop for S.S.Cryptography.Tests drops from generating 2,039 RSA keys across the various sizes to generating only about 43 (the specific number will depend on concurrency). (For an inner-loop test run it's 189 dropping to the same 43 or so.)

The change also lays infrastructure to pool ECDSA/etc keys, and for commonly-imported key values into pooled objects.

In addition to doing a bit less of a stress-test on the key generator routines, this should speed up the tests a bit. On Windows, it shaves a couple of seconds off of the outer loop run (~160s -> ~155s).

At this time the specific-type tests (S.S.C.Cng.Tests, etc) are not using pooling.

Hopefully contributes to #25979.

[ghost]

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

Issue Details

---

Many of our tests want to use generated keys to avoid any notion that the test pass/fail is tied to using a specific key, but they very rarely care that they're using a completely fresh key.

With this change, the tests that don't involve mutating key objects (via Import*, Dispose, or calling set_KeySize), and otherwise were generating a new key every time, will now share keys across tests.

OuterLoop for S.S.Cryptography.Tests drops from generating 2,039 RSA keys across the various sizes to generating only about 43 (the specific number will depend on concurrency). (For an inner-loop test run it's 189 dropping to the same 43 or so.)

The change also lays infrastructure to pool ECDSA/etc keys, and for commonly-imported key values into pooled objects.

In addition to doing a bit less of a stress-test on the key generator routines, this should speed up the tests a bit. On Windows, it shaves a couple of seconds off of the outer loop run (~160s -> ~155s).

At this time the specific-type tests (S.S.C.Cng.Tests, etc) are not using pooling.

Hopefully contributes to #25979.

Author:	bartonjs
Assignees:	-
Labels:	area-System.Security, test-enhancement
Milestone:	8.0.0

[bartonjs]

I went through every call to RSAFactory.Create or RSA.Create to find out if they could be changed to pooled keys, or not. All the ones that did ImportParameters as their first line are from .NET Core 1.0 (hi, former self!), before we had accelerators for that. They generally got changed over so they a) didn't look like false positives and b) can suggest other key parameter values that are worth pooling as well-known keys.

[vcsjones]

To be honest I'm not exactly enthusiastic about this change. It makes testing RSA (and presumably ECDsa and other primitives in the future) stray further from writing tests more naturally how these classes get used. IdemponentRSA is not RSA anymore and increases the chances of a testing mistake (at least for me) such as forgetting to override something, or some interactions of virtuals, overrides, etc. that it does not reflect. Sure, we have other derived RSA objects for testing purposes, but that's usually to test the interaction of virtuals and overrides, not "is this really RSA".

Re-using parameters where we don't care about key generation makes sense to me like handing in TestData.RSA2048Params to RSAFactory.Create(). In that case I don't dislike it because it's using public APIs and isn't not a "real" RSA object.

I think I understand "why", but

1. the perf improvements to the tests is not worth it and
2. for the weird "CNG gets unhappy" I think it would be worth investing time in RSA.Create() on Windows returning something that is backed by BCrypt instead of NCrypt (yeah, that's probably difficult).

[vcsjones]

I'm not sure the intention behind this. RSA has a proper implementation of this now (same for the overloads below) in the virtual. So it seems like the intention is to only allow SHA256 hashing. Why?

[bartonjs]

Apparently just having embedded in my memory that I had to implement it; and SHA256 was the only value being passed in.

[vcsjones]

Should we just remove these overrides then?

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/RSA.cs

Lines 68 to 72 in e4d115a

	protected virtual byte[] HashData(byte[] data, int offset, int count, HashAlgorithmName hashAlgorithm) =>
	HashOneShotHelpers.HashData(hashAlgorithm, new ReadOnlySpan<byte>(data, offset, count));
	protected virtual byte[] HashData(Stream data, HashAlgorithmName hashAlgorithm) =>
	HashOneShotHelpers.HashData(hashAlgorithm, data);

[vcsjones]

Assuming I am right about why this line is there:

Suggested change

	_impl.TryExportRSAPublicKey(Span<byte>.Empty, out _);
	// Eagerly force the key generation
	_impl.TryExportRSAPublicKey(Span<byte>.Empty, out _);

[bartonjs]

To be honest I'm not exactly enthusiastic about this change.

I'm not sure I am, either, if it helps 😄.

IdemponentRSA is not RSA anymore and increases the chances of a testing mistake

The purpose of the wrapper class was really to make sure that everywhere I converted wasn't mutating the object. e.g. it'd be bad if someone rented a 2048 key, imported a 3072 parameters, then put it back in the 2048 bucket... later the goldilocks TrySignHash comes up, expects a 2048-bit signature, and reports that we broke the world.

But, yeah, that highlights test fragility.

It's really sending things to the wrapped object for sign/verify/encrypt/decrypt; it's not a dummy provider... so I wouldn't say "it's not RSA anymore". But it is "the tests are doing things that real code won't".

or the weird "CNG gets unhappy" I think it would be worth investing time in RSA.Create() on Windows returning something that is backed by BCrypt instead of NCrypt

One thing that I'm not certain of is where things are getting unhappy. NTE_INTERNAL_ERROR isn't very descriptive. I feel like 99% of the times we've seen it it has been RSA key generation. So, what exactly failed? Registering with the LSA bridge (unique to NCrypt), key generation (the same for NCrypt or BCrypt as they're both SymCrypt under the covers), something else?

The Windows Crypto team has suggested to us to switch to BCrypt for ephemeral keys, but I believe that's more for perf than reliability. We can certainly do it for RSA.Create(), and could probably get away with it for cert.GetRSAPublicKey() (always) and cert.GetRSAPrivateKey() (Ephemeral PFX loads actually give us a BCrypt key that we have to move to NCrypt). For the public type RSACng it's hard, since it exposes an NCRYPT_KEY_HANDLE via rsa.Key.Handle.

Re-using parameters where we don't care about key generation makes sense to me

If the unknown error is in key generation then changing this idea from pooling working objects to pooling RSAParameters values might still help. (Generate new ones when the pool is empty, so we're not just always testing with the same fixed sets of parameter data.)

But that's... probably more effort than reward.

[vcsjones]

I'm not sure I am, either, if it helps 😄.

It does.

The purpose of the wrapper class was really to make sure that everywhere I converted wasn't mutating the object.

Understood; I'm just wary of it subtly changing what we are testing. Consider code like this:

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs

Lines 452 to 458 in 1f1231c

	RSAImplementation.RSAAndroid? typedKey = privateKey as RSAImplementation.RSAAndroid;
	if (typedKey != null)
	{
	return CopyWithPrivateKeyHandle(typedKey.DuplicateKeyHandle());
	}
	RSAParameters rsaParameters = privateKey.ExportParameters(true);

If we give this code what the RSA.Create() returns, it fast-paths cloning a handle. If it's <something else> then it exports and imports parameters.

I understand that your changes here are not affecting this specific example since it lives in X.509 world, but it's an example of somewhere where going from the internal implementation to a wrapper would cause what code paths get exercised to change.

[bartonjs]

Based on the hesitation here, and a chat I had with @GrabYourPitchforks in the office, I'm going to close this in favor using BCrypt instead of NCrypt when possible (I have RSABCrypt 95% written).

If that doesn't make the STATUS_UNSUCCESSFUL/NTE_INTERNAL_ERROR errors go away, then there are a few things:

• Make the operations that are currently doing retry for STATUS_UNSUCCESSFUL do a detour to NCryptFinalizeKey, to see if we get a better error out of that call.
• Make our tests use a filtering function, e.g. RSAFactory.Create() will call the create routine, then call sign on empty data, and if the key fails throw it away.

For test throughput we could background generate some keys, so that when we get to the serial execution of the revocation tests, with all the keys they use, we move their couple hundred ms of keygen off to a different thread.