diff --git a/code-reviews/Driver.Mqtt.Transport/findings.md b/code-reviews/Driver.Mqtt.Transport/findings.md new file mode 100644 index 00000000..8aa803d5 --- /dev/null +++ b/code-reviews/Driver.Mqtt.Transport/findings.md @@ -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` 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)_