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.

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

Always Encrypted Usability Modifications #902

Merged
merged 5 commits into from
May 15, 2019
Merged

Conversation

ccdigix
Copy link
Contributor

@ccdigix ccdigix commented Dec 5, 2018

As previously discussed on a call with Microsoft (E-Yen Tan, Cheena Malhotra, Raghav Kaushik, and Jakub Szymaszek).

Changes to allow easier access when setting the Azure Key Vault provider's client ID and key for Always Encrypted functionality. This now allows these fields to be set via the connection string URL (not recommended) or via the driver's connection properties (recommended). This is required to use Always Encrypted functionality within data processing solutions that initiate the driver for you such as Apache Spark clusters or third-party tools where you cannot programmatically call the static 'registerColumnEncryptionKeyStoreProviders' method on the SQLServerConnection class.

NOTE: this pull request is merely an indicator of how this could be addressed and probably needs refining before merging (e.g. whilst manually proven there are no automated tests associated with this).

…der's client ID and key for Always Encrypted functionality. This now allows these fields to be set via the connection string URL (not recommended) or via the driver's connection properties (recommended). This is required to use Always Encrypted functionality within data processing solutions that initiate the driver for you such as Apache Spark clusters or third-party tools where you cannot programmatically call the static 'registerColumnEncryptionKeyStoreProviders' method on the SQLServerConnection class.
@msftclas
Copy link

msftclas commented Dec 5, 2018

CLA assistant check
All CLA requirements met.

@codecov-io
Copy link

codecov-io commented Dec 5, 2018

Codecov Report

Merging #902 into dev will decrease coverage by 3.28%.
The diff coverage is 37.03%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #902      +/-   ##
============================================
- Coverage     53.41%   50.12%   -3.29%     
+ Complexity     3334     2881     -453     
============================================
  Files           119      120       +1     
  Lines         27988    28014      +26     
  Branches       4679     4680       +1     
============================================
- Hits          14950    14043     -907     
- Misses        11001    11711     +710     
- Partials       2037     2260     +223
Flag Coverage Δ Complexity Δ
#JDBC42 49.7% <37.03%> (?) 2842 <0> (?)
#JDBC43 50.08% <37.03%> (?) 2879 <0> (?)
Impacted Files Coverage Δ Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerResource.java 100% <ø> (ø) 4 <0> (ø) ⬇️
.../microsoft/sqlserver/jdbc/SQLServerDataSource.java 45.67% <0%> (-3.11%) 67 <0> (-11)
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java 83.58% <100%> (+0.43%) 36 <0> (ø) ⬇️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 46.45% <15.38%> (-2.7%) 344 <0> (-60)
...osoft/sqlserver/jdbc/dataclassification/Label.java 0% <0%> (-100%) 0% <0%> (-3%)
...ver/jdbc/dataclassification/ColumnSensitivity.java 0% <0%> (-100%) 0% <0%> (-2%)
...erver/jdbc/dataclassification/InformationType.java 0% <0%> (-100%) 0% <0%> (-3%)
...r/jdbc/dataclassification/SensitivityProperty.java 0% <0%> (-100%) 0% <0%> (-3%)
.../dataclassification/SensitivityClassification.java 0% <0%> (-75%) 0% <0%> (-2%)
.../com/microsoft/sqlserver/jdbc/SQLServerJdbc43.java 25% <0%> (-50%) 0% <0%> (ø)
... and 44 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3242324...9dafad9. Read the comment docs.

@cheenamalhotra
Copy link
Member

Hi @ccdigix

Thank you for coming up with the PR, we will review it and come up with improvements soon.

@ccdigix
Copy link
Contributor Author

ccdigix commented Dec 8, 2018

Thanks @cheenamalhotra - if you need me to amend the pull request in any way then just let me know.

@cheenamalhotra cheenamalhotra self-assigned this Mar 18, 2019
@cheenamalhotra
Copy link
Member

Hi @ccdigix

I came up with PR ccdigix/dev#1 to address my comments and also add support for properties to be set by SQLServerDataSource.

I also reverted back the change to remove the check in registerColumnEncryptionKeyStoreProviders method since its a Public API of SQLServerConnection class to keep the behavior consistent.

With my changes, we would only read and use connection properties if the globalCustomColumnEncryptionKeyStoreProviders is not already set which aligns with driver's existing behavior.

Would request you to review and test the changes and let me know for any concerns.

@ccdigix
Copy link
Contributor Author

ccdigix commented Mar 19, 2019

Hi @cheenamalhotra - made a few comments above about PR ccdigix/dev#1 but overall it looks good - how do you want to move it forward? Do you need me to merge it fully or are you happy to apply it now to the official Microsoft branch?

Proposed changes for property names and functionalities
@ccdigix
Copy link
Contributor Author

ccdigix commented Mar 26, 2019

Pull request updated and all ready for you @cheenamalhotra

@cheenamalhotra cheenamalhotra merged commit 6697d1b into microsoft:dev May 15, 2019
@cheenamalhotra cheenamalhotra added the Public API Changes in Public API label May 19, 2019
manikandanramaswami added a commit to manikandanramaswami/sql-docs that referenced this pull request Jan 4, 2020
I have updated the documentation based on Pull request in JDBC driver project (microsoft/mssql-jdbc#902) and tested the feature in JDBC Driver 7.4.1 (latest Stable release).
manikandanramaswami added a commit to manikandanramaswami/sql-docs that referenced this pull request Jan 4, 2020
Added newly introduced Connection String Keywords (keyVaultProviderClientId, keyVaultProviderClientKey) in Microsoft JDBC Driver 7.4 version to enable use of Azure Key Vault Provider
Based on MS JDBC Pull Request (microsoft/mssql-jdbc#902) and my testing of its working
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Public API Changes in Public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants