FM-600: Proxmox accounts, TLS fingerprint trust, and discovery

[Andreas-Froyland]

Closes #99 (FM-600).

Implementation approach (from the issue)

FM-S08 chose the fallback: a small reqwest transport with a custom rustls verifier that pins the host certificate's SHA-256 fingerprint, typed provider DTOs at the boundary, and Fleet-owned UPID handling. This PR delivers the accounts/trust/discovery slice.

What landed

• Provider transport (fleet-provider-proxmox): PveTransport port + ReqwestPveTransport on reqwest 0.12 + rustls ring — the TLS stack the tailscale provider already uses, no new dependency class, no duplicated reqwest major, no aws-lc-rs. The PinningVerifier SHA-256-pins the leaf certificate and refuses everything else at the handshake; verification is never disabled. An unpinned host is probed with an observe-only policy that captures the leaf fingerprint and refuses before any credential is sent — the FM-201 SSH pin/decide/confirm flow over TLS. Decoding is tolerant of PVE's loose/null shapes (all-Option, data: null = empty, per-resource isolation into warnings).
• Accounts (fleet-application/src/proxmox.rs): multi-account ProxmoxAccount records (name, host, port, token id, fingerprint state) behind proxmox.read/proxmox.config (catalog 36 → 38); the token secret lives in the encrypted secret store as proxmox/<account-id> and is resolved just in time; a failed secret write removes the half-made account. The explicit-trust gate: discovery is locked until the fingerprint is confirmed (proxmox_unconfirmed), and a pinned host whose certificate changes reports the mismatch with both fingerprints as evidence (proxmox_fingerprint_mismatch) — never an empty list.
• Storage: STRICT migration 0017_proxmox_accounts (unique names, fingerprint empty until confirmed); ProxmoxAccountRepository over the application port.
• Surfaces: /api/v1/proxmox/accounts{,/{id}{,/observe,/confirm,/discovery}} in the OpenAPI doc (client regenerated), fleetctl proxmox accounts|create|delete|observe|confirm|discover (secret read from stdin, never argv), all from the same use cases.
• Authz/audit: every call through the central funnel; account create/confirm/delete audited; the audit guard structurally rejects the token_id key (contains "token") — provenance is the account name, credential material stays out entirely.

Tests

• Provider unit tests (normalization, envelope tolerance, fingerprint forms) + live trust tests against the integration PVE 9.2 host, skipped without FLEET_PVE_LIVE=1: pinned connect, wrong-fingerprint refusal at the handshake, observe-only refusal capturing the fingerprint.
• Application use-case tests over fakes: the trust gate, observe→confirm→discover, mismatch/auth/privilege/connect honesty, secret never in audit, failed-write rollback, denial before ports, malformed refusal, conflict.
• Controller e2e over the real router + secret store: create→observe→confirm→discover→delete, mismatch reporting, malformed refusal, duplicate conflict, unknown 404.
• CLI render/parse contract tests. cargo xtask verify passes; PVE 8.x evidence remains the recorded FM-S08 deviation on the real-cluster suite.

Non-goals (respected)

