Cleanup | Always Encrypted cryptographic algorithm factories#4138
Open
edwardneal wants to merge 4 commits intodotnet:mainfrom
Open
Cleanup | Always Encrypted cryptographic algorithm factories#4138edwardneal wants to merge 4 commits intodotnet:mainfrom
edwardneal wants to merge 4 commits intodotnet:mainfrom
Conversation
This implemented theoretical support for allowing clients to register their own cryptographic algorithms. This was never used, so has been replaced with a static mapping. Placed its replacement into the AlwaysEncrypted namespace.
Moved into the AlwaysEncrypted namespace, enabled nullability annotations, renamed
This includes: * Rename to AeadAes256CbcHmac256Factory. * Enable nullability annotations. * Whitespace/comment changes. * Move the version constant into the algorithm itself (and out of the factory.) * Minor improvement to StringBuilder concatenation.
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
1 participant
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 PR continues some Always Encrypted cleanup and modernization work. It moves more of the internal AE-related classes into the new AlwaysEncrypted namespace, tidying them up as the PR proceeds.
We moved and modernized the public providers which decrypt/encrypt the column master keys in v7.0 GA, and now we proceed to the next stage: the factories which convert a cryptographic algorithm's name to its implementation.
Classes in question:
SqlClientEncryptionAlgorithmFactoryList: after being moved to the new namespace, I've simplified the name toEncryptionAlgorithmFactoryList. This class was slightly more complex than it needed to be: it allowed the provider to register its own cryptographic algorithm into aConcurrentDictionary. This registration process was only ever used to build a static list of one entry, and was never exposed. I've thus replaced it with a simpleswitchblock.SqlClientEncryptionAlgorithmFactory: renamed toEncryptionAlgorithmFactory. No changes beyond comments, this was only ever a base class.SqlAeadAes256CbcHmac256Factory: renamed toAeadAes256CbcHmac256Factory. These are largely just style, comment and whitespace changes. I've also sealed the class and moved the floatingAlgorithmVersionconstant into the actual algorithm's class.I'm keeping the column encryption key handling in a separate PR so that any security review can be done on its own. I think these can provide a measurable improvement to Always Encrypted's memory usage.
Issues
None.
Testing
There were a few tests which referred to this using reflection; I've caught those, and they continue to pass. We've got reasonable test coverage for Always Encrypted so I've not added anything else.