Managed Identity dev experience improvements #1936

tkyc · 2022-10-18T22:17:05Z

PR Resolves the following:

Deprecated the following:

Managed Identity token cache #1825

New authentication property values:

authentication=ActiveDirectoryManagedIdentity
~~authentication=DefaultAzureCredential~~ EDIT: Later changed to authentication=ActiveDirectoryDefault

New environment variables:

INTELLIJ_KEEPASS_PATH
ADDITIONALLY_ALLOWED_TENANTS (comma delimited list of additionally allowed tenant IDs, used with DefaultAzureCredential)

To use the IntellijCredential with the driver, supply the connection string property authentication=DefaultAzureCredential and set the INTELLIJ_KEEPASS_PATH environment variable to the path of the keepass database.

…mssql-jdbc into jdbc-msi-improvements

This reverts commit ecd0a2a.

David-Engel

There is one other change we need to make to align the JDBC driver with other drivers. We should deprecate the msiClientId get/set methods (but they should continue to work. Comment: "Use the getUser/setUser method instead.") This will include changing the Authentication=ActiveDirectoryMSI validation to allow specifying User in the connection string. If both msiClientId and User are specified, User should override msiClientId.

src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerConnection.java

src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerDataSource.java

src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerConnection.java

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDataSource.java

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerSecurityUtility.java

…user property for managed identity client ID

…mssql-jdbc into jdbc-msi-improvements

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDriver.java

src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerSecurityUtility.java

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java

lilgreenbird

please run formatter looks like a lot of these files aren't formatted

lilgreenbird · 2022-10-24T22:14:46Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerSecurityUtility.java

-        boolean isAzureFunction = null != identityEndpoint && !identityEndpoint.isEmpty() && null != identityHeader
-                && !identityHeader.isEmpty();
+            throw new SQLServerException(SQLServerException
+                    .getErrString("R_ManagedIdentityTokenAcquisitionFail"), null);


can we add more info to the error message to make it easier to debug when there's a failure

Similarly, do we have more info if the Optional task fails or doesn't return a result? I don't know the API well...

What additional info did you have in mind? I'll amend the error message to the following to be more clear as to the reason it is null.

Failed to acquire managed identity token. Request for the token succeeded, but no token was returned. The token is null.

Not sure. From the sounds of it, we don't have anything more available. The current iteration looks good.

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerSecurityUtility.java

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java

David-Engel

Partial review

src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerConnection.java

src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerDataSource.java

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDataSource.java

src/main/java/com/microsoft/sqlserver/jdbc/ISQLServerDataSource.java

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDataSource.java

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDriver.java

David-Engel · 2022-10-25T04:06:36Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerSecurityUtility.java

-        boolean isAzureFunction = null != identityEndpoint && !identityEndpoint.isEmpty() && null != identityHeader
-                && !identityHeader.isEmpty();
+            throw new SQLServerException(SQLServerException
+                    .getErrString("R_ManagedIdentityTokenAcquisitionFail"), null);


Similarly, do we have more info if the Optional task fails or doesn't return a result? I don't know the API well...

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerSecurityUtility.java

tkyc · 2022-10-25T17:27:32Z

@David-Engel For some reason there's no "reply" button for your comment. I'll just quote you and reply here.

Similarly, do we have more info if the Optional task fails or doesn't return a result? I don't know the API well...

If the Optional task fails, it will throw its own error. The resulting exception, since we're using the Azure Identity credentials, will be a CredentialUnavailableException.

A CredentialUnavailableException indicates the following:

Error in the provided information for the credential eg. wrong client ID of the user-assigned Managed Identity
Connection was dropped mid query for the token

Howerver, there isn't any more available information other than a null token if the Optional succeeds (eg. request for token went through but a null token was returned). In this case, as you suggested (since we shouldn't return null), we'll throw our own error.

Signed-off-by: Jeff Wasty <[email protected]>

