Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature | AKV Provider Upgrade to use new Azure key Vault libraries #630

Merged
merged 25 commits into from
Feb 26, 2021
Merged

Feature | AKV Provider Upgrade to use new Azure key Vault libraries #630

merged 25 commits into from
Feb 26, 2021

Conversation

cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Jun 30, 2020 •
edited
Loading

Addresses #294

Changes full design and introduces new constructors to accept implementation of TokenCredential to support new libraries.

Breaking Changes:

  • Drops support for .NET Framework 4.6, supports .NET Framework 4.6.1+
  • Drops support for old constructors that accepted "AuthenticationCallback".

cc @Xtrimmer

[6 Aug, 2020] Update:
The old constructors are now removed totally and driver does not implement/support authentication callback in new design now. For user applications willing to support multi-user credentials for acquiring tokens, can implement a custom TokenCredential class with their own logic in "GetToken" and "GetTokenAsync" APIs. An example has been added (doc/samples/AzureKeyVaultProviderLegacyExample_2_0.cs) to the repository to demonstrate the same, will be published to Microsoft Docs.

@cheenamalhotra cheenamalhotra added 🆕 Public API Issues/PRs that introduce new APIs to the driver. 🔨 Breaking Change Issues/PRs that are related with breaking API changes in the driver. labels Jul 7, 2020
@cheenamalhotra cheenamalhotra linked an issue Jul 7, 2020 that may be closed by this pull request
AzureKeyVaultProvider should move to the latest Azure KeyVault Libraries #294
Closed
Xtrimmer
Xtrimmer reviewed Jul 7, 2020
View reviewed changes
...crosoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureKeyVaultProviderTokenCredential.cs Outdated Show resolved Hide resolved
...crosoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureKeyVaultProviderTokenCredential.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureSqlKeyCryptographer.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVUnitTests.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureSqlKeyCryptographer.cs Show resolved Hide resolved
...oft.Data.SqlClient/add-ons/AzureKeyVaultProvider/SqlColumnEncryptionAzureKeyVaultProvider.cs Outdated Show resolved Hide resolved
...oft.Data.SqlClient/add-ons/AzureKeyVaultProvider/SqlColumnEncryptionAzureKeyVaultProvider.cs Outdated Show resolved Hide resolved
...oft.Data.SqlClient/add-ons/AzureKeyVaultProvider/SqlColumnEncryptionAzureKeyVaultProvider.cs Outdated Show resolved Hide resolved
...oft.Data.SqlClient/add-ons/AzureKeyVaultProvider/SqlColumnEncryptionAzureKeyVaultProvider.cs Show resolved Hide resolved
heaths
heaths reviewed Jul 9, 2020
View reviewed changes
Copy link

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only reviewed the production code using Key Vault packages directly and overall it looks okay, but I'm curious why it's all implemented synchronously? Where necessary, you're calling async over sync (and in some places, in ways that can deadlock) but wouldn't it make for a more efficient package for callers to use async?

...crosoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureKeyVaultProviderTokenCredential.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureSqlKeyCryptographer.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureSqlKeyCryptographer.cs Outdated Show resolved Hide resolved
...crosoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureKeyVaultProviderTokenCredential.cs Outdated Show resolved Hide resolved
...crosoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureKeyVaultProviderTokenCredential.cs Outdated Show resolved Hide resolved
...crosoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureKeyVaultProviderTokenCredential.cs Outdated Show resolved Hide resolved
...crosoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureKeyVaultProviderTokenCredential.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureSqlKeyCryptographer.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureSqlKeyCryptographer.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/AzureSqlKeyCryptographer.cs Outdated Show resolved Hide resolved
@cheenamalhotra
Copy link
Member Author

I believe the reason of synchronous methods only is the class SqlColumnEncryptionKeyStoreProvider.
I think it would be possible to extend this to also support counter Async APIs, @Xtrimmer any thoughts?

@cheenamalhotra
Merge branch 'master' into akv-upgrade
33ab71d 
# Conflicts:
#	src/Microsoft.Data.SqlClient/add-ons/Directory.Build.props
#	src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/Setup/ColumnMasterKey.cs
#	src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
@cheenamalhotra
Merge branch 'master' into akv-upgrade
aea83c6
@cheenamalhotra cheenamalhotra added the Area\AKV Provider Issues that are targeted for AKV Provider codebase. label Oct 13, 2020
cheenamalhotra and others added 3 commits February 26, 2021 13:53
@cheenamalhotra @David-Engel
Apply suggestions from code review
68853b8 
Co-authored-by: Javad <v-jarahn@microsoft.com>
Co-authored-by: David Engel <dengel@magnitude.com>
@cheenamalhotra
Apply Suggestions from Code Review
405e564
@cheenamalhotra
Merge branch 'akv-upgrade' of https://github.com/cheenamalhotra/SqlCl…
34af2f7 
…ient into akv-upgrade
@cheenamalhotra cheenamalhotra merged commit c2b4df7 into dotnet:master Feb 26, 2021
@johnnypham johnnypham mentioned this pull request Mar 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heaths heaths heaths left review comments

@JRahnama JRahnama JRahnama approved these changes

@johnnypham johnnypham johnnypham approved these changes

@Xtrimmer Xtrimmer Xtrimmer approved these changes

@karinazhou karinazhou Awaiting requested review from karinazhou

@DavoudEshtehari DavoudEshtehari Awaiting requested review from DavoudEshtehari

@David-Engel David-Engel Awaiting requested review from David-Engel

Assignees
No one assigned
Labels
Area\AKV Provider Issues that are targeted for AKV Provider codebase. 🔨 Breaking Change Issues/PRs that are related with breaking API changes in the driver. 🆕 Public API Issues/PRs that introduce new APIs to the driver.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AzureKeyVaultProvider should move to the latest Azure KeyVault Libraries
6 participants
@cheenamalhotra @heaths @Xtrimmer @johnnypham @David-Engel @JRahnama