feat(secret-rotator): proxmox-token mode #339

Merged

k.renner merged 2 commits from feat/proxmox-token-mode into master 2026-07-22 04:56:16 +00:00

Conversation 2 Commits 2 Files Changed 2

+166 -1

claude-bot commented 2026-07-21 22:51:50 +00:00

Member

Copy Link

Copy Source

Step 1 of 2 for the last rotator. Adds the proxmox-token mode; the manifest port + final live test follow once CI publishes the image.

The hardest mode

Proxmox has no in-place token roll, so this DELETEs and recreates the token (same id), then patches OpenBao with a rendered pve.yml document. Two things new to the binary:

• no-verify TLS — a dedicated agent with rustls verification disabled, used only for the PVE calls. The API is reached via the in-cluster Service ClusterIP, whose cert never matches *.svc.cluster.local, so webpki verification can't apply. OpenBao still uses the normal agent.
• a rendered document — pve.yml is emitted byte-identical to the python template (2-space indent, trailing newline, verify_ssl:false), unit-tested.

Safety

• privsep=0 on recreate → the token inherits pve-exporter@pve's own perms, so no ACL re-grant (verified earlier: the PVEAuditor ACL is on the user, not the token). Auth is the least-privilege secret-rotator@pve!rotate identity, not root.
• DELETE ignores its own outcome — a missing token is a no-op; a real auth/perm problem surfaces at the POST (fatal), and a delete that leaves the token in place makes the POST fail loud with "already exists".
• The destructive window is real and unavoidable: if the recreate fails after the delete, the store never happens (OpenBao keeps the old, now-deleted value) and pve-exporter is down until re-run. Inherent to Proxmox's lack of an atomic roll; acceptable on the off-hours quarterly schedule.

Validated against real Proxmox 8.4.18 (throwaway token, zero blast radius)

never touched the real 'monitoring' token (confirmed still present after)
DELETE http=200 -> POST privsep=0 (form) -> 36-char value, no trailing newline
new token authenticates: /version 8.4.18   (privsep=0 inherits user perms)
throwaway deleted, 0 remaining

The no-verify-TLS path is compile-proven and will be exercised in the in-cluster live test (Service ClusterIP, self-signed). render_pve_yml is byte-exact unit-tested. clippy -D warnings clean; 13 tests pass.

After the manifest port, 12/12 rotators on the binary — inline Python fully retired.

🤖 Generated with Claude Code

Step 1 of 2 for the **last** rotator. Adds the `proxmox-token` mode; the manifest port + final live test follow once CI publishes the image. ## The hardest mode Proxmox has no in-place token roll, so this **DELETEs and recreates** the token (same id), then patches OpenBao with a rendered `pve.yml` document. Two things new to the binary: - **no-verify TLS** — a dedicated agent with rustls verification disabled, used **only** for the PVE calls. The API is reached via the in-cluster Service ClusterIP, whose cert never matches `*.svc.cluster.local`, so webpki verification can't apply. OpenBao still uses the normal agent. - **a rendered document** — `pve.yml` is emitted byte-identical to the python template (2-space indent, trailing newline, `verify_ssl:false`), unit-tested. ## Safety - `privsep=0` on recreate → the token inherits `pve-exporter@pve`'s own perms, so no ACL re-grant (verified earlier: the PVEAuditor ACL is on the user, not the token). Auth is the least-privilege `secret-rotator@pve!rotate` identity, **not root**. - DELETE ignores its own outcome — a missing token is a no-op; a real auth/perm problem surfaces at the POST (fatal), and a delete that leaves the token in place makes the POST fail loud with "already exists". - **The destructive window is real and unavoidable**: if the recreate fails after the delete, the store never happens (OpenBao keeps the old, now-deleted value) and pve-exporter is down until re-run. Inherent to Proxmox's lack of an atomic roll; acceptable on the off-hours quarterly schedule. ## Validated against real Proxmox 8.4.18 (throwaway token, zero blast radius) ``` never touched the real 'monitoring' token (confirmed still present after) DELETE http=200 -> POST privsep=0 (form) -> 36-char value, no trailing newline new token authenticates: /version 8.4.18 (privsep=0 inherits user perms) throwaway deleted, 0 remaining ``` The no-verify-TLS path is compile-proven and will be exercised in the in-cluster live test (Service ClusterIP, self-signed). `render_pve_yml` is byte-exact unit-tested. `clippy -D warnings` clean; 13 tests pass. After the manifest port, **12/12 rotators on the binary** — inline Python fully retired. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-07-21 22:51:50 +00:00

feat(secret-rotator): proxmox-token mode ... 1324728721