…date (#2602) * Update build.gradle * Update pom.xml * Update pom.xml * Update pom.xml * Update pom.xml * Update pom.xml * Update pom.xml * Update pom.xml * ManagedIdentity I Signed-off-by: Jeff Wasty <[email protected]> * Updates for running tests with managed identity (#2416) Signed-off-by: Jeff Wasty <[email protected]> # Conflicts: # src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java # src/test/java/com/microsoft/sqlserver/jdbc/TestResource.java # src/test/java/com/microsoft/sqlserver/jdbc/TestUtils.java # src/test/java/com/microsoft/sqlserver/jdbc/connection/TimeoutTest.java # src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java # src/test/java/com/microsoft/sqlserver/jdbc/unit/statement/BatchExecutionTest.java * Delete mistakenly added files * Delete mistakenly added files * Revert "Delete mistakenly added files" This reverts commit 19abc22. * Revert "Delete mistakenly added files" This reverts commit 04f8c82. * Revert "Updates for running tests with managed identity (#2416)" This reverts commit b727a96. * Revert "ManagedIdentity I" This reverts commit 6d5c519. * Delete garbage * Fixed Managed Identity tests (#1935) Signed-off-by: Jeff Wasty <[email protected]> # Conflicts: # src/test/java/com/microsoft/sqlserver/jdbc/AlwaysEncrypted/MSITest.java * Managed Identity dev experience improvements #1936 Signed-off-by: Jeff Wasty <[email protected]> * Updates for running tests with managed identity #2416 Signed-off-by: Jeff Wasty <[email protected]> * cleanup Signed-off-by: Jeff Wasty <[email protected]> * Update tests for Kerberos Signed-off-by: Jeff Wasty <[email protected]> * Remove unused test * tsc=true for ae Signed-off-by: Jeff Wasty <[email protected]> * Add tags for testDefaultRetry * Remove unstable test Signed-off-by: Jeff Wasty <[email protected]> * Add throttling error message to tests (#2044) Signed-off-by: Jeff Wasty <[email protected]> * Updated fedauth tests to run on diff test server (#2062) Signed-off-by: Jeff Wasty <[email protected]> * Update fedauth tests to use cached tokens and retry if throttled (#2077) Signed-off-by: Jeff Wasty <[email protected]> * Add back missing import Signed-off-by: Jeff Wasty <[email protected]> * Resolve static issue with IOBuffer Signed-off-by: Jeff Wasty <[email protected]> * Add back a missing import * Fixed TestUtils * Remove mistakenly added code * Cleanup * Cleanup testutils * More cleanup * Add more time for 'testDefaultRetry' & Fix tests for testing encrypt options (#2215) * Update setAEConnectionString * Updated fedauth error tests (#2538) Signed-off-by: Jeff Wasty <[email protected]> # Conflicts: # src/test/java/com/microsoft/sqlserver/jdbc/fedauth/ErrorMessageTest.java # src/test/java/com/microsoft/sqlserver/jdbc/fedauth/FedauthCommon.java * Update fedAuth tests * Revert "Updated fedauth error tests (#2538)" This reverts commit dfb4b77. * Revert "Update fedAuth tests" This reverts commit da5c58b. * Added ActiveDirectoryServicePrincipalCertificate for tests * Add 'requireSecret' exclude tag for tests which require adding a secret to app registration * Version update * Add connection retry changes from 2513 to ensure existing timing tests pass * Adjust timings on tests as the fix requires code changes we should not incorporate for this release * Changelog * Update dependency versions --------- Signed-off-by: Jeff Wasty <[email protected]> Co-authored-by: lilgreenbird <[email protected]> Co-authored-by: Terry Chow <[email protected]>

tkyc and others added 9 commits October 14, 2022 15:02

New MSI logic with azure identity

3a4d42e

Added new MSI tests

311eb1a

Added DefaultAzureCredential support

f842261

Amended MSI exception msg

486c7d7

DefaultAzureCredential tests

a40bcf1

Removed deprecated APIs

ecd0a2a

Merge branch 'main' into jdbc-msi-improvements

0e1900c

Major version bump

1ede265

Merge branch 'jdbc-msi-improvements' of https://github.com/microsoft/…

fbde65e

…mssql-jdbc into jdbc-msi-improvements

tkyc requested review from David-Engel, Jeffery-Wasty and lilgreenbird October 19, 2022 16:08

lilgreenbird added this to the 11.3.0 milestone Oct 19, 2022

lilgreenbird changed the title ~~Jdbc msi improvements~~ Managed Identity dev experience improvements Oct 20, 2022

tkyc and others added 4 commits October 21, 2022 15:53

Revert "Removed deprecated APIs"

4e8eb65

This reverts commit ecd0a2a.

PR comment changes

7910329

Code review changes

664c9a9

Merge branch 'main' into jdbc-msi-improvements

203e96d

David-Engel reviewed Oct 24, 2022

View reviewed changes

tkyc added 2 commits October 24, 2022 14:47

Additional CR comment changes; Deprecated get/set msiClientId; Using …

c06e976

…user property for managed identity client ID

Merge branch 'jdbc-msi-improvements' of https://github.com/microsoft/…

bff8c3d

…mssql-jdbc into jdbc-msi-improvements

lilgreenbird reviewed Oct 24, 2022

View reviewed changes

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDriver.java Show resolved Hide resolved

lilgreenbird reviewed Oct 24, 2022

View reviewed changes

src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java Outdated Show resolved Hide resolved

lilgreenbird reviewed Oct 24, 2022

View reviewed changes

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerSecurityUtility.java Outdated Show resolved Hide resolved

lilgreenbird reviewed Oct 24, 2022

View reviewed changes

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java Show resolved Hide resolved

lilgreenbird suggested changes Oct 24, 2022

View reviewed changes

lilgreenbird reviewed Oct 24, 2022

View reviewed changes

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerSecurityUtility.java Outdated Show resolved Hide resolved

lilgreenbird reviewed Oct 24, 2022

View reviewed changes

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java Outdated Show resolved Hide resolved

David-Engel reviewed Oct 24, 2022

View reviewed changes

David-Engel reviewed Oct 25, 2022

View reviewed changes

Updated tests

ff7218e

tkyc added 2 commits October 25, 2022 12:11

Additonal PR comments

1652f6c

Formatting changes

4b3a743

tkyc requested review from David-Engel and lilgreenbird October 25, 2022 19:37

David-Engel approved these changes Oct 25, 2022

View reviewed changes

Jeffery-Wasty approved these changes Oct 26, 2022

View reviewed changes

lilgreenbird approved these changes Oct 26, 2022

View reviewed changes

tkyc merged commit 095c7ee into main Oct 26, 2022

tkyc deleted the jdbc-msi-improvements branch October 26, 2022 21:24

scottaddie mentioned this pull request Apr 8, 2023

Add passwordless to Use Java and JDBC with Azure SQL Database MicrosoftDocs/sql-docs#8829

Closed

David-Engel mentioned this pull request Apr 11, 2023

Renamed driver DefaultAzureCredential authentication to ActiveDirectoryDefault #2055

Merged

Jeffery-Wasty added a commit that referenced this pull request Jan 28, 2025

Managed Identity dev experience improvements #1936

49cc04e

Signed-off-by: Jeff Wasty <[email protected]>

Jeffery-Wasty mentioned this pull request Feb 11, 2025

Update 11.2 servicing branch to run on test pipelines + dependency update #2602

Merged

Managed Identity dev experience improvements #1936

Managed Identity dev experience improvements #1936

Uh oh!

Conversation

tkyc commented Oct 18, 2022 • edited by David-Engel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

David-Engel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lilgreenbird left a comment

Choose a reason for hiding this comment

Uh oh!

lilgreenbird Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

David-Engel Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

tkyc Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

David-Engel Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

David-Engel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

David-Engel Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tkyc commented Oct 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tkyc commented Oct 18, 2022 •

edited by David-Engel

Loading