Cleanup | Deduplicate enclave attestation session caching logic

[edwardneal]

Description

This is one of two foundational PRs, and it eases the resolution of several issues.

At present, we have three enclave attestation providers:

• AzureAttestationEnclaveProvider
• NoneAttestationEnclaveProvider
• HostGuardianServiceEnclaveProvider

All three of these inherit, directly or indirectly, from a common base class (EnclaveProviderBase) which in turn inherits from SqlColumnEncryptionEnclaveProvider.

At present, SqlColumnEncryptionEnclaveProvider requires its derived classes to take responsibility for caching SqlEnclaveSession instances. EnclaveProviderBase provides a series of helpers, and the three derived classes call them. Their logic is almost identical - InvalidateEnclaveSession calls InvalidateEnclaveSessionHelper in every implementation, while every implementation of CreateEnclaveSession has the same basic pattern of handling locking, trying to get a session from the cache using GetEnclaveSessionHelper, then parsing the enclaveAttestationInfo byte array if no such session exists.

The primary motive for this PR is to simplify the various enclave attestation provider derived classes. All of them support caching, and all of them have the same boilerplate caching logic. I've thus pushed most of that responsibility away from the derived classes, into the base EnclaveProviderBase class. The only caching-related responsibility which remains in the derived classes is GetEnclaveSession, and this calls GetEnclaveSessionHelper in the base class; I've not moved this because GetEnclaveSessionHelper isn't currently in a fit shape to move, and making it such would blur a simple mechanical refactor into a change to the enclave providers' calling contract.

Taking a wider look, however: while the attestation provider might want to signal that it doesn't support caching, responsibility for the caching itself should lie within the dedicated EnclaveSessionCache type. I'm planning to follow this PR up with a second one which builds on this consolidation work and moves the now-consolidated caching implementation into the cache. Doing so will also remove any need for the EnclaveProviderBase type to exist.

Oddity: GetEnclaveSession

I'm calling this out because I'm conscious that it looks very odd to move all of the caching logic into EnclaveProviderBase, but then to leave this method unchanged. The key reason here is that GetEnclaveSession performs two functions: if a cached enclave session exists, it'll retrieve it; if it doesn't exist and the generateCustomData parameter is set, it'll populate the customData and the customDataLength parameters. This is the client-side nonce.

There are a few problems with this:

1. This nonce is used to begin the enclave attestation handshake, and its population logic is in the wrong place. We have GetAttestationParameters for this, and we weaken the calling contract when we populate variables independently to this.
2. The nonce is an implementation detail of AzureAttestationEnclaveProvider. The base class shouldn't care about that detail.

I plan to move this generation logic into SqlEnclaveAttestationParameters, but doing so means that I'm changing the calling contract of the enclave providers. I don't want to do that inside this mechanical refactoring PR.

Follow-up PR (and end state)

In the follow-up PR, we'll remove the EnclaveProviderBase type; the three enclave attestation providers will be able to inherit from SqlColumnEncryptionEnclaveProvider. SqlEnclaveAttestationParameters will also be changed, to avoid exposing the ECDiffieHellman type (which doesn't exist in a netstandard2.0 target) and to include the client-side nonce.

With those responsibilities gone, SqlColumnEncryptionEnclaveProvider will only need two methods: CreateEnclaveSession and GetAttestationParameters. The upcoming async support for the enclave attestation provider will only need to add a CreateEnclaveSessionAsync method.

Most importantly, however: we'll have severed the dependency between the enclave attestation providers and the internals of SqlClient. This opens the way to moving the base SqlColumnEncryptionEnclaveProvider class and its associated types into the Abstractions package (and the AzureAttestationEnclaveProvider type into the Azure package.)

Scope

It's worth noting that each of the enclave providers parses enclaveAttestationInfo by performing various Buffer.BlockCopy calls between byte arrays, allocating as it goes. I'm not planning to change that in this PR or its follow-up; these PRs are common work which will simplify a couple of Always Encrypted issues, and I don't want them to drift into general sets of performance improvements.

Issues

This intersects with a few issues.
#4017: shrinks the API surface we'd need to expose.
#4425: while this is simply extracting common functionality rather than making a performance improvement, it does prepare to remove a layer of unnecessary inheritance.
#3672: narrowing the scope of CreateEnclaveSession method and its async counterpart makes unit testing slightly easier.

Testing

Existing tests continue to pass. Common code is simply being moved away from the derived types and into their common base type, so no new tests have been written. Once I start actually consolidating the caching functionality into EnclaveSessionCache, I'll write tests for this.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.