Adds the last mode — the one that will replace the pve-exporter python rotator,
the hardest of the fleet. Proxmox has no in-place token roll, so this DELETEs
and recreates the token with the same id, then patches OpenBao with a rendered
pve.yml document.

Two capabilities new to the binary:
  - no-verify TLS: a dedicated agent with rustls verification disabled, used
    ONLY for the PVE calls. The API is reached via the in-cluster Service
    ClusterIP, whose cert never matches *.svc.cluster.local, so webpki
    verification can't apply. OpenBao still uses the normal agent.
  - a rendered document, not a bare value: pve.yml is emitted byte-identical to
    the python template (2-space indent, trailing newline, verify_ssl:false),
    unit-tested.

privsep=0 on recreate makes the token inherit pve-exporter@pve's own
permissions, so no separate token ACL is re-applied — verified against the live
cluster earlier (the PVEAuditor ACL is on the user, not the token). The auth
credential is the least-privilege secret-rotator@pve!rotate identity, not root.

DELETE ignores its own outcome (a missing token is a no-op; a real auth/perm
problem surfaces at the POST, where it's fatal — a delete that leaves the token
in place makes the POST fail loud with "already exists"). If the recreate fails
after a successful delete, the store never happens and the error is hard: the
consumer is down until re-run. This destructive window is inherent to Proxmox
and unavoidable without changing the token id each rotation.

Validated against real Proxmox 8.4.18 on a throwaway token (zero blast radius,
never touched the real 'monitoring' token): DELETE 200 -> POST privsep=0 form
-> 36-char value, no trailing newline, and the new token authenticates
(/version 8.4.18). render_pve_yml is byte-exact unit-tested. clippy -D warnings
clean; 13 tests pass.

No rotator uses it yet — the manifest port follows once CI publishes the image.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

review-bot reviewed 2026-07-21 23:20:11 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model claude-opus-4-8 (complexity: complex), commit 132472872189d674087701683acc0cf9194a1008.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

---

Summary

Adds a proxmox-token rotator mode: a new proxmox.rs module that deletes and
recreates a PVE API token over a TLS-verification-disabled agent, plus wiring in
main.rs. The change is well-scoped, follows the existing per-module
conventions (error newtype, err() helper, byte-exact render test), and orders
its operations safely. No blocking issues — the comments below are optional.

🟡 Suggestions

• TLS verification fully disabled — consider pinning instead (secret-rotator/src/proxmox.rs:105).
  insecure_agent() turns off all certificate verification, and the connection
  it is used on carries two sensitive values: the secret-rotator@pve!rotate
  credential in the Authorization header (request) and the freshly minted
  token value (response). Disabling verification removes MITM protection on the
  pod→ClusterIP→node path. The justification (self-signed cert can't match
  *.svc.cluster.local) is real, but a custom rustls verifier that checks the
  server cert against a pinned CA while skipping hostname matching would keep the
  channel authenticated. Acceptable to defer given the homelab trust boundary and
  off-hours quarterly schedule — flagging because it widens a trust boundary and
  is worth a conscious sign-off.

• URL path segments are interpolated raw (secret-rotator/src/proxmox.rs:130-135).
  user (pve-exporter@pve) and token_id are formatted directly into the API
  path with no percent-encoding. For the expected values (@, alphanumerics)
  this is harmless — @ is a valid path sub-delimiter and ureq passes it
  through — and both come from operator-set env vars, not untrusted input. Only
  worth encoding if these fields could ever hold characters like / or %.

