fix(secrets): Grpc hub server review follow-ups — GetSecrets batch cap, DI single-registration guard + tests

Five review items on the Task 2 server half. 1. GetSecrets now enforces a server-side cap on request.Names, refusing an over-cap batch with ResourceExhausted BEFORE any store read runs. The well-behaved follower chunks at 500, but client discipline is not a server defence — the family precedent (HistorianGateway's max_values) is that the server declines regardless. The new SecretsGrpcHubServerOptions. MaxNamesPerRequest defaults to 1000, comfortably above that chunk so a compliant sweep never meets it, and Validate() refuses a non-positive value at startup rather than letting it read at runtime as a broken follower. The refusal states the cap and the received count — neither is secret, and an operator cannot resize a batch without them — and never a name. The cap sits behind the interceptor, so it is defence in depth against a misbehaving token holder, not an anonymous-DoS control. 2. AddZbSecretsGrpcHub now refuses a second registration. The two calls do not compose: options binding MERGES (a key absent from the second section leaves the first section's value standing) while the SectionPath stamp is last-one-wins, so a hub registered twice could serve a token bound from one section while every failure message named the other. Detected via a private marker registration. The extension also gains real DI coverage — the intended single-call path asserted for token/SectionPath coherence through a built provider, a custom section path stamped end to end, a missing token throwing from the registration call itself, and the duplicate call throwing. 3. Client-streaming and duplex-streaming denial tests, so all four gated handler shapes are pinned rather than only unary and server-streaming, plus a valid-token test that reaches all three streaming continuations. 4. A cancellation test: GetSecrets hands the ServerCallContext token through to the store, pinned against a fake that records it. 5. The proto's service comment stated the ISecretReplicationHub mapping in prose that no longer matched the RPC name; it now says GetManifestAsync -> GetManifest and GetManyAsync -> GetSecrets explicitly. Comment-only — no message or rpc definition is touched. The GetSecrets rpc comment also notes the new RESOURCE_EXHAUSTED cap, which a client author has to know about. Grpc server suite 50 -> 73 tests; full solution suite green, 0 new warnings. Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1
2026-08-07 06:27:16 -04:00
parent 7fc77f2790
commit 353af72265
7 changed files with 531 additions and 15 deletions
@@ -37,7 +37,9 @@ public static class SecretsGrpcHubServerExtensions
    /// <param name="configuration">The application configuration.</param>
    /// <param name="sectionPath">Configuration section holding the hub options.</param>
    /// <returns>The same <paramref name="services"/> instance, for chaining.</returns>
-    /// <exception cref="InvalidOperationException">No bearer token is configured.</exception>
+    /// <exception cref="InvalidOperationException">
+    /// No bearer token is configured, or the hub has already been registered.
+    /// </exception>
    public static IServiceCollection AddZbSecretsGrpcHub(
        this IServiceCollection services,
        IConfiguration configuration,
@@ -47,6 +49,24 @@ public static class SecretsGrpcHubServerExtensions
        ArgumentNullException.ThrowIfNull(configuration);
        ArgumentException.ThrowIfNullOrWhiteSpace(sectionPath);

+        // Registering twice is refused rather than tolerated, because the two calls do NOT compose:
+        // options binding MERGES (a key absent from the second section leaves the first section's
+        // value standing) while the SectionPath stamp below is last-one-wins. A hub registered from
+        // two sections could therefore serve a token bound from one of them while every failure
+        // message names the other — a misdirection that outlives the mistake that caused it.
+        if (services.Any(descriptor => descriptor.ServiceType == typeof(HubRegistrationMarker)))
+        {
+            throw new InvalidOperationException(
+                "The gRPC secrets hub may only be registered once; AddZbSecretsGrpcHub was called " +
+                "more than once. Remove the duplicate call — a second registration would merge its " +
+                "configuration into the first and leave the reported section path pointing at " +
+                "whichever call ran last.");
+        }
+
+        // Registered as an instance so nothing ever has to activate it: the marker exists to be
+        // looked for, not resolved.
+        services.AddSingleton(new HubRegistrationMarker());
+
        IConfigurationSection section = configuration.GetSection(sectionPath);
        services.Configure<SecretsGrpcHubServerOptions>(section);

@@ -84,4 +104,12 @@ public static class SecretsGrpcHubServerExtensions
        endpoints.MapGrpcService<SecretsHubService>();
        return endpoints;
    }
+
+    /// <summary>
+    /// Presence of this in the container means the hub is already registered. It carries no state
+    /// and is never resolved — a dedicated type is used rather than probing for one of the real
+    /// registrations because every one of those is added with <c>TryAdd</c>-style semantics that a
+    /// host may legitimately have satisfied for its own reasons.
+    /// </summary>
+    private sealed class HubRegistrationMarker;
 }
