review(mqtt-transport): findings for Driver.Mqtt.Transport at b990adae (2 findings, 1 resolved in 74b85271, 1 open)

2026-08-10 06:59:38 -04:00
parent 74b852716c
commit fefb240350
1 changed files with 132 additions and 0 deletions
@@ -0,0 +1,132 @@
+# 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 | 1 |
+
+## 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 | Open |
+
+**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:** _(open)_