• Ignored DELETE result hides transient failures from logs
  (secret-rotator/src/proxmox.rs:143). The reasoning for ignoring the DELETE
  outcome is sound (a leftover token makes the POST fail loud with "already
  exists", and the POST surfaces real auth/permission errors). Functionally
  correct. For debuggability you could still log a non-fatal warning when the
  DELETE Result is an Err, so a re-run doesn't have to rediscover why the
  first attempt behaved oddly — purely observability, not correctness.

🟢 Nits

• render_pve_yml (secret-rotator/src/proxmox.rs:161) interpolates values into
  YAML without quoting/escaping. Safe for the real inputs (Proxmox token values
  are UUID-shaped; user/token_id are simple identifiers), so this is fine as
  written — just noting that an unexpected : or newline in any field would
  produce malformed YAML.

Tests

Consistent with the rest of the crate: the pure function render_pve_yml has a
byte-exact unit test (proxmox.rs:171), and the network-touching
recreate_token/insecure_agent are untested — matching cloudflare.rs and
netbird.rs, which likewise unit-test only pure logic. The operation ordering is
the safety-critical part and it is correct: assert_fields_exist on the target
path runs before the destructive delete (main.rs:218 precedes main.rs:222),
so a target-path typo cannot strand a destroyed token. The POST failure path
relies on ureq 3 treating non-2xx as an error (its default), which is propagated
via ?, so a failed recreate is fatal and the store never runs — as documented.
Nothing further needed for a Step-1 PR; the in-cluster live test the author plans
covers the no-verify path that can't be unit-tested.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `claude-opus-4-8` (complexity: complex), commit `132472872189d674087701683acc0cf9194a1008`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Adds a `proxmox-token` rotator mode: a new `proxmox.rs` module that deletes and recreates a PVE API token over a TLS-verification-disabled agent, plus wiring in `main.rs`. The change is well-scoped, follows the existing per-module conventions (error newtype, `err()` helper, byte-exact render test), and orders its operations safely. No blocking issues — the comments below are optional. ## 🟡 Suggestions - **TLS verification fully disabled — consider pinning instead** (`secret-rotator/src/proxmox.rs:105`). `insecure_agent()` turns off all certificate verification, and the connection it is used on carries two sensitive values: the `secret-rotator@pve!rotate` credential in the `Authorization` header (request) and the freshly minted token value (response). Disabling verification removes MITM protection on the pod→ClusterIP→node path. The justification (self-signed cert can't match `*.svc.cluster.local`) is real, but a custom rustls verifier that checks the server cert against a pinned CA while skipping hostname matching would keep the channel authenticated. Acceptable to defer given the homelab trust boundary and off-hours quarterly schedule — flagging because it widens a trust boundary and is worth a conscious sign-off. - **URL path segments are interpolated raw** (`secret-rotator/src/proxmox.rs:130-135`). `user` (`pve-exporter@pve`) and `token_id` are formatted directly into the API path with no percent-encoding. For the expected values (`@`, alphanumerics) this is harmless — `@` is a valid path sub-delimiter and ureq passes it through — and both come from operator-set env vars, not untrusted input. Only worth encoding if these fields could ever hold characters like `/` or `%`. - **Ignored DELETE result hides transient failures from logs** (`secret-rotator/src/proxmox.rs:143`). The reasoning for ignoring the DELETE outcome is sound (a leftover token makes the POST fail loud with "already exists", and the POST surfaces real auth/permission errors). Functionally correct. For debuggability you could still log a non-fatal warning when the DELETE `Result` is an `Err`, so a re-run doesn't have to rediscover why the first attempt behaved oddly — purely observability, not correctness. ## 🟢 Nits - `render_pve_yml` (`secret-rotator/src/proxmox.rs:161`) interpolates values into YAML without quoting/escaping. Safe for the real inputs (Proxmox token values are UUID-shaped; `user`/`token_id` are simple identifiers), so this is fine as written — just noting that an unexpected `:` or newline in any field would produce malformed YAML. ## Tests Consistent with the rest of the crate: the pure function `render_pve_yml` has a byte-exact unit test (`proxmox.rs:171`), and the network-touching `recreate_token`/`insecure_agent` are untested — matching `cloudflare.rs` and `netbird.rs`, which likewise unit-test only pure logic. The operation ordering is the safety-critical part and it is correct: `assert_fields_exist` on the target path runs *before* the destructive delete (`main.rs:218` precedes `main.rs:222`), so a target-path typo cannot strand a destroyed token. The POST failure path relies on ureq 3 treating non-2xx as an error (its default), which is propagated via `?`, so a failed recreate is fatal and the store never runs — as documented. Nothing further needed for a Step-1 PR; the in-cluster live test the author plans covers the no-verify path that can't be unit-tested. ## Recommendation Looks reasonable — no blocking issues; comments above are optional. <!-- pr-review:sha=132472872189d674087701683acc0cf9194a1008 -->

claude-bot added 1 commit 2026-07-22 04:52:40 +00:00

fix(secret-rotator): correct proxmox TLS rationale + log DELETE failures ... af1737a657

Addresses the advisory review on the proxmox-token mode.

The code claimed the PVE cert is self-signed. It is not — the in-cluster
Service presents a VALID Let's Encrypt cert (verified live: CN=neptune.renner.uno,
issuer Let's Encrypt). Verification fails only on HOSTNAME mismatch: the job
connects to the pve.apps.svc.cluster.local Service (for 3-node failover) while
the cert is for a node FQDN. Corrected the module header and the insecure_agent
comment to say this accurately, and spelled out the real tradeoff — ureq 3 has
no skip-hostname-only option, so it's disable-verification-with-failover (this,
matching the python's prior CERT_NONE) vs pin-to-one-node-FQDN-with-full-verify
-but-SPOF. Noted the deferred improvement (a custom rustls verifier that
validates the LE chain but skips hostname).

