diff --git a/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningOperation.cs b/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningOperation.cs index 410ed270..0aaefb53 100644 --- a/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningOperation.cs +++ b/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. /// /// - /// If the operation fails at any point, both and will - /// throw a when awaited. The exception contains - /// structured error details such as , - /// , and for 409005 conflict errors, - /// . - /// - /// 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. - /// } - /// + /// If the operation fails before completes successfully, both tasks fault with the + /// operation's exception. If has already completed successfully, a later failure + /// affects only . If the completion callback throws, faults + /// with the callback's exception. Cancellation cancels each task that has not already completed. /// public class CertificateSigningOperation { @@ -48,7 +31,7 @@ private readonly TaskCompletionSource _completed /// The result contains the correlation ID and operation expiration time. /// /// - /// Thrown when the CSR is rejected by IoT Hub. Inspect + /// Thrown when the CSR is rejected by IoT Hub. Inspect /// for the specific failure reason (e.g., 400040 for CSR decode failure, 409005 for an active conflicting operation, /// 429002/429003 for throttling). /// @@ -59,9 +42,10 @@ private readonly TaskCompletionSource _completed /// The result contains the certificate chain and correlation ID. /// /// - /// 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 and - /// 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 completes. + /// If the callback throws, this + /// task fails with that exception instead. /// public Task Completed => _completed.Task; diff --git a/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningRequestErrorResponse.cs b/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningRequestErrorResponse.cs index 8e0f4af4..4f415b06 100644 --- a/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningRequestErrorResponse.cs +++ b/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningRequestErrorResponse.cs @@ -1,6 +1,8 @@ // 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 @@ -8,40 +10,75 @@ namespace Microsoft.Azure.Iot.Device.Models.CertificateManagement /// /// Represents an error response from IoT hub about a certificate signing request. /// + /// + /// 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. + /// public class CertificateSigningRequestErrorResponse { + /// + /// 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. + /// [JsonPropertyName("errorCode")] - public required int ErrorCode { get; set; } + public int ErrorCode { get; set; } + /// + /// A human readable description of the error. + /// [JsonPropertyName("message")] public string? Message { get; set; } + /// + /// The IoT hub tracking ID for this error, for support purposes. + /// [JsonPropertyName("trackingId")] - public required string TrackingId { get; set; } + public string? TrackingId { get; set; } + /// + /// When IoT hub generated this error. + /// [JsonPropertyName("timestampUtc")] - public required DateTimeOffset TimestampUtc { get; set; } + public DateTimeOffset? TimestampUtc { get; set; } + /// + /// Certificate management specific details about the error. + /// [JsonPropertyName("info")] - public required CertificateSigningRequestErrorInfo Info { get; set; } + public CertificateSigningRequestErrorInfo Info { get; set; } = new(); + /// + /// How long IoT hub asked this device to wait before trying again, if it asked at all. + /// [JsonPropertyName("retryAfter")] public int? RetryAfterSeconds { get; set; } + /// + /// Certificate management specific details about a certificate signing request error. + /// public class CertificateSigningRequestErrorInfo { + /// + /// Correlation ID for diagnostic and support purposes. + /// [JsonPropertyName("correlationId")] - public required string CorrelationId { get; set; } + public string? CorrelationId { get; set; } + /// + /// The certificate management specific error (for example, "CertificateSigningRequestInvalid"). + /// [JsonPropertyName("credentialError")] - public required string CertificateSigningRequestError { get; set; } + public string? CredentialError { get; set; } + /// + /// A human readable description of . + /// [JsonPropertyName("credentialMessage")] - public string? CertificateSigningRequestMessage { get; set; } - - [JsonPropertyName("requestId")] - public required string RequestId { get; set; } + public string? CredentialMessage { get; set; } + /// + /// When the operation this error refers to expires, if IoT hub reported it. + /// [JsonPropertyName("operationExpires")] public DateTimeOffset? OperationExpires { get; set; } } diff --git a/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningRequestFailedException.cs b/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningRequestFailedException.cs index cb8fd295..6dcc8f52 100644 --- a/dotnet/src/Microsoft.Azure.Iot.Device/Models/CertificateManagement/CertificateSigningRequestFailedException.cs +++ b/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; } } } diff --git a/dotnet/src/Microsoft.Azure.Iot.Device/Unified/Connection/ConnectionClient.cs b/dotnet/src/Microsoft.Azure.Iot.Device/Unified/Connection/ConnectionClient.cs index 041e10f7..2e34f182 100644 --- a/dotnet/src/Microsoft.Azure.Iot.Device/Unified/Connection/ConnectionClient.cs +++ b/dotnet/src/Microsoft.Azure.Iot.Device/Unified/Connection/ConnectionClient.cs @@ -116,6 +116,7 @@ public override async Task HandleConnectedToHubAsync(MqttClientConnectedEventArg /// 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. public async Task 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(args.Publish.Payload)!; + CertificateSigningRequestAccepted accepted = JsonSerializer.Deserialize(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(args.Publish.Payload)!; + hasRequestFinished = true; + CertificateSigningResponse response = JsonSerializer.Deserialize(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(args.Publish.Payload)!; - pendingCertificateSigningOperation.SetFailed(new CertificateSigningRequestFailedException() { Error = error }); + hasRequestFinished = true; + CertificateSigningRequestErrorResponse error = JsonSerializer.Deserialize(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); } } diff --git a/dotnet/tests/Microsoft.Azure.Iot.Device.UnitTests/Unified/ConnectionClientUnitTests.cs b/dotnet/tests/Microsoft.Azure.Iot.Device.UnitTests/Unified/ConnectionClientUnitTests.cs index af8d78a7..b5ebac3b 100644 --- a/dotnet/tests/Microsoft.Azure.Iot.Device.UnitTests/Unified/ConnectionClientUnitTests.cs +++ b/dotnet/tests/Microsoft.Azure.Iot.Device.UnitTests/Unified/ConnectionClientUnitTests.cs @@ -4,6 +4,7 @@ using Microsoft.Azure.Iot.Device.Unified.Connection; using Microsoft.Azure.Iot.Device.Unified.Telemetry; using Microsoft.Azure.Iot.Device.Models; +using Microsoft.Azure.Iot.Device.Models.CertificateManagement; using Microsoft.Azure.Iot.Device.Mqtt; using Xunit; @@ -24,6 +25,248 @@ private static ConnectionContext GetMockConnectionContext(bool isMQTTv5) #pragma warning restore SYSLIB0026 // Type or member is obsolete } + private const string AcceptedPayload = "{\"correlationId\":\"someCorrelationId\",\"operationExpires\":\"2030-01-01T00:00:00Z\"}"; + + private static readonly TimeSpan s_certSigningTimeout = TimeSpan.FromSeconds(10); + + private static async Task<(ConnectionClient Client, MockMqttClient Mqtt, CertificateSigningOperation Operation, string RequestId)> StartCertificateSigningAsync( + Func, Task>? completeCallback = null) + { + MockMqttClient mockMqttClient = new(false); + ConnectionClient connectionClient = new(new() { MqttClient = mockMqttClient }); + connectionClient.HandleCertificateSigningCompleteAsync = completeCallback; + await connectionClient.ConnectToHubAsync(GetMockConnectionContext(false), cancellationToken: TestContext.Current.CancellationToken); + + IotHubCertificateSigningRequest request = new("someDeviceId", "c29tZWNzcg=="); + CertificateSigningOperation operation = await connectionClient.SendCertificateSigningRequestAsync(request, TestContext.Current.CancellationToken); + return (connectionClient, mockMqttClient, operation, request.RequestId); + } + + // Delivers a response from IoT hub. Exceptions thrown by the handler are swallowed, as the MQTT layer would do, so tests can assert on the operation's tasks instead. + private static async Task DeliverCertificateSigningResponseAsync(MockMqttClient mqtt, string status, string requestId, string payload) + { + try + { + await mqtt.SimulateNewMessageAsync(new MqttPublish() + { + Topic = $"$iothub/credentials/res/{status}/?$rid={requestId}", + Payload = System.Text.Encoding.UTF8.GetBytes(payload), + }); + } + catch (Exception) + { + } + } + + private static async Task AssertFailsAsync(Task task) + { + return await Assert.ThrowsAsync( + () => task.WaitAsync(s_certSigningTimeout, TestContext.Current.CancellationToken)); + } + + [Theory] + [InlineData("400", 400040)] + [InlineData("409", 409005)] + [InlineData("429", 429002)] + [InlineData("500", 500001)] + public async Task CertificateSigningErrorBeforeAcceptancePropagatesToBothTasks(string status, int errorCode) + { + var (client, mqtt, operation, requestId) = await StartCertificateSigningAsync(); + using (client) + { + string payload = $"{{\"errorCode\":{errorCode},\"message\":\"some message\",\"trackingId\":\"tid\",\"retryAfter\":5}}"; + await DeliverCertificateSigningResponseAsync(mqtt, status, requestId, payload); + + var acceptedEx = await AssertFailsAsync(operation.Accepted); + var completedEx = await AssertFailsAsync(operation.Completed); + + Assert.Equal(errorCode, acceptedEx.Error.ErrorCode); + Assert.Equal("some message", acceptedEx.Error.Message); + Assert.Equal(5, acceptedEx.Error.RetryAfterSeconds); + Assert.Equal(errorCode, completedEx.Error.ErrorCode); + } + } + + [Fact] + public async Task CertificateSigningErrorAfterAcceptanceFailsOnlyCompletedTask() + { + var (client, mqtt, operation, requestId) = await StartCertificateSigningAsync(); + using (client) + { + await DeliverCertificateSigningResponseAsync(mqtt, "202", requestId, AcceptedPayload); + await operation.Accepted.WaitAsync(s_certSigningTimeout, TestContext.Current.CancellationToken); + + await DeliverCertificateSigningResponseAsync(mqtt, "500", requestId, "{\"errorCode\":500001,\"message\":\"issuance failed\"}"); + + var ex = await AssertFailsAsync(operation.Completed); + Assert.Equal(500001, ex.Error.ErrorCode); + + // Acceptance already succeeded, so a later failure must not rewrite it. + Assert.True(operation.Accepted.IsCompletedSuccessfully); + } + } + + [Fact] + public async Task CertificateSigningErrorWithMinimalPayloadStillFailsBothTasks() + { + var (client, mqtt, operation, requestId) = await StartCertificateSigningAsync(); + using (client) + { + await DeliverCertificateSigningResponseAsync(mqtt, "400", requestId, "{}"); + + var acceptedEx = await AssertFailsAsync(operation.Accepted); + var completedEx = await AssertFailsAsync(operation.Completed); + Assert.Equal(0, acceptedEx.Error.ErrorCode); + Assert.Same(acceptedEx, completedEx); + } + } + + [Theory] + [InlineData("400", "not json")] + [InlineData("400", "")] + [InlineData("400", "null")] + [InlineData("202", "not json")] + public async Task CertificateSigningUnreadableResponseFailsBothTasksInsteadOfHanging(string status, string payload) + { + var (client, mqtt, operation, requestId) = await StartCertificateSigningAsync(certs => Task.FromResult(GetMockConnectionContext(false).AuthenticationProvider)); + using (client) + { + await DeliverCertificateSigningResponseAsync(mqtt, status, requestId, payload); + + await AssertFailsAsync(operation.Accepted); + await AssertFailsAsync(operation.Completed); + } + } + + [Fact] + public async Task CertificateSigningCompleteCallbackThrowingFailsCompletedTask() + { + InvalidOperationException callbackException = new("callback failed"); + var (client, mqtt, operation, requestId) = await StartCertificateSigningAsync(certs => throw callbackException); + using (client) + { + await DeliverCertificateSigningResponseAsync(mqtt, "202", requestId, AcceptedPayload); + await operation.Accepted.WaitAsync(s_certSigningTimeout, TestContext.Current.CancellationToken); + + await DeliverCertificateSigningResponseAsync(mqtt, "200", requestId, "{\"certificates\":[\"cert1\"],\"correlationId\":\"someCorrelationId\"}"); + + var ex = await Assert.ThrowsAsync( + () => operation.Completed.WaitAsync(s_certSigningTimeout, TestContext.Current.CancellationToken)); + Assert.Same(callbackException, ex); + Assert.Equal(0, GetPendingOperationCount(client)); + } + } + + [Fact] + public async Task CertificateSigningTerminalOutcomesClearPendingOperationCache() + { + var (client, mqtt, operation, requestId) = await StartCertificateSigningAsync(certs => Task.FromResult(GetMockConnectionContext(false).AuthenticationProvider)); + using (client) + { + await DeliverCertificateSigningResponseAsync(mqtt, "202", requestId, AcceptedPayload); + Assert.Equal(1, GetPendingOperationCount(client)); // Still in progress + + await DeliverCertificateSigningResponseAsync(mqtt, "200", requestId, "{\"certificates\":[\"cert1\"],\"correlationId\":\"someCorrelationId\"}"); + await operation.Completed.WaitAsync(s_certSigningTimeout, TestContext.Current.CancellationToken); + Assert.Equal(0, GetPendingOperationCount(client)); + + IotHubCertificateSigningRequest failing = new("someDeviceId", "c29tZWNzcg=="); + CertificateSigningOperation failingOperation = await client.SendCertificateSigningRequestAsync(failing, TestContext.Current.CancellationToken); + Assert.Equal(1, GetPendingOperationCount(client)); + await DeliverCertificateSigningResponseAsync(mqtt, "400", failing.RequestId, "{\"errorCode\":400040}"); + await AssertFailsAsync(failingOperation.Completed); + Assert.Equal(0, GetPendingOperationCount(client)); + } + } + + private static int GetPendingOperationCount(ConnectionClient client) + { + var field = typeof(ConnectionClient).GetField("_pendingCertificateSigningOperations", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)!; + return ((System.Collections.IDictionary)field.GetValue(client)!).Count; + } + + [Fact] + public async Task CertificateSigningErrorForOneRequestDoesNotAffectAnother() + { + var (client, mqtt, failingOperation, failingId) = await StartCertificateSigningAsync(); + using (client) + { + IotHubCertificateSigningRequest otherRequest = new("someDeviceId", "c29tZWNzcg=="); + CertificateSigningOperation otherOperation = await client.SendCertificateSigningRequestAsync(otherRequest, TestContext.Current.CancellationToken); + + await DeliverCertificateSigningResponseAsync(mqtt, "400", failingId, "{\"errorCode\":400040}"); + + await AssertFailsAsync(failingOperation.Accepted); + await AssertFailsAsync(failingOperation.Completed); + Assert.False(otherOperation.Accepted.IsCompleted); + Assert.False(otherOperation.Completed.IsCompleted); + } + } + + [Fact] + public async Task CertificateSigningResponseForUnknownRequestIsIgnored() + { + var (client, mqtt, operation, requestId) = await StartCertificateSigningAsync(); + using (client) + { + await DeliverCertificateSigningResponseAsync(mqtt, "400", "unknown-request-id", "{\"errorCode\":400040}"); + + Assert.False(operation.Accepted.IsCompleted); + Assert.False(operation.Completed.IsCompleted); + } + } + + [Fact] + public async Task CertificateSigningRequestPublishFailureThrowsToCaller() + { + MockMqttClient mockMqttClient = new(false); + using ConnectionClient connectionClient = new(new() { MqttClient = mockMqttClient }); + await connectionClient.ConnectToHubAsync(GetMockConnectionContext(false), cancellationToken: TestContext.Current.CancellationToken); + + mockMqttClient.OnPublishAttempt += _ => Task.FromResult(new MqttPublishAck() { ReasonCode = MqttPublishAckReasonCode.NotAuthorized }); + + await Assert.ThrowsAnyAsync(() => connectionClient.SendCertificateSigningRequestAsync( + new IotHubCertificateSigningRequest("someDeviceId", "c29tZWNzcg=="), + TestContext.Current.CancellationToken)); + } + + [Theory] + [InlineData("400", "{\"errorCode\":400040}")] + [InlineData("409", "{\"errorCode\":409005,\"message\":\"conflict\"}")] + [InlineData("429", "{}")] + [InlineData("400", "not json")] + [InlineData("202", "not json")] + public async Task CertificateSigningFailureCarriesRequestIdFromTopic(string status, string payload) + { + var (client, mqtt, operation, requestId) = await StartCertificateSigningAsync(); + using (client) + { + await DeliverCertificateSigningResponseAsync(mqtt, status, requestId, payload); + + var acceptedEx = await AssertFailsAsync(operation.Accepted); + var completedEx = await AssertFailsAsync(operation.Completed); + Assert.Equal(requestId, acceptedEx.RequestId); + Assert.Equal(requestId, completedEx.RequestId); + } + } + + [Fact] + public async Task CertificateSigningFailureRequestIdMatchesTheFailingRequestWhenMultiplePending() + { + var (client, mqtt, firstOperation, firstId) = await StartCertificateSigningAsync(); + using (client) + { + IotHubCertificateSigningRequest secondRequest = new("someDeviceId", "c29tZWNzcg==", "second-request-id"); + CertificateSigningOperation secondOperation = await client.SendCertificateSigningRequestAsync(secondRequest, TestContext.Current.CancellationToken); + + await DeliverCertificateSigningResponseAsync(mqtt, "409", secondRequest.RequestId, "{\"errorCode\":409005}"); + await DeliverCertificateSigningResponseAsync(mqtt, "400", firstId, "{\"errorCode\":400040}"); + + Assert.Equal("second-request-id", (await AssertFailsAsync(secondOperation.Completed)).RequestId); + Assert.Equal(firstId, (await AssertFailsAsync(firstOperation.Completed)).RequestId); + } + } + [Fact] public async Task ProvisionAndConnectUsesSeededConnectionContext() {