Repository navigation
Build internal HTTP clients without loading TLS roots #11482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
084896f
1538190
e32e280
71ea968
0bd47f2
a835e4c
b62a484
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,97 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| // This Source Code Form is subject to the terms of the Mozilla Public | ||||||||||||||||||||||||||||||||||||||||||||||
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||||||||||||||||||||||||||||||||||||||||||||||
| // file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| //! Construction of HTTP clients for talking to other control plane services. | ||||||||||||||||||||||||||||||||||||||||||||||
| //! | ||||||||||||||||||||||||||||||||||||||||||||||
| //! 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>. | ||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, good call, moved in b62a484 |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| use std::time::Duration; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| /// The connect and request timeout applied by a progenitor-generated | ||||||||||||||||||||||||||||||||||||||||||||||
| /// `Client::new`. | ||||||||||||||||||||||||||||||||||||||||||||||
| const PROGENITOR_DEFAULT_TIMEOUT: Duration = Duration::from_secs(15); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| /// Returns a [`reqwest::ClientBuilder`] for plain-HTTP communication with | ||||||||||||||||||||||||||||||||||||||||||||||
| /// other control plane services. | ||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||
| /// The resulting client trusts no TLS root certificates: requests to `https` | ||||||||||||||||||||||||||||||||||||||||||||||
| /// URLs will fail certificate verification. Clients that need to speak TLS | ||||||||||||||||||||||||||||||||||||||||||||||
| /// (e.g., to services outside the rack) should not use this builder. | ||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should probably encourage the use of the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in b62a484 |
||||||||||||||||||||||||||||||||||||||||||||||
| pub fn internal_http_client_builder() -> reqwest::ClientBuilder { | ||||||||||||||||||||||||||||||||||||||||||||||
| reqwest::ClientBuilder::new().tls_certs_only([]) | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| /// Returns a [`reqwest::Client`] for plain-HTTP communication with other | ||||||||||||||||||||||||||||||||||||||||||||||
| /// control plane services, configured with the same timeouts as a | ||||||||||||||||||||||||||||||||||||||||||||||
| /// progenitor-generated `Client::new`. | ||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||
| /// Pass this to a generated client's `new_with_client` in place of calling its | ||||||||||||||||||||||||||||||||||||||||||||||
| /// `new`, which builds a client that loads TLS root certificates from disk. | ||||||||||||||||||||||||||||||||||||||||||||||
| pub fn internal_http_client() -> reqwest::Client { | ||||||||||||||||||||||||||||||||||||||||||||||
| internal_http_client_builder() | ||||||||||||||||||||||||||||||||||||||||||||||
| .connect_timeout(PROGENITOR_DEFAULT_TIMEOUT) | ||||||||||||||||||||||||||||||||||||||||||||||
| .timeout(PROGENITOR_DEFAULT_TIMEOUT) | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+47
to
+48
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a bit odd to me that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, that's yucky. I didn't look closely enough at the I guess the ugly-but-workable solution might be to add a function that takes an |
||||||||||||||||||||||||||||||||||||||||||||||
| .build() | ||||||||||||||||||||||||||||||||||||||||||||||
| .expect("building a client without TLS roots does no I/O") | ||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated in b62a484 |
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(test)] | ||||||||||||||||||||||||||||||||||||||||||||||
| mod test { | ||||||||||||||||||||||||||||||||||||||||||||||
| use super::*; | ||||||||||||||||||||||||||||||||||||||||||||||
| use std::fs::File; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // 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() { | ||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is a variation of what I described in oxidecomputer/progenitor#1440 (comment) , which is a regression test against #10871 |
||||||||||||||||||||||||||||||||||||||||||||||
| // Lower the soft limit on open files, so that we don't need to open | ||||||||||||||||||||||||||||||||||||||||||||||
| // very many to run out. | ||||||||||||||||||||||||||||||||||||||||||||||
| let mut limit = libc::rlimit { rlim_cur: 0, rlim_max: 0 }; | ||||||||||||||||||||||||||||||||||||||||||||||
| // SAFETY: `limit` is a valid, writable `rlimit`. | ||||||||||||||||||||||||||||||||||||||||||||||
| let rc = unsafe { libc::getrlimit(libc::RLIMIT_NOFILE, &mut limit) }; | ||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(rc, 0, "getrlimit: {}", std::io::Error::last_os_error()); | ||||||||||||||||||||||||||||||||||||||||||||||
| limit.rlim_cur = limit.rlim_cur.min(64); | ||||||||||||||||||||||||||||||||||||||||||||||
| // SAFETY: `limit` is a valid `rlimit`. | ||||||||||||||||||||||||||||||||||||||||||||||
| let rc = unsafe { libc::setrlimit(libc::RLIMIT_NOFILE, &limit) }; | ||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(rc, 0, "setrlimit: {}", std::io::Error::last_os_error()); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| let mut held = Vec::new(); | ||||||||||||||||||||||||||||||||||||||||||||||
| let err = loop { | ||||||||||||||||||||||||||||||||||||||||||||||
| match File::open("/dev/null") { | ||||||||||||||||||||||||||||||||||||||||||||||
| Ok(f) => held.push(f), | ||||||||||||||||||||||||||||||||||||||||||||||
| Err(e) => break e, | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||||||||||||||||||||||||
| err.raw_os_error(), | ||||||||||||||||||||||||||||||||||||||||||||||
| Some(libc::EMFILE), | ||||||||||||||||||||||||||||||||||||||||||||||
| "expected to run out of file descriptors, got: {err}" | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // Panics if the client cannot be built. | ||||||||||||||||||||||||||||||||||||||||||||||
| let _ = internal_http_client(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // On these platforms, reqwest's default client loads TLS roots from | ||||||||||||||||||||||||||||||||||||||||||||||
| // disk, so it should fail to build. If this starts succeeding, the | ||||||||||||||||||||||||||||||||||||||||||||||
| // test above no longer demonstrates anything about TLS roots. | ||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||
| // This check is skipped on macOS, where building the default client | ||||||||||||||||||||||||||||||||||||||||||||||
| // does no I/O: the platform verifier consults the system trust store | ||||||||||||||||||||||||||||||||||||||||||||||
| // only when verifying a certificate. | ||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(any(target_os = "linux", target_os = "illumos"))] | ||||||||||||||||||||||||||||||||||||||||||||||
| reqwest::ClientBuilder::new().build().expect_err( | ||||||||||||||||||||||||||||||||||||||||||||||
| "expected a default reqwest client to fail to build without \ | ||||||||||||||||||||||||||||||||||||||||||||||
| file descriptors (has reqwest stopped loading TLS roots when \ | ||||||||||||||||||||||||||||||||||||||||||||||
| building a client?)", | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| drop(held); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thetls_certs_only([])parameter".Removing the unwrapping is a good idea, but will come separately.