[.NET] Fix issues around handling errors during certificate signing operations

dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningOperation.cs

@@ -13,27 +13,10 @@ namespace Microsoft.Azure.Iot.Device.Models.CertificateManagement
     /// Phase 2 (Completed): IoT Hub delivers the issued certificate with a 200 response.
     /// </summary>
     /// <remarks>
-    /// If the operation fails at any point, both <see cref="Accepted"/> and <see cref="Completed"/> will
-    /// throw a <see cref="CertificateSigningRequestException"/> when awaited. The exception contains
-    /// structured error details such as <see cref="CertificateSigningRequestException.ErrorCode"/>,
-    /// <see cref="CertificateSigningRequestException.RetryAfterSeconds"/>, and for 409005 conflict errors,
-    /// <see cref="CertificateSigningRequestException.ActiveRequestId"/>.
-    /// <code>
-    /// try
-    /// {
-    ///     CertificateAcceptedResponse accepted = await operation.Accepted;
-    ///     CertificateSigningResponse completed = await operation.Completed;
-    /// }
-    /// catch (CertificateSigningRequestException ex) when (ex.ErrorCode == 409005)
-    /// {
-    ///     // Conflict: another CSR operation is active. Use Replace = "*" to override.
-    /// }
-    /// catch (CertificateSigningRequestException ex) when (ex.RetryAfterSeconds.HasValue)
-    /// {
-    ///     await Task.Delay(TimeSpan.FromSeconds(ex.RetryAfterSeconds.Value));
-    ///     // Retry the operation.
-    /// }
-    /// </code>
+    /// If the operation fails before <see cref="Accepted"/> completes successfully, both tasks fault with the
+    /// operation's exception. If <see cref="Accepted"/> has already completed successfully, a later failure
+    /// affects only <see cref="Completed"/>. If the completion callback throws, <see cref="Completed"/> faults
+    /// with the callback's exception. Cancellation cancels each task that has not already completed.
     /// </remarks>
     public class CertificateSigningOperation
     {
@@ -48,7 +31,7 @@ private readonly TaskCompletionSource<CertificateSigningResponse> _completed
         /// The result contains the correlation ID and operation expiration time.
         /// </summary>
         /// <exception cref="CertificateSigningRequestFailedException">
-        /// Thrown when the CSR is rejected by IoT Hub. Inspect <see cref="CertificateSigningRequestFailedException.ErrorCode"/>
+        /// Thrown when the CSR is rejected by IoT Hub. Inspect <see cref="CertificateSigningRequestFailedException.Error"/>
         /// for the specific failure reason (e.g., 400040 for CSR decode failure, 409005 for an active conflicting operation,
         /// 429002/429003 for throttling).
         /// </exception>
@@ -59,9 +42,10 @@ private readonly TaskCompletionSource<CertificateSigningResponse> _completed
         /// The result contains the certificate chain and correlation ID.
         /// </summary>
         /// <exception cref="CertificateSigningRequestFailedException">
-        /// Thrown when the certificate issuance fails after acceptance. This can also be thrown if the initial
-        /// request was rejected, since a failure at any phase propagates to both <see cref="Accepted"/> and
-        /// <see cref="Completed"/> tasks.
+        /// Thrown when the certificate issuance fails after acceptance, or when IoT hub's issued certificate response
+        /// could not be read. It can also be thrown if the request is rejected before <see cref="Accepted"/> completes.
+        /// If the <see cref="AbstractConnectionClient.HandleCertificateSigningCompleteAsync"/> callback throws, this
+        /// task fails with that exception instead.
         /// </exception>
         public Task<CertificateSigningResponse> Completed => _completed.Task;
 

dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningRequestErrorResponse.cs

@@ -1,47 +1,84 @@
 // Copyright (c) Microsoft. All rights reserved. Licensed under the MIT license.
 // See LICENSE file in the project root for full license information.
 
+using System.Globalization;
+using System.Text.Json;
 using System.Text.Json.Serialization;
 
 namespace Microsoft.Azure.Iot.Device.Models.CertificateManagement
 {
     /// <summary>
     /// Represents an error response from IoT hub about a certificate signing request.
     /// </summary>
+    /// <remarks>
+    /// IoT hub does not populate every field on every error, so every field here is optional. A field that IoT hub
+    /// omitted, or sent in a shape this client did not recognize, is left at its default value.
+    /// </remarks>
     public class CertificateSigningRequestErrorResponse
     {
+        /// <summary>
+        /// The IoT hub error code (for example, 400040 for a certificate signing request that could not be decoded, or
+        /// 409005 for a conflicting active request). Zero if IoT hub did not send one.
+        /// </summary>
         [JsonPropertyName("errorCode")]
-        public required int ErrorCode { get; set; }
+        public int ErrorCode { get; set; }
 
+        /// <summary>
+        /// A human readable description of the error.
+        /// </summary>
         [JsonPropertyName("message")]
         public string? Message { get; set; }
 
+        /// <summary>
+        /// The IoT hub tracking ID for this error, for support purposes.
+        /// </summary>
         [JsonPropertyName("trackingId")]
-        public required string TrackingId { get; set; }
+        public string? TrackingId { get; set; }
 
+        /// <summary>
+        /// When IoT hub generated this error.
+        /// </summary>
         [JsonPropertyName("timestampUtc")]
-        public required DateTimeOffset TimestampUtc { get; set; }
+        public DateTimeOffset? TimestampUtc { get; set; }
 
+        /// <summary>
+        /// Certificate management specific details about the error.
+        /// </summary>
         [JsonPropertyName("info")]
-        public required CertificateSigningRequestErrorInfo Info { get; set; }
+        public CertificateSigningRequestErrorInfo Info { get; set; } = new();
 
+        /// <summary>
+        /// How long IoT hub asked this device to wait before trying again, if it asked at all.
+        /// </summary>
         [JsonPropertyName("retryAfter")]
         public int? RetryAfterSeconds { get; set; }
 
+        /// <summary>
+        /// Certificate management specific details about a certificate signing request error.
+        /// </summary>
         public class CertificateSigningRequestErrorInfo
         {
+            /// <summary>
+            /// Correlation ID for diagnostic and support purposes.
+            /// </summary>
             [JsonPropertyName("correlationId")]
-            public required string CorrelationId { get; set; }
+            public string? CorrelationId { get; set; }
 
+            /// <summary>
+            /// The certificate management specific error (for example, "CertificateSigningRequestInvalid").
+            /// </summary>
             [JsonPropertyName("credentialError")]
-            public required string CertificateSigningRequestError { get; set; }
+            public string? CredentialError { get; set; }
 
+            /// <summary>
+            /// A human readable description of <see cref="CredentialError"/>.
+            /// </summary>
             [JsonPropertyName("credentialMessage")]
-            public string? CertificateSigningRequestMessage { get; set; }
-
-            [JsonPropertyName("requestId")]
-            public required string RequestId { get; set; }
+            public string? CredentialMessage { get; set; }
 
+            /// <summary>
+            /// When the operation this error refers to expires, if IoT hub reported it.
+            /// </summary>
             [JsonPropertyName("operationExpires")]
             public DateTimeOffset? OperationExpires { get; set; }
         }

dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningRequestFailedException.cs

@@ -10,5 +10,7 @@ namespace Microsoft.Azure.Iot.Device.Models.CertificateManagement
     public class CertificateSigningRequestFailedException : Exception
     {
         public required CertificateSigningRequestErrorResponse Error { get; set; }
+
+        public string? RequestId { get; set; }
     }
 }

dotnet/src/Microsoft.Azure.Iot.Device/Unified/Connection/ConnectionClient.cs

@@ -116,6 +116,7 @@ public override async Task HandleConnectedToHubAsync(MqttClientConnectedEventArg
         /// <returns>A set of tasks. One that completes when IoT hub accepts the request (and starts signing), one that completes when IoT hub completes the signing, and one that completes if any step in the process fails.</returns>
         public async Task<CertificateSigningOperation> SendCertificateSigningRequestAsync(IotHubCertificateSigningRequest request, CancellationToken cancellationToken = default)
         {
+            //TODO how does hub respond if device loses connection at any point during this process?
             ObjectDisposedException.ThrowIf(_isDisposed, this);
 
             if (CurrentConnectionContext == null)
@@ -145,6 +146,9 @@ private async Task HandleReceivedCertificateSigningPublish(MqttPublishReceivedEv
         {
             if (args.Publish.Topic.StartsWith(CertificateSigningResponseTopic))
             {
+                CertificateSigningOperation? pendingCertificateSigningOperation = null;
+                string? requestId = null;
+                bool hasRequestFinished = false;
                 try
                 {
                     string[] topicTokens = args.Publish.Topic.Split("/");
@@ -154,22 +158,25 @@ private async Task HandleReceivedCertificateSigningPublish(MqttPublishReceivedEv
                     }
 
                     string status = topicTokens[3];
-                    string requestId = topicTokens[4].Split(RequestId)[1];
+                    requestId = topicTokens[4].Split(RequestId)[1];
 
-                    if (!_pendingCertificateSigningOperations.TryGetValue(requestId, out var pendingCertificateSigningOperation))
+                    if (!_pendingCertificateSigningOperations.TryGetValue(requestId, out pendingCertificateSigningOperation))
                     {
                         return;
                     }
 
                     if (status.Equals("202"))
                     {
-                        CertificateSigningRequestAccepted accepted = JsonSerializer.Deserialize<CertificateSigningRequestAccepted>(args.Publish.Payload)!;
+                        CertificateSigningRequestAccepted accepted = JsonSerializer.Deserialize<CertificateSigningRequestAccepted>(args.Publish.Payload)
+                            ?? throw new JsonException("Certificate signing completion response was null.");
                         pendingCertificateSigningOperation.SetAccepted(accepted);
                         return;
                     }
                     else if (status.Equals("200"))
                     {
-                        CertificateSigningResponse response = JsonSerializer.Deserialize<CertificateSigningResponse>(args.Publish.Payload)!;
+                        hasRequestFinished = true;
+                        CertificateSigningResponse response = JsonSerializer.Deserialize<CertificateSigningResponse>(args.Publish.Payload)
+                            ?? throw new JsonException("Certificate signing completion response was null.");
                         if (HandleCertificateSigningCompleteAsync != null)
                         {
                             //TODO need a fault-injection like unit test that ensures that the client uses this new authentication provider upon reconnect since our API won't allow users to disconnect then reconnect to hub at will
@@ -180,18 +187,41 @@ private async Task HandleReceivedCertificateSigningPublish(MqttPublishReceivedEv
                         {
                             Trace.TraceError("Certificate signing response could not update authentication provider because user never set \"HandleCertificateSigningCompleteAsync\" callback");
                         }
+
                         pendingCertificateSigningOperation.SetCompleted(response);
                         return;
                     }
                     else
                     {
-                        CertificateSigningRequestErrorResponse error = JsonSerializer.Deserialize<CertificateSigningRequestErrorResponse>(args.Publish.Payload)!;
-                        pendingCertificateSigningOperation.SetFailed(new CertificateSigningRequestFailedException() { Error = error });
+                        hasRequestFinished = true;
+                        CertificateSigningRequestErrorResponse error = JsonSerializer.Deserialize<CertificateSigningRequestErrorResponse>(args.Publish.Payload)
+                            ?? throw new JsonException("Certificate signing completion response was null.");
+                        pendingCertificateSigningOperation.SetFailed(new CertificateSigningRequestFailedException() { Error = error, RequestId = requestId });
                         return;
                     }
                 }
+                catch (JsonException ex)
+                {
+                    // An unreadable response must fail the operation rather than leave it pending forever
+                    pendingCertificateSigningOperation?.SetFailed(new CertificateSigningRequestFailedException()
+                    {
+                        Error = new CertificateSigningRequestErrorResponse() { Message = "Failed to read the certificate signing response from IoT hub: " + ex.Message },
+                        RequestId = requestId,
+                    });
+                }
+                catch (Exception ex) when (pendingCertificateSigningOperation != null)
+                {
+                    // Includes the user's HandleCertificateSigningCompleteAsync callback throwing
+                    pendingCertificateSigningOperation.SetFailed(ex);
+                }
                 finally
                 {
+                    // Any terminal outcome (success, hub error, unreadable response, or a throwing user callback) ends the operation, so stop tracking it locally
+                    if (requestId != null && hasRequestFinished)
+                    {
+                        _pendingCertificateSigningOperations.TryRemove(requestId, out _);
+                    }
+
                     await args.AcknowledgeAsync(CancellationToken.None);
                 }
             }