feat(gateway): warn at startup on missing operator runbook gates - #1855
Conversation
Wave B roadmap item 8. AEGIS_JWT_REQUIRED+bind was already fail-closed (assert_bind_security); this extends the same non-loopback-bind check to *warn* (not fail-closed -- these have legitimate reasons to be absent, e.g. TLS terminated at a reverse proxy) when a network-reachable gateway is missing an admin key, AEGIS_REPLAY_STORE=db, or TLS. Backups are deliberately not part of this check: AEGIS_BACKUP_DIR always resolves to a value (defaults to "backups"), so its presence can't distinguish "operator configured backups" from "nobody thought about it" -- stays a doc-only runbook item. Also filled a real gap found while researching this: the deployment guide's production checklist was missing the admin-key and replay-store items entirely.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
📝 WalkthroughWalkthroughStartup now warns on missing production hardening for non-loopback binds, while documentation records the warning and fail-closed checks. The runtime adds gate detection for the admin key, database replay store, and TLS, with unit coverage. ChangesProduction hardening gate
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant gate_detection
participant tracing
main->>gate_detection: evaluate bind and hardening configuration
gate_detection-->>main: return missing production gates
main->>tracing: warn with missing gate names
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)src/src/main.rsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Roadmap Wave B item 8 by introducing startup warnings on non-loopback binds when production hardening configurations (such as the admin API key, database replay store, or TLS) are missing. It also updates the deployment guide and implementation status documentation, and adds corresponding unit tests. Feedback is provided regarding a minor discrepancy between the startup warning check and the runtime enforcement check for the admin API key, suggesting they be aligned for consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| warn_on_incomplete_production_hardening( | ||
| &bind_addr, | ||
| std::env::var("AEGIS_ADMIN_API_KEY").is_ok_and(|v| !v.trim().is_empty()), |
There was a problem hiding this comment.
There is a discrepancy between how the startup warning and the runtime admin_decision check whether the admin API key is configured. The startup warning uses !v.trim().is_empty(), which treats a whitespace-only key as unconfigured. However, admin_decision uses !k.is_empty(), meaning a whitespace-only key would actually be accepted at runtime if provided by the client. To ensure consistency between the startup warning and runtime enforcement, align the startup check with the runtime check by using !v.is_empty().
std::env::var("AEGIS_ADMIN_API_KEY").is_ok_and(|v| !v.is_empty()),![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/deployment-guide.md`:
- Around line 248-251: Update the deployment checklist’s Secrets guidance around
assert_bind_security to document that AEGIS_DEMO_MODE=true intentionally permits
a non-loopback unauthenticated bind with a warning. Clearly label demo mode as
an insecure escape hatch that must not be used in production, while preserving
the normal startup fail-closed guidance.
In `@src/src/main.rs`:
- Around line 1247-1272: Update missing_production_hardening_gates and the
adjacent production-hardening helper to follow the required Result<T,
AegisError> contract, wrapping successful values in Ok(...). Change the wrapper
to return Result<(), AegisError> and propagate its result at the call site
around the production startup flow near lines 2422-2427.
- Around line 2422-2427: Update the startup hardening-warning flow around
warn_on_incomplete_production_hardening so partial TLS configuration is handled
by only one warning path. Track or propagate whether the earlier TLS branch
already emitted its warning, and prevent the aggregate missing-TLS warning from
being logged for that same missing gate while preserving warnings for other
incomplete production-hardening requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b75e236-6e69-4991-b866-56eda5bfd4a8
📒 Files selected for processing (3)
docs/Implementation_Status.mddocs/deployment-guide.mdsrc/src/main.rs
| > On a non-loopback `AEGIS_BIND_ADDR`, the gateway checks three of these itself at startup and logs a `WARN` naming whichever are missing (`warn_on_incomplete_production_hardening`, `src/src/main.rs`) — it's a reminder, not a fail-closed gate, since each has legitimate reasons to be absent in a given deployment. `AEGIS_JWT_REQUIRED` is the one exception: an unauthenticated public bind fails closed at startup (`assert_bind_security`), it doesn't just warn. | ||
|
|
||
| - [ ] **TLS**: set `AEGIS_TLS_CERT`/`AEGIS_TLS_KEY`, or terminate TLS at a reverse proxy / Kubernetes ingress in front of the gateway. Plain HTTP is fine for `127.0.0.1`-bound local dev only. *(startup-warned)* | ||
| - [ ] **Secrets**: set `AEGIS_JWT_REQUIRED=true` with a real `AEGIS_JWT_SECRET` (not `default_secret`) before exposing the gateway beyond localhost. Set `AEGIS_POLICY_SIGNING_KEY` if you intend to use signed policy bundles. Never commit secret values — use your platform's secret store (Kubernetes `Secret`, systemd `EnvironmentFile`, etc.). *(startup fail-closed)* |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document the explicit demo-mode exception.
assert_bind_security permits a non-loopback unauthenticated bind when AEGIS_DEMO_MODE=true and emits a warning. The checklist currently says such binds fail closed, which can mislead operators. State that demo mode is an intentional insecure escape hatch and must not be used in production.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/deployment-guide.md` around lines 248 - 251, Update the deployment
checklist’s Secrets guidance around assert_bind_security to document that
AEGIS_DEMO_MODE=true intentionally permits a non-loopback unauthenticated bind
with a warning. Clearly label demo mode as an insecure escape hatch that must
not be used in production, while preserving the normal startup fail-closed
guidance.
| fn missing_production_hardening_gates( | ||
| bind_addr: &str, | ||
| admin_key_configured: bool, | ||
| replay_store_db: bool, | ||
| tls_enabled: bool, | ||
| ) -> Vec<&'static str> { | ||
| if host_is_loopback(bind_addr) { | ||
| return Vec::new(); | ||
| } | ||
| let mut missing = Vec::new(); | ||
| if !admin_key_configured { | ||
| missing | ||
| .push("AEGIS_ADMIN_API_KEY (admin/debug/metrics endpoints stay disabled without it)"); | ||
| } | ||
| if !replay_store_db { | ||
| missing.push( | ||
| "AEGIS_REPLAY_STORE=db (replay-nonce dedup is per-process only, unsafe across multiple replicas)", | ||
| ); | ||
| } | ||
| if !tls_enabled { | ||
| missing.push( | ||
| "AEGIS_TLS_CERT/AEGIS_TLS_KEY (or terminate TLS at a reverse proxy in front of this gateway)", | ||
| ); | ||
| } | ||
| missing | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the required Result<T, AegisError> return contract.
Both new production helpers return non-Result types, violating the repository rule. Wrap successful values in Ok(...), return Result<(), AegisError> from the wrapper, and propagate the result at Lines 2422-2427.
Also applies to: 1274-1292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/src/main.rs` around lines 1247 - 1272, Update
missing_production_hardening_gates and the adjacent production-hardening helper
to follow the required Result<T, AegisError> contract, wrapping successful
values in Ok(...). Change the wrapper to return Result<(), AegisError> and
propagate its result at the call site around the production startup flow near
lines 2422-2427.
Source: Coding guidelines
| warn_on_incomplete_production_hardening( | ||
| &bind_addr, | ||
| std::env::var("AEGIS_ADMIN_API_KEY").is_ok_and(|v| !v.trim().is_empty()), | ||
| replay_store_db, | ||
| use_tls.is_some(), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid duplicate warnings for partial TLS configuration.
When only one TLS environment variable is set, the earlier branch already logs a warning, then this call logs the aggregate missing-TLS warning as well. Consolidate these paths so one missing gate produces one startup warning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/src/main.rs` around lines 2422 - 2427, Update the startup
hardening-warning flow around warn_on_incomplete_production_hardening so partial
TLS configuration is handled by only one warning path. Track or propagate
whether the earlier TLS branch already emitted its warning, and prevent the
aggregate missing-TLS warning from being logged for that same missing gate while
preserving warnings for other incomplete production-hardening requirements.
Summary
Closes Wave B roadmap item 8 ("Operator runbook gates: JWT_REQUIRED, admin key, REPLAY_STORE=db, TLS, backups").
Researched first:
AEGIS_JWT_REQUIRED+ bind address was already fail-closed at startup (assert_bind_security, pre-existing). The other four items had zero runtime signal — an operator could deploy to a public bind missing an admin key, a shared replay store, or TLS, and nothing would tell them.warn_on_incomplete_production_hardening(src/src/main.rs): on a non-loopback bind, logs a singleWARNnaming which of {admin key,AEGIS_REPLAY_STORE=db, TLS} aren't configured. Deliberately a warning, not a fail-closed check like the existing JWT/bind one — each of these three has a legitimate reason to be absent in some deployments (TLS terminated at a reverse proxy, single-replica deployment not needing the shared replay store, admin routes intentionally unused), unlike "publicly reachable with zero authentication," which really is unconditionally unsafe.AEGIS_BACKUP_DIRalways resolves to a value (defaults to"backups"), so its mere presence can't distinguish "operator actually scheduled backups" from "nobody configured this." Doing this properly would need new state (e.g. a last-backup timestamp) — out of scope here. Backups stay a doc-only runbook item, called out explicitly in the updated checklist.missing_production_hardening_gates+ thin logging wrapper, mirroring this file's existingassert_bind_security/admin_decisionpattern, for unit testability.docs/deployment-guide.md's "Production checklist" was missing the admin-key and replay-store items entirely (only had TLS/secrets/encryption/monitoring/health/backups/CORS/rate-limits). Added both, plus a note on which items are now startup-warned vs. still doc-only.Test plan
cargo check/cargo fmt --check/cargo clippy --workspace --all-targets -D warnings— cleancargo test --workspace -- --test-threads=1— all passSummary by CodeRabbit
New Features
Documentation