Perf | Reduce allocations in Always Encrypted key handling, reorganise#4256
Open
edwardneal wants to merge 24 commits intodotnet:mainfrom
Open
Perf | Reduce allocations in Always Encrypted key handling, reorganise#4256edwardneal wants to merge 24 commits intodotnet:mainfrom
edwardneal wants to merge 24 commits intodotnet:mainfrom
Conversation
The underlying MemoryCache is thread-safe at the point of retrieval. Add a check outside of the lock to avoid contending it where possible.
The encryption key and the algorithm are tightly coupled; we don't need to specify this explicitly.
Salt strings are always constant.
Lazy-load these - these values will not change.
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4256 +/- ##
==========================================
- Coverage 66.02% 64.30% -1.72%
==========================================
Files 277 272 -5
Lines 42988 65786 +22798
==========================================
+ Hits 28382 42304 +13922
- Misses 14606 23482 +8876
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:
|
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
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
This reduces the number of memory allocations performed by Always Encrypted functionality, focusing primarily upon the encryption keys.
It also introduces nullability annotations to the touched files, makes some minor documentation improvements, removes the redundant
SqlClientorSqlprefixes ofinternaltypes and moves them into the AlwaysEncrypted namespace. These don't result in any public API changes or any changes to the calling contract forSqlColumnEncryptionKeyStoreProvider.The PR's diff appears larger than it truly is; it may be easier to review commit-by-commit.
In sequence:
ColumnEncryptionKeyCacheTtlwas being set to a newTimeSpaninstance rather thanTimeSpan.Zero.SqlColumnEncryptionKeyStoreProvider.DecryptColumnEncryptionKey. We retain the lock in order to preserve the guarantee that this method call will always be serialized.SqlAeadAes256CbcHmac256Algorithmwas being passed the constant algorithm name in the constructor. These two components are tightly coupled, so I removed the redundant parameter name. This had knock-on effects - it meant that thestring.Formatcould be replaced with a constant string, and that the Unicode encoding of this could be cached.SymmetricKeyallocations were used to store the IV, MAC and cryptographic keys. These acted as a lightweight wrapper, providing no defensive copies or immutability guarantees. We now just store the byte arrays directly.SqlSecurityUtility.GetHMACWithSHA256used the netfx cryptographic methods, then allocated and copied. On netcore, we can use the oneshots and eliminate all allocations (and in some circumstances, the copy.)The microbenchmark highlights the impact when instantiating a
AeadAes256CbcHmac256EncryptionKeyunder cold cache conditions: a measurable reduction in GC pressure and a modest throughput improvement.Security considerations
This makes changes which are very close to the core
SqlAeadAes256CbcHmac256Algorithmimplementation. I've deliberately scoped this PR to exclude any changes to this. While I think there are other performance improvements to be made in that area, they don't stand alone in the same way that these do and need their own discussion/review.The CEK derivation logic remains unchanged, as does the key validation logic and key usage ordering.
Issues
None.
Testing
Existing automated tests continue to pass. This doesn't introduce any new functionality or make any behavioural changes, so I've not added any new tests.