From ca0cf19a1f23f47627dac0511c79fbf40c73ce0c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:12:02 -0400 Subject: [PATCH 1/8] fix(auth): aggregate bulk constraint denials into one audit event; ValueTask constraint checks A bulk command with N denied items wrote N audit events into the bounded, drop-on-full audit channel, so a low-privilege key could evict every other pending audit record. Bulk filters now accumulate a ConstraintDenialSummary (count, constraint names, <=20 sample targets) and write one aggregated constraint-denied event per command. The per-item constraint checks are synchronous, so IConstraintEnforcer's Check* methods now return ValueTask instead of allocating a Task per item. --- docs/Authorization.md | 14 ++- .../WorkerLiveMxAccessSmokeTests.cs | 20 +++-- .../Grpc/MxAccessGatewayService.cs | 54 +++++++---- .../Authorization/ConstraintDenialSummary.cs | 90 +++++++++++++++++++ .../Authorization/ConstraintEnforcer.cs | 81 +++++++++++++---- .../Authorization/IConstraintEnforcer.cs | 34 ++++++- .../MxAccessGatewayServiceConstraintTests.cs | 52 ++++++++++- .../Authorization/ConstraintEnforcerTests.cs | 64 +++++++++++++ .../TestSupport/AllowAllConstraintEnforcer.cs | 20 +++-- .../PredicateConstraintEnforcer.cs | 33 +++++-- 10 files changed, 400 insertions(+), 62 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintDenialSummary.cs diff --git a/docs/Authorization.md b/docs/Authorization.md index c22425a..ed5ec8b 100644 --- a/docs/Authorization.md +++ b/docs/Authorization.md @@ -262,9 +262,17 @@ the gateway fails closed. Non-bulk constraint failures return gRPC `PermissionDenied`. Bulk read commands preserve input order and return a failed `SubscribeResult` for each -denied item while still forwarding allowed items to the worker. Every denial -adds an `api_key_audit` entry with the key id, command kind, target, and -blocking constraint; secured values and raw credentials are never logged. +denied item while still forwarding allowed items to the worker. A non-bulk +denial adds one `constraint-denied` audit event (canonical `audit_event` table) +with the key id, command kind, target, and blocking constraint. A bulk command +adds **one aggregated** `constraint-denied` event per command, not one per +denied item: its target is `:bulk` and its details carry the +denied and total item counts, the distinct blocking constraint names, a sample +of at most 20 denied targets (`sampleTruncated` says whether more were +denied), and the client correlation id. The audit writer is a bounded channel +that drops on full, so per-item events let a single low-privilege key flush +every other pending audit record with one large bulk command. Secured values +and raw credentials are never logged. ## Scope Catalog diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs index fcc2506..ed0acf1 100644 --- a/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs @@ -2112,26 +2112,26 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) private sealed class AllowAllConstraintEnforcer : IConstraintEnforcer { /// - public Task CheckReadTagAsync( + public ValueTask CheckReadTagAsync( ApiKeyIdentity? identity, string tagAddress, - CancellationToken cancellationToken) => Task.FromResult(null); + CancellationToken cancellationToken) => ValueTask.FromResult(null); /// - public Task CheckReadHandleAsync( + public ValueTask CheckReadHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, int itemHandle, - CancellationToken cancellationToken) => Task.FromResult(null); + CancellationToken cancellationToken) => ValueTask.FromResult(null); /// - public Task CheckWriteHandleAsync( + public ValueTask CheckWriteHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, int itemHandle, - CancellationToken cancellationToken) => Task.FromResult(null); + CancellationToken cancellationToken) => ValueTask.FromResult(null); /// public Task RecordDenialAsync( @@ -2141,6 +2141,14 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) ConstraintFailure failure, string? correlationId, CancellationToken cancellationToken) => Task.CompletedTask; + + /// + public Task RecordBulkDenialAsync( + ApiKeyIdentity? identity, + string commandKind, + ConstraintDenialSummary summary, + string? correlationId, + CancellationToken cancellationToken) => Task.CompletedTask; } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs index 99ba149..96688a8 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs @@ -488,6 +488,7 @@ public sealed class MxAccessGatewayService( Dictionary denied = []; List allowed = []; + ConstraintDenialSummary? denialSummary = null; for (int index = 0; index < tagAddresses.Count; index++) { string tagAddress = tagAddresses[index]; @@ -500,8 +501,7 @@ public sealed class MxAccessGatewayService( continue; } - await constraintEnforcer.RecordDenialAsync(identity, command.Kind.ToString(), tagAddress, failure, correlationId, cancellationToken) - .ConfigureAwait(false); + (denialSummary ??= new ConstraintDenialSummary(tagAddresses.Count)).Add(tagAddress, failure); denied[index] = new SubscribeResult { ServerHandle = serverHandle, @@ -511,11 +511,17 @@ public sealed class MxAccessGatewayService( }; } - if (denied.Count == 0) + if (denialSummary is null) { return null; } + // One aggregated audit event per bulk command, not one per denied item (see + // ConstraintDenialSummary for why). + await constraintEnforcer + .RecordBulkDenialAsync(identity, command.Kind.ToString(), denialSummary, correlationId, cancellationToken) + .ConfigureAwait(false); + // Build the filtered command directly instead of cloning the original and clearing it: // the clone deep-copied every denied address only to drop it. The payload's other fields // (server_handle) are copied across explicitly. Nothing aliases the request here — these @@ -556,6 +562,7 @@ public sealed class MxAccessGatewayService( Dictionary denied = []; List allowed = []; + ConstraintDenialSummary? denialSummary = null; for (int index = 0; index < tagAddresses.Count; index++) { string tagAddress = tagAddresses[index]; @@ -568,8 +575,7 @@ public sealed class MxAccessGatewayService( continue; } - await constraintEnforcer.RecordDenialAsync(identity, command.Kind.ToString(), tagAddress, failure, correlationId, cancellationToken) - .ConfigureAwait(false); + (denialSummary ??= new ConstraintDenialSummary(tagAddresses.Count)).Add(tagAddress, failure); denied[index] = new BulkReadResult { ServerHandle = serverHandle, @@ -580,11 +586,17 @@ public sealed class MxAccessGatewayService( }; } - if (denied.Count == 0) + if (denialSummary is null) { return null; } + // One aggregated audit event per bulk command, not one per denied item (see + // ConstraintDenialSummary for why). + await constraintEnforcer + .RecordBulkDenialAsync(identity, command.Kind.ToString(), denialSummary, correlationId, cancellationToken) + .ConfigureAwait(false); + MxCommand filtered = new() { Kind = command.Kind }; ReadBulkCommand payload = new() { @@ -621,6 +633,7 @@ public sealed class MxAccessGatewayService( Dictionary denied = []; List allowed = []; + ConstraintDenialSummary? denialSummary = null; for (int index = 0; index < entries.Count; index++) { TEntry entry = entries[index]; @@ -634,14 +647,7 @@ public sealed class MxAccessGatewayService( continue; } - await constraintEnforcer.RecordDenialAsync( - identity, - command.Kind.ToString(), - itemHandle.ToString(System.Globalization.CultureInfo.InvariantCulture), - failure, - correlationId, - cancellationToken) - .ConfigureAwait(false); + (denialSummary ??= new ConstraintDenialSummary(entries.Count)).Add(itemHandle, failure); denied[index] = new BulkWriteResult { ServerHandle = serverHandle, @@ -651,11 +657,17 @@ public sealed class MxAccessGatewayService( }; } - if (denied.Count == 0) + if (denialSummary is null) { return null; } + // One aggregated audit event per bulk command, not one per denied item (see + // ConstraintDenialSummary for why). + await constraintEnforcer + .RecordBulkDenialAsync(identity, command.Kind.ToString(), denialSummary, correlationId, cancellationToken) + .ConfigureAwait(false); + return new WriteBulkConstraintPlan( BuildFilteredWriteBulkCommand(command, allowed), entries.Count, @@ -742,6 +754,7 @@ public sealed class MxAccessGatewayService( Dictionary denied = []; List allowed = []; + ConstraintDenialSummary? denialSummary = null; for (int index = 0; index < itemHandles.Count; index++) { int itemHandle = itemHandles[index]; @@ -754,8 +767,7 @@ public sealed class MxAccessGatewayService( continue; } - await constraintEnforcer.RecordDenialAsync(identity, command.Kind.ToString(), itemHandle.ToString(System.Globalization.CultureInfo.InvariantCulture), failure, correlationId, cancellationToken) - .ConfigureAwait(false); + (denialSummary ??= new ConstraintDenialSummary(itemHandles.Count)).Add(itemHandle, failure); denied[index] = new SubscribeResult { ServerHandle = serverHandle, @@ -765,11 +777,17 @@ public sealed class MxAccessGatewayService( }; } - if (denied.Count == 0) + if (denialSummary is null) { return null; } + // One aggregated audit event per bulk command, not one per denied item (see + // ConstraintDenialSummary for why). + await constraintEnforcer + .RecordBulkDenialAsync(identity, command.Kind.ToString(), denialSummary, correlationId, cancellationToken) + .ConfigureAwait(false); + MxCommand filtered = new() { Kind = command.Kind }; AdviseItemBulkCommand payload = new() { ServerHandle = command.AdviseItemBulk.ServerHandle }; payload.ItemHandles.Add(allowed); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintDenialSummary.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintDenialSummary.cs new file mode 100644 index 0000000..bde3db7 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintDenialSummary.cs @@ -0,0 +1,90 @@ +using System.Globalization; + +namespace ZB.MOM.WW.MxGateway.Server.Security.Authorization; + +/// +/// Accumulates the constraint denials of one bulk command so they are audited as a single +/// aggregated event rather than one event per denied item. +/// +/// +/// A bulk command can carry thousands of items, and the audit writer is a bounded, drop-on-full +/// channel. One audit event per denied item let a single low-privilege key flood that channel and +/// push every other audit record out of it. The summary keeps the forensic signal — how many items +/// were denied, which constraints denied them, and a bounded sample of the targets — at a fixed +/// cost of one audit event per command. +/// +public sealed class ConstraintDenialSummary +{ + /// The maximum number of denied targets retained as a sample. + public const int MaxSampleTargets = 20; + + private readonly List _constraintNames = []; + private readonly List _sampleTargets = []; + + /// Initializes a new instance of the class. + /// The number of items the bulk command carried. + public ConstraintDenialSummary(int totalCount) + { + TotalCount = totalCount; + } + + /// Gets the number of items the bulk command carried. + public int TotalCount { get; } + + /// Gets the number of items denied. + public int DeniedCount { get; private set; } + + /// Gets the distinct constraint names that denied at least one item, in first-seen order. + public IReadOnlyList ConstraintNames => _constraintNames; + + /// Gets up to denied targets, in item order. + public IReadOnlyList SampleTargets => _sampleTargets; + + /// Gets a value indicating whether more items were denied than the sample retains. + public bool SampleTruncated => DeniedCount > _sampleTargets.Count; + + /// Records one denied tag-address item. + /// The denied tag address. + /// The constraint failure that denied it. + public void Add(string target, ConstraintFailure failure) + { + ArgumentNullException.ThrowIfNull(failure); + AddConstraint(failure.ConstraintName); + if (_sampleTargets.Count < MaxSampleTargets) + { + _sampleTargets.Add(target); + } + + DeniedCount++; + } + + /// Records one denied item-handle item. + /// The denied MXAccess item handle. + /// The constraint failure that denied it. + /// The handle is formatted only while the sample still has room. + public void Add(int itemHandle, ConstraintFailure failure) + { + ArgumentNullException.ThrowIfNull(failure); + AddConstraint(failure.ConstraintName); + if (_sampleTargets.Count < MaxSampleTargets) + { + _sampleTargets.Add(itemHandle.ToString(CultureInfo.InvariantCulture)); + } + + DeniedCount++; + } + + private void AddConstraint(string constraintName) + { + // The constraint vocabulary is a handful of fixed names, so a linear scan beats a set. + for (int index = 0; index < _constraintNames.Count; index++) + { + if (string.Equals(_constraintNames[index], constraintName, StringComparison.Ordinal)) + { + return; + } + } + + _constraintNames.Add(constraintName); + } +} diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintEnforcer.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintEnforcer.cs index a61bcfd..21ca812 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintEnforcer.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ConstraintEnforcer.cs @@ -25,7 +25,7 @@ public sealed class ConstraintEnforcer( identity?.EffectiveConstraints.HasWriteConstraints ?? false; /// - public Task CheckReadTagAsync( + public ValueTask CheckReadTagAsync( ApiKeyIdentity? identity, string tagAddress, CancellationToken cancellationToken) @@ -33,14 +33,14 @@ public sealed class ConstraintEnforcer( ApiKeyConstraints constraints = identity?.EffectiveConstraints ?? ApiKeyConstraints.Empty; if (!constraints.HasReadConstraints) { - return Task.FromResult(null); + return ValueTask.FromResult(null); } - return Task.FromResult(CheckReadTarget(constraints, tagAddress)); + return ValueTask.FromResult(CheckReadTarget(constraints, tagAddress)); } /// - public Task CheckReadHandleAsync( + public ValueTask CheckReadHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, @@ -50,19 +50,19 @@ public sealed class ConstraintEnforcer( ApiKeyConstraints constraints = identity?.EffectiveConstraints ?? ApiKeyConstraints.Empty; if (!constraints.HasReadConstraints) { - return Task.FromResult(null); + return ValueTask.FromResult(null); } if (!session.TryGetItemRegistration(serverHandle, itemHandle, out SessionItemRegistration registration)) { - return Task.FromResult(new ConstraintFailure("item_handle", "Item handle is not registered in the constrained session.")); + return ValueTask.FromResult(new ConstraintFailure("item_handle", "Item handle is not registered in the constrained session.")); } - return Task.FromResult(CheckReadTarget(constraints, registration.TagAddress)); + return ValueTask.FromResult(CheckReadTarget(constraints, registration.TagAddress)); } /// - public Task CheckWriteHandleAsync( + public ValueTask CheckWriteHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, @@ -72,23 +72,23 @@ public sealed class ConstraintEnforcer( ApiKeyConstraints constraints = identity?.EffectiveConstraints ?? ApiKeyConstraints.Empty; if (!constraints.HasWriteConstraints) { - return Task.FromResult(null); + return ValueTask.FromResult(null); } if (!session.TryGetItemRegistration(serverHandle, itemHandle, out SessionItemRegistration registration)) { - return Task.FromResult(new ConstraintFailure("item_handle", "Item handle is not registered in the constrained session.")); + return ValueTask.FromResult(new ConstraintFailure("item_handle", "Item handle is not registered in the constrained session.")); } GalaxyTagLookup? target = ResolveTarget(registration.TagAddress); if (target is null) { - return Task.FromResult(new ConstraintFailure("tag_metadata", "Tag metadata is not available in the Galaxy hierarchy cache.")); + return ValueTask.FromResult(new ConstraintFailure("tag_metadata", "Tag metadata is not available in the Galaxy hierarchy cache.")); } if (!MatchesPathOrTag(target.ContainedPath, registration.TagAddress, constraints.WriteSubtrees, constraints.WriteTagGlobs)) { - return Task.FromResult(new ConstraintFailure("write_scope", "Tag is outside the API key write scope.")); + return ValueTask.FromResult(new ConstraintFailure("write_scope", "Tag is outside the API key write scope.")); } if (constraints.MaxWriteClassification is { } maxClassification) @@ -96,18 +96,18 @@ public sealed class ConstraintEnforcer( GalaxyAttribute? attribute = target.Attribute; if (attribute is null) { - return Task.FromResult(new ConstraintFailure("max_write_classification", "Attribute security classification is not available.")); + return ValueTask.FromResult(new ConstraintFailure("max_write_classification", "Attribute security classification is not available.")); } if (attribute.SecurityClassification > maxClassification) { - return Task.FromResult(new ConstraintFailure( + return ValueTask.FromResult(new ConstraintFailure( "max_write_classification", $"Attribute security classification {attribute.SecurityClassification} exceeds allowed maximum {maxClassification}.")); } } - return Task.FromResult(null); + return ValueTask.FromResult(null); } /// @@ -150,6 +150,46 @@ public sealed class ConstraintEnforcer( await auditWriter.WriteAsync(auditEvent, cancellationToken).ConfigureAwait(false); } + /// + public async Task RecordBulkDenialAsync( + ApiKeyIdentity? identity, + string commandKind, + ConstraintDenialSummary summary, + string? correlationId, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(summary); + if (summary.DeniedCount == 0) + { + return; + } + + // One event for the whole command. Target stays ":"-shaped so audit + // queries that split on ':' keep working; the "bulk" target marks the aggregated form. + AuditEvent auditEvent = new() + { + EventId = Guid.NewGuid(), + OccurredAtUtc = DateTimeOffset.UtcNow, + Actor = identity?.KeyId ?? "anonymous", + Action = "constraint-denied", + Outcome = AuditOutcome.Denied, + Category = CanonicalForwardingApiKeyAuditStore.ApiKeyCategory, + Target = $"{commandKind}:bulk", + SourceNode = null, + CorrelationId = Guid.TryParse(correlationId, out var cid) ? cid : (Guid?)null, + DetailsJson = JsonSerializer.Serialize(new BulkDenialDetails( + summary.ConstraintNames, + commandKind, + summary.DeniedCount, + summary.TotalCount, + summary.SampleTargets, + summary.SampleTruncated, + correlationId ?? "")), + }; + + await auditWriter.WriteAsync(auditEvent, cancellationToken).ConfigureAwait(false); + } + private ConstraintFailure? CheckReadTarget( ApiKeyConstraints constraints, string tagAddress) @@ -240,4 +280,15 @@ public sealed class ConstraintEnforcer( return false; } + + // DetailsJson shape of an aggregated bulk denial. Serialized with camelCase names to match the + // single-item envelope's keys ("commandKind", "clientCorrelationId"). + private sealed record BulkDenialDetails( + [property: System.Text.Json.Serialization.JsonPropertyName("constraints")] IReadOnlyList Constraints, + [property: System.Text.Json.Serialization.JsonPropertyName("commandKind")] string CommandKind, + [property: System.Text.Json.Serialization.JsonPropertyName("deniedCount")] int DeniedCount, + [property: System.Text.Json.Serialization.JsonPropertyName("totalCount")] int TotalCount, + [property: System.Text.Json.Serialization.JsonPropertyName("sampleTargets")] IReadOnlyList SampleTargets, + [property: System.Text.Json.Serialization.JsonPropertyName("sampleTruncated")] bool SampleTruncated, + [property: System.Text.Json.Serialization.JsonPropertyName("clientCorrelationId")] string ClientCorrelationId); } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/IConstraintEnforcer.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/IConstraintEnforcer.cs index 95728ae..a777b49 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/IConstraintEnforcer.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/IConstraintEnforcer.cs @@ -3,6 +3,13 @@ using ZB.MOM.WW.MxGateway.Server.Sessions; namespace ZB.MOM.WW.MxGateway.Server.Security.Authorization; +/// Enforces API-key read/write constraints and records constraint denials for audit. +/// +/// The per-item checks return because every production check is +/// synchronous (an in-memory Galaxy hierarchy lookup) and runs once per item of a bulk command: +/// a return allocated a task per item for a value that was already +/// known. An implementation that genuinely needs to await still can. +/// public interface IConstraintEnforcer { /// @@ -34,7 +41,7 @@ public interface IConstraintEnforcer /// Tag address to check. /// Token to observe for cancellation. /// The constraint failure details if denied; otherwise null. - Task CheckReadTagAsync( + ValueTask CheckReadTagAsync( ApiKeyIdentity? identity, string tagAddress, CancellationToken cancellationToken); @@ -46,7 +53,7 @@ public interface IConstraintEnforcer /// The MXAccess item handle. /// Token to observe for cancellation. /// The constraint failure details if denied; otherwise null. - Task CheckReadHandleAsync( + ValueTask CheckReadHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, @@ -60,7 +67,7 @@ public interface IConstraintEnforcer /// The MXAccess item handle. /// Token to observe for cancellation. /// The constraint failure details if denied; otherwise null. - Task CheckWriteHandleAsync( + ValueTask CheckWriteHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, @@ -85,4 +92,25 @@ public interface IConstraintEnforcer ConstraintFailure failure, string? correlationId, CancellationToken cancellationToken); + + /// + /// Records every constraint denial of one bulk command as a single aggregated audit event. + /// + /// The API key identity. + /// The kind of bulk command whose items were denied. + /// The accumulated denials: count, constraint names and a bounded target sample. + /// The per-request client correlation id, if any (same handling as ). + /// Token to observe for cancellation. + /// A task that represents the asynchronous operation. + /// + /// Bulk callers use this instead of one per item: the audit + /// channel is bounded and drops on full, so per-item events let one bulk command evict every + /// other pending audit record. + /// + Task RecordBulkDenialAsync( + ApiKeyIdentity? identity, + string commandKind, + ConstraintDenialSummary summary, + string? correlationId, + CancellationToken cancellationToken); } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceConstraintTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceConstraintTests.cs index 304a9a0..5959384 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceConstraintTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceConstraintTests.cs @@ -87,8 +87,56 @@ public sealed class MxAccessGatewayServiceConstraintTests Assert.True(merged.Results[3].WasSuccessful); Assert.Equal("Tank04.Public", merged.Results[3].TagAddress); Assert.Equal(104, merged.Results[3].ItemHandle); - // Both denied tags recorded. - Assert.Equal(2, enforcer.RecordedDenials.Count); + // Both denied tags recorded in ONE aggregated bulk denial, not one audit call per item. + Assert.Empty(enforcer.RecordedDenials); + (string commandKind, ConstraintDenialSummary summary, _) = Assert.Single(enforcer.RecordedBulkDenials); + Assert.Equal(nameof(MxCommandKind.AddItemBulk), commandKind); + Assert.Equal(2, summary.DeniedCount); + Assert.Equal(4, summary.TotalCount); + Assert.Equal(["Tank01.Locked", "Tank03.Secret"], summary.SampleTargets); + } + + /// + /// Audit-flood guard: a bulk write whose every entry is denied records exactly one + /// aggregated denial carrying the full denied count, the client correlation id and a sample + /// capped at — so one low-privilege + /// key cannot push thousands of events into the bounded, drop-on-full audit channel. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Invoke_WriteBulk_WithManyDeniedEntries_RecordsOneAggregatedDenial() + { + const int EntryCount = 500; + PredicateConstraintEnforcer enforcer = new() { DenyWriteHandle = (_, _) => true }; + FakeSessionManager sessionManager = CreateSessionManagerWithSeed(); + MxAccessGatewayService service = CreateService(sessionManager, enforcer); + WriteBulkCommand payload = new() { ServerHandle = 7 }; + for (int index = 0; index < EntryCount; index++) + { + payload.Entries.Add(new WriteBulkEntry { ItemHandle = index + 1 }); + } + + MxCommandRequest request = new() + { + SessionId = SessionId, + ClientCorrelationId = "bulk-flood-1", + Command = new MxCommand { Kind = MxCommandKind.WriteBulk, WriteBulk = payload }, + }; + + MxCommandReply reply = await service.Invoke(request, new TestServerCallContext()); + + Assert.Equal(0, sessionManager.InvokeCount); + Assert.Equal(EntryCount, reply.WriteBulk.Results.Count); + Assert.Empty(enforcer.RecordedDenials); + (string commandKind, ConstraintDenialSummary summary, string? correlationId) = + Assert.Single(enforcer.RecordedBulkDenials); + Assert.Equal(nameof(MxCommandKind.WriteBulk), commandKind); + Assert.Equal("bulk-flood-1", correlationId); + Assert.Equal(EntryCount, summary.DeniedCount); + Assert.Equal(EntryCount, summary.TotalCount); + Assert.Equal(ConstraintDenialSummary.MaxSampleTargets, summary.SampleTargets.Count); + Assert.True(summary.SampleTruncated); + Assert.Equal(["write-handle"], summary.ConstraintNames); } /// diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs index 707602e..9bbeab1 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ConstraintEnforcerTests.cs @@ -154,6 +154,70 @@ public sealed class ConstraintEnforcerTests Assert.Equal("Read:Secret.Tag", auditEvent.Target); } + /// + /// A bulk denial is written as ONE audit event carrying the denied/total counts, the distinct + /// constraint names and a sample capped at , + /// however many items were denied. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task RecordBulkDenialAsync_WritesOneAggregatedEventWithBoundedSample() + { + ConstraintEnforcer enforcer = CreateEnforcer(out FakeAuditWriter auditWriter); + ConstraintDenialSummary summary = new(totalCount: 1000); + for (int index = 0; index < 999; index++) + { + summary.Add( + $"Area1.Tag{index}", + index % 2 == 0 + ? new ConstraintFailure("read_scope", "Tag is outside the API key read scope.") + : new ConstraintFailure("tag_metadata", "Tag metadata is not available.")); + } + + await enforcer.RecordBulkDenialAsync( + CreateIdentity(ApiKeyConstraints.Empty), + "ReadBulk", + summary, + "trace-1", + CancellationToken.None); + + AuditEvent auditEvent = Assert.Single(auditWriter.Events); + Assert.Equal("operator01", auditEvent.Actor); + Assert.Equal("constraint-denied", auditEvent.Action); + Assert.Equal(AuditOutcome.Denied, auditEvent.Outcome); + Assert.Equal("ReadBulk:bulk", auditEvent.Target); + Assert.Null(auditEvent.CorrelationId); + + using JsonDocument details = JsonDocument.Parse(auditEvent.DetailsJson!); + JsonElement root = details.RootElement; + Assert.Equal(999, root.GetProperty("deniedCount").GetInt32()); + Assert.Equal(1000, root.GetProperty("totalCount").GetInt32()); + Assert.True(root.GetProperty("sampleTruncated").GetBoolean()); + Assert.Equal(ConstraintDenialSummary.MaxSampleTargets, root.GetProperty("sampleTargets").GetArrayLength()); + Assert.Equal( + ["read_scope", "tag_metadata"], + root.GetProperty("constraints").EnumerateArray().Select(e => e.GetString() ?? string.Empty).ToArray()); + Assert.Equal("ReadBulk", root.GetProperty("commandKind").GetString()); + Assert.Equal("trace-1", root.GetProperty("clientCorrelationId").GetString()); + } + + /// An empty summary (nothing denied) writes no audit event at all. + /// A task that represents the asynchronous operation. + [Fact] + public async Task RecordBulkDenialAsync_WithNoDenials_WritesNothing() + { + ConstraintEnforcer enforcer = CreateEnforcer(out FakeAuditWriter auditWriter); + + await enforcer.RecordBulkDenialAsync( + identity: null, + "ReadBulk", + new ConstraintDenialSummary(totalCount: 3), + correlationId: null, + CancellationToken.None); + + Assert.Empty(auditWriter.Events); + } + /// Verifies that historized-only constraint requires historized attribute. /// A task that represents the asynchronous operation. [Fact] diff --git a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/AllowAllConstraintEnforcer.cs b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/AllowAllConstraintEnforcer.cs index 316b061..9e77446 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/AllowAllConstraintEnforcer.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/AllowAllConstraintEnforcer.cs @@ -11,26 +11,26 @@ namespace ZB.MOM.WW.MxGateway.Tests.TestSupport; public sealed class AllowAllConstraintEnforcer : IConstraintEnforcer { /// - public Task CheckReadTagAsync( + public ValueTask CheckReadTagAsync( ApiKeyIdentity? identity, string tagAddress, - CancellationToken cancellationToken) => Task.FromResult(null); + CancellationToken cancellationToken) => ValueTask.FromResult(null); /// - public Task CheckReadHandleAsync( + public ValueTask CheckReadHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, int itemHandle, - CancellationToken cancellationToken) => Task.FromResult(null); + CancellationToken cancellationToken) => ValueTask.FromResult(null); /// - public Task CheckWriteHandleAsync( + public ValueTask CheckWriteHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, int itemHandle, - CancellationToken cancellationToken) => Task.FromResult(null); + CancellationToken cancellationToken) => ValueTask.FromResult(null); /// public Task RecordDenialAsync( @@ -40,4 +40,12 @@ public sealed class AllowAllConstraintEnforcer : IConstraintEnforcer ConstraintFailure failure, string? correlationId, CancellationToken cancellationToken) => Task.CompletedTask; + + /// + public Task RecordBulkDenialAsync( + ApiKeyIdentity? identity, + string commandKind, + ConstraintDenialSummary summary, + string? correlationId, + CancellationToken cancellationToken) => Task.CompletedTask; } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/PredicateConstraintEnforcer.cs b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/PredicateConstraintEnforcer.cs index fe129fa..408fe48 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/PredicateConstraintEnforcer.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/PredicateConstraintEnforcer.cs @@ -26,23 +26,26 @@ public sealed class PredicateConstraintEnforcer : IConstraintEnforcer /// Recorded denial messages — (commandKind, target, correlationId) tuples. public List<(string CommandKind, string Target, string? CorrelationId)> RecordedDenials { get; } = []; + /// Recorded aggregated bulk denials — one entry per bulk command that had any item denied. + public List<(string CommandKind, ConstraintDenialSummary Summary, string? CorrelationId)> RecordedBulkDenials { get; } = []; + /// - public Task CheckReadTagAsync( + public ValueTask CheckReadTagAsync( ApiKeyIdentity? identity, string tagAddress, CancellationToken cancellationToken) { if (DenyTag(tagAddress)) { - return Task.FromResult( + return ValueTask.FromResult( new ConstraintFailure("read-tag", $"Read denied for tag '{tagAddress}'.")); } - return Task.FromResult(null); + return ValueTask.FromResult(null); } /// - public Task CheckReadHandleAsync( + public ValueTask CheckReadHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, @@ -51,15 +54,15 @@ public sealed class PredicateConstraintEnforcer : IConstraintEnforcer { if (DenyReadHandle(serverHandle, itemHandle)) { - return Task.FromResult( + return ValueTask.FromResult( new ConstraintFailure("read-handle", $"Read denied for handle {itemHandle}.")); } - return Task.FromResult(null); + return ValueTask.FromResult(null); } /// - public Task CheckWriteHandleAsync( + public ValueTask CheckWriteHandleAsync( ApiKeyIdentity? identity, GatewaySession session, int serverHandle, @@ -68,11 +71,11 @@ public sealed class PredicateConstraintEnforcer : IConstraintEnforcer { if (DenyWriteHandle(serverHandle, itemHandle)) { - return Task.FromResult( + return ValueTask.FromResult( new ConstraintFailure("write-handle", $"Write denied for handle {itemHandle}.")); } - return Task.FromResult(null); + return ValueTask.FromResult(null); } /// @@ -87,4 +90,16 @@ public sealed class PredicateConstraintEnforcer : IConstraintEnforcer RecordedDenials.Add((commandKind, target, correlationId)); return Task.CompletedTask; } + + /// + public Task RecordBulkDenialAsync( + ApiKeyIdentity? identity, + string commandKind, + ConstraintDenialSummary summary, + string? correlationId, + CancellationToken cancellationToken) + { + RecordedBulkDenials.Add((commandKind, summary, correlationId)); + return Task.CompletedTask; + } } From 19cc74f21a4e974fc5e676901e21d2705b71d3ab Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:14:33 -0400 Subject: [PATCH 2/8] fix(auth): authenticate client- and duplex-streaming gRPC calls GatewayGrpcAuthorizationInterceptor only overrode the unary and server-streaming handlers; the base Interceptor passes client- and duplex-streaming calls through unauthenticated. Both now run the same API-key pipeline and fail closed to the admin scope, since there is no single request message to resolve a scope from. --- docs/Authorization.md | 9 +- .../GatewayGrpcAuthorizationInterceptor.cs | 57 ++++++- ...atewayGrpcAuthorizationInterceptorTests.cs | 156 ++++++++++++++++++ 3 files changed, 211 insertions(+), 11 deletions(-) diff --git a/docs/Authorization.md b/docs/Authorization.md index ed5ec8b..23c71d1 100644 --- a/docs/Authorization.md +++ b/docs/Authorization.md @@ -10,7 +10,7 @@ Authorization runs as a single gRPC server interceptor registered for every call The participating types live under `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/`: -- `GatewayGrpcAuthorizationInterceptor` runs the authenticate-then-authorize pipeline for unary and server-streaming calls. +- `GatewayGrpcAuthorizationInterceptor` runs the authenticate-then-authorize pipeline for every call shape: unary, server-streaming, client-streaming, and duplex-streaming. - `GatewayGrpcScopeResolver` maps a request message (and, for `MxCommandRequest`, the inner `MxCommandKind`) to the scope string that must be present on the caller. - `GatewayScopes` exposes the canonical scope constants used by the resolver and any downstream consumer. - `GatewayRequestIdentityAccessor` and `IGatewayRequestIdentityAccessor` expose the verified identity to handlers and any service code that runs inside the call. @@ -32,7 +32,7 @@ Centralizing the policy in `GatewayGrpcAuthorizationInterceptor` produces three ## Interceptor Flow -`GatewayGrpcAuthorizationInterceptor` overrides both `UnaryServerHandler` and `ServerStreamingServerHandler`. Both call the same private `AuthenticateAndAuthorizeAsync` helper before invoking the continuation, then push the resolved identity onto the accessor for the duration of the call. +`GatewayGrpcAuthorizationInterceptor` overrides all four server handlers: `UnaryServerHandler`, `ServerStreamingServerHandler`, `ClientStreamingServerHandler`, and `DuplexStreamingServerHandler`. Each calls the same private `AuthenticateAndAuthorizeAsync` helper before invoking the continuation, then pushes the resolved identity onto the accessor for the duration of the call. The base `Interceptor` passes any handler it does not override straight through, so a client- or duplex-streaming RPC added to a mapped service would otherwise run with no API key check at all. The unary and server-streaming handlers resolve the required scope from the request message; the client- and duplex-streaming handlers have no single request to inspect before the handler reads the stream, so they fail closed to `admin` — the same scope the resolver assigns an unrecognized request type. No public RPC uses either shape today. ```csharp public override async Task UnaryServerHandler( @@ -40,7 +40,10 @@ public override async Task UnaryServerHandler( ServerCallContext context, UnaryServerMethod continuation) { - ApiKeyIdentity? identity = await AuthenticateAndAuthorizeAsync(request, context).ConfigureAwait(false); + ApiKeyIdentity? identity = await AuthenticateAndAuthorizeAsync( + scopeResolver.ResolveRequiredScope(request), + context) + .ConfigureAwait(false); IDisposable? identityScope = identity is null ? null : identityAccessor.Push(identity); using (identityScope) { diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs index 345ed4d..730c2e2 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs @@ -30,7 +30,10 @@ public sealed class GatewayGrpcAuthorizationInterceptor( ServerCallContext context, UnaryServerMethod continuation) { - ApiKeyIdentity? identity = await AuthenticateAndAuthorizeAsync(request, context).ConfigureAwait(false); + ApiKeyIdentity? identity = await AuthenticateAndAuthorizeAsync( + scopeResolver.ResolveRequiredScope(request), + context) + .ConfigureAwait(false); IDisposable? identityScope = identity is null ? null : identityAccessor.Push(identity); using (identityScope) { @@ -45,7 +48,10 @@ public sealed class GatewayGrpcAuthorizationInterceptor( ServerCallContext context, ServerStreamingServerMethod continuation) { - ApiKeyIdentity? identity = await AuthenticateAndAuthorizeAsync(request, context).ConfigureAwait(false); + ApiKeyIdentity? identity = await AuthenticateAndAuthorizeAsync( + scopeResolver.ResolveRequiredScope(request), + context) + .ConfigureAwait(false); IDisposable? identityScope = identity is null ? null : identityAccessor.Push(identity); using (identityScope) { @@ -53,15 +59,51 @@ public sealed class GatewayGrpcAuthorizationInterceptor( } } + // The base Interceptor passes client- and duplex-streaming calls straight to the handler, so + // without these overrides any such RPC (none exist today, but a .proto edit could add one, and + // the shared Galaxy Repository service is mapped through the same interceptor) would be + // reachable with no API key at all. Both run the identical authentication path. There is no + // single request message to derive a scope from before the handler reads the stream, so they + // fail closed to the scope the resolver assigns an unrecognized request: admin. + + /// + public override async Task ClientStreamingServerHandler( + IAsyncStreamReader requestStream, + ServerCallContext context, + ClientStreamingServerMethod continuation) + { + ApiKeyIdentity? identity = await AuthenticateAndAuthorizeAsync(GatewayScopes.Admin, context) + .ConfigureAwait(false); + IDisposable? identityScope = identity is null ? null : identityAccessor.Push(identity); + using (identityScope) + { + return await continuation(requestStream, context).ConfigureAwait(false); + } + } + + /// + public override async Task DuplexStreamingServerHandler( + IAsyncStreamReader requestStream, + IServerStreamWriter responseStream, + ServerCallContext context, + DuplexStreamingServerMethod continuation) + { + ApiKeyIdentity? identity = await AuthenticateAndAuthorizeAsync(GatewayScopes.Admin, context) + .ConfigureAwait(false); + IDisposable? identityScope = identity is null ? null : identityAccessor.Push(identity); + using (identityScope) + { + await continuation(requestStream, responseStream, context).ConfigureAwait(false); + } + } + /// Authenticates the API key and authorizes the RPC call by required scope. - /// Request message type. - /// Request payload. + /// The scope the call requires, resolved by the caller. /// RPC server call context. /// Authenticated API key identity, or null if authentication is disabled. - private async Task AuthenticateAndAuthorizeAsync( - TRequest request, + private async Task AuthenticateAndAuthorizeAsync( + string requiredScope, ServerCallContext context) - where TRequest : class { if (options.Value.Authentication.Mode == AuthenticationMode.Disabled) { @@ -116,7 +158,6 @@ public sealed class GatewayGrpcAuthorizationInterceptor( ApiKeyIdentity identity = GatewayApiKeyIdentityMapper.ToGatewayIdentity(verification.Identity); - string requiredScope = scopeResolver.ResolveRequiredScope(request); if (!identity.Scopes.Contains(requiredScope)) { throw new RpcException(new Status( diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs index c38b7db..76fc2ab 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs @@ -717,6 +717,151 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests Assert.Null(GatewayGrpcAuthorizationInterceptor.TryResolveKeyId($"Bearer mxgw_{overCap}_secret")); } + /// + /// A client-streaming call is authenticated exactly like a unary call: a missing key is + /// rejected with Unauthenticated and the handler never runs. The base Interceptor passes + /// client- and duplex-streaming calls straight through, so without the override any such RPC + /// added to the service would be reachable unauthenticated. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task ClientStreamingServerHandler_MissingApiKey_ReturnsUnauthenticatedWithoutRunningHandler() + { + GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor( + new FakeApiKeyVerifier(Failure(ApiKeyFailure.MissingOrMalformed)), + new GatewayRequestIdentityAccessor()); + bool handlerRan = false; + + RpcException exception = await Assert.ThrowsAsync( + () => interceptor.ClientStreamingServerHandler( + new EmptyStreamReader(), + new TestServerCallContext([]), + (_, _) => + { + handlerRan = true; + return Task.FromResult(new MxCommandReply()); + })); + + Assert.Equal(StatusCode.Unauthenticated, exception.StatusCode); + Assert.False(handlerRan); + } + + /// + /// A client-streaming call has no single request message to derive a scope from, so the + /// interceptor fails closed to the admin scope, the resolver's default for an + /// unrecognized request. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task ClientStreamingServerHandler_KeyWithoutAdminScope_ReturnsPermissionDenied() + { + GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor( + new FakeApiKeyVerifier(SuccessWithScopes(GatewayScopes.InvokeRead, GatewayScopes.InvokeWrite)), + new GatewayRequestIdentityAccessor()); + + RpcException exception = await Assert.ThrowsAsync( + () => interceptor.ClientStreamingServerHandler( + new EmptyStreamReader(), + ContextWithAuthorization("Bearer mxgw_operator01_secret"), + (_, _) => Task.FromResult(new MxCommandReply()))); + + Assert.Equal(StatusCode.PermissionDenied, exception.StatusCode); + Assert.Contains(GatewayScopes.Admin, exception.Status.Detail, StringComparison.Ordinal); + } + + /// An admin-scoped key reaches the client-streaming handler with its identity pushed. + /// A task that represents the asynchronous operation. + [Fact] + public async Task ClientStreamingServerHandler_AdminScope_RunsHandlerWithIdentity() + { + GatewayRequestIdentityAccessor identityAccessor = new(); + GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor( + new FakeApiKeyVerifier(SuccessWithScopes(GatewayScopes.Admin)), + identityAccessor); + string? keyIdSeenByHandler = null; + + await interceptor.ClientStreamingServerHandler( + new EmptyStreamReader(), + ContextWithAuthorization("Bearer mxgw_operator01_secret"), + (_, _) => + { + keyIdSeenByHandler = identityAccessor.Current?.KeyId; + return Task.FromResult(new MxCommandReply()); + }); + + Assert.Equal("operator01", keyIdSeenByHandler); + Assert.Null(identityAccessor.Current); + } + + /// A duplex-streaming call with no key is rejected before the handler runs. + /// A task that represents the asynchronous operation. + [Fact] + public async Task DuplexStreamingServerHandler_MissingApiKey_ReturnsUnauthenticatedWithoutRunningHandler() + { + GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor( + new FakeApiKeyVerifier(Failure(ApiKeyFailure.MissingOrMalformed)), + new GatewayRequestIdentityAccessor()); + bool handlerRan = false; + + RpcException exception = await Assert.ThrowsAsync( + () => interceptor.DuplexStreamingServerHandler( + new EmptyStreamReader(), + new RecordingServerStreamWriter(), + new TestServerCallContext([]), + (_, _, _) => + { + handlerRan = true; + return Task.CompletedTask; + })); + + Assert.Equal(StatusCode.Unauthenticated, exception.StatusCode); + Assert.False(handlerRan); + } + + /// A duplex-streaming call fails closed to the admin scope, like client streaming. + /// A task that represents the asynchronous operation. + [Fact] + public async Task DuplexStreamingServerHandler_KeyWithoutAdminScope_ReturnsPermissionDenied() + { + GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor( + new FakeApiKeyVerifier(SuccessWithScopes(GatewayScopes.EventsRead)), + new GatewayRequestIdentityAccessor()); + + RpcException exception = await Assert.ThrowsAsync( + () => interceptor.DuplexStreamingServerHandler( + new EmptyStreamReader(), + new RecordingServerStreamWriter(), + ContextWithAuthorization("Bearer mxgw_operator01_secret"), + (_, _, _) => Task.CompletedTask)); + + Assert.Equal(StatusCode.PermissionDenied, exception.StatusCode); + } + + /// An admin-scoped key reaches the duplex handler with its identity pushed. + /// A task that represents the asynchronous operation. + [Fact] + public async Task DuplexStreamingServerHandler_AdminScope_RunsHandlerWithIdentity() + { + GatewayRequestIdentityAccessor identityAccessor = new(); + GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor( + new FakeApiKeyVerifier(SuccessWithScopes(GatewayScopes.Admin)), + identityAccessor); + RecordingServerStreamWriter writer = new(); + + await interceptor.DuplexStreamingServerHandler( + new EmptyStreamReader(), + writer, + ContextWithAuthorization("Bearer mxgw_operator01_secret"), + async (_, responseStream, _) => + { + Assert.Equal("operator01", identityAccessor.Current?.KeyId); + await responseStream.WriteAsync(new MxCommandReply { SessionId = "duplex" }); + }); + + Assert.Equal("duplex", Assert.Single(writer.Messages).SessionId); + Assert.Null(identityAccessor.Current); + } + private static MxAccessGatewayService CreateService( ISessionManager sessionManager, IGatewayRequestIdentityAccessor identityAccessor) @@ -933,4 +1078,15 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests } } + + /// An that yields no messages. + /// The streamed message type. + private sealed class EmptyStreamReader : IAsyncStreamReader + { + /// + public T Current => throw new InvalidOperationException("The stream is empty."); + + /// + public Task MoveNext(CancellationToken cancellationToken) => Task.FromResult(false); + } } From 0b4a1682c839c7ccb8c1bec17f2e9fa757fae2e4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:27:36 -0400 Subject: [PATCH 3/8] fix(grpc): typed alarm-feed overflow, memoized alarm snapshot, cancellable stream writes, uniform reply correlation id - StreamAlarms: slow subscribers are completed with AlarmFeedSubscriberOverflowException and surface as ResourceExhausted (like a StreamEvents overflow) instead of warning-with-stack + Unavailable. - GatewayAlarmMonitor.StreamAsync reuses the memoized alarm-set projection instead of cloning the whole set under the broadcast lock per attach; the prefix filter runs outside the lock. - StreamEvents/StreamAlarms pass the call token to response writes; a write failure after client cancel maps to Cancelled without a warning. - Every MxCommandReply.correlation_id now echoes the request's client_correlation_id (previously only constraint-denied replies did; normal replies leaked the internal envelope id). --- docs/Grpc.md | 8 + gateway.md | 4 +- .../WorkerLiveMxAccessSmokeTests.cs | 14 ++ .../AlarmFeedSubscriberOverflowException.cs | 39 +++++ .../Alarms/GatewayAlarmMonitor.cs | 28 ++- .../Grpc/MxAccessGatewayService.cs | 42 ++++- .../Alarms/AlarmTruncationSignalTests.cs | 116 +++++++++++++ .../Grpc/MxAccessGatewayServiceTests.cs | 164 +++++++++++++++++- .../TestSupport/FakeGatewayAlarmService.cs | 10 ++ .../TestSupport/RecordingLogger.cs | 48 +++++ .../RecordingServerStreamWriter.cs | 19 ++ 11 files changed, 478 insertions(+), 14 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Alarms/AlarmFeedSubscriberOverflowException.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Tests/TestSupport/RecordingLogger.cs diff --git a/docs/Grpc.md b/docs/Grpc.md index 28d1cc4..5c0bf4b 100644 --- a/docs/Grpc.md +++ b/docs/Grpc.md @@ -82,6 +82,8 @@ return mapper.MapCommandReply(workerReply); Carrying the enqueue timestamp into the worker layer is what lets queue-wait time be measured separately from worker-side execution time when troubleshooting timeouts. +Every `MxCommandReply` echoes the request's `client_correlation_id` in `correlation_id` — success, MXAccess failure, and constraint-denied replies alike, and an empty string when the caller sent none. The handler stamps it after mapping the worker reply, overwriting the internal gateway↔worker envelope id the worker reply carries: that id is a pipe-routing detail with no meaning to a client, and clients that sent no correlation id previously received it while clients that sent one received it only on denied replies. + An accepted gRPC command payload can still be too large for the worker pipe: the envelope built around it must fit `MxGateway:Worker:MaxMessageBytes`, which is validated at startup to sit at least a fixed envelope-overhead reserve above `MaxGrpcMessageBytes` (see the headroom rule in [Gateway Configuration](./GatewayConfiguration.md)) so this should not occur for a conformant payload, but if it does, `WorkerClient` raises `WorkerClientException(CommandTooLarge)` and `Invoke` reports `ResourceExhausted` for that command — the session is not faulted, so a client can retry with a smaller payload without reopening the session. Selected control commands have additional public behavior beyond their protobuf shape: @@ -94,6 +96,8 @@ Selected control commands have additional public behavior beyond their protobuf `StreamEvents` is a server-streaming RPC. The handler delegates the full pipeline to `IEventStreamService` and just forwards each `MxEvent` onto the response stream. Keeping the channel and producer/consumer machinery out of the handler means cancellation, exception mapping, and metric bookkeeping live in one place. +Each response write receives the call's cancellation token, so a write parked on a slow or vanished client is released when the call is cancelled rather than waiting on the transport. A write that then fails with an `IOException` or `OperationCanceledException` while the call token is cancelled is a routine client disconnect: it maps to `Cancelled` and logs nothing. The same `IOException` on a live call keeps the generic `Unavailable` + warning mapping, because that is a real transport failure. `StreamAlarms` applies the same rule. + ### `AcknowledgeAlarm` `AcknowledgeAlarm` is a unary, **session-less** RPC that acknowledges a single alarm. The handler validates `alarm_full_reference` and `operator_user` inline (it does not run through `MxAccessGrpcRequestValidator`) and delegates to `IGatewayAlarmService.AcknowledgeAsync`. The always-on `GatewayAlarmMonitor` routes the ack over its own gateway-managed worker session — clients no longer open a session to acknowledge an alarm. A reference that parses as a canonical GUID forwards to `AcknowledgeAlarmCommand`; a `Provider!Group.Tag` reference forwards to `AcknowledgeAlarmByNameCommand`. @@ -125,6 +129,8 @@ The worker calls `AuthenticateUser` as the operator, then writes `operator_comme `StreamAlarms` is a server-streaming, **session-less** RPC that attaches to the gateway's central alarm feed. The handler delegates to `IGatewayAlarmService.StreamAsync`. The stream opens with a `provider_status` and a `snapshot_status` `AlarmFeedMessage` (the current provider mode and snapshot-completeness verdict), then one `AlarmFeedMessage` carrying an `active_alarm` per currently-active alarm (the ConditionRefresh snapshot), then a single `snapshot_complete`, then a `transition` for every subsequent raise / acknowledge / clear — interleaved with a further `provider_status` on each failover/failback and a further `snapshot_status` on each change of the truncation verdict. It is served by the always-on `GatewayAlarmMonitor`, which owns a single gateway-managed worker session and fans out to every attached client — clients no longer open a session of their own. `alarm_filter_prefix`, when set, scopes the stream to a sub-tree. +A subscriber's opening snapshot is taken from the monitor's memoized alarm-set projection (the same one `QueryActiveAlarms` and the dashboard read), captured under the broadcast lock in the same critical section that registers the subscriber, so no transition can fall between the snapshot and the live stream; the prefix filter runs afterwards, outside the lock. Each subscriber has a bounded queue (2048 messages). A subscriber that falls that far behind is dropped and its stream ends with `ResourceExhausted` ("Alarm feed subscriber fell behind and was dropped; reconnect to re-snapshot."), the same status a `StreamEvents` overflow carries; the remedy is to reconnect, which re-sends the snapshot. The drop is not logged as a gateway failure. + ### `QueryActiveAlarms` `QueryActiveAlarms` is a server-streaming, **session-less** RPC that returns a point-in-time snapshot of the currently-active alarm set. Unlike `StreamAlarms`, it does not stay attached for live transitions — it writes the snapshot and completes, which clients use after a reconnect to reseed alarm state or to reconcile alarms that may have been missed during a transport blip. It is served from the always-on `GatewayAlarmMonitor` cache (`IGatewayAlarmService.CurrentAlarms`), so no worker session is opened. The handler does not run through `MxAccessGrpcRequestValidator`; it only null-checks the request inline, then streams one `ActiveAlarmSnapshot` per cached alarm, honouring cancellation between writes. When `alarm_filter_prefix` is non-empty, only alarms whose `alarm_full_reference` starts with that prefix (ordinal `StartsWith`) are emitted; an empty prefix returns the full set. Exceptions are translated by the same `MapException` path as the other handlers. The RPC requires the `events:read` scope (`GatewayScopes.EventsRead`), the same scope as `StreamEvents` and `StreamAlarms`. @@ -294,6 +300,8 @@ Every handler wraps its body in `try { ... } catch (Exception exception) when (e - `SessionManagerException` is mapped by its `ErrorCode`. - `WorkerClientException` is mapped by its `ErrorCode`. +Two stream-specific cases are handled before `MapException`: on `StreamEvents` and `StreamAlarms` a write failure after the client cancelled becomes `Cancelled` without a log entry, and on `StreamAlarms` an `AlarmFeedSubscriberOverflowException` (slow-subscriber drop) becomes `ResourceExhausted` without a log entry. + Anything else is logged at warning and surfaced as `Unavailable` with a generic message — clients see a retryable status, and the unexpected exception is captured in gateway logs rather than leaked over the wire. ```csharp diff --git a/gateway.md b/gateway.md index c5ae6b4..9a6a8ec 100644 --- a/gateway.md +++ b/gateway.md @@ -446,7 +446,9 @@ Rules: enforcement is ever added, this is the property it will rely on. - `correlation_id` links a command to its reply; it is authoritative on the envelope, and the inner `MxCommandReply.correlation_id` echoes it for - MXAccess parity. + MXAccess parity. The envelope id never reaches public clients: the gateway + overwrites the public `MxCommandReply.correlation_id` with the request's + `client_correlation_id` (empty when the client sent none). - Events carry their own correlation id or leave it empty. - Replies must preserve MXAccess HRESULT/status information even when the command is also represented as a protocol-level failure. diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs index ed0acf1..d411a84 100644 --- a/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/WorkerLiveMxAccessSmokeTests.cs @@ -1704,6 +1704,20 @@ public sealed class WorkerLiveMxAccessSmokeTests(ITestOutputHelper output) /// public WriteOptions? WriteOptions { get; set; } + /// + /// Records the message unless the token is already cancelled. + /// + /// The message to write. + /// Token that cancels the write. + /// A task that represents the asynchronous operation. + public Task WriteAsync(T message, CancellationToken cancellationToken) + { + // The interface default throws NotSupportedException for a cancelable token; the + // gateway passes the call's token on every streamed write. + cancellationToken.ThrowIfCancellationRequested(); + return WriteAsync(message); + } + /// /// Records the message and signals any pending waiter. /// diff --git a/src/ZB.MOM.WW.MxGateway.Server/Alarms/AlarmFeedSubscriberOverflowException.cs b/src/ZB.MOM.WW.MxGateway.Server/Alarms/AlarmFeedSubscriberOverflowException.cs new file mode 100644 index 0000000..07c0952 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Alarms/AlarmFeedSubscriberOverflowException.cs @@ -0,0 +1,39 @@ +namespace ZB.MOM.WW.MxGateway.Server.Alarms; + +/// +/// Completes an alarm-feed subscriber that fell behind its bounded queue and was dropped. +/// +/// +/// A typed exception rather than a bare so the gRPC +/// layer can map it to ResourceExhausted — the status a StreamEvents overflow +/// carries — without treating a slow client as a gateway failure (an unrecognized exception is +/// logged as a warning with its stack and surfaced as Unavailable). The remedy is the +/// same as for an event-stream overflow: reconnect, which re-snapshots the alarm set. +/// +public sealed class AlarmFeedSubscriberOverflowException : Exception +{ + /// The message every dropped subscriber is completed with. + public const string DefaultMessage = + "Alarm feed subscriber fell behind and was dropped; reconnect to re-snapshot."; + + /// Initializes a new instance of the class. + public AlarmFeedSubscriberOverflowException() + : base(DefaultMessage) + { + } + + /// Initializes a new instance of the class. + /// The error message. + public AlarmFeedSubscriberOverflowException(string message) + : base(message) + { + } + + /// Initializes a new instance of the class. + /// The error message. + /// The exception that caused this one. + public AlarmFeedSubscriberOverflowException(string message, Exception innerException) + : base(message, innerException) + { + } +} diff --git a/src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs b/src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs index 76fb408..4a6be85 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Alarms/GatewayAlarmMonitor.cs @@ -967,7 +967,9 @@ public sealed class GatewayAlarmMonitor : BackgroundService, IGatewayAlarmServic } // Caller holds _sync. Pushes a feed message to every matching subscriber; - // a subscriber that has fallen behind is completed with an error and dropped. + // a subscriber that has fallen behind is completed with an AlarmFeedSubscriberOverflowException + // and dropped. The typed fault is what lets StreamAlarms answer ResourceExhausted instead of + // treating a slow client as a gateway failure. private void Broadcast(AlarmFeedMessage message, string reference) { for (int index = _subscribers.Count - 1; index >= 0; index--) @@ -980,8 +982,7 @@ public sealed class GatewayAlarmMonitor : BackgroundService, IGatewayAlarmServic if (!subscriber.Channel.Writer.TryWrite(message)) { - subscriber.Channel.Writer.TryComplete(new InvalidOperationException( - "Alarm feed subscriber fell behind and was dropped; reconnect to re-snapshot.")); + subscriber.Channel.Writer.TryComplete(new AlarmFeedSubscriberOverflowException()); _subscribers.RemoveAt(index); } } @@ -997,8 +998,7 @@ public sealed class GatewayAlarmMonitor : BackgroundService, IGatewayAlarmServic Subscriber subscriber = _subscribers[index]; if (!subscriber.Channel.Writer.TryWrite(message)) { - subscriber.Channel.Writer.TryComplete(new InvalidOperationException( - "Alarm feed subscriber fell behind and was dropped; reconnect to re-snapshot.")); + subscriber.Channel.Writer.TryComplete(new AlarmFeedSubscriberOverflowException()); _subscribers.RemoveAt(index); } } @@ -1046,9 +1046,14 @@ public sealed class GatewayAlarmMonitor : BackgroundService, IGatewayAlarmServic _subscribers.Add(subscriber); providerStatus = BuildProviderStatus(); snapshotTruncated = _snapshotTruncated; - snapshot = _alarms.Values - .Where(alarm => prefix.Length == 0 - || alarm.AlarmFullReference.StartsWith(prefix, StringComparison.Ordinal)) + + // Take the memoized projection rather than cloning the whole set per attach: the + // clone runs at most once per alarm-set generation (shared with CurrentAlarms), so a + // burst of reconnecting subscribers no longer holds the broadcast lock for one full + // copy each. The array is never mutated once built — a write to _alarms replaces it + // with null — so capturing the reference here pins exactly the generation this + // subscriber registered against, which is what keeps snapshot-then-live ordering. + snapshot = _currentAlarmsProjection ??= _alarms.Values .Select(alarm => alarm.Clone()) .ToArray(); } @@ -1068,8 +1073,15 @@ public sealed class GatewayAlarmMonitor : BackgroundService, IGatewayAlarmServic SnapshotStatus = new AlarmSnapshotStatus { Truncated = snapshotTruncated }, }; + // The prefix filter runs here, outside _sync, over the immutable projection. foreach (ActiveAlarmSnapshot alarm in snapshot) { + if (prefix.Length != 0 + && !alarm.AlarmFullReference.StartsWith(prefix, StringComparison.Ordinal)) + { + continue; + } + yield return new AlarmFeedMessage { ActiveAlarm = alarm }; } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs index 96688a8..5ed29ea 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs @@ -144,6 +144,12 @@ public sealed class MxAccessGatewayService( publicReply = bulkConstraintPlan.MergeDeniedInto(publicReply); } + // MxCommandReply.correlation_id always echoes the caller's client_correlation_id — + // the same value the constraint-denied replies carry — so a client can match every + // reply to its request. The worker reply carries the gateway's internal envelope id, + // which is a gateway↔worker routing detail and is overwritten here, empty included. + publicReply.CorrelationId = request.ClientCorrelationId; + session.TrackCommandReply(commandToInvoke, publicReply); return publicReply; } @@ -172,7 +178,10 @@ public sealed class MxAccessGatewayService( // volume gateway path. Stopwatch.GetTimestamp/GetElapsedTime measure the exact // same wall-clock span the former Stopwatch.StartNew()/.Elapsed did. long sendStartTimestamp = Stopwatch.GetTimestamp(); - await responseStream.WriteAsync(publicEvent).ConfigureAwait(false); + + // The call token releases a write parked on a slow or vanished client when the + // call is cancelled, instead of leaving it to the transport's own timeout. + await responseStream.WriteAsync(publicEvent, context.CancellationToken).ConfigureAwait(false); metrics.RecordEventStreamSend( publicEvent.Family.ToString(), Stopwatch.GetElapsedTime(sendStartTimestamp)); @@ -180,6 +189,11 @@ public sealed class MxAccessGatewayService( } catch (Exception exception) when (exception is not RpcException) { + if (IsClientAbort(exception, context)) + { + throw ClientCancelled(); + } + throw MapException(exception); } } @@ -225,11 +239,24 @@ public sealed class MxAccessGatewayService( .WithCancellation(context.CancellationToken) .ConfigureAwait(false)) { - await responseStream.WriteAsync(message).ConfigureAwait(false); + await responseStream.WriteAsync(message, context.CancellationToken).ConfigureAwait(false); } } + catch (AlarmFeedSubscriberOverflowException overflow) + { + // The monitor dropped this subscriber for falling behind its bounded queue. That is a + // client-side slowness condition with a client-side remedy (reconnect and + // re-snapshot), so it maps to ResourceExhausted like a StreamEvents overflow rather + // than reaching MapException's warning-with-stack + Unavailable fallback. + throw new RpcException(new Status(StatusCode.ResourceExhausted, overflow.Message)); + } catch (Exception exception) when (exception is not RpcException) { + if (IsClientAbort(exception, context)) + { + throw ClientCancelled(); + } + throw MapException(exception); } } @@ -264,6 +291,17 @@ public sealed class MxAccessGatewayService( } } + // A response write fails with an IOException (or an operation-cancelled exception) when the + // client disconnects mid-stream. With the call token already cancelled that is a routine + // client abort, not a gateway fault: it maps to Cancelled with no warning. The same exception + // on a live call token is a genuine transport failure and keeps the MapException path. + private static bool IsClientAbort(Exception exception, ServerCallContext context) => + context.CancellationToken.IsCancellationRequested + && exception is IOException or OperationCanceledException; + + private static RpcException ClientCancelled() => + new(new Status(StatusCode.Cancelled, "gRPC request was canceled.")); + private string? ResolveClientIdentity() { return identityAccessor.Current?.DisplayName ?? identityAccessor.Current?.KeyId; diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmTruncationSignalTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmTruncationSignalTests.cs index 6aa2177..2852da7 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmTruncationSignalTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Alarms/AlarmTruncationSignalTests.cs @@ -372,6 +372,105 @@ public sealed class AlarmTruncationSignalTests await monitor.StopAsync(CancellationToken.None); } + /// + /// A subscriber that stops reading is dropped once its bounded queue fills, and the fault it + /// observes is the typed — the signal the + /// gRPC layer maps to ResourceExhausted — rather than a bare InvalidOperationException. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task SubscriberThatStopsReading_IsDroppedWithTypedOverflowFault() + { + // Comfortably past the monitor's per-subscriber queue capacity (2048). + const int ProbeCount = 2300; + using GatewayMetrics metrics = new(); + StubSessionManager sessions = new(); + using GatewayAlarmMonitor monitor = CreateMonitor(sessions, metrics); + + using CancellationTokenSource cts = new(); + await monitor.StartAsync(cts.Token); + await sessions.WaitForReconcileAsync(WaitTimeout); + + // The slow subscriber: registered (first frame read) and then never read again. + using CancellationTokenSource slowCts = new(WaitTimeout); + await using IAsyncEnumerator slow = + monitor.StreamAsync(null, slowCts.Token).GetAsyncEnumerator(slowCts.Token); + Assert.True(await slow.MoveNextAsync()); + + // Raises on distinct references: each one is broadcast (no dedup) without forcing a + // reconcile, which keeps the burst cheap under a loaded parallel test run. + for (ulong sequence = 1; sequence <= ProbeCount; sequence++) + { + sessions.EmitEvent(UniqueRaise(sequence)); + } + + // The monitor caches each raise and broadcasts it under the same lock, so once the cache + // holds every raise, every broadcast has been offered to the slow subscriber. (A draining + // reader is no barrier here: under a loaded test run it can fall 2048 frames behind the + // burst and be dropped itself.) + await WaitUntilAsync(() => monitor.CurrentAlarms.Count >= ProbeCount, WaitTimeout); + + // Drain what the slow subscriber had buffered; the terminal fault is the typed overflow. + await Assert.ThrowsAsync(async () => + { + while (await slow.MoveNextAsync()) + { + } + }); + + await cts.CancelAsync(); + await monitor.StopAsync(CancellationToken.None); + } + + /// + /// A StreamAsync attach reuses the memoized alarm-set projection that CurrentAlarms serves + /// (no per-attach clone of the whole set under the broadcast lock) and still applies the + /// subscriber's prefix filter to the snapshot it emits. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task SubscriberAttach_ReusesMemoizedProjectionAndAppliesPrefixFilter() + { + using GatewayMetrics metrics = new(); + StubSessionManager sessions = new() + { + Snapshots = + [ + NewSnapshot("Galaxy!Area.Tank01.Level.HiHi", fromTruncatedSnapshot: false), + NewSnapshot("Galaxy!Area.Pump02.Flow.Lo", fromTruncatedSnapshot: false), + ], + }; + using GatewayAlarmMonitor monitor = CreateMonitor(sessions, metrics); + + using CancellationTokenSource cts = new(); + await monitor.StartAsync(cts.Token); + await sessions.WaitForReconcileAsync(WaitTimeout); + await WaitUntilAsync(() => monitor.CurrentAlarms.Count == 2, WaitTimeout); + IReadOnlyList projection = monitor.CurrentAlarms; + + List streamed = []; + using CancellationTokenSource streamCts = new(WaitTimeout); + await foreach (AlarmFeedMessage message in monitor.StreamAsync("Galaxy!Area.Tank01", streamCts.Token)) + { + if (message.PayloadCase == AlarmFeedMessage.PayloadOneofCase.ActiveAlarm) + { + streamed.Add(message.ActiveAlarm); + } + + if (message.PayloadCase == AlarmFeedMessage.PayloadOneofCase.SnapshotComplete) + { + break; + } + } + + ActiveAlarmSnapshot only = Assert.Single(streamed); + Assert.Equal("Galaxy!Area.Tank01.Level.HiHi", only.AlarmFullReference); + Assert.Contains(projection, alarm => ReferenceEquals(alarm, only)); + + await cts.CancelAsync(); + await monitor.StopAsync(CancellationToken.None); + } + private static int CountOf(List received, AlarmFeedMessage.PayloadOneofCase payloadCase) { lock (received) @@ -442,6 +541,23 @@ public sealed class AlarmTruncationSignalTests }, }; + // A Raise on a reference unique to the sequence, so the monitor never dedups it. + private static MxEvent UniqueRaise(ulong sequence) => new() + { + Family = MxEventFamily.OnAlarmTransition, + WorkerSequence = sequence, + OnAlarmTransition = new OnAlarmTransitionEvent + { + AlarmFullReference = $"Galaxy!Area.Tank{sequence}.Level.Hi", + SourceObjectReference = $"Tank{sequence}.Level", + AlarmTypeName = "AnalogLimitAlarm.Hi", + TransitionKind = AlarmTransitionKind.Raise, + Severity = 500, + SourceProvider = AlarmProviderMode.Alarmmgr, + TransitionTimestamp = Google.Protobuf.WellKnownTypes.Timestamp.FromDateTimeOffset(DateTimeOffset.UtcNow), + }, + }; + private static ActiveAlarmSnapshot NewSnapshot(string reference, bool fromTruncatedSnapshot) { return new ActiveAlarmSnapshot diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs index 1b473aa..ce4cb9d 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs @@ -2,9 +2,11 @@ using System.Diagnostics.Metrics; using System.Runtime.CompilerServices; using Google.Protobuf.WellKnownTypes; using Grpc.Core; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.MxGateway.Contracts; using ZB.MOM.WW.MxGateway.Contracts.Proto; +using ZB.MOM.WW.MxGateway.Server.Alarms; using ZB.MOM.WW.MxGateway.Server.Grpc; using ZB.MOM.WW.MxGateway.Server.Metrics; using ZB.MOM.WW.MxGateway.Server.Security.Authentication; @@ -213,6 +215,8 @@ public sealed class MxAccessGatewayServiceTests Assert.Equal(42, reply.AddItem.ItemHandle); Assert.Equal("status detail", Assert.Single(reply.Statuses).DiagnosticText); Assert.Equal("mxaccess diagnostic", reply.DiagnosticMessage); + // The reply echoes the caller's client_correlation_id, not the internal worker envelope id. + Assert.Equal("client-correlation", reply.CorrelationId); } /// Verifies that StreamEvents writes only events after the specified worker sequence. @@ -463,11 +467,151 @@ public sealed class MxAccessGatewayServiceTests Assert.Contains("server-stream-active-alarms", reply.Capabilities); } + /// + /// Every reply echoes the request's client_correlation_id, including when the caller + /// sent none: the internal worker envelope id never reaches the client. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Invoke_WithoutClientCorrelationId_DoesNotLeakWorkerEnvelopeId() + { + FakeSessionManager sessionManager = new() + { + InvokeReply = new WorkerCommandReply + { + Reply = new MxCommandReply + { + SessionId = "session-1", + CorrelationId = "4f1d1b0e-internal-envelope-guid", + Kind = MxCommandKind.AddItem, + ProtocolStatus = MxAccessGrpcMapper.Ok(), + AddItem = new AddItemReply { ItemHandle = 42 }, + }, + }, + }; + MxAccessGatewayService service = CreateService(sessionManager); + MxCommandRequest request = new() + { + SessionId = "session-1", + Command = new MxCommand + { + Kind = MxCommandKind.AddItem, + AddItem = new AddItemCommand { ServerHandle = 12, ItemDefinition = "Galaxy.Tag.Value" }, + }, + }; + + MxCommandReply reply = await service.Invoke(request, new TestServerCallContext()); + + Assert.Equal(string.Empty, reply.CorrelationId); + } + + /// + /// StreamEvents hands the call's cancellation token to every response write, so a write + /// blocked on a slow or vanished client is released when the call is cancelled. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task StreamEvents_PassesCallCancellationTokenToResponseWrites() + { + FakeSessionManager sessionManager = new(); + sessionManager.Events.Add(CreateWorkerEvent("session-1", workerSequence: 1)); + MxAccessGatewayService service = CreateService(sessionManager); + RecordingServerStreamWriter writer = new(); + using CancellationTokenSource callCts = new(); + + await service.StreamEvents( + new StreamEventsRequest { SessionId = "session-1" }, + writer, + new TestServerCallContext(cancellationToken: callCts.Token)); + + Assert.Single(writer.Messages); + Assert.Equal(callCts.Token, writer.LastWriteCancellationToken); + } + + /// + /// A response write that fails with an because the client went + /// away (the call token is cancelled) maps to Cancelled and logs no warning: a client + /// disconnect is routine, not a gateway failure. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task StreamEvents_WhenWriteFailsAfterClientCancel_ReturnsCancelledWithoutWarning() + { + FakeSessionManager sessionManager = new(); + sessionManager.Events.Add(CreateWorkerEvent("session-1", workerSequence: 1)); + RecordingLogger logger = new(); + MxAccessGatewayService service = CreateService(sessionManager, logger: logger); + using CancellationTokenSource callCts = new(); + ThrowingStreamWriter writer = new(() => + { + callCts.Cancel(); + return new IOException("The client reset the request stream."); + }); + + RpcException exception = await Assert.ThrowsAsync( + () => service.StreamEvents( + new StreamEventsRequest { SessionId = "session-1" }, + writer, + new TestServerCallContext(cancellationToken: callCts.Token))); + + Assert.Equal(StatusCode.Cancelled, exception.StatusCode); + Assert.DoesNotContain(logger.Entries, entry => entry.Level >= LogLevel.Warning); + } + + /// + /// The same IOException with a live call token is a genuine transport failure and keeps the + /// existing Unavailable + warning mapping. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task StreamEvents_WhenWriteFailsWithoutCancel_KeepsUnavailableMapping() + { + FakeSessionManager sessionManager = new(); + sessionManager.Events.Add(CreateWorkerEvent("session-1", workerSequence: 1)); + RecordingLogger logger = new(); + MxAccessGatewayService service = CreateService(sessionManager, logger: logger); + ThrowingStreamWriter writer = new(() => new IOException("Transport failure.")); + + RpcException exception = await Assert.ThrowsAsync( + () => service.StreamEvents( + new StreamEventsRequest { SessionId = "session-1" }, + writer, + new TestServerCallContext())); + + Assert.Equal(StatusCode.Unavailable, exception.StatusCode); + Assert.Contains(logger.Entries, entry => entry.Level == LogLevel.Warning); + } + + /// + /// A StreamAlarms subscriber the monitor drops for falling behind receives ResourceExhausted + /// (the same status a StreamEvents overflow carries), and the gateway logs no + /// warning-with-stack for what is a client-side slowness condition. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task StreamAlarms_WhenSubscriberOverflows_ReturnsResourceExhaustedWithoutWarning() + { + RecordingLogger logger = new(); + FakeGatewayAlarmService alarms = new() { StreamFault = new AlarmFeedSubscriberOverflowException() }; + MxAccessGatewayService service = CreateService(new FakeSessionManager(), alarmService: alarms, logger: logger); + + RpcException exception = await Assert.ThrowsAsync( + () => service.StreamAlarms( + new StreamAlarmsRequest(), + new RecordingServerStreamWriter(), + new TestServerCallContext())); + + Assert.Equal(StatusCode.ResourceExhausted, exception.StatusCode); + Assert.DoesNotContain(logger.Entries, entry => entry.Level >= LogLevel.Warning); + } + private static MxAccessGatewayService CreateService( FakeSessionManager sessionManager, IGatewayRequestIdentityAccessor? identityAccessor = null, GatewayMetrics? metrics = null, - IConstraintEnforcer? constraintEnforcer = null) + IConstraintEnforcer? constraintEnforcer = null, + IGatewayAlarmService? alarmService = null, + ILogger? logger = null) { return new MxAccessGatewayService( sessionManager, @@ -477,8 +621,8 @@ public sealed class MxAccessGatewayServiceTests new MxAccessGrpcMapper(), new FakeEventStreamService(sessionManager), metrics ?? new GatewayMetrics(), - NullLogger.Instance, - new FakeGatewayAlarmService()); + logger ?? NullLogger.Instance, + alarmService ?? new FakeGatewayAlarmService()); } private static ApiKeyIdentity CreateIdentity() @@ -786,4 +930,18 @@ public sealed class MxAccessGatewayServiceTests } } + + /// A stream writer whose token-bearing write throws the supplied exception. + /// The streamed message type. + private sealed class ThrowingStreamWriter(Func createException) : IServerStreamWriter + { + /// + public WriteOptions? WriteOptions { get; set; } + + /// + public Task WriteAsync(T message) => throw createException(); + + /// + public Task WriteAsync(T message, CancellationToken cancellationToken) => throw createException(); + } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/FakeGatewayAlarmService.cs b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/FakeGatewayAlarmService.cs index 369b672..38f873d 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/FakeGatewayAlarmService.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/FakeGatewayAlarmService.cs @@ -40,8 +40,18 @@ public sealed class FakeGatewayAlarmService : IGatewayAlarmService yield return new AlarmFeedMessage { SnapshotComplete = true }; await Task.CompletedTask.ConfigureAwait(false); + if (StreamFault is not null) + { + throw StreamFault; + } } + /// + /// Gets or sets an exception the feed throws after its snapshot, standing in for the monitor + /// completing a subscriber with a fault (for example a slow-subscriber overflow). + /// + public Exception? StreamFault { get; set; } + /// public Task AcknowledgeAsync( AcknowledgeAlarmRequest request, diff --git a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/RecordingLogger.cs b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/RecordingLogger.cs new file mode 100644 index 0000000..847f71d --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/RecordingLogger.cs @@ -0,0 +1,48 @@ +using Microsoft.Extensions.Logging; + +namespace ZB.MOM.WW.MxGateway.Tests.TestSupport; + +/// +/// that records every entry's level and formatted message, +/// so a test can assert that a path logs (or does not log) at a given level. +/// +/// The logger category type. +public sealed class RecordingLogger : ILogger +{ + private readonly object _syncRoot = new(); + private readonly List<(LogLevel Level, string Message, Exception? Exception)> _entries = []; + + /// Gets a snapshot of the recorded entries, in order. + public IReadOnlyList<(LogLevel Level, string Message, Exception? Exception)> Entries + { + get + { + lock (_syncRoot) + { + return _entries.ToArray(); + } + } + } + + /// + public IDisposable? BeginScope(TState state) + where TState : notnull => null; + + /// + public bool IsEnabled(LogLevel logLevel) => true; + + /// + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + ArgumentNullException.ThrowIfNull(formatter); + lock (_syncRoot) + { + _entries.Add((logLevel, formatter(state, exception), exception)); + } + } +} diff --git a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/RecordingServerStreamWriter.cs b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/RecordingServerStreamWriter.cs index 06539b7..c359c38 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/RecordingServerStreamWriter.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/RecordingServerStreamWriter.cs @@ -29,6 +29,25 @@ public sealed class RecordingServerStreamWriter : IServerStreamWriter /// Gets or sets options for writing messages to the stream. public WriteOptions? WriteOptions { get; set; } + /// Gets the cancellation token passed to the most recent token-bearing write. + public CancellationToken LastWriteCancellationToken { get; private set; } + + /// Records the supplied message unless the token is already cancelled. + /// The message to record. + /// Token that cancels the write. + /// A completed task. + /// + /// The interface default throws for a cancelable token, + /// and the gateway passes the call's token on every streamed write, so a test writer must + /// override this overload the way the ASP.NET Core writer does. + /// + public Task WriteAsync(T message, CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + LastWriteCancellationToken = cancellationToken; + return WriteAsync(message); + } + /// Records the supplied message. /// The message to record. /// A completed task. From 6ae5ed905f88a0ee49f572500183b964be8d87c6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:29:31 -0400 Subject: [PATCH 4/8] fix(grpc): record the real StreamEvents disconnect reason StreamDisconnected always recorded "Detached". The reason tag is now a bounded EventStreamDisconnectReasons constant for how the stream ended: SessionClosed, Overflow, WorkerFault, ClientCancelled, Error, or Detached. --- docs/Grpc.md | 2 +- docs/Metrics.md | 4 +- .../Grpc/EventStreamDisconnectReasons.cs | 30 +++ .../Grpc/EventStreamService.cs | 32 ++- .../Gateway/Grpc/EventStreamServiceTests.cs | 197 +++++++++++++++++- 5 files changed, 260 insertions(+), 5 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamDisconnectReasons.cs diff --git a/docs/Grpc.md b/docs/Grpc.md index 5c0bf4b..dd1c7d9 100644 --- a/docs/Grpc.md +++ b/docs/Grpc.md @@ -375,7 +375,7 @@ Under `FailFast` the session is faulted so subsequent commands return `FailedPre ### Cancellation and cleanup -The handler creates a linked cancellation token (`streamCts`) so that completing the consumer (client disconnect, error, or graceful end-of-stream) also cancels the producer. The `finally` block cancels the source, disposes the subscriber slot, awaits the producer (swallowing the expected cancellation), and emits `StreamDisconnected("Detached")` so dashboards see the disconnection regardless of cause. +The handler creates a linked cancellation token (`streamCts`) so that completing the consumer (client disconnect, error, or graceful end-of-stream) also cancels the producer. The `finally` block cancels the source, disposes the subscriber slot, awaits the producer (swallowing the expected cancellation), and emits `StreamDisconnected(reason)` so dashboards see every disconnection. The reason is the bounded constant (`EventStreamDisconnectReasons`) for how the stream ended — `SessionClosed`, `Overflow`, `WorkerFault`, `ClientCancelled`, `Error`, or `Detached` — rather than one constant for every cause, so an operator can tell slow consumers from worker faults from routine client disconnects. `WorkerClientException` thrown by the producer marks the session as faulted before completing the channel — the worker is presumed gone, and any subsequent command on that session must observe the fault rather than silently retry. diff --git a/docs/Metrics.md b/docs/Metrics.md index e3b9bf6..1a8f7a3 100644 --- a/docs/Metrics.md +++ b/docs/Metrics.md @@ -45,7 +45,7 @@ All counters are `Counter`. Tag values come from the call sites listed und | `mxgateway.workers.killed` | `reason` | Forced terminations of worker processes. | | `mxgateway.workers.exited` | `reason` | Clean or fault-driven worker exits. | | `mxgateway.heartbeats.failed` | *(none)* | Aggregate worker heartbeat misses. The exported counter carries **no** `session_id` tag — per-session attribution is unbounded cardinality (every session mints a new time series), so it is kept out of the exporter; use the dashboard snapshot or the log scope for per-session detail. | -| `mxgateway.grpc.streams.disconnected` | `reason` | Detachments of the dashboard or client gRPC event stream. | +| `mxgateway.grpc.streams.disconnected` | `reason` | Ends of a client `StreamEvents` stream. `reason` is one of a fixed set (`EventStreamDisconnectReasons`): `SessionClosed` (the session event stream completed), `Overflow` (subscriber fell behind its bounded queue), `WorkerFault` (the worker event stream faulted), `ClientCancelled` (the client cancelled or disconnected), `Error` (unclassified), `Detached` (the consumer stopped reading without cancelling). | | `mxgateway.retries.attempted` | `area` | Resilience retries executed by gateway components. | ### Histograms @@ -181,7 +181,7 @@ IDisposable backlogRegistration = metrics.RegisterEventStreamBacklogSource( () => subscriber.Reader.CanCount ? subscriber.Reader.Count : 0); ... backlogRegistration.Dispose(); -metrics.StreamDisconnected("Detached"); +metrics.StreamDisconnected(disconnectReason); // an EventStreamDisconnectReasons constant ... metrics.QueueOverflow("grpc-event-stream"); metrics.Fault(SessionManagerErrorCode.EventQueueOverflow.ToString()); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamDisconnectReasons.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamDisconnectReasons.cs new file mode 100644 index 0000000..edbba0f --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamDisconnectReasons.cs @@ -0,0 +1,30 @@ +namespace ZB.MOM.WW.MxGateway.Server.Grpc; + +/// +/// The bounded set of reason tag values recorded on +/// mxgateway.grpc.streams.disconnected when a StreamEvents stream ends. +/// +/// +/// Constants rather than exception messages or free text so the metric's tag cardinality stays +/// fixed: an exporter mints one time series per distinct tag value. +/// +public static class EventStreamDisconnectReasons +{ + /// The session's event stream completed cleanly (session closed or the worker stream ended). + public const string SessionClosed = "SessionClosed"; + + /// The subscriber fell behind its bounded queue and was disconnected. + public const string Overflow = "Overflow"; + + /// The worker event stream terminated with a fault. + public const string WorkerFault = "WorkerFault"; + + /// The client cancelled the call or disconnected. + public const string ClientCancelled = "ClientCancelled"; + + /// The stream ended on an unclassified error. + public const string Error = "Error"; + + /// The consumer stopped reading without cancelling the call. + public const string Detached = "Detached"; +} diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs index 671e933..7d6fafe 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/EventStreamService.cs @@ -129,6 +129,10 @@ public sealed class EventStreamService( IDisposable backlogRegistration = metrics.RegisterEventStreamBacklogSource( () => subscriber.Reader.CanCount ? subscriber.Reader.Count : 0); + // Why this stream ended, recorded as a bounded constant on the disconnect metric. Set at + // the point the end is observed; a consumer that simply stops iterating keeps Detached. + string disconnectReason = EventStreamDisconnectReasons.Detached; + try { // Emit order for a resume: the ReplayGap sentinel FIRST (only when events were @@ -173,13 +177,31 @@ public sealed class EventStreamService( // here. Mirror the original ProduceEventsAsync behavior: fault the // session and record the metric, then propagate the terminal fault to the // gRPC client. + disconnectReason = EventStreamDisconnectReasons.WorkerFault; session.MarkFaulted(workerException.Message); metrics.Fault(WorkerClientErrorCode.WorkerFaulted.ToString()); throw; } + catch (SessionManagerException sessionException) + when (sessionException.ErrorCode == SessionManagerErrorCode.EventQueueOverflow) + { + disconnectReason = EventStreamDisconnectReasons.Overflow; + throw; + } + catch (OperationCanceledException) + { + disconnectReason = EventStreamDisconnectReasons.ClientCancelled; + throw; + } + catch (Exception) + { + disconnectReason = EventStreamDisconnectReasons.Error; + throw; + } if (!hasMore) { + disconnectReason = EventStreamDisconnectReasons.SessionClosed; break; } @@ -214,7 +236,15 @@ public sealed class EventStreamService( backlogRegistration.Dispose(); subscriber.Dispose(); - metrics.StreamDisconnected("Detached"); + // A consumer that stopped iterating mid-stream lands here with the Detached default; + // when the call token has fired, that early stop was the client going away. + if (disconnectReason == EventStreamDisconnectReasons.Detached + && cancellationToken.IsCancellationRequested) + { + disconnectReason = EventStreamDisconnectReasons.ClientCancelled; + } + + metrics.StreamDisconnected(disconnectReason); } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/EventStreamServiceTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/EventStreamServiceTests.cs index 177e6e9..063dad2 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/EventStreamServiceTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/EventStreamServiceTests.cs @@ -263,7 +263,7 @@ public sealed class EventStreamServiceTests GatewayMetricsSnapshot snapshot = metrics.GetSnapshot(); Assert.Equal(1, snapshot.QueueOverflows); Assert.Equal(1, snapshot.Faults); - // The finally block in StreamEventsAsync calls StreamDisconnected("Detached") on the + // The finally block in StreamEventsAsync records StreamDisconnected (reason Overflow) on the // overflow+fault path too; pin it here so a regression removing that call is caught. Assert.Equal(1, snapshot.StreamDisconnects); } @@ -572,6 +572,142 @@ public sealed class EventStreamServiceTests } } + /// + /// A stream that ends because the session's event stream completed records the + /// SessionClosed disconnect reason rather than a blanket constant. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task StreamEventsAsync_WhenEventStreamCompletes_RecordsSessionClosedReason() + { + FakeWorkerClient workerClient = new(); + using GatewayMetrics metrics = new(); + using DisconnectReasonRecorder reasons = new(metrics); + GatewaySession session = CreateReadySession(workerClient); + EventStreamService service = CreateService(new FakeSessionManager(session), metrics); + workerClient.Events.Add(CreateWorkerEvent(sequence: 1, MxEventFamily.OnDataChange)); + workerClient.CompleteAfterConfiguredEvents = true; + + await CollectEventsAsync(service, session.SessionId); + + Assert.Equal([EventStreamDisconnectReasons.SessionClosed], reasons.Reasons); + } + + /// A per-subscriber overflow records the Overflow disconnect reason. + /// A task that represents the asynchronous operation. + [Fact] + public async Task StreamEventsAsync_WhenStreamQueueOverflows_RecordsOverflowReason() + { + FakeWorkerClient workerClient = new(); + using GatewayMetrics metrics = new(); + using DisconnectReasonRecorder reasons = new(metrics); + GatewaySession session = CreateReadySession( + workerClient, + queueCapacity: 1, + metrics: metrics, + backpressurePolicy: EventBackpressurePolicy.DisconnectSubscriber); + EventStreamService service = CreateService( + new FakeSessionManager(session), + metrics, + queueCapacity: 1, + backpressurePolicy: EventBackpressurePolicy.DisconnectSubscriber); + for (ulong sequence = 1; sequence <= 50; sequence++) + { + workerClient.Events.Add(CreateWorkerEvent(sequence, MxEventFamily.OnDataChange)); + } + + workerClient.CompleteAfterConfiguredEvents = true; + await using (IAsyncEnumerator subscriber = service + .StreamEventsAsync(CreateRequest(session.SessionId), callerKeyId: null, CancellationToken.None) + .GetAsyncEnumerator()) + { + await Assert.ThrowsAsync( + async () => + { + while (await subscriber.MoveNextAsync().AsTask().WaitAsync(TestTimeout)) + { + } + }); + } + + Assert.Equal([EventStreamDisconnectReasons.Overflow], reasons.Reasons); + } + + /// A terminal worker fault records the WorkerFault disconnect reason. + /// A task that represents the asynchronous operation. + [Fact] + public async Task StreamEventsAsync_WhenWorkerEventStreamFaults_RecordsWorkerFaultReason() + { + FakeWorkerClient workerClient = new() + { + TerminalException = new WorkerClientException( + WorkerClientErrorCode.WorkerFaulted, + "worker terminal fault"), + }; + GatewaySession session = CreateReadySession(workerClient); + using GatewayMetrics metrics = new(); + using DisconnectReasonRecorder reasons = new(metrics); + EventStreamService service = CreateService(new FakeSessionManager(session), metrics); + await using (IAsyncEnumerator subscriber = service + .StreamEventsAsync(CreateRequest(session.SessionId), callerKeyId: null, CancellationToken.None) + .GetAsyncEnumerator()) + { + await Assert.ThrowsAsync( + async () => await subscriber.MoveNextAsync().AsTask().WaitAsync(TestTimeout)); + } + + Assert.Equal([EventStreamDisconnectReasons.WorkerFault], reasons.Reasons); + } + + /// A client cancellation records the ClientCancelled disconnect reason. + /// A task that represents the asynchronous operation. + [Fact] + public async Task StreamEventsAsync_WhenCanceled_RecordsClientCancelledReason() + { + FakeWorkerClient workerClient = new(); + GatewaySession session = CreateReadySession(workerClient); + using GatewayMetrics metrics = new(); + using DisconnectReasonRecorder reasons = new(metrics); + EventStreamService service = CreateService(new FakeSessionManager(session), metrics); + using CancellationTokenSource cancellationTokenSource = new(); + await using (IAsyncEnumerator subscriber = service + .StreamEventsAsync(CreateRequest(session.SessionId), callerKeyId: null, cancellationTokenSource.Token) + .GetAsyncEnumerator(cancellationTokenSource.Token)) + { + Task moveTask = subscriber.MoveNextAsync().AsTask(); + await WaitUntilAsync(() => session.ActiveEventSubscriberCount == 1); + await cancellationTokenSource.CancelAsync(); + await Assert.ThrowsAnyAsync( + async () => await moveTask.WaitAsync(TestTimeout)); + } + + Assert.Equal([EventStreamDisconnectReasons.ClientCancelled], reasons.Reasons); + } + + /// + /// A consumer that stops reading without cancelling (the gRPC writer gave up) keeps the + /// Detached reason. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task StreamEventsAsync_WhenConsumerStopsEarly_RecordsDetachedReason() + { + FakeWorkerClient workerClient = new(); + GatewaySession session = CreateReadySession(workerClient); + using GatewayMetrics metrics = new(); + using DisconnectReasonRecorder reasons = new(metrics); + EventStreamService service = CreateService(new FakeSessionManager(session), metrics); + workerClient.Events.Add(CreateWorkerEvent(sequence: 1, MxEventFamily.OnDataChange)); + await using (IAsyncEnumerator subscriber = service + .StreamEventsAsync(CreateRequest(session.SessionId), callerKeyId: null, CancellationToken.None) + .GetAsyncEnumerator()) + { + await ReadNextAsync(subscriber); + } + + Assert.Equal([EventStreamDisconnectReasons.Detached], reasons.Reasons); + } + private static async Task ReadNextAsync(IAsyncEnumerator enumerator) { Assert.True(await enumerator.MoveNextAsync().AsTask().WaitAsync(TestTimeout)); @@ -896,4 +1032,63 @@ public sealed class EventStreamServiceTests return ValueTask.CompletedTask; } } + + /// + /// Captures the reason tag of every mxgateway.grpc.streams.disconnected + /// measurement recorded on one instance (filtered by meter + /// reference, so parallel tests sharing the meter name cannot cross-talk). + /// + private sealed class DisconnectReasonRecorder : IDisposable + { + private readonly System.Diagnostics.Metrics.MeterListener _listener = new(); + private readonly List _reasons = []; + + /// Initializes a new instance of the class. + /// The metrics instance whose meter to observe. + public DisconnectReasonRecorder(GatewayMetrics metrics) + { + _listener.InstrumentPublished = (instrument, listener) => + { + if (ReferenceEquals(instrument.Meter, metrics.Meter) + && instrument.Name == "mxgateway.grpc.streams.disconnected") + { + listener.EnableMeasurementEvents(instrument); + } + }; + _listener.SetMeasurementEventCallback((instrument, _, tags, _) => + { + if (!ReferenceEquals(instrument.Meter, metrics.Meter)) + { + return; + } + + foreach (KeyValuePair tag in tags) + { + if (tag.Key == "reason" && tag.Value is string reason) + { + lock (_reasons) + { + _reasons.Add(reason); + } + } + } + }); + _listener.Start(); + } + + /// Gets the recorded reasons, in order. + public string[] Reasons + { + get + { + lock (_reasons) + { + return [.. _reasons]; + } + } + } + + /// + public void Dispose() => _listener.Dispose(); + } } From 68d6b5fd6f94b861a85f509d386bd27b8f4028d0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:30:57 -0400 Subject: [PATCH 5/8] perf(metrics): cache MxEventFamily names for per-event metric tags WorkerClient.StageWorkerEvent and StreamEvents called Enum.ToString per event (per subscriber on the stream path). MxEventFamilyNames serves the contract-fixed names from a frozen lookup, falling back to ToString for undefined values so exported tag values are unchanged. --- .../Grpc/MxAccessGatewayService.cs | 2 +- .../Metrics/MxEventFamilyNames.cs | 28 +++++++++++++ .../Workers/WorkerClient.cs | 2 +- .../Metrics/MxEventFamilyNamesTests.cs | 41 +++++++++++++++++++ 4 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Metrics/MxEventFamilyNames.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Tests/Metrics/MxEventFamilyNamesTests.cs diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs index 5ed29ea..201d3e0 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs @@ -183,7 +183,7 @@ public sealed class MxAccessGatewayService( // call is cancelled, instead of leaving it to the transport's own timeout. await responseStream.WriteAsync(publicEvent, context.CancellationToken).ConfigureAwait(false); metrics.RecordEventStreamSend( - publicEvent.Family.ToString(), + MxEventFamilyNames.Get(publicEvent.Family), Stopwatch.GetElapsedTime(sendStartTimestamp)); } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Metrics/MxEventFamilyNames.cs b/src/ZB.MOM.WW.MxGateway.Server/Metrics/MxEventFamilyNames.cs new file mode 100644 index 0000000..1fe54d4 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Metrics/MxEventFamilyNames.cs @@ -0,0 +1,28 @@ +using System.Collections.Frozen; +using ZB.MOM.WW.MxGateway.Contracts.Proto; + +namespace ZB.MOM.WW.MxGateway.Server.Metrics; + +/// +/// Cached names for the per-event metric tags. +/// +/// +/// allocates a new string on every call, and the event-received +/// and stream-send metrics tag every event with its family — once per event at the worker +/// boundary and once per event per subscriber on the stream. The names are fixed by the +/// contract, so they are computed once here. A value the contract does not define falls back +/// to , which is the same string the uncached call produced. +/// +public static class MxEventFamilyNames +{ + private static readonly FrozenDictionary Names = + Enum.GetValues() + .Distinct() + .ToFrozenDictionary(family => family, family => family.ToString()); + + /// Gets the name of an event family. + /// The event family. + /// The family's name, identical to . + public static string Get(MxEventFamily family) => + Names.TryGetValue(family, out string? name) ? name : family.ToString(); +} diff --git a/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs b/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs index 873dcdd..fc8b48b 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Workers/WorkerClient.cs @@ -692,7 +692,7 @@ public sealed class WorkerClient : IWorkerClient { if (workerEvent.Event is not null) { - _metrics?.EventReceived(SessionId, workerEvent.Event.Family.ToString()); + _metrics?.EventReceived(SessionId, MxEventFamilyNames.Get(workerEvent.Event.Family)); } if (_eventStaging.Writer.TryWrite(workerEvent)) diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Metrics/MxEventFamilyNamesTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Metrics/MxEventFamilyNamesTests.cs new file mode 100644 index 0000000..953280f --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/Metrics/MxEventFamilyNamesTests.cs @@ -0,0 +1,41 @@ +using ZB.MOM.WW.MxGateway.Contracts.Proto; +using ZB.MOM.WW.MxGateway.Server.Metrics; + +namespace ZB.MOM.WW.MxGateway.Tests.Metrics; + +public sealed class MxEventFamilyNamesTests +{ + /// + /// Every defined family maps to exactly the name produces, so + /// switching the per-event metric tags to the cached lookup changes no exported tag value. + /// + [Fact] + public void Get_ForEveryDefinedFamily_MatchesEnumToString() + { + foreach (MxEventFamily family in Enum.GetValues()) + { + Assert.Equal(family.ToString(), MxEventFamilyNames.Get(family)); + } + } + + /// A defined family returns the same cached string instance on every call (no per-event allocation). + [Fact] + public void Get_ForDefinedFamily_ReturnsCachedInstance() + { + Assert.Same( + MxEventFamilyNames.Get(MxEventFamily.OnDataChange), + MxEventFamilyNames.Get(MxEventFamily.OnDataChange)); + } + + /// + /// A numeric value the contract does not define (a newer worker, or a malformed frame) still + /// produces the form rather than throwing. + /// + [Fact] + public void Get_ForUndefinedValue_FallsBackToToString() + { + MxEventFamily undefined = (MxEventFamily)987654; + + Assert.Equal(undefined.ToString(), MxEventFamilyNames.Get(undefined)); + } +} From ed3a2c9c8b5e3dae13f8c77b30b72677e886f9a0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:33:18 -0400 Subject: [PATCH 6/8] fix(auth): single-flight API-key verification misses; prune key-id index on eviction CachingApiKeyVerifier's key-id index was only pruned by Invalidate, so it grew with every token ever verified. A post-eviction callback now drops the slot (under the same lock as Set + index, so a late callback cannot unindex a re-cached entry). Concurrent misses for the same token and key generation now share one inner verification, so a TTL expiry under load no longer stampedes SQLite; keying the flight on the generation keeps the revoke guarantee for joiners. --- docs/Authentication.md | 10 + .../Authentication/CachingApiKeyVerifier.cs | 187 ++++++++++++++++-- .../CachingApiKeyVerifierTests.cs | 114 +++++++++++ 3 files changed, 295 insertions(+), 16 deletions(-) diff --git a/docs/Authentication.md b/docs/Authentication.md index eb37cb7..0e77d1f 100644 --- a/docs/Authentication.md +++ b/docs/Authentication.md @@ -113,6 +113,16 @@ library: library returns carries no expiry timestamp, so the cache cannot cap an entry at the key's expiry (capping it needs the donor library to surface expiry on the verification identity). The default 15 s TTL bounds both windows. + Misses are **single-flight**: concurrent misses for the same token share one inner + verification, so a TTL expiry under load costs one store read instead of one per + in-flight RPC. The flight is keyed on the token hash *and* the key's generation, so + a miss that starts after an `Invalidate` never joins a verification that started + before it and cannot receive the pre-revoke identity. The shared inner call is not + bound to any one caller's cancellation token; a caller that gives up stops waiting + without cancelling the others. The per-key index `Invalidate` uses to find every + cached secret variant is pruned by a post-eviction callback when the cache drops an + entry on its own (TTL expiry, compaction), so it tracks the live cache instead of + growing with every token ever verified. - **`CoalescingMarkApiKeyStore`** wraps the library `IApiKeyStore` and forwards at most one `MarkUsed` write per key per `MxGateway:Security:ApiKeyLastUsedCoalesceSeconds` (default 60 s), so even under a diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/CachingApiKeyVerifier.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/CachingApiKeyVerifier.cs index 2e66215..48d3d8c 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/CachingApiKeyVerifier.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/CachingApiKeyVerifier.cs @@ -58,6 +58,19 @@ public interface IApiKeyCacheInvalidator /// window and it is intentionally kept short (, /// default 15 s). /// +/// +/// Misses are single-flight: concurrent misses for the same token and the same key generation share +/// one inner verification, so a TTL expiry under load costs one SQLite read rather than one per +/// in-flight RPC. A miss that starts after an never joins a flight that +/// started before it (the flight is keyed on the generation too), so the revoke guarantee holds for +/// joiners as well. The shared inner call is not bound to any one caller's cancellation token; each +/// caller stops waiting on its own token without cancelling the others. +/// +/// +/// The key-id index is pruned by a post-eviction callback whenever the cache drops an entry on its +/// own (expiry, compaction), not only by , so it stays proportional to the +/// live cache rather than to every token ever verified. +/// /// public sealed class CachingApiKeyVerifier : IApiKeyVerifier, IApiKeyCacheInvalidator { @@ -69,9 +82,20 @@ public sealed class CachingApiKeyVerifier : IApiKeyVerifier, IApiKeyCacheInvalid private readonly TimeSpan _ttl; // keyId -> set of cache keys currently held for that id, so a revoke/rotate can evict every - // secret variant (e.g. an old secret still within TTL after a rotate). - private readonly ConcurrentDictionary> _keyIdIndex = - new(StringComparer.Ordinal); + // secret variant (e.g. an old secret still within TTL after a rotate). Guarded by _indexSync, + // which also covers the cache Set that an index entry describes: the post-eviction callback + // runs asynchronously, so Set + index and check-cache + unindex must each be atomic with + // respect to the other, or a late callback could unindex a freshly re-cached entry and leave + // it out of reach of a later Invalidate. Every operation under the lock is O(1) and the lock is + // only taken on misses, evictions and invalidations — never on a cache hit. + private readonly object _indexSync = new(); + private readonly Dictionary> _keyIdIndex = new(StringComparer.Ordinal); + + // (cache key, key generation) -> the in-flight inner verification for that pair (single-flight). + private readonly ConcurrentDictionary<(string CacheKey, long Generation), Lazy>> _inflight = + new(); + + private readonly PostEvictionDelegate _onEvicted; // keyId -> monotonic generation, bumped by Invalidate before it evicts. A VerifyAsync snapshots // the generation before calling the inner verifier and only caches if it is unchanged after the @@ -103,6 +127,37 @@ public sealed class CachingApiKeyVerifier : IApiKeyVerifier, IApiKeyCacheInvalid _inner = inner; _cache = cache; _ttl = ttl; + _onEvicted = OnCacheEntryEvicted; + } + + /// Gets the number of cache keys currently held in the key-id index (test seam). + internal int IndexedCacheKeyCount + { + get + { + lock (_indexSync) + { + int count = 0; + foreach (HashSet set in _keyIdIndex.Values) + { + count += set.Count; + } + + return count; + } + } + } + + /// Gets the number of key ids currently present in the key-id index (test seam). + internal int IndexedKeyIdCount + { + get + { + lock (_indexSync) + { + return _keyIdIndex.Count; + } + } } /// @@ -126,11 +181,50 @@ public sealed class CachingApiKeyVerifier : IApiKeyVerifier, IApiKeyCacheInvalid } // Snapshot the key's generation BEFORE the inner verify so a concurrent Invalidate that runs - // while we are in the library is detected below and its repopulation discarded. + // while we are in the library is detected and its repopulation discarded. The snapshot is + // also half of the single-flight key, so a miss that starts after an Invalidate cannot join + // a flight that started before it. string? tokenKeyId = TryParseKeyId(authorizationHeader); long generationAtStart = tokenKeyId is null ? 0 : ReadGeneration(tokenKeyId); + (string CacheKey, long Generation) flightKey = (cacheKey, generationAtStart); - ApiKeyVerification result = await _inner.VerifyAsync(authorizationHeader, ct).ConfigureAwait(false); + Lazy> created = new( + () => VerifyMissAsync(authorizationHeader, cacheKey, tokenKeyId, generationAtStart)); + Lazy> flight = _inflight.GetOrAdd(flightKey, created); + Task verification = flight.Value; + if (ReferenceEquals(flight, created)) + { + // The flight's creator retires it once it completes (success, failure or fault), so the + // next miss for this token starts fresh. Removing by (key, value) pair never retires a + // newer flight that replaced this one. + _ = verification.ContinueWith( + static (_, state) => + { + var (owner, key, completed) = + ((CachingApiKeyVerifier, (string, long), Lazy>))state!; + owner._inflight.TryRemove( + new KeyValuePair<(string CacheKey, long Generation), Lazy>>(key, completed)); + }, + (this, flightKey, flight), + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + } + + return await verification.WaitAsync(ct).ConfigureAwait(false); + } + + // The shared body of one single-flight miss. Not bound to any caller's token: a caller that + // gives up must not cancel the verification other callers are waiting on. + private async Task VerifyMissAsync( + string authorizationHeader, + string cacheKey, + string? tokenKeyId, + long generationAtStart) + { + ApiKeyVerification result = await _inner + .VerifyAsync(authorizationHeader, CancellationToken.None) + .ConfigureAwait(false); if (result.Succeeded && result.Identity is not null) { @@ -158,11 +252,21 @@ public sealed class CachingApiKeyVerifier : IApiKeyVerifier, IApiKeyCacheInvalid } string keyId = result.Identity!.KeyId; - _cache.Set(cacheKey, result, new MemoryCacheEntryOptions + MemoryCacheEntryOptions entryOptions = new() { AbsoluteExpirationRelativeToNow = _ttl, + }; + entryOptions.PostEvictionCallbacks.Add(new PostEvictionCallbackRegistration + { + EvictionCallback = _onEvicted, + State = keyId, }); - IndexCacheKey(keyId, cacheKey); + + lock (_indexSync) + { + _cache.Set(cacheKey, result, entryOptions); + IndexCacheKey(keyId, cacheKey); + } // Set-then-recheck: an Invalidate that landed between the pre-Set check and IndexCacheKey // would have missed this entry (it was not yet indexed). If the generation has moved, evict @@ -170,13 +274,38 @@ public sealed class CachingApiKeyVerifier : IApiKeyVerifier, IApiKeyCacheInvalid if (tokenKeyId is not null && ReadGeneration(tokenKeyId) != generationAtStart) { _cache.Remove(cacheKey); - if (_keyIdIndex.TryGetValue(keyId, out ConcurrentDictionary? set)) + lock (_indexSync) { - set.TryRemove(cacheKey, out _); + UnindexCacheKey(keyId, cacheKey); } } } + // Post-eviction callback: drops the evicted entry's key-id index slot so the index tracks the + // live cache. MemoryCache raises it asynchronously, so by the time it runs the same cache key + // may already hold a newer entry (re-verified after the expiry); that entry owns the slot, and + // the check below runs under the lock that also covers Set + index, so it cannot interleave + // with a re-cache. + private void OnCacheEntryEvicted(object key, object? value, EvictionReason reason, object? state) + { + if (reason == EvictionReason.Replaced || key is not string cacheKey || state is not string keyId) + { + // Replaced: a newer entry under the same key was Set in this entry's place and is + // indexed under the same slot already. + return; + } + + lock (_indexSync) + { + if (_cache.TryGetValue(cacheKey, out _)) + { + return; + } + + UnindexCacheKey(keyId, cacheKey); + } + } + /// public void Invalidate(string keyId) { @@ -189,12 +318,22 @@ public sealed class CachingApiKeyVerifier : IApiKeyVerifier, IApiKeyCacheInvalid // generation refuses to cache (or self-evicts) its now-stale repopulation. _generations.AddOrUpdate(keyId, 1, static (_, current) => current + 1); - if (_keyIdIndex.TryRemove(keyId, out ConcurrentDictionary? cacheKeys)) + string[] cacheKeys; + lock (_indexSync) { - foreach (string cacheKey in cacheKeys.Keys) + if (!_keyIdIndex.Remove(keyId, out HashSet? indexed)) { - _cache.Remove(cacheKey); + return; } + + cacheKeys = [.. indexed]; + } + + // Evicted outside the lock: the eviction callbacks these raise take it themselves (and find + // the slot already gone). + foreach (string cacheKey in cacheKeys) + { + _cache.Remove(cacheKey); } } @@ -257,12 +396,28 @@ public sealed class CachingApiKeyVerifier : IApiKeyVerifier, IApiKeyCacheInvalid return new string(afterPrefix[..separator]); } + // Caller holds _indexSync. private void IndexCacheKey(string keyId, string cacheKey) { - ConcurrentDictionary set = _keyIdIndex.GetOrAdd( - keyId, - static _ => new ConcurrentDictionary(StringComparer.Ordinal)); - set.TryAdd(cacheKey, 0); + if (!_keyIdIndex.TryGetValue(keyId, out HashSet? set)) + { + set = new HashSet(StringComparer.Ordinal); + _keyIdIndex[keyId] = set; + } + + set.Add(cacheKey); + } + + // Caller holds _indexSync. Drops the key id's slot entirely once its last cache key goes, so + // the index does not accumulate empty sets for keys that are no longer cached. + private void UnindexCacheKey(string keyId, string cacheKey) + { + if (_keyIdIndex.TryGetValue(keyId, out HashSet? set) + && set.Remove(cacheKey) + && set.Count == 0) + { + _keyIdIndex.Remove(keyId); + } } private static bool TryComputeCacheKey(string? authorizationHeader, out string cacheKey) diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs index a13dd72..146002c 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs @@ -129,6 +129,120 @@ public sealed class CachingApiKeyVerifierTests Assert.Equal(2, inner.CallCount); } + /// + /// Single-flight: concurrent misses for the same token (a TTL expiry under load) share one + /// inner verification instead of each issuing its own SQLite read. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task VerifyAsync_ConcurrentMisses_ShareOneInnerVerification() + { + GatedVerifier inner = new(Success("operator01")); + using MemoryCache cache = NewCache(); + CachingApiKeyVerifier verifier = new(inner, cache, TimeSpan.FromSeconds(30)); + + Task first = verifier.VerifyAsync(Header, CancellationToken.None); + await inner.Entered; + Task[] joiners = Enumerable + .Range(0, 16) + .Select(_ => verifier.VerifyAsync(Header, CancellationToken.None)) + .ToArray(); + + inner.Release(); + ApiKeyVerification[] results = await Task.WhenAll([first, .. joiners]); + + Assert.All(results, result => Assert.True(result.Succeeded)); + Assert.Equal(1, inner.CallCount); + } + + /// + /// A miss that arrives after must not join a + /// verification that started before it: that flight may return the pre-revoke identity, so the + /// late caller starts its own flight against the inner verifier. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task VerifyAsync_MissAfterInvalidate_DoesNotJoinPreInvalidateFlight() + { + GatedVerifier inner = new(Success("operator01")); + using MemoryCache cache = NewCache(); + CachingApiKeyVerifier verifier = new(inner, cache, TimeSpan.FromSeconds(30)); + + Task preRevoke = verifier.VerifyAsync(Header, CancellationToken.None); + await inner.Entered; + ((IApiKeyCacheInvalidator)verifier).Invalidate("operator01"); + + // The gated first call is still parked; a joined call would park with it. + ApiKeyVerification postRevoke = await verifier + .VerifyAsync(Header, CancellationToken.None) + .WaitAsync(TimeSpan.FromSeconds(5)); + + Assert.True(postRevoke.Succeeded); + Assert.Equal(2, inner.CallCount); + inner.Release(); + await preRevoke; + } + + /// + /// A caller whose own token is cancelled stops waiting, but the shared verification it joined + /// is not cancelled for the other waiters. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task VerifyAsync_CancelledWaiter_DoesNotCancelSharedVerification() + { + GatedVerifier inner = new(Success("operator01")); + using MemoryCache cache = NewCache(); + CachingApiKeyVerifier verifier = new(inner, cache, TimeSpan.FromSeconds(30)); + using CancellationTokenSource cancelled = new(); + + Task survivor = verifier.VerifyAsync(Header, CancellationToken.None); + await inner.Entered; + Task abandoned = verifier.VerifyAsync(Header, cancelled.Token); + await cancelled.CancelAsync(); + + await Assert.ThrowsAnyAsync(() => abandoned); + inner.Release(); + Assert.True((await survivor).Succeeded); + Assert.Equal(1, inner.CallCount); + } + + /// + /// The key-id index is pruned when the cache evicts an entry on its own (expiry, compaction), + /// not only on — otherwise every token ever + /// verified leaves an index entry behind for the life of the process. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task CacheEviction_PrunesKeyIdIndex() + { + FakeVerifier inner = new(Success("operator01")); + using MemoryCache cache = NewCache(); + CachingApiKeyVerifier verifier = new(inner, cache, TimeSpan.FromSeconds(30)); + + await verifier.VerifyAsync(Header, CancellationToken.None); + Assert.Equal(1, verifier.IndexedCacheKeyCount); + + cache.Compact(1.0); + + DateTime deadline = DateTime.UtcNow + TimeSpan.FromSeconds(5); + while (verifier.IndexedCacheKeyCount != 0 && DateTime.UtcNow < deadline) + { + await Task.Delay(10); + } + + Assert.Equal(0, verifier.IndexedCacheKeyCount); + Assert.Equal(0, verifier.IndexedKeyIdCount); + + // The evicted entry is gone, so the next request re-verifies, and Invalidate still reaches + // the repopulated entry through the index. + await verifier.VerifyAsync(Header, CancellationToken.None); + Assert.Equal(2, inner.CallCount); + ((IApiKeyCacheInvalidator)verifier).Invalidate("operator01"); + await verifier.VerifyAsync(Header, CancellationToken.None); + Assert.Equal(3, inner.CallCount); + } + /// /// The store decorator coalesces repeated MarkUsed writes for the same key inside the /// window down to a single forwarded write — the ≤1/min guarantee for last_used_utc. From 65bca804bbbe5e4ad426e4a49ecfd83169124309 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:37:31 -0400 Subject: [PATCH 7/8] feat(security): opt-in authentication for /metrics (SEC-14) New MxGateway:Metrics:RequireAuthentication (default false). When true, /metrics requires an API key with the admin scope (verified through the cached verifier and the shared API-key failure limiter) or a dashboard login with a real dashboard role; anonymous localhost and Authentication:Mode=Disabled do not qualify. An anonymous scrape gets a plain 401 Bearer challenge, a throttled peer 429. /health stays anonymous because the off-repo Overview dashboard polls /health/active. MapZbMetrics returns IEndpointConventionBuilder, so the policy is attached with RequireAuthorization. --- docs/Authorization.md | 2 +- docs/DesignDecisions.md | 35 +++ docs/GatewayConfiguration.md | 29 ++ .../Configuration/GatewayOptions.cs | 3 + .../Configuration/MetricsOptions.cs | 15 ++ .../GatewayApplication.cs | 14 +- .../Authorization/MetricsAuthorization.cs | 210 +++++++++++++++ .../Gateway/GatewayApplicationTests.cs | 64 +++++ ...atewayGrpcAuthorizationInterceptorTests.cs | 1 - .../MetricsAuthorizationTests.cs | 249 ++++++++++++++++++ 10 files changed, 619 insertions(+), 3 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Configuration/MetricsOptions.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/MetricsAuthorization.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/MetricsAuthorizationTests.cs diff --git a/docs/Authorization.md b/docs/Authorization.md index 23c71d1..204b9f7 100644 --- a/docs/Authorization.md +++ b/docs/Authorization.md @@ -99,7 +99,7 @@ Before the verification store read, the helper asks a cheap in-process failure c - **Composite `(transport peer, key id)` partitions.** Reaching `MxGateway:Security:ApiKeyFailureLimit` failures binds the throttle to the address that produced them. The key id alone is never the partition: key ids are not secret — they ride in every token and are listed on the dashboard — so keying on them let any network peer deny a key to its legitimate holder. The key id joins the partition only after a token-shape check (literal `mxgw` prefix, at least three non-empty `_` segments, key id of at most 64 characters), and one address may mint at most 32 key-id partitions before the overflow collapses onto that address's fallback partition. - **A per-key-id aggregate** across all peers (`ApiKeyFailureAggregateLimit`, default 30), which bounds a distributed or source-rotating sprayer that never trips any single partition. -An over-limit state is a valve, not a wall: one request per `ApiKeyFailureProbeIntervalSeconds` (default 5 s) is admitted through to the real verifier, and everything else is refused with `StatusCode.ResourceExhausted` before the store read. The slot is claimed atomically, so a burst arriving together at an interval boundary still yields exactly one admission, and a slot claimed for a request that a later layer then refuses is handed back under a per-state version stamp — never by timestamp comparison, which collides whenever a concurrent failure re-arms the same state on the same clock tick. A successful verification resets both layers — which is why the reset path stays reachable while a key is under active spray. One exception: when the caller's key id was collapsed into its address's shared fallback partition by the per-peer cap, a success clears the key's aggregate but leaves that shared partition alone, since it also holds failures contributed by other key ids from the same address. The tracked partitions form a bounded LRU (`ApiKeyFailureTrackedPeers`) whose eviction prefers fully expired windows and never removes an over-limit partition below a 2x transient overshoot ceiling, so the cap bounds memory without becoming a reset button for an active block. `ResourceExhausted` reveals only that throttling is in effect, not whether any particular secret was valid, preserving the opaque-failure property. Refusals increment `mxgateway.auth.throttled`, tagged `stage=peer|aggregate` and nothing else — `/metrics` is unauthenticated, so neither key ids nor peer addresses may appear there. +An over-limit state is a valve, not a wall: one request per `ApiKeyFailureProbeIntervalSeconds` (default 5 s) is admitted through to the real verifier, and everything else is refused with `StatusCode.ResourceExhausted` before the store read. The slot is claimed atomically, so a burst arriving together at an interval boundary still yields exactly one admission, and a slot claimed for a request that a later layer then refuses is handed back under a per-state version stamp — never by timestamp comparison, which collides whenever a concurrent failure re-arms the same state on the same clock tick. A successful verification resets both layers — which is why the reset path stays reachable while a key is under active spray. One exception: when the caller's key id was collapsed into its address's shared fallback partition by the per-peer cap, a success clears the key's aggregate but leaves that shared partition alone, since it also holds failures contributed by other key ids from the same address. The tracked partitions form a bounded LRU (`ApiKeyFailureTrackedPeers`) whose eviction prefers fully expired windows and never removes an over-limit partition below a 2x transient overshoot ceiling, so the cap bounds memory without becoming a reset button for an active block. `ResourceExhausted` reveals only that throttling is in effect, not whether any particular secret was valid, preserving the opaque-failure property. Refusals increment `mxgateway.auth.throttled`, tagged `stage=peer|aggregate` and nothing else — `/metrics` is anonymous by default (`MxGateway:Metrics:RequireAuthentication` is opt-in), so neither key ids nor peer addresses may appear there. The dashboard login surface is throttled independently: `POST /auth/login` carries a fixed-window ASP.NET Core rate-limiter policy keyed per remote IP (`MxGateway:Security:LoginRateLimit*`), rejecting a burst with HTTP 429 before the LDAP bind is relayed to the directory. See [GatewayConfiguration](./GatewayConfiguration.md#security-options). diff --git a/docs/DesignDecisions.md b/docs/DesignDecisions.md index d8ed73d..88d555e 100644 --- a/docs/DesignDecisions.md +++ b/docs/DesignDecisions.md @@ -430,6 +430,41 @@ Suggested mapping: - Galaxy SQL metadata endpoints if added: `metadata:read` - worker shutdown diagnostics and key management: `admin` +## Metrics Endpoint Authentication (SEC-14) + +Decision: `/metrics` stays anonymous by default and can be put behind +authentication with `MxGateway:Metrics:RequireAuthentication=true`; `/health` +stays anonymous unconditionally. + +- **Opt-in rather than on by default.** Existing Prometheus scrapers on the + deployed hosts scrape anonymously; flipping the default would silently break + them on upgrade. Hosts whose network exposure makes the counters sensitive + enable it explicitly. +- **Health is never gated.** The family Overview dashboard (another repo) polls + `/health/active` without credentials, and liveness/readiness probes are + conventionally anonymous. Health responses carry status only. +- **Credentials reuse existing plumbing.** One `MxGateway.Metrics` scheme + accepts an API key with the `admin` scope (verified through the same cached + `IApiKeyVerifier` and the same per-peer failure limiter as gRPC, so `/metrics` + is not a way around the gRPC brute-force throttle) or, when no authorization + header is sent, the dashboard login cookie with a real dashboard role. No new + credential type or store is introduced. +- **`admin` scope, any dashboard role.** Metrics are an operator surface, so a + data-plane client key (`invoke:*`, `events:read`) is not enough. A dashboard + `Viewer` qualifies because every dashboard role already sees the same aggregate + counters on the dashboard; the exported metrics carry no session ids or tag + values. +- **No environment bypass.** The requirement reads no options, so anonymous + localhost and `Authentication:Mode=Disabled` — both of which satisfy the + dashboard Viewer policy — do not satisfy it. Only dev-only + `Dashboard:DisableLogin` (which authenticates every request as an + Administrator, and is rejected in Production) lets an unauthenticated scrape + through. +- **One scheme, plain 401.** Listing the cookie scheme on the policy directly + would challenge an anonymous scrape with a 302 redirect to `/login`; a single + scheme that falls back to the cookie internally keeps the challenge a `401` + with `WWW-Authenticate: Bearer`, which scrapers understand. + ## Worker Process Identity Decision: run workers as the gateway service identity for v1. diff --git a/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index b9c8e69..8323351 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -419,6 +419,35 @@ model requires otherwise. | `MxGateway:Security:AuditRetentionDays` | `90` | Days of canonical audit history kept in the `audit_event` table. The audit drain sweeps once at startup and hourly thereafter, deleting rows older than this window; without it the table grows without bound inside the same SQLite file the authentication hot path reads. Rows whose timestamp SQLite cannot parse are never swept. Must be greater than zero — retention can be widened but not switched off. | | `MxGateway:Security:ApiKeyFailureTrackedPeers` | `4096` | Maximum distinct partitions tracked by the failure counter (a bounded LRU) so a spray of unique tokens cannot grow memory without limit. It cannot be used to flush an active block either: only a validly shaped `mxgw__` token mints a key-id partition (everything else lands on the sender's transport-peer partition), each address may mint at most 32 key-id partitions before the overflow collapses onto that address's fallback partition, and eviction prefers fully expired windows, never removing an over-limit partition until the map exceeds twice this cap. Must be greater than zero. | +## Metrics Options + +Bound from `MxGateway:Metrics`. Governs the Prometheus `/metrics` scrape endpoint +(mounted by the shared `ZB.MOM.WW.Telemetry` package). The health endpoints +(`/healthz`, `/health/ready`, `/health/active`) are not affected by this section and +always stay anonymous: the family Overview dashboard polls `/health/active` from +another repo without credentials, so gating it would break that consumer. + +| Option | Default | Description | +|--------|---------|-------------| +| `MxGateway:Metrics:RequireAuthentication` | `false` | When `true`, `/metrics` requires an authenticated caller (SEC-14). A scrape is accepted when it presents an API key carrying the `admin` scope as `Authorization: Bearer mxgw__`, or — with no authorization header — a dashboard login cookie holding a real dashboard role (`Administrator` or `Viewer`). Anonymous localhost access and `Authentication:Mode=Disabled` do **not** satisfy it. An anonymous scrape gets `401` with `WWW-Authenticate: Bearer`; an authenticated caller without `admin` scope or a dashboard role gets `403`; a peer throttled by the API-key failure limiter (`MxGateway:Security:ApiKeyFailure*`, shared with the gRPC path) gets `429`. The default keeps the endpoint anonymous so existing scrapers keep working. | + +Configure a Prometheus scraper against an authenticated endpoint with a dedicated +admin-scoped key, for example: + +```yaml +scrape_configs: + - job_name: mxgateway + authorization: + type: Bearer + credentials_file: /etc/prometheus/mxgateway.key # contains mxgw__ + static_configs: + - targets: ["gateway-host:5130"] +``` + +The setting is read from configuration when endpoints are mapped, so changing it +requires a restart. With dev-only `Dashboard:DisableLogin=true` every request is +auto-authenticated as an Administrator, which also satisfies `/metrics`. + ## Galaxy Options | Option | Default | Description | diff --git a/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptions.cs b/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptions.cs index 3063604..39e72ba 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptions.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptions.cs @@ -52,4 +52,7 @@ public sealed class GatewayOptions /// login / API-key rate limiting). /// public SecurityOptions Security { get; init; } = new(); + + /// Gets Prometheus /metrics endpoint options. + public MetricsOptions Metrics { get; init; } = new(); } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Configuration/MetricsOptions.cs b/src/ZB.MOM.WW.MxGateway.Server/Configuration/MetricsOptions.cs new file mode 100644 index 0000000..6507142 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Configuration/MetricsOptions.cs @@ -0,0 +1,15 @@ +namespace ZB.MOM.WW.MxGateway.Server.Configuration; + +/// Options for the Prometheus /metrics scrape endpoint (MxGateway:Metrics). +public sealed class MetricsOptions +{ + /// + /// Gets a value indicating whether /metrics requires an authenticated caller. When + /// , a scrape must present either an API key carrying the admin + /// scope (Authorization: Bearer mxgw_…) or a dashboard login cookie holding a real + /// dashboard role; anonymous localhost access does not qualify. Default + /// keeps the endpoint anonymous for existing scrapers. The /health endpoints stay + /// anonymous either way. + /// + public bool RequireAuthentication { get; init; } +} diff --git a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs index af44744..d80f859 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs @@ -113,6 +113,7 @@ public static class GatewayApplication builder.Services.AddZbSecrets(builder.Configuration, "Secrets"); builder.Services.AddSqliteAuthStore(builder.Configuration); builder.Services.AddGatewayGrpcAuthorization(); + builder.Services.AddGatewayMetricsAuthorization(); AddLoginRateLimiter(builder); builder.Services.AddHealthChecks() .AddTypeActivatedCheck( @@ -350,8 +351,19 @@ public static class GatewayApplication { endpoints.MapStaticAssets(ResolveStaticAssetsManifestPath()); + // /health stays anonymous unconditionally: an off-repo Overview dashboard polls + // /health/active without credentials. endpoints.MapZbHealth(); - endpoints.MapZbMetrics(); + IEndpointConventionBuilder metricsEndpoint = endpoints.MapZbMetrics(); + // Read from configuration rather than IOptions: resolving the options here + // would run whole-options validation during Build instead of at StartAsync. + if (endpoints.ServiceProvider + .GetRequiredService() + .GetValue("MxGateway:Metrics:RequireAuthentication")) + { + // Opt-in (SEC-14): an admin-scoped API key or a dashboard login with a real role. + metricsEndpoint.RequireAuthorization(MetricsAuthorizationDefaults.Policy); + } endpoints.MapGrpcService(); endpoints.MapZbGalaxyRepository(); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/MetricsAuthorization.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/MetricsAuthorization.cs new file mode 100644 index 0000000..4779f16 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/MetricsAuthorization.cs @@ -0,0 +1,210 @@ +using System.Security.Claims; +using System.Text.Encodings.Web; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authorization; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Auth.Abstractions.ApiKeys; +using ZB.MOM.WW.MxGateway.Server.Dashboard; +using ZB.MOM.WW.MxGateway.Server.Metrics; + +namespace ZB.MOM.WW.MxGateway.Server.Security.Authorization; + +/// Names shared by the optional /metrics authentication (MxGateway:Metrics:RequireAuthentication). +public static class MetricsAuthorizationDefaults +{ + /// Authentication scheme that authenticates a metrics scrape. + public const string AuthenticationScheme = "MxGateway.Metrics"; + + /// Authorization policy attached to /metrics when authentication is required. + public const string Policy = "MxGateway.Metrics"; + + /// Claim type carrying one API-key scope on a principal the metrics scheme authenticated. + public const string ScopeClaimType = "mxgateway:scope"; +} + +/// +/// Authenticates a /metrics scrape from either credential the gateway already issues: an API +/// key presented as Authorization: Bearer mxgw_… (verified through the same cached +/// and per-peer failure limiter as gRPC calls), or, when no +/// authorization header is sent, the dashboard login cookie. +/// +/// +/// One scheme fronting both credentials keeps the challenge a plain 401 with +/// WWW-Authenticate: Bearer, which is what a Prometheus scraper understands; listing the +/// cookie scheme on the policy directly would turn an anonymous scrape into a 302 redirect to the +/// login page. A throttled key partition answers 429, mirroring the gRPC +/// ResourceExhausted. Failure reasons are never echoed to the caller and the presented +/// secret is never logged. +/// +public sealed class MetricsAuthenticationHandler( + IOptionsMonitor options, + ILoggerFactory logger, + UrlEncoder encoder, + IApiKeyVerifier apiKeyVerifier, + ApiKeyFailureLimiter failureLimiter, + IAuthenticationSchemeProvider schemeProvider, + GatewayMetrics metrics) + : AuthenticationHandler(options, logger, encoder) +{ + private bool _throttled; + + /// + protected override async Task HandleAuthenticateAsync() + { + string? authorizationHeader = Request.Headers.Authorization; + if (string.IsNullOrWhiteSpace(authorizationHeader)) + { + return await AuthenticateDashboardCookieAsync().ConfigureAwait(false); + } + + // Same throttle as the gRPC interceptor: checked before the store read so online guessing + // against /metrics cannot spend a verification per attempt or bypass the gRPC limiter. + ApiKeyThrottlePartition partition = new( + Context.Connection.RemoteIpAddress?.ToString() ?? "unknown", + GatewayGrpcAuthorizationInterceptor.TryResolveKeyId(authorizationHeader)); + ApiKeyThrottleDecision decision = failureLimiter.Check( + partition, + out ApiKeyFailureLimiter.PartitionResolution resolution); + if (decision is ApiKeyThrottleDecision.ThrottledByPeer or ApiKeyThrottleDecision.ThrottledByAggregate) + { + _throttled = true; + metrics.RecordAuthThrottled(decision == ApiKeyThrottleDecision.ThrottledByPeer ? "peer" : "aggregate"); + return AuthenticateResult.Fail("Too many authentication attempts."); + } + + ApiKeyVerification verification = await apiKeyVerifier + .VerifyAsync(authorizationHeader, Context.RequestAborted) + .ConfigureAwait(false); + if (!verification.Succeeded || verification.Identity is null) + { + failureLimiter.RecordFailure(partition); + return AuthenticateResult.Fail("Missing or invalid API key."); + } + + failureLimiter.Reset(partition, resolution); + + List claims = [new Claim(ClaimTypes.Name, verification.Identity.KeyId)]; + foreach (string scope in verification.Identity.Scopes) + { + claims.Add(new Claim(MetricsAuthorizationDefaults.ScopeClaimType, scope)); + } + + ClaimsPrincipal principal = new(new ClaimsIdentity(claims, Scheme.Name)); + return AuthenticateResult.Success(new AuthenticationTicket(principal, Scheme.Name)); + } + + /// + protected override Task HandleChallengeAsync(AuthenticationProperties properties) + { + if (_throttled) + { + Response.StatusCode = StatusCodes.Status429TooManyRequests; + return Task.CompletedTask; + } + + Response.StatusCode = StatusCodes.Status401Unauthorized; + Response.Headers.WWWAuthenticate = "Bearer"; + return Task.CompletedTask; + } + + // No authorization header: fall back to the dashboard login cookie, so an operator already + // signed in to the dashboard can open /metrics in the same browser. + private async Task AuthenticateDashboardCookieAsync() + { + if (await schemeProvider.GetSchemeAsync(DashboardAuthenticationDefaults.AuthenticationScheme).ConfigureAwait(false) + is null) + { + return AuthenticateResult.NoResult(); + } + + AuthenticateResult cookie = await Context + .AuthenticateAsync(DashboardAuthenticationDefaults.AuthenticationScheme) + .ConfigureAwait(false); + return cookie.Succeeded && cookie.Principal is not null + ? AuthenticateResult.Success(new AuthenticationTicket(cookie.Principal, Scheme.Name)) + : AuthenticateResult.NoResult(); + } +} + +/// Requirement for /metrics access; see . +public sealed class MetricsAccessRequirement : IAuthorizationRequirement +{ +} + +/// +/// Grants /metrics to an API key carrying the admin scope or to a dashboard login +/// holding a real dashboard role (Administrator or Viewer). It reads no options, so neither +/// anonymous localhost nor Authentication:Mode=Disabled can satisfy it. +/// +/// +/// A Viewer login qualifies because every dashboard role already sees the same aggregate counters +/// on the dashboard itself; the metrics carry no per-session or per-tag data. An API key needs +/// admin because metrics are an operator surface, not one of the data-plane scopes a client +/// key is normally issued with. +/// +public sealed class MetricsAccessHandler : AuthorizationHandler +{ + /// + protected override Task HandleRequirementAsync( + AuthorizationHandlerContext context, + MetricsAccessRequirement requirement) + { + foreach (ClaimsIdentity identity in context.User.Identities) + { + if (!identity.IsAuthenticated) + { + continue; + } + + // Scope claims count only on an identity the metrics scheme minted from a verified key. + bool isApiKeyIdentity = string.Equals( + identity.AuthenticationType, + MetricsAuthorizationDefaults.AuthenticationScheme, + StringComparison.Ordinal); + if (isApiKeyIdentity + && identity.HasClaim(MetricsAuthorizationDefaults.ScopeClaimType, GatewayScopes.Admin)) + { + context.Succeed(requirement); + return Task.CompletedTask; + } + + if (!isApiKeyIdentity + && (identity.HasClaim(identity.RoleClaimType, DashboardRoles.Admin) + || identity.HasClaim(identity.RoleClaimType, DashboardRoles.Viewer))) + { + context.Succeed(requirement); + return Task.CompletedTask; + } + } + + return Task.CompletedTask; + } +} + +/// Registration for the optional /metrics authentication. +public static class MetricsAuthorizationServiceCollectionExtensions +{ + /// + /// Registers the metrics authentication scheme, the + /// policy and its handler. Registration is unconditional and inert: the policy only takes effect + /// when MxGateway:Metrics:RequireAuthentication attaches it to the endpoint. + /// + /// Service collection to register into. + /// The same service collection, for chaining. + public static IServiceCollection AddGatewayMetricsAuthorization(this IServiceCollection services) + { + services.AddAuthentication() + .AddScheme( + MetricsAuthorizationDefaults.AuthenticationScheme, + _ => { }); + services.AddAuthorizationBuilder() + .AddPolicy( + MetricsAuthorizationDefaults.Policy, + policy => policy + .AddAuthenticationSchemes(MetricsAuthorizationDefaults.AuthenticationScheme) + .AddRequirements(new MetricsAccessRequirement())); + services.AddSingleton(); + + return services; + } +} diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs index 1ebfbad..a7676dc 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs @@ -98,6 +98,70 @@ public sealed class GatewayApplicationTests } } + /// + /// With MxGateway:Metrics:RequireAuthentication=true an anonymous scrape is refused with a + /// plain 401 Bearer challenge (not a login redirect), while /health/active — polled by the + /// off-repo Overview dashboard without credentials — stays anonymous. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Build_WhenMetricsRequireAuthentication_RefusesAnonymousScrapeButNotHealth() + { + await using WebApplication app = GatewayApplication.Build( + ["--urls=http://127.0.0.1:0", "--MxGateway:Metrics:RequireAuthentication=true"]); + await app.StartAsync(); + try + { + using HttpClient client = new() { BaseAddress = new Uri(app.Urls.First()) }; + + using HttpResponseMessage metrics = await client.GetAsync("/metrics"); + using HttpRequestMessage badKeyRequest = new(HttpMethod.Get, "/metrics"); + badKeyRequest.Headers.TryAddWithoutValidation("Authorization", "Bearer mxgw_nobody_not-a-real-secret"); + using HttpResponseMessage badKey = await client.SendAsync(badKeyRequest); + using HttpResponseMessage health = await client.GetAsync("/health/active"); + + Assert.Equal(HttpStatusCode.Unauthorized, metrics.StatusCode); + Assert.Contains(metrics.Headers.WwwAuthenticate, value => value.Scheme == "Bearer"); + Assert.Equal(HttpStatusCode.Unauthorized, badKey.StatusCode); + Assert.NotEqual(HttpStatusCode.Unauthorized, health.StatusCode); + Assert.NotEqual(HttpStatusCode.Forbidden, health.StatusCode); + } + finally + { + await app.StopAsync(); + } + } + + /// + /// The dashboard-login path: with dev-only Dashboard:DisableLogin every request carries + /// the auto-login Administrator principal under the dashboard cookie scheme, which the metrics + /// scheme accepts when no authorization header is sent. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Build_WhenMetricsRequireAuthentication_AcceptsDashboardLogin() + { + await using WebApplication app = GatewayApplication.Build( + [ + "--urls=http://127.0.0.1:0", + "--MxGateway:Metrics:RequireAuthentication=true", + "--MxGateway:Dashboard:DisableLogin=true", + ]); + await app.StartAsync(); + try + { + using HttpClient client = new() { BaseAddress = new Uri(app.Urls.First()) }; + + using HttpResponseMessage response = await client.GetAsync("/metrics"); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + finally + { + await app.StopAsync(); + } + } + /// Verifies that Build maps dashboard and authentication endpoints when the dashboard is enabled. /// A task that represents the asynchronous operation. [Fact] diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs index 76fc2ab..64828fe 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs @@ -1078,7 +1078,6 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests } } - /// An that yields no messages. /// The streamed message type. private sealed class EmptyStreamReader : IAsyncStreamReader diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/MetricsAuthorizationTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/MetricsAuthorizationTests.cs new file mode 100644 index 0000000..8d83767 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/MetricsAuthorizationTests.cs @@ -0,0 +1,249 @@ +using System.Security.Claims; +using System.Text.Encodings.Web; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Auth.Abstractions.ApiKeys; +using ZB.MOM.WW.MxGateway.Server.Dashboard; +using ZB.MOM.WW.MxGateway.Server.Metrics; +using ZB.MOM.WW.MxGateway.Server.Security.Authorization; + +using LibApiKeyIdentity = ZB.MOM.WW.Auth.Abstractions.ApiKeys.ApiKeyIdentity; + +namespace ZB.MOM.WW.MxGateway.Tests.Security.Authorization; + +/// +/// The optional /metrics authentication (MxGateway:Metrics:RequireAuthentication, +/// SEC-14): the scheme that authenticates a scrape and the requirement that authorizes it. +/// +public sealed class MetricsAuthorizationTests +{ + private const string AdminHeader = "Bearer mxgw_scraper_secret"; + + /// A verified API key authenticates and carries its scopes as claims. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Authenticate_WithValidApiKey_SucceedsWithScopeClaims() + { + CountingVerifier verifier = new(Success(GatewayScopes.Admin)); + DefaultHttpContext httpContext = ContextWithAuthorization(AdminHeader); + + AuthenticateResult result = await AuthenticateAsync(verifier, httpContext); + + Assert.True(result.Succeeded); + Assert.Equal("scraper", result.Principal!.Identity!.Name); + Assert.True(result.Principal.HasClaim(MetricsAuthorizationDefaults.ScopeClaimType, GatewayScopes.Admin)); + Assert.Equal(AdminHeader, verifier.LastHeader); + } + + /// An invalid key fails authentication with an opaque reason. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Authenticate_WithInvalidApiKey_Fails() + { + CountingVerifier verifier = new(new ApiKeyVerification(false, null, ApiKeyFailure.SecretMismatch)); + + AuthenticateResult result = await AuthenticateAsync(verifier, ContextWithAuthorization(AdminHeader)); + + Assert.False(result.Succeeded); + Assert.NotNull(result.Failure); + Assert.DoesNotContain("secret", result.Failure!.Message, StringComparison.OrdinalIgnoreCase); + } + + /// + /// Repeated bad keys from one peer trip the shared failure limiter, after which the verifier is + /// no longer reached and the challenge answers 429 — /metrics is not a bypass of the gRPC throttle. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Authenticate_AfterRepeatedFailures_IsThrottledBeforeVerification() + { + CountingVerifier verifier = new(new ApiKeyVerification(false, null, ApiKeyFailure.SecretMismatch)); + ApiKeyFailureLimiter limiter = new( + limit: 2, + TimeSpan.FromMinutes(1), + maxPartitions: 16, + aggregateLimit: 0, + probeInterval: TimeSpan.Zero, + TimeProvider.System); + + for (int attempt = 0; attempt < 2; attempt++) + { + await AuthenticateAsync(verifier, ContextWithAuthorization(AdminHeader), limiter); + } + + DefaultHttpContext throttled = ContextWithAuthorization(AdminHeader); + MetricsAuthenticationHandler handler = await CreateHandlerAsync(verifier, throttled, limiter); + AuthenticateResult result = await handler.AuthenticateAsync(); + await handler.ChallengeAsync(new AuthenticationProperties()); + + Assert.False(result.Succeeded); + Assert.Equal(2, verifier.CallCount); + Assert.Equal(StatusCodes.Status429TooManyRequests, throttled.Response.StatusCode); + } + + /// An anonymous scrape is challenged with a plain 401 Bearer, not a login redirect. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Challenge_WithoutCredentials_Returns401Bearer() + { + DefaultHttpContext httpContext = new(); + MetricsAuthenticationHandler handler = await CreateHandlerAsync( + new CountingVerifier(Success(GatewayScopes.Admin)), + httpContext); + + AuthenticateResult result = await handler.AuthenticateAsync(); + await handler.ChallengeAsync(new AuthenticationProperties()); + + Assert.True(result.None); + Assert.Equal(StatusCodes.Status401Unauthorized, httpContext.Response.StatusCode); + Assert.Equal("Bearer", httpContext.Response.Headers.WWWAuthenticate.ToString()); + } + + /// An API key holding the admin scope satisfies the metrics requirement. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Requirement_ApiKeyWithAdminScope_Succeeds() + { + Assert.True(await EvaluateAsync(ApiKeyPrincipal(GatewayScopes.Admin))); + } + + /// An API key without the admin scope does not satisfy the requirement. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Requirement_ApiKeyWithoutAdminScope_Fails() + { + Assert.False(await EvaluateAsync(ApiKeyPrincipal(GatewayScopes.EventsRead, GatewayScopes.InvokeRead))); + } + + /// A scope claim counts only on an identity the metrics scheme minted from a verified key. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Requirement_ScopeClaimOnForeignIdentity_Fails() + { + ClaimsPrincipal principal = new(new ClaimsIdentity( + [new Claim(MetricsAuthorizationDefaults.ScopeClaimType, GatewayScopes.Admin)], + DashboardAuthenticationDefaults.AuthenticationScheme)); + + Assert.False(await EvaluateAsync(principal)); + } + + /// A dashboard login holding a real dashboard role satisfies the requirement. + /// The dashboard role. + /// A task that represents the asynchronous operation. + [Theory] + [InlineData(DashboardRoles.Admin)] + [InlineData(DashboardRoles.Viewer)] + public async Task Requirement_DashboardLoginWithRole_Succeeds(string role) + { + ClaimsPrincipal principal = new(new ClaimsIdentity( + [new Claim(ClaimTypes.Name, "operator"), new Claim(ClaimTypes.Role, role)], + DashboardAuthenticationDefaults.AuthenticationScheme)); + + Assert.True(await EvaluateAsync(principal)); + } + + /// An unauthenticated principal (anonymous localhost included) never satisfies it. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Requirement_Unauthenticated_Fails() + { + ClaimsPrincipal principal = new(new ClaimsIdentity([new Claim(ClaimTypes.Role, DashboardRoles.Admin)])); + + Assert.False(await EvaluateAsync(principal)); + } + + private static async Task EvaluateAsync(ClaimsPrincipal principal) + { + MetricsAccessRequirement requirement = new(); + AuthorizationHandlerContext context = new([requirement], principal, resource: null); + await new MetricsAccessHandler().HandleAsync(context); + return context.HasSucceeded; + } + + private static ClaimsPrincipal ApiKeyPrincipal(params string[] scopes) + { + List claims = [new Claim(ClaimTypes.Name, "scraper")]; + claims.AddRange(scopes.Select(scope => new Claim(MetricsAuthorizationDefaults.ScopeClaimType, scope))); + return new ClaimsPrincipal(new ClaimsIdentity(claims, MetricsAuthorizationDefaults.AuthenticationScheme)); + } + + private static async Task AuthenticateAsync( + IApiKeyVerifier verifier, + HttpContext httpContext, + ApiKeyFailureLimiter? limiter = null) + { + MetricsAuthenticationHandler handler = await CreateHandlerAsync(verifier, httpContext, limiter); + return await handler.AuthenticateAsync(); + } + + private static async Task CreateHandlerAsync( + IApiKeyVerifier verifier, + HttpContext httpContext, + ApiKeyFailureLimiter? limiter = null) + { + MetricsAuthenticationHandler handler = new( + new StaticOptionsMonitor(), + NullLoggerFactory.Instance, + UrlEncoder.Default, + verifier, + limiter ?? new ApiKeyFailureLimiter(1000, TimeSpan.FromMinutes(1), 1024, 0, TimeSpan.Zero, TimeProvider.System), + new AuthenticationSchemeProvider(Options.Create(new AuthenticationOptions())), + new GatewayMetrics()); + await handler.InitializeAsync( + new AuthenticationScheme( + MetricsAuthorizationDefaults.AuthenticationScheme, + displayName: null, + typeof(MetricsAuthenticationHandler)), + httpContext); + return handler; + } + + private static DefaultHttpContext ContextWithAuthorization(string header) + { + DefaultHttpContext context = new(); + context.Request.Headers.Authorization = header; + context.Connection.RemoteIpAddress = System.Net.IPAddress.Parse("198.51.100.7"); + return context; + } + + private static ApiKeyVerification Success(params string[] scopes) => new( + Succeeded: true, + Identity: new LibApiKeyIdentity( + KeyId: "scraper", + DisplayName: "Scraper", + Scopes: new HashSet(scopes, StringComparer.Ordinal), + Constraints: null), + Failure: null); + + private sealed class CountingVerifier(ApiKeyVerification result) : IApiKeyVerifier + { + /// Gets the number of verifications performed. + public int CallCount { get; private set; } + + /// Gets the last authorization header presented. + public string? LastHeader { get; private set; } + + /// + public Task VerifyAsync(string authorizationHeader, CancellationToken ct) + { + CallCount++; + LastHeader = authorizationHeader; + return Task.FromResult(result); + } + } + + private sealed class StaticOptionsMonitor : IOptionsMonitor + { + /// + public AuthenticationSchemeOptions CurrentValue { get; } = new(); + + /// + public AuthenticationSchemeOptions Get(string? name) => CurrentValue; + + /// + public IDisposable? OnChange(Action listener) => null; + } +} From 82a856b61ed3bc620ec7f17eba1fa3439c0b1d5b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:47:36 -0400 Subject: [PATCH 8/8] fix(metrics-auth): DisableLogin no longer opens /metrics; log correlation pair; host-level tests - The metrics scheme skips its dashboard-cookie fallback while Dashboard:DisableLogin is on, so the dev auto-login handler cannot satisfy RequireAuthentication=true. - Invoke logs client_correlation_id <-> worker envelope id once at Debug. - MetricsEndpointAuthenticationTests drives the real pipeline: anonymous loopback 401 (no /login redirect), Viewer cookie 200, admin key 200, non-admin key 403, DisableLogin 401, /health/active 200. - Docs: DisableLogin behaviour; the /metrics limiter partition is per IP vs gRPC per connection, so only the per-key aggregate is shared. --- docs/DesignDecisions.md | 11 +- docs/GatewayConfiguration.md | 16 +- docs/Grpc.md | 2 +- .../Grpc/MxAccessGatewayService.cs | 13 ++ .../Authorization/MetricsAuthorization.cs | 14 +- .../Gateway/GatewayApplicationTests.cs | 64 ------ .../Grpc/MxAccessGatewayServiceTests.cs | 44 +++++ .../MetricsEndpointAuthenticationTests.cs | 182 ++++++++++++++++++ .../MetricsAuthorizationTests.cs | 4 +- 9 files changed, 276 insertions(+), 74 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Tests/Gateway/MetricsEndpointAuthenticationTests.cs diff --git a/docs/DesignDecisions.md b/docs/DesignDecisions.md index 88d555e..52e70df 100644 --- a/docs/DesignDecisions.md +++ b/docs/DesignDecisions.md @@ -456,10 +456,13 @@ stays anonymous unconditionally. values. - **No environment bypass.** The requirement reads no options, so anonymous localhost and `Authentication:Mode=Disabled` — both of which satisfy the - dashboard Viewer policy — do not satisfy it. Only dev-only - `Dashboard:DisableLogin` (which authenticates every request as an - Administrator, and is rejected in Production) lets an unauthenticated scrape - through. + dashboard Viewer policy — do not satisfy it. Nor does dev-only + `Dashboard:DisableLogin`: its auto-login handler sits under the cookie scheme + name, so the metrics scheme skips the cookie fallback while it is on rather than + letting a dev flag silently open an endpoint the operator asked to protect. +- **Throttle sharing is partial.** The `/metrics` per-peer limiter partition is + the remote IP, while the gRPC one is the gRPC peer string (per connection), so + only the per-key-id aggregate is shared between the two paths. - **One scheme, plain 401.** Listing the cookie scheme on the policy directly would challenge an anonymous scrape with a 302 redirect to `/login`; a single scheme that falls back to the cookie internally keeps the challenge a `401` diff --git a/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index 8323351..06cdd47 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -429,7 +429,7 @@ another repo without credentials, so gating it would break that consumer. | Option | Default | Description | |--------|---------|-------------| -| `MxGateway:Metrics:RequireAuthentication` | `false` | When `true`, `/metrics` requires an authenticated caller (SEC-14). A scrape is accepted when it presents an API key carrying the `admin` scope as `Authorization: Bearer mxgw__`, or — with no authorization header — a dashboard login cookie holding a real dashboard role (`Administrator` or `Viewer`). Anonymous localhost access and `Authentication:Mode=Disabled` do **not** satisfy it. An anonymous scrape gets `401` with `WWW-Authenticate: Bearer`; an authenticated caller without `admin` scope or a dashboard role gets `403`; a peer throttled by the API-key failure limiter (`MxGateway:Security:ApiKeyFailure*`, shared with the gRPC path) gets `429`. The default keeps the endpoint anonymous so existing scrapers keep working. | +| `MxGateway:Metrics:RequireAuthentication` | `false` | When `true`, `/metrics` requires an authenticated caller (SEC-14). A scrape is accepted when it presents an API key carrying the `admin` scope as `Authorization: Bearer mxgw__`, or — with no authorization header — a dashboard login cookie holding a real dashboard role (`Administrator` or `Viewer`). Anonymous localhost access, `Authentication:Mode=Disabled`, and `Dashboard:DisableLogin` do **not** satisfy it. An anonymous scrape gets `401` with `WWW-Authenticate: Bearer`; an authenticated caller without `admin` scope or a dashboard role gets `403`; a peer throttled by the API-key failure limiter (`MxGateway:Security:ApiKeyFailure*`, shared with the gRPC path) gets `429`. The default keeps the endpoint anonymous so existing scrapers keep working. | Configure a Prometheus scraper against an authenticated endpoint with a dedicated admin-scoped key, for example: @@ -445,8 +445,18 @@ scrape_configs: ``` The setting is read from configuration when endpoints are mapped, so changing it -requires a restart. With dev-only `Dashboard:DisableLogin=true` every request is -auto-authenticated as an Administrator, which also satisfies `/metrics`. +requires a restart. Dev-only `Dashboard:DisableLogin=true` registers an +always-succeeding auto-login handler under the dashboard cookie scheme; the +metrics scheme deliberately skips its cookie fallback while that flag is on, so +`DisableLogin` never silently opens a `/metrics` endpoint that +`RequireAuthentication` protects — only an admin-scoped API key is accepted then. + +The `/metrics` path reuses the gRPC API-key failure limiter +(`MxGateway:Security:ApiKeyFailure*`), but its per-peer partition is keyed on the +remote **IP address**, whereas the gRPC interceptor's is the gRPC peer string +(address and port — effectively per connection). The two paths therefore share +only the per-key-id aggregate layer (`ApiKeyFailureAggregateLimit`); per-peer +failure counts on one path do not throttle the other. ## Galaxy Options diff --git a/docs/Grpc.md b/docs/Grpc.md index dd1c7d9..c43508e 100644 --- a/docs/Grpc.md +++ b/docs/Grpc.md @@ -82,7 +82,7 @@ return mapper.MapCommandReply(workerReply); Carrying the enqueue timestamp into the worker layer is what lets queue-wait time be measured separately from worker-side execution time when troubleshooting timeouts. -Every `MxCommandReply` echoes the request's `client_correlation_id` in `correlation_id` — success, MXAccess failure, and constraint-denied replies alike, and an empty string when the caller sent none. The handler stamps it after mapping the worker reply, overwriting the internal gateway↔worker envelope id the worker reply carries: that id is a pipe-routing detail with no meaning to a client, and clients that sent no correlation id previously received it while clients that sent one received it only on denied replies. +Every `MxCommandReply` echoes the request's `client_correlation_id` in `correlation_id` — success, MXAccess failure, and constraint-denied replies alike, and an empty string when the caller sent none. The handler stamps it after mapping the worker reply, overwriting the internal gateway↔worker envelope id the worker reply carries: that id is a pipe-routing detail with no meaning to a client, and clients that sent no correlation id previously received it while clients that sent one received it only on denied replies. To keep the two ids joinable, `Invoke` logs the pair (command kind, session id, client correlation id, worker envelope correlation id — no values) once per call at `Debug`, so a client error report quoting its correlation id can be matched to gateway and worker log lines. An accepted gRPC command payload can still be too large for the worker pipe: the envelope built around it must fit `MxGateway:Worker:MaxMessageBytes`, which is validated at startup to sit at least a fixed envelope-overhead reserve above `MaxGrpcMessageBytes` (see the headroom rule in [Gateway Configuration](./GatewayConfiguration.md)) so this should not occur for a conformant payload, but if it does, `WorkerClient` raises `WorkerClientException(CommandTooLarge)` and `Invoke` reports `ResourceExhausted` for that command — the session is not faulted, so a client can retry with a smaller payload without reopening the session. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs index 201d3e0..826014c 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Grpc/MxAccessGatewayService.cs @@ -148,6 +148,19 @@ public sealed class MxAccessGatewayService( // the same value the constraint-denied replies carry — so a client can match every // reply to its request. The worker reply carries the gateway's internal envelope id, // which is a gateway↔worker routing detail and is overwritten here, empty included. + // Record the pairing once, before the envelope id is overwritten, so a client error + // report quoting its correlation id can be matched to gateway and worker log lines + // (which carry the envelope id). Ids and the command kind only — never values. + if (logger.IsEnabled(LogLevel.Debug)) + { + logger.LogDebug( + "Invoke {CommandKind} for session {SessionId}: client correlation id {ClientCorrelationId} maps to worker envelope correlation id {WorkerCorrelationId}.", + command.Kind, + request.SessionId, + request.ClientCorrelationId, + publicReply.CorrelationId); + } + publicReply.CorrelationId = request.ClientCorrelationId; session.TrackCommandReply(commandToInvoke, publicReply); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/MetricsAuthorization.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/MetricsAuthorization.cs index 4779f16..d9ed334 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/MetricsAuthorization.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/MetricsAuthorization.cs @@ -4,6 +4,7 @@ using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authorization; using Microsoft.Extensions.Options; using ZB.MOM.WW.Auth.Abstractions.ApiKeys; +using ZB.MOM.WW.MxGateway.Server.Configuration; using ZB.MOM.WW.MxGateway.Server.Dashboard; using ZB.MOM.WW.MxGateway.Server.Metrics; @@ -34,7 +35,9 @@ public static class MetricsAuthorizationDefaults /// cookie scheme on the policy directly would turn an anonymous scrape into a 302 redirect to the /// login page. A throttled key partition answers 429, mirroring the gRPC /// ResourceExhausted. Failure reasons are never echoed to the caller and the presented -/// secret is never logged. +/// secret is never logged. The limiter's per-peer partition here is the remote IP, whereas the gRPC +/// interceptor's is the gRPC peer string (address and port, i.e. per connection), so the two paths +/// share only the per-key-id aggregate layer. /// public sealed class MetricsAuthenticationHandler( IOptionsMonitor options, @@ -43,6 +46,7 @@ public sealed class MetricsAuthenticationHandler( IApiKeyVerifier apiKeyVerifier, ApiKeyFailureLimiter failureLimiter, IAuthenticationSchemeProvider schemeProvider, + IOptions gatewayOptions, GatewayMetrics metrics) : AuthenticationHandler(options, logger, encoder) { @@ -111,6 +115,14 @@ public sealed class MetricsAuthenticationHandler( // signed in to the dashboard can open /metrics in the same browser. private async Task AuthenticateDashboardCookieAsync() { + // Dashboard:DisableLogin registers an always-succeeding auto-login handler under the cookie + // scheme name. Falling back to it would let every anonymous request satisfy /metrics, so + // with DisableLogin on, RequireAuthentication=true accepts API keys only. + if (gatewayOptions.Value.Dashboard.DisableLogin) + { + return AuthenticateResult.NoResult(); + } + if (await schemeProvider.GetSchemeAsync(DashboardAuthenticationDefaults.AuthenticationScheme).ConfigureAwait(false) is null) { diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs index a7676dc..1ebfbad 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/GatewayApplicationTests.cs @@ -98,70 +98,6 @@ public sealed class GatewayApplicationTests } } - /// - /// With MxGateway:Metrics:RequireAuthentication=true an anonymous scrape is refused with a - /// plain 401 Bearer challenge (not a login redirect), while /health/active — polled by the - /// off-repo Overview dashboard without credentials — stays anonymous. - /// - /// A task that represents the asynchronous operation. - [Fact] - public async Task Build_WhenMetricsRequireAuthentication_RefusesAnonymousScrapeButNotHealth() - { - await using WebApplication app = GatewayApplication.Build( - ["--urls=http://127.0.0.1:0", "--MxGateway:Metrics:RequireAuthentication=true"]); - await app.StartAsync(); - try - { - using HttpClient client = new() { BaseAddress = new Uri(app.Urls.First()) }; - - using HttpResponseMessage metrics = await client.GetAsync("/metrics"); - using HttpRequestMessage badKeyRequest = new(HttpMethod.Get, "/metrics"); - badKeyRequest.Headers.TryAddWithoutValidation("Authorization", "Bearer mxgw_nobody_not-a-real-secret"); - using HttpResponseMessage badKey = await client.SendAsync(badKeyRequest); - using HttpResponseMessage health = await client.GetAsync("/health/active"); - - Assert.Equal(HttpStatusCode.Unauthorized, metrics.StatusCode); - Assert.Contains(metrics.Headers.WwwAuthenticate, value => value.Scheme == "Bearer"); - Assert.Equal(HttpStatusCode.Unauthorized, badKey.StatusCode); - Assert.NotEqual(HttpStatusCode.Unauthorized, health.StatusCode); - Assert.NotEqual(HttpStatusCode.Forbidden, health.StatusCode); - } - finally - { - await app.StopAsync(); - } - } - - /// - /// The dashboard-login path: with dev-only Dashboard:DisableLogin every request carries - /// the auto-login Administrator principal under the dashboard cookie scheme, which the metrics - /// scheme accepts when no authorization header is sent. - /// - /// A task that represents the asynchronous operation. - [Fact] - public async Task Build_WhenMetricsRequireAuthentication_AcceptsDashboardLogin() - { - await using WebApplication app = GatewayApplication.Build( - [ - "--urls=http://127.0.0.1:0", - "--MxGateway:Metrics:RequireAuthentication=true", - "--MxGateway:Dashboard:DisableLogin=true", - ]); - await app.StartAsync(); - try - { - using HttpClient client = new() { BaseAddress = new Uri(app.Urls.First()) }; - - using HttpResponseMessage response = await client.GetAsync("/metrics"); - - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - } - finally - { - await app.StopAsync(); - } - } - /// Verifies that Build maps dashboard and authentication endpoints when the dashboard is enabled. /// A task that represents the asynchronous operation. [Fact] diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs index ce4cb9d..8ee15aa 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs @@ -505,6 +505,50 @@ public sealed class MxAccessGatewayServiceTests Assert.Equal(string.Empty, reply.CorrelationId); } + /// + /// Invoke logs the client correlation id alongside the worker envelope id once, at Debug, so + /// a client error report can be matched to gateway and worker logs. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Invoke_LogsClientToWorkerCorrelationPairAtDebug() + { + FakeSessionManager sessionManager = new() + { + InvokeReply = new WorkerCommandReply + { + Reply = new MxCommandReply + { + SessionId = "session-1", + CorrelationId = "envelope-7f3a", + Kind = MxCommandKind.AddItem, + ProtocolStatus = MxAccessGrpcMapper.Ok(), + AddItem = new AddItemReply { ItemHandle = 42 }, + }, + }, + }; + RecordingLogger logger = new(); + MxAccessGatewayService service = CreateService(sessionManager, logger: logger); + MxCommandRequest request = new() + { + SessionId = "session-1", + ClientCorrelationId = "client-trace-9", + Command = new MxCommand + { + Kind = MxCommandKind.AddItem, + AddItem = new AddItemCommand { ServerHandle = 12, ItemDefinition = "Galaxy.Tag.Value" }, + }, + }; + + await service.Invoke(request, new TestServerCallContext()); + + (LogLevel level, string message, _) = Assert.Single( + logger.Entries, + entry => entry.Message.Contains("envelope-7f3a", StringComparison.Ordinal)); + Assert.Equal(LogLevel.Debug, level); + Assert.Contains("client-trace-9", message, StringComparison.Ordinal); + } + /// /// StreamEvents hands the call's cancellation token to every response write, so a write /// blocked on a slow or vanished client is released when the call is cancelled. diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Gateway/MetricsEndpointAuthenticationTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/MetricsEndpointAuthenticationTests.cs new file mode 100644 index 0000000..662c576 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/Gateway/MetricsEndpointAuthenticationTests.cs @@ -0,0 +1,182 @@ +using System.Net; +using System.Net.Http; +using System.Security.Claims; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Auth.ApiKeys.Admin; +using ZB.MOM.WW.MxGateway.Server; +using ZB.MOM.WW.MxGateway.Server.Dashboard; +using ZB.MOM.WW.MxGateway.Server.Security.Authorization; + +namespace ZB.MOM.WW.MxGateway.Tests.Gateway; + +/// +/// Host-level coverage of MxGateway:Metrics:RequireAuthentication=true (SEC-14): each case +/// builds the real gateway with and drives the full +/// middleware pipeline (authentication, authorization, the dashboard cookie scheme, the API-key +/// store) over HTTP on an ephemeral loopback port. Requests therefore arrive from loopback, which +/// is exactly the anonymous-localhost case the dashboard Viewer policy would admit and /metrics +/// must not. +/// +public sealed class MetricsEndpointAuthenticationTests +{ + // A fixed test pepper: the configuration-backed pepper provider reads MxGateway:ApiKeyPepper, + // and the auth store itself is isolated per test process by TestHostEnvironmentInitializer. + private const string TestPepper = "metrics-endpoint-tests-pepper-0123456789abcdef"; + + /// + /// An anonymous loopback scrape gets a plain 401 Bearer challenge — not a 302 to /login, and not + /// the anonymous-localhost admission the dashboard Viewer policy grants. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task AnonymousLoopbackScrape_Returns401BearerNotLoginRedirect() + { + await using WebApplication app = await StartAsync(); + using HttpClient client = CreateClient(app); + + using HttpResponseMessage response = await client.GetAsync("/metrics"); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + Assert.Null(response.Headers.Location); + Assert.Contains(response.Headers.WwwAuthenticate, value => value.Scheme == "Bearer"); + } + + /// /health/active stays anonymous: the off-repo Overview dashboard polls it without credentials. + /// A task that represents the asynchronous operation. + [Fact] + public async Task HealthActive_StaysAnonymous() + { + await using WebApplication app = await StartAsync(); + using HttpClient client = CreateClient(app); + + using HttpResponseMessage response = await client.GetAsync("/health/active"); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + /// A dashboard login cookie holding the Viewer role is accepted. + /// A task that represents the asynchronous operation. + [Fact] + public async Task DashboardViewerCookie_Returns200() + { + await using WebApplication app = await StartAsync(); + using HttpClient client = CreateClient(app); + using HttpRequestMessage request = new(HttpMethod.Get, "/metrics"); + request.Headers.Add("Cookie", MintDashboardCookie(app, DashboardRoles.Viewer)); + + using HttpResponseMessage response = await client.SendAsync(request); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + /// A real API key with the admin scope is accepted. + /// A task that represents the asynchronous operation. + [Fact] + public async Task AdminScopedApiKey_Returns200() + { + await using WebApplication app = await StartAsync(); + string token = await CreateKeyAsync(app, [GatewayScopes.Admin]); + using HttpClient client = CreateClient(app); + + using HttpResponseMessage response = await SendWithBearerAsync(client, token); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + /// A real API key without the admin scope authenticates but is forbidden. + /// A task that represents the asynchronous operation. + [Fact] + public async Task NonAdminApiKey_Returns403() + { + await using WebApplication app = await StartAsync(); + string token = await CreateKeyAsync(app, [GatewayScopes.EventsRead, GatewayScopes.InvokeRead]); + using HttpClient client = CreateClient(app); + + using HttpResponseMessage response = await SendWithBearerAsync(client, token); + + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + } + + /// + /// Dev-only Dashboard:DisableLogin registers an always-succeeding auto-login handler under + /// the cookie scheme name; /metrics must not fall back to it, or the flag would silently open + /// an endpoint the operator explicitly asked to protect. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task DisableLogin_DoesNotOpenMetrics() + { + await using WebApplication app = await StartAsync("--MxGateway:Dashboard:DisableLogin=true"); + using HttpClient client = CreateClient(app); + + using HttpResponseMessage response = await client.GetAsync("/metrics"); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + } + + private static async Task StartAsync(params string[] extraArgs) + { + // Ephemeral port (:0): xUnit runs collections in parallel, so a fixed port would collide. + WebApplication app = GatewayApplication.Build( + [ + "--urls=http://127.0.0.1:0", + "--MxGateway:Metrics:RequireAuthentication=true", + $"--MxGateway:ApiKeyPepper={TestPepper}", + .. extraArgs, + ]); + await app.StartAsync(); + return app; + } + + private static HttpClient CreateClient(WebApplication app) => + new(new HttpClientHandler { AllowAutoRedirect = false, UseCookies = false }) + { + BaseAddress = new Uri(app.Urls.First()), + }; + + private static async Task SendWithBearerAsync(HttpClient client, string token) + { + using HttpRequestMessage request = new(HttpMethod.Get, "/metrics"); + request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token}"); + return await client.SendAsync(request); + } + + // Creates a key through the same admin command set the CLI and dashboard use. Key ids are unique + // per call because the isolated auth store is shared by every test in the process. + private static async Task CreateKeyAsync(WebApplication app, string[] scopes) + { + ApiKeyAdminCommands commands = app.Services.GetRequiredService(); + await commands.InitDbAsync(remoteAddress: null, CancellationToken.None); + CreateKeyResult created = await commands.CreateKeyAsync( + $"metrics{Guid.NewGuid():N}", + "Metrics test key", + new HashSet(scopes, StringComparer.Ordinal), + null, + null, + remoteAddress: null, + CancellationToken.None); + return created.Token ?? throw new InvalidOperationException("Key creation returned no token."); + } + + // Protects a ticket with the running host's own cookie options (data-protection keys, ticket + // format, cookie name) — the cookie a real dashboard login would issue, minus the LDAP bind. + private static string MintDashboardCookie(WebApplication app, string role) + { + CookieAuthenticationOptions cookieOptions = app.Services + .GetRequiredService>() + .Get(DashboardAuthenticationDefaults.AuthenticationScheme); + ClaimsIdentity identity = new( + [new Claim(ClaimTypes.Name, "viewer-user"), new Claim(ClaimTypes.Role, role)], + DashboardAuthenticationDefaults.AuthenticationScheme, + ClaimTypes.Name, + ClaimTypes.Role); + AuthenticationTicket ticket = new( + new ClaimsPrincipal(identity), + DashboardAuthenticationDefaults.AuthenticationScheme); + return $"{cookieOptions.Cookie.Name}={cookieOptions.TicketDataFormat.Protect(ticket)}"; + } +} diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/MetricsAuthorizationTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/MetricsAuthorizationTests.cs index 8d83767..6694744 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/MetricsAuthorizationTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/MetricsAuthorizationTests.cs @@ -6,6 +6,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using ZB.MOM.WW.Auth.Abstractions.ApiKeys; +using ZB.MOM.WW.MxGateway.Server.Configuration; using ZB.MOM.WW.MxGateway.Server.Dashboard; using ZB.MOM.WW.MxGateway.Server.Metrics; using ZB.MOM.WW.MxGateway.Server.Security.Authorization; @@ -190,7 +191,8 @@ public sealed class MetricsAuthorizationTests UrlEncoder.Default, verifier, limiter ?? new ApiKeyFailureLimiter(1000, TimeSpan.FromMinutes(1), 1024, 0, TimeSpan.Zero, TimeProvider.System), - new AuthenticationSchemeProvider(Options.Create(new AuthenticationOptions())), + new AuthenticationSchemeProvider(Options.Create(new Microsoft.AspNetCore.Authentication.AuthenticationOptions())), + Options.Create(new GatewayOptions()), new GatewayMetrics()); await handler.InitializeAsync( new AuthenticationScheme(