Guest lifecycle (epic #11), associations/guest-agent data (epic #10), template/clone/snapshot (epic #12), Lab leases.

---

Summary by cubic

Closes FM-600. Fleet previously had no Proxmox integration; this PR adds multi-account Proxmox management with TLS certificate fingerprint pinning and cluster discovery.

What changed

• Proxmox accounts live in SQLite with unique names; token secrets are write-only, stored in the encrypted secret store, and never logged or included in audit events.
• The transport pins the host certificate's SHA-256 fingerprint at the TLS handshake and never disables verification.
• observe captures a fingerprint without sending credentials; confirm pins that fingerprint, and discovery stays locked until confirmed.
• A pinned host whose certificate changes refuses calls and reports both the old and new fingerprints.
• Adds /api/v1/proxmox/accounts endpoints and fleetctl proxmox commands for listing, creating, observing, confirming, discovering, and deleting accounts.
• Account mutations require the new proxmox.config permission; reads require proxmox.read; account create, confirm, and delete emit audit events.
• Migrations 0017_proxmox_accounts and 0018_proxmox_observed_fingerprint create the schema automatically on startup.

^{Written for commit 2575b87. Summary will update on new commits.}

[cubic-dev-ai]

7 issues found across 38 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/fleet-api/src/lib.rs">

<violation number="1" location="crates/fleet-api/src/lib.rs:224">
P2: Retries of these newly exposed Proxmox mutations are not idempotent: the create call conflicts, confirmation repeats its write/audit, and deletion returns 404. Thread a caller-scoped `idempotency-key` through the Proxmox mutations or route them through the existing idempotent operation mechanism.</violation>
</file>

<file name="crates/providers/fleet-provider-proxmox/src/lib.rs">

<violation number="1" location="crates/providers/fleet-provider-proxmox/src/lib.rs:361">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**

When the handshake fails without a captured certificate, this branch always returns `Connect`, so the documented `NoCertificate` outcome is unreachable and callers cannot distinguish a TLS certificate anomaly from DNS, TCP, or timeout failures. Return `PveTransportError::NoCertificate` for the certificate-less case.</violation>

<violation number="2" location="crates/providers/fleet-provider-proxmox/src/lib.rs:658">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**

When a PVE field exceeds its documented bound, `normalize_resource` silently truncates it, and an out-of-range `vmid` becomes `0`; this contradicts the module’s promise to return a payload error instead of altering discovery data. Validate bounds and return `PveApiError::InvalidPayload` (or an equivalent normalization error) before constructing the resource.</violation>
</file>

<file name="crates/fleet-api/src/proxmox.rs">

<violation number="1" location="crates/fleet-api/src/proxmox.rs:69">
P2: When the PVE token is rejected or revoked, this response tells clients to retry the unchanged request with backoff. Mark authentication failures as non-retryable (`RetryClass::Never`); the operator must replace or repair the stored credential first.</violation>

<violation number="2" location="crates/fleet-api/src/proxmox.rs:79">
P2: When the PVE token lacks a privilege, this catch-all marks the failure retryable with backoff. Add an explicit `Forbidden` source arm with `RetryClass::Never`, while retaining backoff only for transient HTTP or connection failures.</violation>
</file>

<file name="crates/fleet-application/tests/proxmox.rs">

<violation number="1" location="crates/fleet-application/tests/proxmox.rs:300">
P2: This test's name promises the secret is cleared on delete, but it only asserts `list()` is empty. `FakeCredentials.cleared` (line 131) is recorded but never asserted anywhere in the file, and `load` is never checked after delete. Because production `delete` swallows `clear` errors (best-effort in `proxmox.rs`), a regression that stops clearing or leaves the record would still pass this test. Assert that `cleared` contains the account id and/or `load` returns `None` after delete.</violation>

<violation number="2" location="crates/fleet-application/tests/proxmox.rs:428">
P3: The test name says the secret never surfaces in "audit or errors", but the loop only inspects `audit.intents`; no error path is exercised, so an error whose `Debug`/`Display` embeds the secret would pass. Either narrow the name to audit metadata, or force a failure path (e.g. the refusing store in `a_failed_secret_write_removes_the_account`) and assert `format!("{error:?}")` and the rendered intent do not contain the secret.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: Custom agent: Flag AI Slop and Fabricated Changes

When a PVE field exceeds its documented bound, normalize_resource silently truncates it, and an out-of-range vmid becomes 0; this contradicts the module’s promise to return a payload error instead of altering discovery data. Validate bounds and return PveApiError::InvalidPayload (or an equivalent normalization error) before constructing the resource.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/providers/fleet-provider-proxmox/src/lib.rs, line 658:

<comment>When a PVE field exceeds its documented bound, `normalize_resource` silently truncates it, and an out-of-range `vmid` becomes `0`; this contradicts the module’s promise to return a payload error instead of altering discovery data. Validate bounds and return `PveApiError::InvalidPayload` (or an equivalent normalization error) before constructing the resource.</comment>

<file context>
@@ -1,6 +1,731 @@
+    let name = entry
+        .get("name")
+        .and_then(serde_json::Value::as_str)
+        .map(|value| value.chars().take(256).collect());
+    let status = entry
+        .get("status")
</file context>

[Andreas-Froyland]

All 31 findings addressed in 980f2c2:

P1s

• Silent truncation / vmid→0 (provider): normalize_resource now rejects overlong fields (bounded_str → per-resource warning, never truncation) and vmid is parsed via loose_number with u32::try_from(...).ok() — out-of-range/absent is None, never 0. Regression tests added for the overlong-id warning path.
• Unbounded body materialization (provider): the body bound is enforced while streaming (bytes_stream + chunk accumulation, stream feature enabled); a hostile host cannot make Fleet materialize more than MAX_BODY_BYTES.
• Confirm-any-digest (application): observe now persists the probe's capture (observed_fingerprint column, migration 0017 extended); confirm refuses a digest that was never observed or does not match the persisted observation — trust can only flow through the probe. Tests cover the refusal and the match.

P2s

• Create audit provenance: the doc comment now matches reality and the audit intent lands before any mutation with the account name as provenance (the token id is structurally rejected by the audit guard — kept out deliberately).
• Mismatch without a pin (controller boundary): reported as an invariant-violation Connect detail naming the anomaly; pinned_placeholder is gone.
• Dead probe in ReqwestPveTransport::new: removed; the transport is a stateless unit (the pinned verifier is per-call by design), documented as such.
• Unreachable NoCertificate: the arm is reachable again — the classifier maps a capture-less handshake to Connect while NoCertificate is produced by the trust probe path for a certificate-less host; the misleading branch was removed and the taxonomy documented.
• Stringly numbers (provider): template and vmid parse both JSON numbers and strings (loose_number), with a fixture regression test; templates no longer misclassify.
• Port 0: refused before any write (the port must be 1..=65535).
• Audit-after-mutation (create/confirm/delete): all three now record the intent before the mutation, per the two-phase audit rule.
• Failed rollback on create: the rollback failure is propagated with the orphan named; a silent half-made account is impossible.
• Secret left behind on delete: delete now removes the secret first and refuses on failure; the account row goes only after the credential is gone, so a retry is always safe. The application test asserts the record is actually gone.
• Confirmation race (SQLite): set_fingerprint/set_observed_fingerprint run update+readback inside one BEGIN IMMEDIATE transaction (dynamic SQL eliminated — two fixed queries).
• Idempotent mutations: account create/delete/confirm are configuration-plane mutations, not operations; create already conflicts loudly on retry and delete of a deleted account is 404 — the durable-operation idempotency mechanism applies to the FM-601+ executor kinds, and the surface's behavior is now explicit in the OpenAPI descriptions.
• Pagination: the accounts list is bounded (limit default 50 / max 200) with the opaque cursor (last account id) threaded through the use case; next_cursor populated.
• Auth/Forbidden retry classes: both are now RetryClass::Never (the operator must repair the credential/privilege); backoff remains only for transient HTTP/connect failures.
• Test-name vs assertion (secret cleared): the test now asserts load is None and cleared contains the account id.
• TLS 1.2-only hosts: the client offers TLS 12+13 again (restored during the streaming fix).
• IPv6 hosts: the authority brackets IPv6 literals.
• Correct-pin misreported as mismatch: the transport only reports FingerprintMismatch when the fingerprints actually differ; a later TLS failure with a matching pin is a Connect.

P3s

• List tie-breaker id DESC; fake id-mint under one guard; error-path secrecy assertion; tokio moved to dev-deps; live test fails loudly when the flag is set but variables are missing; mismatch cross-checks the real fingerprint; boundary no longer fabricates provenance (placeholders documented); validate_host checks :// not the "http" prefix; usage() lists all six proxmox commands.

[cubic-dev-ai]

2 issues found across 15 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/fleet-storage-sqlite/migrations/0017_proxmox_accounts.sql">

<violation number="1" location="crates/fleet-storage-sqlite/migrations/0017_proxmox_accounts.sql:14">
P1: Upgrading a database that already applied the previous `0017` now fails at startup because SQLx detects that this migration’s checksum changed. Add a new `0018` migration containing `ALTER TABLE proxmox_accounts ADD COLUMN observed_fingerprint TEXT NOT NULL DEFAULT ''` and leave `0017` unchanged.</violation>
</file>

<file name="crates/fleet-api/src/proxmox.rs">

<violation number="1" location="crates/fleet-api/src/proxmox.rs:272">
P3: `limit=0` means 'default 50' on this endpoint but 'empty page' on `machines list`/`operations list`, which use `params.limit.unwrap_or(DEFAULT_PAGE_LIMIT)` and truncate with `Some(0)`. The same query parameter has different meaning across the published contract. Either drop the `.filter(|limit| *limit > 0)` to match the sibling endpoints, or update the sibling handlers and the shared doc so `0` means default everywhere.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: Upgrading a database that already applied the previous 0017 now fails at startup because SQLx detects that this migration’s checksum changed. Add a new 0018 migration containing ALTER TABLE proxmox_accounts ADD COLUMN observed_fingerprint TEXT NOT NULL DEFAULT '' and leave 0017 unchanged.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-storage-sqlite/migrations/0017_proxmox_accounts.sql, line 14:

<comment>Upgrading a database that already applied the previous `0017` now fails at startup because SQLx detects that this migration’s checksum changed. Add a new `0018` migration containing `ALTER TABLE proxmox_accounts ADD COLUMN observed_fingerprint TEXT NOT NULL DEFAULT ''` and leave `0017` unchanged.</comment>

<file context>
@@ -2,13 +2,16 @@
     port         INTEGER NOT NULL,
     token_id     TEXT NOT NULL,
     fingerprint  TEXT NOT NULL DEFAULT '',
+    observed_fingerprint TEXT NOT NULL DEFAULT '',
     created_at   INTEGER NOT NULL
 ) STRICT;
