-
Notifications
You must be signed in to change notification settings - Fork 314
Performance | Introduce lower-allocation AlwaysEncrypted primitives #3554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Performance | Introduce lower-allocation AlwaysEncrypted primitives #3554
Conversation
providerName.AsSpan().ToLowerInvariant(masterKeyMetadataSpan); | ||
masterKeyPath.AsSpan().ToLowerInvariant(masterKeyMetadataSpan.Slice(providerName.Length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the characters in the input variable restricted such that it is certain that the output lower invariant replacement version is certain to be of exactly the same length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indirectly. This is due to be used by the certificate store provider, and the constructor takes an RSA
instance. The only way to get this instance is by using the master key path to select a valid certificate, and the only valid master key paths are ASCII text in a few formats:
[LocalMachine|CurrentUser]/My/[40 character SHA1 thumbprint]
My/[40 character SHA1 thumbprint]
[40 character SHA1 thumbprint]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent. It might be worth leaving a comment in the code somewhere so anyone who reads the code in future knows that there are input restrictions.
Add a comment and a debug assertion to make it clear that the invariant lowercase value of masterKeyPath should be the same length as the original string.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consolidation looks good overall, with some questions/suggestions.
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/ColumnMasterKeyMetadata.cs
Outdated
Show resolved
Hide resolved
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/ColumnMasterKeyMetadata.cs
Outdated
Show resolved
Hide resolved
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/ColumnMasterKeyMetadata.cs
Show resolved
Hide resolved
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/ColumnMasterKeyMetadata.cs
Show resolved
Hide resolved
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/ColumnMasterKeyMetadata.cs
Show resolved
Hide resolved
...lient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/EncryptedColumnEncryptionKeyParameters.cs
Show resolved
Hide resolved
...lient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/EncryptedColumnEncryptionKeyParameters.cs
Outdated
Show resolved
Hide resolved
...lient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/EncryptedColumnEncryptionKeyParameters.cs
Outdated
Show resolved
Hide resolved
...lient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/EncryptedColumnEncryptionKeyParameters.cs
Show resolved
Hide resolved
...lient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/EncryptedColumnEncryptionKeyParameters.cs
Outdated
Show resolved
Hide resolved
Add/updated XML documentation to the public APIs. Add intermediary variable containing the signature size for clarity. Use AlgorithmOffset rather than hardcoded offset in EncryptedColumnEncryptionKeyParameters. Specifically define using block in ColumnMasterKeyMetadata.
class -> struct
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the previous updates. Just a couple more and one question about disposal.
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/ColumnMasterKeyMetadata.cs
Show resolved
Hide resolved
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/ColumnMasterKeyMetadata.cs
Show resolved
Hide resolved
...lient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/EncryptedColumnEncryptionKeyParameters.cs
Show resolved
Hide resolved
...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AlwaysEncrypted/ColumnMasterKeyMetadata.cs
Show resolved
Hide resolved
Thanks. Responded, and merged following the merge of #3014. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! RSA.SignHash() will throw CryptographicException if "An error occurred creating the signature.", so need to document that one as well.
I'd missed that - added to |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Description
I'm trying to improve query performance in AE scenarios, and part of this means a review of the column encryption providers. These were written prior to a number of cryptography improvements in .NET 5.0; as a result of these improvements, we can save a little memory.
Besides the performance improvements, each provider reproduces the same serialization logic. These primitives centralise that logic; they use the column master key parameters to perform encryption and decryption of a column encryption key with an RSA instance. The only thing that the three
SqlColumnEncryptionKeyStoreProvider
derivatives need to do is retrieve an RSA instance using the master key path.I considered moving this logic into a protected method on
SqlColumnEncryptionKeyStoreProvider
, but a third-party provider isn't guaranteed to perform encryption/decryption of the CEK using an RSA instance (or indeed, on the same server -SqlColumnEncryptionAzureKeyVaultProvider
passes that duty to Azure Key Vault.)A subsequent PR will refactor the column encryption providers to use these primitives, and the benchmark results for this are below. To summarise: no significant variations in execution time, and around a 15% reduction in AlwaysEncrypted memory usage.
Benchmarks
SqlColumnEncryptionCertificateStoreProvider
SqlColumnEncryptionCngProvider
SqlColumnEncryptionCspProvider
Testing
These primitives aren't used anywhere yet, so there's no test coverage. The existing providers have this coverage though, so the new code will gain coverage from the follow-up PR. I have a local branch where the column encryption providers use this code, and almost all tests pass. The two failing tests are capitalisation errors in exception messages produced by the CSP provider, which will be fixed at the same time.