Should AlwaysEncryptedAttestationException be public?

[kmscode]

Reference file: src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/AlwaysEncryptedAttestationException.cs

Intermittently / Randomly (though more frequently when attempting to stress test my applications code I will receive the below AlwaysEncryptedAttestationException. However, I cannot directly catch this specific exception - I assume due to the class being internal.

Ideally I would like to retry in such a scenario.

Should this class be public?

Microsoft.Data.SqlClient.AlwaysEncryptedAttestationException: The validation of an attestation token failed. Cannot retrieve a public key from the attestation public key endpoint, or the retrieved key has an invalid format. Error details: 'A task was canceled.'.
 ---> System.AggregateException: One or more errors occurred. (IDX20803: Unable to obtain configuration from: 'https://name.region.attest.azure.net/.well-known/openid-configuration'.)
 ---> System.InvalidOperationException: IDX20803: Unable to obtain configuration from: 'https://name.region.attest.azure.net/.well-known/openid-configuration'.
 ---> System.IO.IOException: IDX20804: Unable to retrieve document from: 'https://name.region.attest.azure.net/.well-known/openid-configuration'.
 ---> System.Threading.Tasks.TaskCanceledException: The request was canceled due to the configured HttpClient.Timeout of 100 seconds elapsing.
 ---> System.TimeoutException: A task was canceled.
 ---> System.Threading.Tasks.TaskCanceledException: A task was canceled.
   at System.Threading.Tasks.TaskCompletionSourceWithCancellation`1.WaitWithCancellationAsync(CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.GetHttp11ConnectionAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithVersionDetectionAndRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.DiagnosticsHandler.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
   --- End of inner exception stack trace ---
   --- End of inner exception stack trace ---
   at System.Net.Http.HttpClient.HandleFailure(Exception e, Boolean telemetryStarted, HttpResponseMessage response, CancellationTokenSource cts, CancellationToken cancellationToken, CancellationTokenSource pendingRequestsCts)
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
   at Microsoft.IdentityModel.Protocols.HttpDocumentRetriever.GetDocumentAsync(String address, CancellationToken cancel)
   --- End of inner exception stack trace ---
   at Microsoft.IdentityModel.Protocols.HttpDocumentRetriever.GetDocumentAsync(String address, CancellationToken cancel)
   at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectConfigurationRetriever.GetAsync(String address, IDocumentRetriever retriever, CancellationToken cancel)
   at Microsoft.IdentityModel.Protocols.ConfigurationManager`1.GetConfigurationAsync(CancellationToken cancel)
   --- End of inner exception stack trace ---
   at Microsoft.IdentityModel.Protocols.ConfigurationManager`1.GetConfigurationAsync(CancellationToken cancel)
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)
   at Microsoft.Data.SqlClient.AzureAttestationEnclaveProvider.GetOpenIdConfigForSigningKeys(String url, Boolean forceUpdate)
   --- End of inner exception stack trace ---
   at Microsoft.Data.SqlClient.AzureAttestationEnclaveProvider.GetOpenIdConfigForSigningKeys(String url, Boolean forceUpdate)
   at Microsoft.Data.SqlClient.AzureAttestationEnclaveProvider.VerifyAzureAttestationInfo(String attestationUrl, EnclaveType enclaveType, String attestationToken, EnclavePublicKey enclavePublicKey, Byte[] nonce)
   at Microsoft.Data.SqlClient.AzureAttestationEnclaveProvider.CreateEnclaveSession(Byte[] attestationInfo, ECDiffieHellman clientDHKey, EnclaveSessionParameters enclaveSessionParameters, Byte[] customData, Int32 customDataLength, SqlEnclaveSession& sqlEnclaveSession, Int64& counter)
   at Microsoft.Data.SqlClient.EnclaveDelegate.CreateEnclaveSession(SqlConnectionAttestationProtocol attestationProtocol, String enclaveType, EnclaveSessionParameters enclaveSessionParameters, Byte[] attestationInfo, SqlEnclaveAttestationParameters attestationParameters, Byte[] customData, Int32 customDataLength)
   at Microsoft.Data.SqlClient.SqlCommand.ReadDescribeEncryptionParameterResults(SqlDataReader ds, ReadOnlyDictionary`2 describeParameterEncryptionRpcOriginalRpcMap)
   at Microsoft.Data.SqlClient.SqlCommand.PrepareForTransparentEncryption(CommandBehavior cmdBehavior, Boolean returnStream, Boolean isAsync, Int32 timeout, TaskCompletionSource`1 completion, Task& returnTask, Boolean asyncWrite, Boolean& usedCache, Boolean inRetry)
   at Microsoft.Data.SqlClient.SqlCommand.RunExecuteReader(CommandBehavior cmdBehavior, RunBehavior runBehavior, Boolean returnStream, TaskCompletionSource`1 completion, Int32 timeout, Task& task, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String method)
   at Microsoft.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, Boolean sendToPipe, Int32 timeout, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String methodName)
   at Microsoft.Data.SqlClient.SqlCommand.ExecuteNonQuery()
   at Namespace.Class.InsertMethod(ObjectType ot, SqlConnection connection, SqlTransaction sqlTransaction) in D:\a\1\s\Namespace\Class.cs:line 809

