Refactor enclave providers to separate enclave-type from attestation-mechanism (composition over inheritance)

[paulmedynski]

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

[github-actions]

🔍 Triage Summary

Check	Result
Issue type	Task
Environment	N/A — not a bug report; no environment details required
Area	Area\AKV Provider (Always Encrypted / secure enclave provider infrastructure)
Duplicates	None found
Regression	Not indicated — this is a code-quality / maintainability refactor

Analysis

This issue proposes a well-reasoned refactoring of the Always Encrypted secure-enclave provider class hierarchy to eliminate a false "is-a" relationship between HostGuardianServiceEnclaveProvider and VirtualizationBasedSecurityEnclaveProviderBase. The current design conflates two orthogonal concerns — enclave type (VBS/SGX) and attestation mechanism (HGS/Azure Attestation/None) — into a single inheritance chain, and encodes them inconsistently across siblings. The proposal replaces inheritance with composition via IEnclaveReport and IAttestationService abstractions, which is a sound design choice. No public API break is intended; this is a P3 maintainability improvement with no functional user impact.

Next Steps

• Assign this issue to the Copilot coding agent to implement the refactoring. Specific guidance:
  1. Start with the low-risk, standalone cleanup: rename VirtualSecureModeEnclaveProviderBase.cs → VirtualizationBasedSecurityEnclaveProviderBase.cs and VirtualSecureModeEnclaveProvider.cs → HostGuardianServiceEnclaveProvider.cs; fix the "Virtual Secure Modules" comment in the affected file.
  2. Introduce internal IAttestationService and IEnclaveReport interfaces in the Microsoft.Data.SqlClient namespace.
  3. Implement VbsEnclaveReport, SgxEnclaveReport, HgsAttestationService, AzureAttestationService, and NoAttestationService as internal types backed by logic extracted from the current base classes.
  4. Reduce HostGuardianServiceEnclaveProvider, AzureAttestationEnclaveProvider, and NoneAttestationEnclaveProvider to thin public shells delegating to the composed internals.
  5. Run reference-assembly comparison (netcore/ref/ and netfx/ref/) to confirm no public API surface changes.
  6. Verify all existing Always Encrypted enclave integration and unit tests still pass, including both sync and async code paths.

Note: This triage summary is auto-generated by an AI agent. The analysis and suggestions above have not been verified by a human maintainer. Please treat as preliminary guidance only.

Generated by SqlClient Issue Auto-Triage for issue #4480 · 31.6 AIC · ⌖ 7.38 AIC · ⊞ 8.6K · ◷

[edwardneal]

It's worth noting that SqlColumnEncryptionEnclaveProvider is only a public API in System.Data.SqlClient - it's not publicly exposed in MDS. This should introduce slightly more flexibility in the approach (and I'm personally counting on being able to change it in order to be able to expose it via netstandard2.0 and harmonise the target-specific runtime API surfaces with the ref assemblies.)