V3: Bestool should fall back to mTLS when Tailscale identity untrusted

[passcod]

🦸 Review Hero

• Run Review Hero
• Run Review Hero on Claude (before merge) Final review with Claude just before merging a substantive PR. Tick one or the other.

[review-hero]

[Bugs & Correctness] suggestion

The new module doc states unconditionally that when a tailscale call is refused for the tailnet identity "the transport falls back to mTLS and retries the call", but the raw-requests escape hatch ReqwestTransport::request (line 458) hands a RequestBuilder to the caller and can never retry — a raw POST/PUT over tailscale that canopy refuses still surfaces the 403 with no fallback, while raw get does fall back. Suggest scoping the doc claim (e.g. "generated calls and raw GETs fall back; requests started via request do not"), so callers of the escape hatch aren't surprised by the asymmetry.

[review-hero]

[Bugs & Correctness] suggestion

Error::transport(std::io::Error::other(err.to_string())) flattens the miette error from leave_tailnet (e.g. a certificate-load failure inside build_mtls_http with its wrap context) into a bare string, dropping the source chain — and this fires at exactly the moment the feature activates and an operator most needs to know why tailscale was abandoned and why mTLS could not be built. Consider a lossless conversion into bes_canopy_api::Error (or at least wrapping the original report so its cause chain survives) instead of to_string().

[review-hero]

[Performance] suggestion

Every call now clones the request's headers (and body) on each attempt, even though a retry only ever happens once and only on the rare tailnet-refusal path. The pre-change code consumed the request via into_parts() and moved the body into the reqwest builder, so the hot path was clone-free; the new send(&CanopyRequest) pays a HeaderMap clone (plus a full body copy if the wire type isn't refcounted) on every single canopy call for the sake of retryability. Restructure instead: consume the request in call (e.g. into_parts() into owned parts passed to a private send(parts)), and only rebuild/clone for the second attempt when a fallback actually fires — keeping the per-call allocation profile the same as before this change.

[review-hero]

[Security] suggestion

refuses_tailnet_identity hand-rolls a second parser for canopy's problem+json type field, diverging from the existing problem_slug helper in crates/canopy/src/names.rs:110 which all other problem-type matching goes through. The suffix match ends_with("/auth-tailnet-node-not-permitted") accepts any multi-segment prefix (e.g. /a/b/auth-tailnet-node-not-permitted) but rejects a bare "auth-tailnet-node-not-permitted" slug, whereas problem_slug normalises to the last path segment and would treat both consistently. If canopy ever changes how the type URI is shaped (or emits the bare slug), this duplicated matcher silently stops triggering the mTLS fallback while every other problem-type consumer still works. Extract/reuse the shared slug parsing (e.g. make problem_slug pub(crate) and match problem_slug(...) == Some("auth-tailnet-node-not-permitted"), ideally from a named constant) so the fallback trigger cannot drift from the rest of the codebase's problem-type handling.

[review-hero]

🦸 Review Hero Summary (round 1) · reviewed 6867b9d · on z-ai/glm-5.3-flash:nitro
9 agents reviewed this PR | 0 critical | 4 suggestions | 1 nitpick | Filtering: consensus 3 voters, 4 below threshold

Below consensus threshold (4 unique issues not confirmed by majority)

Location	Agent	Severity	Comment
crates/canopy/src/reqwest_transport.rs:328	Security	suggestion	The switch from tailnet identity to mTLS is logged only at debug. Canopy rejecting the node's tailnet identity is a meaningful security signal — the node may have been de-permitted or removed fro...
crates/canopy/src/reqwest_transport.rs:329	Performance	suggestion	leave_tailnet calls build_mtls_http while holding the state write lock. Building the mTLS client parses the device key PEM into an identity and constructs a fresh reqwest::Client, which can t...
crates/canopy/src/reqwest_transport.rs:391	Performance	nitpick	On any tailscale-path 403, raw_get clones the headers and buffers the entire response body with response.bytes() before checking whether the refusal is a tailnet-identity one — and when it isn'...
crates/canopy/src/reqwest_transport.rs:480	Security	suggestion	The mTLS fallback re-sends the full request — including non-idempotent POST bodies — whenever a tailscale-path call gets a 403 whose body matches the tailnet-refusal type. This is only safe if cano...

