fix(api): give every boot guard an explicit process role, and fail closed on unusable JWT keys

deploy/.env.example

@@ -23,6 +23,13 @@ POSTGRES_PASSWORD=replace-with-a-strong-password
 # Database__AllowInsecureConnection=false
 Jwt__Issuer=cluckwork
 Jwt__Audience=cluckwork-api
+# BOTH keys are checked at boot, and the SERVING process refuses to start unless
+# each is present and actually parses (#510). The armor below is a placeholder,
+# not a key — copy this file unchanged and the API will tell you so and exit,
+# rather than booting green, passing /health/ready, and 500ing every login. Keep
+# the surrounding quotes and the escaped \n; that is the format PemKey.Normalize
+# expects. The one-shot verbs (migrate / seed / recover-admin / bootstrap-admin /
+# healthcheck) never need these — they neither issue nor validate a token (#347).
 Jwt__PublicKeyPem="-----BEGIN PUBLIC KEY-----\nreplace-me\n-----END PUBLIC KEY-----"
 Jwt__PrivateKeyPem="-----BEGIN PRIVATE KEY-----\nreplace-me\n-----END PRIVATE KEY-----"
 

docs/decisions/README.md

@@ -23,6 +23,7 @@ follow the `→` link from the `AGENTS.md` bullet to get here.
 | [Seed command and simulation profile (#280, #284, #279)](280-seed-and-simulation.md) | AGENTS · Conventions |
 | [First-run admin provisioning: `bootstrap-admin` (#283)](283-first-run-admin-provisioning.md) | AGENTS · Conventions |
 | [Migrate command + prod migration split (#263)](263-migrate-command.md) | AGENTS · Conventions |
+| [Process role, not statement order (#347)](347-process-role.md) | AGENTS · Conventions |
 | [Production Postgres TLS floor + libpq mapping (#261/#262)](261-postgres-tls-floor.md) | AGENTS · Conventions |
 | [GSS/Kerberos negotiation off by default (#332)](332-gss-kerberos.md) | AGENTS · Conventions |
 | [Container health probe: the `healthcheck` verb (#266)](266-container-health-probe.md) | AGENTS · Conventions |
@@ -40,4 +41,6 @@ The five short-enough Conventions bullets that kept their full rationale inline
 `AGENTS.md` — break-glass recovery (#265), farm timezone (#264), the proxy-trust
 boot guard (#260), the design-time migration connection (#318), and container
 image hardening (#267) — have no record here because none of their text was
-relocated.
+relocated. #260 is the partial exception: its *rule* is still inline, but why it
+applies to the serving process and not to the CLI verbs now lives in the
+[#347 record](347-process-role.md) along with #319 and #316.

src/Cluckwork.Api/Cli/CliDispatcher.cs

@@ -20,13 +20,13 @@ public static class CliDispatcher
         new BootstrapAdminCliCommand(),
     ];
 
-    // Whether these args will dispatch to a one-off verb rather than start the
-    // web host. TryRunAsync can only answer this AFTER Build(), but service
-    // registration needs to know sooner: a registration-time guard that throws
-    // would abort a verb before it ever dispatched. Same reason #260/#319 place
-    // their serving-process guards after this dispatcher.
-    public static bool IsCliInvocation(string[] args) =>
-        args.Length > 0 && Array.Exists(Commands, c => c.Name == args[0]);
+    // Whether these args dispatch to a one-off verb rather than start the web
+    // host is answered by ProcessRoles.From(args) (Hosting/ProcessRole.cs), which
+    // reads Commands above so the two can't disagree. It deliberately does NOT
+    // live here: this dispatcher only knows the four verbs that run AFTER
+    // Build(), and `healthcheck` — dispatched before the host exists — is a
+    // one-shot process too. A predicate built from Commands alone called it
+    // Serving (#347).
 
     public static async Task<int?> TryRunAsync(WebApplication app, string[] args)
     {

src/Cluckwork.Api/Configuration/OtlpOptions.cs

@@ -31,9 +31,14 @@ public sealed class OtlpOptions
 
     public bool Enabled => !string.IsNullOrWhiteSpace(Endpoint);
 
+    // What an unset Otlp:Protocol means, and what a one-shot verb falls back to
+    // when it degrades to export-disabled (#347) — nothing is exported in that
+    // state, so the value only has to be a definite one rather than the right one.
+    public const OtlpExportProtocol DefaultProtocol = OtlpExportProtocol.Grpc;
+
     public OtlpExportProtocol ParseProtocol() => Protocol?.Trim().ToLowerInvariant() switch
     {
-        null or "" or "grpc" => OtlpExportProtocol.Grpc,
+        null or "" or "grpc" => DefaultProtocol,
         "http/protobuf" => OtlpExportProtocol.HttpProtobuf,
         var other => throw new InvalidOperationException(
             $"Otlp:Protocol must be 'grpc' or 'http/protobuf', got '{other}'.")

src/Cluckwork.Api/Hosting/CluckworkIdentityServiceCollectionExtensions.cs

@@ -13,9 +13,14 @@ namespace Cluckwork.Api.Hosting;
 
 internal static class CluckworkIdentityServiceCollectionExtensions
 {
+    // role is OneShot for the operator verbs. Nothing here issues or validates a
+    // token for them — see the key guard below, which is serving-only for that
+    // reason and would otherwise be a fresh instance of the #331 class this file
+    // now has to avoid (#347/#510).
     public static IServiceCollection AddCluckworkIdentity(
         this IServiceCollection services,
-        IConfiguration configuration)
+        IConfiguration configuration,
+        ProcessRole role = ProcessRole.Serving)
     {
         services.AddScoped<CurrentUserContext>();
         services.AddScoped<ICurrentUser>(sp =>
@@ -41,15 +46,32 @@ public static IServiceCollection AddCluckworkIdentity(
 
         services.Configure<JwtOptions>(
             configuration.GetSection(JwtOptions.SectionName));
-        var jwtPublicKeyPem = PemKey.Normalize(configuration["Jwt:PublicKeyPem"]
-            ?? throw new InvalidOperationException(
-                "Jwt:PublicKeyPem is not configured."));
+        // #510 — this guard used to be dead for every real deployment, and the
+        // key's CONTENT was never checked at boot at all.
+        //
+        // `configuration[...] ?? throw` only catches NULL. The shipped
+        // appsettings.json carries `"PublicKeyPem": ""`, so a deployment that
+        // loses its environment variable gets an EMPTY STRING and sailed past —
+        // the guard fired only when appsettings.json was absent entirely, which
+        // is not how the image runs. And ImportFromPem sat inside the AddJwtBearer
+        // delegate, so a malformed key was a per-request failure: the farm booted,
+        // /health/ready went green, the container HEALTHCHECK passed, and every
+        // authenticated request 500'd. An orchestrator saw a healthy instance that
+        // rejected every login.
+        //
+        // Both halves are fixed here, at boot, matching the fail-closed stance of
+        // the #261/#262 TLS floor and the #264 tzdata canary — a farm with no
+        // usable signing key must not pretend to be up.
+        var jwtPublicKeyPem = EnsureUsablePublicKey(configuration, role);
 
         services
             .AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
             .AddJwtBearer(options =>
             {
                 var rsa = RSA.Create();
+                // Still imported here because RSA is not thread-safe and this
+                // instance is the one the options hold; the boot-time import
+                // above is what decides whether the key is usable at all.
                 rsa.ImportFromPem(jwtPublicKeyPem);
 
                 options.MapInboundClaims = false;
@@ -68,6 +90,7 @@ public static IServiceCollection AddCluckworkIdentity(
             });
 
         services.AddAuthorization(options => options.AddCluckworkPolicies());
+
         services.AddSingleton<
             Microsoft.AspNetCore.Authorization.IAuthorizationMiddlewareResultHandler,
             ForbiddenProblemResultHandler>();
@@ -102,4 +125,55 @@ public static IServiceCollection AddCluckworkIdentity(
 
         return services;
     }
+
+    // Serving-only (#347). A one-shot verb neither issues nor validates a token —
+    // `recover-admin` resets a password, `bootstrap-admin` creates an Owner,
+    // `migrate` applies DDL — so requiring a signing key of them would abort the
+    // break-glass path over configuration it does not use. That is precisely the
+    // #331 class, and making a NEW guard eager is how it would come back; this is
+    // why #510's fix belongs with #347 rather than before it.
+    //
+    // OneShotVerbMinimalConfigTests pins it from the other side: its environment
+    // carries no Jwt:* at all, so a verb that started demanding one goes red.
+    private static string EnsureUsablePublicKey(IConfiguration configuration, ProcessRole role)
+    {
+        var publicKeyPem = configuration["Jwt:PublicKeyPem"];
+        if (role is not ProcessRole.Serving)
+            return publicKeyPem ?? string.Empty;
+
+        // BOTH keys, because both fail the same way and only one of them was ever
+        // looked at here. The private key is read per-request by JwtTokenService
+        // and StepUpGrantService, so a corrupt one boots green and then 500s the
+        // login it is needed for — the same defect as the public key, one file
+        // over.
+        EnsureUsable("Jwt:PublicKeyPem", publicKeyPem);
+        EnsureUsable("Jwt:PrivateKeyPem", configuration["Jwt:PrivateKeyPem"]);
+        return PemKey.Normalize(publicKeyPem!);
+    }
+
+    private static void EnsureUsable(string key, string? pem)
+    {
+        // IsNullOrWhiteSpace, not `?? throw`: the shipped appsettings.json carries
+        // an EMPTY string for both keys, so a null check is unreachable in any
+        // real deployment and the guard was decorative (#510).
+        if (string.IsNullOrWhiteSpace(pem))
+            throw new InvalidOperationException(
+                $"{key} is not configured. The API cannot sign or validate tokens without it, "
+                + "so it refuses to start rather than accept traffic it must reject.");
+
+        using var rsa = RSA.Create();
+        try
+        {
+            rsa.ImportFromPem(PemKey.Normalize(pem));
+        }
+        catch (Exception ex) when (ex is ArgumentException or CryptographicException)
+        {
+            // Named, and at BOOT. Previously this surfaced per-request from inside
+            // the AddJwtBearer delegate: /health/ready stayed green, the container
+            // HEALTHCHECK passed, and every authenticated request 500'd — an
+            // orchestrator saw a healthy instance that rejected every login.
+            throw new InvalidOperationException(
+                $"{key} is not a usable PEM key: {ex.Message}", ex);
+        }
+    }
 }

src/Cluckwork.Api/Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs

@@ -11,15 +11,48 @@ namespace Cluckwork.Api.Hosting;
 
 internal static class CluckworkRateLimitingServiceCollectionExtensions
 {
+    // #347 review — role is OneShot for the operator verbs. Every limiter built
+    // here exists to shape INBOUND HTTP, which a run-then-exit verb never serves,
+    // so this whole section is serving-only machinery and its validation must be
+    // scoped like one. It was not: a malformed CIDR or a nonzero
+    // ReportsConcurrency:QueueLimit aborted `migrate`/`recover-admin` at service
+    // registration — #331's shape again, and a stranger inconsistency than that,
+    // because RateLimiting:TrustedProxies being EMPTY was already correctly
+    // serving-only (#260) while the SAME key being malformed was hostile to every
+    // role.
     public static CluckworkRateLimitingRegistration AddCluckworkRateLimiting(
         this IServiceCollection services,
-        IConfiguration configuration)
+        IConfiguration configuration,
+        ProcessRole role = ProcessRole.Serving)
     {
-        var rateLimiting = configuration
-            .GetSection(RateLimitingOptions.SectionName)
-            .Get<RateLimitingOptions>() ?? new RateLimitingOptions();
-        rateLimiting.Validate();
-        var trustedProxies = rateLimiting.ParseTrustedProxies();
+        var rateLimiting = new RateLimitingOptions();
+        IPNetwork[] trustedProxies;
+        try
+        {
+            // The BINDING is inside the boundary too, not just the validation: a
+            // non-numeric `RateLimiting:Login:PermitLimit` throws from Get<T>()
+            // before any validator runs, and that aborted every verb just as
+            // surely (#347 review). Same lesson as the OTLP section — scope
+            // everything that can reject this configuration, or the next
+            // unscoped part of it is the next #331.
+            rateLimiting = configuration
+                .GetSection(RateLimitingOptions.SectionName)
+                .Get<RateLimitingOptions>() ?? new RateLimitingOptions();
+            rateLimiting.Validate();
+            trustedProxies = rateLimiting.ParseTrustedProxies();
+        }
+        catch (InvalidOperationException ex) when (role is ProcessRole.OneShot)
+        {
+            // Same degrade as the OTLP one: warn on stderr and carry on with
+            // defaults that nothing in this process will consult, rather than
+            // taking out an operational escape hatch over configuration for a
+            // server this process is not going to be. Defaults, not the operator's
+            // values — the operator's are the ones just declared unusable.
+            Console.Error.WriteLine(
+                $"warning: rate limiting not configured for this command — {ex.Message}");
+            rateLimiting = new RateLimitingOptions();
+            trustedProxies = [];
+        }
 
         services.AddRateLimiter(limiter =>
         {

src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs

@@ -10,15 +10,18 @@ namespace Cluckwork.Api.Hosting;
 
 internal static class CluckworkTelemetryServiceCollectionExtensions
 {
-    // isServingProcess is false for the one-off CLI verbs (migrate / seed /
-    // recover-admin). They dispatch after Build(), so a throw during service
-    // registration would abort them before CliDispatcher ever ran — see the
-    // endpoint-resolution block below for why that matters.
+    // role is OneShot for the operator verbs (migrate / seed / recover-admin /
+    // bootstrap-admin / healthcheck). They dispatch after Build(), so a throw
+    // during service registration would abort them before CliDispatcher ever ran
+    // — see the endpoint-resolution block below for why that matters. #347: the
+    // role is computed once in Program.cs and passed in, so this is the guard's
+    // own declared scope rather than a positional bool whose meaning lives at a
+    // call site far away.
     public static CluckworkTelemetryRegistration AddCluckworkTelemetry(
         this IServiceCollection services,
         IConfiguration configuration,
         IHostEnvironment environment,
-        bool isServingProcess = true)
+        ProcessRole role = ProcessRole.Serving)
     {
         // ReadFrom.Services lets DI-registered enrichers/sinks join the pipeline —
         // the integration tests tap the logger this way (#214).
@@ -55,32 +58,52 @@ public static CluckworkTelemetryRegistration AddCluckworkTelemetry(
 
         // OTLP export is config-gated (#214). Endpoint and protocol are validated
         // eagerly so configuration errors fail at boot.
-        var otlp = configuration.GetSection(OtlpOptions.SectionName).Get<OtlpOptions>()
-            ?? new OtlpOptions();
-        var protocol = otlp.ParseProtocol();
         var isProduction = environment.IsProduction();
+        var otlp = new OtlpOptions();
+        var protocol = OtlpOptions.DefaultProtocol;
         Uri? traceEndpoint = null;
         Uri? metricsEndpoint = null;
-        if (otlp.Enabled)
+        try
         {
-            try
+            // Everything that can reject this section sits INSIDE this boundary —
+            // binding, protocol parsing, endpoint resolution — and that
+            // completeness is the whole point. Two rounds of review each found one
+            // more part of it outside: first ParseProtocol (so `Otlp:Protocol=bogus`
+            // killed `recover-admin` with SIGABRT 134, #331 verbatim, three lines
+            // from its own fix), then the BINDING itself (so a non-boolean
+            // `Otlp:AllowInsecureEndpoint` threw from Get<OtlpOptions>() before any
+            // of it ran). Scope the subsystem, not the setting that bit you.
+            otlp = configuration.GetSection(OtlpOptions.SectionName).Get<OtlpOptions>()
+                ?? new OtlpOptions();
+            protocol = otlp.ParseProtocol();
+            if (otlp.Enabled)
             {
                 traceEndpoint = otlp.ResolveTraceEndpoint(isProduction);
                 metricsEndpoint = otlp.ResolveMetricsEndpoint(isProduction);
             }
-            catch (InvalidOperationException ex) when (!isServingProcess)
-            {
-                // A one-off verb must not die on a telemetry misconfiguration it
-                // does not depend on. `recover-admin` in particular is the
-                // break-glass path for a locked-out farm (#265) and is
-                // deliberately NOT environment-gated — an unrelated bad
-                // Otlp:Endpoint blocking an emergency password reset would be a
-                // worse failure than the one this validation prevents. Degrade to
-                // export DISABLED rather than exporting insecurely: the verb runs,
-                // and nothing leaves the process over an unvalidated endpoint.
-                Console.Error.WriteLine(
-                    $"warning: OTLP export disabled for this command — {ex.Message}");
-            }
+        }
+        catch (InvalidOperationException ex) when (role is ProcessRole.OneShot)
+        {
+            // A one-off verb must not die on a telemetry misconfiguration it does
+            // not depend on. `recover-admin` in particular is the break-glass path
+            // for a locked-out farm (#265) and is deliberately NOT
+            // environment-gated — an unrelated bad Otlp:* setting blocking an
+            // emergency password reset would be a worse failure than the one this
+            // validation prevents. Degrade to export DISABLED rather than
+            // exporting insecurely: the verb runs, and nothing leaves the process
+            // over an unvalidated endpoint.
+            //
+            // Everything is reset, not just the part that threw: if the trace
+            // endpoint resolved and the metrics endpoint did not, keeping the
+            // first would export half a signal set from a configuration this
+            // process has already declared unusable — and a partially bound
+            // `otlp` would still feed Headers to an exporter.
+            otlp = new OtlpOptions();
+            protocol = OtlpOptions.DefaultProtocol;
+            traceEndpoint = null;
+            metricsEndpoint = null;
+            Console.Error.WriteLine(
+                $"warning: OTLP export disabled for this command — {ex.Message}");
         }
 
         Action<OtlpExporterOptions> ConfigureOtlpExporter(Uri endpoint) => options =>