Remove the insecure TLS override (#983) - #984
Conversation
`danger_accept_invalid_certs(true)`, reached through `bootroot-agent --insecure`, disabled certificate verification outright. The repository's certificate-verification policy does not admit that: verification is never disabled or weakened to make a handshake work, and there is no temporary exception — only a permanent one introduced temporarily. So the flag goes rather than being narrowed. No runtime mode now accepts a certificate the configured trust cannot anchor, and `insecure_mode` stops being threaded through the ACME client, the issuance flow, the registrar surface and the daemon. The oneshot path carried an `IssuanceRuntime` for that one field and never read the rest of it, so it and the `config_path` it was built from go with it; the daemon path, which does reload config per retry, keeps both. The compose smoke path was the only caller that ran without trust material at all. `agent.toml.compose` now carries a `[trust]` section to fill in from the deployment's own `secrets/certs/`, and the agent scenarios build the bundle and both pins from there for every runtime config they write. They also name the loopback rather than address it: step-ca's certificate carries `localhost` as a DNS SAN and no IP SAN, so dialling `127.0.0.1` would fail hostname verification against the very certificate the run was told to trust. Closes #983
The `[trust]` comment promised the placeholders would "fail the handshake closed", but they never reach a handshake: they are not 64 hex characters, so `validate_trust_settings` rejects the config outright with `trust.trusted_ca_sha256 must be 64 hex chars`. An operator who left them in would go looking for a TLS error and find a config error instead. The installation manuals ended the paragraph before the trust preparation with a colon, which now introduces prose rather than the block it used to. Part of #983
Removing `--insecure` left `agent.toml.compose` as the one template that had to grow a `[trust]` section to stay runnable, and nothing asserts it is still there. Dropping it again would not fail a build or a test: the compose smoke path would simply fall back to the system CA store, which cannot anchor a self-signed step-ca, and the breakage would surface only when an operator ran the scenarios by hand. The neighbouring template test already stages both shipped configs and loads them, so assert the two trust keys there. The placeholder values are not checked -- `validate_trust_settings` rejects those, and `from_file` does not run it. Part of #983
The trust-key assertion added beside the profile one repeated its whole setup: read the template out of the manifest directory, stage a copy under a `.toml` name the config loader can infer a format from, and load it. Two copies of that is two places to fix when a third template ships or the staging trick stops being needed. Name the template list once and put the staging behind a helper, so each test is the assertion it exists for. Part of #983
|
[Reviewer Round 1] Approved — no blocking findings. The change fully removes the runtime TLS-bypass path: the flag is absent from parsing and invocation plumbing, and The tests meaningfully cover the regression: the binary rejects I also verified the Code Scanning API: alert 178 is the only open alert, and all other alerts specified by the issue are dismissed with the requested reasons and non-empty audit comments. The PR body correctly uses |
|
[Review Verdict Round 1: APPROVED] |
Suggested squash commitTitle Body |
Closes #983.
What changed
danger_accept_invalid_certs(true)insrc/tls.rs, reached throughbootroot-agent --insecure, is gone. That was CodeQL alert 178 (rust/disabled-certificate-check), and it is the one finding in this batch the issue asks to fix rather than dismiss — it conflicts directly with the repository's certificate-verification policy, which admits no temporary exception.The flag is removed rather than narrowed, so there is no supported runtime mode that accepts a certificate the configured trust cannot anchor.
build_http_clientnow has two modes: the configured CA bundle (with its optional pins) or the system roots.insecure_modestops being threaded throughAcmeClient, the issuance flow, the registrar surface's issuance and renewal, and the daemon.Two consequences worth naming:
IssuanceRuntimefor that single field and read nothing else off it, so the struct and theconfig_pathit was built from leaverun_oneshot— including theconfig_pathparameter on the publicbootroot::run_oneshot. The daemon path, which does reload the config per retry, keeps both.Two adjacent spellings of
--insecureare deliberately left alone, because neither is a certificate-verification decision:scripts/preflight/extra/cli-scenarios.shusescurl --insecurefor a step-ca readiness probe, which is harness liveness rather than a bootroot runtime mode, andstep certificate create --insecurein the two init TLS steps is step's own flag for creating a key with no password.The compose smoke path
agent.toml.composewas the only caller that ran with no trust material at all, which is why it needed--insecure. It now carries a[trust]section to fill in from the deployment's ownsecrets/certs/, andscripts/preflight/extra/agent-scenarios.shbuilds the bundle and both pins from there for every runtime config it writes.published_endpointin that script also had to stop addressing the loopback and start naming it. step-ca's certificate carrieslocalhostas a DNS SAN and no IP SAN, and the compose file binds these ports to127.0.0.1, so an agent diallinghttps://127.0.0.1:<port>would fail hostname verification against the very certificate the run was told to trust.The
[trust]comment in that template also had to stop promising the placeholders would "fail the handshake closed". They never reach a handshake — they are not 64 hex characters, sovalidate_trust_settingsrejects the config outright — and an operator who left them in would have gone looking for a TLS error and found a config error.Tests
agent_args:--insecureno longer appears in the help text and is rejected by the parser as an unknown argument.bootroot_agent_hardening: the oldoneshot_insecure_override_allows_untrusted_serveris replaced byoneshot_rejects_an_insecure_override_flag, which asserts on the flag itself. The other assertions in that file would all still pass if the flag came back — the untrusted-server run fails either way — so the flag is what has to be asserted on.registrar_certs: the merge-gate test that ran underinsecure_modeto keep the transport from reading the bundle is removed; that path is unreachable now, and the gate itself is covered directly byacme::flow'stest_write_merged_ca_bundle_fails_when_existing_unreadable.insecure_mode_does_not_select_bootstrap_pinsis replaced bybootstrap_pins_are_selected_only_for_a_repairable_bundle, which pins down all three bundle states.acme::client:allows_insecure_when_disabledis removed;rejects_self_signed_without_trustis now the whole story for an unanchored server.[trust]keys survive in every shipped template,agent.toml.composeincluded. Dropping that section again would otherwise fail no build and no test — the compose path would silently fall back to the system CA store, which cannot anchor a self-signed step-ca, and the breakage would surface only when an operator ran the scenarios by hand.The other 55 alerts
Alerts 128-177 and 179-183 were already dismissed on
mainwith the reasons and comments the issue's triage specifies, each comment naming #983. I verified all 55 via the code-scanning API: every one carries a non-empty dismissal comment, and the reasons match the triage line by line (false positive / used in tests / won't fix). No code-scanning query is disabled or excluded — the repository has no CodeQL config file at all, and this change adds none.Alert 178 is the only open alert left, and it closes once CodeQL analyzes
mainafter this merges.Test plan
danger_accept_invalid_certsremains anywhere in the tree, and no runtime mode accepts an unanchored certificate. (The alert itself closes only after CodeQL analyzesmainpost-merge.)cargo fmt -- --config group_imports=StdExternalCrate --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-features— 1077 + 1287 + all integration targets pass, 0 failedmarkdownlint-cli2 "**/*.md" "#node_modules" "#target"— 0 issues, 32 filesscripts/check-docs.sh—mkdocs build --strictand the theme assertions pass, both localescargo audit— exits 0 on the two allowed advisories that are the standing baselinevalidate-e2e-*scriptsscripts/preflight/extra/agent-scenarios.sh happyagainst a live compose stack — all five scenarios issue over verified TLS with no--insecurescripts/preflight/ci/e2e-matrix.sh— NOT run locally; itsbootroot initstep needs passwordlesssudo, which this host cannot supply (sudo -n trueexits 1 here). Gated instead by CI's 14Docker E2Ejobs, all green on this head SHA:local-hosts,local-no-hosts,remote-hosts,remote-no-hosts,rotation,reinit-recovery,stepca-san,two-instance,openbao-tls-reown,openbao-tls-no-delta,registrar-verbs,registrar-internal,registrar-internal-init,registrar-redteam.2e68ef6: 22 checks pass, 0 fail (Quality Check,Unit & CLI Smoke, all 14Docker E2Earms,Analyze (rust)/(python)/(actions),Instructions / check,Change Filter). The one non-pass isRegistrar Redteam (docs-only), a docs-only conditional job that correctly does not run for a code change.commands::infra::tests::preflight_compose_published_ports_checks_openbao_localhost_during_installis worth naming as a known flake rather than a regression: it fails when host port 8200 is busy and passes when re-run alone. The full-suite run above was green on a quiet host, with no failures anywhere.