dist: skip stale cert when rebuilding scheduler HTTP client - #2707
Open
timn-nexthop wants to merge 4 commits into
Open
timn-nexthop wants to merge 4 commits into
timn-nexthop wants to merge 4 commits into
Conversation
When a build server registers with a new certificate, the scheduler rebuilds its outbound reqwest client with the new cert plus every other cert it knows about. The loop over `certs.values()` ran before `certs.insert(...)` overwrote the map entry, so it still contained the stale cert for the same `server_id` — meaning both the old and new self-signed certs for that server were installed as trust anchors in the rebuilt client. Each build server's cert is self-signed, so old and new share a Subject DN. TLS validators index trust anchors by Subject; with two anchors having the same name, path building can pick the stale one, fail signature verification against its public key, and reject the handshake. The result is that cert rotation on a build server deterministically breaks the scheduler's ability to talk to it until the scheduler restarts. Skip the entry matching `server_id` when iterating existing certs so only the up-to-date cert for that server ends up as a trust anchor.
sylvestre
reviewed
Sep 22, 2026
| client_builder = client_builder.add_root_certificate( | ||
| reqwest::Certificate::from_pem(cert_pem).expect("previously valid cert"), | ||
| reqwest::Certificate::from_pem(existing_cert_pem) | ||
| .expect("previously valid cert"), |
Collaborator
There was a problem hiding this comment.
since you're touching it, could you please drop the expect() and propagate the error?
sylvestre
reviewed
Sep 22, 2026
| ); | ||
| for (_, cert_pem) in certs.values() { | ||
| // Add all OTHER existing certificates (skip the one we're updating) | ||
| for (sid, (_, existing_cert_pem)) in certs.iter() { |
Collaborator
There was a problem hiding this comment.
could be simpler: insert into certs first, then loop over all values, no skip needed?
Author
There was a problem hiding this comment.
Not sure if it's simpler, but done.
sylvestre
reviewed
Sep 22, 2026
| @@ -742,14 +742,19 @@ mod server { | |||
| server_id.addr() | |||
Collaborator
There was a problem hiding this comment.
please add a test for the rotation case; maybe_update_certs could be moved out of the closure to make it testable.
When rebuilding the scheduler's HTTP client, the certificates already stored for other servers were parsed with `expect()`, so a bad stored cert would panic the request handler. Return the error instead so only that heartbeat fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Insert the server's new certificate into the map first, then build the client from the map's values. The new cert replaces the stale entry for that server, so it no longer needs to be added separately and the old one skipped. If building the client fails, restore the previous map entry. Otherwise the map would hold a cert the client doesn't trust, and the next heartbeat with the same digest would skip the rebuild. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move `maybe_update_certs` out of `Scheduler::start` so it can be tested directly, and add a test that rotates a build server's certificate and checks which certs the scheduler's client trusts afterwards, including when an update fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
|
Note that I couldn't reproduce this bug with the test on my server today. I think it's still running using openssl on the box, so maybe something changed there. I still think this change does make the code more correct, even if (today) it's not causing trouble for me. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a build server registers with a new certificate, the scheduler rebuilds its outbound reqwest client with the new cert plus every other cert it knows about. The loop over
certs.values()ran beforecerts.insert(...)overwrote the map entry, so it still contained the stale cert for the sameserver_id— meaning both the old and new self-signed certs for that server were installed as trust anchors in the rebuilt client.Each build server's cert is self-signed, so old and new share a Subject DN. TLS validators index trust anchors by Subject; with two anchors having the same name, path building can pick the stale one, fail signature verification against its public key, and reject the handshake. The result is that cert rotation on a build server deterministically (depending on TLS library) breaks the scheduler's ability to talk to it until the scheduler restarts.
Skip the entry matching
server_idwhen iterating existing certs so only the up-to-date cert for that server ends up as a trust anchor.We've been running with this change since mid December and it's fixed this problem for us.