Nitpicks

File	Line	Agent	Comment
crates/canopy/src/reqwest_transport.rs	402	Bugs & Correctness	The reconstructed 403 refusal in raw_get copies status and headers but drops the response version, while the parallel construction in send (line 513) also copies version. Copy *refusal.version_mut() = version; for consistency — callers today don't read it, but the asymmetry invites diverg...

Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.

-------

`crates/canopy/src/reqwest_transport.rs:402`: The reconstructed 403 refusal in `raw_get` copies status and headers but drops the response version, while the parallel construction in `send` (line 513) also copies `version`. Copy `*refusal.version_mut() = version;` for consistency — callers today don't read it, but the asymmetry invites divergence if one of them starts to.

-------

`crates/canopy/src/reqwest_transport.rs:135`: The new module doc states unconditionally that when a tailscale call is refused for the tailnet identity "the transport falls back to mTLS and retries the call", but the `raw-requests` escape hatch `ReqwestTransport::request` (line 458) hands a `RequestBuilder` to the caller and can never retry — a raw POST/PUT over tailscale that canopy refuses still surfaces the 403 with no fallback, while raw `get` does fall back. Suggest scoping the doc claim (e.g. "generated calls and raw GETs fall back; requests started via `request` do not"), so callers of the escape hatch aren't surprised by the asymmetry.

-------

`crates/canopy/src/reqwest_transport.rs:478`: `Error::transport(std::io::Error::other(err.to_string()))` flattens the miette error from `leave_tailnet` (e.g. a certificate-load failure inside `build_mtls_http` with its wrap context) into a bare string, dropping the source chain — and this fires at exactly the moment the feature activates and an operator most needs to know why tailscale was abandoned and why mTLS could not be built. Consider a lossless conversion into `bes_canopy_api::Error` (or at least wrapping the original report so its cause chain survives) instead of `to_string()`.

-------

`crates/canopy/src/reqwest_transport.rs:499`: Every call now clones the request's headers (and body) on each attempt, even though a retry only ever happens once and only on the rare tailnet-refusal path. The pre-change code consumed the request via `into_parts()` and moved the body into the reqwest builder, so the hot path was clone-free; the new `send(&CanopyRequest)` pays a `HeaderMap` clone (plus a full body copy if the wire type isn't refcounted) on every single canopy call for the sake of retryability. Restructure instead: consume the request in `call` (e.g. `into_parts()` into owned parts passed to a private `send(parts)`), and only rebuild/clone for the second attempt when a fallback actually fires — keeping the per-call allocation profile the same as before this change.

-------

`crates/canopy/src/reqwest_transport.rs:534`: refuses_tailnet_identity hand-rolls a second parser for canopy's problem+json `type` field, diverging from the existing `problem_slug` helper in crates/canopy/src/names.rs:110 which all other problem-type matching goes through. The suffix match `ends_with("/auth-tailnet-node-not-permitted")` accepts any multi-segment prefix (e.g. `/a/b/auth-tailnet-node-not-permitted`) but rejects a bare `"auth-tailnet-node-not-permitted"` slug, whereas `problem_slug` normalises to the last path segment and would treat both consistently. If canopy ever changes how the type URI is shaped (or emits the bare slug), this duplicated matcher silently stops triggering the mTLS fallback while every other problem-type consumer still works. Extract/reuse the shared slug parsing (e.g. make `problem_slug` pub(crate) and match `problem_slug(...) == Some("auth-tailnet-node-not-permitted")`, ideally from a named constant) so the fallback trigger cannot drift from the rest of the codebase's problem-type handling.