lmxopcua/code-reviews/Driver.Mqtt.Transport/findings.md

# 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 with
  `LazyThreadSafetyMode.ExecutionAndPublication`, and one is created per `BuildClientOptions` call
  — 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 `CryptographicException` out of `X509Chain.Build` is
  caught and refused rather than escaping into the TLS callback as an opaque handshake crash; a
  non-chain `SslPolicyErrors` bit (hostname mismatch, absent certificate) stays fatal, so the pin
  replaces the trust *anchor* only.
- **Performance (6).** One chain build per handshake, `owned?.Dispose()` in a `finally` for the
  copy made when the presented certificate is not already an `X509Certificate2`. 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.Tests` project; `BuildClientOptions` is
  covered from `Driver.Mqtt.Tests` (`MqttConnectionTests` drives the real validator against real
  self-signed chains — pinned CA accepted, stranger rejected, missing PEM fails closed, no I/O at
  build time — and `MqttBrowseSessionTests` covers 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 `.Contracts` and `.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).