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:
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.
Posted to the PR's comment threads when you submit.
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.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.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.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1430,
"verdict": "ready",
"event": "comment",
"summary": "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.\n\n**`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.\n\n**`af42561`: limits in the help, and the runner-renewal warning.** `authRevokeDescription` and the regenerated `docs/docs/command/auth-revoke.md` now cover:\n- how hex and decimal input is read\n- that the server can't tell a typo from a real serial\n- that there's no un-revoke, so you reissue instead\n- which certificates it refuses to revoke\n- what to do about a leaked runner certificate\n\nThe 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.\n\nRepeating 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.\n\nMy two earlier human-judgment threads are closed, and the API-certificate guard hasn't changed since the last round.\n\nI'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.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss60OiH4",
"body": "`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."
},
{
"thread_id": "PRRT_kwDONZ__ss60OiIL",
"body": "`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."
},
{
"thread_id": "PRRT_kwDONZ__ss60OiIQ",
"body": "`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."
}
],
"posted_to_pr": true,
"head_sha": "af4256173ddcf6fa08c628112b9ec70e824eb40d"
}The blocker from my last review is fixed, and I didn't find anything new that should hold this back. It's ready for human review.
API certificate guard (6c9c1db). Foundation.Start now parses c.apiCert and passes rpc.CertSerial(apiCert.SerialNumber) into NewServer. Revoke refuses that serial right after the caller-serial check, before it reads or writes the store. Both sides compare the same canonical form, because ParseCertSerial ends by calling CertSerial. So an uppercase or zero-padded spelling of the API serial is still refused. TestRevokeRefusesCoordinatorAPICertificate checks exactly that with strings.ToUpper, and also confirms nothing was stored, which was the part behind the restart failure.
The guard also keeps the runner side safe. The exec proxy in workload_control.go dials runners through the same rs that carries the API cert, and runners now accept exec and attach only from the coordinator's certificate. If that serial could be revoked, every runner would refuse the proxy too.
Not naming a fix in the error is the right call while there's no way to rotate the API cert. Pointing people at a procedure that doesn't exist would be worse than the rotation hint I originally asked for.
HTTP-only note (b83865d). The WithCertRevocations doc now says enforcement lives in Server.ServeHTTP and doesn't cover the TCP or WebSocket listeners. That's the note I asked for.
Runner staying closed after a bad first snapshot. You've said you're keeping this on purpose: it only happens with corrupt or hand-written data, it's logged at Error, and a restart is simpler than a second recovery path. That's your call. I've replied on the thread and left it for a human to close.
I'm still not recommending approval in place of a human review. This changes authentication on every HTTP listener the coordinator and runners expose, and it makes boot fail closed on the contents of etcd. A person should sign off on that, even though I have no open defects.
Posted to the PR's comment threads when you submit.
Foundation.Start gets the serial from c.apiCert with rpc.CertSerial and passes it in, and Revoke refuses it before touching the store. ParseCertSerial canonicalizes through the same CertSerial, so an uppercase or padded spelling can't slip past. TestRevokeRefusesCoordinatorAPICertificate covers both the refusal and the empty listing afterwards, so nothing is left on disk for a restart to reload. Leaving the rotation hint out of the error makes sense while there's no rotation path. Resolving.WithCertRevocations now. It says enforcement lives in Server.ServeHTTP and that the WithTCPBindAddr/WithWSBindAddr listeners aren't covered, which is what I was after. I filed this under human judgment in my last review, so I'll leave closing it to a reviewer.Verdict: ready
One command in this PR can take the coordinator down with no recovery path through the API, so I'm marking it not ready. The rest of the design holds up, and the fix is small.
Blocker: nothing stops anyone from revoking the coordinator's own API certificate (servers/certrevocation/server.go:58)
Revoke refuses only the caller's own serial. The coordinator's RPC state is built with WithCertPEMs(c.apiCert, c.apiKey), and NewState puts that cert into the client TLS config. That means the loopback client created at foundation.go, rs.Connect(rs.LoopbackAddr(), "entities"), presents the API cert to the coordinator's own listener. That client backs eac, every controller that uses it, and the revocation Watch itself.
Revoking that serial has two effects:
revokeLocked cancels the loopback's in-flight requests, and every new request gets a 401.certrevocation.Load adds the serial back before the loopback is created, and LoadAPICert keeps reusing server/api.crt while it's still valid. Boot then fails at the loopback.Because the set only grows, the only ways out are hand-editing etcd or deleting api.crt, and neither is documented. A leaked API key is exactly when an operator would run this command, and "reissue the cert" doesn't apply to the cert the coordinator is running on. Refuse the server's own serial the same way the caller's is refused, point the error at how to rotate the API cert, and add a test next to TestRevokeRefusesCallersOwnCertificate.
What I checked and trust
Server.ServeHTTP ahead of authentication, which covers both the local and cloud auth paths. TestRevokedCertificateBeatsOtherCredentials confirms a valid bearer token can't carry a revoked cert through.admit checks the set and registers the in-flight request under one lock, so a revocation either refuses a request or finds it registered and cancels it.closeOnRevocation tears down the WebTransport session, and TestRevocationStopsDetachedStream shows it stopping a handler that ignores cancellation.indexwatch.Watcher.Start always returns nil and retries snapshots itself.For a human to judge (inline)
pkg/certrevocation/certrevocation.go:102). If a runner's first snapshot contains one bad record, it stays unloaded until it restarts, even after the record is repaired. While unloaded it refuses the coordinator's exec and node-admin calls. Please confirm this is the intended tradeoff.pkg/rpc/server.go:535). Enforcement lives only in ServeHTTP; the TCP, WebSocket and mem message-session paths skip it. Nothing enables those listeners today, but it's worth writing down.Even with the blocker fixed, this changes authentication on every listener the coordinator and runners expose, so it should go through human review rather than merge on my read alone.
servers/certrevocation/server.go:58 — This guard protects the caller's certificate, but not the certificate this server runs on. In Foundation.Start, rs is built with WithCertPEMs(c.apiCert, c.apiKey), and NewState puts that cert into clientTlsCfg.Certificates. So the loopback rs.Connect(rs.LoopbackAddr(), "entities"), which backs eac, every controller and this package's own Watch, presents the API cert to this same listener. If someone revokes that serial:
revokeLocked cancels the loopback's in-flight requests, and every new one gets a 401.
After a restart, certrevocation.Load adds the serial back before the loopback is created. LoadAPICert keeps reusing server/api.crt as long as it's still valid, so boot fails at the loopback, and nothing in the API can undo it.
Recovery means hand-deleting the etcd record or deleting api.crt, and neither is documented. A leaked API key is a realistic reason to run auth revoke on exactly this serial, and the description's advice to reissue the cert doesn't work for a cert the coordinator is running on.
Suggested fix: pass the server's own cert serial into NewServer and refuse it here the same way you refuse the caller's, with a message that says how to rotate the API cert first. Add a test next to TestRevokeRefusesCallersOwnCertificate.
EventAdded/EventUpdated only Add, and indexwatch sends another EventSync only after a compaction. So even once the record is repaired in etcd, the runner keeps returning 503 to every cert, including the coordinator's exec proxy and node-admin calls, until it restarts. The commit message's parallel with the coordinator failing to boot is a fair argument. If the goal is just to fail closed until the data is good, re-listing on a timer while unloaded would let the runner recover without a restart.ServeHTTP covers HTTP/3, the REST gateway and the local listener. The message-session transports (WithTCPBindAddr, WithWSBindAddr, mem) dispatch through sessionRouter/prepareMethodCall and never reach this line. Neither the coordinator nor the runner turns those listeners on today, so nothing is exposed now. A short note here or on WithCertRevocations saying enforcement is HTTP-only would help whoever later enables a TCP or WebSocket listener on a State that verifies certs.Verdict: not_ready