Code review changes

edwardneal · edwardneal · commit 7eef910c8b8f · 2025-02-24T07:34:05.000Z
* Reorder properties and constructors
* Move AEConnectionStringProviderWithCspParameters to its own file
* Tweak to the AKV token acquisition
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/AEConnectionStringProviderWithCspParameters.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/AEConnectionStringProviderWithCspParameters.cs
@@ -0,0 +1,46 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Security.Cryptography;
+
+namespace Microsoft.Data.SqlClient.ManualTesting.Tests.AlwaysEncrypted
+{
+    public class AEConnectionStringProviderWithCspParameters : IEnumerable<object[]>
+    {
+        public IEnumerator<object[]> GetEnumerator()
+        {
+            const string ProvidersRegistryKeyPath = @"SOFTWARE\Microsoft\Cryptography\Defaults\Provider";
+            using Microsoft.Win32.RegistryKey defaultCryptoProvidersRegistryKey = Microsoft.Win32.Registry.LocalMachine.OpenSubKey(ProvidersRegistryKeyPath);
+
+            foreach (string subKeyName in defaultCryptoProvidersRegistryKey.GetSubKeyNames())
+            {
+                CspParameters providerCspParameters;
+
+                // NOTE: RSACryptoServiceProvider.SignData() fails for other providers when testing locally
+                if (!subKeyName.Contains(@"RSA and AES"))
+                {
+                    continue;
+                }
+
+                using (Microsoft.Win32.RegistryKey providerKey = defaultCryptoProvidersRegistryKey.OpenSubKey(subKeyName))
+                {
+                    // Get Provider Name and its type
+                    string providerName = providerKey.Name.Substring(providerKey.Name.LastIndexOf(@"\", StringComparison.Ordinal) + 1);
+                    int providerType = (int)providerKey.GetValue(@"Type");
+
+                    providerCspParameters = new CspParameters(providerType, providerName);
+                }
+
+                foreach (string connStrAE in DataTestUtility.AEConnStrings)
+                {
+                    yield return new object[] { connStrAE, providerCspParameters };
+                }
+            }
+        }
+        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+    }
+}
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/AzureKeyVaultKeyFixture.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/AzureKeyVaultKeyFixture.cs
@@ -8,12 +8,12 @@ namespace Microsoft.Data.SqlClient.ManualTesting.Tests.AlwaysEncrypted
 {
     public sealed class AzureKeyVaultKeyFixture : AzureKeyVaultKeyFixtureBase
     {
-        public string GeneratedKeyUri { get; }
-
         public AzureKeyVaultKeyFixture()
             : base(DataTestUtility.AKVBaseUri, DataTestUtility.GetTokenCredential())
         {
             GeneratedKeyUri = CreateKey(nameof(GeneratedKeyUri), 2048).ToString();
         }
+
+        public string GeneratedKeyUri { get; }
     }
 }
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/DatabaseHelper.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/DatabaseHelper.cs
@@ -342,39 +342,4 @@ public IEnumerator<object[]> GetEnumerator()
         }
         IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
     }
