From 78542ab2d21d73b502940ca683adf0a4c4c2ff21 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 12:22:53 -0400 Subject: [PATCH] review(Driver.Historian.Wonderware.Client.Contracts): redact SharedSecret in ToString (High) First review at 7286d320. -001 (High): record ToString() leaked SharedSecret into logs -> override ToString() that omits it. -003 (Medium): [Range(1,65535)] on Port. -004 thumbprint doc. -002 (SHA-1 pin -> SHA-256, lives in FrameChannel) deferred cross-module. --- .../findings.md | 139 ++++++++++++++++++ .../WonderwareHistorianClientOptions.cs | 26 +++- 2 files changed, 160 insertions(+), 5 deletions(-) create mode 100644 code-reviews/Driver.Historian.Wonderware.Client.Contracts/findings.md diff --git a/code-reviews/Driver.Historian.Wonderware.Client.Contracts/findings.md b/code-reviews/Driver.Historian.Wonderware.Client.Contracts/findings.md new file mode 100644 index 00000000..972f19a2 --- /dev/null +++ b/code-reviews/Driver.Historian.Wonderware.Client.Contracts/findings.md @@ -0,0 +1,139 @@ +# Code Review — Driver.Historian.Wonderware.Client.Contracts + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Contracts` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `a19b0f86` | +| 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.Historian.Wonderware.Client.Contracts-003 | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | Driver.Historian.Wonderware.Client.Contracts-001, Driver.Historian.Wonderware.Client.Contracts-002 | +| 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 test project — per review instructions; verified by build only | +| 10 | Documentation & comments | Driver.Historian.Wonderware.Client.Contracts-004 | + +## Findings + +### Driver.Historian.Wonderware.Client.Contracts-001 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Security | +| Location | `WonderwareHistorianClientOptions.cs:24-30` | +| Status | Resolved | + +**Description:** `WonderwareHistorianClientOptions` is a `sealed record`. The C# compiler +auto-generates a `ToString()` for records that includes every positional constructor +parameter by name and value — in this case including `SharedSecret`. If the options object +is ever passed to structured logging (e.g. `_logger.LogDebug("{opts}", options)`), printed +in an exception message, surfaced in a health-check endpoint, or serialized by a framework +that calls `ToString()`, the shared secret will appear in plain text in log files or HTTP +responses. There is currently no `ToString()` override to redact it. The +`FrameChannel.ConnectInternalAsync` log line in the sibling project does not log `_options` +directly, so there is no active leak today, but the type is a latent trap for any future +logging addition. + +**Recommendation:** Override `ToString()` to redact the secret, including only the +non-sensitive connection fields (Host, Port, PeerName, UseTls, ServerCertThumbprint). + +**Resolution:** Resolved 2026-06-19 — added `override ToString()` to the record that +outputs Host, Port, PeerName, UseTls, and ServerCertThumbprint only, omitting +SharedSecret; verified by build (no test project for this module). `dotnet build` on both +the Contracts project and the consumer Client project passes with 0 errors/warnings. + +--- + +### Driver.Historian.Wonderware.Client.Contracts-002 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Security | +| Location | `WonderwareHistorianClientOptions.cs:50-54` | +| Status | Deferred | + +**Description:** `ServerCertThumbprint` is documented as a "SHA-1 thumbprint". The +consumer (`FrameChannel.DefaultTcpConnectFactory`, sibling project) pins via the no-arg +`GetCertHashString()` overload, which returns a 40-character SHA-1 hex string. SHA-1 is +a deprecated hash algorithm for certificate identification. An operator following current +tooling (Windows Certificate Manager, `certutil -sha256`, `openssl x509 -fingerprint +-sha256`) would supply a 64-character SHA-256 thumbprint, which will silently never match +and cause every TLS handshake to fail with a generic auth error and no indication that the +algorithm mismatch is the cause. + +**Recommendation:** Upgrade the consumer to `cert.GetCertHashString(HashAlgorithmName.SHA256)` +and update the doc to require a 64-char SHA-256 thumbprint. This requires a coordinated +change to `FrameChannel.DefaultTcpConnectFactory` in the `Client` project (not a +wire-contract change). Until then the doc has been updated (finding 004) to explicitly warn +operators that only 40-char SHA-1 thumbprints are accepted. + +**Resolution:** Deferred — the fix requires editing `FrameChannel.DefaultTcpConnectFactory` +in the sibling `Client` project, outside this Contracts module's scope. The documentation +has been updated (finding 004 fix) to warn operators of the SHA-1-only constraint. Track +as a follow-up in the `Client` module's review. + +--- + +### Driver.Historian.Wonderware.Client.Contracts-003 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `WonderwareHistorianClientOptions.cs:26` | +| Status | Resolved | + +**Description:** The `Port` positional constructor parameter has no `[Range]` validation +attribute (unlike `ProbeTimeoutSeconds`, which carries `[Range(1, 60)]`). A misconfigured +zero, negative, or out-of-range port (valid TCP range 1–65535) passes construction +silently. The consumer then fails at `TcpClient.ConnectAsync` time with a `SocketException` +whose error gives no hint that the port value is invalid. The sibling +`ServerHistorianOptions.Validate()` warns on `Port <= 0` at startup, but that fires only +if the read-path historian is wired; write-path-only or probe-only usage has no guard. + +**Recommendation:** Add `[Range(1, 65535)]` on the `Port` positional parameter. + +**Resolution:** Resolved 2026-06-19 — added `[Range(1, 65535)]` to the `Port` positional +parameter; also updated the `` XML doc to state the valid range. +Build passes with 0 errors/warnings on both the Contracts and consumer Client projects. + +--- + +### Driver.Historian.Wonderware.Client.Contracts-004 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `WonderwareHistorianClientOptions.cs:50-54` | +| Status | Resolved | + +**Description:** The `ServerCertThumbprint` XML doc said "SHA-1 thumbprint (hex, no +spaces)" but did not state the expected length (40 hex characters for SHA-1). An operator +supplying a SHA-256 thumbprint (64 hex chars — the format shown by modern tooling) would +get a silent TLS handshake failure. The doc also did not explain which hash algorithm the +consumer uses or why SHA-256 thumbprints will not work. + +**Recommendation:** Update the XML doc to state the 40-character SHA-1 constraint +explicitly and warn that SHA-256 thumbprints will silently fail. + +**Resolution:** Resolved 2026-06-19 — updated the `ServerCertThumbprint` XML doc to state +"40 hex characters, no spaces, case-insensitive" and added a `` block explaining +that the consumer uses `GetCertHashString()` (SHA-1) and that a 64-char SHA-256 thumbprint +will silently never match. Build passes with 0 errors/warnings. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Contracts/WonderwareHistorianClientOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Contracts/WonderwareHistorianClientOptions.cs index 4e7c7257..afe856ee 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Contracts/WonderwareHistorianClientOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Contracts/WonderwareHistorianClientOptions.cs @@ -16,14 +16,14 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client; /// /// /// Sidecar TCP host (DNS name or IP) the client dials. -/// Sidecar TCP port (matches the sidecar's OTOPCUA_HISTORIAN_TCP_PORT). +/// Sidecar TCP port (matches the sidecar's OTOPCUA_HISTORIAN_TCP_PORT). Valid range: 1–65535. /// Per-process shared secret the sidecar will verify in the Hello frame. /// Diagnostic peer identifier sent in Hello — typically the OtOpcUa instance id. /// Cap on the TCP connect + Hello round trip on each (re)connect. /// Cap on a single read/write call once connected. public sealed record WonderwareHistorianClientOptions( string Host, - int Port, + [Range(1, 65535)] int Port, string SharedSecret, string PeerName = "OtOpcUa", TimeSpan? ConnectTimeout = null, @@ -47,9 +47,25 @@ public sealed record WonderwareHistorianClientOptions( public bool UseTls { get; init; } /// - /// Optional SHA-1 thumbprint (hex, no spaces) the client pins the sidecar's TLS server - /// cert against. When null/empty and is true, the client validates - /// the cert chain normally (CA-issued cert). + /// Optional SHA-1 thumbprint (40 hex characters, no spaces, case-insensitive) the client + /// pins the sidecar's TLS server cert against. When null/empty and + /// is true, the client validates the cert chain normally + /// (CA-issued cert). /// + /// + /// The consumer matches against X509Certificate.GetCertHashString() (SHA-1, 40 + /// hex chars). Supplying a SHA-256 thumbprint (64 hex chars, the format shown by modern + /// tooling such as certutil or Windows Certificate Manager) will never match and + /// will cause the TLS handshake to fail silently. Only 40-character SHA-1 hex strings + /// are accepted. + /// public string? ServerCertThumbprint { get; init; } + + /// + /// + /// Redacts so the value cannot appear in log output when the + /// options object is passed to a structured-logging statement. + /// + public override string ToString() => + $"WonderwareHistorianClientOptions {{ Host={Host}, Port={Port}, PeerName={PeerName}, UseTls={UseTls}, ServerCertThumbprint={ServerCertThumbprint ?? ""} }}"; }