mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 12:51:13 +00:00
[AZ-494] Enable JWT iss/aud validation with fail-fast startup
Option B per user decision: production ships with empty Jwt.Issuer / Jwt.Audience in appsettings.json so the API process refuses to start unless JWT_ISSUER + JWT_AUDIENCE env vars are supplied. Development ships with grep-friendly DEV-ONLY- placeholders so local + docker flows keep working unchanged. AuthenticationServiceCollectionExtensions flips ValidateIssuer + ValidateAudience to true and wires ValidIssuer / ValidAudience via a new ResolveRequiredOrThrow helper that all three required values (secret, iss, aud) now share. JwtTokenFactory.Create + CreateExpired gain optional iss / aud parameters (default null) so existing call sites compile unchanged. JwtTestHelpers adds MintAuthenticated / MintExpired wrappers that resolve iss + aud from env, plus ResolveIssuerOrThrow / ResolveAudienceOrThrow. PerfBootstrap.MintToken + Program.cs JWT bootstrap migrated to the new surface so the perf harness and the integration runner both validate against the same contract. Adds 4 fail-fast unit tests (missing/empty issuer + audience), 2 negative integration scenarios (WrongIssuer_Returns401, WrongAudience_Returns401), and re-tags every existing integration mint site via MintAuthenticated. Compose, .env.example, run-tests.sh, run-performance-tests.sh all load + export JWT_ISSUER + JWT_AUDIENCE alongside JWT_SECRET. Resolves F-AUTH-2 (security_report.md + owasp_review.md). AC-7 (cross-repo suite/_docs/10_auth.md write) deferred — outside this workspace; tracked in deploy_cycle2.md R3 follow-up. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,80 @@
|
||||
# Code Review — Batch 05 cycle 3 (AZ-494)
|
||||
|
||||
**Reviewer**: autodev in-band
|
||||
**Scope**: AZ-494 (JWT iss/aud validation) — implementation, tests, config, docs
|
||||
**Verdict**: **PASS_WITH_WARNINGS** — 0 Critical, 0 High, 0 Medium, 1 Low (cross-repo doc), 1 acknowledged operational gate by user decision.
|
||||
|
||||
## 1. Spec compliance
|
||||
|
||||
All ACs except AC-7 are addressed. AC-7 (suite-level contract) requires writing to a file in the parent monorepo (`suite/_docs/10_auth.md`), which sits outside this workspace and outside autodev's blast radius. The cycle-2 deploy report has been updated to document the cross-repo obligation so it doesn't get lost.
|
||||
|
||||
The user's Option B selection is honoured exactly:
|
||||
|
||||
- `appsettings.json` ships with empty `Jwt.Issuer` / `Jwt.Audience` → production deploy without env vars fails at startup.
|
||||
- `appsettings.Development.json` ships with `DEV-ONLY-`-prefixed placeholders → local + Docker-compose flows work without touching `.env`.
|
||||
- The env-var precedence + fail-fast pattern is identical to the existing `JWT_SECRET` flow from AZ-487, so there's no second mental model for operators to learn.
|
||||
|
||||
## 2. Code quality
|
||||
|
||||
| Area | Observation | Verdict |
|
||||
|---|---|---|
|
||||
| Single source of truth | `JwtTokenFactory.Create` remains the only `new JwtSecurityToken(...)` call site in the source tree (verified by grep). The new iss/aud params slot in alongside the existing `secret` / `subject` / `lifetime` / `extraClaims` / `algorithm` parameters — no parallel code path was introduced. | ✓ |
|
||||
| Helper layering | `JwtTestHelpers.MintAuthenticated` / `MintExpired` are thin wrappers (≤ 12 lines each) that resolve iss/aud from env and delegate to the factory. They live in `SatelliteProvider.IntegrationTests` because they read env vars — exactly the runner-side concern documented in `module-layout.md`. | ✓ |
|
||||
| Fail-fast extraction | The third copy of the "resolve required value, throw with a humanised message" pattern was extracted into a single `ResolveRequiredOrThrow` helper. `ResolveSecretOrThrow` remains separate because it has the additional ≥ 32-byte size check from AZ-487. This is acceptable: two distinct invariants. | ✓ |
|
||||
| Test mutual-isolation | Each new `[Fact]` saves and restores both `JWT_ISSUER` and `JWT_AUDIENCE` via `try/finally`, mirroring the existing pattern for `JWT_SECRET`. No parallel-test interference risk. | ✓ |
|
||||
| Surface area minimisation | `JwtTokenFactory.Create` gained two optional, nullable parameters at the END of the signature with defaults of `null`. Old callers behave identically. | ✓ |
|
||||
| Error message clarity | All three fail-fast messages name (a) the env var, (b) the config key, (c) the AZ-494 spec, and use the human label ("JWT issuer", "JWT audience"). | ✓ |
|
||||
| Logging | No new debug/trace logs added — only the existing startup-time `InvalidOperationException` path. This matches `coderule.mdc`. | ✓ |
|
||||
| Naming | `MintAuthenticated` / `MintExpired` / `ResolveIssuerOrThrow` / `ResolveAudienceOrThrow` all describe caller intent precisely. No vague "data"/"item"/"candidate" names. | ✓ |
|
||||
|
||||
## 3. Security
|
||||
|
||||
| Aspect | Status |
|
||||
|---|---|
|
||||
| F-AUTH-2 (iss/aud not validated) | **Resolved** — `ValidateIssuer = true` + `ValidateAudience = true`, both wired to real values |
|
||||
| Token revocation list | Still absent — listed as a new Low finding in `owasp_review.md` A07 |
|
||||
| Secret leakage | No iss/aud value is logged at WARN / ERROR / INFO. Only the startup banner (Console.Out) prints them — intentional, for diagnosis. | ✓ |
|
||||
| Production safety | `appsettings.json` empty → process won't start without explicit env vars. Cannot accidentally ship a hard-coded prod issuer. | ✓ |
|
||||
| Test/prod isolation | `DEV-ONLY-` prefix means a grep can surface placeholder values at any time. | ✓ |
|
||||
|
||||
## 4. Performance
|
||||
|
||||
No measurable impact. `TokenValidationParameters` now checks two additional string comparisons per request. Sub-microsecond per request — far below the 1 ms AZ-487 NFR budget. PT-07 / PT-08 from AZ-492 will pick up any aggregate regression at the next perf run.
|
||||
|
||||
## 5. Cross-task consistency
|
||||
|
||||
- AZ-487 (JWT base layer) — extended, not replaced. The `RequireSignedTokens` / `RequireExpirationTime` / `ClockSkew = 30 s` / secret ≥ 32 bytes invariants all remain.
|
||||
- AZ-491 (consolidate JWT test helpers) — the new `MintAuthenticated` / `MintExpired` wrappers strengthen the AZ-491 contract (delegate to the canonical factory; no parallel mint logic).
|
||||
- AZ-492 (perf harness) — `PerfBootstrap.MintToken` was updated in the same way as integration call sites. Without this update the perf harness would have started emitting 401s after AZ-494 landed.
|
||||
- AZ-493 (DB reset hook) — unaffected; runs against the same env.
|
||||
- AZ-495 (doc folder convention) — unaffected.
|
||||
- AZ-496 (bump aspnetcore 8.0.25) — directly relevant: AZ-494's `ValidateIssuer` / `ValidateAudience` semantics are unchanged across `Microsoft.AspNetCore.Authentication.JwtBearer` 8.0.21 → 8.0.25.
|
||||
|
||||
## 6. Findings
|
||||
|
||||
### Low
|
||||
|
||||
**L1 — Cross-repo doc deferred (AC-7)**
|
||||
- Where: `suite/_docs/10_auth.md` (outside this workspace).
|
||||
- Why noted: AC-7 explicitly calls for an update there; the autodev process treats this as out-of-scope per the workspace-boundary rule.
|
||||
- Disposition: documented in `deploy_cycle2.md`'s R3 follow-up section.
|
||||
|
||||
### Acknowledged operational gate (NOT a finding)
|
||||
|
||||
The Option B contract is **deliberately** that production deploys without real iss/aud values fail at startup. That's the user-selected forcing function for admin-team confirmation. Not a code defect.
|
||||
|
||||
## 7. AC-by-AC re-check
|
||||
|
||||
| AC | Verification | Verdict |
|
||||
|---|---|---|
|
||||
| AC-1 | `JwtIntegrationTests.WrongIssuer_Returns401` asserts 401 against `https://wrong-issuer.invalid/` | ✓ |
|
||||
| AC-2 | `JwtIntegrationTests.WrongAudience_Returns401` asserts 401 against `wrong-audience-not-satellite` | ✓ |
|
||||
| AC-3 | `JwtIntegrationTests.ValidToken_Returns200_OnHealthyEndpoint` mints through `MintAuthenticated` (env-sourced iss/aud) | ✓ |
|
||||
| AC-4 | 4 fail-fast unit tests + manual smoke ("docker compose up without env vars") path documented | ✓ |
|
||||
| AC-5 | All integration test call sites migrated to `MintAuthenticated`; verified via grep | ✓ |
|
||||
| AC-6 | `security_report.md` + `owasp_review.md` updated | ✓ |
|
||||
| AC-7 | Cross-repo write deferred; documented | ◐ deferred |
|
||||
|
||||
## 8. Verdict
|
||||
|
||||
**PASS_WITH_WARNINGS** — proceed to ship. The single Low finding is structurally outside this workspace.
|
||||
Reference in New Issue
Block a user