Skip to content

Add SSH certificate authentication for targets, issued by Vault - #2397

Open
janisdombr wants to merge 63 commits into
warp-tech:mainfrom
janisdombr:feat/vault-ssh-certificate-auth
Open

Add SSH certificate authentication for targets, issued by Vault#2397
janisdombr wants to merge 63 commits into
warp-tech:mainfrom
janisdombr:feat/vault-ssh-certificate-auth

Conversation

@janisdombr

@janisdombr janisdombr commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Warpgate authenticates to an SSH target with a short-lived OpenSSH user certificate signed on demand by HashiCorp Vault, instead of a private key it stores. The ephemeral keypair is generated per connection and never persisted, so a compromise of the Warpgate host yields nothing a target would accept.

Targets trust the CA through TrustedUserCAKeys and need no authorized_keys. The certificate's key ID carries the Warpgate username and session UUID, so the target's own sshd log attributes a proxied session to a person rather than to the gateway.

VaultAuth offers workload identity only — kubernetes, AppRole, AWS, Azure and GCP. Each reads its credential from a file or a metadata service, never from the config: a static Vault password would merely relocate the long-lived secret this feature exists to remove. Full compatibility with OpenBao is supported.

Verified end to end against real infrastructure — AWS STS, a GCE instance, an Azure VM and a k3d cluster.

tests/test_ssh_target_cert_auth.py runs against a stub issuer and needs neither Vault nor a cluster.

Discussion: #26

Special thanks to @theredspoon for the detailed test, OpenBao evaluation, and security recommendations.

Description

...

AI Usage

Choose the level of AI involvement for this PR.

  • Fully vibe coded
  • AI-designed, AI-coded, manually checked
  • Human-designed, AI-coded
  • Human-designed, human-coded (includes AI autocompletions and boilerplate gen)

This is not to block AI contributions but rather to speed up PR review (saves time on trying to deduce the logic behind AI hallucinations).

@rumfellow

Copy link
Copy Markdown

Will be happy to see this merged since it's the only deployment blocker for us due to security concerns.

@theredspoon

Copy link
Copy Markdown
Contributor

Went through this again against the current head (1578fc67), plus a step back from line-level review to look at the design itself.

Real progress since the last round: the AWS static-credential gap is now a genuine, well-designed fix, AwsError::StaticCredentialsDisallowed rejects when no STS session token is present, correctly distinguishing static IAM keys from any temporary/workload-identity credential. And the cached Vault token is now wrapped in zeroize::Zeroizing instead of the old Secret<String>, closing the memory-zeroization gap from last round.

Three things from the last round are still open, each with a concrete fix.

Vault issuer errors reaching the SSH client are still truncated to 256 characters rather than sanitized by content, so a policy or role name can survive that length. The fix is a client_message()-style method on VaultError that returns a generic, category-level message ("Vault denied the certificate signing request," "Vault is currently unavailable") to the SSH client, with the full error logged server-side instead, and this isn't Vault-specific, ConnectionError::Aws falls through the same catch-all in session.rs today, so it's worth fixing as a shared mechanism rather than a one-off for this arm.

stub_vault.py has no request-shape validation for the AWS, Azure, or GCP login paths, only Kubernetes and AppRole get checked, so the suite can't catch a malformed request on three of the five methods, worth adding the same presence checks those two already get.

Also, test_aws_signs_the_global_endpoint_by_default is currently broken, confirmed by actually running it: it supplies static credentials with no session token, so StaticCredentialsDisallowed correctly rejects before Warpgate ever reaches Vault, and the test's own assertion crashes with an empty-list error since Vault is never contacted. Fix is small: add AWS_SESSION_TOKEN to that test's env the same way the sibling test does.

The bigger thing: stepping back from individual lines, there's a structural question worth resolving before this merges. Under the current design, Vault can't distinguish one target/session from another, role, principals, and key_id are all values Warpgate's own code asserts in the signing request, not anything Vault independently verifies. Under the model this replaces, compromising Warpgate's stored credentials was bounded by whatever was actually stored for actually-configured targets. Under this one, a compromised Warpgate can request a cert for any role its Vault token is allowed to sign for, and role defaults to one shared value across every target unless each one is individually configured otherwise. Certs also aren't revocable today, no KRL, no rotation path in this diff. None of this shows up in a line-by-line read, because every line does what it says, it's a property of what the whole system ends up guaranteeing. Posted a concrete proposal for this as a follow-up comment.

One more thing worth knowing before this merges: #2185 also adds Vault integration (a different problem, relocating static secrets into KV rather than issuing certs, but it collides mechanically with this PR in several places, workspace crate registration, Services, ConnectionError, SSHTargetAuth), and its TLS/mount-configurability work already solves two gaps in this PR's own Vault client. Posted the details as a comment on that PR, but flagging it here too since it affects how and when this one should land.

This doesn't mean the direction is wrong. Ephemeral, non-stored credentials is the right fix for a real, long-standing gap, and the mechanics here are solid.

@theredspoon

Copy link
Copy Markdown
Contributor

Opened #2400 with a concrete design for the authorization question from the review above, rather than posting the whole thing inline here.

