[release/2.2] Added AAD Authentication using Access Token #31039

AfsanehR-zz · 2018-07-12T22:23:44Z

Fixes #13660
2.2 Servicing:

Description:
The desktop .NET Framework 4.6 and newer has an AccessToken property on the SqlConnection class which can be used to authenticate to an Azure SQL database using an access token issued by Azure AD. However, this property is not present on the version of SqlConnection provided in the System.Data.SqlClient NuGet package of CoreFX. The lack of the AccessToken property makes it difficult, if not impossible, to support modern directory-based auth scenarios for Azure SQL DB while also targeting .NET Standard.

Customer Impact:
The lack of a way to connect to an Azure SQL database in CoreFx keeps existing Azure SQL customers from adopting CoreFX. It also precludes customers who already use CoreFX from subscribing to Azure SQL services. This small change will enable customers to move forward with a CoreFx + Azure SQL combination.

The code is already in the master branch of the corefx repo.

Regression?
None. These changes are ported over from .NET Framework to .NET Core.

Risk:
Low. The code changes already existed on .NET Framework.

The same pr#30342 is already merged on master branch.

removed unused parts in code removed unnecessary code fix a comment Review feedbacks from Stephen Toub + Added a verification test addressed review feedbacks use Debug.Fail instead of Debug.Assert

keeratsingh

Approving as this is a port of #30342 on Release 2.2 branch

Wraith2 · 2018-07-13T19:59:52Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs

@@ -34,6 +34,7 @@ public sealed partial class SqlConnection : DbConnection, ICloneable
        private SqlCredential _credential;
        private string _connectionString;
        private int _connectRetryCount;
+        private string _accessToken; // Access Token to be used for token based authententication


typo in comment, authentication

Wraith2 · 2018-07-13T20:04:00Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs

+                // If a connection is connecting or is ever opened, AccessToken cannot be set
+                if (!InnerConnection.AllowSetConnectionString)
+                {
+                    throw ADP.OpenConnectionPropertySet("AccessToken", InnerConnection.State);


possible nameof(AccessToken) ?

AfsanehR-zz · 2018-07-18T20:00:14Z

@Petermarcu @stephentoub @danmosemsft Could you please review this pr? This is the same as #30342 targeting to 2.2 Release branch. I have modified the code changes @Wraith2 has suggested, will update the PR once I receive your feedback. Thanks!

stephentoub · 2018-07-19T05:45:04Z

src/System.Data.SqlClient/src/System/Data/Common/AdapterUtil.SqlClient.cs

+        {
+            return ADP.InvalidOperation(SR.GetString(SR.ADP_InvalidMixedUsageOfAccessTokenAndUserIDPassword));
+        }
+        static internal Exception InvalidMixedUsageOfCredentialAndAccessToken()


Nit: it'd be good to consistently use "internal static" rather than sometimes using "static internal"

stephentoub · 2018-07-19T05:46:58Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnectionPoolKey.cs

+            {
+                return _accessToken;
+            }
+        }


This can just be:

internal string AccessToken => _accessToken;

similar to the property right above it.

stephentoub · 2018-07-19T05:52:24Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnectionPoolKey.cs

@@ -74,6 +85,13 @@ private void CalculateHashCode()
                    _hashValue = _hashValue * 17 + _credential.GetHashCode();
                }
            }
