[6.1] Fix encryption key cache design for AKV provider #3477
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Fix AzureSqlKeyCryptographer to be pure sync * Fix Symmetric key cache design + Key vault cache * Fix tests * Add back multi-user cache scenario * Add cache lock in global CEK cache to prevent concurrency issues. * Touch-ups * More changes to streamline getKey calls * More updates * - Addressed review comments. * - Fixing tests that expect certain localized strings that have changed. --------- Co-authored-by: Paul Medynski <31868385+paulmedynski@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Ports the fix for encryption key cache design in Azure Key Vault (AKV) provider to the 6.1 release branch, ensuring thread-safe caching and more precise error messages.
- Updated manual tests to assert quoted parameter names in exception messages.
- Introduced
SemaphoreSlimlocks around symmetric key and AKV provider caches for concurrency safety. - Refactored code to use C# 12 collection expressions and improved exception resource structures.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/ExceptionTestAKVStore.cs | Refined asserted exception messages to quote parameter names. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSymmetricKeyCache.cs | Wrapped cache access in a semaphore lock and simplified object initializers. |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs | Updated comments and set zero TTL on AKV key store providers. |
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs | Applied C# 12 collection expressions and updated CEK cache comments. |
| src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Utils.cs | Switched exception templates and added a GetKeyFailed helper. |
| src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Strings.resx | Updated resource entries for consistent quoting and added GetKeyFailed. |
| src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/SqlColumnEncryptionAzureKeyVaultProvider.cs | Introduced locking around local key cache, refactored ToHexString, and simplified fetch logic. |
| src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/LocalCache.cs | Rearranged using directives and added doc comment for cache inspection in tests. |
| src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureSqlKeyCryptographer.cs | Made class sealed, implemented IDisposable, and added semaphore-based thread safety. |
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Strings.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/SqlColumnEncryptionAzureKeyVaultProvider.cs:183
- The exception only includes the key name; consider passing the full key identifier URI (
masterKeyPath) toGetKeyFailedso consumers have full context of which vault path failed.
/// <returns>Plain text column encryption key</returns>
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/LocalCache.cs:95
- You've documented the use of this method in unit tests—please add or update unit tests to explicitly cover
LocalCache.ContainsKeyand ensure the caching behavior works as intended.
/// Used in unit tests to verify that the cache contains the expected entries.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSymmetricKeyCache.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs
Show resolved
Hide resolved
paulmedynski
approved these changes
Jul 15, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/6.1 #3477 +/- ##
===============================================
- Coverage 68.87% 64.71% -4.16%
===============================================
Files 280 280
Lines 62322 62355 +33
===============================================
- Hits 42927 40356 -2571
- Misses 19395 21999 +2604
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Ports #3464 to release/6.1