Joseph Doherty
2545bd7f7d
Records the disposition and rationale for each: the primary-host gate is no longer disposed (matching MqttConnection's documented rule), a reconnect that resolves zero filters for live topics now throws like the zero-granted case it is indistinguishable from, and a password configured without a username is warned about rather than silently dropped. Both module headers drop to 0 open.
8.3 KiB
Code Review — Driver.Mqtt.Transport
| Field | Value |
|---|---|
| Module | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Transport |
| Reviewer | Claude Code |
| Review date | 2026-08-10 |
| Commit reviewed | b990adae |
| Status | Reviewed |
| Open findings | 0 |
Checklist coverage
A comprehensive review completes every category, recording "No issues found" where a category produced nothing rather than leaving it blank.
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | Driver.Mqtt.Transport-002 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | No issues found |
| 4 | Error handling & resilience | No issues found |
| 5 | Security | Driver.Mqtt.Transport-001 |
| 6 | Performance & resource management | No issues found |
| 7 | Design-document adherence | No issues found |
| 8 | Code organization & conventions | No issues found |
| 9 | Testing coverage | No issues found |
| 10 | Documentation & comments | No issues found |
Scope
One hand-written file, MqttClientOptionsFactory.cs (275 LOC), plus the csproj. The module is the
leaf extracted by Gitea #512 so the runtime driver and the browse session assemble a broker
connection through one copy of the TLS posture, the PEM CA-pin chain validator, the credential
handling and the protocol-version mapping — a second copy would be a silent security divergence
(browse quietly not honouring a pinned CA while the driver does). Nothing under bin/ or obj/
was reviewed.
Notes on the categories that produced nothing
Recorded rather than left implicit, because in a 275-line file "no issues" is a claim that should be checkable:
- Concurrency (3). The type is static and pure; the only mutable state is the
Lazy<X509Certificate2Collection?>captured per validator, built withLazyThreadSafetyMode.ExecutionAndPublication, and one is created perBuildClientOptionscall — i.e. per connect attempt — which is what makes a rotated CA file get picked up on the next connect rather than pinned for process life. - Error handling (4). Every failure on the pin path fails closed: an unreadable or empty PEM
returns null and the validator refuses; a
CryptographicExceptionout ofX509Chain.Buildis caught and refused rather than escaping into the TLS callback as an opaque handshake crash; a non-chainSslPolicyErrorsbit (hostname mismatch, absent certificate) stays fatal, so the pin replaces the trust anchor only. - Performance (6). One chain build per handshake,
owned?.Dispose()in afinallyfor the copy made when the presented certificate is not already anX509Certificate2. No I/O on the build path at all, which is the property that lets a config-validation caller use it. - Testing (9). There is no
Driver.Mqtt.Transport.Testsproject;BuildClientOptionsis covered fromDriver.Mqtt.Tests(MqttConnectionTestsdrives the real validator against real self-signed chains — pinned CA accepted, stranger rejected, missing PEM fails closed, no I/O at build time — andMqttBrowseSessionTestscovers the client-id suffix and forced clean session). That is genuine coverage of this module's behaviour through its two consumers, which is the shape the extraction was for; not recorded as a finding. - Conventions / design (2, 7, 8). Namespace
ZB.MOM.WW.OtOpcUa.Driver.Mqtt(deliberately the same as.Contractsand.Driver, so the extraction was not a source-breaking change), .NET 10,TreatWarningsAsErrors, no MQTTnet leak into.Contracts. The csproj comment states the module's charter — connection policy only, nothing that connects — and the file honours it.
Findings
Driver.Mqtt.Transport-001
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Security |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Transport/MqttClientOptionsFactory.cs:120-128 (ConfigureTls) |
| Status | Resolved |
Description: AllowUntrustedServerCertificate and CaCertificatePath are independent settings,
and when both are set the hatch wins silently: the branch installs
WithAllowUntrustedCertificates(true) + WithIgnoreCertificateChainErrors(true) +
static _ => true and returns before the pin is ever consulted. Every broker certificate is
accepted, pinned chain or not, and nothing in any log says so.
The combination is not exotic. allowUntrustedServerCertificate is the dev/on-prem escape hatch an
operator turns on while standing a broker up, and caCertificatePath is what they add afterwards to
make MITM fail closed; forgetting to clear the first leaves a deployment that looks pinned in its
config, passes review, and validates nothing. The failure is silent in the direction that matters —
the operator's belief is "the pin is in force" and the reality is "any certificate is accepted" —
and neither the driver's health surface nor Test-connect can distinguish the two.
Both knobs are explicit, so the fix is not to second-guess which one the operator meant: an implementation that let the pin override the flag would break a deployment that genuinely wants the hatch, and one that refused to build would fail a driver at connect over a config it has been accepting. Saying so, once, at the seam that resolves the conflict is the proportionate answer.
Recommendation: Warn (naming the pin path that is not being enforced) when
AllowUntrustedServerCertificate is set and CaCertificatePath is non-blank, and leave the flag
authoritative. The factory already takes an optional ILogger for exactly this class of deferred
diagnostic, and never formats credentials.
Resolution: Fixed 74b85271, 2026-08-10 — ConfigureTls logs a warning naming the shadowed CA
path before taking the accept-everything branch; behaviour is otherwise unchanged. Verified no
existing assertion depends on the branch being silent (713 tests green).
Driver.Mqtt.Transport-002
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Transport/MqttClientOptionsFactory.cs:97-100 |
| Status | Resolved |
Description: Credentials are attached only when Username is non-empty:
if (!string.IsNullOrEmpty(options.Username)) builder.WithCredentials(options.Username, options.Password ?? "").
A config carrying a Password and no Username therefore connects anonymously, with the
configured secret silently dropped. On MQTT 3.1.1 that is protocol-correct (a password may not be
sent without a username), but this driver defaults to ProtocolVersion = V500, where a
password-only CONNECT is legal and is how some brokers carry a token.
Impact is bounded — the broker either accepts the anonymous connect (in which case the deployment is less authenticated than its config says) or refuses it, which now surfaces as a classified CONNACK rejection rather than a mystery. It is recorded because the silent-drop direction is the one an operator cannot see from the config.
Recommendation: Warn when Password is non-empty and Username is blank, so the discarded
credential is visible; optionally send password-only credentials under V500, where the protocol
permits it. Do not log the password itself — nothing on this path formats it today, and the
options record's PrintMembers override exists to keep it that way.
Resolution (2026-08-10): Fixed 5d4dd17d — the recommendation's first half only. A password
configured with a blank username now logs a warning saying the connection is being made
anonymously and naming the two ways to make that intentional; the credential is still not sent,
and the secret is never formatted into the message. The optional half (a password-only CONNECT
under V500) was deliberately not taken: 3.1.1 forbids it, so it cannot be the uniform
behaviour of a factory that serves both versions, and adding it under V500 alone would change the
wire behaviour of deployments that connect happily today — the same "the explicit config stays
authoritative, say so rather than second-guess it" line Driver.Mqtt.Transport-001 settled on.
TDD: the warning assertion failed RED (log.CountContaining("password") should be 1 but was 0)
before the change. Paired with a falsifying control — a config carrying neither credential must
stay silent, or the fix would warn on every ordinary anonymous deployment and the first test would
still pass. Suite green (734).