+            else if (_accessToken != null)
+            {
+                unchecked


Why is the unchecked needed? Is checked arithmetic enabled for this project?

I can see unchecked being used in other parts of the SqlClient project.

danmoseley · 2018-07-24T16:02:19Z

@afsanehr let's ask @Petermarcu what process he wants here when he returns tomorrow.

…nto AccessToken2.2

…ccessToken2.2 # Conflicts: # pkg/Microsoft.NETCore.Platforms/Microsoft.NETCore.Platforms.pkgproj # src/packages.builds

AfsanehR-zz · 2018-08-14T22:53:17Z

@dotnet-bot Test OSX x64 Debug Build please

AfsanehR-zz · 2018-08-15T15:10:43Z

@dotnet-bot Test OSX x64 Debug Build

AfsanehR-zz · 2018-08-15T16:36:14Z

@danmosemsft Just confirming @Petermarcu approved merging this pr.

danmoseley · 2018-08-15T20:44:13Z

@afsanehr @keeratsingh I see this adds API (addition to ref*cs). I don't believe we can do that in servicing safely. Was this already discussed? If not, this shoudl go in 2.2 or 3.0.

Also if https://github.com/dotnet/corefx/issues/31082 depends on this the same would be true for that.

danmoseley · 2018-08-22T17:43:19Z

This is in the 2.2 branch but we did not have the package authoring correct to get it in the impending preview 1. We can follow up to get it in preview 2.

@weshaggard

weshaggard · 2018-08-22T19:40:37Z

@afsanehr yes given that you are adding new APIs you should bump the minor version of the package as well as the assembly. You will also likely need to bump master to a higher version as well. You will also need to add the SqlClient package to the list of packages to build similar to #31891

steji113 · 2018-11-30T05:36:14Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnection.cs

@@ -1403,7 +1471,7 @@ public static void ChangePassword(string connectionString, SqlCredential credent
                throw ADP.InvalidArgumentLength(nameof(newSecurePassword), TdsEnums.MAXLEN_NEWPASSWORD);
            }

-            SqlConnectionPoolKey key = new SqlConnectionPoolKey(connectionString, credential);
+            SqlConnectionPoolKey key = new SqlConnectionPoolKey(connectionString, credential: null, accessToken: null);


Was changing credential to null here expected? There is another similar spot below on line 1512 as well.

/cc: @danmosemsft / @stephentoub for visibility

@afsanehr @keeratsingh own this

Line 1512 has a bug and credential shouldn't be passed in as null, instead of the credential parameter should be passed through.

Also this line, i.e. 1474, its buggy. No null should be passed in for Credential

Added AAD Authentication using Access Token passed as string

5beee34

removed unused parts in code removed unnecessary code fix a comment Review feedbacks from Stephen Toub + Added a verification test addressed review feedbacks use Debug.Fail instead of Debug.Assert

AfsanehR-zz added this to the 2.2 milestone Jul 12, 2018

AfsanehR-zz added the area-System.Data.SqlClient label Jul 12, 2018

AfsanehR-zz self-assigned this Jul 12, 2018

AfsanehR-zz requested review from keeratsingh and saurabh500 July 13, 2018 16:29

keeratsingh approved these changes Jul 13, 2018

View reviewed changes

keeratsingh requested review from stephentoub, danmoseley and Petermarcu July 13, 2018 17:46

Wraith2 reviewed Jul 13, 2018

View reviewed changes

Wraith2 approved these changes Jul 18, 2018

View reviewed changes

stephentoub reviewed Jul 19, 2018

View reviewed changes

AfsanehR-zz changed the base branch from release/2.2 to release/2.1 August 14, 2018 20:35

AfsanehR-zz added 4 commits August 14, 2018 13:45

Merge branch 'release/2.1' into AccessToken2.2

a71baab

Adressed review comments

5bac7bb

Merge branch 'AccessToken2.2' of https://github.com/v-afrafi/corefx i…

fb6429b

…nto AccessToken2.2

Merge branch 'release/2.1' of https://github.com/dotnet/corefx into A…

c71cd8e

…ccessToken2.2 # Conflicts: # pkg/Microsoft.NETCore.Platforms/Microsoft.NETCore.Platforms.pkgproj # src/packages.builds

AfsanehR-zz modified the milestones: 2.2, 2.1.x Aug 14, 2018

AfsanehR-zz added the Servicing-consider Issue for next servicing release review label Aug 15, 2018

AfsanehR-zz changed the base branch from release/2.1 to release/2.2 August 16, 2018 16:36

AfsanehR-zz modified the milestones: 2.1.x, 2.2 Aug 16, 2018

keeratsingh pushed a commit to keeratsingh/corefx that referenced this pull request Aug 16, 2018

Removed dependencies on dotnet#31039

a792fb2

karelz changed the title ~~Added AAD Authentication using Access Token~~ [release/2.2] Added AAD Authentication using Access Token Aug 16, 2018

keeratsingh merged commit 1ae377f into dotnet:release/2.2 Aug 17, 2018

danmoseley removed the Servicing-consider Issue for next servicing release review label Aug 21, 2018

steji113 reviewed Nov 30, 2018

View reviewed changes

AfsanehR-zz mentioned this pull request Feb 22, 2019

Fix passing null for credentials in SqlConnectionPoolKey #34260

Merged

wtgodbe mentioned this pull request Feb 22, 2019

Bump version for SqlClient & turn on package build #35525

Merged

danmoseley mentioned this pull request Jan 31, 2020

Include SqlClient update in 2.2 packaging dotnet/runtime#27198

Closed

stijnherreman mentioned this pull request Feb 1, 2020

Possible copy-paste mistake in SqlConnection introduced by AAD Authentication work dotnet/runtime#28254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/2.2] Added AAD Authentication using Access Token #31039

[release/2.2] Added AAD Authentication using Access Token #31039

AfsanehR-zz commented Jul 12, 2018 •

edited

Loading

keeratsingh left a comment

Wraith2 Jul 13, 2018

Wraith2 Jul 13, 2018

AfsanehR-zz commented Jul 18, 2018

stephentoub Jul 19, 2018

AfsanehR-zz Aug 14, 2018

stephentoub Jul 19, 2018

AfsanehR-zz Aug 14, 2018

stephentoub Jul 19, 2018

AfsanehR-zz Aug 14, 2018

danmoseley commented Jul 24, 2018 •

edited

Loading

AfsanehR-zz commented Aug 14, 2018 •

edited

Loading

AfsanehR-zz commented Aug 15, 2018

AfsanehR-zz commented Aug 15, 2018

danmoseley commented Aug 15, 2018

danmoseley commented Aug 22, 2018

weshaggard commented Aug 22, 2018

steji113 Nov 30, 2018

steji113 Nov 30, 2018

danmoseley Nov 30, 2018

saurabh500 Dec 21, 2018

saurabh500 Dec 21, 2018

[release/2.2] Added AAD Authentication using Access Token #31039

[release/2.2] Added AAD Authentication using Access Token #31039

Conversation

AfsanehR-zz commented Jul 12, 2018 • edited Loading

keeratsingh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AfsanehR-zz commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Jul 24, 2018 • edited Loading

AfsanehR-zz commented Aug 14, 2018 • edited Loading

AfsanehR-zz commented Aug 15, 2018

AfsanehR-zz commented Aug 15, 2018

danmoseley commented Aug 15, 2018

danmoseley commented Aug 22, 2018

weshaggard commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AfsanehR-zz commented Jul 12, 2018 •

edited

Loading

danmoseley commented Jul 24, 2018 •

edited

Loading

AfsanehR-zz commented Aug 14, 2018 •

edited

Loading