Fix | Addressing failure on providing correct error message when symmetric key decryption fails using Always Encrypted.

[JRahnama]

The issue is happening when IsDBNull called on an encrypted column with backed keys in Azure key vault. If, for any reason, application fails to decrypt column keys, not the value the actual key for the column, the driver is waiting for the pending data to be received and throws unknown TDS token header, by setting the pending data to false, application will not process the rest to read the data.

 string command = $"SELECT [SSN] FROM [{s_schema}].[{s_table}]";
            using SqlCommand sqlCommand = new(command, connection);
            try
            {
                connection.Open();
                using SqlCommand cmd = connection.CreateCommand();
                cmd.CommandText = command;
                try
                {
                    using SqlDataReader reader = cmd.ExecuteReader();
                    if (reader.HasRows)
                    {
                        while (reader.Read())
                        {
                            if (!reader.IsDBNull(0))
                            {
                                _ = reader[0];
                                Console.WriteLine("It's not null db");
                            }
                        }
                    }
                }
                catch (Exception ex)
                {
                    Console.WriteLine(ex);
                }
~~~

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -1.09 ⚠️

Comparison is base (73c6b2f) 71.53% compared to head (75733a9) 70.44%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1948      +/-   ##
==========================================
- Coverage   71.53%   70.44%   -1.09%     
==========================================
  Files         306      306              
  Lines       61841    61567     -274     
==========================================
- Hits        44235    43371     -864     
- Misses      17606    18196     +590     

Flag	Coverage Δ
addons	92.38% <ø> (ø)
netcore	73.22% <0.00%> (-2.48%)	⬇️
netfx	69.09% <0.00%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs	68.37% <0.00%> (-5.55%)	⬇️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs	70.81% <0.00%> (-0.04%)	⬇️

... and 47 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[DavoudEshtehari]

You removed the DrainData(stateObj) function call later. Can you explain what the implication was?

Also, I am just adding a note not to forget to include tests.

[JRahnama]

You removed the DrainData(stateObj) function call later. Can you explain what the implication was?

Also, I am just adding a note not to forget to include tests.

Adding a test is bit complicated. It needs some modification to prevent the application from obtaining the key to decrypt column key. DrainData is reproducing the issue as there is no data at this point for the column. Basically at this point setting pending data to false will cause the driver to skip looking for the column value and exception will be thrown immediately.

[DavoudEshtehari]

What would be the result of this change with MARS enabled, and/or parallel async calls? These are the questions that require more tests.

[JRahnama]

What would be the result of this change with MARS enabled, and/or parallel async calls? These are the questions that require more tests.

So, the issue only happens on ISDBNull to get the column's info and if you dont use that no issue comes up at all. For async and MARS we do have tests to run and read values, I am not sure if ISDBNull has been called inside those tests, but no failure on the pipelines so far. I will try to come up with a solution for the test purposes.

[lcheunglci]

Thanks! I've tested this fix locally and it seems good to me. We'll have to add the test in a separate PR.