From c5ea3a7b59f747aae9e611e94f860c2dd902ca02 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:12:03 -0400 Subject: [PATCH 1/2] =?UTF-8?q?feat(secrets):=20Grpc=20hub=20client=20?= =?UTF-8?q?=E2=80=94=20pull-only=20sweep=20service=20+=20options=20+=20DI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Client half of the central-hosted secrets hub: a site node follows central and never pushes. - `ISecretsHubReader` + `GrpcSecretsHubClient` — the read half of the hub as a follower sees it (manifest + bulk fetch, nothing else). Bearer credential and a per-call deadline are attached per call; responses convert through `WireToDomain`. Faults are deliberately not caught here, so an unreachable hub can never be mistaken for an empty one. - `WireToDomain` — strict, per-row proto -> domain parse mirroring the Akka replicator's `SecretRowDto`: a name the domain would reject, a content type naming no enum member, or a non-round-trip timestamp is refused at the boundary. Explicit proto3 presence keeps null distinguishable from empty. - `GrpcSecretSyncService` — `SqlServerSecretSyncService` with the push half removed. One `ReconcileAsync` per tick (pull-only by the reconciler's own contract, and batched, so a cold-start pull set cannot exceed gRPC's 4 MB cap), never throws out of `ExecuteAsync`, warns once and retries on the next interval. The local store is left completely undecorated — the sweep is its only hub-ward writer. - `SecretsGrpcHubClientOptions` — fail-closed `Validate()` naming the exact configuration key, including the section a caller actually bound. The bearer token is never echoed by any message. - `AddZbSecretsGrpcHubClient` — binds, eagerly validates, and registers the channel under a service KEY so it cannot lose a `TryAdd` race to an application's own `GrpcChannel` and dial the wrong endpoint with the hub credential attached. Tests 52/52 (red-first), including a source-scan guard pinning that no client file names `ComputePushSet`, `ReadLocalAsync`, or `ApplyReplicatedAsync`, and a reflection pin that the reader seam exposes exactly two read operations. Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1 --- .../SecretsGrpcHubClientExtensions.cs | 94 ++++ .../GrpcSecretSyncService.cs | 148 +++++ .../GrpcSecretsHubClient.cs | 111 ++++ .../SecretsGrpcHubClientOptions.cs | 112 ++++ .../Wire/WireToDomain.cs | 123 +++++ .../GrpcSecretSyncServiceTests.cs | 504 ++++++++++++++++++ .../SecretsGrpcHubClientOptionsTests.cs | 132 +++++ .../WireToDomainTests.cs | 205 +++++++ 8 files changed, 1429 insertions(+) create mode 100644 ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/DependencyInjection/SecretsGrpcHubClientExtensions.cs create mode 100644 ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretSyncService.cs create mode 100644 ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretsHubClient.cs create mode 100644 ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/SecretsGrpcHubClientOptions.cs create mode 100644 ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/Wire/WireToDomain.cs create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretSyncServiceTests.cs create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/SecretsGrpcHubClientOptionsTests.cs create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/WireToDomainTests.cs diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/DependencyInjection/SecretsGrpcHubClientExtensions.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/DependencyInjection/SecretsGrpcHubClientExtensions.cs new file mode 100644 index 0000000..c896c45 --- /dev/null +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/DependencyInjection/SecretsGrpcHubClientExtensions.cs @@ -0,0 +1,94 @@ +using Grpc.Net.Client; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Replicator.Grpc.V1; + +namespace ZB.MOM.WW.Secrets.Replicator.Grpc.DependencyInjection; + +/// +/// Dependency-injection entry point for a site node that follows the central-hosted secrets hub. +/// +/// +/// +/// The hub and every follower need the same KEK. Only ciphertext crosses the wire, so a node +/// whose master key differs cannot decrypt rows central wrote — it fails closed on resolve with a +/// kek_id mismatch, which reads like data corruption but is a deployment error. +/// +/// +/// This is the follower half only. Central hosts the hub service and must not register this +/// client; a site node registers this client and must not host the service. Nothing here can push a +/// row hub-ward — the contract has no write RPC — so the direction of replication is a property of +/// the deployment topology rather than a setting that could be got wrong. +/// +/// +public static class SecretsGrpcHubClientExtensions +{ + /// + /// Service key under which this package's is registered, so it can + /// never be confused with a channel the application registered for something else. + /// + public const string ChannelServiceKey = "zb-secrets-grpc-hub"; + + /// + /// Registers the pull-only hub client and its background sweep: a channel to the configured + /// endpoint, the generated gRPC client, the reader seam over it, and a hosted service that keeps + /// the node's local store converged with central. + /// + /// + /// The local is not registered or decorated here — call + /// AddZbSecrets as usual and this sits alongside it. That is the whole difference from the + /// SQL-Server hub-replication mode, which wraps the store in a publishing decorator: pull-only + /// means local writes never leave the node, so there is nothing to publish and the store stays + /// exactly as the application configured it. A host that forgets AddZbSecrets fails loudly + /// at startup when the sweep cannot resolve the store, rather than silently replicating nothing. + /// + /// The service collection to add to. + /// The application configuration. + /// Configuration section holding the hub-client options. + /// The same instance, for chaining. + /// + /// The bound options are unusable — see . + /// + public static IServiceCollection AddZbSecretsGrpcHubClient( + this IServiceCollection services, + IConfiguration configuration, + string sectionPath = SecretsGrpcHubClientOptions.DefaultSectionPath) + { + ArgumentNullException.ThrowIfNull(services); + ArgumentNullException.ThrowIfNull(configuration); + ArgumentException.ThrowIfNullOrWhiteSpace(sectionPath); + + IConfigurationSection section = configuration.GetSection(sectionPath); + services.Configure(section); + + // Bind and validate NOW as well as through the options pipeline: a missing endpoint or an + // unset bearer token should fail at startup with a message naming the key, not surface as a + // sweep that warns once every interval and never converges. The eagerly-bound instance is + // discarded immediately — it is a credential holder, and everything downstream reads the + // options from IOptions rather than closing over it. + var eager = new SecretsGrpcHubClientOptions(); + section.Bind(eager); + eager.Validate(sectionPath); + + // Singleton, so the container owns its disposal — a channel per sweep would leak an HTTP/2 + // connection every interval. KEYED, because GrpcChannel is a framework type an application + // very plausibly registers for one of its own gRPC dependencies: an unkeyed TryAddSingleton + // would silently lose to that registration and this client would then dial someone else's + // endpoint with the hub credential attached. + services.TryAddKeyedSingleton(ChannelServiceKey, (sp, _) => GrpcChannel.ForAddress( + sp.GetRequiredService>().Value.Endpoint)); + + services.TryAddSingleton(sp => new GrpcSecretsHubClient( + new SecretsHub.SecretsHubClient( + sp.GetRequiredKeyedService(ChannelServiceKey)), + sp.GetRequiredService>(), + sp.GetService())); + + services.AddHostedService(); + + return services; + } +} diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretSyncService.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretSyncService.cs new file mode 100644 index 0000000..b572d6c --- /dev/null +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretSyncService.cs @@ -0,0 +1,148 @@ +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Replication; + +namespace ZB.MOM.WW.Secrets.Replicator.Grpc; + +/// +/// Keeps a site node's local secret store converged with the central-hosted hub by running a +/// pull-only anti-entropy sweep on an interval. +/// +/// +/// +/// This is the SQL-Server package's sync service with one half removed, and the removal is the +/// design. In this topology every update originates at central and a site node is a strict follower, +/// so there is no push set to compute and no local write to propagate outward. The wire contract has +/// no write RPC to push with either — the narrow contract IS the enforcement, and this service is +/// kept honest to match it so the invariant reads the same from both ends. +/// +/// +/// The local store is left completely untouched as a write target for ordinary local use: nothing +/// here decorates it, so local reads and any local writes behave exactly as they did before +/// replication was switched on. This sweep is the only writer that carries hub state into it. +/// +/// +/// The sweep never throws out of : a hub outage must degrade the node to +/// "serving its last-known-good local secrets" rather than crash the host. Failures are logged once +/// and retried on the next tick. +/// +/// +internal sealed partial class GrpcSecretSyncService : BackgroundService +{ + private readonly SecretReplicationReconciler _reconciler; + private readonly ISecretsHubReader _hub; + private readonly SecretsGrpcHubClientOptions _options; + private readonly ILogger _logger; + private readonly TimeProvider _timeProvider; + + /// Creates the sync service. + /// The node's local store — the only thing this service writes to. + /// The central hub, read-only. + /// Resolver-cache seam, evicted for every applied row. + /// Hub-client options. + /// Logger for sweep outcomes and failures. + /// Clock, injectable so tests need not wait a real interval. + public GrpcSecretSyncService( + ISecretStore local, + ISecretsHubReader hub, + ISecretCacheInvalidator? cacheInvalidator, + IOptions options, + ILogger logger, + TimeProvider? timeProvider = null) + { + ArgumentNullException.ThrowIfNull(options); + ArgumentNullException.ThrowIfNull(local); + + _hub = hub ?? throw new ArgumentNullException(nameof(hub)); + _options = options.Value; + _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + _timeProvider = timeProvider ?? TimeProvider.System; + _reconciler = new SecretReplicationReconciler( + local, + cacheInvalidator, + onRowFailed: (name, ex) => LogRowDiscarded(ex, name.Value)); + } + + /// + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + if (_options.SyncOnStartup) + { + await SweepSafelyAsync(stoppingToken).ConfigureAwait(false); + } + + using PeriodicTimer timer = new(_options.SyncInterval, _timeProvider); + + while (await timer.WaitForNextTickAsync(stoppingToken).ConfigureAwait(false)) + { + await SweepSafelyAsync(stoppingToken).ConfigureAwait(false); + } + } + + /// + /// Runs one pull-only reconcile against the hub. + /// + /// A token to cancel the sweep. + /// How many rows were pulled from the hub. + internal async Task SweepAsync(CancellationToken ct) + { + IReadOnlyList hubManifest = + await _hub.GetManifestAsync(ct).ConfigureAwait(false); + + // The reconciler's pull path is exactly the required algorithm — diff the hub's manifest + // against the local one, fetch what the hub holds newer, apply last-writer-wins, evict the + // resolver cache per applied row — and it is documented as "a pull, never a push". Calling + // it rather than re-deriving the diff here also keeps the batching: a cold-starting node's + // pull set is central's ENTIRE inventory, and an unchunked GetSecrets would exceed gRPC's + // 4 MB default message cap and then fail identically on every interval forever. + return await _reconciler + .ReconcileAsync(hubManifest, (names, token) => _hub.GetManyAsync(names, token), ct) + .ConfigureAwait(false); + } + + private async Task SweepSafelyAsync(CancellationToken ct) + { + try + { + int pulled = await SweepAsync(ct).ConfigureAwait(false); + + if (pulled > 0) + { + LogSweepConverged(pulled); + } + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + // Host shutdown — not a failure. + } + catch (Exception ex) + { + // Deliberately catch-all. Everything reachable from here — an unreachable hub, a + // rejected credential, a deadline, a row the hub sent that this node's parse refuses — + // has the same correct response: keep serving the local store and try again. Letting any + // of it escape would fault the BackgroundService and take the host down over a transient + // central outage, which is precisely the availability the pull-only topology exists for. + LogSweepFailed(ex); + } + } + + [LoggerMessage( + EventId = 1, + Level = LogLevel.Information, + Message = "Secret hub sync converged: pulled {Pulled} row(s) from the central hub.")] + private partial void LogSweepConverged(int pulled); + + [LoggerMessage( + EventId = 2, + Level = LogLevel.Warning, + Message = "Secret hub sync failed; the node continues serving its local store and will retry on the next interval.")] + private partial void LogSweepFailed(Exception exception); + + [LoggerMessage( + EventId = 3, + Level = LogLevel.Warning, + Message = "Discarding replicated secret row {SecretName} that could not be applied locally; the rest of the batch continues.")] + private partial void LogRowDiscarded(Exception exception, string secretName); +} diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretsHubClient.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretsHubClient.cs new file mode 100644 index 0000000..8eef05d --- /dev/null +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretsHubClient.cs @@ -0,0 +1,111 @@ +using Grpc.Core; +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; + +namespace ZB.MOM.WW.Secrets.Replicator.Grpc; + +/// +/// The read half of the hub, as a follower sees it: compare inventories, then fetch rows in bulk. +/// +/// +/// +/// This is the whole of what a site node may do to central's secret store, and the interface says so +/// — there is no apply, no upsert, no delete. It mirrors the read half of the SQL-Server package's +/// ISecretReplicationHub without referencing it: the two topologies are deliberately separate +/// packages, and importing that interface would drag a SQL-Server dependency onto every site node +/// while also handing this one a write method it must never call. +/// +/// +/// It exists as a seam so the sweep is testable against an in-process fake instead of needing a live +/// gRPC server for every convergence case. +/// +/// +internal interface ISecretsHubReader +{ + /// Returns the hub's manifest — names, revisions, timestamps, tombstone flags. + /// A token to cancel the operation. + /// The hub's manifest entries. + Task> GetManifestAsync(CancellationToken ct); + + /// Fetches several encrypted rows by name; names absent from the hub are omitted. + /// The names to fetch. + /// A token to cancel the operation. + /// The matching encrypted rows. + Task> GetManyAsync(IReadOnlyList names, CancellationToken ct); +} + +/// +/// over the generated secrets_hub.v1 gRPC client: attaches the +/// bearer credential and a per-call deadline, and converts every response through +/// . +/// +/// +/// Faults are deliberately not caught here. An is the honest signal +/// that the hub did not answer, and the sweep — the only caller — is where that is turned into "keep +/// serving the last-known-good local store and retry next tick". Swallowing it here would make an +/// unreachable hub indistinguishable from an empty one, which is the one failure mode this +/// subsystem must never have. +/// +internal sealed class GrpcSecretsHubClient : ISecretsHubReader +{ + private const string AuthorizationHeader = "Authorization"; + + private readonly SecretsHub.SecretsHubClient _client; + private readonly SecretsGrpcHubClientOptions _options; + private readonly TimeProvider _timeProvider; + + /// Creates the hub client. + /// The generated gRPC client, bound to the configured channel. + /// Endpoint, credential, and per-call deadline. + /// Clock used to compute the per-call deadline. + public GrpcSecretsHubClient( + SecretsHub.SecretsHubClient client, + IOptions options, + TimeProvider? timeProvider = null) + { + ArgumentNullException.ThrowIfNull(options); + + _client = client ?? throw new ArgumentNullException(nameof(client)); + _options = options.Value; + _timeProvider = timeProvider ?? TimeProvider.System; + } + + /// + public async Task> GetManifestAsync(CancellationToken ct) + { + GetManifestResponse response = await _client + .GetManifestAsync(new GetManifestRequest(), CreateCallOptions(ct)) + .ConfigureAwait(false); + + return WireToDomain.ToManifest(response.Entries); + } + + /// + public async Task> GetManyAsync( + IReadOnlyList names, CancellationToken ct) + { + ArgumentNullException.ThrowIfNull(names); + + var request = new GetSecretsRequest(); + + foreach (SecretName name in names) + { + request.Names.Add(name.Value); + } + + GetSecretsResponse response = await _client + .GetSecretsAsync(request, CreateCallOptions(ct)) + .ConfigureAwait(false); + + return WireToDomain.ToRows(response.Secrets); + } + + // Built per call rather than once: the deadline is an absolute instant, so a cached CallOptions + // would carry a deadline that expired shortly after startup and then fail every sweep forever. + private CallOptions CreateCallOptions(CancellationToken ct) => new( + headers: new Metadata { { AuthorizationHeader, $"Bearer {_options.BearerToken}" } }, + deadline: _timeProvider.GetUtcNow().UtcDateTime.Add(_options.CallDeadline), + cancellationToken: ct); +} diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/SecretsGrpcHubClientOptions.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/SecretsGrpcHubClientOptions.cs new file mode 100644 index 0000000..93a68b3 --- /dev/null +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/SecretsGrpcHubClientOptions.cs @@ -0,0 +1,112 @@ +namespace ZB.MOM.WW.Secrets.Replicator.Grpc; + +/// +/// Configuration for a site node that follows the central-hosted secrets hub. Bound from a +/// configuration section by AddZbSecretsGrpcHubClient. +/// +/// +/// +/// Every field here describes how to reach the hub. There is deliberately no knob that +/// changes the direction of replication: this package is pull-only by construction and the wire +/// contract carries no write RPC, so a follower cannot be configured into pushing. +/// +/// +public sealed class SecretsGrpcHubClientOptions +{ + /// + /// The hub's gRPC address, as an absolute http or https URI (for example + /// https://central:5001). Required. + /// + public string Endpoint { get; set; } = string.Empty; + + /// + /// Shared bearer token presented on every call. Required — an unset token is a hard startup + /// failure rather than an anonymous call, because the hub authenticates on this alone. + /// + /// + /// This value is never logged, never included in an exception message, and never echoed back by + /// validation: a startup log is read by far more people than the deployment secret store is. + /// Supply it the way the mesh PSKs are supplied — appsettings or environment — not as a + /// ${secret:} reference, which would need the very store this client exists to fill. + /// + public string BearerToken { get; set; } = string.Empty; + + /// + /// How often the follower sweeps the hub for rows it is missing or behind on. Defaults to + /// 30 seconds. Must be positive. + /// + public TimeSpan SyncInterval { get; set; } = TimeSpan.FromSeconds(30); + + /// + /// When (the default), a sweep runs at startup before the first interval + /// elapses, so a node that was down while central wrote does not serve stale secrets for a whole + /// interval after it comes back. + /// + public bool SyncOnStartup { get; set; } = true; + + /// + /// Deadline applied to each hub call. Defaults to 30 seconds — long enough for a busy central + /// node, short enough that an unreachable hub fails the sweep and is retried on the next tick + /// instead of pinning the sweep loop indefinitely. + /// + public TimeSpan CallDeadline { get; set; } = TimeSpan.FromSeconds(30); + + /// + /// Throws if the options are unusable. Called by the DI extension at registration time so a + /// misconfigured node fails at startup rather than on the first silent sweep failure. + /// + /// + /// The configuration section the values were bound from, so each message names the key an + /// operator actually has to edit rather than the default one. + /// + /// + /// The endpoint is missing or is not an absolute http/https URI, the bearer token is missing, or + /// an interval is not positive. + /// + public void Validate(string sectionPath = DefaultSectionPath) + { + ArgumentException.ThrowIfNullOrWhiteSpace(sectionPath); + + if (string.IsNullOrWhiteSpace(Endpoint)) + { + throw new InvalidOperationException( + $"gRPC secrets hub client requires an Endpoint ({sectionPath}:Endpoint)."); + } + + if (!Uri.TryCreate(Endpoint, UriKind.Absolute, out Uri? uri) || + (uri.Scheme != Uri.UriSchemeHttp && uri.Scheme != Uri.UriSchemeHttps)) + { + // Rejected here rather than at GrpcChannel.ForAddress, which throws deep inside the + // first sweep — by which point the failure looks like a hub outage instead of a typo. + throw new InvalidOperationException( + $"gRPC secrets hub client Endpoint must be an absolute http or https URI " + + $"({sectionPath}:Endpoint, was '{Endpoint}')."); + } + + if (string.IsNullOrWhiteSpace(BearerToken)) + { + // Fail closed. An empty token would otherwise be presented as `Bearer `, which the hub + // rejects on every call — an authentication outage that looks like a network problem. + throw new InvalidOperationException( + $"gRPC secrets hub client requires a BearerToken ({sectionPath}:BearerToken); " + + "it is unset, so this node has no way to authenticate to the hub."); + } + + if (SyncInterval <= TimeSpan.Zero) + { + throw new InvalidOperationException( + $"gRPC secrets hub client SyncInterval must be positive " + + $"({sectionPath}:SyncInterval, was {SyncInterval})."); + } + + if (CallDeadline <= TimeSpan.Zero) + { + throw new InvalidOperationException( + $"gRPC secrets hub client CallDeadline must be positive " + + $"({sectionPath}:CallDeadline, was {CallDeadline})."); + } + } + + /// The configuration section these options are bound from unless a caller overrides it. + public const string DefaultSectionPath = "Secrets:GrpcHub"; +} diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/Wire/WireToDomain.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/Wire/WireToDomain.cs new file mode 100644 index 0000000..e662ab0 --- /dev/null +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/Wire/WireToDomain.cs @@ -0,0 +1,123 @@ +using System.Globalization; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Replicator.Grpc.V1; + +namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Wire; + +/// +/// Rebuilds domain types from the secrets_hub.v1 wire contract. This is the trust boundary a +/// follower applies to everything the hub sends it. +/// +/// +/// +/// Parsing here is strict, and rejection is per row: a value protobuf accepted but the domain +/// would not — a name that is rooted or path-traversing, a content type naming no enum member, a +/// timestamp that is not round-trip ISO-8601 — throws rather than being coerced. The sweep's +/// reconciler isolates the failure to that one row (see SecretReplicationReconciler.ApplyAsync), +/// so a single bad row is logged and skipped instead of poisoning the batch behind it. +/// +/// +/// This mirrors the Akka replicator's SecretRowDto.ToStoredSecret deliberately — one +/// fail-closed parse shape family-wide, so the two transports cannot disagree about what a +/// replicated row is allowed to contain. +/// +/// +/// Two checks the Akka DTO makes are absent here on purpose. Protobuf cannot express a null +/// bytes or a null string — an omitted field decodes to empty, never null — so the +/// null-blob guards have nothing to catch. Emptiness itself is deliberately not judged: an empty +/// is legitimate (AES-GCM ciphertext is the length of its +/// plaintext, so a secret whose value is the empty string has exactly that), and rejecting it here +/// would refuse a valid row. A structurally nonsensical row fails closed one layer down, at decrypt. +/// +/// +internal static class WireToDomain +{ + /// Rebuilds a manifest from the wire form, in the order the hub sent it. + /// The wire manifest entries. + /// The domain manifest entries. + public static IReadOnlyList ToManifest(IEnumerable entries) + { + ArgumentNullException.ThrowIfNull(entries); + + return [.. entries.Select(ToEntry)]; + } + + /// Rebuilds one manifest entry from the wire form. + /// The wire manifest entry. + /// The domain manifest entry. + /// The name is not a valid secret name. + /// The timestamp is not a round-trip ISO-8601 value. + public static SecretManifestEntry ToEntry(ManifestEntry entry) + { + ArgumentNullException.ThrowIfNull(entry); + + return new SecretManifestEntry + { + // SecretName re-validates on construction, so a malformed or path-traversing name from + // the hub is rejected here rather than reaching the store. + Name = new SecretName(entry.Name), + Revision = entry.Revision, + UpdatedUtc = ParseTimestamp(entry.UpdatedUtc), + IsDeleted = entry.IsDeleted, + }; + } + + /// Rebuilds encrypted rows from the wire form, in the order the hub sent them. + /// The wire rows. + /// The domain rows. + public static IReadOnlyList ToRows(IEnumerable rows) + { + ArgumentNullException.ThrowIfNull(rows); + + return [.. rows.Select(ToRow)]; + } + + /// Rebuilds one encrypted row from the wire form. + /// The wire row. + /// The domain row. + /// The name or content type is not valid. + /// A timestamp is not a round-trip ISO-8601 value. + public static StoredSecret ToRow(SecretRow row) + { + ArgumentNullException.ThrowIfNull(row); + + return new StoredSecret + { + Name = new SecretName(row.Name), + // Explicit presence, not `?? null`: proto3 optional is what keeps a description that was + // deliberately set to the empty string distinguishable from one that was never set. + Description = row.HasDescription ? row.Description : null, + ContentType = ParseContentType(row.ContentType), + Ciphertext = row.Ciphertext.ToByteArray(), + Nonce = row.Nonce.ToByteArray(), + Tag = row.Tag.ToByteArray(), + WrappedDek = row.WrappedDek.ToByteArray(), + WrapNonce = row.WrapNonce.ToByteArray(), + WrapTag = row.WrapTag.ToByteArray(), + KekId = row.KekId, + Revision = row.Revision, + IsDeleted = row.IsDeleted, + DeletedUtc = row.HasDeletedUtc ? ParseTimestamp(row.DeletedUtc) : null, + CreatedUtc = ParseTimestamp(row.CreatedUtc), + UpdatedUtc = ParseTimestamp(row.UpdatedUtc), + CreatedBy = row.HasCreatedBy ? row.CreatedBy : null, + UpdatedBy = row.HasUpdatedBy ? row.UpdatedBy : null, + }; + } + + // RoundtripKind, so an offset-bearing "O" string comes back with its offset intact. The hub and + // the follower compare UpdatedUtc for last-writer-wins, so "close enough" is not enough: a value + // that shifted by a tick or lost its offset would make a converged pair look permanently unequal. + private static DateTimeOffset ParseTimestamp(string value) => DateTimeOffset.Parse( + value, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind); + + // Enum.Parse is wrong for untrusted input on two counts: a numeric string that overflows the + // underlying type throws OverflowException where callers expect ArgumentException, and any + // in-range number is accepted even if it names no member — persisting an undefined enum value + // that later readers assume is valid. + private static SecretContentType ParseContentType(string value) => + Enum.TryParse(value, out SecretContentType parsed) && Enum.IsDefined(parsed) + ? parsed + : throw new ArgumentException( + $"Replicated secret row has an unrecognized content type '{value}'.", nameof(value)); +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretSyncServiceTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretSyncServiceTests.cs new file mode 100644 index 0000000..c48e3cd --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretSyncServiceTests.cs @@ -0,0 +1,504 @@ +using System.Diagnostics; +using System.Reflection; +using Grpc.Net.Client; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Replicator.Grpc.DependencyInjection; +using ZB.MOM.WW.Secrets.Sqlite; + +namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Tests; + +/// +/// Convergence behaviour of the follower's pull-only sweep, exercised against real SQLite stores on +/// both sides so the assertions are about actual persisted state rather than mock interactions. +/// +public sealed class GrpcSecretSyncServiceTests : IDisposable +{ + private readonly List _dbPaths = []; + private readonly SqliteSecretStore _local; + private readonly SqliteSecretStore _hubStore; + private readonly FakeHubReader _hub; + private readonly RecordingLogger _logger = new(); + private readonly RecordingCacheInvalidator _cache = new(); + + public GrpcSecretSyncServiceTests() + { + _local = CreateStore(); + _hubStore = CreateStore(); + _hub = new FakeHubReader(_hubStore); + } + + private SqliteSecretStore CreateStore() + { + string path = Path.Combine(Path.GetTempPath(), $"zb-grpc-sync-{Guid.NewGuid():N}.db"); + _dbPaths.Add(path); + var factory = new SecretsSqliteConnectionFactory(path); + new SqliteSecretsStoreMigrator(factory).MigrateAsync(CancellationToken.None).GetAwaiter().GetResult(); + return new SqliteSecretStore(factory); + } + + private GrpcSecretSyncService CreateService(TimeSpan? interval = null) => new( + _local, + _hub, + _cache, + Options.Create(new SecretsGrpcHubClientOptions + { + Endpoint = "https://central:5001", + BearerToken = "unused-by-the-fake", + SyncInterval = interval ?? TimeSpan.FromHours(1), + }), + _logger); + + private static StoredSecret Row(string name, byte marker) => new() + { + Name = new SecretName(name), + ContentType = SecretContentType.Text, + Ciphertext = [marker], + Nonce = [1], + Tag = [2], + WrappedDek = [3], + WrapNonce = [4], + WrapTag = [5], + KekId = "sha256:test", + Revision = 0, + CreatedUtc = DateTimeOffset.UtcNow, + UpdatedUtc = DateTimeOffset.UtcNow, + }; + + [Fact] + public async Task Sweep_pulls_a_secret_the_node_has_never_seen() + { + await _hubStore.UpsertAsync(Row("db/password", 0xAA), CancellationToken.None); + + int pulled = await CreateService().SweepAsync(CancellationToken.None); + + Assert.Equal(1, pulled); + + StoredSecret? local = await _local.GetAsync(new SecretName("db/password"), CancellationToken.None); + Assert.NotNull(local); + Assert.Equal(0xAA, local!.Ciphertext[0]); + } + + [Fact] + public async Task Sweep_pulls_the_row_byte_identically() + { + await _hubStore.UpsertAsync(Row("db/password", 0xAA), CancellationToken.None); + await _hubStore.UpsertAsync(Row("db/password", 0xBB), CancellationToken.None); + + StoredSecret hubRow = (await _hubStore.GetAsync(new SecretName("db/password"), CancellationToken.None))!; + + await CreateService().SweepAsync(CancellationToken.None); + + StoredSecret localRow = (await _local.GetAsync(new SecretName("db/password"), CancellationToken.None))!; + + Assert.Equal(hubRow.Ciphertext, localRow.Ciphertext); + Assert.Equal(hubRow.WrappedDek, localRow.WrappedDek); + Assert.Equal(hubRow.KekId, localRow.KekId); + Assert.Equal(hubRow.Revision, localRow.Revision); + Assert.Equal(hubRow.UpdatedUtc, localRow.UpdatedUtc); + } + + [Fact] + public async Task Sweep_applies_a_hub_tombstone_over_a_live_local_row() + { + await _hubStore.UpsertAsync(Row("api/key", 0xBB), CancellationToken.None); + await CreateService().SweepAsync(CancellationToken.None); + + await _hubStore.DeleteAsync(new SecretName("api/key"), "admin", CancellationToken.None); + int pulled = await CreateService().SweepAsync(CancellationToken.None); + + Assert.Equal(1, pulled); + + StoredSecret localRow = (await _local.GetAsync(new SecretName("api/key"), CancellationToken.None))!; + Assert.True(localRow.IsDeleted); + } + + [Fact] + public async Task Sweep_is_idempotent_so_a_converged_follower_stays_quiet() + { + await _hubStore.UpsertAsync(Row("api/key", 0xBB), CancellationToken.None); + + GrpcSecretSyncService service = CreateService(); + await service.SweepAsync(CancellationToken.None); + + int pulled = await service.SweepAsync(CancellationToken.None); + + Assert.Equal(0, pulled); + } + + [Fact] + public async Task Sweep_skips_the_fetch_when_the_hub_has_nothing_newer() + { + await CreateService().SweepAsync(CancellationToken.None); + + Assert.Equal(0, _hub.GetManyCallCount); + } + + [Fact] + public async Task Sweep_never_sends_a_local_only_secret_hub_ward() + { + // Pull-only: a row this node holds and the hub does not stays put. In the SQL-Server + // topology this would be pushed; here central is the sole origin of every update, and the + // contract has no write RPC to push it with. + await _local.UpsertAsync(Row("only/local", 0x11), CancellationToken.None); + + int pulled = await CreateService().SweepAsync(CancellationToken.None); + + Assert.Equal(0, pulled); + Assert.Empty(await _hubStore.GetManifestAsync(CancellationToken.None)); + } + + [Fact] + public void The_hub_reader_seam_exposes_no_write_operation() + { + // The structural half of pull-only. The proto has no write RPC; this pins that the client's + // own seam cannot grow one either, so a future edit has to break this test on purpose. + string[] members = typeof(ISecretsHubReader) + .GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly) + .Select(m => m.Name) + .Order(StringComparer.Ordinal) + .ToArray(); + + Assert.Equal(["GetManifestAsync", "GetManyAsync"], members); + } + + [Theory] + [InlineData("ComputePushSet")] + [InlineData("ReadLocalAsync")] + [InlineData("ApplyReplicatedAsync")] + public void The_client_package_never_names_a_hub_ward_write_path(string forbidden) + { + // ComputePushSet and ReadLocalAsync are the two reconciler entry points that assemble rows + // to move toward a peer; ApplyReplicatedAsync on a hub-side store is how they would land. + // Pull-only is an owner-confirmed invariant, so the guard is a source scan rather than a + // behavioural test: a future edit has to delete this test on purpose to reintroduce a push. + foreach (string file in ClientSourceFiles()) + { + string source = File.ReadAllText(file); + + Assert.DoesNotContain( + forbidden, + StripComments(source), + StringComparison.Ordinal); + } + } + + // The client half of the package — deliberately excludes the server-side files, which legitimately + // read a local store to serve the hub. + private static IEnumerable ClientSourceFiles() + { + string root = PackageSourceRoot(); + + string[] files = + [ + Path.Combine(root, "GrpcSecretSyncService.cs"), + Path.Combine(root, "GrpcSecretsHubClient.cs"), + Path.Combine(root, "Wire", "WireToDomain.cs"), + Path.Combine(root, "DependencyInjection", "SecretsGrpcHubClientExtensions.cs"), + ]; + + foreach (string file in files) + { + Assert.True(File.Exists(file), $"expected client source file is missing: {file}"); + } + + return files; + } + + private static string PackageSourceRoot() + { + DirectoryInfo? dir = new(AppContext.BaseDirectory); + + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.Secrets.slnx"))) + { + dir = dir.Parent; + } + + Assert.NotNull(dir); + + return Path.Combine(dir!.FullName, "src", "ZB.MOM.WW.Secrets.Replicator.Grpc"); + } + + // Comments explain what the code deliberately does NOT do, and naming the forbidden member is + // exactly how that is explained — so the scan looks at code only. + private static string StripComments(string source) + { + IEnumerable lines = source + .Split('\n') + .Select(line => line.TrimStart()) + .Where(line => !line.StartsWith("//", StringComparison.Ordinal) + && !line.StartsWith("///", StringComparison.Ordinal) + && !line.StartsWith('*')); + + return string.Join('\n', lines); + } + + [Fact] + public async Task Sweep_evicts_the_resolver_cache_for_applied_names_only() + { + await _hubStore.UpsertAsync(Row("db/password", 0xAA), CancellationToken.None); + await _local.UpsertAsync(Row("only/local", 0x11), CancellationToken.None); + + await CreateService().SweepAsync(CancellationToken.None); + + Assert.Equal(["db/password"], _cache.Invalidated.Select(n => n.Value)); + } + + [Fact] + public async Task A_hub_outage_on_the_manifest_call_propagates_out_of_the_raw_sweep() + { + // Establishes that the fake really fails, so the next two tests prove containment rather + // than an accidentally-healthy hub. + _hub.FailManifestWith = () => new InvalidOperationException("hub unreachable"); + + await Assert.ThrowsAsync( + () => CreateService().SweepAsync(CancellationToken.None)); + } + + [Fact] + public async Task A_hub_outage_on_the_manifest_call_never_escapes_the_background_loop() + { + await _local.UpsertAsync(Row("api/key", 0xBB), CancellationToken.None); + StoredSecret before = (await _local.GetAsync(new SecretName("api/key"), CancellationToken.None))!; + + _hub.FailManifestWith = () => new InvalidOperationException("hub unreachable"); + + await RunOneStartupSweepAsync(() => _hub.ManifestCallCount > 0); + + StoredSecret after = (await _local.GetAsync(new SecretName("api/key"), CancellationToken.None))!; + Assert.Equal(before.Ciphertext, after.Ciphertext); + Assert.Equal(before.Revision, after.Revision); + Assert.Equal(before.UpdatedUtc, after.UpdatedUtc); + Assert.False(after.IsDeleted); + Assert.Equal(1, _logger.WarningCount); + } + + [Fact] + public async Task A_hub_outage_on_the_fetch_call_never_escapes_the_background_loop() + { + await _hubStore.UpsertAsync(Row("db/password", 0xAA), CancellationToken.None); + _hub.FailFetchWith = () => new InvalidOperationException("hub unreachable mid-fetch"); + + await RunOneStartupSweepAsync(() => _hub.GetManyCallCount > 0); + + Assert.Null(await _local.GetAsync(new SecretName("db/password"), CancellationToken.None)); + Assert.Equal(1, _logger.WarningCount); + } + + [Fact] + public async Task A_quiet_sweep_logs_nothing_at_information() + { + await RunOneStartupSweepAsync(() => _hub.ManifestCallCount > 0); + + Assert.Equal(0, _logger.InformationCount); + Assert.Equal(0, _logger.WarningCount); + } + + [Fact] + public async Task A_sweep_that_moved_rows_reports_how_many_it_pulled() + { + await _hubStore.UpsertAsync(Row("db/password", 0xAA), CancellationToken.None); + + await RunOneStartupSweepAsync(() => _hub.GetManyCallCount > 0); + + Assert.Equal(1, _logger.InformationCount); + } + + [Fact] + public async Task Startup_sync_can_be_switched_off() + { + await _hubStore.UpsertAsync(Row("db/password", 0xAA), CancellationToken.None); + + var service = new GrpcSecretSyncService( + _local, + _hub, + _cache, + Options.Create(new SecretsGrpcHubClientOptions + { + Endpoint = "https://central:5001", + BearerToken = "unused-by-the-fake", + SyncInterval = TimeSpan.FromHours(1), + SyncOnStartup = false, + }), + _logger); + + await service.StartAsync(CancellationToken.None); + await service.StopAsync(CancellationToken.None); + + Assert.Equal(0, _hub.ManifestCallCount); + } + + [Fact] + public void Registration_refuses_a_configuration_with_no_bearer_token() + { + IConfiguration config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Secrets:GrpcHub:Endpoint"] = "https://central:5001", + }) + .Build(); + + InvalidOperationException ex = Assert.Throws( + () => new ServiceCollection().AddZbSecretsGrpcHubClient(config)); + + Assert.Contains("Secrets:GrpcHub:BearerToken", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void Registration_does_not_hijack_an_application_s_own_grpc_channel() + { + // GrpcChannel is a framework type, so an application that already has one registered for its + // own dependency must keep it — and this package must still dial the hub, not that endpoint. + IConfiguration config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Secrets:GrpcHub:Endpoint"] = "https://central:5001", + ["Secrets:GrpcHub:BearerToken"] = "token", + }) + .Build(); + + using GrpcChannel applicationChannel = GrpcChannel.ForAddress("https://somewhere-else:9999"); + + ServiceCollection services = new(); + services.AddSingleton(applicationChannel); + services.AddZbSecretsGrpcHubClient(config); + + using ServiceProvider provider = services.BuildServiceProvider(); + + Assert.Same(applicationChannel, provider.GetRequiredService()); + Assert.NotSame( + applicationChannel, + provider.GetRequiredKeyedService( + SecretsGrpcHubClientExtensions.ChannelServiceKey)); + } + + // Runs the hosted service through one startup sweep and shuts it down, asserting that nothing + // escaped ExecuteAsync. The long sync interval means exactly one sweep happens. + private async Task RunOneStartupSweepAsync(Func sweepObserved) + { + GrpcSecretSyncService service = CreateService(); + + await service.StartAsync(CancellationToken.None); + + var deadline = Stopwatch.StartNew(); + + while (!sweepObserved() && deadline.Elapsed < TimeSpan.FromSeconds(10)) + { + await Task.Delay(10); + } + + await service.StopAsync(CancellationToken.None); + + Assert.True(sweepObserved(), "the startup sweep never ran"); + Assert.NotNull(service.ExecuteTask); + Assert.False(service.ExecuteTask!.IsFaulted); + } + + public void Dispose() + { + Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools(); + + foreach (string db in _dbPaths) + { + foreach (string path in new[] { db, db + "-wal", db + "-shm" }) + { + if (File.Exists(path)) + { + File.Delete(path); + } + } + } + } + + /// + /// An backed by a real SQLite store, standing in for the + /// central-hosted hub. Backed by a real store rather than a dictionary for the same reason the + /// SQL-Server suite is: the behaviour under test is last-writer-wins convergence, and a + /// hand-rolled fake would re-implement the very logic the tests exist to check. + /// + private sealed class FakeHubReader(SqliteSecretStore store) : ISecretsHubReader + { + public int ManifestCallCount { get; private set; } + + public int GetManyCallCount { get; private set; } + + public Func? FailManifestWith { get; set; } + + public Func? FailFetchWith { get; set; } + + public Task> GetManifestAsync(CancellationToken ct) + { + ManifestCallCount++; + + return FailManifestWith is null + ? store.GetManifestAsync(ct) + : Task.FromException>(FailManifestWith()); + } + + public async Task> GetManyAsync( + IReadOnlyList names, CancellationToken ct) + { + GetManyCallCount++; + + if (FailFetchWith is not null) + { + throw FailFetchWith(); + } + + var rows = new List(names.Count); + + foreach (SecretName name in names) + { + StoredSecret? row = await store.GetAsync(name, ct); + + if (row is not null) + { + rows.Add(row); + } + } + + return rows; + } + } + + private sealed class RecordingCacheInvalidator : ISecretCacheInvalidator + { + public List Invalidated { get; } = []; + + public void Invalidate(SecretName name) => Invalidated.Add(name); + } + + private sealed class RecordingLogger : ILogger + { + public int InformationCount { get; private set; } + + public int WarningCount { get; private set; } + + 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) + { + switch (logLevel) + { + case LogLevel.Information: + InformationCount++; + break; + case LogLevel.Warning: + WarningCount++; + break; + default: + break; + } + } + } +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/SecretsGrpcHubClientOptionsTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/SecretsGrpcHubClientOptionsTests.cs new file mode 100644 index 0000000..cec924d --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/SecretsGrpcHubClientOptionsTests.cs @@ -0,0 +1,132 @@ +namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Tests; + +/// +/// Fail-closed validation of the follower's hub-client options. Every message must name the exact +/// configuration key an operator has to fix, and none of them may ever echo the bearer token. +/// +public sealed class SecretsGrpcHubClientOptionsTests +{ + private const string Token = "s3cr3t-hub-token"; + + private static SecretsGrpcHubClientOptions Valid() => new() + { + Endpoint = "https://central:5001", + BearerToken = Token, + }; + + [Fact] + public void Defaults_match_the_documented_contract() + { + var options = new SecretsGrpcHubClientOptions(); + + Assert.Equal(TimeSpan.FromSeconds(30), options.SyncInterval); + Assert.Equal(TimeSpan.FromSeconds(30), options.CallDeadline); + Assert.True(options.SyncOnStartup); + } + + [Fact] + public void A_complete_configuration_validates() + { + Valid().Validate(); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void A_missing_endpoint_names_its_key(string endpoint) + { + SecretsGrpcHubClientOptions options = Valid(); + options.Endpoint = endpoint; + + InvalidOperationException ex = Assert.Throws(() => options.Validate()); + Assert.Contains("Secrets:GrpcHub:Endpoint", ex.Message, StringComparison.Ordinal); + } + + [Theory] + [InlineData("central:5001")] + [InlineData("/relative/path")] + [InlineData("ftp://central:5001")] + [InlineData("unix:///var/run/hub.sock")] + public void An_endpoint_that_is_not_an_absolute_http_uri_names_its_key(string endpoint) + { + SecretsGrpcHubClientOptions options = Valid(); + options.Endpoint = endpoint; + + InvalidOperationException ex = Assert.Throws(() => options.Validate()); + Assert.Contains("Secrets:GrpcHub:Endpoint", ex.Message, StringComparison.Ordinal); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void A_missing_bearer_token_names_its_key(string token) + { + SecretsGrpcHubClientOptions options = Valid(); + options.BearerToken = token; + + InvalidOperationException ex = Assert.Throws(() => options.Validate()); + Assert.Contains("Secrets:GrpcHub:BearerToken", ex.Message, StringComparison.Ordinal); + } + + public static TheoryData> InvalidMutations() => + [ + o => o.Endpoint = string.Empty, + o => o.Endpoint = "not a uri", + o => o.BearerToken = string.Empty, + o => o.SyncInterval = TimeSpan.Zero, + o => o.CallDeadline = TimeSpan.FromSeconds(-1), + ]; + + [Theory] + [MemberData(nameof(InvalidMutations))] + public void No_validation_message_ever_echoes_the_bearer_token( + Action makeInvalid) + { + // A configuration error surfaces in logs and crash dumps. Naming the key is the whole job; + // quoting the value would leak the hub credential to everyone who can read a startup log. + SecretsGrpcHubClientOptions options = Valid(); + makeInvalid(options); + + InvalidOperationException ex = Assert.Throws(() => options.Validate()); + + Assert.DoesNotContain(Token, ex.Message, StringComparison.Ordinal); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void A_non_positive_sync_interval_names_its_key(int seconds) + { + SecretsGrpcHubClientOptions options = Valid(); + options.SyncInterval = TimeSpan.FromSeconds(seconds); + + InvalidOperationException ex = Assert.Throws(() => options.Validate()); + Assert.Contains("Secrets:GrpcHub:SyncInterval", ex.Message, StringComparison.Ordinal); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void A_non_positive_call_deadline_names_its_key(int seconds) + { + SecretsGrpcHubClientOptions options = Valid(); + options.CallDeadline = TimeSpan.FromSeconds(seconds); + + InvalidOperationException ex = Assert.Throws(() => options.Validate()); + Assert.Contains("Secrets:GrpcHub:CallDeadline", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void Messages_name_the_section_the_caller_actually_bound() + { + // The registration extension takes a section path, so a deployment that binds somewhere else + // must be told about ITS key, not the default one. + SecretsGrpcHubClientOptions options = Valid(); + options.BearerToken = string.Empty; + + InvalidOperationException ex = Assert.Throws( + () => options.Validate("Central:Secrets:Hub")); + + Assert.Contains("Central:Secrets:Hub:BearerToken", ex.Message, StringComparison.Ordinal); + } +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/WireToDomainTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/WireToDomainTests.cs new file mode 100644 index 0000000..acbac6d --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/WireToDomainTests.cs @@ -0,0 +1,205 @@ +using Google.Protobuf; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Replicator.Grpc.V1; +using ZB.MOM.WW.Secrets.Replicator.Grpc.Wire; + +namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Tests; + +/// +/// The proto -> domain boundary: a follower rebuilding rows a hub sent it. These are the checks +/// protobuf itself cannot make — a name that normalizes and validates, a content type that names a +/// real member, and timestamps whose exact representation survives, because last-writer-wins +/// compares them. +/// +public sealed class WireToDomainTests +{ + private static SecretRow FullRow() => new() + { + Name = "db/password", + Description = "primary db", + ContentType = nameof(SecretContentType.ConnectionString), + Ciphertext = ByteString.CopyFrom(0xAA, 0xBB), + Nonce = ByteString.CopyFrom(1, 2, 3), + Tag = ByteString.CopyFrom(4, 5), + WrappedDek = ByteString.CopyFrom(6), + WrapNonce = ByteString.CopyFrom(7), + WrapTag = ByteString.CopyFrom(8), + KekId = "sha256:abc", + Revision = 7, + IsDeleted = false, + CreatedUtc = new DateTimeOffset(2026, 8, 7, 1, 2, 3, TimeSpan.Zero).ToString("O"), + UpdatedUtc = new DateTimeOffset(2026, 8, 7, 4, 5, 6, TimeSpan.Zero).ToString("O"), + CreatedBy = "alice", + UpdatedBy = "bob", + }; + + [Fact] + public void Row_maps_every_field_onto_the_domain_type() + { + StoredSecret row = WireToDomain.ToRow(FullRow()); + + Assert.Equal("db/password", row.Name.Value); + Assert.Equal("primary db", row.Description); + Assert.Equal(SecretContentType.ConnectionString, row.ContentType); + Assert.Equal(new byte[] { 0xAA, 0xBB }, row.Ciphertext); + Assert.Equal(new byte[] { 1, 2, 3 }, row.Nonce); + Assert.Equal(new byte[] { 4, 5 }, row.Tag); + Assert.Equal(new byte[] { 6 }, row.WrappedDek); + Assert.Equal(new byte[] { 7 }, row.WrapNonce); + Assert.Equal(new byte[] { 8 }, row.WrapTag); + Assert.Equal("sha256:abc", row.KekId); + Assert.Equal(7, row.Revision); + Assert.False(row.IsDeleted); + Assert.Null(row.DeletedUtc); + Assert.Equal("alice", row.CreatedBy); + Assert.Equal("bob", row.UpdatedBy); + } + + [Fact] + public void Row_rejects_an_unrecognized_content_type() + { + // The reason the wire carries a NAME rather than a proto enum: an unknown value is rejected + // at the boundary instead of silently decoding to the zero value and retyping the secret. + SecretRow wire = FullRow(); + wire.ContentType = "Kerberos"; + + ArgumentException ex = Assert.Throws(() => WireToDomain.ToRow(wire)); + Assert.Contains("Kerberos", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void Row_rejects_a_numeric_content_type_that_names_no_member() + { + SecretRow wire = FullRow(); + wire.ContentType = "9999"; + + Assert.Throws(() => WireToDomain.ToRow(wire)); + } + + [Fact] + public void Row_rejects_a_name_the_domain_would_not_accept() + { + SecretRow wire = FullRow(); + wire.ContentType = nameof(SecretContentType.Text); + wire.Name = "../../etc/passwd"; + + Assert.Throws(() => WireToDomain.ToRow(wire)); + } + + [Fact] + public void Row_distinguishes_an_absent_description_from_an_empty_one() + { + SecretRow absent = FullRow(); + absent.ClearDescription(); + Assert.Null(WireToDomain.ToRow(absent).Description); + + SecretRow empty = FullRow(); + empty.Description = string.Empty; + Assert.Equal(string.Empty, WireToDomain.ToRow(empty).Description); + } + + [Fact] + public void Row_distinguishes_absent_from_empty_for_every_nullable_field() + { + SecretRow absent = FullRow(); + absent.ClearCreatedBy(); + absent.ClearUpdatedBy(); + + StoredSecret row = WireToDomain.ToRow(absent); + + Assert.Null(row.CreatedBy); + Assert.Null(row.UpdatedBy); + } + + [Fact] + public void Row_round_trips_a_non_utc_offset_exactly() + { + // The whole reason the contract uses "O" strings and not google.protobuf.Timestamp: an + // offset-bearing DateTimeOffset must come back byte-identical, because LWW compares it. + var updated = new DateTimeOffset(2026, 8, 7, 4, 5, 6, 789, TimeSpan.FromHours(-5)); + + SecretRow wire = FullRow(); + wire.UpdatedUtc = updated.ToString("O"); + + StoredSecret row = WireToDomain.ToRow(wire); + + Assert.Equal(updated, row.UpdatedUtc); + Assert.Equal(updated.Offset, row.UpdatedUtc.Offset); + Assert.Equal(updated.ToString("O"), row.UpdatedUtc.ToString("O")); + } + + [Fact] + public void Row_maps_a_tombstone_with_its_deletion_timestamp() + { + var deleted = new DateTimeOffset(2026, 8, 7, 9, 9, 9, TimeSpan.Zero); + + SecretRow wire = FullRow(); + wire.IsDeleted = true; + wire.DeletedUtc = deleted.ToString("O"); + + StoredSecret row = WireToDomain.ToRow(wire); + + Assert.True(row.IsDeleted); + Assert.Equal(deleted, row.DeletedUtc); + } + + [Fact] + public void Row_leaves_the_deletion_timestamp_null_when_the_field_is_absent() + { + SecretRow wire = FullRow(); + wire.ClearDeletedUtc(); + + Assert.Null(WireToDomain.ToRow(wire).DeletedUtc); + } + + [Fact] + public void Row_rejects_a_malformed_timestamp() + { + SecretRow wire = FullRow(); + wire.UpdatedUtc = "yesterday"; + + Assert.Throws(() => WireToDomain.ToRow(wire)); + } + + [Fact] + public void Manifest_entry_maps_and_validates_its_name() + { + var updated = new DateTimeOffset(2026, 8, 7, 4, 5, 6, TimeSpan.FromHours(2)); + + SecretManifestEntry entry = WireToDomain.ToEntry(new ManifestEntry + { + Name = "api/key", + Revision = 3, + UpdatedUtc = updated.ToString("O"), + IsDeleted = true, + }); + + Assert.Equal("api/key", entry.Name.Value); + Assert.Equal(3, entry.Revision); + Assert.Equal(updated, entry.UpdatedUtc); + Assert.True(entry.IsDeleted); + } + + [Fact] + public void Manifest_entry_rejects_a_name_the_domain_would_not_accept() + { + Assert.Throws(() => WireToDomain.ToEntry(new ManifestEntry + { + Name = "/rooted", + Revision = 1, + UpdatedUtc = DateTimeOffset.UtcNow.ToString("O"), + })); + } + + [Fact] + public void Batch_conversions_preserve_order() + { + SecretRow first = FullRow(); + SecretRow second = FullRow(); + second.Name = "api/key"; + + IReadOnlyList rows = WireToDomain.ToRows([first, second]); + + Assert.Equal(["db/password", "api/key"], rows.Select(r => r.Name.Value)); + } +} From 1c31e6bcb8a66abe31df275d454f81cf9e8eb209 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:29:51 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(secrets):=20Grpc=20hub=20client=20?= =?UTF-8?q?=E2=80=94=20per-row=20parse=20isolation=20(review=20Critical)?= =?UTF-8?q?=20+=20real-client=20CallInvoker=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single malformed row or manifest entry poisoned the whole sweep, permanently. WireToDomain.ToRows/ToManifest materialized eagerly, so one unparseable element threw out of the fetch callback, past SecretReplicationReconciler's per-batch handling, into SweepSafelyAsync's whole-sweep catch. The follower's pull set is derived from the hub's manifest, so the identical bad element arrived in the identical batch on every interval and nothing ever converged — logged as "sync failed", indistinguishable from a hub outage. Isolation now matches the Akka sibling's split exactly: WireToDomain stays a strict per-item parser and loses its two batch entry points (their absence is pinned by a reflection test), while GrpcSecretsHubClient converts each item inside its own try — the same shape as SecretReplicationActor.Materialize. A dropped item costs one warning naming the secret and a reason category, and every well-formed sibling still converges. Only parse-shaped exceptions are caught (ArgumentException, FormatException); a defect still propagates. A row whose own name is unparseable is logged under a marker rather than echoing the untrusted string, and no ciphertext, DEK or credential reaches a log line. GrpcSecretsHubClient had zero coverage — the existing fake implements the ISecretsHubReader seam directly, bypassing the header, the deadline and the conversion, which are the only things that class does. It is now driven for real through a fake CallInvoker handed to the generated client. The stale doc comment claiming the reconciler isolated the failure is replaced by one describing the mechanism that actually exists, and Endpoint gains a note that a plain-http hub sends the bearer token cleartext (doc only, no validation change). Tests 52 -> 67 in the package suite; solution green, no new warnings. Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1 --- .../SecretsGrpcHubClientExtensions.cs | 2 + .../GrpcSecretsHubClient.cs | 122 ++++++++- .../SecretsGrpcHubClientOptions.cs | 9 + .../Wire/WireToDomain.cs | 42 ++-- .../GrpcSecretSyncServiceTests.cs | 109 +++++--- .../GrpcSecretsHubClientTests.cs | 233 ++++++++++++++++++ .../TestDoubles.cs | 186 ++++++++++++++ .../WireToDomainTests.cs | 19 +- 8 files changed, 649 insertions(+), 73 deletions(-) create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretsHubClientTests.cs create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/TestDoubles.cs diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/DependencyInjection/SecretsGrpcHubClientExtensions.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/DependencyInjection/SecretsGrpcHubClientExtensions.cs index c896c45..8ad6847 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/DependencyInjection/SecretsGrpcHubClientExtensions.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/DependencyInjection/SecretsGrpcHubClientExtensions.cs @@ -2,6 +2,7 @@ using Grpc.Net.Client; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using ZB.MOM.WW.Secrets.Abstractions; using ZB.MOM.WW.Secrets.Replicator.Grpc.V1; @@ -85,6 +86,7 @@ public static class SecretsGrpcHubClientExtensions new SecretsHub.SecretsHubClient( sp.GetRequiredKeyedService(ChannelServiceKey)), sp.GetRequiredService>(), + sp.GetRequiredService>(), sp.GetService())); services.AddHostedService(); diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretsHubClient.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretsHubClient.cs index 8eef05d..7fa76f8 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretsHubClient.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretsHubClient.cs @@ -1,4 +1,5 @@ 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; @@ -29,7 +30,11 @@ internal interface ISecretsHubReader /// The hub's manifest entries. Task> GetManifestAsync(CancellationToken ct); - /// Fetches several encrypted rows by name; names absent from the hub are omitted. + /// + /// Fetches several encrypted rows by name. Names absent from the hub are omitted, and so is any + /// row the follower cannot parse — an implementation returns the well-formed rows rather than + /// failing the batch. + /// /// The names to fetch. /// A token to cancel the operation. /// The matching encrypted rows. @@ -42,33 +47,60 @@ internal interface ISecretsHubReader /// . /// /// -/// Faults are deliberately not caught here. An is the honest signal -/// that the hub did not answer, and the sweep — the only caller — is where that is turned into "keep -/// serving the last-known-good local store and retry next tick". Swallowing it here would make an -/// unreachable hub indistinguishable from an empty one, which is the one failure mode this +/// +/// Transport faults are deliberately not caught. An is the honest +/// signal that the hub did not answer, and the sweep — the only caller — is where that is turned +/// into "keep serving the last-known-good local store and retry next tick". Swallowing it here would +/// make an unreachable hub indistinguishable from an empty one, which is the one failure mode this /// subsystem must never have. +/// +/// +/// Parse faults are caught, per item. That is not the same decision: the hub answered, and +/// the answer is mostly good. Failing the whole response over one unparseable element would stall +/// convergence permanently rather than transiently — the follower's pull set comes from the hub's +/// manifest, so the identical bad element would arrive in the identical batch on every interval and +/// nothing would ever be applied, while the log said only "sync failed" exactly as it does for an +/// outage. So each item is converted inside its own try, one warning names what was dropped +/// and why, and every well-formed sibling is returned. This is the same containment the Akka +/// replicator applies in SecretReplicationActor.Materialize. +/// +/// +/// A dropped item is dropped whole and never coerced — the fail-closed half of the same rule. Only +/// parse-shaped exceptions are caught (, ); +/// anything else is a defect rather than bad input and is left to propagate. +/// /// -internal sealed class GrpcSecretsHubClient : ISecretsHubReader +internal sealed partial class GrpcSecretsHubClient : ISecretsHubReader { private const string AuthorizationHeader = "Authorization"; + /// + /// Stands in for a name in a log line when the name itself is what failed to parse. The hub's + /// raw string is deliberately not echoed: it is the untrusted value under suspicion. + /// + private const string UnnameableItem = "(unparseable name)"; + private readonly SecretsHub.SecretsHubClient _client; private readonly SecretsGrpcHubClientOptions _options; + private readonly ILogger _logger; private readonly TimeProvider _timeProvider; /// Creates the hub client. /// The generated gRPC client, bound to the configured channel. /// Endpoint, credential, and per-call deadline. + /// Logger for items the hub sent that this node could not parse. /// Clock used to compute the per-call deadline. public GrpcSecretsHubClient( SecretsHub.SecretsHubClient client, IOptions options, + ILogger logger, TimeProvider? timeProvider = null) { ArgumentNullException.ThrowIfNull(options); _client = client ?? throw new ArgumentNullException(nameof(client)); _options = options.Value; + _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _timeProvider = timeProvider ?? TimeProvider.System; } @@ -79,7 +111,23 @@ internal sealed class GrpcSecretsHubClient : ISecretsHubReader .GetManifestAsync(new GetManifestRequest(), CreateCallOptions(ct)) .ConfigureAwait(false); - return WireToDomain.ToManifest(response.Entries); + var entries = new List(response.Entries.Count); + + foreach (ManifestEntry wire in response.Entries) + { + try + { + entries.Add(WireToDomain.ToEntry(wire)); + } + catch (Exception ex) when (IsParseFailure(ex)) + { + // Dropping a manifest entry costs only that name's convergence: it is simply not in + // this sweep's pull set, and a corrected entry converges on the next one. + LogManifestEntryDiscarded(ex, Describe(wire.Name), Categorize(ex)); + } + } + + return entries; } /// @@ -99,7 +147,23 @@ internal sealed class GrpcSecretsHubClient : ISecretsHubReader .GetSecretsAsync(request, CreateCallOptions(ct)) .ConfigureAwait(false); - return WireToDomain.ToRows(response.Secrets); + var rows = new List(response.Secrets.Count); + + foreach (SecretRow wire in response.Secrets) + { + try + { + rows.Add(WireToDomain.ToRow(wire)); + } + catch (Exception ex) when (IsParseFailure(ex)) + { + // The reconciler treats a missing row exactly as it treats a name the hub no longer + // holds, so the batch applies its siblings and returns a smaller count. + LogRowDiscarded(ex, Describe(wire.Name), Categorize(ex)); + } + } + + return rows; } // Built per call rather than once: the deadline is an absolute instant, so a cached CallOptions @@ -108,4 +172,46 @@ internal sealed class GrpcSecretsHubClient : ISecretsHubReader headers: new Metadata { { AuthorizationHeader, $"Bearer {_options.BearerToken}" } }, deadline: _timeProvider.GetUtcNow().UtcDateTime.Add(_options.CallDeadline), cancellationToken: ct); + + // The exceptions WireToDomain raises for input the domain refuses: SecretName and the content + // type check throw ArgumentException, the round-trip timestamp parse throws FormatException. + // Narrow on purpose — a NullReferenceException or an OutOfMemoryException is a defect or a + // resource failure, and silently dropping rows over either would hide it behind a warning. + private static bool IsParseFailure(Exception exception) => + exception is ArgumentException or FormatException; + + // The name is the only field of an item that may reach a log sink, and only once the domain has + // accepted it. Nothing else is logged: a row is ciphertext, a wrapped DEK and two nonces, and a + // warning is not a place any of that belongs. + private static string Describe(string name) + { + try + { + return new SecretName(name).Value; + } + catch (ArgumentException) + { + return UnnameableItem; + } + } + + // A category rather than a restatement of the exception, so an operator reading the warning + // knows which authored field to go and look at. + private static string Categorize(Exception exception) => exception switch + { + FormatException => "malformed timestamp", + _ => "invalid name or content type", + }; + + [LoggerMessage( + EventId = 1, + Level = LogLevel.Warning, + Message = "Discarding a malformed manifest entry for {SecretName} sent by the secrets hub ({Reason}); every other entry is reconciled normally.")] + private partial void LogManifestEntryDiscarded(Exception exception, string secretName, string reason); + + [LoggerMessage( + EventId = 2, + Level = LogLevel.Warning, + Message = "Discarding a malformed secret row for {SecretName} sent by the secrets hub ({Reason}); every other row in the batch is applied normally.")] + private partial void LogRowDiscarded(Exception exception, string secretName, string reason); } diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/SecretsGrpcHubClientOptions.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/SecretsGrpcHubClientOptions.cs index 93a68b3..469ee3f 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/SecretsGrpcHubClientOptions.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/SecretsGrpcHubClientOptions.cs @@ -17,6 +17,15 @@ public sealed class SecretsGrpcHubClientOptions /// The hub's gRPC address, as an absolute http or https URI (for example /// https://central:5001). Required. /// + /// + /// A plain http endpoint is accepted so an h2c-only or in-cluster deployment can be + /// stood up, but it sends in cleartext on every call — anyone + /// on the path reads the hub credential and can then pull central's entire secret inventory. + /// Treat http as a development and loopback-only convenience; a site node reaching + /// central across a plant network must use https. This is documentation rather than a + /// rule the validator enforces, because "internal" is a property of a deployment's network and + /// cannot be decided from a URI string. + /// public string Endpoint { get; set; } = string.Empty; /// diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/Wire/WireToDomain.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/Wire/WireToDomain.cs index e662ab0..692377c 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/Wire/WireToDomain.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/Wire/WireToDomain.cs @@ -10,16 +10,24 @@ namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Wire; /// /// /// -/// Parsing here is strict, and rejection is per row: a value protobuf accepted but the domain +/// Parsing here is strict and one item at a time: a value protobuf accepted but the domain /// would not — a name that is rooted or path-traversing, a content type naming no enum member, a -/// timestamp that is not round-trip ISO-8601 — throws rather than being coerced. The sweep's -/// reconciler isolates the failure to that one row (see SecretReplicationReconciler.ApplyAsync), -/// so a single bad row is logged and skipped instead of poisoning the batch behind it. +/// timestamp that is not round-trip ISO-8601 — throws rather than being coerced. /// /// -/// This mirrors the Akka replicator's SecretRowDto.ToStoredSecret deliberately — one -/// fail-closed parse shape family-wide, so the two transports cannot disagree about what a -/// replicated row is allowed to contain. +/// There is deliberately no batch entry point here, and that absence is the design. A method +/// converting a whole response would fail the whole response on one bad element, and because a +/// follower's pull set is derived from the hub's manifest, the identical bad element would re-fail +/// the identical sweep on every interval forever — a permanent stall that reads in the log exactly +/// like a hub outage. Isolation therefore lives one layer up, in +/// GrpcSecretsHubClient, which converts each item inside its own try, logs one warning +/// naming the item, drops it, and returns every well-formed sibling. +/// +/// +/// That split mirrors the Akka replicator exactly — SecretRowDto.ToStoredSecret throws and +/// SecretReplicationActor.Materialize catches per row — so the two transports share one +/// fail-closed parse shape and one containment rule, and cannot disagree about what a replicated row +/// is allowed to contain. /// /// /// Two checks the Akka DTO makes are absent here on purpose. Protobuf cannot express a null @@ -32,16 +40,6 @@ namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Wire; /// internal static class WireToDomain { - /// Rebuilds a manifest from the wire form, in the order the hub sent it. - /// The wire manifest entries. - /// The domain manifest entries. - public static IReadOnlyList ToManifest(IEnumerable entries) - { - ArgumentNullException.ThrowIfNull(entries); - - return [.. entries.Select(ToEntry)]; - } - /// Rebuilds one manifest entry from the wire form. /// The wire manifest entry. /// The domain manifest entry. @@ -62,16 +60,6 @@ internal static class WireToDomain }; } - /// Rebuilds encrypted rows from the wire form, in the order the hub sent them. - /// The wire rows. - /// The domain rows. - public static IReadOnlyList ToRows(IEnumerable rows) - { - ArgumentNullException.ThrowIfNull(rows); - - return [.. rows.Select(ToRow)]; - } - /// Rebuilds one encrypted row from the wire form. /// The wire row. /// The domain row. diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretSyncServiceTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretSyncServiceTests.cs index c48e3cd..c4d396b 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretSyncServiceTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretSyncServiceTests.cs @@ -7,6 +7,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using ZB.MOM.WW.Secrets.Abstractions; using ZB.MOM.WW.Secrets.Replicator.Grpc.DependencyInjection; +using ZB.MOM.WW.Secrets.Replicator.Grpc.V1; using ZB.MOM.WW.Secrets.Sqlite; namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Tests; @@ -21,7 +22,8 @@ public sealed class GrpcSecretSyncServiceTests : IDisposable private readonly SqliteSecretStore _local; private readonly SqliteSecretStore _hubStore; private readonly FakeHubReader _hub; - private readonly RecordingLogger _logger = new(); + private readonly RecordingLogger _logger = new(); + private readonly RecordingLogger _clientLogger = new(); private readonly RecordingCacheInvalidator _cache = new(); public GrpcSecretSyncServiceTests() @@ -331,6 +333,81 @@ public sealed class GrpcSecretSyncServiceTests : IDisposable Assert.Equal(0, _hub.ManifestCallCount); } + [Fact] + public async Task A_malformed_row_never_poisons_the_batch_it_arrived_in() + { + // The failure this guards against is not "one row is lost" — it is that the pull set never + // changes, so the identical malformed row re-fails the identical sweep every interval and + // the node never converges on anything, indistinguishable in the log from a hub outage. + SecretRow bad = TestWire.Row("api/key", 0xBB); + bad.ContentType = "Kerberos"; + + var invoker = new FakeSecretsHubCallInvoker + { + ManifestResponse = new GetManifestResponse + { + Entries = + { + TestWire.Entry("db/password"), TestWire.Entry("api/key"), TestWire.Entry("svc/token"), + }, + }, + SecretsResponse = new GetSecretsResponse + { + Secrets = { TestWire.Row("db/password", 0xAA), bad, TestWire.Row("svc/token", 0xCC) }, + }, + }; + + int pulled = await CreateServiceOverTheWire(invoker).SweepAsync(CancellationToken.None); + + Assert.Equal(2, pulled); + Assert.NotNull(await _local.GetAsync(new SecretName("db/password"), CancellationToken.None)); + Assert.NotNull(await _local.GetAsync(new SecretName("svc/token"), CancellationToken.None)); + Assert.Null(await _local.GetAsync(new SecretName("api/key"), CancellationToken.None)); + Assert.Single(_clientLogger.Warnings); + } + + [Fact] + public async Task A_malformed_manifest_entry_never_poisons_the_manifest_it_arrived_in() + { + ManifestEntry bad = TestWire.Entry("api/key"); + bad.UpdatedUtc = "not-a-timestamp"; + + var invoker = new FakeSecretsHubCallInvoker + { + ManifestResponse = new GetManifestResponse + { + Entries = { TestWire.Entry("db/password"), bad, TestWire.Entry("svc/token") }, + }, + SecretsResponse = new GetSecretsResponse + { + Secrets = { TestWire.Row("db/password", 0xAA), TestWire.Row("svc/token", 0xCC) }, + }, + }; + + int pulled = await CreateServiceOverTheWire(invoker).SweepAsync(CancellationToken.None); + + Assert.Equal(2, pulled); + Assert.Equal(["db/password", "svc/token"], invoker.LastSecretsRequest!.Names); + Assert.Single(_clientLogger.Warnings); + } + + // A sweep wired to the REAL hub client over a fake CallInvoker, so the parse boundary the other + // tests bypass (the FakeHubReader hands over domain types already) is actually exercised. + private GrpcSecretSyncService CreateServiceOverTheWire(FakeSecretsHubCallInvoker invoker) + { + IOptions options = Options.Create(new SecretsGrpcHubClientOptions + { + Endpoint = "https://central:5001", + BearerToken = "hub-token", + SyncInterval = TimeSpan.FromHours(1), + }); + + var client = new GrpcSecretsHubClient( + new SecretsHub.SecretsHubClient(invoker), options, _clientLogger); + + return new GrpcSecretSyncService(_local, client, _cache, options, _logger); + } + [Fact] public void Registration_refuses_a_configuration_with_no_bearer_token() { @@ -471,34 +548,4 @@ public sealed class GrpcSecretSyncServiceTests : IDisposable public void Invalidate(SecretName name) => Invalidated.Add(name); } - private sealed class RecordingLogger : ILogger - { - public int InformationCount { get; private set; } - - public int WarningCount { get; private set; } - - 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) - { - switch (logLevel) - { - case LogLevel.Information: - InformationCount++; - break; - case LogLevel.Warning: - WarningCount++; - break; - default: - break; - } - } - } } diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretsHubClientTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretsHubClientTests.cs new file mode 100644 index 0000000..deb5a04 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/GrpcSecretsHubClientTests.cs @@ -0,0 +1,233 @@ +using Microsoft.Extensions.Options; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Replicator.Grpc.V1; + +namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Tests; + +/// +/// The real hub client, driven over a fake CallInvoker: what it puts on the wire (credential +/// and deadline) and what it does with what comes back (per-item conversion, per-item isolation). +/// +/// +/// Everything here is deliberately exercised through GrpcSecretsHubClient itself rather than +/// through a fake of the ISecretsHubReader seam. A seam fake proves nothing about the header, +/// the deadline, or the conversion — all three live only in this class, and all three are exactly +/// where a follower silently talks to the hub wrong. +/// +public sealed class GrpcSecretsHubClientTests +{ + private static readonly DateTimeOffset Now = new(2026, 8, 7, 12, 0, 0, TimeSpan.Zero); + + private readonly FakeSecretsHubCallInvoker _invoker = new(); + private readonly RecordingLogger _logger = new(); + + private readonly SecretsGrpcHubClientOptions _options = new() + { + Endpoint = "https://central:5001", + BearerToken = "hub-token", + CallDeadline = TimeSpan.FromSeconds(17), + }; + + private GrpcSecretsHubClient CreateClient() => new( + new SecretsHub.SecretsHubClient(_invoker), + Options.Create(_options), + _logger, + new FixedTimeProvider(Now)); + + [Fact] + public async Task Every_call_presents_the_bearer_credential() + { + _invoker.ManifestResponse = new GetManifestResponse { Entries = { TestWire.Entry("db/password") } }; + _invoker.SecretsResponse = new GetSecretsResponse { Secrets = { TestWire.Row("db/password", 0xAA) } }; + + GrpcSecretsHubClient client = CreateClient(); + + await client.GetManifestAsync(CancellationToken.None); + await client.GetManyAsync([new SecretName("db/password")], CancellationToken.None); + + Assert.Equal(["GetManifest", "GetSecrets"], _invoker.Methods); + Assert.Equal("Bearer hub-token", _invoker.AuthorizationOn(0)); + Assert.Equal("Bearer hub-token", _invoker.AuthorizationOn(1)); + } + + [Fact] + public async Task Every_call_carries_a_deadline_of_now_plus_the_configured_budget() + { + await CreateClient().GetManifestAsync(CancellationToken.None); + + Assert.Equal(Now.UtcDateTime.AddSeconds(17), _invoker.Calls[0].Deadline); + } + + [Fact] + public async Task The_deadline_is_recomputed_per_call_rather_than_cached() + { + // A CallOptions built once at construction would carry an absolute deadline that expires + // shortly after startup and then fails every sweep forever, which reads as a hub outage. + GrpcSecretsHubClient client = CreateClient(); + + await client.GetManifestAsync(CancellationToken.None); + await client.GetManifestAsync(CancellationToken.None); + + Assert.Equal(_invoker.Calls[0].Deadline, _invoker.Calls[1].Deadline); + Assert.NotSame(_invoker.Calls[0].Headers, _invoker.Calls[1].Headers); + } + + [Fact] + public async Task The_fetch_asks_for_exactly_the_names_it_was_given() + { + _invoker.SecretsResponse = new GetSecretsResponse(); + + await CreateClient().GetManyAsync( + [new SecretName("db/password"), new SecretName("api/key")], CancellationToken.None); + + Assert.Equal(["db/password", "api/key"], _invoker.LastSecretsRequest!.Names); + } + + [Fact] + public async Task A_wire_row_comes_back_as_a_domain_row() + { + _invoker.SecretsResponse = new GetSecretsResponse { Secrets = { TestWire.Row("db/password", 0xAA) } }; + + IReadOnlyList rows = + await CreateClient().GetManyAsync([new SecretName("db/password")], CancellationToken.None); + + StoredSecret row = Assert.Single(rows); + Assert.Equal("db/password", row.Name.Value); + Assert.Equal(SecretContentType.Text, row.ContentType); + Assert.Equal(new byte[] { 0xAA }, row.Ciphertext); + Assert.Equal(TestWire.FixedInstant, row.UpdatedUtc); + } + + [Fact] + public async Task A_wire_manifest_entry_comes_back_as_a_domain_entry() + { + _invoker.ManifestResponse = new GetManifestResponse { Entries = { TestWire.Entry("api/key") } }; + + IReadOnlyList entries = + await CreateClient().GetManifestAsync(CancellationToken.None); + + SecretManifestEntry entry = Assert.Single(entries); + Assert.Equal("api/key", entry.Name.Value); + Assert.Equal(TestWire.FixedInstant, entry.UpdatedUtc); + } + + [Fact] + public async Task A_malformed_row_is_dropped_and_its_well_formed_siblings_survive() + { + // The whole point: one unparseable row must not cost the batch. Eagerly materializing the + // response would fail all three, and the identical pull set would re-fail every interval. + SecretRow bad = TestWire.Row("api/key", 0xBB); + bad.ContentType = "Kerberos"; + + _invoker.SecretsResponse = new GetSecretsResponse + { + Secrets = { TestWire.Row("db/password", 0xAA), bad, TestWire.Row("svc/token", 0xCC) }, + }; + + IReadOnlyList rows = await CreateClient().GetManyAsync( + [new SecretName("db/password"), new SecretName("api/key"), new SecretName("svc/token")], + CancellationToken.None); + + Assert.Equal(["db/password", "svc/token"], rows.Select(r => r.Name.Value)); + } + + [Fact] + public async Task A_dropped_row_costs_exactly_one_warning_that_names_it() + { + SecretRow bad = TestWire.Row("api/key", 0xBB); + bad.ContentType = "Kerberos"; + + _invoker.SecretsResponse = new GetSecretsResponse + { + Secrets = { TestWire.Row("db/password", 0xAA), bad }, + }; + + await CreateClient().GetManyAsync( + [new SecretName("db/password"), new SecretName("api/key")], CancellationToken.None); + + string warning = Assert.Single(_logger.Warnings); + Assert.Contains("api/key", warning, StringComparison.Ordinal); + } + + [Fact] + public async Task A_dropped_row_never_logs_the_material_it_was_carrying() + { + // A parse failure is an operations problem, so it is logged — but a replicated row is + // ciphertext plus its wrapped DEK, and none of that may reach a log sink. + SecretRow bad = TestWire.Row("api/key", 0xBB); + bad.UpdatedUtc = "yesterday"; + + _invoker.SecretsResponse = new GetSecretsResponse { Secrets = { bad } }; + + await CreateClient().GetManyAsync([new SecretName("api/key")], CancellationToken.None); + + string warning = Assert.Single(_logger.Warnings); + + foreach (string forbidden in new[] { "sha256:test", "hub-token", "Ciphertext", "WrappedDek" }) + { + Assert.DoesNotContain(forbidden, warning, StringComparison.OrdinalIgnoreCase); + } + } + + [Fact] + public async Task A_row_whose_own_name_is_unparseable_is_dropped_without_echoing_it() + { + // The name is the one field a warning may legitimately carry — but here the name IS what + // failed to parse, so it is replaced by a marker rather than trusted into the log line. + SecretRow bad = TestWire.Row("db/password", 0xAA); + bad.Name = "../../etc/passwd"; + + _invoker.SecretsResponse = new GetSecretsResponse { Secrets = { bad } }; + + IReadOnlyList rows = + await CreateClient().GetManyAsync([new SecretName("db/password")], CancellationToken.None); + + Assert.Empty(rows); + + string warning = Assert.Single(_logger.Warnings); + Assert.DoesNotContain("etc/passwd", warning, StringComparison.Ordinal); + } + + [Fact] + public async Task A_malformed_manifest_entry_is_dropped_and_the_rest_of_the_manifest_survives() + { + ManifestEntry bad = TestWire.Entry("api/key"); + bad.UpdatedUtc = "not-a-timestamp"; + + _invoker.ManifestResponse = new GetManifestResponse + { + Entries = { TestWire.Entry("db/password"), bad, TestWire.Entry("svc/token") }, + }; + + IReadOnlyList entries = + await CreateClient().GetManifestAsync(CancellationToken.None); + + Assert.Equal(["db/password", "svc/token"], entries.Select(e => e.Name.Value)); + Assert.Single(_logger.Warnings); + } + + [Fact] + public async Task Conversion_preserves_the_order_the_hub_sent() + { + _invoker.SecretsResponse = new GetSecretsResponse + { + Secrets = { TestWire.Row("db/password", 0xAA), TestWire.Row("api/key", 0xBB) }, + }; + + IReadOnlyList rows = await CreateClient().GetManyAsync( + [new SecretName("db/password"), new SecretName("api/key")], CancellationToken.None); + + Assert.Equal(["db/password", "api/key"], rows.Select(r => r.Name.Value)); + } + + [Fact] + public async Task A_well_formed_response_logs_nothing() + { + _invoker.ManifestResponse = new GetManifestResponse { Entries = { TestWire.Entry("db/password") } }; + + await CreateClient().GetManifestAsync(CancellationToken.None); + + Assert.Empty(_logger.Warnings); + Assert.Empty(_logger.Information); + } +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/TestDoubles.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/TestDoubles.cs new file mode 100644 index 0000000..ff5d757 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/TestDoubles.cs @@ -0,0 +1,186 @@ +using Google.Protobuf; +using Grpc.Core; +using Microsoft.Extensions.Logging; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Replicator.Grpc.V1; + +namespace ZB.MOM.WW.Secrets.Replicator.Grpc.Tests; + +/// +/// A that answers the two hub RPCs from canned responses and records what +/// the client sent. +/// +/// +/// This is what lets the tests drive the real GrpcSecretsHubClient — the generated +/// client accepts a , so substituting one exercises the header, the +/// deadline and the wire-to-domain conversion without a live server. A fake of the +/// ISecretsHubReader seam would bypass every one of those. +/// +internal sealed class FakeSecretsHubCallInvoker : CallInvoker +{ + /// Response returned for GetManifest. + public GetManifestResponse ManifestResponse { get; set; } = new(); + + /// Response returned for GetSecrets. + public GetSecretsResponse SecretsResponse { get; set; } = new(); + + /// The options every call was invoked with, in call order. + public List Calls { get; } = []; + + /// The RPC names invoked, in call order. + public List Methods { get; } = []; + + /// The most recent GetSecrets request, so tests can assert what was asked for. + public GetSecretsRequest? LastSecretsRequest { get; private set; } + + /// Returns the Authorization header value seen on the call at the given index. + /// Zero-based call index. + /// The header value, or when the header was not sent. + public string? AuthorizationOn(int index) => + Calls[index].Headers?.GetValue("authorization"); + + /// + public override AsyncUnaryCall AsyncUnaryCall( + Method method, string? host, CallOptions options, TRequest request) + { + ArgumentNullException.ThrowIfNull(method); + + Calls.Add(options); + Methods.Add(method.Name); + + if (request is GetSecretsRequest secretsRequest) + { + LastSecretsRequest = secretsRequest; + } + + object response = method.Name switch + { + "GetManifest" => ManifestResponse, + "GetSecrets" => SecretsResponse, + _ => throw new InvalidOperationException($"unexpected RPC '{method.Name}'."), + }; + + return new AsyncUnaryCall( + Task.FromResult((TResponse)response), + Task.FromResult(new Metadata()), + () => Status.DefaultSuccess, + () => new Metadata(), + () => { }); + } + + /// + public override TResponse BlockingUnaryCall( + Method method, string? host, CallOptions options, TRequest request) => + throw new NotSupportedException("The hub client only makes async unary calls."); + + /// + public override AsyncServerStreamingCall AsyncServerStreamingCall( + Method method, string? host, CallOptions options, TRequest request) => + throw new NotSupportedException("The hub contract has no streaming RPC."); + + /// + public override AsyncClientStreamingCall AsyncClientStreamingCall( + Method method, string? host, CallOptions options) => + throw new NotSupportedException("The hub contract has no streaming RPC."); + + /// + public override AsyncDuplexStreamingCall AsyncDuplexStreamingCall( + Method method, string? host, CallOptions options) => + throw new NotSupportedException("The hub contract has no streaming RPC."); +} + +/// A clock frozen at a caller-chosen instant, so a deadline assertion can be exact. +/// The instant every read returns. +internal sealed class FixedTimeProvider(DateTimeOffset now) : TimeProvider +{ + /// + public override DateTimeOffset GetUtcNow() => now; +} + +/// Captures level and formatted message for every log entry. +/// The logger category. +internal sealed class RecordingLogger : ILogger +{ + /// Formatted messages logged at . + public List Information { get; } = []; + + /// Formatted messages logged at . + public List Warnings { get; } = []; + + /// Count of information entries. + public int InformationCount => Information.Count; + + /// Count of warning entries. + public int WarningCount => Warnings.Count; + + /// + 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); + + string message = formatter(state, exception); + + switch (logLevel) + { + case LogLevel.Information: + Information.Add(message); + break; + case LogLevel.Warning: + Warnings.Add(message); + break; + default: + break; + } + } +} + +/// Builders for well-formed wire messages that individual tests then damage. +internal static class TestWire +{ + /// A well-formed wire row. + /// The secret name. + /// A byte written into the ciphertext so the row is identifiable. + /// The wire row. + public static SecretRow Row(string name, byte marker) => new() + { + Name = name, + ContentType = nameof(SecretContentType.Text), + Ciphertext = ByteString.CopyFrom(marker), + Nonce = ByteString.CopyFrom((byte)1), + Tag = ByteString.CopyFrom((byte)2), + WrappedDek = ByteString.CopyFrom((byte)3), + WrapNonce = ByteString.CopyFrom((byte)4), + WrapTag = ByteString.CopyFrom((byte)5), + KekId = "sha256:test", + Revision = 0, + IsDeleted = false, + CreatedUtc = FixedInstant.ToString("O"), + UpdatedUtc = FixedInstant.ToString("O"), + }; + + /// A well-formed manifest entry matching . + /// The secret name. + /// The wire manifest entry. + public static ManifestEntry Entry(string name) => new() + { + Name = name, + Revision = 0, + UpdatedUtc = FixedInstant.ToString("O"), + IsDeleted = false, + }; + + /// The timestamp every builder stamps, so rows differ only where a test makes them differ. + public static readonly DateTimeOffset FixedInstant = + new(2026, 8, 7, 1, 2, 3, TimeSpan.Zero); +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/WireToDomainTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/WireToDomainTests.cs index acbac6d..da94346 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/WireToDomainTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.Grpc.Tests/WireToDomainTests.cs @@ -1,3 +1,4 @@ +using System.Reflection; using Google.Protobuf; using ZB.MOM.WW.Secrets.Abstractions; using ZB.MOM.WW.Secrets.Replicator.Grpc.V1; @@ -192,14 +193,18 @@ public sealed class WireToDomainTests } [Fact] - public void Batch_conversions_preserve_order() + public void There_is_no_batch_entry_point_to_convert_a_whole_response_with() { - SecretRow first = FullRow(); - SecretRow second = FullRow(); - second.Name = "api/key"; + // A batch method here would fail a whole response on one bad element, and since the pull set + // is derived from the hub's manifest the identical bad element would re-fail the identical + // sweep forever. Isolation belongs to GrpcSecretsHubClient, which converts item by item, so + // this pins that the strict parser offers nothing that could poison a batch again. + string[] members = typeof(WireToDomain) + .GetMethods(BindingFlags.Public | BindingFlags.Static | BindingFlags.DeclaredOnly) + .Select(m => m.Name) + .Order(StringComparer.Ordinal) + .ToArray(); - IReadOnlyList rows = WireToDomain.ToRows([first, second]); - - Assert.Equal(["db/password", "api/key"], rows.Select(r => r.Name.Value)); + Assert.Equal(["ToEntry", "ToRow"], members); } }