fix(auth): ScadaBridge inbound auth review fixes — scope-before-DB, pinned 403 body, pepper fail-fast, log category

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,

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;
 
+/// <summary>
+/// Review N1: log-category marker for the inbound API endpoint. Used as the type
+/// argument to <see cref="ILogger{TCategoryName}"/> so inbound-API auth/authz log
+/// lines are categorized under this ScadaBridge.InboundAPI type rather than under
+/// the shared <c>ZB.MOM.WW.Auth</c> library's <c>IApiKeyVerifier</c>. Exists only
+/// to name the log category (<see cref="EndpointExtensions"/> is static and cannot
+/// be a generic type argument); it is never instantiated.
+/// </summary>
+public sealed class InboundApiEndpoint
+{
+    private InboundApiEndpoint() { }
+}
+
 /// <summary>
 /// 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<ILogger<IApiKeyVerifier>>();
+        // Review N1: log under an endpoint-owned category (a marker type in the
+        // ScadaBridge.InboundAPI namespace) rather than ILogger<IApiKeyVerifier>,
+        // 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<ILogger<InboundApiEndpoint>>();
         var verifier = httpContext.RequestServices.GetRequiredService<IApiKeyVerifier>();
         var repository = httpContext.RequestServices.GetRequiredService<IInboundApiRepository>();
         var executor = httpContext.RequestServices.GetRequiredService<InboundScriptExecutor>();
@@ -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<string> 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)
         {

src/ZB.MOM.WW.ScadaBridge.InboundAPI/Middleware/AuditWriteMiddleware.cs

@@ -36,7 +36,7 @@ namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Middleware;
 /// (no <c>UseAuthentication</c>-backed scheme populates <see cref="HttpContext.User"/>
 /// for X-API-Key callers), so the handler stashes the resolved API key name on
 /// <see cref="HttpContext.Items"/> under <see cref="AuditActorItemKey"/> after
-/// <c>ApiKeyValidator.ValidateAsync</c> succeeds. The middleware reads it in
+/// <c>IApiKeyVerifier.VerifyAsync</c> succeeds. The middleware reads it in
 /// its <c>finally</c> block — on auth failures the key remains absent and
 /// <see cref="AuditEvent.Actor"/> stays null (we never echo back an
 /// unauthenticated principal).
@@ -67,7 +67,7 @@ public sealed class AuditWriteMiddleware
 {
     /// <summary>
     /// <see cref="HttpContext.Items"/> key used by the endpoint handler to publish
-    /// the resolved API key name once <c>ApiKeyValidator.ValidateAsync</c> has
+    /// the resolved API key name once <c>IApiKeyVerifier.VerifyAsync</c> has
     /// succeeded. Exposed as a constant so the handler and middleware share a
     /// single source of truth (no stringly-typed coupling).
     /// </summary>

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]