From 39a10a277a929fc5c8ad3fccc9e9c1b139081625 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:45:08 -0400 Subject: [PATCH] fix(security): wrap the ConfigDb key store on admin nodes + registration guard --- .../plans/R3-05-acl-hot-path-expiry-plan.md | 12 +++++++ ...-05-acl-hot-path-expiry-plan.md.tasks.json | 2 +- .../DataPlaneAuthRegistration.cs | 17 +++++++-- .../OpcUa/ApiKeyOpcUaUserAuthenticator.cs | 15 ++++++-- .../OpcUa/BoundedApiKeyStore.cs | 12 +++++++ .../OpcUa/ConfigDbApiKeyStore.cs | 15 ++++++-- .../OpcUa/ApiKeyStoreRegistrationTests.cs | 36 +++++++++++++++++++ 7 files changed, 100 insertions(+), 9 deletions(-) diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md index 90dbebeb..1220328e 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -645,3 +645,15 @@ for the orchestrator. constructs one, and xUnit never runs two tests of one class in parallel. Adding a second class that exercises the decorator means adding a discriminating tag. - 14 tests in the class, green. + +### T5 — wire the decorator + registration guard + +- **RED observed:** `error CS1061: 'BoundedApiKeyStore' does not contain a definition for 'Inner'` / + `… for 'Clock'` — the guard could not see inside the wrapper because nothing was wrapping yet. +- **DEVIATION (small):** `BoundedApiKeyStore` gained public `Inner` and `Clock` properties, for the same + reason `DataPlaneAuthorizer.Clock` did in T1 — a guard that cannot see what is inside the wrapper cannot + tell a correctly-wired node from one wrapping the wrong store. +- Doc-drift half of the finding landed here: the authenticator's "no network I/O on this path at all" + remark is now qualified by node role and points at the bound, and `ConfigDbApiKeyStore` records both + that it is wrapped in production and that "revocation on the next request" is **activation**-scoped. +- Host.Tests: 123 green (was 106 at base). diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json index f253cd64..1928adba 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json @@ -35,7 +35,7 @@ { "id": "T5", "subject": "Wire BoundedApiKeyStore on admin/fused nodes in Program.cs + registration guard test; correct the authenticator's 'no network I/O at all' remark", - "status": "pending", + "status": "done", "blockedBy": [ "T4" ] diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs index cb04f265..19d5385d 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs @@ -67,9 +67,20 @@ public static class DataPlaneAuthRegistration // NOT from IClusterRoleInfo: that depends on the ActorSystem, and the same options-not-services // rule is what keeps the redundancy-singleton registration in Program.cs from recursing into // ActorSystem construction. Null on a node with no cluster-{ClusterId} role ⇒ no filter. - services.AddSingleton(sp => new ConfigDbApiKeyStore( - sp.GetRequiredService>(), - ConfigDbApiKeyStore.ResolveClusterId(sp.GetRequiredService>().Value))); + // + // arch-review 03/S18 — and it is WRAPPED. The lookup runs inline in the SDK's synchronous + // impersonation callback, under a SessionManager-wide lock, with CancellationToken.None: an + // unanswered ConfigDb query therefore stalls every session activation in the server. The + // decorator adds the deadline that bounds that, plus a short TTL that turns a burst of + // activations into one round trip. It cannot admit anything the inner store did not. + services.AddSingleton(sp => new BoundedApiKeyStore( + new ConfigDbApiKeyStore( + sp.GetRequiredService>(), + ConfigDbApiKeyStore.ResolveClusterId( + sp.GetRequiredService>().Value)), + sp.GetRequiredService>(), + sp.GetRequiredService().CreateLogger(), + sp.GetRequiredService())); services.AddSingleton(); // Minting surface for the AdminUI. Same node condition as the store above: both need a ConfigDb. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs index dc0d0f85..b4e67ee7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ApiKeyOpcUaUserAuthenticator.cs @@ -18,9 +18,18 @@ namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; /// This runs inline on session activation. The SDK raises ImpersonateUser /// synchronously while holding a SessionManager-wide event lock, so a slow authenticator does not /// stall one request — it serializes every session activation server-wide. That is why -/// verification is a single indexed lookup plus one HMAC, and why there is no directory call, -/// no retry, and no network I/O on this path at all. It is the main practical advantage of keys -/// over the LDAP bind this replaces. +/// verification is a single indexed lookup plus one HMAC, with no directory call and no retry — +/// the main practical advantage of keys over the LDAP bind this replaces. +/// +/// +/// ⚠️ "No network I/O" is TRUE ONLY ON A DRIVER-ONLY NODE (arch-review 03/S18). The claim +/// describes , which is two dictionary probes on an in-memory +/// snapshot. On an admin or fused node is +/// , and a UserName activation costs two EF round trips +/// (the key row, then the principal's NodeAcl rows) — inside this same lock. What bounds +/// that is , which wraps it in production; the deadline it applies +/// surfaces here as an ordinary throw and is denied by the catch-all below, like any other store +/// failure. /// /// /// Fail closed, and say little. Every rejection returns the same opaque message. The diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs index 21e112d5..cbf4cf76 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs @@ -81,6 +81,18 @@ public sealed class BoundedApiKeyStore : IApiKeyStore _time = time; } + /// The store being bounded. + /// + /// Exposed so the composition root's wiring can be asserted. A decorator registered in one branch + /// of a role conditional is the "built but never wired" shape, and a guard that cannot see what is + /// inside the wrapper cannot tell a correctly-wired node from one wrapping the wrong store. + /// + public IApiKeyStore Inner => _inner; + + /// The clock this bound's deadline and TTL are measured on. + /// Same reason as : the shared-clock invariant is only pinnable by reference. + public TimeProvider Clock => _time; + /// /// /// Keyed by alone. is deliberately not part diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs index 09178426..e3cdc004 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs @@ -14,8 +14,19 @@ namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; /// A driver-only node has no ConfigDb since per-cluster mesh Phase 4, so this store cannot be /// registered there; that node uses instead (#520 stage 4). /// -/// Reading per request is what makes revocation immediate here, which is why this store is -/// preferred wherever a ConfigDb exists — including on a fused admin+driver node. +/// Reading per activation is what makes revocation prompt here, which is why this store is +/// preferred wherever a ConfigDb exists — including on a fused admin+driver node. ⚠️ It reaches a +/// new session activation only: an already-open session re-checks its key against the +/// in-memory artifact snapshot on every operation, not against this store, so clearing +/// Enabled here does not stop an open session until the next deployment. See +/// docs/security.md. +/// +/// +/// ⚠️ In production this store is WRAPPED by (arch-review +/// 03/S18), because these queries run inline in the SDK's impersonation callback under a +/// SessionManager-wide lock. A call may therefore be cancelled by the wrapper's deadline even +/// though the caller passed — the resulting +/// is a denial, not a bug. /// /// /// ⚠️ The ConfigDb holds the WHOLE fleet's ACL rows, not just this node's cluster's. That is diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs index 7aa19673..1548c3ea 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs @@ -7,6 +7,7 @@ using Xunit; using ZB.MOM.WW.OtOpcUa.Cluster; using ZB.MOM.WW.OtOpcUa.Configuration; using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; using ZB.MOM.WW.OtOpcUa.Host.Configuration; using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; @@ -40,6 +41,41 @@ public sealed class ApiKeyStoreRegistrationTests authorizer.Clock.ShouldBeSameAs(registered); } + /// + /// An admin (or fused admin+driver) node's key store is the + /// decorator, wrapping the ConfigDb store. + /// + /// + /// The decorator lives in ONE branch of a role conditional, which is the "built but never wired" + /// shape OVERALL theme 3 names. Unwired, every unit test in BoundedApiKeyStoreTests still + /// passes and the deadline protects nothing. + /// + [Fact] + public void An_admin_node_wraps_the_ConfigDb_store_in_the_bound() + { + using var sp = BuildNode(hasAdmin: true, hasDriver: true); + + var store = sp.GetRequiredService().ShouldBeOfType(); + store.Inner.ShouldBeOfType(); + store.Clock.ShouldBeSameAs(sp.GetRequiredService()); + } + + /// + /// A driver-only node's key store is the bare artifact store — deliberately NOT wrapped. + /// + /// + /// It is two dictionary probes on an in-memory snapshot, so it can neither hang nor be worth + /// caching; wrapping it would add a deadline to a call that cannot miss one and a second copy of + /// data the snapshot already holds. + /// + [Fact] + public void A_driver_only_node_gets_the_bare_artifact_store() + { + using var sp = BuildNode(hasAdmin: false, hasDriver: true); + + sp.GetRequiredService().ShouldBeOfType(); + } + /// Builds the data-plane authentication wiring for one node role combination. /// Whether the node carries the admin role. /// Whether the node carries the driver role.