@@ -13,11 +13,13 @@ option csharp_namespace = "ZB.MOM.WW.Secrets.Replicator.Grpc.V1";
 // The central-hosted secrets hub, as seen by a site node.
 //
 // PULL-ONLY BY CONSTRUCTION. This service mirrors exactly the read half of
-// ISecretReplicationHub — GetManifestAsync and GetManyAsync. Its third method,
-// ApplyReplicatedAsync, is deliberately NOT mirrored: in this topology every update
-// originates at central, and a site node is a strict follower. The narrow contract IS the
-// enforcement — with no write RPC on the wire, a mis-wired or compromised site node has no
-// way to push a row into the hub, regardless of what its authorization happens to allow.
+// ISecretReplicationHub, method for method: GetManifestAsync -> GetManifest, and
+// GetManyAsync -> GetSecrets (the RPC is named for what it returns rather than for the
+// .NET method it fronts). Its third method, ApplyReplicatedAsync, is deliberately NOT
+// mirrored: in this topology every update originates at central, and a site node is a
+// strict follower. The narrow contract IS the enforcement — with no write RPC on the wire,
+// a mis-wired or compromised site node has no way to push a row into the hub, regardless of
+// what its authorization happens to allow.
 // Adding a third RPC to this service would silently retire that guarantee, so do not.
 //
 // Everything crossing this boundary is ciphertext or metadata about ciphertext. The
@@ -33,6 +35,9 @@ service SecretsHub {
  // Fetches the named encrypted rows in bulk. Names absent from the hub are omitted from
  // the response rather than reported as errors — the caller's manifest may be a moment
  // stale, and a row deleted between the two calls is not a fault.
+  //
+  // The server caps how many names one call may carry and answers RESOURCE_EXHAUSTED above
+  // it, so a caller must chunk large sweeps rather than send one request per manifest.
  rpc GetSecrets(GetSecretsRequest) returns (GetSecretsResponse);
 }

@@ -22,12 +22,30 @@ public sealed class SecretsGrpcHubServerOptions
    /// <summary>The configuration section the hub is bound from unless a host overrides it.</summary>
    internal const string DefaultSectionPath = "Secrets:GrpcHub";

+    /// <summary>
+    /// The default ceiling on names in one <c>GetSecrets</c> call. Set comfortably above the
+    /// follower's own fetch chunk so a compliant sweep never approaches it.
+    /// </summary>
+    internal const int DefaultMaxNamesPerRequest = 1000;
+
    /// <summary>
    /// The shared bearer token a follower must present. Required — an unset token is not "open
    /// access", it is a hub that refuses every call (see <see cref="SecretsHubAuthInterceptor"/>).
    /// </summary>
    public string BearerToken { get; set; } = string.Empty;

+    /// <summary>
+    /// The most names a single <c>GetSecrets</c> call may carry. A larger request is refused with
+    /// <c>ResourceExhausted</c> rather than served.
+    /// </summary>
+    /// <remarks>
+    /// The well-behaved follower already chunks its fetches well below this, so the cap is not a
+    /// throughput knob — it is the server declining to let one authenticated caller turn a single
+    /// request into an unbounded sequential store sweep. The cap sits behind authentication, so it
+    /// is defence in depth against a misbehaving token holder, not an anonymous-DoS control.
+    /// </remarks>
+    public int MaxNamesPerRequest { get; set; } = DefaultMaxNamesPerRequest;
+
    /// <summary>
    /// The configuration section this instance was bound from, used so failure messages name the
    /// key an operator actually has to set.
@@ -43,7 +61,9 @@ public sealed class SecretsGrpcHubServerOptions
    /// Throws if the options are unusable. Called by the registration extension so a hub with no
    /// token fails at startup, rather than starting and rejecting every follower at runtime.
    /// </summary>
-    /// <exception cref="InvalidOperationException">The bearer token is missing or blank.</exception>
+    /// <exception cref="InvalidOperationException">
+    /// The bearer token is missing or blank, or the request cap is not positive.
+    /// </exception>
    public void Validate()
    {
        if (string.IsNullOrWhiteSpace(BearerToken))
@@ -54,5 +74,14 @@ public sealed class SecretsGrpcHubServerOptions
                $"The gRPC secrets hub requires a shared bearer token ({SectionPath}:BearerToken). " +
                "Until it is set the hub refuses every call.");
        }
