From 3d6eecfb769a40b919477f6906c40dd1a47f1da2 Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Mon, 9 Sep 2024 12:25:51 -0700 Subject: [PATCH 01/12] Added forced column decryption error test. --- .../ManualTests/AlwaysEncrypted/AKVTests.cs | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index 3f2b7a83fa..e392cdb092 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -58,6 +58,68 @@ public void TestEncryptDecryptWithAKV() DatabaseHelper.ValidateResultSet(sqlDataReader); } + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringSetupForAE), nameof(DataTestUtility.IsAKVSetupAvailable))] + public void ForcedColumnDecryptErrorTestShouldPass() + { + SqlConnectionStringBuilder builder = new(DataTestUtility.TCPConnectionStringHGSVBS) + { + ColumnEncryptionSetting = SqlConnectionColumnEncryptionSetting.Enabled, + AttestationProtocol = SqlConnectionAttestationProtocol.NotSpecified, + EnclaveAttestationUrl = "" + }; + + using (SqlConnection sqlConnection = new(builder.ConnectionString)) + { + sqlConnection.Open(); + Table.DeleteData(_akvTableName, sqlConnection); + Customer customer = new(45, "Microsoft", "Corporation"); + + using (SqlTransaction sqlTransaction = sqlConnection.BeginTransaction()) + { + DatabaseHelper.InsertCustomerData(sqlConnection, sqlTransaction, _akvTableName, customer); + sqlTransaction.Commit(); + } + } + + // Setup Empty key store provider + Dictionary emptyKeyStoreProviders = new() + { + { "AZURE_KEY_VAULT", new EmptyKeyStoreProvider() } + }; + + // Try this test 3 times to ensure that the connection is not left in a bad state after a decrypt error. + for (int i = 0; i < 3; i++) + { + // Setup connection using the empty key store provider + using (SqlConnection sqlConnection = new SqlConnection(builder.ConnectionString)) + { + sqlConnection.Open(); + sqlConnection.RegisterColumnEncryptionKeyStoreProvidersOnConnection(emptyKeyStoreProviders); + + using SqlCommand sqlCommand = new($"SELECT FirstName FROM [{_akvTableName}] WHERE FirstName = @firstName", + sqlConnection); + SqlParameter customerFirstParam = sqlCommand.Parameters.AddWithValue(@"firstName", @"Microsoft"); + customerFirstParam.Direction = System.Data.ParameterDirection.Input; + customerFirstParam.ForceColumnEncryption = true; + + using SqlDataReader sqlDataReader = sqlCommand.ExecuteReader(); + + try + { + while (sqlDataReader.Read()) + { + string firstName = sqlDataReader.GetString(0); + } + } + catch (Exception ex) + { + Console.WriteLine($"Error message = {ex.Message}"); + Assert.Contains("Failed to decrypt column", ex.Message); + } + } + } + } + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsAKVSetupAvailable))] [PlatformSpecific(TestPlatforms.Windows)] public void TestRoundTripWithAKVAndCertStoreProvider() @@ -120,5 +182,18 @@ public void TestLocalCekCacheIsScopedToProvider() Exception ex = Assert.Throws(() => sqlCommand.ExecuteReader()); Assert.StartsWith("The current credential is not configured to acquire tokens for tenant", ex.InnerException.Message); } + + private class EmptyKeyStoreProvider : SqlColumnEncryptionKeyStoreProvider + { + public override byte[] DecryptColumnEncryptionKey(string masterKeyPath, string encryptionAlgorithm, byte[] encryptedColumnEncryptionKey) + { + return new byte[32]; + } + + public override byte[] EncryptColumnEncryptionKey(string masterKeyPath, string encryptionAlgorithm, byte[] columnEncryptionKey) + { + return new byte[32]; + } + } } } From 16dfc145c4eb5d2427b7f07de324b22e859b45ef Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Mon, 9 Sep 2024 13:14:36 -0700 Subject: [PATCH 02/12] Add Assert.Fail if decryption succeeded. --- .../tests/ManualTests/AlwaysEncrypted/AKVTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index e392cdb092..76f46e387a 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -110,6 +110,8 @@ public void ForcedColumnDecryptErrorTestShouldPass() { string firstName = sqlDataReader.GetString(0); } + // If this is reached, then decryption succeeded unexpectedly. + Assert.Fail( "Column decryption should have failed."); } catch (Exception ex) { From 8b370648b8e6072974f6ad66803ff0534fc1db3e Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Mon, 9 Sep 2024 13:47:58 -0700 Subject: [PATCH 03/12] Verify that the Decryption failed. --- .../tests/ManualTests/AlwaysEncrypted/AKVTests.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index 76f46e387a..a1cf662ee1 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -106,12 +106,15 @@ public void ForcedColumnDecryptErrorTestShouldPass() try { + string firstName = string.Empty; while (sqlDataReader.Read()) { - string firstName = sqlDataReader.GetString(0); + firstName = sqlDataReader.GetString(0); } // If this is reached, then decryption succeeded unexpectedly. - Assert.Fail( "Column decryption should have failed."); + Console.WriteLine($"firstName = {firstName}"); + Assert.Contains(@"Microsoft", firstName); + Console.WriteLine( "Column decryption should have failed."); } catch (Exception ex) { From c383163e2657a2b97dd11104ffefe1604fe1f81c Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Mon, 9 Sep 2024 14:27:50 -0700 Subject: [PATCH 04/12] Revised decryption failure detection. --- .../ManualTests/AlwaysEncrypted/AKVTests.cs | 26 ++++--------------- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index a1cf662ee1..846dbe9d2b 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -35,7 +35,7 @@ public void TestEncryptDecryptWithAKV() AttestationProtocol = SqlConnectionAttestationProtocol.NotSpecified, EnclaveAttestationUrl = "" }; - using SqlConnection sqlConnection = new (builder.ConnectionString); + using SqlConnection sqlConnection = new(builder.ConnectionString); sqlConnection.Open(); Customer customer = new(45, "Microsoft", "Corporation"); @@ -48,7 +48,7 @@ public void TestEncryptDecryptWithAKV() } // Test INPUT parameter on an encrypted parameter - using SqlCommand sqlCommand = new ($"SELECT CustomerId, FirstName, LastName FROM [{_akvTableName}] WHERE FirstName = @firstName", + using SqlCommand sqlCommand = new($"SELECT CustomerId, FirstName, LastName FROM [{_akvTableName}] WHERE FirstName = @firstName", sqlConnection); SqlParameter customerFirstParam = sqlCommand.Parameters.AddWithValue(@"firstName", @"Microsoft"); customerFirstParam.Direction = System.Data.ParameterDirection.Input; @@ -103,24 +103,8 @@ public void ForcedColumnDecryptErrorTestShouldPass() customerFirstParam.ForceColumnEncryption = true; using SqlDataReader sqlDataReader = sqlCommand.ExecuteReader(); - - try - { - string firstName = string.Empty; - while (sqlDataReader.Read()) - { - firstName = sqlDataReader.GetString(0); - } - // If this is reached, then decryption succeeded unexpectedly. - Console.WriteLine($"firstName = {firstName}"); - Assert.Contains(@"Microsoft", firstName); - Console.WriteLine( "Column decryption should have failed."); - } - catch (Exception ex) - { - Console.WriteLine($"Error message = {ex.Message}"); - Assert.Contains("Failed to decrypt column", ex.Message); - } + var error = Assert.Throws(() => sqlDataReader.Read()); + Assert.Contains("Failed to decrypt column", error.Message); } } } @@ -129,7 +113,7 @@ public void ForcedColumnDecryptErrorTestShouldPass() [PlatformSpecific(TestPlatforms.Windows)] public void TestRoundTripWithAKVAndCertStoreProvider() { - using SQLSetupStrategyCertStoreProvider certStoreFixture = new (); + using SQLSetupStrategyCertStoreProvider certStoreFixture = new(); byte[] plainTextColumnEncryptionKey = ColumnEncryptionKey.GenerateRandomBytes(ColumnEncryptionKey.KeySizeInBytes); byte[] encryptedColumnEncryptionKeyUsingAKV = _fixture.AkvStoreProvider.EncryptColumnEncryptionKey(DataTestUtility.AKVUrl, @"RSA_OAEP", plainTextColumnEncryptionKey); byte[] columnEncryptionKeyReturnedAKV2Cert = certStoreFixture.CertStoreProvider.DecryptColumnEncryptionKey(certStoreFixture.CspColumnMasterKey.KeyPath, @"RSA_OAEP", encryptedColumnEncryptionKeyUsingAKV); From 1451501fa65a7007f6150f729b132782059b6238 Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Mon, 9 Sep 2024 14:32:03 -0700 Subject: [PATCH 05/12] Revised decryption failure detection. --- .../tests/ManualTests/AlwaysEncrypted/AKVTests.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index 846dbe9d2b..e22cddea5e 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -104,7 +104,10 @@ public void ForcedColumnDecryptErrorTestShouldPass() using SqlDataReader sqlDataReader = sqlCommand.ExecuteReader(); var error = Assert.Throws(() => sqlDataReader.Read()); - Assert.Contains("Failed to decrypt column", error.Message); + if (error != null) + Assert.Contains("Failed to decrypt column", error.Message); + else + Assert.Fail("Decryption unexpectedly succeeded."); } } } From 0c2e3744332dccafdfb8bb45776e819d5a9d87f5 Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Mon, 9 Sep 2024 15:05:57 -0700 Subject: [PATCH 06/12] Further revised decryption failure detection. --- .../tests/ManualTests/AlwaysEncrypted/AKVTests.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index e22cddea5e..4714bba49e 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -103,11 +103,12 @@ public void ForcedColumnDecryptErrorTestShouldPass() customerFirstParam.ForceColumnEncryption = true; using SqlDataReader sqlDataReader = sqlCommand.ExecuteReader(); - var error = Assert.Throws(() => sqlDataReader.Read()); - if (error != null) + while (sqlDataReader.Read()) + { + var error = Assert.Throws(() => DatabaseHelper.CompareResults(sqlDataReader, new string[] { @"string" }, 1)); + Console.WriteLine($"Error = {error.Message}"); Assert.Contains("Failed to decrypt column", error.Message); - else - Assert.Fail("Decryption unexpectedly succeeded."); + } } } } From 02b745952c5867dc319e409338cc547009cc13b9 Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Tue, 10 Sep 2024 09:09:23 -0700 Subject: [PATCH 07/12] Use IsTargetReadyForAeWithKeyStore in ConditionalFact --- .../tests/ManualTests/AlwaysEncrypted/AKVTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index 4714bba49e..c384b49c6f 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -58,7 +58,7 @@ public void TestEncryptDecryptWithAKV() DatabaseHelper.ValidateResultSet(sqlDataReader); } - [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringSetupForAE), nameof(DataTestUtility.IsAKVSetupAvailable))] + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsTargetReadyForAeWithKeyStore), nameof(DataTestUtility.IsAKVSetupAvailable))] public void ForcedColumnDecryptErrorTestShouldPass() { SqlConnectionStringBuilder builder = new(DataTestUtility.TCPConnectionStringHGSVBS) From b2baec6b955c0350b0ad502cce3b0b518687cf37 Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Wed, 11 Sep 2024 09:27:06 -0700 Subject: [PATCH 08/12] Restore ConditionalFact to added unit test. --- .../tests/ManualTests/AlwaysEncrypted/AKVTests.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index c384b49c6f..b20c4644c2 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -58,7 +58,7 @@ public void TestEncryptDecryptWithAKV() DatabaseHelper.ValidateResultSet(sqlDataReader); } - [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsTargetReadyForAeWithKeyStore), nameof(DataTestUtility.IsAKVSetupAvailable))] + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringSetupForAE), nameof(DataTestUtility.IsAKVSetupAvailable))] public void ForcedColumnDecryptErrorTestShouldPass() { SqlConnectionStringBuilder builder = new(DataTestUtility.TCPConnectionStringHGSVBS) @@ -71,8 +71,7 @@ public void ForcedColumnDecryptErrorTestShouldPass() using (SqlConnection sqlConnection = new(builder.ConnectionString)) { sqlConnection.Open(); - Table.DeleteData(_akvTableName, sqlConnection); - Customer customer = new(45, "Microsoft", "Corporation"); + Customer customer = new(88, "Microsoft2", "Corporation2"); using (SqlTransaction sqlTransaction = sqlConnection.BeginTransaction()) { @@ -98,7 +97,7 @@ public void ForcedColumnDecryptErrorTestShouldPass() using SqlCommand sqlCommand = new($"SELECT FirstName FROM [{_akvTableName}] WHERE FirstName = @firstName", sqlConnection); - SqlParameter customerFirstParam = sqlCommand.Parameters.AddWithValue(@"firstName", @"Microsoft"); + SqlParameter customerFirstParam = sqlCommand.Parameters.AddWithValue(@"firstName", @"Microsoft2"); customerFirstParam.Direction = System.Data.ParameterDirection.Input; customerFirstParam.ForceColumnEncryption = true; From 987301d4e4331654d6a727ead43ddfa40649f1ee Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Fri, 13 Sep 2024 14:19:29 -0700 Subject: [PATCH 09/12] Added comments to explain the purpose of unit test. --- .../ManualTests/AlwaysEncrypted/AKVTests.cs | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index b20c4644c2..73887bcded 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -58,8 +58,17 @@ public void TestEncryptDecryptWithAKV() DatabaseHelper.ValidateResultSet(sqlDataReader); } + /* + This unit test is designed for PR #2618, which addresses an issue where a failed decryption leaves a connection in a bad state + when it is returned to the connection pool. If a subsequent connection is retried it will result in an "Internal connection fatal error", + which causes that connection to be doomed, preventing it from being returned to the pool. + Consequently, retrying a new connection will encounter the same decryption error, leading to a repetitive failure cycle. + + The purpose of this unit test is to simulate a decryption error and verify that the connection remains usable when returned to the pool. + It aims to confirm that three consecutive connections will consistently fail with the "Failed to decrypt column" error. + */ [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringSetupForAE), nameof(DataTestUtility.IsAKVSetupAvailable))] - public void ForcedColumnDecryptErrorTestShouldPass() + public void ForcedColumnDecryptErrorTestShouldFail() { SqlConnectionStringBuilder builder = new(DataTestUtility.TCPConnectionStringHGSVBS) { @@ -68,6 +77,7 @@ public void ForcedColumnDecryptErrorTestShouldPass() EnclaveAttestationUrl = "" }; + // Setup record to query using (SqlConnection sqlConnection = new(builder.ConnectionString)) { sqlConnection.Open(); @@ -86,17 +96,17 @@ public void ForcedColumnDecryptErrorTestShouldPass() { "AZURE_KEY_VAULT", new EmptyKeyStoreProvider() } }; - // Try this test 3 times to ensure that the connection is not left in a bad state after a decrypt error. + // Three consecutive connections should fail with "Failed to decrypt column" error. This proves that an error in decryption + // does not leave the connection in a bad state. for (int i = 0; i < 3; i++) { - // Setup connection using the empty key store provider using (SqlConnection sqlConnection = new SqlConnection(builder.ConnectionString)) { sqlConnection.Open(); + // Setup connection using the empty key store provider thereby forcing a decryption error. sqlConnection.RegisterColumnEncryptionKeyStoreProvidersOnConnection(emptyKeyStoreProviders); - using SqlCommand sqlCommand = new($"SELECT FirstName FROM [{_akvTableName}] WHERE FirstName = @firstName", - sqlConnection); + using SqlCommand sqlCommand = new($"SELECT FirstName FROM [{_akvTableName}] WHERE FirstName = @firstName", sqlConnection); SqlParameter customerFirstParam = sqlCommand.Parameters.AddWithValue(@"firstName", @"Microsoft2"); customerFirstParam.Direction = System.Data.ParameterDirection.Input; customerFirstParam.ForceColumnEncryption = true; @@ -105,7 +115,6 @@ public void ForcedColumnDecryptErrorTestShouldPass() while (sqlDataReader.Read()) { var error = Assert.Throws(() => DatabaseHelper.CompareResults(sqlDataReader, new string[] { @"string" }, 1)); - Console.WriteLine($"Error = {error.Message}"); Assert.Contains("Failed to decrypt column", error.Message); } } From 88fdd76d323c66a1580d041516674e36732b1b72 Mon Sep 17 00:00:00 2001 From: Aris Rellegue <134557572+arellegue@users.noreply.github.com> Date: Wed, 18 Sep 2024 15:07:39 -0700 Subject: [PATCH 10/12] Update src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com> --- .../tests/ManualTests/AlwaysEncrypted/AKVTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index 73887bcded..ec6c560a69 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -59,7 +59,7 @@ public void TestEncryptDecryptWithAKV() } /* - This unit test is designed for PR #2618, which addresses an issue where a failed decryption leaves a connection in a bad state + This unit test is going to assess an issue where a failed decryption leaves a connection in a bad state when it is returned to the connection pool. If a subsequent connection is retried it will result in an "Internal connection fatal error", which causes that connection to be doomed, preventing it from being returned to the pool. Consequently, retrying a new connection will encounter the same decryption error, leading to a repetitive failure cycle. From c04afdf179b74df725e0875027e699d2eeae11ba Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Wed, 18 Sep 2024 17:00:56 -0700 Subject: [PATCH 11/12] Update comment. --- .../ManualTests/AlwaysEncrypted/AKVTests.cs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index ec6c560a69..4eafeb3873 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -66,6 +66,26 @@ This unit test is going to assess an issue where a failed decryption leaves a co The purpose of this unit test is to simulate a decryption error and verify that the connection remains usable when returned to the pool. It aims to confirm that three consecutive connections will consistently fail with the "Failed to decrypt column" error. + + Before the fix was implemented, this is what happens internally: + + 1. A connection is used from a connection in pool. + 2. That connection is used to query a table with a column that is encrypted. + 3. When the decryption failed for some reason, a "Failed to decrypt column." error is raised. The connection is returned to the connection pool + with the error still in its buffer. + 4. When the client connects a second time, a connection is used from the pool again and grabbing the same connection recently returned back to the pool. + 5. This time when the connection is opened it will raise an "Internal connection fatal error." because that connection still has an existing error in it. + Thus, the driver dooms that connection since the error is fatal and not returned it back to the connection pool. + 6. When the client connects the third time, it will grab an unused connection from the connection pool. + 7. That new connection is used to query a table with a column that is encrypted. + 8. When the decryption failed for some reason, a "Failed to decrypt column." error is raised. The connection is returned to the connection pool + with the error still in its buffer. + 9. And the cycle continues. + + So, this unit test will try to connect 3 times and do the same query to ensure that the expected error messages are identical each time, + "Failed to decrypt column". This proves that the fix worked, and the connection is returned to the pool in a good state. + Although, 2 tries are enough to ascertain that the fix worked but for completeness, same numbers of connection attempts as in the repro + are performed. */ [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringSetupForAE), nameof(DataTestUtility.IsAKVSetupAvailable))] public void ForcedColumnDecryptErrorTestShouldFail() From 5bb116a63655f2e5e6b70d462c8181cf2def373c Mon Sep 17 00:00:00 2001 From: v-arellegue Date: Fri, 20 Sep 2024 13:06:23 -0700 Subject: [PATCH 12/12] Revised the unit test comments. --- .../ManualTests/AlwaysEncrypted/AKVTests.cs | 30 ++++++------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs index 4eafeb3873..f39dbb3bea 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/AKVTests.cs @@ -62,30 +62,10 @@ public void TestEncryptDecryptWithAKV() This unit test is going to assess an issue where a failed decryption leaves a connection in a bad state when it is returned to the connection pool. If a subsequent connection is retried it will result in an "Internal connection fatal error", which causes that connection to be doomed, preventing it from being returned to the pool. - Consequently, retrying a new connection will encounter the same decryption error, leading to a repetitive failure cycle. + Consequently, retrying a third connection will encounter the same decryption error, leading to a repetitive failure cycle. The purpose of this unit test is to simulate a decryption error and verify that the connection remains usable when returned to the pool. It aims to confirm that three consecutive connections will consistently fail with the "Failed to decrypt column" error. - - Before the fix was implemented, this is what happens internally: - - 1. A connection is used from a connection in pool. - 2. That connection is used to query a table with a column that is encrypted. - 3. When the decryption failed for some reason, a "Failed to decrypt column." error is raised. The connection is returned to the connection pool - with the error still in its buffer. - 4. When the client connects a second time, a connection is used from the pool again and grabbing the same connection recently returned back to the pool. - 5. This time when the connection is opened it will raise an "Internal connection fatal error." because that connection still has an existing error in it. - Thus, the driver dooms that connection since the error is fatal and not returned it back to the connection pool. - 6. When the client connects the third time, it will grab an unused connection from the connection pool. - 7. That new connection is used to query a table with a column that is encrypted. - 8. When the decryption failed for some reason, a "Failed to decrypt column." error is raised. The connection is returned to the connection pool - with the error still in its buffer. - 9. And the cycle continues. - - So, this unit test will try to connect 3 times and do the same query to ensure that the expected error messages are identical each time, - "Failed to decrypt column". This proves that the fix worked, and the connection is returned to the pool in a good state. - Although, 2 tries are enough to ascertain that the fix worked but for completeness, same numbers of connection attempts as in the repro - are performed. */ [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringSetupForAE), nameof(DataTestUtility.IsAKVSetupAvailable))] public void ForcedColumnDecryptErrorTestShouldFail() @@ -118,6 +98,14 @@ public void ForcedColumnDecryptErrorTestShouldFail() // Three consecutive connections should fail with "Failed to decrypt column" error. This proves that an error in decryption // does not leave the connection in a bad state. + // In each try, when a "Failed to decrypt error" is thrown, the connection's TDS Parser state object buffer is drained of any + // pending data so it does not interfere with future operations. In addition, the TDS parser state object's reader.DataReady flag + // is set to false so that the calling function that catches the exception will not continue to use the reader. Otherwise, it will + // timeout waiting to read data that doesn't exist. Also, the TDS Parser state object HasPendingData flag is also set to false + // to indicate that the buffer has been cleared and to avoid it getting cleared again in SqlDataReader.TryCloseInternal function. + // Finally, after successfully handling the decryption error, the connection is then returned back to the connection pool without + // an error. A proof that the connection's state object is clean is in the second connection being able to throw the same error. + // The third connection is for making sure we test 3 times as the minimum number of connections to reproduce the issue previously. for (int i = 0; i < 3; i++) { using (SqlConnection sqlConnection = new SqlConnection(builder.ConnectionString))