Refactor enclave providers to separate enclave-type from attestation-mechanism (composition over inheritance)
- Dominant language
- C#
- Stars
- 989
- Forks
- 340
- Avg merge
- 4d 18h
- Merged PRs (30d)
- 69
Description
## Summary
The Always Encrypted secure-enclave provider classes conflate two orthogonal concerns into a single inheritance chain, producing a class hierarchy that models an "is-a" relationship that does not actually exist. Most notably, `HostGuardianServiceEnclaveProvider` inherits from `VirtualizationBasedSecurityEnclaveProviderBase`, which implies *HGS is-a VBS enclave provider*. HGS (Host Guardian Service) is an **attestation mechanism**, not an enclave and not a provider of enclaves. These are two separate concerns.
## Background: two orthogonal axes
Establishing a trusted enclave session involves two independent decisions:
| Axis | Values | Responsibility |
|------|--------|----------------|
| **Enclave type** | VBS, SGX | Parse the enclave identity/report payload and check it against the expected policy |
| **Attestation mechanism** | HGS, Azure Attestation, None | Establish trust that the enclave report is genuine |
These axes are independent — e.g. a VBS enclave can be attested via HGS *or* via Azure Attestation *or* not at all. The correct conceptual model is a pair `(enclaveType, attestationMechanism)`, not a single inheritance line.
## Current design and why it's wrong
Current hierarchy (`src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/`):
```
SqlColumnEncryptionEnclaveProvider (public abstract API)
└─ EnclaveProviderBase (shared session-cache / lock helpers)
├─ VirtualizationBasedSecurityEnclaveProviderBase (file: VirtualSecureModeEnclaveProviderBase.cs)
│ └─ HostGuardianServiceEnclaveProvider (file: VirtualSecureModeEnclaveProvider.cs)
├─ AzureAttestationEnclaveProvider
└─ NoneAttestationEnclaveProvider
```
Problems:
1. **Category error / false is-a.** `HostGuardianServiceEnclaveProvider : VirtualizationBasedSecurityEnclaveProviderBase` encodes the enclave type (VBS) in the base and the attestation mechanism (HGS) in the leaf. This reads as "HGS is a kind of VBS enclave provider," which is not a real is-a relationship. The base class also fully implements VBS-specific verification, leaving only the attestation transport (`MakeRequest`, `GetAttestationUrl`) abstract — so the inheritance axis is actually "attestation transport," while the class *names* describe enclave type.
2. **Inconsistent modeling between siblings.** The two axes aren't even encoded the same way across providers:
- The VBS/HGS branch bakes **enclave type into the type hierarchy**.
- `AzureAttestationEnclaveProvider` treats **enclave type as runtime data** — its verification takes an `EnclaveType enclaveType` parameter and handles VBS/SGX within one class.
The same concern is modeled two different ways, a strong signal that the inheritance axis was chosen accidentally rather than to express real polymorphism.
3. **Misleading names / file names.** The file `VirtualSecureModeEnclaveProviderBase.cs` contains the type `VirtualizationBasedSecurityEnclaveProviderBase` (VSM vs VBS — different concepts), and `VirtualSecureModeEnclaveProvider.cs` actually contains `HostGuardianServiceEnclaveProvider` plus the `AttestationInfo` models. A stray comment also describes the flow as "per the protocol used by Virtual Secure Modules," when the verification is VBS/HGS-specific.
## Proposed direction: composition over inheritance
Separate the two concerns into composable collaborators and have the provider delegate to them:
```
EnclaveSessionProvider
├─ IEnclaveReport (VbsEnclaveReport | SgxEnclaveReport) // parse identity/report, check policy
└─ IAttestationService (HgsAttestation | AzureAttestation | NoAttestation) // establish trust in the report
```
- "VBS enclave attested by HGS" becomes a **composition** (`new EnclaveSessionProvider(vbsReport, hgsAttestation)`), not a subclass.
- HGS is correctly modeled as an `IAttestationService` and never appears as an "EnclaveProvider."
- New enclave types or attestation services drop in without new hierarchy branches or combinatorial subclasses.
## Constraints / notes for implementation
- `SqlColumnEncryptionEnclaveProvider` is **public API** and providers are selected/registered by name, so the existing public provider classes cannot simply be deleted. They should be reduced to thin shells that wire up the appropriate `(IEnclaveReport, IAttestationService)` composition and delegate to a shared internal implementation. No public API break intended.
- Only a subset of the `(enclaveType × mechanism)` matrix ships today, so this is a maintainability/clarity refactor rather than a functional bug — but the current shape risks forcing another awkward hierarchy decision the next time an enclave type or attestation service is added.
- Behavior must remain identical; existing Always Encrypted enclave tests should continue to pass, and both sync and async paths should be covered.
## Suggested scope
- [ ] Introduce internal `IAttestationService` (HGS, Azure Attestation, None) and `IEnclaveReport` (VBS, SGX) abstractions.
- [ ] Move VBS verification logic out of `VirtualizationBasedSecurityEnclaveProviderBase` into a VBS report/attestation composition.
- [ ] Reduce `HostGuardianServiceEnclaveProvider`, `AzureAttestationEnclaveProvider`, and `NoneAttestationEnclaveProvider` to thin public shells that compose the collaborators.
- [ ] Low-risk cleanup that can land first: rename files to match their types (`VirtualizationBasedSecurityEnclaveProviderBase.cs`, `HostGuardianServiceEnclaveProvider.cs`) and fix the "Virtual Secure Modules" comment.
- [ ] Ensure no public API changes; verify with reference-assembly comparison and existing enclave tests.
## Affected files
- `src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/VirtualSecureModeEnclaveProviderBase.cs`
- `src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/VirtualSecureModeEnclaveProvider.cs`
- `src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AzureAttestationBasedEnclaveProvider.cs`
- `src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/NoneAttestationEnclaveProvider.cs`
- `src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/EnclaveProviderBase.cs`
- `src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionEnclaveProvider.cs`
Contributor guide
Research direction
Read the affected provider files, especially VirtualSecureModeEnclaveProviderBase.cs, VirtualSecureModeEnclaveProvider.cs, and EnclaveProviderBase.cs, then inspect the existing Always Encrypted enclave tests. Trace the synchronous and asynchronous verification paths and review the public provider API before designing the composition. Done means behavior and public APIs remain unchanged, reference assemblies match, and the existing enclave tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend-api-design, security
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100