Code review findings: 15 defects from /code-review of #32 (005-credential-broker)

[pofallon]

This issue collects the 15 confirmed/plausible defects surfaced by a high-effort code review (5 finder angles + 1 verifier + 1 sweep) over the credential-broker work in #32 / branch 005-credential-broker.

Most-severe first. Each entry is independently actionable; consider splitting into per-finding issues before triage if a single thread gets unwieldy.

---

1. Showstopper — broker_install URL v0.1.0 returns 404 today

• File: ansible/roles/broker_install/defaults/main.yml:7
• Failure: curl -sI -L https://github.com/get2knowio/remo-broker/releases/download/v0.1.0/... returns HTTP/2 404 for both the binary and .sha256. Every remo {aws,hetzner,incus,proxmox} create now includes broker_install unconditionally, so the very first user to upgrade hits a play failure during their first create.
• Fix sketch: Coordinate with get2knowio/remo-broker to cut v0.1.0 (see companion remo-broker prompt). Gate the role on `REMO_BROKER_BACKEND` being set until then.

2. Security — mint exceptions leak the freshly-minted token via {payload!r}

• File: src/remo_cli/providers/broker.py:86 (_onepassword_mint) and :146 (_vault_mint)
• Failure: 1Password / Vault response returns the token under an unexpected key (schema drift, future API version). Token is present in `payload` but extractor misses it; `raise BackendError(f"...{payload!r}")` formats the entire payload — including the live token — into the exception message. `providers/hetzner.py:215` then prints it via `print_error(f"Bootstrap minting failed: {exc}")`. Token ends up in shell scrollback / CI logs / terminal recording.
• Fix: Never format `payload` into the exception message. Log only field-presence info ("missing 'token_id'; saw keys: [...]").

3. Showstopper — lookup('pipe', 'fnox get …') hard-fails when fnox absent

• File: ansible/group_vars/all.yml:4 (also ansible/roles/aws_server/defaults/main.yml:16-17, ansible/aws_teardown.yml:26-27)
• Failure: Pipe lookups evaluate eagerly during variable resolution. If fnox is not on PATH or the key isn't set, Ansible raises before any user-facing validation task can fire. Previously `lookup('env', ...)` returned empty gracefully and a friendly-error `assert` produced the install-pointer message.
• Fix: Wrap the pipe call in a default and surface a friendly error in a pre-flight task. Example: `lookup('pipe', 'fnox get hetzner_api_token 2>/dev/null || true') | default('', true)`.

4. Subsystem dead — @cli.result_callback never fires because subcommands sys.exit()

