From 3a31dd2de75f0d73d1c164be932237b49fa6b6c3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:27:14 -0500 Subject: [PATCH] fix(host): follow an in-place site PSK rotation without a central restart (ScadaBridge#96) SitePskProvider cached each site's gRPC preshared key forever; only site removal invalidated it, so rotating SB-GRPC-PSK-{site} broke the central<->site link on both central nodes until they restarted. The cached key is now dropped when: - the secret is written through /admin/secrets: central replaces the secrets library's ISecretCacheInvalidator with SitePskRotationInvalidator, which also publishes SitePskInvalidated on a new DistributedPubSub topic (site-psk-invalidation) so the peer central node drops its copy and its resolver cache entry too (SitePskInvalidationSubscriber); - a site rejects a central->site call with PermissionDenied/Unauthenticated: WithSiteCredentials wraps every per-site channel's handler in PskRejectionRefreshHandler, so the next call re-reads the key (covers out-of-process writes such as the secret CLI); - a site->central call presents a token that does not match the cached key: CentralControlAuthInterceptor invalidates and re-reads once before refusing; - ScadaBridge:Communication:SitePsks reloads. Invalidation is case-insensitive because SecretName lowercases names. Rotation runbook (topology-guide), Component-Communication/Host and the #85 cache audit (D2) updated. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- docs/deployment/topology-guide.md | 10 +- ...026-11-09-central-cache-staleness-audit.md | 6 +- docs/requirements/Component-Communication.md | 12 ++ docs/requirements/Component-Host.md | 1 + .../Grpc/ISitePskProvider.cs | 78 ++++++++ .../Actors/AkkaHostedService.cs | 12 ++ .../CentralControlAuthInterceptor.cs | 41 +++-- src/ZB.MOM.WW.ScadaBridge.Host/Program.cs | 4 + .../SitePskProvider.cs | 63 ++++++- .../SitePskRotationInvalidator.cs | 168 ++++++++++++++++++ .../CentralControlAuthInterceptorTests.cs | 54 ++++++ .../CompositionRootTests.cs | 12 ++ .../ControlPlaneAuthEndToEndTests.cs | 85 +++++++++ .../SitePskProviderTests.cs | 57 ++++++ .../SitePskRotationInvalidatorTests.cs | 89 ++++++++++ 15 files changed, 666 insertions(+), 26 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.Host/SitePskRotationInvalidator.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SitePskRotationInvalidatorTests.cs diff --git a/docs/deployment/topology-guide.md b/docs/deployment/topology-guide.md index 63628cb2..7a15f6c4 100644 --- a/docs/deployment/topology-guide.md +++ b/docs/deployment/topology-guide.md @@ -557,8 +557,14 @@ One key **per site**, never one for the fleet: a compromised site must not yield key. And never share it with `LocalDb:Replication:ApiKey` — that authenticates the *pair partner* for database replication, a different trust relationship on the same listener. -**Rotation:** set the new value on both sides, then restart the pair (pairs restart together -anyway — see above). **Upgrading to a build that has this gate requires seeding the key first**, +**Rotation:** set the new value on both sides, then restart the site pair (pairs restart together +anyway — see above; the site reads its key once, at boot). **Central needs no restart** (Gitea #96): +writing `SB-GRPC-PSK-` through `/admin/secrets` on either central node drops the cached key +on both central nodes at once, and a key changed any other way (the `secret` CLI, a `SitePsks` +edit) is picked up on the first call the site rejects, or the first site call central's own gate +cannot match — within `Secrets:ResolveCacheTtl` (30 s) of +the change. Expect the link to that site to fail between the two halves of the change; doing +central first and the site straight after keeps that window short. **Upgrading to a build that has this gate requires seeding the key first**, including in the on-host `deploy/` overlays. The key is a bearer token over plaintext h2c, so it is readable and replayable by anyone on the diff --git a/docs/plans/2026-11-09-central-cache-staleness-audit.md b/docs/plans/2026-11-09-central-cache-staleness-audit.md index 66ca434d..f7258b23 100644 --- a/docs/plans/2026-11-09-central-cache-staleness-audit.md +++ b/docs/plans/2026-11-09-central-cache-staleness-audit.md @@ -28,7 +28,7 @@ node after a change is made through the other one. Same defect class as #83 | `PendingUpdatesCache` (DeploymentManager, #91) | Fleet pending-updates snapshot | Deployment finished on this node | `DeploymentManager:PendingUpdatesCacheTtl`, default **60 s** | OK — display only | | `CentralCommunicationActor` site-address cache (+ `GrpcSiteTransport` channel pairs) | Site gRPC endpoints | Site create/update/delete → `RefreshSiteAddresses` | Periodic DB reload, **60 s** (hard-coded) | OK — rare edit, self-heals | | `SiteStreamGrpcClientFactory` clients | Per (site, endpoint) stream clients | Site delete → `RemoveSiteAsync` (local only) | Not stale data: a removed site's client on the peer is reclaimed by idle eviction, `Communication:GrpcClientIdleTtl` (**30 min**) × 1.5 after last use | OK — resource, not correctness | -| `SitePskProvider` (Host) | Per-site gRPC PSK from config / secret store | Site removal (via either gRPC client path, on every node within the 60 s reload) | **Unbounded** for an in-place key rotation — on **both** nodes, not only the peer | Owner decision — see D2 | +| `SitePskProvider` (Host) | Per-site gRPC PSK from config / secret store | Site removal; `SB-GRPC-PSK-{site}` write via `/admin/secrets`; site auth rejection; `SitePsks` reload (#96) | Write via `/admin/secrets`: **none** (peer told over `DistributedPubSub` topic `site-psk-invalidation`); other writes: first rejected call after `Secrets:ResolveCacheTtl` | Fixed — #96 (see D2) | | `DefaultSecretResolver` (shared `ZB.MOM.WW.Secrets`) | Decrypted secret values | Lib's own invalidator (UI writes, sync service) | `Secrets:ResolveCacheTtl`, **30 s** | OK — owned by the shared lib | | `KpiSnapshotCache` (CentralUI) | KPI query results (operational data) | — | **8 s** | OK — not config | | `SharedAlarmSummaryService` (CentralUI) | Per-site alarm summary fan-out | — | **12 s** cold-cache window | OK — not config | @@ -69,7 +69,9 @@ broadcast was added. new key) leaves **both** central nodes dialling with the old key until they restart. Options: make central restart part of the rotation runbook; give the cache a TTL; or invalidate on a `PermissionDenied` from the site. Not peer-specific, so it is out of #85's fix scope — file it - separately. + separately. **Resolved by #96:** all three — invalidate on the rotation write (and the peer, via + the new `site-psk-invalidation` topic; the first config-invalidation broadcast topic D1/D3 could + join), refresh on a site's auth rejection, and drop the cache on a `SitePsks` reload. - **D3 — `ExternalDefinitionLookupCache` peer bound (10 s).** Already tracked by #83; the recommendation there (accept and document) still stands. If D1 gets a broadcast topic, this cache and `ApiMethodCache` should join it rather than add a second mechanism. diff --git a/docs/requirements/Component-Communication.md b/docs/requirements/Component-Communication.md index a2ed9ffc..9ab6f54f 100644 --- a/docs/requirements/Component-Communication.md +++ b/docs/requirements/Component-Communication.md @@ -204,6 +204,18 @@ because sites are created at runtime and cannot be enumerated in configuration a `ControlPlaneCredentials` binds the key to the channel as `CallCredentials`, so no individual call site can omit it. +Central caches each resolved key, and drops the entry (Gitea #96) when: the site is removed; the +secret `SB-GRPC-PSK-` is written through central's secrets write path (`/admin/secrets`) — +`SitePskRotationInvalidator` wraps the secrets library's `ISecretCacheInvalidator` and also +broadcasts a `SitePskInvalidated` on the central `DistributedPubSub` topic `site-psk-invalidation` +so the peer central node drops its copy too; the site rejects a call with `PermissionDenied` / +`Unauthenticated` (`PskRejectionRefreshHandler`, wrapped around every per-site channel by +`WithSiteCredentials`, so the next call re-reads the key — this covers keys rotated outside the +process, e.g. by the `secret` CLI, after at most the resolver's `Secrets:ResolveCacheTtl`); a +site's inbound call to central presents a token that does not match the cached key +(`CentralControlAuthInterceptor` drops the entry and re-reads once before refusing); or +`ScadaBridge:Communication:SitePsks` reloads. An in-place rotation therefore needs no central restart. + This is distinct from `LocalDb:Replication:ApiKey`, which gates `/localdb_sync.v1.LocalDbSync/` on the same listener via its own interceptor: that key authenticates the *pair partner* for database replication, a different trust relationship, and the diff --git a/docs/requirements/Component-Host.md b/docs/requirements/Component-Host.md index 7735fb7e..b2ebf95d 100644 --- a/docs/requirements/Component-Host.md +++ b/docs/requirements/Component-Host.md @@ -351,4 +351,5 @@ built)* section for where the shipped code deviates from the original design). - **Port options come from the bound `NodeOptions`.** `Program.cs` binds `ScadaBridge:Node` once and reads `GrpcPort` / `CentralGrpcPort` / `MetricsPort` from it; the 8083/8084 defaults live only in `NodeOptions`. Central's dedicated metrics listener remains an explicit *presence* check on the raw key (a defaulted read would bind a new port on every existing central node). - **Inbound method warm-up is a hosted service** (`InboundMethodWarmupService`, registered after `AkkaHostedService`), not a pre-`RunAsync` loop — see Component-InboundAPI.md. - **`SitePskProvider` distinguishes a secret-store fault from a missing secret.** A store fault logs a Warning with the exception and throws `SecretStoreUnavailableException` (not cached); only a genuinely absent/empty/tombstoned secret produces the "seed `SB-GRPC-PSK-{site}`" message. +- **`SitePskProvider`'s cache follows an in-place rotation (Gitea #96).** Central replaces the secrets library's `ISecretCacheInvalidator` with `SitePskRotationInvalidator` (`AddSitePskRotationInvalidation`), which drops the site's cached key when `SB-GRPC-PSK-{site}` is written and publishes `SitePskInvalidated` on the `site-psk-invalidation` `DistributedPubSub` topic; `AkkaHostedService.RegisterCentralActors` starts the `SitePskInvalidationSubscriber` that applies the peer's broadcasts. A site's auth rejection and a `SitePsks` reload also drop the cache — see Component-Communication.md → *Authentication*. - Central registers `ExternalSystemGatewayOptions.DefinitionCacheTtl = 10 s` ahead of `AddExternalSystemGateway()` so an explicit config value wins; sites leave it off. diff --git a/src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/ISitePskProvider.cs b/src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/ISitePskProvider.cs index 509578d9..9c515fee 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/ISitePskProvider.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/ISitePskProvider.cs @@ -110,6 +110,12 @@ public static class ControlPlaneCredentials /// A null provider leaves the options untouched — the shape used by test-only and /// default constructors that never dial a gated endpoint. /// + /// + /// Also wraps the channel's HTTP handler in , so a + /// call the peer rejects as unauthenticated drops the cached key and the NEXT call re-reads + /// it (Gitea #96). The rejected call itself is not retried; every caller already retries + /// (heartbeats, stream reconnects, the next pull tick). + /// /// Channel options being built. /// Key provider, or null to leave the channel unauthenticated. /// The site this channel talks to. @@ -127,6 +133,78 @@ public static class ControlPlaneCredentials options.Credentials = ChannelCredentials.Create( ChannelCredentials.Insecure, ForSite(provider, siteId)); options.UnsafeUseInsecureChannelCallCredentials = true; + if (options.HttpHandler is not PskRejectionRefreshHandler) + { + // Null means "let GrpcChannel build its default", which is this same handler shape. + options.HttpHandler = new PskRejectionRefreshHandler( + provider, + siteId, + options.HttpHandler ?? new SocketsHttpHandler { EnableMultipleHttp2Connections = true }); + } + return options; } } + +/// +/// Drops a site's cached preshared key when the peer rejects a call as unauthenticated, so a key +/// rotated in place is re-read on the next call instead of being presented until a restart +/// (Gitea #96). +/// +/// +/// +/// Why an HTTP handler. Every per-site channel is built through +/// , which can see the channel's HTTP +/// handler but not its call invoker. The control-plane interceptors reject before the service +/// runs, which gRPC sends as a trailers-only response — the grpc-status rides the response +/// headers, where this handler can read it without touching the body. That covers unary and +/// streaming calls alike. +/// +/// +/// No throttle. An invalidation only costs the next call a re-read, which the secret +/// resolver serves from its own short-TTL cache; a site with a genuinely wrong key costs at most +/// one store read per resolver TTL. +/// +/// +public sealed class PskRejectionRefreshHandler : DelegatingHandler +{ + private readonly ISitePskProvider _provider; + private readonly string _siteId; + + /// Creates the handler. + /// The key cache to invalidate. + /// The site the channel talks to. + /// The transport handler. + public PskRejectionRefreshHandler( + ISitePskProvider provider, string siteId, HttpMessageHandler innerHandler) + : base(innerHandler) + { + ArgumentNullException.ThrowIfNull(provider); + ArgumentException.ThrowIfNullOrWhiteSpace(siteId); + _provider = provider; + _siteId = siteId; + } + + /// + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); + if (IsAuthRejection(response)) + { + _provider.Invalidate(_siteId); + } + + return response; + } + + /// + /// True when the response is a trailers-only gRPC PermissionDenied (7) or + /// Unauthenticated (16) — the statuses the control-plane interceptors reject with. + /// + /// The HTTP response. + /// Whether the call was rejected as unauthenticated. + internal static bool IsAuthRejection(HttpResponseMessage response) => + response.Headers.TryGetValues("grpc-status", out var values) + && values.FirstOrDefault() is "7" or "16"; +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Actors/AkkaHostedService.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Actors/AkkaHostedService.cs index e76b9caf..a7ff5a13 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Actors/AkkaHostedService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Actors/AkkaHostedService.cs @@ -463,6 +463,18 @@ akka {{ "CentralControlGrpcService readiness set (service bound: {Bound})", centralControlGrpc is not null); + // Peer half of site-PSK rotation invalidation (Gitea #96): subscribe to the topic, and give + // the invalidator the mediator so a rotation written on this node reaches the other one. + var pskRotation = _serviceProvider.GetService(); + if (pskRotation is not null) + { + _actorSystem!.ActorOf( + Props.Create(() => new SitePskInvalidationSubscriber(pskRotation)), + "site-psk-invalidation"); + pskRotation.SetMediator( + Akka.Cluster.Tools.PublishSubscribe.DistributedPubSub.Get(_actorSystem!).Mediator); + } + // Wire up the CommunicationService with the actor reference var commService = _serviceProvider.GetService(); commService?.SetCommunicationActor(centralCommActor); diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/CentralControlAuthInterceptor.cs b/src/ZB.MOM.WW.ScadaBridge.Host/CentralControlAuthInterceptor.cs index d0831092..85884b32 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/CentralControlAuthInterceptor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/CentralControlAuthInterceptor.cs @@ -158,10 +158,35 @@ public sealed class CentralControlAuthInterceptor : Interceptor throw Denied("missing site identity header"); } - string expected; + var expected = await ResolveExpectedAsync(siteId, context).ConfigureAwait(false); + var presented = ExtractBearerToken(context.RequestHeaders); + + if (presented is not null && !FixedTimeEquals(presented, expected)) + { + // Gitea #96: the cached key may predate an in-place rotation the site already runs + // on. Drop it and re-read once; the secret resolver's own short-TTL cache bounds what + // a stream of wrong tokens can cost the store. + _pskProvider.Invalidate(siteId); + expected = await ResolveExpectedAsync(siteId, context).ConfigureAwait(false); + } + + if (presented is null || !FixedTimeEquals(presented, expected)) + { + _logger.LogWarning( + "Rejected a central control-plane call to {Method} from site {SiteId}: {Reason}.", + context.Method, siteId, + presented is null ? "no bearer token presented" : "bearer token did not match"); + throw Denied("control plane authentication failed"); + } + + CentralControlSiteIdentity.SetVerified(context, siteId); + } + + private async Task ResolveExpectedAsync(string siteId, ServerCallContext context) + { try { - expected = await _pskProvider.GetAsync(siteId, context.CancellationToken) + return await _pskProvider.GetAsync(siteId, context.CancellationToken) .ConfigureAwait(false); } catch (OperationCanceledException) @@ -178,18 +203,6 @@ public sealed class CentralControlAuthInterceptor : Interceptor context.Method, siteId); throw Denied("no key configured for the presented site"); } - - var presented = ExtractBearerToken(context.RequestHeaders); - if (presented is null || !FixedTimeEquals(presented, expected)) - { - _logger.LogWarning( - "Rejected a central control-plane call to {Method} from site {SiteId}: {Reason}.", - context.Method, siteId, - presented is null ? "no bearer token presented" : "bearer token did not match"); - throw Denied("control plane authentication failed"); - } - - CentralControlSiteIdentity.SetVerified(context, siteId); } private static RpcException Denied(string reason) diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs index fd3ed532..80a63d24 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs @@ -411,6 +411,10 @@ try // options validation) and why the connection string can never itself be a ${secret:} // reference. builder.Services.AddScadaBridgeSecrets(builder.Configuration, SecretsNodeRole.Central); + // An in-place rotation of a site's SB-GRPC-PSK-{siteId} through /admin/secrets must drop + // central's cached copy of that key on BOTH central nodes (Gitea #96). Wraps the library's + // ISecretCacheInvalidator; the peer half is wired in AkkaHostedService.RegisterCentralActors. + builder.Services.AddSitePskRotationInvalidation(); // Secrets UI authorization: adds the named policies secrets:manage + secrets:reveal // (role-based) consumed by the /admin/secrets page. AddSecretsAuthorization only ADDS // these two policies via Configure — it composes additively with diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SitePskProvider.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SitePskProvider.cs index 13028b84..91c539b0 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/SitePskProvider.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SitePskProvider.cs @@ -22,19 +22,31 @@ namespace ZB.MOM.WW.ScadaBridge.Host; /// connection already uses for its per-connection API keys. /// /// -/// Caching. Successful resolves are cached indefinitely; a site's key changes only on -/// rotation, and rotation restarts the pair. Failures are deliberately NOT cached, so a key -/// seeded after the first (failed) dial is picked up on the next attempt instead of requiring a -/// restart. drops an entry when a site is removed or a key is rotated -/// in place. +/// Caching. Successful resolves are cached until something says the key may have changed. +/// Failures are deliberately NOT cached, so a key seeded after the first (failed) dial is picked +/// up on the next attempt instead of requiring a restart. drops an entry, +/// and four things call it (Gitea #96 — before that only site removal did, so an in-place +/// rotation left both central nodes dialling with the old key until they restarted): /// +/// +/// site removal (the gRPC client paths); +/// a write to SB-GRPC-PSK-{siteId} through this node's secret write path +/// (/admin/secrets) — , which also tells the peer +/// central node over DistributedPubSub; +/// a site rejecting a call with PermissionDenied/Unauthenticated +/// (ControlPlaneCredentials.WithSiteCredentials), so a key rotated out of band (the +/// secret CLI, a direct SQL write) is re-read on the next call — after the secret +/// resolver's own Secrets:ResolveCacheTtl at most; +/// a reload of ScadaBridge:Communication:SitePsks, which clears the whole cache. +/// +/// /// /// Fail-closed. A key absent from BOTH sources throws, and the dial that needed it fails. /// It never degrades to an unauthenticated call. The message names the exact config key and the /// exact secret name, so the fix is one setting or one secret CLI seed. /// /// -public sealed class SitePskProvider : ISitePskProvider +public sealed class SitePskProvider : ISitePskProvider, IDisposable { /// Prefix of the per-site secret name; the site identifier is appended verbatim. public const string SecretNamePrefix = "SB-GRPC-PSK-"; @@ -43,6 +55,7 @@ public sealed class SitePskProvider : ISitePskProvider private readonly ISecretResolver _resolver; private readonly IOptionsMonitor _options; private readonly ILogger _logger; + private readonly IDisposable? _optionsChange; /// Creates the provider over the host's secret resolver. /// Runtime secret resolver from the host container. @@ -64,6 +77,11 @@ public sealed class SitePskProvider : ISitePskProvider _resolver = resolver; _options = options; _logger = logger; + + // A reloaded SitePsks map may carry a rotated key for any site; drop everything so the + // next dial to each site re-reads its source. Re-reading is cheap (a dictionary lookup, + // or the secret resolver's own cache), so clearing all is simpler than diffing. + _optionsChange = options.OnChange((_, _) => _cache.Clear()); } /// Builds the secret name for a site. @@ -142,13 +160,42 @@ public sealed class SitePskProvider : ISitePskProvider } /// + /// + /// Case-insensitive: the rotation hook derives the site id from a SecretName, which the + /// secrets library lowercases, and a secret name addresses the same secret in any case. + /// public void Invalidate(string siteId) { - if (!string.IsNullOrWhiteSpace(siteId)) + if (string.IsNullOrWhiteSpace(siteId)) { - _cache.TryRemove(siteId, out _); + return; + } + + foreach (var key in _cache.Keys) + { + if (string.Equals(key, siteId, StringComparison.OrdinalIgnoreCase)) + { + _cache.TryRemove(key, out _); + } } } + + /// + /// Returns the site identifier a secret name is the PSK of, or null when the name is + /// not a name. The prefix matches in any case, because the + /// secrets library stores names lowercased. + /// + /// A secret name. + /// The site identifier, or null. + public static string? SiteIdFromSecretName(string secretName) => + secretName is not null + && secretName.Length > SecretNamePrefix.Length + && secretName.StartsWith(SecretNamePrefix, StringComparison.OrdinalIgnoreCase) + ? secretName[SecretNamePrefix.Length..] + : null; + + /// + public void Dispose() => _optionsChange?.Dispose(); } /// diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SitePskRotationInvalidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SitePskRotationInvalidator.cs new file mode 100644 index 00000000..70a98b14 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SitePskRotationInvalidator.cs @@ -0,0 +1,168 @@ +using Akka.Actor; +using Akka.Cluster.Tools.PublishSubscribe; +using Akka.Event; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; +using ZB.MOM.WW.ScadaBridge.Communication.Grpc; +using ZB.MOM.WW.Secrets; +using ZB.MOM.WW.Secrets.Abstractions; + +namespace ZB.MOM.WW.ScadaBridge.Host; + +/// +/// Central's : forwards every eviction to the secret +/// resolver's cache as before, and when the evicted secret is a site's gRPC preshared key +/// (SB-GRPC-PSK-{siteId}) also drops that site's entry from +/// and tells the peer central node to do the same (Gitea #96). +/// +/// +/// +/// Why the invalidator seam. Every ZB.MOM.WW.Secrets write path — the +/// /admin/secrets editor and delete modal — calls +/// right after a rotate or delete. Hooking that seam catches an in-place PSK rotation at the +/// moment it is written, without ScadaBridge owning a write path of its own. +/// +/// +/// Why the peer needs telling. Both central nodes cache the key in their own +/// (and their own resolver cache), and the write only ran on one of +/// them. The peer is told over the central DistributedPubSub mediator on +/// — the same mediator the peer health replication uses. A node receives its +/// own broadcast too; applying it twice is harmless, so there is no origin filtering. +/// +/// +/// Writes made outside this process (the secret CLI, a direct SQL write) never reach this +/// seam; the site's rejection of the stale key covers those (see +/// ControlPlaneCredentials.WithSiteCredentials). +/// +/// +public sealed class SitePskRotationInvalidator : ISecretCacheInvalidator +{ + /// The DistributedPubSub topic the central nodes exchange PSK invalidations on. + public const string Topic = "site-psk-invalidation"; + + private readonly ISecretCacheInvalidator _inner; + private readonly ISitePskProvider _pskProvider; + private readonly ILogger _logger; + private volatile IActorRef? _mediator; + + /// Creates the invalidator. + /// The library's own invalidator (the secret resolver's cache). + /// Central's per-site PSK cache. + /// Logger. + public SitePskRotationInvalidator( + ISecretCacheInvalidator inner, + ISitePskProvider pskProvider, + ILogger logger) + { + ArgumentNullException.ThrowIfNull(inner); + ArgumentNullException.ThrowIfNull(pskProvider); + ArgumentNullException.ThrowIfNull(logger); + _inner = inner; + _pskProvider = pskProvider; + _logger = logger; + } + + /// + /// Supplies the DistributedPubSub mediator once the actor system exists. Until then + /// (and on a host without one) evictions stay local. + /// + /// The mediator to publish peer invalidations to. + public void SetMediator(IActorRef mediator) => _mediator = mediator; + + /// + public void Invalidate(SecretName name) + { + _inner.Invalidate(name); + + var siteId = SitePskProvider.SiteIdFromSecretName(name.Value); + if (siteId is null) + { + return; + } + + _pskProvider.Invalidate(siteId); + _logger.LogInformation( + "gRPC preshared key secret '{SecretName}' was written; dropped the cached key for site " + + "{SiteId} and notified the peer central node.", name.Value, siteId); + _mediator?.Tell(new Publish(Topic, new SitePskInvalidated(siteId))); + } + + /// + /// Applies an invalidation received from a central node (the peer, or this node's own + /// broadcast): evicts the resolver's cached secret and the PSK cache entry. Never re-publishes. + /// + /// The site whose key was rotated. + public void ApplyPeerInvalidation(string siteId) + { + if (string.IsNullOrWhiteSpace(siteId)) + { + return; + } + + _inner.Invalidate(new SecretName(SitePskProvider.SecretNameFor(siteId))); + _pskProvider.Invalidate(siteId); + } +} + +/// +/// Peer message: the gRPC preshared key of was rotated on a central node. +/// +/// The site whose key changed. +public sealed record SitePskInvalidated(string SiteId); + +/// +/// Central actor that subscribes to and applies +/// each to this node's caches. +/// +public sealed class SitePskInvalidationSubscriber : ReceiveActor +{ + private readonly SitePskRotationInvalidator _invalidator; + + /// Creates the subscriber. + /// This node's invalidator. + public SitePskInvalidationSubscriber(SitePskRotationInvalidator invalidator) + { + _invalidator = invalidator; + Receive(m => _invalidator.ApplyPeerInvalidation(m.SiteId)); + Receive(_ => { }); + } + + /// + protected override void PreStart() + { + try + { + DistributedPubSub.Get(Context.System).Mediator.Tell( + new Subscribe(SitePskRotationInvalidator.Topic, Self)); + } + catch (Exception ex) + { + // Non-clustered host (TestKit): peer invalidation is simply unavailable. + Context.GetLogger().Debug("DistributedPubSub not available — peer PSK invalidation disabled: {0}", ex.Message); + } + } +} + +/// DI wiring for . +public static class SitePskRotationServiceCollectionExtensions +{ + /// + /// Replaces the secrets library's with + /// wrapping the library's resolver. Central only; + /// call after AddScadaBridgeSecrets and after is + /// registered. + /// + /// The service collection. + /// The same collection. + public static IServiceCollection AddSitePskRotationInvalidation(this IServiceCollection services) + { + services.TryAddSingleton(sp => new SitePskRotationInvalidator( + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService>())); + services.Replace(ServiceDescriptor.Singleton( + sp => sp.GetRequiredService())); + return services; + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CentralControlAuthInterceptorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CentralControlAuthInterceptorTests.cs index 49f5cc43..8283b9ad 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CentralControlAuthInterceptorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CentralControlAuthInterceptorTests.cs @@ -274,4 +274,58 @@ public class CentralControlAuthInterceptorTests CentralControlAuthInterceptor.DefaultGatedPrefixes, p => p == $"/{method}/"); } + /// A provider whose cached key is stale until runs. + private sealed class RotatingPskProvider(string cached, string rotated) : ISitePskProvider + { + private string _current = cached; + + /// Gets the number of invalidations. + public int Invalidations { get; private set; } + + /// + public ValueTask GetAsync(string siteId, CancellationToken ct) => new(_current); + + /// + public void Invalidate(string siteId) + { + Invalidations++; + _current = rotated; + } + } + + /// + /// Gitea #96: a site already on its rotated key is accepted even while central's cached copy + /// is stale — the mismatch drops the cache and re-reads once. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task StaleCachedKey_IsReReadOnce_SoARotatedSiteIsAccepted() + { + var provider = new RotatingPskProvider("pre-rotation", "rotated"); + var interceptor = new CentralControlAuthInterceptor( + provider, NullLogger.Instance); + + var result = await Invoke(interceptor, CreateContext(ControlMethod, SiteA, "Bearer rotated")); + + Assert.Equal("ok", result); + Assert.Equal(1, provider.Invalidations); + } + + /// Negative control: a wrong token is still refused after the one re-read, and a right one never invalidates. + /// A task that represents the asynchronous operation. + [Fact] + public async Task WrongToken_IsStillRefused_AfterTheReRead_AndAMatchNeverInvalidates() + { + var provider = new RotatingPskProvider("k", "k"); + var interceptor = new CentralControlAuthInterceptor( + provider, NullLogger.Instance); + + Assert.Equal("ok", await Invoke(interceptor, CreateContext(ControlMethod, SiteA, "Bearer k"))); + Assert.Equal(0, provider.Invalidations); + + var ex = await Assert.ThrowsAsync( + () => Invoke(interceptor, CreateContext(ControlMethod, SiteA, "Bearer wrong"))); + Assert.Equal(StatusCode.PermissionDenied, ex.StatusCode); + Assert.Equal(1, provider.Invalidations); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs index 4dbcf466..7de164c5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs @@ -407,6 +407,18 @@ public class CentralCompositionRootTests : IDisposable Assert.Null(_factory.Services.GetService()); } + /// + /// Gitea #96: the secrets write path's + /// must be the PSK-aware wrapper on central, or an in-place site-PSK rotation through + /// /admin/secrets never reaches the PSK cache. + /// + [Fact] + public void Central_SecretCacheInvalidator_IsTheSitePskRotationInvalidator() + { + Assert.IsType( + _factory.Services.GetService()); + } + /// Verifies that the shared audit seam resolves to the central direct-write bridge. [Fact] public void Central_SharedAuditSeam_IsTheCentralDirectWriteBridge() diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthEndToEndTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthEndToEndTests.cs index 5893d509..306e825f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthEndToEndTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ControlPlaneAuthEndToEndTests.cs @@ -7,8 +7,10 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; +using NSubstitute; using ZB.MOM.WW.ScadaBridge.Communication; using ZB.MOM.WW.ScadaBridge.Communication.Grpc; +using ZB.MOM.WW.Secrets.Abstractions; namespace ZB.MOM.WW.ScadaBridge.Host.Tests; @@ -267,4 +269,87 @@ public class ControlPlaneAuthEndToEndTests : IAsyncLifetime var service = _host.Services.GetRequiredService(); Assert.Equal("site-a", service.LastSiteHeader); } + // --- Gitea #96: a site's rejection drops central's cached key --------------------------- + + /// + /// Central's real over a store that served a stale key first and + /// the rotated (site-accepted) key after: the in-place rotation scenario. + /// + private static (SitePskProvider Provider, ISecretResolver Resolver) RotatedProvider() + { + var resolver = Substitute.For(); + resolver.GetAsync(Arg.Any(), Arg.Any()) + .Returns("the-pre-rotation-key", SiteKey); + var options = Substitute.For>(); + options.CurrentValue.Returns(new CommunicationOptions()); + return (new SitePskProvider(resolver, options, NullLogger.Instance), resolver); + } + + private GrpcChannel Channel(ISitePskProvider provider) => + GrpcChannel.ForAddress(_server.BaseAddress, new GrpcChannelOptions { HttpHandler = _server.CreateHandler() } + .WithMessageLimits().WithSiteCredentials(provider, "site-a")); + + /// + /// Verifies that after a site rejects the cached (pre-rotation) key on a unary call, the next + /// call re-reads the key and is accepted — no restart needed. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task RejectedUnaryCall_DropsTheCachedKey_SoTheNextCallUsesTheRotatedOne() + { + var (provider, _) = RotatedProvider(); + using var channel = Channel(provider); + var client = new SiteStreamService.SiteStreamServiceClient(channel); + + var ex = await Assert.ThrowsAsync( + async () => await client.PullAuditEventsAsync(new PullAuditEventsRequest())); + Assert.Equal(StatusCode.PermissionDenied, ex.StatusCode); + + var reply = await client.PullAuditEventsAsync(new PullAuditEventsRequest { BatchSize = 1 }); + Assert.False(reply.MoreAvailable); + } + + /// Verifies the same refresh for a rejected streaming call. + /// A task that represents the asynchronous operation. + [Fact] + public async Task RejectedStreamingCall_DropsTheCachedKey_SoTheNextCallUsesTheRotatedOne() + { + var (provider, _) = RotatedProvider(); + using var channel = Channel(provider); + var client = new SiteStreamService.SiteStreamServiceClient(channel); + + using (var rejected = client.SubscribeInstance(new InstanceStreamRequest { CorrelationId = "c1" })) + { + var ex = await Assert.ThrowsAsync( + async () => await rejected.ResponseStream.MoveNext(CancellationToken.None)); + Assert.Equal(StatusCode.PermissionDenied, ex.StatusCode); + } + + using var accepted = client.SubscribeInstance(new InstanceStreamRequest { CorrelationId = "c2" }); + Assert.True(await accepted.ResponseStream.MoveNext(CancellationToken.None)); + Assert.Equal("c2", accepted.ResponseStream.Current.CorrelationId); + } + + /// + /// Negative control: accepted calls never drop the cache, so the store is read once no + /// matter how many calls succeed. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task AcceptedCalls_KeepTheCachedKey() + { + var resolver = Substitute.For(); + resolver.GetAsync(Arg.Any(), Arg.Any()).Returns(SiteKey); + var options = Substitute.For>(); + options.CurrentValue.Returns(new CommunicationOptions()); + var provider = new SitePskProvider(resolver, options, NullLogger.Instance); + using var channel = Channel(provider); + var client = new SiteStreamService.SiteStreamServiceClient(channel); + + await client.PullAuditEventsAsync(new PullAuditEventsRequest()); + await client.PullAuditEventsAsync(new PullAuditEventsRequest()); + await client.PullAuditEventsAsync(new PullAuditEventsRequest()); + + await resolver.Received(1).GetAsync(Arg.Any(), Arg.Any()); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SitePskProviderTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SitePskProviderTests.cs index 661f4e5b..87f3ab64 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SitePskProviderTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SitePskProviderTests.cs @@ -204,4 +204,61 @@ public class SitePskProviderTests Assert.Equal("key-a", await provider.GetAsync("site-a", CancellationToken.None)); Assert.Equal("key-b", await provider.GetAsync("site-b", CancellationToken.None)); } + /// + /// Verifies that a reload of the configured SitePsks map drops cached keys, so a key + /// rotated in configuration is used without a restart (Gitea #96). + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task ReloadedSitePsks_DropsTheCache_SoAKeyRotatedInConfigIsUsed() + { + var resolver = Substitute.For(); + var options = new CommunicationOptions(); + options.SitePsks["site-a"] = "old"; + Action? listener = null; + var monitor = Substitute.For>(); + monitor.CurrentValue.Returns(_ => options); + monitor.OnChange(Arg.Do>(l => listener = l)); + using var provider = new SitePskProvider(resolver, monitor, NullLogger.Instance); + + Assert.Equal("old", await provider.GetAsync("site-a", CancellationToken.None)); + + options = new CommunicationOptions(); + options.SitePsks["site-a"] = "rotated"; + Assert.NotNull(listener); + listener!(options, null); + + Assert.Equal("rotated", await provider.GetAsync("site-a", CancellationToken.None)); + } + + /// Verifies the secret-name to site-id mapping the rotation hook relies on. + /// A secret name. + /// The expected site id, or null. + [Theory] + [InlineData("SB-GRPC-PSK-site-a", "site-a")] + [InlineData("SB-GRPC-PSK-", null)] + [InlineData("OTHER-SECRET", null)] + [InlineData("sb-grpc-psk-site-a", "site-a")] + public void SiteIdFromSecretName_MapsOnlyPskNames(string name, string? expected) + { + Assert.Equal(expected, SitePskProvider.SiteIdFromSecretName(name)); + } + /// + /// Verifies that invalidation ignores case: the rotation hook derives the site id from a + /// lowercased secret name, but the site may be cached under its mixed-case identifier. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Invalidate_IgnoresCase_BecauseSecretNamesAreLowercased() + { + var resolver = Substitute.For(); + resolver.GetAsync(Arg.Any(), Arg.Any()) + .Returns("old", "rotated"); + var provider = Create(resolver); + + Assert.Equal("old", await provider.GetAsync("Site-A", CancellationToken.None)); + provider.Invalidate("site-a"); + + Assert.Equal("rotated", await provider.GetAsync("Site-A", CancellationToken.None)); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SitePskRotationInvalidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SitePskRotationInvalidatorTests.cs new file mode 100644 index 00000000..c03a9517 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SitePskRotationInvalidatorTests.cs @@ -0,0 +1,89 @@ +using Akka.Actor; +using Akka.Cluster.Tools.PublishSubscribe; +using Akka.TestKit.Xunit2; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Communication.Grpc; +using ZB.MOM.WW.Secrets.Abstractions; + +namespace ZB.MOM.WW.ScadaBridge.Host.Tests; + +/// +/// Gitea #96: an in-place rotation of SB-GRPC-PSK-{siteId} written through the secrets +/// write path must drop central's cached key on this node and on the peer central node. +/// +public class SitePskRotationInvalidatorTests : TestKit +{ + private readonly ISecretCacheInvalidator _inner = Substitute.For(); + private readonly ISitePskProvider _psk = Substitute.For(); + + private SitePskRotationInvalidator Create() => + new(_inner, _psk, NullLogger.Instance); + + /// + /// Verifies that writing a site's PSK secret evicts the resolver cache, drops the site's + /// cached key and publishes the invalidation to the peer. + /// + [Fact] + public void WritingAPskSecret_DropsTheCachedKey_AndTellsThePeer() + { + var mediator = CreateTestProbe(); + var invalidator = Create(); + invalidator.SetMediator(mediator.Ref); + + // The secrets library lowercases names, so the site id arrives lowercased too. + invalidator.Invalidate(new SecretName("SB-GRPC-PSK-site-a")); + + _inner.Received(1).Invalidate(new SecretName("SB-GRPC-PSK-site-a")); + _psk.Received(1).Invalidate("site-a"); + var publish = mediator.ExpectMsg(); + Assert.Equal(SitePskRotationInvalidator.Topic, publish.Topic); + Assert.Equal(new SitePskInvalidated("site-a"), publish.Message); + } + + /// Negative control: any other secret only evicts the resolver cache. + [Fact] + public void WritingAnyOtherSecret_OnlyEvictsTheResolverCache() + { + var mediator = CreateTestProbe(); + var invalidator = Create(); + invalidator.SetMediator(mediator.Ref); + + invalidator.Invalidate(new SecretName("SMTP-PASSWORD")); + + _inner.Received(1).Invalidate(new SecretName("SMTP-PASSWORD")); + _psk.DidNotReceiveWithAnyArgs().Invalidate(default!); + mediator.ExpectNoMsg(TimeSpan.FromMilliseconds(200)); + } + + /// Verifies that before the actor system exists an invalidation still applies locally. + [Fact] + public void WithoutAMediator_TheLocalInvalidationStillApplies() + { + Create().Invalidate(new SecretName("SB-GRPC-PSK-site-a")); + + _psk.Received(1).Invalidate("site-a"); + } + + /// + /// Verifies that the subscriber applies a peer's invalidation to this node's resolver and PSK + /// caches, without re-publishing it. + /// + [Fact] + public void Subscriber_AppliesAPeerInvalidation_WithoutRepublishing() + { + var mediator = CreateTestProbe(); + var invalidator = Create(); + invalidator.SetMediator(mediator.Ref); + var subscriber = Sys.ActorOf(Props.Create(() => new SitePskInvalidationSubscriber(invalidator))); + + subscriber.Tell(new SitePskInvalidated("site-b")); + + AwaitAssert(() => + { + _psk.Received(1).Invalidate("site-b"); + _inner.Received(1).Invalidate(new SecretName("SB-GRPC-PSK-site-b")); + }); + mediator.ExpectNoMsg(TimeSpan.FromMilliseconds(200)); + } +}