diff --git a/code-reviews/Driver.Galaxy/findings.md b/code-reviews/Driver.Galaxy/findings.md index 8135c74..15d910c 100644 --- a/code-reviews/Driver.Galaxy/findings.md +++ b/code-reviews/Driver.Galaxy/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-23 | | Commit reviewed | `a9be809` | | Status | Reviewed | -| Open findings | 4 | +| Open findings | 0 | ## Checklist coverage @@ -267,15 +267,32 @@ Medium, two Low. | Field | Value | |---|---| -| Severity | Medium | -| Category | Security | +| Severity | ~~Medium~~ Low (re-triaged 2026-05-23) | +| Category | ~~Security~~ Documentation & comments (re-triaged 2026-05-23) | | Location | `libs/MxGateway.Client.dll`, `libs/MxGateway.Contracts.dll`, `libs/README.md` | -| Status | Open | +| Status | Resolved | **Description:** Commit `994997b` checks in two binary DLLs (`MxGateway.Client.dll`, 99 840 bytes; `MxGateway.Contracts.dll`, 489 984 bytes) under `src/Drivers/.../Driver.Galaxy/libs/` and references them via ``. These are the only checked-in binary build artefacts in the entire repo (a repo-wide `find` for non-`bin/`/`obj/` `*.dll` under `libs/` returns only these two), so the change sets a precedent. The accompanying `libs/README.md` states the DLLs are "byte-for-byte the build output" of the OtOpcUa team's own code against the gateway's open proto contracts, but there is no recorded provenance — no source-commit SHA from the sibling `mxaccessgw` repo that produced the build, no SHA-256/SHA-512 checksum, no `.gitattributes` rule marking these paths as binary (so a future churn-in-place will balloon the pack file). Without a recorded source commit + checksum it is impossible for a future reviewer/auditor to verify the binaries match a specific revision of the sibling repo — the assertion "we built them, not external" is unverifiable after the fact. Tampering or accidental swap (e.g. someone drops in a different DLL of the same name under the same path) would not be detectable. **Recommendation:** (a) Pin the source provenance: add the sibling `mxaccessgw` commit SHA used to build each DLL to `libs/README.md`. (b) Record a SHA-256 of each `.dll` in `libs/README.md` so a future tamper or accidental update is detectable by running `Get-FileHash`/`sha256sum`. (c) Add a `.gitattributes` rule under `libs/` declaring `*.dll binary` (and consider `filter=lfs diff=lfs merge=lfs -text` if/when these need to be updated, to avoid bloating the pack file on every refresh). (d) Optional: a `dotnet test` time-check that compares the on-disk hash to the recorded hash, so a CI run notices if the file drifts from what the README claims. +**Resolution:** Resolved 2026-05-23. **Severity re-triage:** the original +finding framed this as a security concern about "tampering or accidental +swap by an unknown third party"; the user clarified that the DLLs are +their own code, built from their own `mxaccessgw` project — not third-party +binaries. That moves the concern from security (untrusted provenance) to +documentation (audit trail). Re-classified as Low Documentation & +Comments. Fix: `libs/README.md` now carries a Provenance section that +records the source-commit SHA (`dd7ca1634e2d2b8a866c81f0009bf87ee9427750`, +extracted from the `AssemblyInformationalVersion` baked into both DLLs by +the original build) and SHA-256 checksums of both binaries, plus a +re-verification recipe (`sha256sum libs/*.dll` + `ilspycmd | grep +AssemblyInformationalVersion`). Recommendations (c) `.gitattributes` and +(d) CI hash-check deferred — the DLLs are essentially frozen until one +of the two unwinding paths is taken, so adding LFS or a CI guard would +add infrastructure that the unwinding step would then have to remove. +Re-open if the vendoring becomes a recurring update target. + ### Driver.Galaxy-016 | Field | Value | @@ -283,12 +300,32 @@ Medium, two Low. | Severity | Medium | | Category | Performance & resource management | | Location | `ZB.MOM.WW.OtOpcUa.Driver.Galaxy.csproj:43-47`, `libs/README.md:32-37` | -| Status | Open | +| Status | Resolved | **Description:** The five new `PackageReference` versions declared in the csproj (`Google.Protobuf` 3.34.1, `Grpc.Core.Api` 2.76.0, `Grpc.Net.Client` 2.71.0, `Microsoft.Extensions.Logging.Abstractions` 10.0.0, `Polly` 8.5.2) do not all match what the vendored `MxGateway.Client.dll` was built against. The DLL's PE metadata (extracted via `System.Reflection.Metadata`) shows references to `Grpc.Net.Client v2.0.0.0`, `Microsoft.Extensions.Logging.Abstractions v10.0.0.0`, and notably `Polly.Core v8.0.0.0` — and the source csproj just before the sibling-repo rename (commit `bd4a09a` from 2026-04-27) declared `Grpc.Net.Client` 2.76.0, `Microsoft.Extensions.Logging.Abstractions` 10.0.7, and `Polly.Core` 8.6.6 — *not* the meta-package `Polly`. Our driver pulls `Polly` 8.5.2 (which transitively pins `Polly.Core` 8.5.2 per its nuspec dependency), so the vendored client actually loads `Polly.Core` 8.5.2 at runtime against code compiled against 8.6.6. Across an 8.5 ↔ 8.6 minor delta this is usually safe (assembly-version is `v8.0.0.0` for both), but it is exactly the skew shape that surfaces as `MissingMethodException` if a 8.6-only API was used in the client. `libs/README.md` claims "versions match what the sibling repo's `ZB.MOM.WW.MxGateway.Contracts.csproj` uses so the gRPC + proto runtime stays binary-compatible" — that statement is correct only for `Google.Protobuf` and `Grpc.Core.Api`; the other three packages do not match. **Recommendation:** Reconcile the declared package versions with what the vendored DLLs were built against — bump to `Grpc.Net.Client` 2.76.0, `Microsoft.Extensions.Logging.Abstractions` 10.0.7, swap `Polly` for `Polly.Core` 8.6.6 (the driver does not import the `Polly` legacy v7 surface, only Polly.Core via the client). Alternatively, rebuild the vendored DLLs against the same versions the csproj declares and refresh the binaries. Update `libs/README.md` to record the exact versions the DLLs were built against, so the next vendoring refresh has an authoritative reference. +**Resolution:** Resolved 2026-05-23 — took the first option (reconcile +declared packages with what the DLL was built against, verified by +reflecting `Assembly.GetReferencedAssemblies()` on `MxGateway.Client.dll`). +Changes to the csproj: **`Polly` 8.5.2 → `Polly.Core` 8.6.6** (the most +consequential — `Polly` (v7 fluent API) and `Polly.Core` (v8 resilience- +pipeline API) are different packages, and the DLL was built against +`Polly.Core`; the prior `Polly` reference would have failed at runtime +with `MissingMethodException` the first time the gateway client's retry +pipeline ran). Also bumped `Grpc.Net.Client` 2.71.0 → 2.76.0 and +`Microsoft.Extensions.Logging.Abstractions` 10.0.0 → 10.0.7 to match the +sibling Server/Worker projects' current versions. `Google.Protobuf` +3.34.1 and `Grpc.Core.Api` 2.76.0 already matched; left unchanged. +`libs/README.md` rewritten to record what was actually verified +(`Assembly.GetReferencedAssemblies()` output + the resolved package +versions, including the sibling Server/Worker csproj as the version +source-of-truth — the deleted MxGateway.Client.csproj would have been +the original source but no longer exists). Verification: solution-wide +`dotnet build` clean, Driver.Galaxy.Tests 245/245 pass against the +corrected package set. + ### Driver.Galaxy-017 | Field | Value | @@ -296,12 +333,26 @@ Medium, two Low. | Severity | Low | | Category | Design-document adherence | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/` (no source change), gateway proto contract | -| Status | Open | +| Status | Deferred | **Description:** The vendored `MxGateway.Contracts.dll` only carries the OLD `MxGateway.Contracts.Proto[.Galaxy]` namespace (PE-namespace dump confirms — `MxGateway.Client`, `MxGateway.Contracts`, `MxGateway.Contracts.Proto`, `MxGateway.Contracts.Proto.Galaxy` only). The sibling `mxaccessgw` repo's live `Protos/mxaccess_gateway.proto`, `mxaccess_worker.proto`, and `galaxy_repository.proto` files now generate into `ZB.MOM.WW.MxGateway.Contracts.Proto.*`. The proto wire format itself can still evolve (new RPCs, renamed fields, removed fields) and the driver has no contract-version handshake (a repo-wide search for `ContractVersion|ProtocolVersion|ApiVersion|WireVersion` in the driver returns nothing) — so a gateway service that evolves its proto past what the vendored client knows will fail silently at runtime: gRPC `UNIMPLEMENTED` for a renamed RPC, default-value reads for a removed scalar field, or worse, a wire-tag collision if a field number is reused. The risk surface grew with vendoring: previously the `ProjectReference` would have hard-failed at build time if the proto changed shape; now the driver builds green against a frozen contract that may not match the running gateway. **Recommendation:** (a) Add a single `Ping`/`GetVersion` RPC call at gateway-session open, comparing the gateway's reported contract version against a string baked into `libs/README.md` (or a `GatewayContractVersion` const) and refusing the session on mismatch with a clear log. (b) Document in `libs/README.md` the exact mxaccessgw commit SHA (and proto-file SHA-256s) the vendored DLLs were built from, so a parity-rig operator can grep the live gateway for the matching commit. (c) Add a soak/parity test that asserts the live gateway's proto descriptor still matches what the vendored DLL expects — fail loud rather than degrade. +**Resolution:** Deferred 2026-05-23 — the recommendation's part (b) +(record the mxaccessgw source-commit SHA in `libs/README.md`) is satisfied +by the Driver.Galaxy-015 resolution, which records both DLLs were built +from mxaccessgw commit `dd7ca1634e2d2b8a866c81f0009bf87ee9427750`. Parts +(a) and (c) — adding a `GetVersion` RPC at session-open and a parity +test against the live gateway's proto descriptor — are substantial new +RPC + plumbing work that is not in scope for this code-review-resolution +sweep. The risk surface is bounded because either of the two unwinding +paths in `libs/README.md` (sibling repo restores `MxGateway.Client.csproj`, +or this driver migrates to the new namespace) will move the codebase +past the vendoring + close this concern naturally. Re-open if neither +unwinding path is taken within the next quarter and the live gateway +service does evolve its proto under the driver. + ### Driver.Galaxy-018 | Field | Value | @@ -309,7 +360,7 @@ Medium, two Low. | Severity | Low | | Category | Documentation & comments | | Location | `libs/README.md:32-37`, `ZB.MOM.WW.OtOpcUa.Driver.Galaxy.csproj:40-47` | -| Status | Open | +| Status | Resolved | **Description:** Several small documentation issues in the vendoring artefacts: 1. `libs/README.md` says "Versions match what the sibling repo's `ZB.MOM.WW.MxGateway.Contracts.csproj` uses" — but `ZB.MOM.WW.MxGateway.Contracts.csproj` only declares `Google.Protobuf` 3.34.1 and `Grpc.Core.Api` 2.76.0; the other three packages (`Grpc.Net.Client`, `Microsoft.Extensions.Logging.Abstractions`, `Polly`) come from the (now-deleted) `MxGateway.Client.csproj`, not the contracts csproj. The README points at the wrong source-of-truth file. See Driver.Galaxy-016 for the related version-skew issue. @@ -318,3 +369,19 @@ Medium, two Low. 4. The csproj `` value relies on the bare assembly simple-name; an explicit `` plus `false` would document the contract surface inside the csproj where a reviewer reads it. **Recommendation:** (a) Update `libs/README.md` to (i) point at `MxGateway.Client.csproj` for the `Grpc.Net.Client`/`Microsoft.Extensions.Logging.Abstractions`/`Polly` version source, (ii) record the mxaccessgw commit SHA the vendored binaries were built from, and (iii) record SHA-256 hashes (see Driver.Galaxy-015). (b) Add `false` to both `` items in the csproj to make the intent explicit and refresh-robust. + +**Resolution:** Resolved 2026-05-23 — most of (a) was addressed alongside +Driver.Galaxy-015 + -016: `libs/README.md` rewritten to (i) point at the +sibling Server/Worker csproj as the live version source-of-truth (the +`MxGateway.Client.csproj` cited in the recommendation no longer exists — +the deleted-csproj reference would not have been actionable for a +future reader), (ii) record source commit +`dd7ca1634e2d2b8a866c81f0009bf87ee9427750`, and (iii) record SHA-256 +checksums of both vendored DLLs. (b) `false` +was intentionally NOT added — the vendored DLL's AssemblyVersion is +`1.0.0.0` and MSBuild's default for `` Include="bare-name" +items is already `SpecificVersion=false`, so the spelling-it-out +recommendation would be cosmetic without changing behaviour. If the +vendored DLLs are ever refreshed against a build with a different +`AssemblyVersion` the explicit attribute could be added then; for now +the existing csproj works correctly. diff --git a/code-reviews/README.md b/code-reviews/README.md index 5c4d1ff..69ca9bd 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -29,7 +29,7 @@ Each module's `findings.md` is the source of truth; this file is generated from | [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 0 | 8 | | [Driver.FOCAS](Driver.FOCAS/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | | [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 5 | -| [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 4 | 18 | +| [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 0 | 18 | | [Driver.Historian.Wonderware](Driver.Historian.Wonderware/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | | [Driver.Historian.Wonderware.Client](Driver.Historian.Wonderware.Client/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 10 | | [Driver.Modbus](Driver.Modbus/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | @@ -50,12 +50,8 @@ Findings with status `Open` or `In Progress`, ordered by severity. |---|---|---|---|---| | Core.Scripting-013 | Medium | Security | `ScriptEvaluator.cs:202-225` (`BuildWrapperSource`) | The synthesized wrapper pastes the user's source verbatim between `{` and `}` braces inside a static method body, with a `#line 1` directive and no escaping. The legacy `CSharpScript.CreateDelegate` path was robust to this because Roslyn's… | | Core.Scripting-014 | Medium | Concurrency & thread safety | `CompiledScriptCache.cs:91-103` (`Clear`) | `Clear()` snapshots `_cache.Keys.ToArray()` then iterates, calling `TryRemove(key, out var lazy)` on each — the key-only overload, not the value-scoped one used in `GetOrCompile`'s catch block. Between the snapshot and a given `TryRemove`,… | -| Driver.Galaxy-015 | Medium | Security | `libs/MxGateway.Client.dll`, `libs/MxGateway.Contracts.dll`, `libs/README.md` | Commit `994997b` checks in two binary DLLs (`MxGateway.Client.dll`, 99 840 bytes; `MxGateway.Contracts.dll`, 489 984 bytes) under `src/Drivers/.../Driver.Galaxy/libs/` and references them via ``. These are the onl… | -| Driver.Galaxy-016 | Medium | Performance & resource management | `ZB.MOM.WW.OtOpcUa.Driver.Galaxy.csproj:43-47`, `libs/README.md:32-37` | The five new `PackageReference` versions declared in the csproj (`Google.Protobuf` 3.34.1, `Grpc.Core.Api` 2.76.0, `Grpc.Net.Client` 2.71.0, `Microsoft.Extensions.Logging.Abstractions` 10.0.0, `Polly` 8.5.2) do not all match what the vendo… | | Core.ScriptedAlarms-013 | Low | Documentation & comments | `ScriptedAlarmEngine.cs:66-81` | The new internal test accessors `TryGetScratchReadCacheForTest` and `TryGetScratchContextForTest` (introduced by the Core.ScriptedAlarms-009 resolution at `0001cdd`) return the *live* per-alarm scratch — the same `Dictionary - + - - - + + + diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/libs/README.md b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/libs/README.md index 63400e8..58e0c0f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/libs/README.md +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/libs/README.md @@ -5,7 +5,31 @@ This directory holds binary copies of `MxGateway.Client.dll` and build (May 2026). The DLLs are referenced from the driver's csproj as `` items rather than `ProjectReference`. -## Why +## Provenance + +Both DLLs are built from this team's own `mxaccessgw` source tree — they are +not third-party binaries. The build commit + checksums below are recorded so +future readers can verify the artefacts match the expected source without +needing to ask the original author. + +| File | Source commit | SHA-256 | +|---|---|---| +| `MxGateway.Client.dll` | `dd7ca1634e2d2b8a866c81f0009bf87ee9427750` (mxaccessgw repo, pre-restructure) | `3507f770adc8c1b27b2fc4645079c6e4e02d5c65b9545c12d637cd2a080a00bd` | +| `MxGateway.Contracts.dll` | `dd7ca1634e2d2b8a866c81f0009bf87ee9427750` (mxaccessgw repo, pre-restructure) | `437dc6cb6994c7c4d858c82f69af890732c7ffbfa0463fbd8a63ce7930d251b4` | + +The build commit is the same for both DLLs and is embedded as +`AssemblyInformationalVersion` inside each binary — re-verify by running: +`ilspycmd | grep AssemblyInformationalVersion`. + +To re-verify the checksums (e.g. after a clone): +```bash +sha256sum libs/MxGateway.Client.dll libs/MxGateway.Contracts.dll +``` + +If either SHA-256 or the embedded source commit no longer matches what's +listed above, the artefact has been replaced — verify before trusting. + +## Why vendored The sibling `mxaccessgw` repo restructured: the `clients/dotnet/MxGateway.Client` project the driver previously referenced via path-based `ProjectReference` no @@ -17,32 +41,46 @@ the `MxGatewayClient` / `MxGatewaySession` / `GalaxyRepositoryClient` types the old client library provided (the sibling repo dropped the client library entirely, keeping only the contracts). -Vendoring the binaries unblocks the build in minutes instead of hours, freezes +Vendoring the binaries unblocked the build in minutes instead of hours, freezes the gateway contract surface at a known-good version, and preserves the option to migrate properly later without an emergency rewrite. ## What's vendored -| File | Source | Built against | -|---|---|---| -| `MxGateway.Client.dll` | mxaccessgw repo, `clients/dotnet/MxGateway.Client/` (pre-restructure) | net10.0, `MxGateway.Contracts.dll` | -| `MxGateway.Contracts.dll` | mxaccessgw repo, `src/MxGateway.Contracts/` (pre-restructure) | net10.0, proto namespace `MxGateway.Contracts.Proto[.Galaxy]` | +| File | Built against | +|---|---| +| `MxGateway.Client.dll` | net10.0, references `MxGateway.Contracts.dll` | +| `MxGateway.Contracts.dll` | net10.0, proto namespace `MxGateway.Contracts.Proto[.Galaxy]` | -The NuGet packages the vendored DLLs need transitively (Google.Protobuf, -Grpc.Core.Api, Grpc.Net.Client, Microsoft.Extensions.Logging.Abstractions, -Polly) are declared as direct `PackageReference` in the driver csproj — when -the dropped `ProjectReference` was in place those packages were transitively -provided; with binary references the consumer must declare them explicitly. -Versions match what the sibling repo's `ZB.MOM.WW.MxGateway.Contracts.csproj` -uses so the gRPC + proto runtime stays binary-compatible. +The NuGet packages the vendored DLLs reference (verified by reflecting +`Assembly.GetReferencedAssemblies()` against `MxGateway.Client.dll`) are +declared as direct `PackageReference` in the driver csproj — when the dropped +`ProjectReference` was in place those packages were transitively provided; +with binary references the consumer must declare them explicitly: + +| Package | Reason | +|---|---| +| `Google.Protobuf` 3.34.1 | Proto message types in `MxGateway.Contracts.dll` | +| `Grpc.Core.Api` 2.76.0 | Base gRPC client types in `MxGateway.Client.dll` | +| `Grpc.Net.Client` 2.76.0 | HTTP/2 transport used by `MxGatewayClient` | +| `Microsoft.Extensions.Logging.Abstractions` 10.0.7 | `ILogger` used by the client | +| `Polly.Core` 8.6.6 | Retry pipeline used by `MxGatewayClient` | + +Versions match the sibling mxaccessgw repo's current Server / Worker +projects (`ZB.MOM.WW.MxGateway.Server.csproj`, +`ZB.MOM.WW.MxGateway.Worker.csproj`) so the runtime versions stay close to +what the gateway team uses. The pre-Driver.Galaxy-016 declarations were +incorrect — most visibly `Polly 8.5.2` was declared where the DLL actually +needs `Polly.Core` (a different package: `Polly` v7 is the older fluent API; +`Polly.Core` v8 is the modern resilience-pipeline API the gateway client was +built against). A `Polly` reference would have failed at runtime with +`MissingMethodException` the first time a retry pipeline ran. ## Decompiled-source archive The vendored DLLs are byte-for-byte the build output. The full source can be recovered with `ilspycmd MxGateway.Client.dll > MxGateway.Client.cs` if a code -review or audit needs it. There is no proprietary code in either DLL — they -are the OtOpcUa team's own client implementation against the gateway's open -proto contracts. +review or audit needs it. ## How to unwind