-
-    public class AEConnectionStringProviderWithCspParameters : IEnumerable<object[]>
-    {
-        public IEnumerator<object[]> GetEnumerator()
-        {
-            const string ProvidersRegistryKeyPath = @"SOFTWARE\Microsoft\Cryptography\Defaults\Provider";
-            using Microsoft.Win32.RegistryKey defaultCryptoProvidersRegistryKey = Microsoft.Win32.Registry.LocalMachine.OpenSubKey(ProvidersRegistryKeyPath);
-
-            foreach (string subKeyName in defaultCryptoProvidersRegistryKey.GetSubKeyNames())
-            {
-                CspParameters providerCspParameters;
-
-                // NOTE: RSACryptoServiceProvider.SignData() fails for other providers when testing locally
-                if (!subKeyName.Contains(@"RSA and AES"))
-                {
-                    continue;
-                }
-
-                using (Microsoft.Win32.RegistryKey providerKey = defaultCryptoProvidersRegistryKey.OpenSubKey(subKeyName))
-                {
-                    // Get Provider Name and its type
-                    string providerName = providerKey.Name.Substring(providerKey.Name.LastIndexOf(@"\", StringComparison.Ordinal) + 1);
-                    int providerType = (int)providerKey.GetValue(@"Type");
-
-                    providerCspParameters = new CspParameters(providerType, providerName);
-                }
-
-                foreach (string connStrAE in DataTestUtility.AEConnStrings)
-                {
-                    yield return new object[] { connStrAE, providerCspParameters };
-                }
-            }
-        }
-        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
-    }
 }
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SQLSetupStrategy.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SQLSetupStrategy.cs
@@ -61,7 +61,7 @@ public class SQLSetupStrategy : ColumnMasterKeyCertificateFixture
         public SQLSetupStrategy()
             : base(true)
         {
-            ColumnMasterKeyPath = string.Concat(StoreLocation.CurrentUser.ToString(), "/", StoreName.My.ToString(), "/", ColumnMasterKeyCertificate.Thumbprint);
+            ColumnMasterKeyPath = string.Format("{0}/{1}/{2}", StoreLocation.CurrentUser, StoreName.My, ColumnMasterKeyCertificate.Thumbprint);
         }
 
         protected SQLSetupStrategy(string customKeyPath)
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SQLSetupStrategyCspProvider.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/SQLSetupStrategyCspProvider.cs
@@ -15,8 +15,6 @@ public class SQLSetupStrategyCspProvider : SQLSetupStrategy
 
         private readonly List<CspParameters> _cspKeyParameters = new List<CspParameters>();
 
-        public SqlColumnEncryptionCspProvider CspProvider { get; }
-
         public SQLSetupStrategyCspProvider(CspParameters cspParameters)
             : base(cspParameters.ProviderName + "/" + cspParameters.KeyContainerName)
         {
@@ -32,6 +30,8 @@ public SQLSetupStrategyCspProvider(CspParameters cspParameters)
             SetupDatabase();
         }
 
+        public SqlColumnEncryptionCspProvider CspProvider { get; }
+
         internal override void SetupDatabase()
         {
             ColumnMasterKey columnMasterKey = new CspProviderColumnMasterKey(GenerateUniqueName("CspExt"), SqlColumnEncryptionCspProvider.ProviderName, ColumnMasterKeyPath);
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestTrustedMasterKeyPaths.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestTrustedMasterKeyPaths.cs
@@ -147,17 +147,14 @@ public void TestTrustedColumnEncryptionMasterKeyPathsWithMultipleServers(string
             }
 
             // Add entries for one server
-            List<string> server1TrustedKeyPaths = new List<string>();
-
-            // Add some random key paths
-            foreach (char c in new char[] { 'A', 'B' })
+            List<string> server1TrustedKeyPaths = new List<string>()
             {
-                string invalidKeyPath = string.Format(@"{0}/my/{1}", StoreLocation.CurrentUser.ToString(), dummyThumbprint);
-                server1TrustedKeyPaths.Add(invalidKeyPath);
-            }
-
-            // Add the key path used by the test
-            server1TrustedKeyPaths.Add(columnMasterKeyPath);
+                // Add some random key paths
+                string.Format(@"{0}/my/{1}", StoreLocation.CurrentUser.ToString(), dummyThumbprint),
+                string.Format(@"{0}/my/{1}", StoreLocation.CurrentUser.ToString(), dummyThumbprint),
+                // Add the key path used by the test
+                columnMasterKeyPath
+            };
 
             // Add it to the dictionary
             SqlConnection.ColumnEncryptionTrustedMasterKeyPaths.Add(connBuilder.DataSource, server1TrustedKeyPaths);
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/SqlClientCustomTokenCredential.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/SqlClientCustomTokenCredential.cs
@@ -18,11 +18,8 @@ public class SqlClientCustomTokenCredential : TokenCredential
         private const string DEFAULT_PREFIX = "/.default";
         private const string AKVKeyName = "TestSqlClientAzureKeyVaultProvider";
 
-        private static string AKVUrl = (new Uri(DataTestUtility.AKVBaseUri, $"/keys/{AKVKeyName}")).AbsoluteUri;
-
         string _authority = "";
         string _resource = "";
-        string _akvUrl = "";
 
         public override AccessToken GetToken(TokenRequestContext requestContext, CancellationToken cancellationToken) =>
             AcquireTokenAsync().GetAwaiter().GetResult();
@@ -34,10 +31,11 @@ private async Task<AccessToken> AcquireTokenAsync()
         {
             // Added to reduce HttpClient calls.
             // For multi-user support, a better design can be implemented as needed.
-            if (_akvUrl != AKVUrl)
+            if (string.IsNullOrEmpty(_authority) || string.IsNullOrEmpty(_resource))
             {
                 using (HttpClient httpClient = new HttpClient())
                 {
+                    string AKVUrl = (new Uri(DataTestUtility.AKVBaseUri, $"/keys/{AKVKeyName}")).AbsoluteUri;
                     HttpResponseMessage response = await httpClient.GetAsync(AKVUrl);
                     string challenge = response?.Headers.WwwAuthenticate.FirstOrDefault()?.ToString();
                     string trimmedChallenge = ValidateChallenge(challenge);
@@ -69,8 +67,6 @@ private async Task<AccessToken> AcquireTokenAsync()
                         }
                     }
                 }
-                // Since this is a test, we only create single-instance temp cache
-                _akvUrl = AKVUrl;
             }
 
             AccessToken accessToken = await AzureActiveDirectoryAuthenticationCallback(_authority, _resource);
diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
@@ -49,6 +49,7 @@
     <Compile Include="AlwaysEncrypted\SqlBulkCopyTruncation.cs" />
     <Compile Include="AlwaysEncrypted\SqlNullValues.cs" />
     <Compile Include="AlwaysEncrypted\TestFixtures\DatabaseHelper.cs" />
+    <Compile Include="AlwaysEncrypted\TestFixtures\AEConnectionStringProviderWithCspParameters.cs" />
     <Compile Include="AlwaysEncrypted\TestFixtures\AzureKeyVaultKeyFixture.cs" />
     <Compile Include="AlwaysEncrypted\TestFixtures\SQLSetupStrategy.cs" />
     <Compile Include="AlwaysEncrypted\TestFixtures\SQLSetupStrategyCertStoreProvider.cs" />
diff --git a/src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Fixtures/ColumnMasterKeyCertificateFixture.cs b/src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Fixtures/ColumnMasterKeyCertificateFixture.cs
@@ -9,13 +9,13 @@ namespace Microsoft.Data.SqlClient.TestUtilities.Fixtures
 {
     public class ColumnMasterKeyCertificateFixture : CertificateFixtureBase
     {
-        public X509Certificate2 ColumnMasterKeyCertificate { get; }
-
         public ColumnMasterKeyCertificateFixture()
             : this(true)
         {
         }
 
+        public X509Certificate2 ColumnMasterKeyCertificate { get; }
+
         protected ColumnMasterKeyCertificateFixture(bool createCertificate)
         {
             if (createCertificate)
diff --git a/src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Fixtures/CspCertificateFixture.cs b/src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/Fixtures/CspCertificateFixture.cs
@@ -10,12 +10,6 @@ namespace Microsoft.Data.SqlClient.TestUtilities.Fixtures
 {
     public class CspCertificateFixture : CertificateFixtureBase
     {
-        public X509Certificate2 CspCertificate { get; }
-
-        public string CspCertificatePath { get; }
-
-        public string CspKeyPath { get; }
-
         public CspCertificateFixture()
         {
             CspCertificate = CreateCertificate(nameof(CspCertificate), Array.Empty<string>(), Array.Empty<string>(), true);
@@ -26,6 +20,12 @@ public CspCertificateFixture()
             CspKeyPath = GetCspPathFromCertificate();
         }
 
+        public X509Certificate2 CspCertificate { get; }
+
+        public string CspCertificatePath { get; }
+
+        public string CspKeyPath { get; }
+
         private string GetCspPathFromCertificate()
         {
             RSA privateKey = CspCertificate.GetRSAPrivateKey();

Original file line number	Diff line number	Diff line change
`@@ -8,12 +8,12 @@ namespace Microsoft.Data.SqlClient.ManualTesting.Tests.AlwaysEncrypted`
`8`	`8`	`{`
`9`	`9`	`public sealed class AzureKeyVaultKeyFixture : AzureKeyVaultKeyFixtureBase`
`10`	`10`	`{`
`11`		`- public string GeneratedKeyUri { get; }`
`12`		`-`
`13`	`11`	`public AzureKeyVaultKeyFixture()`
`14`	`12`	`: base(DataTestUtility.AKVBaseUri, DataTestUtility.GetTokenCredential())`
`15`	`13`	`{`
`16`	`14`	`GeneratedKeyUri = CreateKey(nameof(GeneratedKeyUri), 2048).ToString();`
`17`	`15`	`}`
	`16`	`+`
	`17`	`+ public string GeneratedKeyUri { get; }`
`18`	`18`	`}`
`19`	`19`	`}`
Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@ public class SQLSetupStrategy : ColumnMasterKeyCertificateFixture`
`61`	`61`	`public SQLSetupStrategy()`
`62`	`62`	`: base(true)`
`63`	`63`	`{`
`64`		`- ColumnMasterKeyPath = string.Concat(StoreLocation.CurrentUser.ToString(), "/", StoreName.My.ToString(), "/", ColumnMasterKeyCertificate.Thumbprint);`
	`64`	`+ ColumnMasterKeyPath = string.Format("{0}/{1}/{2}", StoreLocation.CurrentUser, StoreName.My, ColumnMasterKeyCertificate.Thumbprint);`
`65`	`65`	`}`
`66`	`66`
`67`	`67`	`protected SQLSetupStrategy(string customKeyPath)`
Original file line number	Diff line number	Diff line change
`@@ -15,8 +15,6 @@ public class SQLSetupStrategyCspProvider : SQLSetupStrategy`
`15`	`15`
`16`	`16`	`private readonly List<CspParameters> _cspKeyParameters = new List<CspParameters>();`
`17`	`17`
`18`		`- public SqlColumnEncryptionCspProvider CspProvider { get; }`
`19`		`-`
`20`	`18`	`public SQLSetupStrategyCspProvider(CspParameters cspParameters)`
`21`	`19`	`: base(cspParameters.ProviderName + "/" + cspParameters.KeyContainerName)`
`22`	`20`	`{`
`@@ -32,6 +30,8 @@ public SQLSetupStrategyCspProvider(CspParameters cspParameters)`
`32`	`30`	`SetupDatabase();`
`33`	`31`	`}`
`34`	`32`
	`33`	`+ public SqlColumnEncryptionCspProvider CspProvider { get; }`
	`34`	`+`
`35`	`35`	`internal override void SetupDatabase()`
`36`	`36`	`{`
`37`	`37`	`ColumnMasterKey columnMasterKey = new CspProviderColumnMasterKey(GenerateUniqueName("CspExt"), SqlColumnEncryptionCspProvider.ProviderName, ColumnMasterKeyPath);`
Original file line number	Diff line number	Diff line change
`@@ -18,11 +18,8 @@ public class SqlClientCustomTokenCredential : TokenCredential`
`18`	`18`	`private const string DEFAULT_PREFIX = "/.default";`
`19`	`19`	`private const string AKVKeyName = "TestSqlClientAzureKeyVaultProvider";`
`20`	`20`
`21`		`- private static string AKVUrl = (new Uri(DataTestUtility.AKVBaseUri, $"/keys/{AKVKeyName}")).AbsoluteUri;`
`22`		`-`
`23`	`21`	`string _authority = "";`
`24`	`22`	`string _resource = "";`
`25`		`- string _akvUrl = "";`
`26`	`23`
`27`	`24`	`public override AccessToken GetToken(TokenRequestContext requestContext, CancellationToken cancellationToken) =>`
`28`	`25`	`AcquireTokenAsync().GetAwaiter().GetResult();`
`@@ -34,10 +31,11 @@ private async Task<AccessToken> AcquireTokenAsync()`
`34`	`31`	`{`
`35`	`32`	`// Added to reduce HttpClient calls.`
`36`	`33`	`// For multi-user support, a better design can be implemented as needed.`
`37`		`- if (_akvUrl != AKVUrl)`
	`34`	`+ if (string.IsNullOrEmpty(_authority) \|\| string.IsNullOrEmpty(_resource))`
`38`	`35`	`{`
`39`	`36`	`using (HttpClient httpClient = new HttpClient())`
`40`	`37`	`{`
	`38`	`+ string AKVUrl = (new Uri(DataTestUtility.AKVBaseUri, $"/keys/{AKVKeyName}")).AbsoluteUri;`
`41`	`39`	`HttpResponseMessage response = await httpClient.GetAsync(AKVUrl);`
`42`	`40`	`string challenge = response?.Headers.WwwAuthenticate.FirstOrDefault()?.ToString();`
`43`	`41`	`string trimmedChallenge = ValidateChallenge(challenge);`
`@@ -69,8 +67,6 @@ private async Task<AccessToken> AcquireTokenAsync()`
`69`	`67`	`}`
`70`	`68`	`}`
`71`	`69`	`}`
`72`		`- // Since this is a test, we only create single-instance temp cache`
`73`		`- _akvUrl = AKVUrl;`
`74`	`70`	`}`
`75`	`71`
`76`	`72`	`AccessToken accessToken = await AzureActiveDirectoryAuthenticationCallback(_authority, _resource);`
Original file line number	Diff line number	Diff line change
`@@ -9,13 +9,13 @@ namespace Microsoft.Data.SqlClient.TestUtilities.Fixtures`
`9`	`9`	`{`
`10`	`10`	`public class ColumnMasterKeyCertificateFixture : CertificateFixtureBase`
`11`	`11`	`{`
`12`		`- public X509Certificate2 ColumnMasterKeyCertificate { get; }`
`13`		`-`
`14`	`12`	`public ColumnMasterKeyCertificateFixture()`
`15`	`13`	`: this(true)`
`16`	`14`	`{`
`17`	`15`	`}`
`18`	`16`
	`17`	`+ public X509Certificate2 ColumnMasterKeyCertificate { get; }`
	`18`	`+`
`19`	`19`	`protected ColumnMasterKeyCertificateFixture(bool createCertificate)`
`20`	`20`	`{`
`21`	`21`	`if (createCertificate)`
Original file line number	Diff line number	Diff line change
`@@ -10,12 +10,6 @@ namespace Microsoft.Data.SqlClient.TestUtilities.Fixtures`
`10`	`10`	`{`
`11`	`11`	`public class CspCertificateFixture : CertificateFixtureBase`
`12`	`12`	`{`
`13`		`- public X509Certificate2 CspCertificate { get; }`
`14`		`-`
`15`		`- public string CspCertificatePath { get; }`
`16`		`-`
`17`		`- public string CspKeyPath { get; }`
`18`		`-`
`19`	`13`	`public CspCertificateFixture()`
`20`	`14`	`{`
`21`	`15`	`CspCertificate = CreateCertificate(nameof(CspCertificate), Array.Empty<string>(), Array.Empty<string>(), true);`
`@@ -26,6 +20,12 @@ public CspCertificateFixture()`
`26`	`20`	`CspKeyPath = GetCspPathFromCertificate();`
`27`	`21`	`}`
`28`	`22`
	`23`	`+ public X509Certificate2 CspCertificate { get; }`
	`24`	`+`
	`25`	`+ public string CspCertificatePath { get; }`
	`26`	`+`
	`27`	`+ public string CspKeyPath { get; }`
	`28`	`+`
`29`	`29`	`private string GetCspPathFromCertificate()`
`30`	`30`	`{`
`31`	`31`	`RSA privateKey = CspCertificate.GetRSAPrivateKey();`