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 2577b436..0ecd9c86 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -578,3 +578,28 @@ T10 anytime after T5/T8; T11 last (after T5 + T9 merge-ready); T12 anytime ``` Overall effort: **~1 dev-day code+unit, plus the rig session.** The two HIGH-RISK slices are T2/T3 (authentication path — every new branch must land on deny) and T11 (the gate that proves the clock axis actually reaches a real client). T9 is the largest single task and is the only one that can falsify T8 end to end. + +--- + +## Execution notes + +Branch `fix/r3-05-acl-hot-path-expiry`, executed 2026-08-20 off master `dd7f4954`. TDD per task: +failing test first, red observed and recorded, then green. T11 (live gate) deliberately left `pending` +for the orchestrator. + +### T1 — TimeProvider registration + option knobs + +- **RED observed:** `error CS1061: 'ServiceCollection' does not contain a definition for + 'AddOtOpcUaDataPlaneAuth'` and `'DataPlaneAuthorizer' does not contain a definition for 'Clock'` — + the seam and the assertion surface both absent, as predicted. +- **DEVIATION (structural, required by the plan's own test):** the plan says the guard test "builds the + service collection for an admin node". `Program.cs` is **top-level statements in an executable**, so no + test can drive its wiring. The `DataPlaneAuthorizer` registration and the three-branch key-store / + authenticator conditional were therefore **moved verbatim** into a new + `Host/Configuration/DataPlaneAuthRegistration.AddOtOpcUaDataPlaneAuth(hasAdmin, hasDriver)` — the same + shape `LocalDbRegistration` already uses — and `Program.cs` now calls it. No branch changed. +- **DEVIATION (small):** `DataPlaneAuthorizer` gained a public `Clock` property. The two-clock hazard the + class already documents can only be pinned by reference, and `_time` was private. +- **Collateral:** `MeshRegistrationRoleScopingTests.HasAdminMarkerScan_MatchesTheKnownShapeOfProgramCs` + pins the `if (hasAdmin)` marker count in `Program.cs`; the move took one braced marker with it, so the + pin went 6 → 5 (the test's own message asks for exactly that when a marker moves). 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 77023481..46fc793e 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 @@ -5,7 +5,7 @@ { "id": "T1", "subject": "Register TimeProvider in DI (shared by authorizer, key store and revalidation timer) + ApiKeyOptions.StoreTimeoutMs/StoreCacheSeconds + AclOptions.RevalidationIntervalSeconds (03/S18, 03/S19)", - "status": "pending", + "status": "done", "blockedBy": [] }, { diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs index 1efe641c..524fa93b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclOptions.cs @@ -42,4 +42,23 @@ public sealed class AclOptions /// false, which preserves the pre-stage-5 role gates byte-for-byte. /// public bool Enabled { get; set; } + + /// + /// How often, in seconds, live monitored items are re-authorized against the clock — in addition + /// to on every deployment (arch-review 03/S19). + /// + /// + /// + /// A subscription is authorized once and then streams, so before this a key that expired by the + /// CLOCK kept feeding a live subscription until a deployment happened to advance the generation + /// — indefinitely on a fleet that has been deployed once and left running. Every other seam + /// re-asks per request and honours ExpiresUtc the moment it passes. + /// + /// + /// 0 or negative disables the timer, leaving revalidation deploy-triggered only (the + /// pre-R3-05 behaviour). The timer is inert while is false: the + /// pass early-returns when ACL enforcement is off. + /// + /// + public int RevalidationIntervalSeconds { get; set; } = 60; } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs index 47d122e4..ea2d64ee 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/ApiKeyOptions.cs @@ -27,4 +27,29 @@ public sealed class ApiKeyOptions /// ApiKeySecret.ComputeHash throws on one and this stays empty to make that fire early. /// public string Pepper { get; set; } = string.Empty; + + /// + /// Hard per-call deadline, in milliseconds, on the API-key store lookup that runs inline on OPC UA + /// session activation (arch-review 03/S18). Exceeding it denies the activation and emits a + /// distinct Warning plus otopcua.opcua.apikey_store.bounded{outcome=timeout}. + /// + /// + /// 0 or negative disables the deadline, restoring the pre-R3-05 unbounded behaviour. Do not + /// do that on a node with a ConfigDb: the SDK raises impersonation synchronously under a + /// SessionManager-wide lock, so an unanswered lookup stalls every session activation + /// in the server, not just its own. + /// + public int StoreTimeoutMs { get; set; } = 2000; + + /// + /// TTL, in seconds, of the read-through cache in front of the API-key store (arch-review 03/S18). + /// Bounds how long a revocation authored in the ConfigDb can take to reach a new session + /// activation. + /// + /// + /// 0 or negative disables the cache, restoring a store read per activation. The window it + /// introduces is strictly smaller than the one that already exists for already-open sessions + /// on every node kind, which is the next deployment — see docs/security.md. + /// + public int StoreCacheSeconds { get; set; } = 5; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs new file mode 100644 index 00000000..cb04f265 --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/DataPlaneAuthRegistration.cs @@ -0,0 +1,100 @@ +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +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.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.Host.Configuration; + +/// +/// Composition of the OPC UA data-plane authentication + authorization seams (#520), extracted from +/// Program.cs so the role-conditional branches are reachable from a test. +/// +/// +/// A store decorator registered in one branch of a role conditional is exactly the +/// "built but never wired" shape, and Program.cs's top-level statements cannot be driven by a +/// test — so the wiring moved here rather than the guard being written against a comment. The +/// branches are unchanged from the ones that lived inline; see ApiKeyStoreRegistrationTests. +/// +public static class DataPlaneAuthRegistration +{ + /// + /// Registers the shared clock, the node-scoped ACL gate and the role-appropriate API-key store + + /// user authenticator. + /// + /// The service collection. + /// Whether this node carries the admin role (and therefore a ConfigDb). + /// Whether this node carries the driver role. + /// The same collection, for chaining. + public static IServiceCollection AddOtOpcUaDataPlaneAuth( + this IServiceCollection services, bool hasAdmin, bool hasDriver) + { + ArgumentNullException.ThrowIfNull(services); + + // ONE clock for the whole authorization surface. The authorizer, the key-store bound and the + // revalidation timer all read time, and DataPlaneAuthorizer already documents what a second, + // diverging clock costs: every request denied, with a verdict indistinguishable from "no grant + // matched". TryAdd so a test (or a future host) can substitute one before this runs. + services.TryAddSingleton(TimeProvider.System); + + // #520 stage 5 — the node-scoped ACL gate. Registered on EVERY node, not just driver ones, because + // OtOpcUaServerHostedService takes it as a constructor parameter and the built-in container resolves + // constructors by whether every parameter is registered, not by whether the parameter type is nullable. + // That is the same trap the IOpcUaUserAuthenticator and IDataPlaneAuthoritySource registrations below + // both carry a comment about; an absent registration is a node that will not start. + services.AddSingleton(sp => + new TriePermissionEvaluator(sp.GetRequiredService())); + services.AddSingleton(sp => new DataPlaneAuthorizer( + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService>().Value.Enabled, + sp.GetRequiredService().CreateLogger(), + sp.GetRequiredService())); + + if (hasAdmin) + { + // A node WITH a ConfigDb reads keys straight from it, which is what makes revocation take effect + // on the very next activation rather than on the next deployment. Preferred wherever it is + // available, including on a fused admin+driver node. + // The ClusterId confines the grants this node honours to its OWN cluster — the ConfigDb holds + // the whole fleet's ACL rows, unlike the artifact a driver-only node reads, which + // ParseDataPlaneAuthority already scoped. Taken from AkkaClusterOptions (pure configuration), + // 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))); + services.AddSingleton(); + + // Minting surface for the AdminUI. Same node condition as the store above: both need a ConfigDb. + services.AddSingleton(sp => new ApiKeyAdminService( + sp.GetRequiredService>(), + sp.GetRequiredService>().Value.Pepper)); + } + else if (hasDriver) + { + // Phase 4 left a driver-only node with no ConfigDb, so its keys ride the deployment artifact. + // Before stage 4 this node fell through to NullOpcUaUserAuthenticator and denied EVERY UserName + // token — safe, but it meant no machine client could authenticate against a site node at all. + // Deliberately NOT wrapped by the S18 bound: the artifact store is two dictionary probes on an + // in-memory snapshot, so it can neither hang nor be worth caching. + services.AddSingleton(); + services.AddSingleton(); + } + else + { + // Neither role: nothing serves a data plane here. MUST be registered explicitly, not left absent — + // OtOpcUaServerHostedService takes IOpcUaUserAuthenticator as a REQUIRED constructor parameter, so + // an unregistered seam is not a quiet fallback to deny-all; it fails DI and the node does not start. + services.AddSingleton(NullOpcUaUserAuthenticator.Instance); + } + + return services; + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs index 12eeee6b..e6e0fbbb 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs @@ -398,55 +398,10 @@ if (hasDriver) builder.Services.AddSingleton(); } - // #520 stage 5 — the node-scoped ACL gate. Registered on EVERY node, not just driver ones, because - // OtOpcUaServerHostedService takes it as a constructor parameter and the built-in container resolves - // constructors by whether every parameter is registered, not by whether the parameter type is nullable. - // That is the same trap the IOpcUaUserAuthenticator and IDataPlaneAuthoritySource registrations above - // both carry a comment about; an absent registration is a node that will not start. - builder.Services.AddSingleton(sp => - new TriePermissionEvaluator(sp.GetRequiredService())); - builder.Services.AddSingleton(sp => new DataPlaneAuthorizer( - sp.GetRequiredService(), - sp.GetRequiredService(), - sp.GetRequiredService>().Value.Enabled, - sp.GetRequiredService().CreateLogger())); - - if (hasAdmin) - { - // A node WITH a ConfigDb reads keys straight from it, which is what makes revocation take effect - // on the very next request rather than on the next deployment. Preferred wherever it is available, - // including on a fused admin+driver node. - // The ClusterId confines the grants this node honours to its OWN cluster — the ConfigDb holds - // the whole fleet's ACL rows, unlike the artifact a driver-only node reads, which - // ParseDataPlaneAuthority already scoped. Taken from AkkaClusterOptions (pure configuration), - // NOT from IClusterRoleInfo: that depends on the ActorSystem, and the same options-not-services - // rule is what keeps the redundancy-singleton registration below from recursing into ActorSystem - // construction. Null on a node with no cluster-{ClusterId} role ⇒ no filter (legacy behaviour). - builder.Services.AddSingleton(sp => new ConfigDbApiKeyStore( - sp.GetRequiredService>(), - ConfigDbApiKeyStore.ResolveClusterId(sp.GetRequiredService>().Value))); - builder.Services.AddSingleton(); - - // Minting surface for the AdminUI. Same node condition as the store above: both need a ConfigDb. - builder.Services.AddSingleton(sp => new ApiKeyAdminService( - sp.GetRequiredService>(), - sp.GetRequiredService>().Value.Pepper)); - } - else if (hasDriver) - { - // Phase 4 left a driver-only node with no ConfigDb, so its keys ride the deployment artifact. - // Before stage 4 this node fell through to NullOpcUaUserAuthenticator and denied EVERY UserName - // token — safe, but it meant no machine client could authenticate against a site node at all. - builder.Services.AddSingleton(); - builder.Services.AddSingleton(); - } - else - { - // Neither role: nothing serves a data plane here. MUST be registered explicitly, not left absent — - // OtOpcUaServerHostedService takes IOpcUaUserAuthenticator as a REQUIRED constructor parameter, so - // an unregistered seam is not a quiet fallback to deny-all; it fails DI and the node does not start. - builder.Services.AddSingleton(NullOpcUaUserAuthenticator.Instance); - } + // #520 + arch-review 03/S18 — the node-scoped ACL gate, the shared clock and the role-appropriate + // API-key store + user authenticator, all in one extension so the role-conditional branches are + // reachable from a test (Program.cs's top-level statements are not). See DataPlaneAuthRegistration. + builder.Services.AddOtOpcUaDataPlaneAuth(hasAdmin, hasDriver); // Bind + validate the OPC UA host options the same way (fail-fast at start via ValidateOnStart) // and let OtOpcUaServerHostedService consume the validated IOptions instance rather than diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs index 3f9a64a1..127e5778 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/DataPlaneAuthorizer.cs @@ -108,6 +108,15 @@ public sealed class DataPlaneAuthorizer /// public bool Enabled { get; } + /// The clock this gate evaluates key expiry against. + /// + /// Exposed so the composition root's wiring can be asserted by reference. The remarks on + /// UserAuthorizationState record what a second, diverging clock costs: every request denied, + /// with a verdict indistinguishable from "no grant matched". One registered singleton threaded into + /// every consumer is the fix, and a test that cannot see which instance landed here cannot pin it. + /// + public TimeProvider Clock => _time; + /// /// Memoized grant walks for the request-rate seams (#520 stage 6). See /// for what it is and is not allowed to cache. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshRegistrationRoleScopingTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshRegistrationRoleScopingTests.cs index 58d5d93f..b3e4d7b2 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshRegistrationRoleScopingTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/MeshRegistrationRoleScopingTests.cs @@ -236,8 +236,10 @@ public sealed class MeshRegistrationRoleScopingTests } /// - /// Pins the scanner's expected result shape against today's Program.cs — 6 markers, - /// 5 braced blocks + the 1 braceless ConfigDb gate, so 6 scanned ranges. Without this, a + /// Pins the scanner's expected result shape against today's Program.cs — 5 markers, + /// 4 braced blocks + the 1 braceless ConfigDb gate, so 5 scanned ranges (the data-plane + /// key-store branch moved into DataPlaneAuthRegistration for arch-review 03/S18, taking + /// one braced marker with it). Without this, a /// scanner bug that silently drops or merges a marker (as the pre-fix version did to the /// braceless marker — it fabricated a phantom range spanning an unrelated if-block and swallowed /// a real nested 'if (hasAdmin)' block inside it, yielding 5 ranges instead of 6) would not @@ -248,13 +250,13 @@ public sealed class MeshRegistrationRoleScopingTests { var source = ReadProgramSource(); - CountOccurrences(source, HasAdminMarker).ShouldBe(6, + CountOccurrences(source, HasAdminMarker).ShouldBe(5, "Program.cs's 'if (hasAdmin)' marker count changed. If a marker was added or removed, " + "update this pin — and check whether the new/removed occurrence is braced or braceless — " + "rather than letting the scanner silently mis-shape its result."); var ranges = FindHasAdminBlockRanges(source); - ranges.Count.ShouldBe(6, + ranges.Count.ShouldBe(5, "expected one scanned range per 'if (hasAdmin)' marker, including the braceless ConfigDb " + "gate. A mismatch means the scanner adopted an unrelated brace or otherwise merged/dropped " + "a marker instead of handling each one independently."); 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 new file mode 100644 index 00000000..7aa19673 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/ApiKeyStoreRegistrationTests.cs @@ -0,0 +1,77 @@ +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; +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.Configuration; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; + +namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa; + +/// +/// Guards the role-conditional data-plane authentication wiring (arch-review 03/S18). A decorator +/// registered in one branch of a role conditional is the "built but never wired" shape, so the branch +/// each node role lands in is asserted rather than assumed. +/// +public sealed class ApiKeyStoreRegistrationTests +{ + /// + /// The authorizer's clock and the registered clock must be the SAME instance. + /// + /// + /// already documents that a divergence between two clocks denies + /// every request with a verdict indistinguishable from "no grant matched" — found once by a test + /// clock two hours ahead. One registered singleton threaded into every consumer makes the + /// divergence unrepresentable instead of merely unlikely, so the registration is what is pinned. + /// + [Fact] + public void The_authorizer_and_the_key_store_share_one_TimeProvider() + { + using var sp = BuildNode(hasAdmin: true, hasDriver: true); + + var registered = sp.GetRequiredService(); + var authorizer = sp.GetRequiredService(); + + registered.ShouldBeSameAs(Probe.Instance); + authorizer.Clock.ShouldBeSameAs(registered); + } + + /// 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. + /// The built provider; the caller disposes it. + internal static ServiceProvider BuildNode(bool hasAdmin, bool hasDriver) + { + var services = new ServiceCollection(); + services.AddLogging(b => b.AddProvider(NullLoggerProvider.Instance)); + services.AddSingleton(Probe.Instance); + services.Configure(_ => { }); + services.Configure(_ => { }); + services.Configure(o => o.Roles = ["admin", "driver", "cluster-MAIN"]); + services.AddSingleton(EmptyDataPlaneAuthoritySource.Instance); + services.AddSingleton(); + services.AddSingleton(); + + if (hasAdmin) + { + services.AddDbContextFactory( + o => o.UseInMemoryDatabase($"apikey-reg-{Guid.NewGuid():N}")); + } + + services.AddOtOpcUaDataPlaneAuth(hasAdmin, hasDriver); + return services.BuildServiceProvider(); + } + + /// A clock distinguishable from by reference. + private sealed class Probe : TimeProvider + { + public static readonly Probe Instance = new(); + + /// + public override DateTimeOffset GetUtcNow() => new(2026, 8, 20, 12, 0, 0, TimeSpan.Zero); + } +}