[johnnypham]

I see no reason why it can't be public. I think this driver is the only one that throws this kind of exception. Thoughts @David-Engel?

[David-Engel]

@johnnypham I always hesitate when considering adding new public surface area. Why wouldn't we catch that and instead throw a SqlException? I don't think AlwaysEncryptedAttestationException was meant to be exposed publicly.

[johnnypham]

I thought SqlException should only be thrown when the data source generates an error. But I see that JDBC just throws an SqlServerException with an error number of zero, so we could just do that. AlwaysEncryptedAttestationException would be unused then and could be removed.

[kmscode]

Thanks for the quick reviews! Much appreciated. I wasn't too keen on just jumping to public also, otherwise I would have just submitted a pull request.

As I was researching this, I did find this code analysis rule. But the odd thing is, that rule isn't triggered on code analysis - I guess because the exceptions are public themselves.

But in any case, throwing a SqlException (with appropriate details to allow detection as a transient error) would meet my needs.

[kmscode]

@johnnypham @David-Engel Just wanted to touch base on this one. We're sprucing up error handling code before going live with a new web app and this one is a bit of a blocker for us. We believer we have a workaround for the specific exception noted (we're going to try catching the inner TaskCanceledException) but if anything else causes AlwaysEncryptedAttestationException we won't be able to determine if it's a hard or transient failure. If there's any possibility of a patch for SqlClient being available soon, I would most likely want to push back my release accordingly.

Thanks again!

[roji]

@kmscode note that as a workaround you can always invoke GetType().GetName() on an Exception and match AlwaysEncryptedAttestationException by name - not pretty but should make this non-blocking.

[kmscode]

@roji yes - this was discussed also and will likely be done as well (I think the exact words used during my teams discussion was "ugly" but "not pretty" is nicer 😄). The specifically received exception and the TaskCanceledException will be getting added to our transient error handling code. The rest will be treated as hard failures for log reviews should we ever encounter them - 🤞 hopefully we don't!

[johnnypham]

@johnnypham @David-Engel Just wanted to touch base on this one. We're sprucing up error handling code before going live with a new web app and this one is a bit of a blocker for us. We believer we have a workaround for the specific exception noted (we're going to try catching the inner TaskCanceledException) but if anything else causes AlwaysEncryptedAttestationException we won't be able to determine if it's a hard or transient failure. If there's any possibility of a patch for SqlClient being available soon, I would most likely want to push back my release accordingly.

Thanks again!

I have some changes almost ready (here) that replace all instances of AlwaysEncryptedAttestationException with SqlException. Most of them don't seem to be transient errors. However, since the classes that throw them are not public, there's no documentation available that explains the exceptions thrown for each error scenario. You can go through the changes and see how you'd want to handle them.

[kmscode]

@johnnypham Thanks! I think this solves the problem since we can catch SqlException easily - then we'd have to update our transient error detection logic - which may still require looking to the inner exception - based on your comments above. But at least this would be a good change for anyone using Secure Enclaves to be able to easily catch/attempt to handle these exceptions. Thanks again!

[johnnypham]

If there's any possibility of a patch for SqlClient being available soon, I would most likely want to push back my release accordingly.

This fix alone probably won't warrant a hotfix/patch. If we release a hotfix before the GA release in May (none planned as of now), then this can be included in the hotfix. It'll be in a preview release if you're willing to take a dependency on a preview version. If so, we can definitely prioritize it for the next preview release.

[JRahnama]

Closing the issue as the fix was released in the GA.