Follow up questions on Windows AES one-shot encryptor

[Sergio0694]

Hey @vcsjones, I had a few follow up questions about #58270 😄
First, just thank you so much for all your great work on crypto APIs in .NET 6, they're so much better now!

About BasicSymmetricCipherLiteBCrypt.

1. We're still copying the IV to an array every time when both encrypting and decrypting:

runtime/src/libraries/Common/src/Internal/Cryptography/BasicSymmetricCipherLiteBCrypt.cs

Lines 30 to 34 in f9a1c78

	if (!iv.IsEmpty)
	{
	// Must copy the input IV
	_currentIv = iv.ToArray();
	}

I understand we can't pass the IV directly because the implementation on Windows writes on that block while encrypting/decrypting, but can't we rent this buffer from a pool instead? Or better yet, given this ILiteSymmetricCipher instance is used inline by the caller, couldn't the caller just stackallocate a block, copy the IV to it, pass a pointer to the cipher, and then it could just use that pointer assuming it points to a fixed block with the IV, which could then just directly be passed to the native API?

2. If I'm reading this bit right, aren't we importing the same key every time?

runtime/src/libraries/Common/src/Internal/Cryptography/BasicSymmetricCipherLiteBCrypt.cs

Line 39 in f9a1c78

_hKey = Interop.BCrypt.BCryptImportKey(algorithm, key);

I know that when using OpenSSL we do need to reinitialize the handle as it uses both the key and the IV, but given on Windows it only uses the key and the IV is a separate parameter, couldn't we cache this imported key when using the instance methods of AesImplementation? That should reduce the overhead a bit when reusing the same Aes instance, correct?

About ILiteSymmetricCipher: couldn't the various implementations be structs?
There should be no difference when they're used just through the interface, as whoever's creating the instance would just box them once and return them directly as ILiteSymmetricCipher, but the one-shot APIs could leverage this to just declare the locals with the actual cipher type and avoid allocations there too? Given the type is internal it should be fine even if making these types structs would technically make them more error prone due to accidental copies? Or was this too much of a concern? 🤔

Thanks! And again, great work on all the crypto APIs! 🚀

[vcsjones]

but can't we rent this buffer from a pool instead? Or better yet, given this ILiteSymmetricCipher instance is used inline by the caller, couldn't the caller just stack allocate a block, copy the IV to it, pass a pointer to the cipher, and then it could just use that pointer assuming it points to a fixed block with the IV, which could then just directly be passed to the native API?

It probably can be done that way. I had that same comment in #55601 but the work around it hasn't been done.

couldn't we cache this imported key when using the instance methods of AesImplementation

We can, and that is an outstanding item in #55601, but it's somewhat non-trivial because the one shots are currently thread safe. You can't use a Windows key handle from multiple threads, and I don't want the APIs to go from thread safe to not thread safe. So _hKey would need to be per-thread. There is also the matter of invalidating the _hKey if the Key property changes.

About ILiteSymmetricCipher: couldn't the various implementations be structs?

They could, and I started out that way, but I am having a hard time remembering why I went back to classes. I think the AesCng class made that more work than I was willing to take on at the time.

I am taking the time to do this "right" with structs (and using generics with constraints to even avoid boxing) for the stream hashing, so I can probably revisit this when I am done with that.

I will update #55601 with these comments so that I don't forget them.

[Sergio0694]

Sweet, thank you for the additional info!
And glad to hear these are all things you're looking into 😄

"it's somewhat non-trivial because the one shots are currently thread safe. You can't use a Windows key handle from multiple threads, and I don't want the APIs to go from thread safe to not thread safe"

I'm just a bit confused on this - I thought that only static APIs were guarantees to be thread-safe, but not instance ones, regardless of whether they were one-shot or not? Maybe I just misread @GrabYourPitchforks' comment on that? 🤔

[vcsjones]

I thought that only static APIs were guarantees to be thread-safe, but not instance ones, regardless of whether they were one-shot or not?

That’s true, but the APIs happen to be thread safe today in .NET 6. We could break it, but it would not surprise me if it people took a dependency on the APIs being thread safe as I have seen with CreateTransform. The unfortunate result of this is usually data corruption.

When I revisit this I can figure out if we need to remain thread safe, but generally the purpose of one shots is to be misuse resistant (also perf) and losing thread safety makes the API a bit less resistant to misuse.

[Sergio0694]

Ah, I see. That's kinda unfortunate though, because according to the docs the instance APIs deliberately do not guarantee to be thread-safe. It would be a bit of a shame to have to be limited or to lose perf for people ignoring the docs here, and also it would result in a situation where the docs explicitly say an API is not thread-safe, but then everyone just relies on the implementation detail causing it to be (and also it would be inconsistent with all our other non thread-safe instance APIs elsewhere) 🤔