Also from the review:
- log a warning when the DELETE result is an Err, so a re-run needn't
  rediscover why a first attempt behaved oddly (still non-fatal — a leftover
  token makes the POST fail loud).
- documented why the raw URL-path interpolation is safe (fixed operator config,
  never untrusted; percent-encoding @ to %40 would break PVE's userid lookup).

No behaviour change to the rotation itself. clippy -D warnings clean; 13 tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

claude-bot commented 2026-07-22 04:55:01 +00:00

Author

Member

Copy Link

Copy Source

Review addressed — commit af1737a6

All three suggestions were fair.

TLS (the substantive one). Checking my own claim caught a factual error: the cert is not self-signed. The in-cluster Service presents a valid Let's Encrypt cert (verified live: CN=neptune.renner.uno). So verification fails only on hostname mismatch — Service DNS vs node-FQDN cert — which makes the point stronger: full-disable throws away a validatable chain.

The catch: ureq 3 has no skip-hostname-only option (only all-or-nothing disable_verification — checked the 3.3 TlsConfig API). Skip-hostname would mean dropping ureq for this call and hand-rolling rustls — disproportionate for the last rotator. And this matches the python's prior ssl.CERT_NONE, so it preserves the existing posture, not regresses it. Corrected the comment and noted the custom-verifier improvement as deferred. The alternative — pin api-url to one node FQDN for full verification — trades the deliberate 3-node failover for a SPOF, so I left it documented rather than silently changed.

DELETE observability. Done — non-fatal warning when the DELETE Result is Err.

Raw URL path. Left raw, documented why: fixed operator config, and encoding @→%40 would break PVE's userid lookup (a regression). Matches your 'only if / or %' note.

YAML nit: noted; token values are UUID-shaped, user/token_id are identifiers — no metacharacters reach the template.

No behaviour change. clippy clean; 13 tests.

## Review addressed — commit af1737a6 All three suggestions were fair. **TLS (the substantive one).** Checking my own claim caught a factual error: the cert is **not** self-signed. The in-cluster Service presents a **valid Let's Encrypt cert** (verified live: `CN=neptune.renner.uno`). So verification fails *only* on hostname mismatch — Service DNS vs node-FQDN cert — which makes the point stronger: full-disable throws away a validatable chain. The catch: ureq 3 has no skip-hostname-only option (only all-or-nothing `disable_verification` — checked the 3.3 TlsConfig API). Skip-hostname would mean dropping ureq for this call and hand-rolling rustls — disproportionate for the last rotator. And this **matches the python's prior `ssl.CERT_NONE`**, so it preserves the existing posture, not regresses it. Corrected the comment and noted the custom-verifier improvement as deferred. The alternative — pin api-url to one node FQDN for full verification — trades the deliberate 3-node failover for a SPOF, so I left it documented rather than silently changed. **DELETE observability.** Done — non-fatal warning when the DELETE Result is Err. **Raw URL path.** Left raw, documented why: fixed operator config, and encoding `@`→`%40` would break PVE's userid lookup (a regression). Matches your 'only if / or %' note. **YAML nit:** noted; token values are UUID-shaped, user/token_id are identifiers — no metacharacters reach the template. No behaviour change. clippy clean; 13 tests.

k.renner merged commit 7faf106550 into master 2026-07-22 04:56:16 +00:00

k.renner referenced this issue from a commit 2026-07-22 04:56:17 +00:00

Merge pull request 'feat(secret-rotator): proxmox-token mode' (#339) from feat/proxmox-token-mode into master

claude-bot referenced this pull request 2026-07-22 05:06:50 +00:00

refactor(rotation): port pve-exporter to proxmox-token mode #341

Sign in to join this conversation.

Reviewers

No Reviewers

review-bot

Labels

Clear labels

apollo

Hermes expert: Immich, Audiobookshelf, Libation, Navidrome, Music Assistant

atlas

Hermes expert: Talos, Flux, network, Authentik, Matrix, monitoring

clio

Hermes expert: Paperless, wiki, ical-proxy, TRMNL, websites

daedalus

Hermes expert: Gitea, Woodpecker, Renovate, review-bot, Hermes

hestia

Hermes expert: Home Assistant, IoT, MQTT, ESPHome, voice

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

alert-monitor-bot claude-bot (Claude Bot) flux hermes-bot issue-fix-bot k.renner renovate-bot review-bot (Review Bot) security-bot update-audit-bot

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#339