Add SSH certificate authentication for targets, issued by Vault - #2397
Add SSH certificate authentication for targets, issued by Vault#2397janisdombr wants to merge 68 commits into
Conversation
|
Will be happy to see this merged since it's the only deployment blocker for us due to security concerns. |
050912e to
1578fc6
Compare
|
Went through this again against the current head ( Real progress since the last round: the AWS static-credential gap is now a genuine, well-designed fix, 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
Also, 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, 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, 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. |
|
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. |
c00a253 to
bc0fb04
Compare
|
@theredspoon Thank you for the follow-up review!
|
|
Went through the current head (
let http = reqwest::Client::builder().timeout(config.timeout).build()?;That leaves reqwest's default policy in place, which follows redirects and only strips Unbounded buffering + panic in error-body truncation
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
};
Smaller items
|
|
@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 Zeroization is end-to-end now: the login body goes through typed structs instead of a 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: One I'd like your view on: a role with 425fb05. |
4d8e294 to
dd06172
Compare
|
Confirmed everything in On The "hostile Vault already has target access anyway" framing undersells this. The realistic case day to day is more mundane than either: a legitimate, uncompromised Vault, an operator who copies or templates a role with 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 Two more, from this round:
expires_at: (auth.lease_duration > 0).then(|| {
Instant::now() + Duration::from_secs(auth.lease_duration).saturating_sub(TOKEN_EXPIRY_MARGIN)
}),
IPv6 loopback is misclassified as insecure. One more, lower priority: the AWS path is the one exception to end-to-end zeroization. |
|
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
The fix needs to be a deterministic signal, not a race. Separately: Vault config doesn't hot-reload
Cloud metadata tokens can transit an ambient proxy
Checked against Vault's actual server source ( Three real things remain from that investigation:
Response-wrapped AppRole secret IDs need the unwrapped value cached
Response wrapping protects one-time delivery of the secret ID, it doesn't force single-use of the secret ID itself. 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: Lower priority
|
409e2e5 to
4b825c1
Compare
|
Both rounds are in commit 409e2e5. @theredspoon 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 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 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.
c5dea27 to
58f831a
Compare
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.
58f831a to
d818090
Compare
|
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
Also OpenBao refuses to enable an audit device over the API, and its config stanza needs Two CI gates are red and neither is from this branch:
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.
|
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 This came out of two advisories rather than out of the diff: CVE-2024-7594, where an empty 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 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.
Checked and clean, for the record: russh 0.62.6 is current against all fourteen of its advisories, and CI is still red on |
|
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 ( What #2413 doesn't cover, since it's written against 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 Pinned critical options are only checked when the certificate actually carries them
Smaller items, roughly by severity
One more, separate from the above: the terminal-escape-sequence fix in 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. |
|
Verified the two CI/measurement items against The "in CI, measured" section isn't, by its own numbers. It opens describing 53 of 53 guards discriminate is cited to Ask: correct the write-up so it doesn't describe this as running in CI, repoint the citation to |
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.
`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.
|
@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 So Citation repointed. The artifact — and this is where it gets worse. Going to fetch it found 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:
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 The instrument. The tool is in this PR at
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. 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. |
|
Confirmed all three fixes ( 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. |
|
@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 Measured properly, from api 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 ( 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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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.
Twenty iterations of the two hostile suites in one job. The full Test workflow takes forty minutes and reproduces this about one run in four; these suites take three minutes each, so the same number of attempts costs an hour instead of most of a day. Not for PR warp-tech#2397. This workflow exists on this branch and no other.
The three-corner comparison put this in the certificate path: forty attempts of the same five hostile modes reproduced nothing on upstream and nothing on this branch with public-key targets, while the certificate suite reproduces on the first iteration. So trace the client here rather than in the probe that hits once in sixty. The gateway's log already shows it refusing the target and closing the session within a second; what is missing is which message the client never got. Fork-only: this edits the suite's connect() and is not for PR warp-tech#2397.
…RL parsed Requiring HTTPS for the Vault address put every suite's issuer behind TLS, and `openssl req -x509` emits CA:TRUE, which rustls refuses as an end entity while the macOS verifier accepts it — green locally, 79 failures in CI, every one of them CaUsedAsEndEntity. The stub now serves a leaf issued by its own CA, and the contract suite's Vault and OpenBao get a chain generated here rather than `-dev-tls`, which mints a fresh certificate on every start and so made the restart test watch the gateway reject an identity it had never trusted. That CA declares keyUsage: OpenSSL refuses an issuer that never claimed the right to sign, and the refusal reaches the client as a server that appears not to have started. `url()` returns a parsed Url joined onto the base instead of a string built by format!, so the scheme `validate_address` insists on travels with the value rather than surviving as a convention. The base is normalised to end in a slash: Url::join replaces the last path segment otherwise, and a Vault reached through a reverse proxy at /vault would have lost that prefix from every request — a regression the new test pins down. The ten assertions that interpolated a refusal reason no longer do. The reason is derived from the certificate, so each counted as writing key material to a log; the interpolated parts were option and extension names, never secrets. A failing test now prints the tail of the gateway log.
Brings in v0.28.5, the WebSocket header alignment (warp-tech#2507), ssh-dss under allow_insecure_algos (warp-tech#2506), and warp-tech#2499, which renames SessionId to UserSessionId, replaces TargetOptions matching with TargetSSHOptions::extract, and splits chain resolution into an admin path and an approved path. Eight files conflicted. Two resolutions keep this branch's behaviour over upstream's, deliberately. The admin host-key endpoint stays on CheckHostKey rather than returning to Connect: for a certificate target, Connect would carry on into authenticating and mint a real certificate to run a diagnostic. And the resolved chain keeps its per-hop identity instead of being reduced to plain SSH options, because that identity is what lets connect_chain answer about the target that was asked about rather than the last one. Upstream reached the same shape independently, so their ApprovedTarget maps onto it unchanged. The schemas were regenerated from the merged types rather than taken from either side. Taking upstream's dropped SSHTargetAuth_SshTargetCertificateAuth and still compiled, which is the failure that check exists to catch.
… signal Three Vault client tests were latency assertions without meaning to be. The shared test config allowed five seconds per request — never a property under test, since the tests that care about the timeout set their own — but it decided how slow a machine had to be before a login over TLS counted as the behaviour under test failing. After the laptop woke from hibernation five tests failed every run, all on the login, and passed with a longer allowance. It is thirty seconds now. `test_an_endless_error_body_is_not_buffered` separates a reader that stops at the cap from one that waits out the whole stream, so the distance between them is what matters and not either number: they were ten seconds and two apart, and the early stop alone measured 3.8 seconds under load and 7.6 on the woken machine. Sixty against twenty keeps the shape and the room. Installing the crypto provider moves into the helper every TLS test reaches. Three tests installed it and the rest relied on one of those three running first — true in file order, not under cargo's parallelism. The mutation matrix passes the suites the timeout CI passes them. Left at the default, a discriminator could time out before its mutation was applied and the guard be reported `already failing`, which accuses the guard of what was only the harness being impatient. The gateway log tail is now chosen by naming the signal rather than excluding each dependency's tracing in turn. That list lost three times — to the tokio runtime, to the config watcher reporting the data directory every ten seconds, and to rustls printing every handshake — and each time a real failure arrived carrying no information.
Forty-six comment blocks recorded development history rather than reasons: what an earlier version did, what a review argued, what `cargo mutants` found, which round noticed what, what the first draft of a test got wrong. A reader needs the rule and the reason for it; a maintainer meeting this branch for the first time needs its provenance least of all. Each block keeps its invariant and loses the narration. Nothing is removed wholesale, so the saving is modest — 195 comment lines out, 131 back in — and the density is unchanged. That was not the goal: the long blocks that carry the density were read and left alone, because they argue why the code is shaped as it is, and cutting them would take away the reasoning a reviewer needs. No code changed, in any file. One pre-existing defect fixed on the way: the doc comment above `a_far_future_expiry_is_described_rather_than_panicked_on` had two unrelated paragraphs merged into it, so a sentence about the authentication budget ran mid-line into one about certificate expiry.
Hi @Eugeny, how are you? Do you think this PR is still worth merging? I know it grew far beyond what I expected and it does look like an AI battlefield, but every step was checked by us personally. I tried to cut the comments down, but almost all of them turned out to carry reasoning that makes review easier rather than harder, so most of them stayed. |
|
Sorry to keep you waiting! I will review and merge this, but unfortunately my bandwidth is full right now - I'm working hard to get the admin approvals out of the door, and the OpenBao PR is next after that. It might take a bit longer unfortunately but there are no other roadblocks against this PR at the moment |
|
@Eugeny thank you very much for your answer. I'll keep this PR updated as long as you need. |
Brings in the non-admin target page fix (warp-tech#2523), the RDP dithering fix (warp-tech#2524) and the RDP interactive logon option (warp-tech#2526). The only conflict was the git-describe version string in the admin schema, which CI regenerates before comparing.
5 participants
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.
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).