• File: src/remo_cli/cli/main.py:18
• Failure: Every subcommand in cli/providers/*.py, cli/audit.py, cli/rotate.py, cli/init.py ends with sys.exit(rc). SystemExit propagates past Click's `MultiCommand.invoke`, so the result_callback never runs. Consequence: `overdue_reminders()` (T083a passive overdue-rotation warning) AND the pre-existing passive update-check are dead code in practice. Tests cover `overdue_reminders()` directly, hiding the dead-hook bug.
• Fix: Either move the post-command hook to a Click result_callback that returns from subcommands (refactor all sys.exit(rc) → return rc), or wrap commands in a try/except SystemExit that runs the hook before re-raising. Add a CLI integration test that asserts the hook actually fires.

5. Destructive — _aws_sm_revoke deletes the per-developer IAM role shared across instances

• File: src/remo_cli/providers/broker.py:195
• Failure: `_ensure_broker_instance_role` names the role by `dev_id` alone, so two `remo aws create` calls share the role + profile. When the user destroys instance A, `_aws_sm_revoke` calls `_attach_broker_deny_all_policy` then `_delete_broker_instance_role` against that shared role. Deny-all immediately breaks IMDS credentials on instance B/C/…; the delete then removes the role permanently. No `list-instance-profiles-for-role` guard.
• Fix: Make the role per-instance (or check for in-use instance profiles before tearing down). Document the per-developer-vs-per-instance scoping decision in contracts/bootstrap-delivery.md.

6. FR-020 silently broken — Hetzner label keys reject :

• File: src/remo_cli/providers/hetzner.py:228 (write site); :172 (silent swallow)
• Failure: Hetzner Cloud's label-key regex disallows `:`. PUTs of `remo:bootstrap-token-id` / `remo:rotation-cadence-days` / `remo:last-rotation-at` return HTTP 400. `_set_server_label` catches `urllib.error.URLError` silently → label never lands → `_lookup_token_id` returns None → `revoke_before_destroy` skips revocation → backend token orphaned forever. Same defect makes `_read_rotation_metadata` permanently return (7, None, None), marking every Hetzner host overdue.
• Fix: Use _/./- instead of : in label keys (e.g., remo_bootstrap_token_id). Audit the entire remo:* label namespace; update tests + reader.

7. FR-020 silently broken — revoke_before_destroy treats None from _lookup_token_id as success

• File: src/remo_cli/core/broker_revoke.py:33 (and :84 for the lookup that swallows)
• Failure: `_lookup_token_id` wraps the entire Hetzner GET in `except Exception: return None`, returning the same value when the API is unreachable as when no label was ever set. Caller then `return True` and destroy proceeds. Asymmetric: a successful lookup followed by a revoke failure correctly blocks with exit 5, but a lookup failure silently succeeds → a Hetzner outage during destroy = guaranteed leaked token.
• Fix: Distinguish "no token to revoke" (return None) from "lookup failed" (raise; caller honors --force). Drop the bare except.

8. FR-020 only enforced on Hetzner — AWS/Incus/Proxmox destroy skip the hook

• File: src/remo_cli/providers/aws.py:665, incus.py:247, proxmox.py:356
• Failure: `grep -rn revoke_before_destroy src/remo_cli/providers/` shows the only caller is hetzner.py:344. The other three providers' `destroy()` paths run the teardown playbook / API call without ever invoking `broker.revoke_bootstrap_token`. The exit-5 contract in contracts/cli-surface.md is unreachable on three of four providers.
• Fix: Wire `revoke_before_destroy` into every provider's destroy(). See deferred T074 in specs/005-credential-broker/tasks.md.

9. Service starts in failed state — handler always fires on fresh install

• File: ansible/roles/broker_install/handlers/main.yml:2 (paired with tasks/main.yml:70,100,107)
• Failure: The template renders on every play (changed=true on first install), notifying `restart remo-broker`. The handler unconditionally `state: restarted`s the unit at end-of-play, but with no token on a fresh install the daemon exits non-zero → systemd transitions to `failed`. The post-install verify task (line 107) is gated on `broker_token_present` and skips, so Ansible reports green. Operator sees success; broker is broken.
• Fix: Either gate the handler on `broker_token_present | default(false)`, or make the verify task unconditional with a friendly skip-message when no token is present yet.

10. Helper permanently empty — add_node touch blocks the real install

• File: src/remo_cli/providers/incus.py:706 (and providers/proxmox.py:128); blocks ansible/roles/incus_bootstrap/tasks/main.yml copy task with force: false
• Failure: `add_node` SSH-runs `sudo touch /usr/local/libexec/remo-broker-tokens; sudo chmod 0755 ...`, creating an empty zero-byte file. The later `incus_bootstrap` role's helper copy task uses `force: false` so it never overwrites. The broker daemon's expected node-side dispatcher exits 0 with no output. Every per-developer token-management on Incus/Proxmox is silently broken.
• Fix: Drop the touch; let the Ansible role install the real helper. Or change the Ansible task to force: true once the helper script content is known.

11. Supply-chain — whitespace-only .sha256 body disables integrity check

• File: ansible/roles/broker_install/tasks/main.yml:44
• Failure: `failed_when: content == ''` only catches a literally empty body. A `.sha256` containing only `\n` or spaces passes the check; `(content).split() | first | default('')` yields `''`; `get_url` runs with `checksum: "sha256:"` which Ansible accepts as no-verify. A compromised mirror could substitute a tampered broker binary and the install would succeed silently.
• Fix: Validate the extracted checksum against ^[0-9a-fA-F]{64}$ before passing to get_url.

12. remo init is a no-op — config.yml not bridged to env

• File: src/remo_cli/cli/init.py:37 (and every consumer that reads os.environ.get(\"REMO_BROKER_BACKEND\", \"\"))
• Failure: `_save_config` persists `broker.backend` to `~/.config/remo/config.yml`. But `providers/hetzner.py:298`, `providers/aws.py:561`, `core/broker_revoke.py:26`, `cli/rotate.py:85` all read the env var. After `remo init --backend 1password` the broker code paths remain inert until the user separately exports `REMO_BROKER_BACKEND=1password`.
• Fix: Add core/broker_config.py::get_backend() with env-first / file-fallback resolution; replace every direct os.environ.get(\"REMO_BROKER_BACKEND\", ...) with the helper. Same treatment for REMO_BROKER_ADMIN_SA_KEY.

13. JSONC strip corrupts string literals containing //

• File: src/remo_cli/core/devcontainer.py:59
• Failure: Regex `(^|[^:])//[^\\n]*` strips `//` line-comments but also matches inside string literals whenever the preceding char isn't `:` (e.g. `"foo": "a//b"`). `json.loads` then fails; `ensure_socket_mount` returns False silently. Currently masked by the helper having no production callers; will bite the moment integration ships.
• Fix: Use a proper JSONC tokenizer (or commentjson / json5) that skips quoted strings. Add a regression test for \"path\": \"a//b\".

14. Naive vs aware datetime — --since filter and freshness check crash uncaught

• File: src/remo_cli/cli/rotate.py:78 (_parse_iso) and src/remo_cli/core/audit.py:100 (_parse_ts)
• Failure: `datetime.fromisoformat` returns a NAIVE datetime when input lacks `Z` or explicit offset. `_now() - last_rotation` (rotate.py:78 freshness check, :71 _is_overdue) and `_parse_ts(ln.ts) >= cutoff` (audit.py:68) then raise `TypeError: can't subtract offset-naive and offset-aware datetimes`. `remo rotate-bootstrap` and `remo audit --since 1h` crash uncaught if any broker-written timestamp lacks the offset suffix.
• Fix: Normalize to aware in _parse_iso/_parse_ts (tzinfo=timezone.utc if absent). Add tests with bare-ISO inputs.

15. SSH MITM window — accept-new on a freshly-allocated public IP

• File: src/remo_cli/providers/hetzner.py:132
• Failure: First SSH push uses `-o StrictHostKeyChecking=accept-new`. No out-of-band host-key verification. An on-path attacker (BGP hijack, hostile network, stale IP reuse) presenting their own host key gets the bootstrap token piped on stdin; `accept-new` pins it without complaint, subsequent runs continue silently against the wrong endpoint.
• Fix: Hetzner Cloud's API surfaces the host fingerprint after server creation — fetch it and verify against the live key before the push. Failing that, document the residual risk in `docs/credential-broker.md` threat model.

---

Method: 5 parallel finder angles (line-by-line / removed-behavior / cross-file / language-pitfalls / wrapper-correctness) → 8 candidates each → dedup → 1 verifier with quote-the-line evidence → sweep for gaps. Of 40 raw candidates, 24 verified CONFIRMED, 2 PLAUSIBLE, 1 REFUTED; 5 from the sweep made the top-15 cut.

cc / context: #32 (PR introducing these), specs/005-credential-broker/.

🤖 Generated with Claude Code