Devolutions / Devolutions/devolutions-gateway

Follow-up: agent-tunnel cert renewal review comments from #1775

Open
#1,788 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
159
Forks
28
Avg merge
2d 23h
Merged PRs (30d)
21

Description

Follow-up to the Copilot review on #1775 (`feat(agent): implement certificate renewal`). The PR is merged as-is because none of the items below affect the happy path, but they should be cleaned up.

## Items

### [P0] Atomic write for renewed cert + CA — `devolutions-agent/src/tunnel.rs:521`

- [ ] Replace the two `std::fs::write(cert_path, ...)` / `std::fs::write(ca_path, ...)` calls with a write-temp-then-rename sequence (same directory, then `std::fs::rename`).
- [ ] On Unix, set `0o600` on the temp file before rename — match what `persist_enrollment_response` already does for the initial enrollment.

**Why this matters:** the current `std::fs::write` truncates in place. A crash, power loss, or full disk between the two writes leaves the client cert / CA file half-written; future mTLS handshakes then fail and the agent **cannot recover remotely** — admin has to re-enroll the machine on-site. Happy path is fine; only the failure mode is bad, and the failure mode is expensive.

### [P1] Don't fail the connection on local CSR-prep errors — `devolutions-agent/src/tunnel.rs:498`

- [ ] In `try_renew_certificate`, replace the `?` on `read_agent_name_from_cert` and `generate_csr_from_existing_key` with `match` arms that log a warning and return `Ok(None)`.

**Why this matters:** the doc comment on `try_renew_certificate` says only control-stream IO / protocol errors should be `Err`, but right now a transient local file read failure (e.g. permission blip) bubbles up to `run_single_connection`'s `Err` branch and forces a backoff/reconnect on an otherwise healthy QUIC connection. Should be a recoverable \"skip renewal, keep using existing cert\" path.

### [P2] Unit tests for new renewal helpers — `devolutions-agent/src/enrollment.rs:248-318`

- [ ] `is_cert_expiring`: generate a short-lived rcgen cert, assert true above threshold and false below.
- [ ] `read_agent_name_from_cert`: round-trip a known CN.
- [ ] `generate_csr_from_existing_key`: assert the produced PEM parses back via `rcgen::CertificateSigningRequestParams::from_pem` and that the public key matches the input key.

### [P3] Fix misleading comment — `devolutions-agent/src/enrollment.rs:282`

- [ ] Rewrite the comment on `read_agent_name_from_cert`. The current text implies the gateway looks the agent up in its registry by `CommonName`, but `AgentRegistry` is keyed by `agent_id` (UUID, extracted from SAN `urn:uuid:`). The real reason to reuse the CN in the renewal CSR is just to keep the issued cert's CN stable across renewals — the gateway ignores the CSR subject anyway.

## Original review

https://github.com/Devolutions/devolutions-gateway/pull/1775#pullrequestreview-4200093301

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.