diff --git a/ZB.MOM.WW.Auth/README.md b/ZB.MOM.WW.Auth/README.md index ca2d5cf..fa430b4 100644 --- a/ZB.MOM.WW.Auth/README.md +++ b/ZB.MOM.WW.Auth/README.md @@ -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 diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Abstractions/ApiKeys/ApiKeyContracts.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Abstractions/ApiKeys/ApiKeyContracts.cs index 3a2b75e..f2a803f 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Abstractions/ApiKeys/ApiKeyContracts.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Abstractions/ApiKeys/ApiKeyContracts.cs @@ -8,7 +8,35 @@ public sealed record ApiKeyOptions public bool RunMigrationsOnStartup { get; init; } = true; } -public enum ApiKeyFailure { MissingOrMalformed, KeyNotFound, KeyRevoked, PepperUnavailable, SecretMismatch, KeyExpired } +/// Why an API-key verification failed. New values are only ever appended, so stored or logged numeric values keep their meaning. +public enum ApiKeyFailure +{ + /// No credential, or one that does not parse as <prefix>_<keyId>_<secret>. + MissingOrMalformed, + + /// No key with the presented key id exists. + KeyNotFound, + + /// The key exists but has been revoked. + KeyRevoked, + + /// The pepper could not be resolved, so no key can be verified. + PepperUnavailable, + + /// The presented secret does not match the stored hash. + SecretMismatch, + + /// The key exists but its absolute expiry has passed. + KeyExpired, + + /// + /// 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 VerifyAsync. A host may answer it with 503 + /// rather than 401, since the caller did nothing wrong. + /// + StoreUnavailable, +} public sealed record ApiKeyIdentity(string KeyId, string DisplayName, IReadOnlySet Scopes, object? Constraints); diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs index 95ea7ff..c17d825 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs @@ -10,7 +10,8 @@ namespace ZB.MOM.WW.Auth.ApiKeys; /// /// 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 +/// (since 0.4.4), and a failed last-used write is ignored. A successful identity carries the key's scopes and the /// opaque ConstraintsJson blob (which the verifier does not interpret); it never carries the /// presented secret, the pepper, or the stored secret hash. /// @@ -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); diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests/SqlServerUnreachableVerifierTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests/SqlServerUnreachableVerifierTests.cs new file mode 100644 index 0000000..3eff742 --- /dev/null +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests/SqlServerUnreachableVerifierTests.cs @@ -0,0 +1,34 @@ +using ZB.MOM.WW.Auth.Abstractions.ApiKeys; + +namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests; + +/// +/// 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 VerifyAsync as a +/// structured , not as a SqlException that +/// surfaces as an unaudited 500. Offline: it dials a refused local port. +/// +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); + } +} diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyVerifierTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyVerifierTests.cs index f2b6a82..c0be807 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyVerifierTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.Tests/ApiKeyVerifierTests.cs @@ -356,6 +356,45 @@ public class ApiKeyVerifierTests // --- Cancellation --- + // --- StoreUnavailable (Auth-010, scadaproj#46) --- + + public static TheoryData 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( + () => verifier.VerifyAsync(Header(KeyId, Secret), CancellationToken.None)); + } + [Fact] public async Task VerifyAsync_AlreadyCancelled_Throws() { @@ -398,10 +437,13 @@ public class ApiKeyVerifierTests /// When set, throws this exception (after recording the call). public Exception? MarkUsedException { get; set; } + /// When set, throws this exception (after recording the call). + public Exception? FindException { get; set; } + public Task FindByKeyIdAsync(string keyId, CancellationToken ct) { FindByKeyIdCalled = true; - return Task.FromResult(Record); + return FindException is not null ? Task.FromException(FindException) : Task.FromResult(Record); } public Task FindActiveByKeyIdAsync(string keyId, CancellationToken ct) => diff --git a/code-reviews/Auth/findings.md b/code-reviews/Auth/findings.md index 17a31a7..7392fbf 100644 --- a/code-reviews/Auth/findings.md +++ b/code-reviews/Auth/findings.md @@ -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`.