Build internal HTTP clients without loading TLS roots by smklein · Pull Request #11482 · oxidecomputer/omicron

smklein · 2026-10-13T21:15:19Z

Since the upgrade to reqwest 0.13 (#9917), building a reqwest::Client loads the system's TLS root certificates from disk. This can fail, as we've seen, when the process is out of file descriptors. Progenitor's generated Client::new unwraps that result, which is how Nexus panicked in #10871 (see also oxidecomputer/progenitor#1440).

Control plane services on the underlay currently talk to each other over plain HTTP, so they have no use for those certificates. This PR adds omicron_common::http_client::{internal_http_client_builder, internal_http_client}, which trust no TLS roots (tls_certs_only([])) and therefore do no I/O when building a client, and uses them in place of generated Client::new calls and reqwest::ClientBuilder::new() for internal clients. Requests to https URLs through these clients fail certificate verification; clients that need TLS (webhooks, IdP metadata) are unchanged.

This intentionally keeps the existing unwrap/expect behavior; a follow-up PR will propagate client-construction errors instead.

Part of #10871.

Building a reqwest client loads the system's TLS root certificates from disk, and fails if none can be read (e.g., when out of file descriptors). Control plane services talk to each other over plain HTTP, so add builders that trust no TLS roots and therefore do no I/O. See #10871.

Progenitor-generated Client::new builds a reqwest client that loads TLS root certificates from disk, and unwraps the result. For clients that talk plain HTTP to other control plane services, build the client with internal_http_client() instead, which does no I/O.

Clients that talk plain HTTP to other control plane services don't need TLS root certificates. Build them with internal_http_client_builder(), so building the client does no I/O.

smklein · 2026-10-13T21:21:14Z

-        let client = reqwest::Client::new();
+        let client = internal_http_client_builder()
+            .build()
+            .expect("building a client without TLS roots does no I/O");


just as an "FYI":

progenitor was internally doing this unwrap here in request::Client::new (see: oxidecomputer/progenitor#1440) - so although you, a reviewer, might ask "why are we unwrapping now", the answer is: "we did before, and the goal is to keep this PR focused on one thing: plumbing through the tls_certs_only([]) parameter".

Removing the unwrapping is a good idea, but will come separately.

…scriptors

smklein · 2026-10-13T21:29:47Z

+    // This test exhausts the process's file descriptors, so it relies on
+    // nextest running each test in its own process.
+    #[test]
+    fn internal_http_client_builds_without_file_descriptors() {


This test is a variation of what I described in oxidecomputer/progenitor#1440 (comment) , which is a regression test against #10871

sunshowers · 2026-10-13T21:53:06Z

Dumb question I want to ask is whether we want this to live in progenitor, and/or whether it's possible to add a clippy disallowed-methods lint for this.

smklein · 2026-10-13T21:57:11Z

Dumb question I want to ask is whether we want this to live in progenitor, and/or whether it's possible to add a clippy disallowed-methods lint for this.

When you say "this", are you referring to the "helper to build an HTTP-only API, which passes this no-TLS argument"?

I could go either way on it - this is something that we'll be probably removing over time, but just accurately describes the current situation. I'd understand not wanting to push forward the "here's how to bypass HTTP" lever to the forefront of the API. But I do think progenitor should avoid generating a method which can panic based on I/O (hence: oxidecomputer/progenitor#1440 being an issue).

Also we can clippy disallowed-method here to dodge each generated Client::new, while progenitor is still doing the unwrapping internally -- but it's a little weird, because we also would need to disallow each individual generated "Client" type individually. Trivial to add, but a little more of a pain in the butt, because we gotta add each one separately.

sunshowers · 2026-10-13T21:59:01Z

When you say "this", are you referring to the "helper to build an HTTP-only API, which passes this no-TLS argument"?

Yeah, basically, a helper for us. Maybe behind a Cargo feature if we feel nervous about exposing it generally. I bet @ahl has much more sophisticated thoughts here than I do.

sunshowers · 2026-10-13T22:06:12Z

Also we can clippy disallowed-method here to dodge each generated Client::new, while progenitor is still doing the unwrapping internally -- but it's a little weird, because we also would need to disallow each individual generated "Client" type individually

Ah, right. And if we add a new client we'd have to remember to add it there. (Maybe we need a checklist for adding a new API client.)

hawkw

this looks good to me, with a few minor notes.

hawkw · 2026-10-13T21:52:20Z

+//!
+//! By default, building a [`reqwest::Client`] loads the system's TLS root
+//! certificates from disk, and fails if none can be read (for example, if the
+//! process has run out of file descriptors). Services within the control plane
+//! talk to each other over plain HTTP, so they have no use for those
+//! certificates.
+//!
+//! See <https://github.com/oxidecomputer/omicron/issues/10871>.


nit, take it or leave it: i'm not really sure if this makes sense in the module level comments; I think even once we are doing underlay TLS or something, we will probably still want to keep this code around, and we can imagine this module growing other non-reqwest-default configs. Personally, I think that the doc comment here should really be saying that "this is a centralized set of configurations for all internal control plane HTTP clients" and I think this comment belongs on the actual call to tls_certs_only([]).

yeah, good call, moved in b62a484

hawkw · 2026-10-13T21:57:36Z

+/// URLs will fail certificate verification. Clients that need to speak TLS
+/// (e.g., to services outside the rack) should not use this builder.


I think we should probably encourage the use of the external_client module in Nexus here, specifically, because it applies its own set of non-default configs that we should probably be using on any HTTP requests we send outside the rack. This includes SSRF protection for cases where the external URL we're talking to is untrusted, which (currently) is all cases where we send external requests, so we should really ensure that we always use that module for such requests for security reasons.

Added in b62a484

hawkw · 2026-10-13T22:04:42Z

+        .connect_timeout(PROGENITOR_DEFAULT_TIMEOUT)
+        .timeout(PROGENITOR_DEFAULT_TIMEOUT)


it's a bit odd to me that internal_http_client_builder sets the TLS thing but doesn't set the other configs we set by default. it feels like we should basically set all the defaults there and then allow a (hypothetical?) caller that wants to override them override the individual builder settings to get the default connect timeout/timeout settings too?

I realize that these are the main settings that every caller that currently even uses the builder path instead of just calling this function are currently overwriting, so there isn't any immediate benefit to having the builder function also set them. But, I feel like with these kind of interfaces, in general I feel like it makes more sense for the API that provides a builder to set all the settings that we expect callers to want to potentially clobber, so they're getting the same defaults as the non-builder path for anything they don't clobber.

Also, I did double-check what the builder does, and it just sets the thing again when you call one of these settings multiple times: https://docs.rs/reqwest/latest/src/reqwest/async_impl/client.rs.html#1444-1478

asdf so the complication here is that, although we can change the timeouts here, we cannot remove them. There are several clients that only have one of the connect/total timeouts set:

Call site Purpose Connect timeout Total timeout

sled-agent/src/artifact_store.rs:161 Artifact downloads between sleds 15s none

sled-agent/src/instance.rs:2552 Per-instance propolis client none none

sled-agent/src/nexus.rs:38 sled-agent → Nexus (via internal DNS) none none

sled-agent/scrimlet-reconcilers/src/handle.rs:115 Scrimlet reconcilers (also sets 10s pool idle timeout) 15s none

sled-agent/rack-setup/src/service.rs:347 RSS → sled-agent (set sled config) 60s none

sled-agent/rack-setup/src/service.rs:425 RSS → sled-agent (wait for reconciliation) 60s none

sled-agent/rack-setup/src/service.rs:961 RSS → cockroach-admin (cluster init) 60s none

cockroach-admin/src/http_entrypoints.rs:90 Proxy to CockroachDB /_status/vars none none

cockroach-admin/src/http_entrypoints.rs:134 Proxy to CockroachDB /_status/nodes none none

oximeter/collector/src/collection_task.rs:235 Oximeter → metric producers none 30s

If we "set all the defaults" we cannot unset them for these cases. We could set an "arbitrarily high" timeout here instead? But the caller would need to do that explicitly, I think?

Oh, that's yucky. I didn't look closely enough at the reqwest code to realize that we couldn't set them to None again. Sigh.

I guess the ugly-but-workable solution might be to add a function that takes an &mut ClientBuilder and sets the defaults timeouts from this module on it, so that callers that want to keep the default timeouts but configure some other thing can do that. But we can save that for when it becomes actually necessary.

hawkw · 2026-10-13T22:17:23Z

+        .connect_timeout(PROGENITOR_DEFAULT_TIMEOUT)
+        .timeout(PROGENITOR_DEFAULT_TIMEOUT)
+        .build()
+        .expect("building a client without TLS roots does no I/O")


IIUC ClientBuilder::build() will also return errors if the builder's config is invalid, so I think we might want to be a little more explicit here and say something like

Suggested change

.expect("building a client without TLS roots does no I/O")

.expect(

"because building a client without TLS roots does no I/O, it will \

only return an error in the event of a programmer error",

)

updated in b62a484

- Describe the module as the central configuration for internal HTTP clients, and move the TLS-roots explanation to the tls_certs_only call. - Point callers that talk to services outside the rack at Nexus's external_client module. - Clarify that the remaining expect only fires on programmer error.

smklein added 4 commits October 13, 2026 12:39

Use internal_http_client_builder() for internal reqwest clients

e32e280

Clients that talk plain HTTP to other control plane services don't need TLS root certificates. Build them with internal_http_client_builder(), so building the client does no I/O.

omicron-common: trim http_client docs, drop trivial test

71ea968

smklein commented Oct 13, 2026

View reviewed changes

smklein added 2 commits October 13, 2026 14:26

omicron-common: test that internal HTTP clients build without file de…

0bd47f2

…scriptors

omicron-common: explain why the fd-exhaustion control check skips macOS

a835e4c

smklein commented Oct 13, 2026

View reviewed changes

smklein marked this pull request as ready for review October 13, 2026 21:32

smklein requested review from jgallagher and sunshowers October 13, 2026 21:33

hawkw approved these changes Oct 13, 2026

View reviewed changes

smklein mentioned this pull request Oct 13, 2026

Propagate HTTP client construction errors #11486

Draft

smklein merged commit 4bfd1a1 into main Oct 14, 2026
20 checks passed

smklein deleted the internal-http-client-no-tls-roots branch October 14, 2026 17:37

hawkw mentioned this pull request Oct 23, 2026

[reconfigurator] background task for stopping marked VMMs #11380

Merged

		/// URLs will fail certificate verification. Clients that need to speak TLS
		/// (e.g., to services outside the rack) should not use this builder.

		.connect_timeout(PROGENITOR_DEFAULT_TIMEOUT)
		.timeout(PROGENITOR_DEFAULT_TIMEOUT)

Call site	Purpose	Connect timeout	Total timeout
`sled-agent/src/artifact_store.rs:161`	Artifact downloads between sleds	15s	none
`sled-agent/src/instance.rs:2552`	Per-instance propolis client	none	none
`sled-agent/src/nexus.rs:38`	sled-agent → Nexus (via internal DNS)	none	none
`sled-agent/scrimlet-reconcilers/src/handle.rs:115`	Scrimlet reconcilers (also sets 10s pool idle timeout)	15s	none
`sled-agent/rack-setup/src/service.rs:347`	RSS → sled-agent (set sled config)	60s	none
`sled-agent/rack-setup/src/service.rs:425`	RSS → sled-agent (wait for reconciliation)	60s	none
`sled-agent/rack-setup/src/service.rs:961`	RSS → cockroach-admin (cluster init)	60s	none
`cockroach-admin/src/http_entrypoints.rs:90`	Proxy to CockroachDB `/_status/vars`	none	none
`cockroach-admin/src/http_entrypoints.rs:134`	Proxy to CockroachDB `/_status/nodes`	none	none
`oximeter/collector/src/collection_task.rs:235`	Oximeter → metric producers	none	30s

Conversation

smklein commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smklein Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunshowers commented Oct 13, 2026

Uh oh!

smklein commented Oct 13, 2026

Uh oh!

sunshowers commented Oct 13, 2026

Uh oh!

sunshowers commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smklein commented Oct 13, 2026 •

edited

Loading

smklein Oct 13, 2026 •

edited

Loading

smklein Oct 13, 2026 •

edited

Loading

sunshowers commented Oct 13, 2026 •

edited

Loading