Repository navigation
Conversation
A certificate from `miren auth generate` was good until it expired, with no way to withdraw one short of rotating the CA. The check sits in ServeHTTP rather than an authenticator, because the local and cloud authenticators each turn a certificate into an identity and a check in one would leave a hole in the other. Running ahead of authentication also means a valid bearer token cannot carry a revoked certificate through. Requests read an in-memory set and never the store, so an etcd outage cannot fail auth; the server enforces what it last knew. The listener opens before the entity store exists, so the set starts unloaded and refuses every certificate with a retryable 503 until it loads. Authentication only runs when a request starts, so revoking also has to end what is already running. The set tracks in-flight requests by serial and cancels them, with admission and registration in one critical section so nothing slips between check and revocation. Cancellation is not enough alone: a WebTransport session does not inherit the request context, and detached handlers such as builds keep sending, so revoking closes the session as well. The set only grows. A snapshot can predate a revocation already enforced here, so a serial missing from one proves nothing.
Revocations are entities named for their serial, so revoking twice lands on one record. The server keeps no ledger of what it issued, so Revoke accepts any well-formed serial and cannot tell a typo from a real certificate. Boot fails closed. The load reads the store directly, since the loopback client would go through the listener waiting on it, and any record that cannot be read or enforced stops the server: it might be the only thing keeping a revoked certificate out. After boot it fails static. Revoke writes through so the certificate is refused by the time it returns, and a watch picks up anything else. The watch only adds. A resync can be older than a write-through and listings omit undecodable records, and a queued delete can be older than a fresh Revoke of the same serial. So deleting a revocation by hand takes effect on restart, and there is deliberately no un-revoke API. The service checks identity itself because the authorizer waves every certificate through, and runner certificates must not qualify. Revoke also refuses the certificate it is running with.
Serials circulate in two formats: audit logs print decimal, openssl prints hex. Parsing one as the other succeeds and revokes a certificate that does not exist, so digits alone mean decimal, 0x or any hex letter means hex, and output echoes both forms so a wrong guess is visible. A revoked certificate gets its own diagnostic. The generic access-denied text sends people to `miren login`, which can never fix this.
Runner listeners trust the same CA as the coordinator, so enforcing revocation only there left a revoked certificate able to dial a runner directly and read metrics or exec into its sandboxes. Runners now watch the same revocations and enforce them on their own listener. They have no store to load from at boot, so they refuse certificates until the first snapshot arrives, and a bad record in that snapshot leaves them unloaded, matching the coordinator failing to boot. A revocation reaches a runner a moment after Revoke returns. Exec and attach now also accept only the coordinator, whose proxy is how every legitimate call arrives. Without that, any unrevoked client certificate could reach a sandbox past the checks the proxy applies.
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The coordinator dials itself with its API certificate, so revoking that serial cut off the entity client behind every controller. The set only grows and the certificate is restored from cluster state on boot, so a restart failed at the loopback instead of recovering, leaving only a hand edit of etcd. The refusal names no fix because there is no way to rotate this certificate yet.
Message-session listeners dispatch without passing through ServeHTTP. Nothing enables them on a State that verifies certificates today, but whoever does needs to know revocation will not apply there.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
evanphx
left a comment
evanphx
left a comment
There was a problem hiding this comment.
Nice work. The admission design holds up: checking and registering under one lock, closing sessions on revocation for detached handlers, and a set that only grows. I checked every caller of dev.miren.runtime/exec, and limiting runner exec to the coordinator breaks no direct callers. A few non-blocking notes inline. The runner certificate one is the only one I'd want written down before this ships.
| if serial == rpc.CallerCertSerial(ctx) { | ||
| return fmt.Errorf("refusing to revoke certificate %s: this command is running with it; reissue first, then revoke from the new certificate", serial) | ||
| } | ||
| if serial == s.apiSerial { |
There was a problem hiding this comment.
A gap worth writing down. Someone holding a leaked runner certificate can call RunnerRegistration.RefreshCertificate before it's revoked. That mints a new certificate with the same CommonName and a fresh serial. Revoking the original serial doesn't touch the new one. The server can't detect this because it keeps no record of what it issued. Refresh does already refuse runners that are no longer registered, so the real fix for a leaked runner certificate is to remove the runner. Could the command's help, or the warning printed after a revoke, say "for a runner's certificate, also remove the runner"?
There was a problem hiding this comment.
af42561 covers this in two places. The auth revoke description, which ends up on the generated docs page, says to run miren runner delete and then check the audit log for other serials with that runner's subject. The warning printed after every successful revoke says the same thing, which matters because --help doesn't render the description. The CLI can't tell from a serial whether it belonged to a runner, so printing the warning every time is the honest choice. I'll leave closing this to you.
| exec_v1alpha.SandboxExec | ||
| } | ||
|
|
||
| func (e coordinatorOnlyExec) Exec(ctx context.Context, state *exec_v1alpha.SandboxExecExec) error { |
There was a problem hiding this comment.
Nit: coordinatorOnlyExec reuses requireCoordinator, whose errors say "only the coordinator may administer this node" and "node administration requires…". Someone refused on exec or attach will find that confusing. Could the message name the method, or use a neutral wording like "only the coordinator may call this on a runner"?
There was a problem hiding this comment.
e2e74ef makes all three errors in requireCoordinator neutral ("this call requires the coordinator's certificate…", "only the coordinator may call this on a runner…"), so they fit exec/attach, query, and node admin alike. Nothing still asserts on the old wording. I'll leave closing this to you.
| Body: "miren auth generate", | ||
| }), | ||
| )) | ||
| d.Dispatch("auth revoke", Infer("auth revoke", "Revoke a client certificate so the cluster refuses it", AuthRevoke, |
There was a problem hiding this comment.
Suggestion: the generated auth-revoke.md lists only flags. The limits explained in the PR description belong in the command's long help, so they also land in the generated docs. Those limits are: no un-revoke (reissue the certificate instead), the server can't tell a typo from a real serial, and how hex vs. decimal input is read.
There was a problem hiding this comment.
authRevokeDescription now covers the hex/decimal rule, the typo limitation, no un-revoke (reissue instead), the certificates it refuses to revoke, and the runner case. docs/docs/command/auth-revoke.md has been regenerated with that text. I checked the parsing description against parseCertSerial and they agree. I'll leave closing this to you.
requireCoordinator started out guarding node administration, and its errors said so. It now also guards the query service and, since this PR, exec and attach, so someone refused on exec was told about node administration. The wording now fits every caller.
The limits only lived in the PR description, and the generated auth-revoke page listed nothing but flags, which is a poor place to learn them in the middle of a leak. A runner certificate gets its own paragraph. RefreshCertificate lets it mint a replacement with a fresh serial, and the cluster keeps no record of what it issued, so revoking the old serial leaves the replacement alone. Removing the runner stops further renewals but does not touch a replacement already minted, so the help says to do both and to go after any serial the audit log turns up. --help does not render WithDescription for any command, so that text reaches only the docs page. The runner point also goes in the warning printed after a revoke, where someone mid-leak will actually see it. We can't tell from a serial whether it was a runner's, so it prints every time and says so.
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
Still ready for human review. The two new commits take up evanphx's three inline suggestions, and I didn't find anything new that should block.
e2e74ef: neutral requireCoordinator wording. The three errors in components/runner/nodeadmin.go now say "this call requires…" and "only the coordinator may call this on a runner". They read correctly whether the refusal comes from node admin, the query service, or the new coordinatorOnlyExec wrapper. Nothing in the tree still asserts on the old wording. exec_v1alpha.SandboxExec has only Exec and Attach, and the wrapper guards both, so no exec method gets past the coordinator check through the embedded interface.
af42561: limits in the help, and the runner-renewal warning. authRevokeDescription and the regenerated docs/docs/command/auth-revoke.md now cover:
- how hex and decimal input is read
- that the server can't tell a typo from a real serial
- that there's no un-revoke, so you reissue instead
- which certificates it refuses to revoke
- what to do about a leaked runner certificate
The parsing description matches parseCertSerial. Digits alone take the decimal branch. A letter, a colon, or 0x takes the hex branch, which strips the colons. The confirmation prompt does print both forms. The claim that an all-digit hex serial is "vanishingly unlikely" depends on 128-bit serials, and that holds: caauth.generateSerialNumber issues them, and the 63-bit generator in pkg/rpc/cert.go only makes self-signed certs.
Repeating the runner advice in the stderr warning after a revoke is the right move. As the commit says, --help doesn't render WithDescription, and the warning is what someone handling a leak will actually read. The advice also matches how revocation behaves. A replacement certificate that was already minted isn't touched. runner delete stops further RefreshCertificate calls. The audit trail records subject and serial, so the other serials can be found and revoked.
My two earlier human-judgment threads are closed, and the API-certificate guard hasn't changed since the last round.
I'm still not recommending approval in place of a human review, for the reason I gave before. This changes authentication on every HTTP listener the coordinator and runners expose, and it makes boot fail closed based on what's in etcd. A person should sign off on that, even though I have no open defects. I've replied on evanphx's threads with what I see and left closing them to him.
🍪 full review note · reviewed at af42561
A client certificate from
miren auth generatestays valid for a year, and until now the only way to withdraw a leaked one was to rotate the whole CA. This adds a serial blocklist in the entity store andmiren auth revoke <serial>to add to it, withmiren auth revocation listto check what's in force.The check runs in
ServeHTTP, ahead of authentication, so it covers both the local and cloud auth paths, and a revoked cert is refused even alongside a valid token. Revoking takes effect on the coordinator by the time the command returns: the cert's next request is refused, and its open streams are torn down at the transport, since detached handlers like builds ignore cancellation. Runners trust the same CA, so they watch the same list and enforce it on their own listeners, a moment later. Runner exec and attach now also accept only the coordinator's certificate.Failure modes are the part most worth a look. Boot fails closed: certs get a retryable 503 until the list loads, and an unreadable record stops the server. After that it fails static, because requests only read an in-memory set, so an etcd outage can't fail auth or lock operators out. The set only grows, which rules out a stale snapshot or delete quietly undoing a revocation. The cost is that there's no un-revoke: a hand-deleted revocation only lifts on restart, and the normal recovery is reissuing the cert.
Two limits to know about. The server keeps no record of what it issued, so
auth revokecan't tell a typo from a real serial; it warns and echoes both hex and decimal, because audit logs and openssl disagree on format. And a cloud JWT caller may need an RBAC rule before it can call the new service, while operator certificates work today.Part of MIR-779