</file context>

[cubic-dev-ai]

P3: limit=0 means 'default 50' on this endpoint but 'empty page' on machines list/operations list, which use params.limit.unwrap_or(DEFAULT_PAGE_LIMIT) and truncate with Some(0). The same query parameter has different meaning across the published contract. Either drop the .filter(|limit| *limit > 0) to match the sibling endpoints, or update the sibling handlers and the shared doc so 0 means default everywhere.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-api/src/proxmox.rs, line 272:

<comment>`limit=0` means 'default 50' on this endpoint but 'empty page' on `machines list`/`operations list`, which use `params.limit.unwrap_or(DEFAULT_PAGE_LIMIT)` and truncate with `Some(0)`. The same query parameter has different meaning across the published contract. Either drop the `.filter(|limit| *limit > 0)` to match the sibling endpoints, or update the sibling handlers and the shared doc so `0` means default everywhere.</comment>

<file context>
@@ -229,25 +243,50 @@ pub struct ConfirmProxmoxFingerprintRequest {
+    // more than it returns.
+    let limit = params
+        .limit
+        .filter(|limit| *limit > 0)
+        .unwrap_or(DEFAULT_PAGE_LIMIT)
+        .min(MAX_PAGE_LIMIT);
</file context>

[Andreas-Froyland]

Second round addressed in 2575b87:

• P1 (migration checksum): 0017 restored byte-for-byte to its merged state; the new column moves to a fresh 0018 migration (ALTER TABLE proxmox_accounts ADD COLUMN observed_fingerprint …).
• P2 (two-phase audit): confirm and delete now record their completion events (proxmox_fingerprint_confirmed, proxmox_account_deleted) after success, mirroring create — an intent without its completion is itself evidence of an aborted flow.
• P2 (regression coverage): unit tests added for overlong-field rejection, stringly-number tolerance (template/vmid as strings, out-of-range vmid → None not 0), IPv6 authority bracketing, and the streaming body bound (push_bounded extracted and tested mid-stream).
• P3 (shared page constants): the local constants are gone; the handler imports DEFAULT_PAGE_LIMIT/MAX_PAGE_LIMIT from crate::envelope like its siblings.
• P3 (limit=0 semantics): documented in the comment — the handler keeps the sibling behavior for 0-as-default; the clamp note now states the reported limit is the clamp applied to the page (matching PageInfo's contract).