Short version: the core piece there, identity-templated Vault roles plus per-session scoped child tokens, so Vault verifies the principal instead of trusting what Warpgate asserts, belongs in this PR before merge, not a fast-follow. Without it, this design can plausibly have a worse worst-case blast radius than what it replaces (fleet-wide, non-revocable access versus today's bounded-to-stored-credentials), so it's not a good candidate for shipping as a documented limitation. The remaining hardening in the issue (full IdP-verified non-repudiation, host-binding, revocation) is genuinely separable follow-up work once that baseline is in.

@janisdombr
janisdombr force-pushed the feat/vault-ssh-certificate-auth branch 2 times, most recently from c00a253 to bc0fb04 Compare August 10, 2026 15:19
@janisdombr

Copy link
Copy Markdown
Contributor Author

@theredspoon Thank you for the follow-up review!
I've addressed the three technical points in commit bc0fb04c:

  1. AWS Test Fix: Added AWS_SESSION_TOKEN to test_aws_signs_the_global_endpoint_by_default's test environment so it tests global endpoint signing without triggering StaticCredentialsDisallowed.
  2. SSH Terminal Error Sanitization: Added client_message() to VaultError, AwsError, and ConnectionError. Full error bodies and Vault topology details are now strictly logged server-side (tracing::error!), while SSH client terminals receive safe, generic messages ("Target connection failed: Vault denied the certificate signing request").
  3. Payload Validation in Stub: Added request-shape presence checks for AWS (iam_http_request_method, iam_request_url, iam_request_body, iam_request_headers), Azure (jwt, subscription_id), and GCP (jwt) login paths in stub_vault.py.

@theredspoon

Copy link
Copy Markdown
Contributor

Went through the current head (bc0fb04c) again. Two new issues that weren't caught in earlier rounds, plus a few smaller items.

X-Vault-Token can leak on redirect

warpgate-vault/src/client.rs:94 builds the reqwest::Client with no redirect policy:

let http = reqwest::Client::builder().timeout(config.timeout).build()?;

That leaves reqwest's default policy in place, which follows redirects and only strips Authorization/cookies/proxy-auth headers on a cross-origin hop. It has no concept of X-Vault-Token as sensitive, so a 307/308 from the sign or unwrap endpoint (compromised Vault, misconfigured proxy, or MITM) replays the token to a different host, or moves an HTTPS request to HTTP. Please resolve with redirect::Policy::none() on this client and a regression test asserting the token is never forwarded cross-origin.

Unbounded buffering + panic in error-body truncation

warpgate-vault/src/client.rs:354-367:

let body = response.text().await.unwrap_or_default();
let max_len = 256;
let body = if body.len() > max_len {
    format!("{}... (truncated)", &body[..max_len])
} else {
    body
};

response.text() buffers the entire body before the length check runs, so a hostile or misbehaving endpoint can force unbounded allocation. Separately, &body[..256] panics whenever byte 256 falls inside a multi-byte UTF-8 character (255 ASCII bytes followed by é, for example). This is reachable from anything answering as the configured Vault address. Please resolve by streaming a bounded prefix and truncating at a char boundary, or truncating the already-bounded raw bytes lossily.

Smaller items

  • read_credential() (client.rs:338-348) zeroizes the buffer it reads into, but returns a fresh, non-zeroized trimmed copy that then gets copied again into the JSON login body. The cached Vault token is correctly zeroized; the K8s JWT / AppRole secret ID / wrapping token read from disk are not. Please resolve end-to-end: zeroize trimmed and the JSON copy too, not just the initial read buffer.
  • tests/stub_vault.py:61-82: the Azure login check only requires jwt + subscription_id, not resource_group_name/vm_name/vmss_name; the AWS check only verifies four fields are truthy without decoding or checking the request is actually GetCallerIdentity. Some of this is covered by assertions elsewhere in the integration tests, but the stub itself validates less than its shape suggests.
  • Azure/GCP metadata_address is administrator-configurable and blindly GETed (warpgate-common/src/config/mod.rs:458 / metadata::gcp_identity_token), so a compromised config file can trigger SSRF from the Warpgate host. This doesn't cross a privilege boundary on its own since editing warpgate.yaml already requires host access, but it should get a line in the docs.

@janisdombr

Copy link
Copy Markdown
Contributor Author

@theredspoon both fixed, thanks.

Redirects are refused outright now, which covers the metadata calls too. The error body is read chunk-wise with a 256-byte cap and truncated lossily, so a split character can't panic it. Chasing that one, I found the success path had
no bound at all — a 200 MB signed_key took a live gateway from 74 MB to 680 MB RSS, per session in flight.

Zeroization is end-to-end now: the login body goes through typed structs instead of a serde_json::Value, so there's no stray copy of the JWT or secret ID left around. The one remaining is reqwest's own send buffer, which I
commented rather than pretended about.

The stub validators actually validate now — decoded AWS payload, full Azure coordinates, JWT shape, GCP audience — and have tests of their own. You were right that they were asserting nothing.

A pass over the rest turned up a few more: lease_duration: 0 was read as expired, so every request re-logged in; nothing was checked about the returned certificate, so a host cert or one over a key we don't hold both went on the
wire; a comma in the target username widened valid_principals. Also added an optional certificate_ttl.

One I'd like your view on: a role with default_critical_options can put a force-command in the cert, and the target runs that instead of what the user typed. I made it warn rather than refuse — a restricted role might set one
deliberately, and a hostile Vault has target access anyway. If you think the stealth is the point, I'll make it refuse behind an opt-in.

425fb05. metadata_address is in the docs now. OpenBao offer still very
welcome.

@janisdombr
janisdombr force-pushed the feat/vault-ssh-certificate-auth branch 2 times, most recently from 4d8e294 to dd06172 Compare August 10, 2026 22:46
@theredspoon

Copy link
Copy Markdown
Contributor

Confirmed everything in dd061726 — redirect refusal, the chunked/lossy truncation fix, the response-size cap, lease_duration: 0 handling, the certificate type/key checks, and the comma-in-principal validation all hold as described. Ran the focused test suite too, all passing.

On critical_options: refuse by default, gated behind an explicit per-target opt-in.

The "hostile Vault already has target access anyway" framing undersells this. force-command isn't a subset of what a fully compromised Vault could already do directly: it runs under the connecting user's own principal and key_id, so it launders attribution in the target's own sshd log in a way a direct malicious connection never would. There's also a lower-privilege path than full Vault compromise: Vault's ACLs separate write access to ssh/roles/* from sign/* and from actual network reachability to targets. Someone with only role-config write, no signing rights and no path to the target, could plant a default_critical_options on a role and wait for a legitimate session to carry it through Warpgate. That's a materially lower bar than the #2400 threat model, and Warpgate is the only place that check can land.

The realistic case day to day is more mundane than either: a legitimate, uncompromised Vault, an operator who copies or templates a role with default_critical_options set, and a connecting user who gets no signal at all beyond a warn-level server log they're not watching.

Suggest: default-reject any critical option. Per-target opt-in as a named allow-list of expected option keys, not a bare boolean, and for force-command specifically, pin the exact expected command where practical rather than accepting any value. A rejection should reach the user the same way the certificate-mismatch errors do now, not just the log.

Two more, from this round:

lease_duration can panic Warpgate. warpgate-vault/src/client.rs:348-350:

expires_at: (auth.lease_duration > 0).then(|| {
    Instant::now() + Duration::from_secs(auth.lease_duration).saturating_sub(TOKEN_EXPIRY_MARGIN)
}),

lease_duration is an untrusted u64 straight from Vault's response, no upper bound. Instant + Duration panics on overflow. A misbehaving or compromised Vault returning an oversized lease crashes the process, on every login path. Please resolve with checked_add, rejecting an unrepresentable lease as an API error rather than crashing on it.

IPv6 loopback is misclassified as insecure. validate_address (client.rs:38) checks host == "::1", but url::Url::host_str() returns "[::1]" with brackets for an IPv6 host, confirmed by compiling and checking directly. A genuine loopback IPv6 Vault address (http://[::1]:8200) gets rejected as insecure the same as a real remote HTTP address would. Low severity, but a real bug for anyone running Vault dev-mode over IPv6 loopback.

One more, lower priority: the AWS path is the one exception to end-to-end zeroization. StsIdentityRequest.headers (an ordinary HashMap<String, String> carrying the SigV4 signature and session token) and the base64-encoded strings built from it in aws_login_body() are never wrapped in Zeroizing, unlike the Kubernetes/AppRole/Azure/GCP/token paths. Worth closing for consistency, not urgent given these are temporary credentials rather than static keys.

@theredspoon

theredspoon commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Ran a wider architectural sweep across the codebase, not just this PR's diff, then went back and verified every proposed fix against the real code and this PR's own existing patterns.

Certificate minting via the host-key-check admin endpoint

warpgate-admin/src/api/ssh_connection_test.rs's connection-test handler returns to its own caller as soon as HostKeyReceived fires, but the underlying RemoteClient task doesn't get that signal and falls through to authenticate_session regardless (warpgate-protocol-ssh/src/client/mod.rs:824). For a Certificate-auth target with an already-trusted host key, that mints a real cert and opens a real authenticated session in the background. Measured directly with a throwaway integration test: the session holds for a minimum of 310.6 seconds (the 5-minute inactivity timeout plus a 10s pad), indefinitely if ssh.keepalive_interval is configured, and the task itself leaks on every press, since create() discards its JoinHandle (client/mod.rs:361) and the read loop never exits on this path, so Drop for RemoteClient never runs. The throwaway session UUID is never registered via register_session, so certificate_key_id() falls back to warpgate:<random-uuid> with no username. (First press against an untrusted host key self-cleans in ~1ms, since HostKeyUnknown fires after the admin loop already broke on HostKeyReceived — it's every press after the key is trusted that holds.)

The fix needs to be a deterministic signal, not a race. abort_rx/abort_tx exist, and having the task treat a dropped abort_tx as cancellation is safe for real sessions (ServerSession's Drop impl explicitly sends the abort signal first, so there's no legitimate case where a real session drops it while still wanting the connection) — but tokio::select! is unbiased, and authenticate_session is called inside the same fut_connect arm that resolves concurrently with HostKeyReceived firing mid-KEX, so a drop signal alone still loses the race roughly half the time. Please resolve with an explicit intent passed from ssh_connection_test.rs into the connect command (a stop_after_host_key flag or a dedicated command variant) that makes wait_for_connection return before authenticate_session deterministically, not conditionally on a race. The dropped-abort_tx handling is still worth adding as defense in depth alongside it.

Separately: create() should hold and abort its JoinHandle instead of discarding it, but scoped to the admin-side caller specifically. Applying it to the shared RemoteClientHandles type would hard-abort real sessions instead of letting ServerSession::drop's existing graceful disconnect() run, losing a clean Disconnect::ByApplication on the target side.

Vault config doesn't hot-reload

services.rs:80-86 builds VaultClient once from the startup config snapshot into vault: Option<Arc<VaultClient>>. Every other config section hot-reloads through the watch::Sender in config.rs's reload path; Vault was never wired in. VaultConfig/VaultAuth are missing PartialEq/Eq (all fields are String/PathBuf/Option/Duration, so deriving both is straightforward, and ListenerParams already does the same for the same reason). The swappable-cell mechanism this needs already exists in this codebase: warpgate-core/src/rate_limiting/swappable_cell.rs's SwappableLimiterCell, built on watch::Sender<Option<T>>, documented as "a cell containing a reference which can be swapped out wholesale." One ordering constraint: Services::new runs before watch_config is called (run.rs:83 vs :122), so the rebuild loop can't live inside Services::new — it needs to be spawned from run.rs after watch_config, the same place the existing ListenerSupervisor gets spawned, following that same diff-and-rebuild shape (listener_supervisor.rs:132-171), keeping the old client on a validation failure the same way it keeps the current listener. Please resolve, editing or removing the vault: section currently has no effect until a restart.

Cloud metadata tokens can transit an ambient proxy

VaultClient's single reqwest::Client (client.rs:202-205, comment at :200 says "the same client fetches cloud metadata") is used both for real Vault calls and for Azure/GCP metadata-token fetches, with no explicit proxy policy, so reqwest's default of honoring HTTP_PROXY/HTTPS_PROXY/NO_PROXY applies. Both metadata defaults are plain HTTP (169.254.169.254, metadata.google.internal); GCP's is a hostname, which a typical IP-based NO_PROXY list won't match. Please resolve with a second metadata_http: reqwest::Client built with .no_proxy(), used only for the two metadata call sites, leaving the main Vault-address client's ambient proxy support untouched. AWS doesn't go through this client.

key_id enforcement and error messages

Checked against Vault's actual server source (calculateKeyID, builtin/logical/ssh/path_issue_sign.go, unchanged since v1.4.0): a role with allow_user_key_ids=false only falls back to the token's display name when the caller sends no key_id at all. When a non-empty key_id is sent and the role doesn't allow it, Vault returns an error and issues nothing. Warpgate always sends a non-empty key_id (client/mod.rs:841), so a misconfigured role already fails closed today, there's no silent wrong-attribution here, and a client-side key_id check would be unreachable.

Three real things remain from that investigation:

  • The error Warpgate surfaces for this case is the generic "Vault denied the certificate signing request." Please resolve by mapping the specific setting key_id is not allowed by role response body to a message that names the fix (allow_user_key_ids=true on the role).
  • certificate_mismatch() doesn't check valid_principals. Vault returns the requested principal set verbatim (trimmed, deduped, sorted) or hard-errors, never silently widens it, independent of the key_id question above. Please resolve by adding that check, using certificate.valid_principals().iter().any(|p| p == principal) rather than exact equality, since Vault sorts the returned set.
  • Found separately while checking this: a Warpgate-side refusal (the existing cert-type/pubkey mismatch checks, or the principals check above) currently surfaces as ConnectionError::Authentication, whose client message is "SSH target rejected Warpgate's authentication request" — inaccurate, since the target never received anything in that case. Please resolve with its own error variant.

Response-wrapped AppRole secret IDs need the unwrapped value cached

login_body() re-reads and re-unwraps the same file on every login (client.rs:375-379), but wrapping tokens are single-use, so every login after the first fails, surfacing only as the same generic "Vault denied the certificate signing request."

Response wrapping protects one-time delivery of the secret ID, it doesn't force single-use of the secret ID itself. secret_id_num_uses/secret_id_ttl separately govern how many times the unwrapped secret ID can authenticate, and HashiCorp's own AppRole guidance for long-running services is to unwrap once at initialization and reuse the result for subsequent logins until it expires. That matches this PR's own README, which already describes the intent as "the secret ID is read fresh on every login, so it can be rotated underneath a running Warpgate", rotation as something available on demand, not required before every login.

Please resolve by caching the unwrapped secret ID, keyed on the raw file content, reusing it while the file is unchanged and only re-unwrapping when the content actually changes (an operator writing a fresh wrapping token). Keep a distinct error for the real failure case, an unwrap attempt (first use, or after a detected change) that fails because the token is stale or already consumed: VaultError::SecretIdUnwrap { path }, naming the file and stating that the provisioning process needs to write a fresh wrapping token, e.g. via vault write -f -wrap-ttl=<ttl> auth/approle/role/<role>/secret-id.

Lower priority

  • A target with an empty username substitutes the connecting Warpgate user's own username as valid_principals. Not a bypass, Vault's allowed_users still rejects anything out of policy, but please resolve by documenting this mode in the README alongside the existing allowed_users guidance.
  • default_extensions on a returned certificate are neither checked nor logged, while critical_options now are. Please resolve by logging default_extensions the same way, for the same operator-visibility reason critical_options was.

@janisdombr
janisdombr force-pushed the feat/vault-ssh-certificate-auth branch from 409e2e5 to 4b825c1 Compare August 11, 2026 06:48
@janisdombr

Copy link
Copy Markdown
Contributor Author

Both rounds are in commit 409e2e5.

@theredspoon
Before the details: your analysis and recommendations are worth more than everything I put in this PR. The code and tests were the easy part; what you did was find the things that made them wrong. Without your reviews this would
have shipped as one continuous hole with a feature description on top, not as an improvement. Twelve findings across the rounds, and the two most serious - the host-key check minting certificates, and the AppRole path that broke on
every login after the first are ones no amount of testing my own diff would have surfaced, because I was testing the diff and you were reviewing the system it landed in.

On critical options you changed my mind. "A hostile Vault already has target access" conflated two different capabilities: force-command isn't extra access, it's laundered attribution, and the target's own log is the thing this feature exists to make trustworthy. The role-write-without-sign path settles it. So: default-reject, per-target allow-list of names with optional pinned values, and the refusal reaches the connecting user rather than a log nobody watches.

Everything else landed as you described it checked_add on the lease, url::Host for IPv6, Zeroizing on the AWS path, the allow_user_key_ids message, valid_principals checked with any not equality, the unwrapped secret ID cached against file content, a VaultCell rebuilt from run.rs beside the listener supervisors, and a separate no_proxy client for metadata.

Two places I'm weaker than I'd like, said plainly:

The host-key check I took the explicit-intent route, a dedicated RCCommand::CheckHostKey that returns before authenticate_session, final hop only. What I can demonstrate is the leak: revert it and my test fails on connections still open after the request returned. What I could not reproduce is the certificate actually being minted the leaked task stalls before signing in my setup, over a 5s window. That assertion is a guard, not evidence; your 310.6s measurement is the real data point. If you can share how you drove it to sign I'll make it deterministic.

The JoinHandle I didn't thread one through. CheckHostKey ends the task, and the admin caller sends an explicit abort afterwards, scoped so ServerSession's graceful disconnect stays untouched. Two mechanisms rather than the third you
suggested; say the word and I'll add it.

Tests are 15 Rust unit and 57 integration, up from 12 and 48. Each new one was verified by breaking the code it defends including one that didn't fail on the first attempt, the valid_principals case, which rejects that certificate too. Rewritten to assert who did the refusing.

Warpgate authenticates to an SSH target with a short-lived OpenSSH user
certificate signed on demand by HashiCorp Vault, instead of a private key it
stores. The ephemeral keypair is generated per connection and never persisted,
so a compromise of the Warpgate host yields nothing a target would accept.

Targets trust the CA through TrustedUserCAKeys and need no authorized_keys.
The certificate's key ID carries the Warpgate username and session UUID, so the
target's own sshd log attributes a proxied session to a person rather than to
the gateway.

VaultAuth offers workload identity only — kubernetes, AppRole, AWS, Azure and
GCP. Each reads its credential from a file or a metadata service, never from
the config: a static Vault password would merely relocate the long-lived secret
this feature exists to remove. Full compatibility with OpenBao is supported.

Verified end to end against real infrastructure — AWS STS, a GCE instance, an
Azure VM and a k3d cluster.

tests/test_ssh_target_cert_auth.py runs against a stub issuer and needs neither
Vault nor a cluster.

Discussion: warp-tech#26

Special thanks to @theredspoon for the detailed test, OpenBao evaluation, and security recommendations.
- The admin host-key check ran on into authenticating to the target. On a
  certificate target that minted a real certificate and opened a real session
  nobody was attached to, held until the inactivity timeout, with a key ID
  naming no user. Now a dedicated RCCommand::CheckHostKey stops before
  authentication, on the final hop only so jump hosts still authenticate.

- A certificate could arrive carrying critical options nobody asked for. A
  force-command there replaces what the user typed while keeping their own
  principal and key ID on the session, so the target's log attributes it to
  them. Write access to a Vault role is a lower bar than the right to sign with
  it, so this is the only place it can be caught. Refused by default; a target
  may name the options it expects and pin their values.

- Nothing checked that the certificate named the account being reached.
  valid_principals is now verified against the target's username.

- A response-wrapped AppRole secret ID was re-unwrapped on every login. A
  wrapping token is single-use, so every login after the first failed, as a
  generic denial. The unwrapped secret ID is now cached against the file
  content, and a genuine unwrap failure names the file and the fix.

- lease_duration from Vault fed an unchecked Instant addition, so an oversized
  lease crashed the process on the login path. Now rejected as a bad response.

- Cloud metadata tokens went through the same client as Vault, which honours
  HTTP_PROXY by default; GCE's hostname defeats a typical IP-based NO_PROXY.
  Metadata now uses a client built with no_proxy().

- The AWS login path was the one place credentials were not zeroized.

- An IPv6 loopback Vault address was classified as a remote plaintext endpoint,
  because host_str renders it with brackets.

- Editing the vault: section had no effect until a restart, alone among config
  sections. A VaultCell on a watch channel is rebuilt from run.rs; a
  configuration that fails to build keeps the working client.

- A certificate Warpgate itself refused reported "SSH target rejected
  Warpgate's authentication request", naming the wrong party. It has its own
  error now, and the reason reaches the connecting user.

- A role that forbids key IDs now produces a message naming allow_user_key_ids.

Tests: 15 Rust unit and 57 integration, up from 12 and 48; each new one
verified by breaking the code it defends. The stub models single-use wrapping
tokens, without which the AppRole defect was invisible.

Found by @theredspoon's review, which is worth more than the code it corrects.
@janisdombr
janisdombr force-pushed the feat/vault-ssh-certificate-auth branch from c5dea27 to 58f831a Compare August 11, 2026 11:54
The stub in tests/ is fast and can be made to misbehave, but it only knows what
we told it — and two of the defects found in review were invisible for exactly
as long as it was the only witness. tests/vault_server.py runs the suite against
a real HashiCorp Vault and a real OpenBao, reading requests back out of the
server's own audit device, so the payload under assertion is the one the server
received. Every behaviour the stub models is now pinned against both.

Three defects came out of it:

- Every login left a copy of the credential in freed memory. login_payload used
  serde_json::to_string, whose String grows as it is written and frees each
  smaller buffer without wiping it; Zeroizing only ever wipes the buffer that
  survives to the end. Size decides whether it shows: measured with a 4 KiB
  credential, which is what a Kubernetes service account token or a signed AWS
  header set actually is. Now serialized into a buffer reserved up front.

- The certificate's key ID was never checked against the one requested. A
  certificate carrying a 64 KiB key ID authenticated normally. The target's sshd
  logs that field verbatim, and "the target's own log names the person" is the
  claim this path exists to deliver, so an issuer returning a different one
  breaks attribution silently.

- The reason an authentication failed never reached the person connecting.
  ConnectionError::Authentication carried no detail; the reason went to the
  server log and the user got a fixed string. For a certificate refused because
  it is outside its validity window — the documented clock-skew hazard — that
  sends whoever is debugging it to check credentials that are fine. The variant
  now carries its reason and the certificate arm names the window.

Also documented: OpenBao refuses to enable an audit device over the API, and its
config stanza needs type, path and an options block — a top-level file_path is
accepted with a warning and then ignored, which looks exactly like a working
audit device that writes nothing.

Tests: 16 contract tests across Vault and OpenBao (five versions under
WARPGATE_VAULT_MATRIX=full), 8 for certificates a real issuer would never emit,
6 property tests over the validators, and 3 that watch the allocator to check
the zeroization claim rather than trusting it.
@janisdombr
janisdombr force-pushed the feat/vault-ssh-certificate-auth branch from 58f831a to d818090 Compare August 11, 2026 12:03
@janisdombr

Copy link
Copy Markdown
Contributor Author

Pushed d818090, rebased onto current main.

This round came from building the test infrastructure rather than from reading the diff again. tests/vault_server.py runs the suite against a real Vault and a real OpenBao, reading requests back out of the server's own audit device, so
assertions are on what the server received rather than on what our stub chose to remember. Three defects fell out:

  • Every login left a copy of the credential in freed memory. serde_json::to_string grows its String as it writes and frees each smaller buffer unwiped; Zeroizing only wipes the one that survives. Only shows at realistic sizes measured with a 4 KiB credential, which is what a K8s service account token actually is.
  • The certificate's key ID was never checked against the one requested. A 64 KiB key ID authenticated normally, which quietly breaks the attribution this whole path exists to provide.
  • The reason an authentication failed never reached the user — it went to the server log only. For a certificate outside its validity window, the documented clock-skew case, that sends someone to debug credentials that are fine.

Also OpenBao refuses to enable an audit device over the API, and its config stanza needs type, path and an options block — a top-level file_path is accepted with a warning and then ignored. Documented, since the issuance record on the Vault side is half the point.

Two CI gates are red and neither is from this branch:

  • biome fails on AuthPolicyEditor.svelte, which came in with 55af452 and is byte-identical here.
  • cargo-deny fails on RUSTSEC-2026-0253 (lru via ratatui), published after main's last green run. deny.toml already carries RUSTSEC-2026-0002 for the same crate with "no update available".

I left both alone rather than touch unrelated files in a security PR.

Three defects, found by reading other projects' advisories and by pointing two
tools at this code that had not been used on it before.

- A certificate naming more than the target account was accepted. The check
  asked whether the requested principal was among those returned; Vault returns
  the requested set verbatim or refuses, so anything extra means the answer did
  not come from this request. Each extra name is another account the target will
  accept the certificate for, chosen by whoever answered rather than by the
  operator, and under AuthorizedPrincipalsFile it need not resemble a username.
  Now required to be exactly the account asked for.

  This came from CVE-2024-7594, where an empty valid_principals yielded a
  certificate good for any user on the host, and CVE-2026-35414, where a comma
  inside a principal splits one name into two for one of sshd's checks and not
  the other. The second is also why the rule is "exactly one name" rather than
  "contains": it notes the attack works when the CA does not reject commas in
  what it is asked to sign, which is the check Warpgate already makes on the
  request side.

- A certificate could write escape sequences to the connecting user's terminal.
  The refusal message quotes the critical option's name straight out of the
  certificate and is printed to the PTY, so a name containing \x1b[2J cleared
  their screen rather than appearing in the text. Certificate-derived strings
  are now quoted with {:?}.

- The outbound SSH handshake had no bound of its own. A target that completes
  the TCP connection, sends a valid identification string and then goes silent
  held the gateway's task, socket and session slot until the *inbound* session's
  inactivity timeout fired — measured at 55s with that timeout set to 45s. That
  setting governs how long an idle interactive session may live and is
  legitimately raised to hours, every one of which extended this hold to match.
  Bounded now by a dedicated 30s deadline, with an error naming the stage so an
  operator is not sent to look at credentials.

tests/hostile_ssh_server.py is new: six ways of being a bad SSH server, none of
which needs Docker. The rest of the suite treats the target as honest, which is
the one trust boundary nothing here had pushed on — and russh, which Warpgate is
the client half of, has published pre-authentication panics reachable from the
peer. Five of the six modes were survived without change.

cargo mutants found the fourth problem, in the tests rather than the code: it
replaced the error-body reader with one returning an empty string and everything
still passed, because the assertions were all upper bounds. Ten mutants survived
in that one function. The truncation marker is now pinned from both sides.
@janisdombr

Copy link
Copy Markdown
Contributor Author

Pushed 6bd00e1. Three more defects, found by reading other projects' advisories and by pointing two tools at this code that had not been used on it before.

A certificate naming more than the target account was accepted. The check asked whether the requested principal was among those returned. Vault returns the requested set verbatim or refuses, so anything extra means the answer did not come from this request and each extra name is another account the target will accept the certificate for, chosen by whoever answered rather than by the operator. Under AuthorizedPrincipalsFile it need not resemble a username at all. Now required to be exactly the account asked for.

This came out of two advisories rather than out of the diff: CVE-2024-7594, where an empty valid_principals yielded a certificate good for any user on the host, and CVE-2026-35414, where a comma inside a principal splits one name into two for one of sshd's checks and not the other. The second is also why the rule is "exactly one name" rather than "contains" it notes the attack works when the CA does not reject commas in what it is asked to sign, which is the check on the request side that was already there.

A certificate could write escape sequences to the connecting user's terminal. The refusal message quotes the critical option's name straight out of the certificate and is printed to the PTY, so a name containing \x1b[2J cleared their screen rather than appearing in the text. Certificate-derived strings are now quoted with {:?}.

The outbound SSH handshake had no bound of its own. A target that completes the TCP connection, sends a valid identification string and then goes silent held the gateway's task, socket and session slot until the inbound session's inactivity timeout fired measured at 55s with that timeout set to 45s. That setting governs how long an idle interactive session may live and is legitimately raised to hours, every one of which extended this hold to match. Bounded now by a dedicated 30s deadline, with an error that names the stage.

tests/hostile_ssh_server.py is new and needs no Docker: six ways of being a bad SSH server. The rest of the suite treats the target as honest, which is the one trust boundary nothing here had pushed on and russh, which Warpgate is the client half of, has published pre-authentication panics reachable from the peer. Five of the six modes were survived without change; russh bounds the identification string itself, which covers two of them.

cargo mutants found the fourth problem, in the tests rather than the code: it replaced the error-body reader with one returning an empty string and everything still passed, because the assertions were all upper bounds. Ten mutants survived in that one function.

Checked and clean, for the record: russh 0.62.6 is current against all fourteen of its advisories, and allow_insecure_algos keeps the strict-KEX extensions, so the Terrapin mitigation is not lost in the mode meant for older devices.

CI is still red on biome and cargo-deny, and neither is from this branch the same two are red on #2409 and #2410. AuthorizedPrincipalsFile.svelte came in with 55af452 and is byte-identical here; RUSTSEC-2026-0253 (lru via ratatui) was published after main's last green run, and deny.toml already carries RUSTSEC-2026-0002 for the same crate.

@theredspoon

theredspoon commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Ran a final-gate pass with three independent reviewers plus direct verification against real sshd servers, since this round changed enough surface (the real-Vault/real-OpenBao test harness, the critical_options allow-list logic, the CheckHostKey command) to be worth a genuinely fresh look rather than re-confirming what's already fixed. Everything from the last round not mentioned below has been confirmed separately. Two real, previously-unflagged issues, plus a cluster of smaller ones.

Host-key check returns the wrong key for any target behind a jump host

Already independently reported and being fixed: issue #2412 and its open fix, PR #2413 (resolve_ssh_chain tags each hop with its target_id, and the admin endpoint now waits for the specific hop being asked about rather than breaking on the first HostKeyReceived it sees). No need to duplicate that here.

What #2413 doesn't cover, since it's written against main and this PR's stop_after_host_key/CheckHostKey gating doesn't exist there yet: whether an intermediate hop's own authentication is actually prevented when only that hop's key is being checked. Right now stop_after_host_key is gated on is_last/hop_count, not on which target the caller asked about, so once #2413 lands and this rebases onto it, the same target_id tagging needs to also drive the stop_after_host_key decision, not just which key gets returned. Otherwise a caller checking an intermediate hop's key specifically (which #2413 makes possible to do correctly) still doesn't stop that hop from authenticating.

Related to that: no certificate gets minted for the jump host today, but that's not a construction guarantee the way it is for the final hop, it's the admin endpoint's abort winning a race against the SSH handshake, the same category of fragility CheckHostKey exists to eliminate. Confirmed empirically (40 consecutive presses, both host-key-known and unknown, 0 Vault sign requests, 0 userauth attempts logged), so not currently exploitable, but nothing structurally prevents it and there's zero test coverage for any chain longer than one hop.

Pinned critical options are only checked when the certificate actually carries them

certificate_mismatch()'s critical-options loop (client/mod.rs:167-185) iterates over options present in the returned certificate and checks each is allowed, correctly rejecting anything unexpected. It never checks the other direction: that a target's configured, pinned options are actually present. A target configured with a pinned force-command accepts a certificate carrying no force-command at all, no restriction, full shell. This is exactly the threat model already established for this feature, someone with Vault role-write but not sign-rights, just approached from the other side: instead of adding an unexpected option, they remove an expected one, and nothing here catches it. Please resolve by checking that every entry in the target's allowed_options with no value pin, or a specific value pin, is actually present in the returned certificate, not just that nothing extra showed up.

Smaller items, roughly by severity

  • warpgate-vault/src/metadata.rs's Azure (.json(), lines 40-52 and 54-66) and GCP (.text(), lines 78-92) calls buffer the full response with no size cap and no zeroizing of the intermediate buffer, the exact defect class this round's own read_json fix (client.rs, MAX_RESPONSE_BODY) closed for the main Vault client, just not mirrored here. Reachable on every Azure/GCP relogin.
  • LOGIN_PAYLOAD_CAPACITY (client.rs:118, 32KiB) is a pre-allocation hint, not a bound. read_credential doesn't cap what it reads from token_path/secret_id_path. A credential file larger than 32KiB reintroduces the grow-and-copy leak this round's zeroization fix was built to close, silently.
  • certificate_mismatch()'s valid_principals check verifies the requested principal is present, not that nothing else is. Already fixed in 6bd00e187, pushed while this review was in progress: now requires an exact single match rather than containment, stronger than what this would have asked for, backed by two real CVEs (CVE-2024-7594, CVE-2026-35414) that make this a better-documented severity than "low, needs a fully rogue Vault."
  • No check anywhere on the certificate's own validity window. tests/test_vault_hostile_certs.py:112 references SECURITY_TESTING.md to explain why this is accepted risk; that file doesn't exist in the repo. A misconfigured or compromised Vault role can hand back a certificate valid for years, unnoticed, bounded only by the role's own max_ttl, undocumented in the README's certificate_ttl section. Worth noting: Add Ssh cert auth for targets #1847, the alternative self-hosted-CA implementation of this same feature, defaults its own issued certificates to a 1-minute window, so short validity is already the project's own expectation, just not enforced on the receiving end here.
  • CI can go green with neither real issuer actually tested: a Docker image pull failure calls pytest.skip() (tests/vault_server.py:77) rather than failing the run, and the OpenBao image is pinned to mutable latest. This is a new pattern for this repo specifically, not an existing convention: nothing else in the test suite skips on infrastructure failure, .github/workflows/test.yml builds every other test image in an explicit step that fails the job outright on error. Given the harness is meant to be the contract gate, worth making pull failure a hard failure and pinning both images to a digest.
  • The AWS SigV4 headers are copied unwiped at least three more times beyond the serde_json::to_string line already flagged, warpgate-aws/src/sts_identity.rs's Credentials, the Identity it's moved into, and the http::Request header map apply_to_request_http1x writes the session token into. Worth scoping that fix to the whole AWS path rather than the one call site.
  • Two contract tests don't test what they claim: tests/test_vault_contract.py's key-ID test supplies a truncated/malformed public key that both real Vault and OpenBao reject at parse time, before allow_user_key_ids policy is ever consulted, so it passes without exercising the thing it's named for. Separately, server.signs (tests/vault_server.py:282) reads only the audit device's request entries, so the test asserting the certificate carries the right principals re-checks what Warpgate sent, not what either real server actually returned.
  • Sub-second certificate_ttl values fail only at connect time (Duration::as_secs() truncates to "0s", both real issuers reject it), not at config load. Worth validating at config time instead.
  • Minor UI bug: clearing a pinned critical-option value in the admin UI (Options.svelte) writes back an empty string rather than clearing the pin, so an operator who types a value and deletes it gets an exact-match-empty pin instead of "any value" as the placeholder implies. Fails closed, but the UI says the opposite of what happens.
  • warpgate-vault/tests/zeroization.rs:123-144 has orphaned, truncated doc comments describing tests that were deleted, and login_payload is private, so the suite's own safety-net tests reimplement the safe pattern inline rather than exercising the real function, reverting the actual fix would leave every assertion in that file green.
  • .github/workflows/docker.yml's IMAGE_NAME change to ${{ github.repository }} looks like unrelated fork scaffolding inside a security PR, worth dropping from the branch.

One more, separate from the above: the terminal-escape-sequence fix in 6bd00e187 looks like it's the first time this codebase has addressed that bug class at all, and there's no shared sanitization helper anywhere for it. warpgate-protocol-ssh/src/server/service_output.rs, command_detector.rs, and session.rs:584 all write untrusted-ish strings toward a PTY too; worth a pass to check whether any of them have the same exposure now that the pattern's been named once.

Given how many of the above are tests passing without exercising what they claim to, worth doing your own adversarial pass over the test suite specifically, not just the production code, and writing down whatever gaps that turns up so they don't quietly regress later.

Until now "the guards discriminate" was a statement about the last afternoon
somebody ran the matrix by hand and reported a number. Two of the numbers
reported that way were wrong. A green check is the durable form of that claim.

`--changed <base>` selects the guards whose anchor file a change touched, and
prints how many of how many: a run that reports on a subset has to name the
subset, or a green check implies a sweep that never happened. The workflow runs
that subset on `pull_request` and all fifty-three on a schedule and on dispatch,
the second being the backstop the first leans on. `fetch-depth: 0`, because
`--changed` has nothing to diff against otherwise; the script names that case
rather than reporting zero changed guards.

A guard with no named discriminator fails the build, and every guard that did
not discriminate is now named in the failure rather than only counted.

The `cargo test -p warpgate-vault` precondition is skipped in `--named` mode.
It runs that whole suite — 250 seconds, measured — on every invocation to
establish the tree passes before anything is mutated, and `--named` establishes
the same thing per guard and more narrowly: the named test must pass *before*
the mutation, or the guard is reported `already failing` and no verdict comes
out of it.

A sweep also printed nothing until every guard had finished, so an hour-long run
was indistinguishable from a hung one by anything except `ps`. Two of today's
runs were killed for exactly that reason. Each guard now reports as it lands.
Two sweeps ran side by side today. The lock was written unconditionally rather
than checked, so the second started happily beside the first and both rewrote
the same source files — two mutations live at once, and any verdict either
produced would have been about a tree neither of them described. It was noticed
by counting processes, which is not a control.

A lock left behind by a killed run now blocks a start too, and that is the right
way round: a stale lock costs one command to clear, and the refusal names both
the command and the check to run first. A contaminated sweep costs a number that
looks like evidence.

The pre-checks were also silent. They build a test binary per crate and run two
workspace-wide `cargo check` passes, which together ran for over an hour on a
full sweep while printing nothing at all — so the run was indistinguishable from
a hung one, and three of today's were killed on exactly that ambiguity. Each
phase now announces itself before it starts.
Raised by the auditor from the sweep's own output, which is the first defect
this round found by watching an instrument run rather than by reading it.

`verify_named` restored the mutated *source* in its `finally` and never rebuilt
the binary compiled from it; the clean rebuild happened once, after the whole
loop. So `target/debug/warpgate` carried the last mutation applied to it, and
every integration guard's baseline ran the previous guard's mutated gateway. It
surfaced as `already failing` on two guards of a sweep whose tree was clean.

The consequence is wider than those two. A `discriminates` verdict asserts the
named test passed before the mutation and failed after it, and the first half of
that was measured against someone else's mutation. The verdicts from that sweep
are void rather than partial, and it restarts rather than resumes.

The gateway is now rebuilt from clean source before a baseline that needs one,
and only then — which turns out to be 16 of the 53 guards. The other 37 are
pinned by Rust unit tests, which `cargo test -p` compiles itself and which never
look at `target/debug/warpgate`; building it for them was work whose result
nothing read.

Each guard now prints `baseline`, `building the gateway with the guard off`, and
`testing with the guard off` as it reaches them, so a slow guard is legible as
slow rather than as stuck.

`--fail-fast` stops at the first guard that does not discriminate and says how
many remain **unknown, not passing**. Guards added or repointed in round J are
ordered first, so that flag reaches the least-established ones in minutes rather
than hours. Order changes no verdict: every guard is measured against its own
baseline.
The claim this feature's test suite has been asked to support since the first
review round, produced in a single sweep rather than assembled from separate
afternoons: each guard disabled in turn, and the test named after it failing
exactly when the guard is gone and passing when it is not.

`tests/mutation-matrix.json` carries `partial: false`, `refused: null`, and 53
results of one status. Fourteen hours and twenty minutes, of which roughly
fifty-five minutes were pre-checks.

The number is worth what the instrument is worth, and this round found six
defects in the instrument. It would not start at all — its own duplicate check
tripped on a duplicate. Its documented invocation ran from nowhere. It paid the
whole repository's collection cost for a single guard, in three separate places.
It printed nothing for hours, so three runs were killed as hung. Two runs
executed side by side rewriting the same files, because the lock was written
rather than checked. And the mutated gateway leaked into the next guard's
baseline, which invalidated 28 verdicts of the preceding sweep — found by the
auditor, from a running sweep's output, not from reading the code.

All six were fixed before this run, and the run started from scratch rather than
resuming, because a sweep half-measured against another guard's mutation is void
rather than partial.

Guards 29 and 31, the two that produced the false `already failing` that exposed
the last of those defects, pass cleanly here.

What this does not say: that these are the right guards, or that the set is
complete. Neither claim is made.
Fourteen commits, of which the release itself, a vt100 upgrade fixing a resize
panic, pubkey failures counting towards IP blocking, and a session view linking
to its user and target.

Only the two generated OpenAPI schemas conflicted, and they were regenerated
from the merged Rust types rather than resolved by hand. That was not
ceremony: taking either side wholesale would have dropped upstream's new
`user_id`, `target_id` and `remote_address` fields out of the published API,
silently, because a hand-resolved generated file corresponds to no version of
the types it is generated from.

`warpgate-protocol-ssh/src/server/session.rs` merged cleanly despite both sides
editing it.

No guard's anchor file is touched by any of it, so `--changed` selects nothing
and the 53-guard sweep is not repeated. That rule cannot see a guard broken in a
file the change did not touch — which is why the scheduled full sweep exists —
so the integration suites are run instead, at ten minutes against fourteen hours.
@janisdombr

Copy link
Copy Markdown
Contributor Author

@theredspoon Pushed — 46 commits, merged with v0.28.0, 0 behind main.

Twenty-one of your twenty-five asks are closed and no product code is open. But the thing worth your time is not in your list.

Un-skipping one test found a panic

Item 24 asked me to un-skip test_a_certificate_that_never_expires and strengthen its assertions. I did, and it failed — not on an assertion, on a client timeout. The gateway's log ended at "Certificate carries extensions" with no refusal, and then:

thread 'tokio-rt-worker' panicked at library/alloc/src/string.rs:2943:14:
a Display implementation returned an error unexpectedly: Error

humantime's Display returns Err rather than truncating for any time at or after the year 9999 (humantime-2.4.0/src/date.rs:267), and to_string() panics when a Display errors. The validity window is rendered for the diagnostic message above the call to certificate_mismatch. So ssh-keygen -V always:forever — and equally a Vault role with no TTL, or one with an absurd max_ttl — killed the worker mid-connection.

Three consequences, all observed rather than reasoned about:

  • the guard that refuses a never-expiring certificate could never fire, because it sits after the line that panicked;
  • the client was left holding a connection nobody would ever answer, which is item 8's class again;
  • it is reachable by the issuer, which this design treats as hostile throughout.

Why nothing caught it: the unit test for that guard calls certificate_mismatch directly and never reaches the panicking line, and the one test that did reach it was skipped on the symptom the panic produced — "holds the session open for ~45s for a reason not yet isolated". A test disabled because of the defect it had found, parked for a week.

That is your closing observation, one level deeper than you put it. Fixed in 859fe990c, with a guard that drives the first second of the year 9999 and also asserts an ordinary expiry still renders as a date, so a fix that calls everything unrenderable fails it.

Four claims of mine were wrong

  • Item 22's count. I said nine sites, "verified by grep". The grep required % on the same physical line as the macro and could not see multi-line tracing! calls. It is eleven. A check I chose, ran and reported on myself, which passed because it could not see the thing it was looking for.
  • Why item 24 was skipped. I said the missing PTY meant the refusal had nowhere to go. Wrong — the task had panicked. The log said so; my reading of the code did not.
  • "HACKED=x" in the terminal-escape test. ssh-keygen splits critical:NAME=VALUE on the first =, so the value is not part of the name the message quotes. Correcting it exposed the better point: "the name is present" and "no raw escape arrived" are both satisfied by a fix that strips the name entirely. The escape now has to appear in its inert form, \u{1b}[2J.
  • A mutation-matrix run I reported as measuring something. It was killed, and a killed runner's exit code is indistinguishable from a caught guard. Recorded as not run.

Your list

Item 8 — the target's USERAUTH answer has a flat 30s bound of its own, applied at all five call sites so a new credential type cannot arrive unbounded by being forgotten. New TargetAuthenticationTimeout variant, so the party that went quiet is named.

Item 15/18 — key_id_field percent-encodes (% first, then :). Reading the comment you flagged as false turned up a second defect it was concealing: UNATTRIBUTED goes into the same field and user_key_id_field consulted only TOKEN_ATTRIBUTIONS, so a user named unattributed was indistinguishable from a session with no user recorded. Two lists of reserved names, one consulted. role_id is Secret<String>; config-schema.json is byte-identical, verified by regenerating.

Item 19 — thirteen sites, all through tempfile::TempDir. Item 28 — deleted; it tested the zeroize crate's own Deref. Item 29 — anchored, and corrected as above. Item 33 — the reason is captured before Done, and the command loop's Some(()) is now _; those were not two spellings of one thing, since a closed channel disables that branch rather than firing it.

Item 31 — unreachable_reason(kind) separates "cannot reach it" from "its key is untrusted", on the kind and never the OS string, because this is the sanitiser. It had no test at all; it has two, and the mutation for one restores e.to_string(), which is the bug the arm exists to prevent.

Item 17 residual — error_body's buffer is Zeroizing and reserved. read_bounded_json's serde_json scratch is not fixed and cannot be from here; recorded as a limit rather than closed. I-10's test guards the helper, not the call site, and now says so in its own doc comment — driving read_bounded would put hyper's buffers inside the canary detector's window, and a test that fails for a cause we cannot fix is worse than one that states what it does not check.

Doc nits done. And you were right to narrow the role-validation claim: what shipped is a syntactic name-format rule shared by the save path and the connect path, not a check that the role exists. warpgate-admin has no dependency on warpgate-vault — verified in its Cargo.toml — and there is no lookup to call. The write-up says that now.

Item I-4/I-6 attribution: you are right, both landed in 110820c5. Noted here rather than by editing the earlier comment.

The mutation matrix, in CI and measured

.github/workflows/mutation-matrix.yml: the guards whose anchor file a PR touched on pull_request, all of them on a schedule and on dispatch. A guard with no named discriminator fails the build.

With the honest cost attached, because it changes the recommendation. A full sweep is 14h20m on an M-series laptop. GitHub-hosted runners are 4 vCPU and slower, and the job limit is six hours, so the full sweep does not fit there at all. And the per-PR saving is smaller than it sounds: --changed against four commits selected 34 of 53 guards, because client/mod.rs carries two thirds of them. You may well want only the scheduled half, or a self-hosted runner, or the whole thing sharded — that is your call and I would rather hand you the number than the recommendation.

Six defects turned up in the instrument itself along the way. It would not start at all — its own duplicate check tripped on a duplicate that had been committed. Its documented invocation ran from nowhere. It paid the whole repository's collection cost for a single guard, in three separate places. It printed nothing for hours, so three runs were killed as hung. Two runs executed side by side rewriting the same files, because the lock was written rather than checked. And the mutated gateway leaked into the next guard's baseline, which voided 28 verdicts of a preceding sweep.

53 of 53 guards discriminate, measured in one run at 02792acfc after all six were fixed, tests/mutation-matrix.json carrying partial: false and refused: null. What that does not say: that these are the right guards, or that the set is complete. Different claims, neither made here.

Not done, deliberately

Upstream logs client-controlled strings through Display at about a dozen sites in warpgate-protocol-ssh/src/server/session.rs — exec commands, environment variable names and values, subsystem names, socket paths, a login name, and two "Target {} not authorized for user {}" lines interpolating the name from the client's own login string. Same mechanism as item 22 by a shorter route: no Vault, no admin access, just an SSH client sending an environment variable. Every one verified present on origin/main, so none of it is ours. It belongs upstream as its own change rather than widening a diff you have already called large — but I did not want to decide the scope of somebody else's security fix silently, so: your call.

Your process suggestion

It sat unanswered for six days and you were right to raise it again. It is now half-built, and not in the shape you drew.

Rather than RED and GREEN subagents inside one process, the split is across two providers with hard role separation: I build and operate checks, an independent auditor specifies them and rules, and neither writes into the other's territory. Deterministic checks are run by orchestration, as you said — that is what the matrix is.

It paid for itself in a way I would not have predicted. The auditor found the dirty-binary leak above — from a running sweep's output, not from reading the code. I had looked at the same two already failing lines in my own status notes and written "look into this later". Every other defect this round came from someone reading; that one came from someone watching. It is the strongest argument for the split that I have, and it is not the argument either of us made for it.

Everything green after the merge: 63 + 26 + 4 unit tests, 109 integration tests against a real sshd.

@theredspoon

Copy link
Copy Markdown
Contributor

Verified the two CI/measurement items against 618fd351 (haven't re-checked since the upstream sync, though warpgate-vault/ has zero diff across it). Everything other than what's below checks out. Thanks for closing this much of the list cleanly.

The "in CI, measured" section isn't, by its own numbers. It opens describing .github/workflows/mutation-matrix.yml as already running: guards on pull_request, all fifty-three on schedule and dispatch, failing the build on a missing discriminator. The next paragraph says the full sweep (14h20m) doesn't fit GitHub-hosted CI's 6h cap, the --changed subset saves less than expected (34/53, no runtime given), and leaves the shape "your call." That's not a finished workflow, and the repo confirms it: no mutation-matrix.yml anywhere, gh pr checks 2397 at 618fd351 shows only the standard set, and 34550c72 (the commit narrating this) only touches tests/mutation_matrix.py, no YAML.

53 of 53 guards discriminate is cited to 02792acfc, which is the baseline-leak fix, not a result, its message describes the mechanism only. The actual run is 93e37ae, about 15h later ("Measure every guard in one run: 53 of 53 discriminate"), which is where partial: false, refused: null, and the 14h20m figure actually come from. Either way, tests/mutation-matrix.json was never committed (and isn't gitignored), so there's nothing to check the number against, the same unverifiable-run problem you flagged yourself for a different run in this same comment.

Ask: correct the write-up so it doesn't describe this as running in CI, repoint the citation to 93e37ae, and commit the artifact (or link the log) so 53/53 is checkable.

The artifact's headline numbers were `len(MUTATIONS)` and `len(DISCRIMINATES)`
— the height of the guard table and the number of declarations in it — under
the names `guards_total` and `guards_with_a_named_discriminator`. Both read as
results. A four-guard `--changed` run and a fourteen-hour sweep emitted the
same "53 / 53", so the file could not tell them apart, and `partial` was set
from whether a `--named <substring>` filter had been passed, which made every
`--changed` subset call itself complete.

That is how a review asking to check "53 of 53 discriminate" against the
artifact found an artifact from a four-guard run stating exactly that number.
The counts now count this run: guards selected, guards measured, guards that
discriminated, and `partial` derived from whether the run covered the table.

`--shard INDEX/TOTAL` splits the guards round-robin so a sweep can be spread
across machines. Round-robin rather than contiguous blocks because the
integration guards rebuild the gateway and cost minutes each while the unit
guards cost seconds, and the table groups them by subject — contiguous blocks
would hand one shard every expensive guard.
janisdombr added a commit to janisdombr/warpgate that referenced this pull request Aug 24, 2026
`workflow_dispatch` is only offered for workflows on the repository's default
branch, and this one is deliberately not there: it lives on a branch of its own
so it stays out of PR warp-tech#2397. A push to that branch is the trigger left, and it
suits a job that takes hours across eight runners — a sweep is something you
ask for, not something that happens to you.
Two defects, both of which let a run state a result it had not established.

Outcomes were inferred rather than read. Only the `FAILED` lines were parsed
and every other named test was recorded as passed, so a skipped test, a test
whose fixture raised, a deselected test and a test pytest never mentioned were
all indistinguishable from a passing one — the confusion `failing_tests`
already refuses for a whole suite, left open for a single test. With `-rA` each
test states an outcome; anything that is not plainly passed or failed is now
unclear, an unclear baseline records `no baseline` rather than proceeding to
measure against a precondition never met, and an unclear guard-off run records
`no verdict` rather than `does not discriminate`.

The test's name was taken from the wrong end of the line. A summary line is
`FAILED <nodeid> - <message>`, the message in these suites is a slice of
Warpgate's own log, and that log carries Rust module paths — so splitting on
the last `::` returned `logging:` and `config:` instead of a test name, and the
test was not recognised as having failed. Two guards were reported as failing
to discriminate on exactly this; both do discriminate, in CI and locally.

Also: the gateway binary is fingerprinted across the guard-off build, because
an A/B whose halves ran the same binary is not an A/B and reads as a coverage
hole.

Verified: 53 of 53 guards discriminate, every guard measured, shards accounting
for the whole table — https://github.com/janisdombr/warpgate/actions/runs/32802468772
A skip is the one outcome pytest's short summary does not attribute to a test:
the line is `SKIPPED [1] <file>:<line>: <reason>`, with no nodeid in it, so a
skipped test cannot be matched to the name a guard declares. It was recorded as
"never reported by pytest", which is true and unhelpful — it reads like the run
lost the test rather than like the test was skipped on purpose.

A name that goes unreported while skips were printed now says so and quotes the
first one. The verdict is unchanged: a skipped test is still not a baseline and
still not a discriminator.
@janisdombr

Copy link
Copy Markdown
Contributor Author

@theredspoon thank you

Confirmed on all three, and following the third down found the number itself was wrong. Taking them in order.

The workflow never existed in the repository. You're right that 34550c729 touches only tests/mutation_matrix.py. The cause is .gitignore:36, which is .github:

$ git check-ignore -v .github/workflows/mutation-matrix.yml
.gitignore:36:.github	.github/workflows/mutation-matrix.yml

So git add staged nothing, exited 0, and the commit went out with a message describing a file that stayed in my working tree. I wrote the message from what I had done rather than from what the commit contained. Nothing in this PR has ever run in CI, and the section claiming otherwise was wrong.

Citation repointed. 02792acfc is "Stop one guard's mutated gateway becoming the next guard's baseline" — the fix, not a result. The run was 93e37ae8b.

The artifact — and this is where it gets worse. Going to fetch it found tests/mutation-matrix.json on disk from a four-guard run, reading "partial": false, "guards_total": 53, "guards_with_a_named_discriminator": 53. Those two counts were len(MUTATIONS) and len(DISCRIMINATES) — the height of the guard table and the number of declarations in it, under names that read as results — and partial was set from whether a --named <substring> filter had been passed, so a --changed subset called itself complete. Every artifact the tool ever wrote said 53/53. The sweep's own was overwritten by the next subset run and is gone.

So I withdrew the number, fixed the counts to count the run, put the workflow on a branch of my fork where it could actually execute, and ran it. That first real run came back 51 of 53. Then two more defects surfaced, each hiding the next:

  • The workflow never built the gateway. The integration guards run target/debug/warpgate, which the matrix produces only as a side effect of mutating it, so the first baseline in every shard ran against a path that did not exist. pytest raised in the fixture, and the outcome rule counted an ERROR as a pass. Fourteen guards had been measured against a baseline that never ran.
  • The test's name was read from the wrong end of pytest's summary line. It is FAILED <nodeid> - <message>, the message in these suites is a slice of Warpgate's own log, and that log is full of warpgate_common_http::logging — so split("::")[-1] returned logging: and config: instead of test names. A test whose name could not be read was not in the failed set, so it was recorded as passed. That is the whole of the original "does not discriminate": both guards were failing in CI exactly as they do locally, and the run could not read its own evidence.

53 of 53, on a run you can open: https://github.com/janisdombr/warpgate/actions/runs/32802468772 — every guard measured, a summary job requiring the shards to account for the whole table before it will call the result a sweep.

I should be plain that this invalidates my earlier defence of the number as well as the number. When you asked for the artifact I argued a discriminates verdict required a real FAILED line and so the 51 were sound in at least one direction. They were not: where the baseline silently errored and the guard-off half then ran against the freshly built binary, the verdict came out discriminates on a precondition never met.

The instrument. The tool is in this PR at tests/mutation_matrix.py, with the parser and baseline fixes. The workflow is deliberately not, and isn't proposed for upstream — it's on a branch of my fork:

On the six hours — you were right, and so was the number that made me say it, but for a reason that turns out to weaken the argument. An unsharded "every guard" job could only ever be started, never finished, and the workflow now shards eight ways. But the 14h20m I quoted for the local sweep is not fourteen hours of work. pmset -g log for that night:

2026-08-20 01:44:28  Sleep  Entering Sleep state due to 'Clamshell Sleep'
2026-08-20 09:19:13  Wake   Wake from Deep Idle [CDNVA] : due to ... lid

Seven hours thirty-five minutes of a closed laptop, counted as run time. The real figure is at most 6h45m of work — against about 5.3 machine-hours across the eight CI shards. A ratio of 1.27, not the 24 I was implicitly claiming. So the sweep is roughly six or seven hours of single-machine work, which is close to the six-hour ceiling rather than comfortably over it; sharding is still the right shape, but "does not fit" was overstated and I should not have leaned on it.

@theredspoon

Copy link
Copy Markdown
Contributor

Confirmed all three fixes (3325418, 191a2c0, d581e3a) against the diffs, and the .gitignore:36 explanation. Confirmed the fork run too: janisdombr/warpgate@tooling/mutation-matrix, run 32802468772, 8/8 shards green plus the gating summary job, matches the description.

One number doesn't add up. You wrote the local sweep was "at most 6h45m... against about 5.3 machine-hours across the eight CI shards." Summing the final run's 8 shard durations directly from the job timestamps gives about 2.55 hours, not 5.3. The only way I got close to 5.3 was adding the final run to the immediately preceding failed attempt (2.55h + 2.68h ≈ 5.23h), which isn't "across the eight CI shards" of one run.

Ask: show where 5.3 comes from, or correct it.

@janisdombr

janisdombr commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

@theredspoon You're right, and the way I got 5.3 is worse than the number.

It was not a sum of anything. It was 8 shards × ~40 min, where 40 was my recollection of the run's wall clock. Both inputs are wrong: the wall clock was 28.8 minutes, and multiplying wall clock by shard count is not how machine-hours work when the shards have different durations. Your reconstruction (2.55h + 2.68h, the final run plus the failed attempt before it) lands on 5.23 by coincidence — I never added those.

Measured properly, from api
run1 and run2
completed_at - started_at per job:

 21.1 min  Guards discriminate (1/8)
 13.8 min  Guards discriminate (2/8)
 18.9 min  Guards discriminate (3/8)
 28.6 min  Guards discriminate (4/8)
 18.4 min  Guards discriminate (5/8)
 21.2 min  Guards discriminate (6/8)
 18.7 min  Guards discriminate (7/8)
 12.3 min  Guards discriminate (8/8)
  0.1 min  Every guard discriminates
 ------
 2.55 machine-hours     wall clock 28.8 min

2.55, and that breaks the argument I built on it. I told you an unsharded "every guard" job could only ever be started, never finished, against GitHub's six-hour cap. At 2.55 machine-hours a single job would take about two and a half hours and fit inside the cap with room to spare. Sharding buys wall-clock latency — 28.8 minutes instead of ~2.5 hours — which is worth having, but it is not the feasibility argument I gave you, and the workflow comment saying otherwise is wrong. I'll correct it.

On the local side: 6h45m is an upper bound, not a measurement. It is 14h20m of reported wall clock minus 7h35m the laptop spent asleep (Clamshell Sleep at 2026-08-20 01:44:28, lid wake at 09:19:13), and it assumes the run occupied that whole window. The sweep's own artifact was overwritten, so I cannot pin its start and end. The residual — 6.75h against 2.55h, a factor of 2.65 — I have not explained and am not going to guess at.

The one number I'd stand behind is 2.55 machine-hours, because it comes from the job timestamps and you can pull the same JSON.

Five commits: the v0.28.4 version bump, an RDP lossless-compression option, a
role-assignment permission fix, and a workflow edit.

Both generated OpenAPI schemas conflicted. They are not files to resolve by
hand — whatever a three-way merge produces from generated JSON is not what the
generator would emit, and nothing downstream would notice. Resolved by taking
one side to clear the conflict and regenerating from the merged Rust types.

That turned out to matter. The admin schema, resolved textually, was missing
`RdpTargetCompression` — the type upstream's RDP commit adds. Regeneration
restored it. The gateway schema needed no semantic change: regenerated, it
carries upstream's content in full plus this branch's `Info.has_vault`, and
differs from the committed file only in the `git describe` build stamp.
…ecordings

Three commits. Two overlap this branch only by file; one changes something this
branch depends on.

`56d131d5c` removes the `Mutex` from `session` so a hanging channel open no
longer holds the whole session. Every `session.lock()` it drops is upstream's
own; this branch adds none. Its bound and ours address the same class of hang at
different sites: ours wraps the `channel_open_direct_tcpip` that builds the
chain to the target, theirs the one serving a channel the client asked for.

`2a272c3a4` makes the three PTY sinks synchronous — `emit_pty_output`,
`emit_service_message` and `emit_pty_error` are no longer `async`. This branch
applies its control-character escaping inside those sinks, so the signatures are
taken from upstream and the escaping re-applied in the new bodies. Kept on this
side: `ConnectionError::Authentication` carrying its reason, `client_message()`
rather than `{error}` on the path to a connected user, and the whole-chain
`RCCommand::Connect` the per-hop attribution needs.

Also adds a guard. `test_a_hostile_option_name_cannot_write_to_the_terminal`
had no guard naming it, so nothing would have noticed if the escaping it relies
on were dropped by a merge like this one. It is `{name:?}` in `client/mod.rs` —
`Debug`, which renders an escape sequence inert. Two earlier attempts anchored
this guard on the session sinks instead and neither discriminated; the matrix
said so before any of it left the machine.

let response = self
.http
.post(self.url(path))
CodeQL reports "cleartext transmission of sensitive information" on the request
built in `VaultClient::url`, High severity, reaching it from the AppRole secret
ID. The alert is a false positive as the code stood: `validate_address` refuses
any address that is not HTTPS unless it is loopback, `VaultClient::new` is the
only constructor, and it calls that check on its first line. There is no path to
a request that has not been through it, and a property test pins the rule.

The analyser cannot see that, and it is right not to trust it. The guarantee was
a property of call order rather than of the code: safe only while `new` remains
the sole constructor and keeps calling `validate_address` first, which is
exactly the sort of invariant a later refactor drops without anything noticing.

So `validate_address` now returns the parsed `url::Url` instead of `()`, the
client stores it, and `url()` builds every request from that field rather than
from `config.address`. The only way to obtain something this client will send a
request to is to have come through the check. The scheme warning reads the
parsed scheme too, rather than matching a string prefix.

No behaviour changes: the same addresses are accepted and refused, and the same
warning is emitted for a loopback Vault over plain HTTP. 30 tests in the crate
pass, including the property test asserting every accepted address is HTTPS or
loopback.
…ation

Four commits, of which one matters here: `3c44340cd` adds a `target` to the
ticket-request responses in the gateway API.

That is why `check-schema-compatibility` failed on the previous head. The gate
regenerates this branch's schema and compares it against **main's current**
committed one, so it reports every API addition upstream has made that this
branch has not yet merged as a removal on our side. Nothing was wrong with the
schema we had; it was four commits stale.

Both schemas conflicted again and were resolved the same way as before — take a
side to clear the conflict, then regenerate from the merged Rust types, because
a three-way merge of generated JSON produces something no generator would emit.
Verified after regeneration: nothing upstream carries is missing from either
file (admin gains 26 paths from this branch, gateway 2, and loses none).
CodeQL reports eleven high-severity cleartext-transmission alerts against this
crate, and they are not false positives. Every login sends a credential — a
projected service account token, an AppRole secret ID, a signed cloud identity —
and `validate_address` permitted `http://` for loopback, so a path existed along
which one of those crossed the wire in the clear. The exception was there so a
development Vault needed no certificate. That is a poor trade in a change whose
entire subject is not leaving secrets exposed.

The exception is gone: an address is HTTPS or it is refused. The warning that
existed to announce the insecure case goes with it, being unreachable.

The tests followed the rule rather than the other way round. The Python stub
serves TLS from a self-signed certificate and hands its path to `vault.ca_bundle`;
the Rust stand-ins do the same through `rcgen` and one shared certificate. Cloud
metadata keeps a second, plaintext listener, because a real metadata service
answers over HTTP on a link-local address and testing it over TLS would have
exercised a shape that does not exist.

Two things surfaced on the way:

`rustls` refuses a server certificate without `extendedKeyUsage=serverAuth`, and
reports it as `EkuError` — which reads as a plain verification failure and names
nothing. The stub's certificate now carries it.

`alloc_port` returned `last_port += 1` without checking whether anything held
that port, so a caller learned it was taken only when its own `bind` raised
`Address already in use`, well inside a test that then failed for a reason
unrelated to its subject. It cost about one failure per run of the
hostile-target suite. It now probes the way the callers bind before returning.

53 unit tests in the affected crates and all 93 integration tests pass. The
address guard is renamed and re-anchored to the new rule, and discriminates.
`test_an_untrusted_vault_certificate_is_refused` serves a certificate it
generated itself and expects the handshake to be refused. Giving every config in
this module the shared test `ca_bundle` changed what that test exercises: with a
bundle the client verifies against a root store the test supplied, without one
it uses the platform verifier. Those are different mechanisms and they disagree
across platforms — the test passed on macOS and failed on Linux in CI.

It now builds its config without a bundle, which is what it had before the
stand-in servers gained TLS. The subject is the default trust decision, and the
comment says so.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants