diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs index b60c5617..00f0e8fb 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs @@ -71,7 +71,21 @@ public static class StartupValidator "required for Central") .Require("ScadaBridge:Security:JwtSigningKey", _ => !string.IsNullOrEmpty(configuration.GetSection("ScadaBridge:Security")["JwtSigningKey"]), - "required for Central")) + "required for Central") + // Review #4 (fail-fast pepper validation): the inbound API-key pepper + // backs the peppered-HMAC secret compare in the shared + // ZB.MOM.WW.Auth.ApiKeys verifier (wired by AddZbApiKeyAuth at the + // Central composition root). A missing or too-short pepper does not + // fault at boot — the verifier just fails every secret compare, so the + // inbound API silently serves 401s to otherwise-valid keys. Validate it + // here (Central-only, pre-host) so a misconfigured pepper fails fast at + // startup with a clear message instead of as a runtime auth blackout. + // The Require predicate receives config[key] directly; the >=16-char + // floor matches the test pepper's minimum and the secret-strength + // baseline used elsewhere. + .Require("ScadaBridge:InboundApi:ApiKeyPepper", + value => !string.IsNullOrEmpty(value) && value.Length >= 16, + "is required and must be at least 16 characters for Central (backs the inbound API-key peppered-HMAC verifier)")) // SeedNodes count (unconditional, after SiteId). .Require("ScadaBridge:Cluster:SeedNodes", _ => seedNodes != null && seedNodes.Count >= 2, diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs index fe34b238..d7a82ef4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs @@ -12,6 +12,19 @@ using ZB.MOM.WW.ScadaBridge.InboundAPI.Middleware; namespace ZB.MOM.WW.ScadaBridge.InboundAPI; +/// +/// Review N1: log-category marker for the inbound API endpoint. Used as the type +/// argument to so inbound-API auth/authz log +/// lines are categorized under this ScadaBridge.InboundAPI type rather than under +/// the shared ZB.MOM.WW.Auth library's IApiKeyVerifier. Exists only +/// to name the log category ( is static and cannot +/// be a generic type argument); it is never instantiated. +/// +public sealed class InboundApiEndpoint +{ + private InboundApiEndpoint() { } +} + /// /// WP-1: POST /api/{methodName} endpoint registration. /// WP-2: Method routing and parameter validation. @@ -53,7 +66,13 @@ public static class EndpointExtensions HttpContext httpContext, string methodName) { - var logger = httpContext.RequestServices.GetRequiredService>(); + // Review N1: log under an endpoint-owned category (a marker type in the + // ScadaBridge.InboundAPI namespace) rather than ILogger, + // so inbound-API auth log lines land under ScadaBridge — not the shared + // ZB.MOM.WW.Auth library namespace — when operators filter by category. + // (EndpointExtensions is static and cannot be an ILogger type argument, so + // the namespace-local InboundApiEndpoint marker carries the category.) + var logger = httpContext.RequestServices.GetRequiredService>(); var verifier = httpContext.RequestServices.GetRequiredService(); var repository = httpContext.RequestServices.GetRequiredService(); var executor = httpContext.RequestServices.GetRequiredService(); @@ -92,13 +111,30 @@ public static class EndpointExtensions // Auth re-arch (A+B), enumeration-safety: "method not found" and "key not // in scope for this method" MUST produce an indistinguishable response. - // The method is resolved for execution (script / parameters / timeout), but - // the not-found and not-in-scope branches both return 403 with the SAME - // body. Resolving the method first and folding both negatives into one - // branch keeps the two cases byte-identical (status + message), so a caller - // holding a valid key cannot probe which method names exist. - var method = await repository.GetMethodByNameAsync(methodName, httpContext.RequestAborted); + // Both the not-found and not-in-scope branches return 403 with the SAME + // body; folding both negatives into one branch keeps the two cases + // byte-identical (status + message), so a caller holding a valid key + // cannot probe which method names exist. + // + // Review #3 (scope-check-before-DB-lookup): the in-memory scope check runs + // FIRST and the DB GetMethodByNameAsync only runs when the caller is in + // scope. This removes a timing oracle — a not-in-scope caller no longer + // pays a DB round-trip whose latency could distinguish "valid scope but no + // such method" from "not in scope" — and saves the round-trip entirely on + // the reject path. The combined `method == null || !inScope` guard still + // emits the single identical 403 body, preserving enumeration-safety. + // + // Review #2 (scope case-policy): scope strings ARE the method names, and + // identity.Scopes.Contains(methodName) is an ordinal, case-SENSITIVE + // comparison (HashSet with the default StringComparer.Ordinal). A + // scope must therefore match the registered method name's casing EXACTLY — + // "Echo" does not grant "echo". This is the intended invariant: method + // names are case-sensitive identifiers, and a key's granted scopes must be + // provisioned with the exact casing of the methods they authorize. var inScope = identity.Scopes.Contains(methodName); + var method = inScope + ? await repository.GetMethodByNameAsync(methodName, httpContext.RequestAborted) + : null; if (method == null || !inScope) { diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/Middleware/AuditWriteMiddleware.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/Middleware/AuditWriteMiddleware.cs index 467690ff..7766e4df 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/Middleware/AuditWriteMiddleware.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/Middleware/AuditWriteMiddleware.cs @@ -36,7 +36,7 @@ namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Middleware; /// (no UseAuthentication-backed scheme populates /// for X-API-Key callers), so the handler stashes the resolved API key name on /// under after -/// ApiKeyValidator.ValidateAsync succeeds. The middleware reads it in +/// IApiKeyVerifier.VerifyAsync succeeds. The middleware reads it in /// its finally block — on auth failures the key remains absent and /// stays null (we never echo back an /// unauthenticated principal). @@ -67,7 +67,7 @@ public sealed class AuditWriteMiddleware { /// /// key used by the endpoint handler to publish - /// the resolved API key name once ApiKeyValidator.ValidateAsync has + /// the resolved API key name once IApiKeyVerifier.VerifyAsync has /// succeeded. Exposed as a constant so the handler and middleware share a /// single source of truth (no stringly-typed coupling). /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs index 526be1e4..4914b433 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs @@ -58,6 +58,14 @@ public sealed class EndpointExtensionsTests : IDisposable private const string TokenPrefix = "sbk"; private const string ApiKeyStoreSection = "ScadaBridge:InboundApi:ApiKeyStore"; + // Review #1: the EXACT canonical 403 body shared by the "method not found" and + // "key not in scope" branches. Asserting against this literal (not just that the + // two bodies are equal-to-each-other) catches a single-branch divergence where + // BOTH branches change in lockstep but away from the agreed enumeration-safe + // body — the equal-to-each-other check alone would still pass in that case. + // Pinned to EndpointExtensions.NotApprovedMessage's JSON shape. + private const string NotApprovedBodyJson = """{"error":"API key not approved for this method"}"""; + // Each test gets its own throwaway SQLite database so seeded keys never leak // between tests; the file is deleted on Dispose. private readonly string _sqlitePath = @@ -178,8 +186,13 @@ public sealed class EndpointExtensionsTests : IDisposable var request = BuildPost("secured", "{}", token); var response = await client.SendAsync(request); + var body = await response.Content.ReadAsStringAsync(); Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + // Review #1: pin the not-in-scope body to the exact canonical literal so a + // single-branch divergence is caught here too (not only in the + // identical-bodies comparison test). + Assert.Equal(NotApprovedBodyJson, body); } [Fact] @@ -211,6 +224,12 @@ public sealed class EndpointExtensionsTests : IDisposable Assert.Equal(HttpStatusCode.Forbidden, notInScopeResponse.StatusCode); // The crux of the enumeration-safety invariant: identical bodies. Assert.Equal(notInScopeBody, unknownBody); + // Review #1: pin BOTH bodies to the exact canonical literal, so a future + // change that diverges either branch from the agreed body is caught even if + // the two branches change together (the equal-to-each-other check above + // would not catch that on its own). + Assert.Equal(NotApprovedBodyJson, unknownBody); + Assert.Equal(NotApprovedBodyJson, notInScopeBody); } [Fact]