fix(auth): ApiKeyVerifier reports store faults as StoreUnavailable (Auth-010)

A key-store lookup that threw (shared SQL Server unreachable, SQLite I/O fault) escaped VerifyAsync despite the "only exception path is cancellation" contract, surfacing as an unaudited 500 / gRPC Unknown. ApiKeyFailure gains StoreUnavailable (appended; numeric values unchanged) and the lookup maps any non-cancellation exception to it. Unit theory and an offline refused-port SqlServerApiKeyStore test fail pre-fix. (scadaproj#46) Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
2026-11-09 15:21:00 -05:00
parent b8d3fa2ba1
commit c4366539ca
6 changed files with 153 additions and 6 deletions
@@ -45,6 +45,7 @@ All five packages are versioned **lockstep** from `Directory.Build.props`. The c
  - where the directory computes `msDS-PrincipalName` for the entry (a base read; AD's `DOMAIN\name`, derived from `objectSid`), it must equal the Who-am-I answer, domain included;
  - otherwise the name must have exactly one holder under `SearchBase`, and it must be the entry found — no extra search is made when `UserNameAttribute` is `sAMAccountName` and the user typed the bound name, because the user search already proved that. Two holders → `GroupLookupFailed`.
  - A single-domain AD (BTDB Manager) is unaffected. Not yet live-verified against AD — the fake connection covers the paths; GLAuth has no Who-am-I.
+- **`ApiKeyFailure.StoreUnavailable` (Auth-010, `Abstractions` addition).** `ApiKeyVerifier.VerifyAsync` no longer lets a key-store fault escape: a `FindByKeyIdAsync` that throws (shared SQL Server unreachable, a SQLite I/O fault) returns `Succeeded = false` with the new `StoreUnavailable` failure, so it takes the host's normal — audited — auth-failure path instead of an unhandled 500 / gRPC `Unknown`. Cancellation still throws. The value is appended, so existing numeric values are unchanged; a host may answer it with 503 rather than 401. A misconfigured `SqlServerApiKeyOptions` still throws when the store is first resolved (outside `VerifyAsync`).
 - **Canonical username from a multi-valued attribute (Auth-009).** `LdapAuthResult.Username` is now the `UserNameAttribute` value that equals the typed name ignoring case — `cn: [John Smith, jsmith]` signs `jsmith` in as `jsmith`, not whichever value the server lists first. The first value is used only when none matches. Both bind modes.

 ### 0.4.1 — LDAP review fixes
@@ -8,7 +8,35 @@ public sealed record ApiKeyOptions
    public bool RunMigrationsOnStartup { get; init; } = true;
 }

-public enum ApiKeyFailure { MissingOrMalformed, KeyNotFound, KeyRevoked, PepperUnavailable, SecretMismatch, KeyExpired }
+/// <summary>Why an API-key verification failed. New values are only ever appended, so stored or logged numeric values keep their meaning.</summary>
+public enum ApiKeyFailure
+{
+    /// <summary>No credential, or one that does not parse as <c>&lt;prefix&gt;_&lt;keyId&gt;_&lt;secret&gt;</c>.</summary>
+    MissingOrMalformed,
+
+    /// <summary>No key with the presented key id exists.</summary>
+    KeyNotFound,
+
+    /// <summary>The key exists but has been revoked.</summary>
+    KeyRevoked,
+
+    /// <summary>The pepper could not be resolved, so no key can be verified.</summary>
+    PepperUnavailable,
+
+    /// <summary>The presented secret does not match the stored hash.</summary>
+    SecretMismatch,
+
+    /// <summary>The key exists but its absolute expiry has passed.</summary>
+    KeyExpired,
+
+    /// <summary>
+    /// The key store could not be queried (database unreachable, misconfigured, I/O fault), so the
+    /// credential could be neither accepted nor rejected on its merits. Added in 0.4.4 (Auth-010);
+    /// before it, the store's exception escaped <c>VerifyAsync</c>. A host may answer it with 503
+    /// rather than 401, since the caller did nothing wrong.
+    /// </summary>
+    StoreUnavailable,
+}

 public sealed record ApiKeyIdentity(string KeyId, string DisplayName, IReadOnlySet<string> Scopes, object? Constraints);

@@ -10,7 +10,8 @@ namespace ZB.MOM.WW.Auth.ApiKeys;
 /// <remarks>
 /// The failure reason is discriminated for the caller/audit pipeline, but the verifier returns a
 /// structured result rather than throwing (the caller decides the opaque client-facing message).
-/// The only exception path is cancellation. A successful identity carries the key's scopes and the
+/// The only exception path is cancellation: a key-store lookup that faults is reported as
+/// <see cref="ApiKeyFailure.StoreUnavailable"/> (since 0.4.4), and a failed last-used write is ignored. A successful identity carries the key's scopes and the
 /// opaque <c>ConstraintsJson</c> blob (which the verifier does not interpret); it never carries the
 /// presented secret, the pepper, or the stored secret hash.
 /// </remarks>
@@ -44,7 +45,24 @@ public sealed class ApiKeyVerifier(
        }

        // 3. Look up the record (including revoked ones) so we can discriminate not-found vs revoked.
-        ApiKeyRecord? record = await store.FindByKeyIdAsync(parsed.KeyId, ct).ConfigureAwait(false);
+        //    A store fault is a structured StoreUnavailable failure, not an exception (Auth-010): the
+        //    store may be a shared network database, so "unreachable" is an operating condition, and
+        //    consumers map Succeeded == false to their auth-failure path (and audit it) — an escaping
+        //    SqlException became an unaudited 500 / gRPC Unknown instead. Cancellation still throws.
+        ApiKeyRecord? record;
+        try
+        {
+            record = await store.FindByKeyIdAsync(parsed.KeyId, ct).ConfigureAwait(false);
+        }
+        catch (OperationCanceledException)
+        {
+            throw;
+        }
+        catch (Exception)
+        {
+            return Fail(ApiKeyFailure.StoreUnavailable);
+        }
+
        if (record is null)
        {
            return Fail(ApiKeyFailure.KeyNotFound);
@@ -0,0 +1,34 @@
+using ZB.MOM.WW.Auth.Abstractions.ApiKeys;
+
+namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests;
+
+/// <summary>
+/// Auth-010 (scadaproj#46), end to end through the real SQL Server store with no server: the shared
+/// store being unreachable (failover, lost route) must come back from <c>VerifyAsync</c> as a
+/// structured <see cref="ApiKeyFailure.StoreUnavailable"/>, not as a <c>SqlException</c> that
+/// surfaces as an unaudited 500. Offline: it dials a refused local port.
+/// </summary>
+public sealed class SqlServerUnreachableVerifierTests
+{
+    private sealed class Pepper : IApiKeyPepperProvider
+    {
+        public string? GetPepper() => "0123456789abcdef";
+    }
+
+    [Fact]
+    public async Task VerifyAsync_AgainstAnUnreachableSqlServer_ReturnsStoreUnavailable()
+    {
+        var factory = new AuthSqlServerConnectionFactory(new SqlServerApiKeyOptions
+        {
+            // Port 1 on loopback refuses immediately; no retries, so the test stays fast.
+            ConnectionString = "Server=tcp:127.0.0.1,1;Connect Timeout=3;ConnectRetryCount=0;TrustServerCertificate=true",
+        });
+        var verifier = new ApiKeyVerifier(
+            new ApiKeyOptions { TokenPrefix = "sbk" }, new SqlServerApiKeyStore(factory), new Pepper());
+
+        var result = await verifier.VerifyAsync("sbk_line1_supersecretvalue", CancellationToken.None);
+
+        Assert.False(result.Succeeded);
+        Assert.Equal(ApiKeyFailure.StoreUnavailable, result.Failure);
+    }
+}
@@ -356,6 +356,45 @@ public class ApiKeyVerifierTests

    // --- Cancellation ---

+    // --- StoreUnavailable (Auth-010, scadaproj#46) ---
+
+    public static TheoryData<Exception> StoreFaults() => new()
+    {
+        // Stand-ins for what real stores throw: a database fault (SqlClient's SqlException on a
+        // closed port or failover), an InvalidOperationException (e.g. a pooled connection in a bad
+        // state), and an I/O fault from a local SQLite file.
+        new System.Data.DataException("store unreachable"),
+        new InvalidOperationException("The connection is in a broken state."),
+        new IOException("disk I/O error"),
+    };
+
+    [Theory]
+    [MemberData(nameof(StoreFaults))]
+    public async Task VerifyAsync_StoreLookupThrows_ReturnsStoreUnavailable_InsteadOfThrowing(Exception fault)
+    {
+        var store = new FakeApiKeyStore { FindException = fault };
+        var verifier = BuildVerifier(store, new FakePepperProvider(Pepper));
+
+        ApiKeyVerification result = await verifier.VerifyAsync(Header(KeyId, Secret), CancellationToken.None);
+
+        Assert.False(result.Succeeded);
+        Assert.Equal(ApiKeyFailure.StoreUnavailable, result.Failure);
+        Assert.Null(result.Identity);
+        Assert.True(store.FindByKeyIdCalled);
+        Assert.False(store.MarkUsedCalled);
+    }
+
+    [Fact]
+    public async Task VerifyAsync_StoreLookupCancelled_Propagates()
+    {
+        // Cancellation stays the one exception path.
+        var store = new FakeApiKeyStore { FindException = new OperationCanceledException() };
+        var verifier = BuildVerifier(store, new FakePepperProvider(Pepper));
+
+        await Assert.ThrowsAnyAsync<OperationCanceledException>(
+            () => verifier.VerifyAsync(Header(KeyId, Secret), CancellationToken.None));
+    }
+
    [Fact]
    public async Task VerifyAsync_AlreadyCancelled_Throws()
    {
@@ -398,10 +437,13 @@ public class ApiKeyVerifierTests
        /// <summary>When set, <see cref="MarkUsedAsync"/> throws this exception (after recording the call).</summary>
        public Exception? MarkUsedException { get; set; }

+        /// <summary>When set, <see cref="FindByKeyIdAsync"/> throws this exception (after recording the call).</summary>
+        public Exception? FindException { get; set; }
+
        public Task<ApiKeyRecord?> FindByKeyIdAsync(string keyId, CancellationToken ct)
        {
            FindByKeyIdCalled = true;
-            return Task.FromResult(Record);
+            return FindException is not null ? Task.FromException<ApiKeyRecord?>(FindException) : Task.FromResult(Record);
        }

        public Task<ApiKeyRecord?> FindActiveByKeyIdAsync(string keyId, CancellationToken ct) =>
@@ -10,7 +10,7 @@
 | Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) |
 | Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) |
 | Commit reviewed | `91fb2b4` |
-| Open findings | 2 |
+| Open findings | 1 |

 ## Summary

@@ -475,7 +475,7 @@ Tests (`LdapAuthServiceTests`):
 | Severity | Low |
 | Priority | P3 |
 | Category | Error handling & resilience |
-| Status | Open |
+| Status | Resolved |
 | Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs:47` (contract stated at `:13`) |

 **Description**
@@ -502,3 +502,27 @@ Wrap the lookup and map non-cancellation exceptions to a structured failure, for
 `ApiKeyFailure.StoreUnavailable`. Otherwise, correct the remark to say store exceptions propagate. Add a test
 where the store throws. Optionally add `SqlServerApiKeyOptions` startup validation
 (`AddOptions().Validate(...).ValidateOnStart()`), mirroring Auth-001.
+
+**Resolution**
+
+Resolved 2026-11-09 (scadaproj#46, Auth `0.4.4`, unpublished). Re-verified offline: an `ApiKeyVerifier` over a real
+`SqlServerApiKeyStore` that dials a refused loopback port threw `Microsoft.Data.SqlClient.SqlException` out of
+`VerifyAsync`. The fix takes the review's first option. `ApiKeyFailure` gains **`StoreUnavailable`**, appended last so
+the existing numeric values are unchanged. `VerifyAsync` now wraps the `FindByKeyIdAsync` lookup: an
+`OperationCanceledException` is rethrown, and any other exception returns `Fail(StoreUnavailable)`. The class remark
+("the only exception path is cancellation") is now true, and it names the new failure.
+
+Consumers already map `Succeeded == false` to their auth-failure path and audit it, so a store outage becomes an
+audited auth failure instead of an unaudited 500 or gRPC `Unknown`. A host that wants 503 can switch on the new value.
+MxAccessGateway's `CachingApiKeyVerifier` caches only successes, so a transient `StoreUnavailable` is never cached.
+
+The optional `SqlServerApiKeyOptions` `ValidateOnStart` was **not** added. Option validation still happens in the
+`AuthSqlServerConnectionFactory` constructor, so a misconfigured section still throws when the store is first
+resolved, which is outside `VerifyAsync`. Validating at start could stop an existing host that registers the package
+without configuring the section from booting. Left as a follow-up candidate.
+
+Tests:
+- `ApiKeyVerifierTests.VerifyAsync_StoreLookupThrows_ReturnsStoreUnavailable_InsteadOfThrowing` is a theory over a
+  `DataException`, an `InvalidOperationException` and an `IOException`. All three cases fail pre-fix.
+- `VerifyAsync_StoreLookupCancelled_Propagates` is a guard.
+- `SqlServerUnreachableVerifierTests` (offline, refused loopback port) fails pre-fix with the `SqlException`.