dohertj2/lmxopcua
1
0
Fork 0
Code Issues 20 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(mqtt): pin the CA accept path, honour presented intermediates, classify the dispose race

Browse Source 
Review follow-ups on MqttConnection (Task 3):

- Every certificate test asserted rejection, so the accept branch was
  unreachable-by-regression. Adds a leaf genuinely issued by the pinned CA and
  asserts acceptance.
- ValidateAgainstPinnedCa never seeded ChainPolicy.ExtraStore from the incoming
  chain, so a leaf behind an intermediate delivered during the handshake failed
  despite a legitimate path to the pinned root. Seeds from both the incoming
  chain's elements and its ExtraStore; CustomRootTrust still means only the
  pinned roots may terminate the chain.
- A DisposeAsync racing an in-flight connect escaped as an unclassified
  exception; it now folds into ObjectDisposedException.
- Promotes the single-caller concurrency invariant into the type remarks, with
  the accurate blast radius (a leaked live connection, not a benign throw).
  Serialising the lifecycle remains Task 4's job.
- X509Chain.Build can throw; an exception escaping a TLS validation callback is
  an opaque handshake crash, so it is caught and refused.
- Adds a connect-retry test (Task 4's reconnect loop reuses the instance) and a
  disposed-then-connect test.

Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
This commit is contained in:
Joseph Doherty
2026-07-24 14:38:16 -04:00
parent ded4c41798
commit 5629f3698d
2 changed files with 239 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttConnection.cs
+69 -3
View File
@@ -1,4 +1,5 @@
using System.Net.Security;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using Microsoft.Extensions.Logging;
using MQTTnet;
@@ -29,6 +30,18 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt;
/// <see cref="MqttDriverOptions.Password"/>; the options record itself redacts it in
/// <c>ToString()</c>.
/// </para>
/// <para>
/// <b>NOT thread-safe — single-caller by contract.</b> <see cref="ConnectAsync"/> and
/// <see cref="DisposeAsync"/> take no lock and must never run concurrently. Calling them
/// concurrently <b>leaks a live broker connection</b>: <see cref="DisposeAsync"/> can
/// observe <c>_disposed == false</c>, set it, exchange a still-null <c>_client</c> for
/// null, and return having disposed nothing — after which the in-flight
/// <see cref="ConnectAsync"/> creates a client, assigns it and connects successfully.
/// That client holds an open socket that no later <see cref="DisposeAsync"/> can ever
/// reach, because <c>_disposed</c> is already <c>true</c>. Serialising the lifecycle is
/// the job of the reconnect supervisor added in Task 4; it is deliberately not solved
/// here.
/// </para>
/// Reconnect/backoff (Task 4) and subscription (Task 6) are deliberately not implemented here.
/// </remarks>
public sealed class MqttConnection : IAsyncDisposable
@@ -96,16 +109,24 @@ public sealed class MqttConnection : IAsyncDisposable
/// Connects to the broker, failing at <see cref="MqttDriverOptions.ConnectTimeoutSeconds"/>
/// rather than waiting indefinitely on an unresponsive peer.
/// </summary>
/// <remarks>
/// May be retried on the same instance after a failed attempt — the underlying
/// <see cref="IMqttClient"/> is created once and reused across attempts, which is what the
/// Task-4 reconnect loop depends on. Not safe to call concurrently with itself or with
/// <see cref="DisposeAsync"/>; see the type-level remarks.
/// </remarks>
/// <param name="cancellationToken">Caller cancellation; linked with the connect deadline.</param>
/// <exception cref="TimeoutException">The connect deadline elapsed.</exception>
/// <exception cref="OperationCanceledException"><paramref name="cancellationToken"/> was cancelled.</exception>
/// <exception cref="ObjectDisposedException">
/// The connection was disposed, either before the attempt started or while it was in flight.
/// </exception>
public async Task ConnectAsync(CancellationToken cancellationToken)
{
ObjectDisposedException.ThrowIf(_disposed, this);
// Single-caller by contract: the reconnect supervisor (Task 4) owns connect/disconnect
// sequencing, so no lock is taken here. The options are rebuilt per attempt so a rotated
// CA file is picked up on the next connect rather than being pinned for the process life.
// The options are rebuilt per attempt so a rotated CA file is picked up on the next
// connect rather than being pinned for the life of the process.
var client = _client ??= new MqttClientFactory().CreateMqttClient();
var clientOptions = BuildClientOptions(_options, clientIdSuffix: null, _logger);
@@ -124,6 +145,18 @@ public sealed class MqttConnection : IAsyncDisposable
{
await client.ConnectAsync(clientOptions, linked.Token).ConfigureAwait(false);
}
// A concurrent DisposeAsync disposes the client out from under the awaiting connect, and
// whatever MQTTnet throws for that matches neither token filter. Classify it rather than
// letting an undocumented exception escape the contract below.
catch (Exception ex) when (_disposed)
{
throw new ObjectDisposedException(
nameof(MqttConnection),
new InvalidOperationException(
$"MQTT driver '{_driverId}': connect to {_options.Host}:{_options.Port} was aborted because the "
+ "connection was disposed while the attempt was in flight.",
ex));
}
// MQTTnet wraps a cancelled connect in MqttConnectingFailedException rather than letting
// the OperationCanceledException surface, so the two legs are told apart by which token
// fired, not by the exception type. Caller intent wins over the deadline.
@@ -297,8 +330,30 @@ public sealed class MqttConnection : IAsyncDisposable
using var chain = new X509Chain();
chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
chain.ChainPolicy.CustomTrustStore.AddRange(trustedRoots);
// Revocation is deliberately not checked: the pin exists precisely for self-issued
// dev / on-prem CAs, which typically publish no CRL or OCSP responder, so checking
// would fail every such chain. The pin itself is the revocation story — remove the CA.
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
// Seed the intermediates the broker presented during the handshake. A leaf signed by
// an intermediate that chains up to the pinned root is a common broker topology, and
// that intermediate arrives on the wire rather than being installed locally — without
// it in the candidate pool a legitimate trust path simply cannot be built.
// CustomRootTrust still means only `trustedRoots` may terminate the chain, so a rogue
// self-signed cert smuggled in here cannot become an anchor.
// Both sources are read: platforms differ in whether the peer-supplied intermediates
// land in the incoming chain's elements, its ExtraStore, or both.
if (args.Chain is not null)
{
foreach (var element in args.Chain.ChainElements)
{
chain.ChainPolicy.ExtraStore.Add(element.Certificate);
}
chain.ChainPolicy.ExtraStore.AddRange(args.Chain.ChainPolicy.ExtraStore);
}
if (chain.Build(presented))
{
return true;
@@ -310,6 +365,17 @@ public sealed class MqttConnection : IAsyncDisposable
string.Join(", ", chain.ChainStatus.Select(s => s.Status)));
return false;
}
catch (CryptographicException ex)
{
// X509Chain.Build throws on a malformed certificate or an unusable policy. An exception
// escaping a TLS validation callback surfaces as an opaque handshake crash, so classify
// it here and refuse: an unverifiable certificate is a rejected certificate.
logger?.LogError(
ex,
"MQTT broker certificate could not be validated against the pinned CA '{CaCertificatePath}'; rejecting.",
caCertificatePath);
return false;
}
finally
{
owned?.Dispose();