-
Notifications
You must be signed in to change notification settings - Fork 127
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
Allows configuring TLS backend #386
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,9 +19,10 @@ pub static TLDR_PAGES_DIR: &str = "tldr-pages"; | |
static TLDR_OLD_PAGES_DIR: &str = "tldr-master"; | ||
|
||
#[derive(Debug)] | ||
pub struct Cache { | ||
pub struct Cache<'a> { | ||
cache_dir: PathBuf, | ||
enable_styles: bool, | ||
tls_backend: &'a str, // for setting up reqwest client | ||
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 this should use an enum like so: enum TlsBackend {
// ...
} This way we also avoid the lifetime parameter :) 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. Of course! |
||
} | ||
|
||
#[derive(Debug)] | ||
|
@@ -85,14 +86,15 @@ pub enum CacheFreshness { | |
Missing, | ||
} | ||
|
||
impl Cache { | ||
pub fn new<P>(cache_dir: P, enable_styles: bool) -> Self | ||
impl<'a> Cache<'a> { | ||
pub fn new<P>(cache_dir: P, enable_styles: bool, tls_backend: &'a str) -> Self | ||
where | ||
P: Into<PathBuf>, | ||
{ | ||
Self { | ||
cache_dir: cache_dir.into(), | ||
enable_styles, | ||
tls_backend, | ||
} | ||
} | ||
|
||
|
@@ -135,9 +137,17 @@ impl Cache { | |
self.cache_dir.join(TLDR_PAGES_DIR) | ||
} | ||
|
||
/// Download the archive from the specified URL. | ||
fn download(archive_url: &str) -> Result<Vec<u8>> { | ||
/// Builds HTTPS client based on configuration. | ||
/// | ||
/// Note that `Cargo.toml` also defines default feature . | ||
fn build_client(&self) -> Result<Client> { | ||
let mut builder = Client::builder(); | ||
builder = match self.tls_backend { | ||
"native-roots" => builder.use_rustls_tls().tls_built_in_native_certs(true), | ||
"webpki-roots" => builder.use_rustls_tls().tls_built_in_webpki_certs(true), | ||
"native-tls" => builder.use_native_tls().tls_built_in_native_certs(true), | ||
_ => builder, // TLS backend defaults to the compiled feature | ||
}; | ||
if let Ok(ref host) = env::var("HTTP_PROXY") { | ||
if let Ok(proxy) = Proxy::http(host) { | ||
builder = builder.proxy(proxy); | ||
|
@@ -148,9 +158,11 @@ impl Cache { | |
builder = builder.proxy(proxy); | ||
} | ||
} | ||
let client = builder | ||
.build() | ||
.context("Could not instantiate HTTP client")?; | ||
builder.build().context("Could not instantiate HTTP client") | ||
} | ||
|
||
/// Download the archive from the specified URL. | ||
fn download(client: &Client, archive_url: &str) -> Result<Vec<u8>> { | ||
let mut resp = client | ||
.get(archive_url) | ||
.send()? | ||
|
@@ -166,8 +178,9 @@ impl Cache { | |
pub fn update(&self, archive_url: &str) -> Result<()> { | ||
self.ensure_cache_dir_exists()?; | ||
|
||
let client = self.build_client()?; | ||
// First, download the compressed data | ||
let bytes: Vec<u8> = Self::download(archive_url)?; | ||
let bytes: Vec<u8> = Self::download(&client, archive_url)?; | ||
|
||
// Decompress the response body into an `Archive` | ||
let mut archive = ZipArchive::new(Cursor::new(bytes)) | ||
|
@@ -176,7 +189,7 @@ impl Cache { | |
// Clear cache directory | ||
// Note: This is not the best solution. Ideally we would download the | ||
// archive to a temporary directory and then swap the two directories. | ||
// But renaming a directory doesn't work across filesystems and Rust | ||
// But renaming a directory doesn't work across file systems and Rust | ||
// does not yet offer a recursive directory copying function. So for | ||
// now, we'll use this approach. | ||
self.clear() | ||
|
@@ -504,4 +517,30 @@ mod tests { | |
|
||
assert_eq!(&buf, b"Hello\n"); | ||
} | ||
|
||
macro_rules! https_client_tests { | ||
($($name:ident: $backend:expr),* $(,)?) => { | ||
$( | ||
#[test] | ||
fn $name() { | ||
let dir = tempfile::tempdir().unwrap(); | ||
|
||
let _ = Cache::build_client(&Cache { | ||
cache_dir: dir.into_path(), | ||
enable_styles: false, | ||
tls_backend: $backend, | ||
}); | ||
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. We are not checking whether the returned Additionally, I think we can make 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. True. Done. |
||
|
||
// intentionally empty, assumes we have built the client. | ||
} | ||
)* | ||
} | ||
} | ||
|
||
https_client_tests! { | ||
tests_https_client_with_native_roots: "native-roots", | ||
tests_https_client_with_wekpki_roots: "wekpki-roots", | ||
tests_https_client_with_native_tls: "native-tls", | ||
tests_https_client_with_default_backend: "default", | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,10 @@ use crate::types::PathSource; | |
pub const CONFIG_FILE_NAME: &str = "config.toml"; | ||
pub const MAX_CACHE_AGE: Duration = Duration::from_secs(2_592_000); // 30 days | ||
const DEFAULT_UPDATE_INTERVAL_HOURS: u64 = MAX_CACHE_AGE.as_secs() / 3600; // 30 days | ||
// Chooses a default TLS backend. | ||
// `default` will choose a backend based on the compiled default TLS backend feature. | ||
// Read more in `Cargo.toml'. | ||
const DEFAULT_TLS_BACKEND: &str = "default"; | ||
|
||
fn default_underline() -> bool { | ||
false | ||
|
@@ -166,19 +170,26 @@ const fn default_auto_update_interval_hours() -> u64 { | |
DEFAULT_UPDATE_INTERVAL_HOURS | ||
} | ||
|
||
fn default_tls_backend() -> String { | ||
DEFAULT_TLS_BACKEND.to_string() | ||
} | ||
|
||
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] | ||
struct RawUpdatesConfig { | ||
#[serde(default)] | ||
pub auto_update: bool, | ||
#[serde(default = "default_auto_update_interval_hours")] | ||
pub auto_update_interval_hours: u64, | ||
#[serde(default = "default_tls_backend")] | ||
pub tls_backend: String, | ||
} | ||
|
||
impl Default for RawUpdatesConfig { | ||
fn default() -> Self { | ||
Self { | ||
auto_update: false, | ||
auto_update_interval_hours: DEFAULT_UPDATE_INTERVAL_HOURS, | ||
tls_backend: DEFAULT_TLS_BACKEND.to_string(), | ||
} | ||
} | ||
} | ||
|
@@ -190,6 +201,7 @@ impl From<RawUpdatesConfig> for UpdatesConfig { | |
auto_update_interval: Duration::from_secs( | ||
raw_updates_config.auto_update_interval_hours * 3600, | ||
), | ||
tls_backend: DEFAULT_TLS_BACKEND.to_string(), | ||
} | ||
} | ||
} | ||
|
@@ -252,10 +264,16 @@ pub struct DisplayConfig { | |
pub use_pager: bool, | ||
} | ||
|
||
#[derive(Copy, Clone, Debug, PartialEq, Eq)] | ||
#[derive(Clone, Debug, PartialEq, Eq)] | ||
pub struct UpdatesConfig { | ||
pub auto_update: bool, | ||
pub auto_update_interval: Duration, | ||
/// Allows choosing a TLS backend supported by `reqwest`. Available TLS backends: | ||
/// # - `native-roots`: Rustls with native roots | ||
/// # - `webpki-roots`: Rustls with `WebPK` roots | ||
/// # - `native-tls`: Native TLS (`SChannel` on Windows, Secure Transport on macOS and OpenSSL otherwise) | ||
/// Read more in `Cargo.toml` | ||
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 documentation can then probably move to the definition of the |
||
pub tls_backend: String, | ||
} | ||
|
||
#[derive(Clone, Debug, PartialEq, Eq)] | ||
|
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.
I had something else in mind: By disabling the
native-roots
(etc.) features of tealdeer, we would also disable the features for reqwest here, so that the resulting tealdeer binary is as small as possible.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.
I see. Fixed. I saw the reqwest 0.12.9 supports additional roots. So I also extended it. What do you think about it?