+
+        if (MaxNamesPerRequest <= 0)
+        {
+            // A zero or negative cap would refuse every GetSecrets call, which reads at runtime like
+            // a broken follower rather than a typo in configuration — so it is refused at startup.
+            throw new InvalidOperationException(
+                $"The gRPC secrets hub request cap ({SectionPath}:MaxNamesPerRequest) must be " +
+                $"greater than zero; it is {MaxNamesPerRequest}.");
+        }
    }
 }
@@ -1,5 +1,6 @@
 using Grpc.Core;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
 using ZB.MOM.WW.Secrets.Abstractions;
 using ZB.MOM.WW.Secrets.Replicator.Grpc.V1;
 using ZB.MOM.WW.Secrets.Replicator.Grpc.Wire;
@@ -27,14 +28,30 @@ public sealed class SecretsHubService : SecretsHub.SecretsHubBase
 {
    private readonly ISecretStore _store;
    private readonly ILogger<SecretsHubService> _logger;
+    private readonly int _maxNamesPerRequest;

    /// <summary>Creates the hub service over the node's secret store.</summary>
    /// <param name="store">The store to serve. Only its read members are used.</param>
+    /// <param name="options">The hub options, read for the per-request name cap.</param>
    /// <param name="logger">Diagnostics sink.</param>
-    public SecretsHubService(ISecretStore store, ILogger<SecretsHubService> logger)
+    public SecretsHubService(
+        ISecretStore store,
+        IOptions<SecretsGrpcHubServerOptions> options,
+        ILogger<SecretsHubService> logger)
    {
+        ArgumentNullException.ThrowIfNull(options);
+
        _store = store ?? throw new ArgumentNullException(nameof(store));
        _logger = logger ?? throw new ArgumentNullException(nameof(logger));
+
+        // The registration extension validates the cap, so a non-positive value here means the
+        // options were built some other way. Falling back to the default is the safe direction: the
+        // alternative reading of "0" is a hub with no cap at all, which is the thing the cap exists
+        // to prevent.
+        int configured = options.Value.MaxNamesPerRequest;
+        _maxNamesPerRequest = configured > 0
+            ? configured
+            : SecretsGrpcHubServerOptions.DefaultMaxNamesPerRequest;
    }

    /// <summary>
@@ -66,21 +83,47 @@ public sealed class SecretsHubService : SecretsHub.SecretsHubBase
    /// Fetches the named encrypted rows in bulk.
    /// </summary>
    /// <remarks>
+    /// <para>
    /// A requested name the hub does not hold is <b>omitted</b> from the response, and an unparseable
    /// name is <b>skipped</b> — neither is an error. Both describe the same ordinary situation: the
    /// follower is acting on a manifest that was true a moment ago. Failing the whole call would turn
    /// a routine race into a stalled sweep, and would let one bad name deny the follower every other
    /// row in the batch.
+    /// </para>
+    /// <para>
+    /// A request carrying more than <see cref="SecretsGrpcHubServerOptions.MaxNamesPerRequest"/>
+    /// names <b>is</b> refused, before any read runs. That is not the stale-manifest case: it is a
+    /// caller asking the hub to walk its store an unbounded number of times inside one call, and the
+    /// server declines independently of whatever chunking the client believes it is doing.
+    /// </para>
    /// </remarks>
    /// <param name="request">The names to fetch.</param>
    /// <param name="context">The call context.</param>
    /// <returns>One row per distinct name the hub holds.</returns>
+    /// <exception cref="RpcException">The request carries more names than the configured cap.</exception>
    public override async Task<GetSecretsResponse> GetSecrets(
        GetSecretsRequest request, ServerCallContext context)
    {
        ArgumentNullException.ThrowIfNull(request);
        ArgumentNullException.ThrowIfNull(context);

+        if (request.Names.Count > _maxNamesPerRequest)
+        {
+            // The cap and the count are both safe to state — neither is secret, and an operator
+            // cannot fix a batch that is too large without knowing by how much. The names are NOT
+            // stated: they are the one part of this request that identifies secrets.
+            _logger.LogWarning(
+                "Secrets hub refused a GetSecrets request of {RequestedCount} name(s); the cap is " +
+                "{MaxNamesPerRequest}.",
+                request.Names.Count,
+                _maxNamesPerRequest);
+
+            throw new RpcException(new Status(
+                StatusCode.ResourceExhausted,
+                $"GetSecrets accepts at most {_maxNamesPerRequest} names per request; this one " +
+                $"carried {request.Names.Count}. Split the fetch into smaller batches."));
+        }
+
        var response = new GetSecretsResponse();
        var seen = new HashSet<string>(StringComparer.Ordinal);
        int skipped = 0;