Skip to content

Commit

Permalink
feat: Enable selecting system certificate store
Browse files Browse the repository at this point in the history
This commit adds the ability to select what certificate root store to
use when making HTTPS connections. Previously, `cargo-release` was
configured to use the default for `tame-index`, which is the "webpki"
root of trust maintained by Mozilla. This is a good and reasonable set
of certificates, _except_ for users on networks which substitute
certificates.

Substituting certificates is something enterprise networks will
frequently do so they can man-in-the-middle HTTPS connections made on
their network and thus maintain visibility into the network activities
of their employees. In this setup, the users' devices will generally be
running enterprise-managed software which replaces certificates used by
public websites with ones provided by the network software the
enterprise uses, with the root certificates for these substituted chains
being placed in the users' local system certificate store. In that case,
with only the "webpki" certificate store loaded for `cargo-release`, the
substituted certificates will fail to validate, and publication of new
versions (indeed, even checking publication status of the crate
attempting to be published) will fail with an HTTPS error about an
untrusted certificate.

The solution chosen here was to add a configuration element, and a CLI
flag under the "publish" section, which lets the user pick between
"webpki" (Mozilla) or "native" (local system) certificate trust stores.
The portion of the code in `src/ops/index.rs` which handles connecting
to the registry index has been updated to configure which certificate
store to use based on the user's selection.

The one final wrinkle is that we get `reqwest`, the dependency which
actually handles HTTPS connections, through the `tame-index` crate,
which re-exports it. To enable the APIs in `reqwest` for configuring
what TLS certs to pick up, we have to enable the "native-certs" feature
on `tame-index`, while leaving the default features on. This is
something `tame-index` normally recommends _against_, because it assumes
you want to exclusively activate one of them at compile time. In our
case, we need the selection to happen at runtime, so we need both to be
compiled in.

Signed-off-by: Andrew Lilley Brinker <[email protected]>
  • Loading branch information
alilleybrinker committed May 13, 2024
1 parent 4b09269 commit 17ec2c5
Show file tree
Hide file tree
Showing 11 changed files with 127 additions and 27 deletions.
52 changes: 45 additions & 7 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,12 @@ vendored-openssl = ["git2/vendored-openssl"]

[dependencies]
cargo_metadata = "0.18"
tame-index = { version = "0.11", features = ["sparse"] }
# Use this with default-features set to "true" (implicitly) so that reqwest,
# a transitive dependency, is compiled with support for both webpki
# certificates AND native certificates. We want support for both to be
# present, and then to let the user _select_ through configuration which
# one they want to be used.
tame-index = { version = "0.11", features = ["sparse", "native-certs"] }
git2 = { version = "0.18.3", default-features = false }
toml_edit = { version = "0.22.12", features = ["serde"] }
toml = "0.8.12"
Expand Down
16 changes: 9 additions & 7 deletions docs/reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,14 @@ Commit:
--sign-commit Sign git commit

Publish:
--no-publish Do not run cargo publish on release
--registry <NAME> Cargo registry to upload to
--no-verify Don't verify the contents by building them
--features <FEATURES> Provide a set of features that need to be enabled
--all-features Enable all features via `all-features`. Overrides `features`
--target <TRIPLE> Build for the target triple
--no-publish Do not run cargo publish on release
--registry <NAME> Cargo registry to upload to
--no-verify Don't verify the contents by building them
--features <FEATURES> Provide a set of features that need to be enabled
--all-features Enable all features via `all-features`. Overrides `features`
--target <TRIPLE> Build for the target triple
--certs-source <CERTS> Select what certificates to use (default: 'webpki') [possible values:
webpki, native]

Tag:
--no-tag Do not create git tag
Expand Down Expand Up @@ -138,7 +140,7 @@ Workspace configuration is read from the following (in precedence order)
| `target` | \- | string | \- | Target triple to use for the verification build |
| `dependent-version` | \- | `upgrade`, `fix`, `error`, `warn`, `ignore` | `upgrade` | Policy for upgrading path dependency versions within the workspace |
| `metadata` | \- | `optional`, `required`, `ignore`, `persistent` | `optional` | Policy for presence of absence of `--metadata` flag when changing the version |

| `certs-source` | \- | `webpki`, `native` | `webpki` | Policy for using Mozilla's standard certificate root of trust (`webpki`) or using the system certificate root of trust (`native`) |

Note: fields are from the package-configuration unless otherwise specified.

Expand Down
26 changes: 26 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub struct Config {
pub dependent_version: Option<DependentVersion>,
pub metadata: Option<MetadataPolicy>,
pub target: Option<String>,
pub certs_source: Option<CertsSource>,
}

impl Config {
Expand Down Expand Up @@ -85,6 +86,7 @@ impl Config {
dependent_version: Some(empty.dependent_version()),
metadata: Some(empty.metadata()),
target: None,
certs_source: None,
}
}

Expand Down Expand Up @@ -164,6 +166,9 @@ impl Config {
if let Some(target) = source.target.as_deref() {
self.target = Some(target.to_owned());
}
if let Some(certs) = source.certs_source {
self.certs_source = Some(certs);
}
}

pub fn allow_branch(&self) -> impl Iterator<Item = &str> {
Expand Down Expand Up @@ -299,6 +304,10 @@ impl Config {
pub fn metadata(&self) -> MetadataPolicy {
self.metadata.unwrap_or_default()
}

pub fn certs_source(&self) -> CertsSource {
self.certs_source.unwrap_or_default()
}
}

#[derive(Debug, Clone, Serialize, Deserialize)]
Expand Down Expand Up @@ -342,6 +351,18 @@ pub enum DependentVersion {
Fix,
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, clap::ValueEnum)]
#[serde(rename_all = "kebab-case")]
#[value(rename_all = "kebab-case")]
#[derive(Default)]
pub enum CertsSource {
/// Use "webpki" certs from Mozilla's root certificate store.
#[default]
Webpki,
/// Use "native" certs from the system root certificate store.
Native,
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, clap::ValueEnum)]
#[serde(rename_all = "kebab-case")]
#[value(rename_all = "kebab-case")]
Expand Down Expand Up @@ -630,6 +651,10 @@ pub struct PublishArgs {
/// Build for the target triple
#[arg(long, value_name = "TRIPLE")]
target: Option<String>,

/// Select what certificates to use (default: 'webpki')
#[arg(long, value_name = "CERTS")]
certs_source: Option<CertsSource>,
}

impl PublishArgs {
Expand All @@ -641,6 +666,7 @@ impl PublishArgs {
enable_features: (!self.features.is_empty()).then(|| self.features.clone()),
enable_all_features: self.all_features.then_some(true),
target: self.target.clone(),
certs_source: self.certs_source,
..Default::default()
}
}
Expand Down
8 changes: 5 additions & 3 deletions src/ops/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::path::Path;

use bstr::ByteSlice;

use crate::config;
use crate::config::{self, CertsSource};
use crate::error::CargoResult;
use crate::ops::cmd::call;

Expand Down Expand Up @@ -115,6 +115,7 @@ pub fn wait_for_publish(
version: &str,
timeout: std::time::Duration,
dry_run: bool,
certs_source: CertsSource,
) -> CargoResult<()> {
if !dry_run {
if registry.is_some() {
Expand All @@ -128,7 +129,7 @@ pub fn wait_for_publish(
let mut logged = false;
loop {
index.update_krate(registry, name);
if is_published(index, registry, name, version) {
if is_published(index, registry, name, version, certs_source) {
break;
} else if timeout < now.elapsed() {
anyhow::bail!("timeout waiting for crate to be published");
Expand All @@ -153,8 +154,9 @@ pub fn is_published(
registry: Option<&str>,
name: &str,
version: &str,
certs_source: CertsSource,
) -> bool {
match index.has_krate_version(registry, name, version) {
match index.has_krate_version(registry, name, version, certs_source) {
Ok(has_krate_version) => has_krate_version.unwrap_or(false),
Err(err) => {
// For both http and git indices, this _might_ be an error that goes away in
Expand Down
30 changes: 23 additions & 7 deletions src/ops/index.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::config::CertsSource;
use tame_index::krate::IndexKrate;
use tame_index::utils::flock::FileLock;

Expand All @@ -22,8 +23,12 @@ impl CratesIoIndex {
&mut self,
registry: Option<&str>,
name: &str,
certs_source: CertsSource,
) -> Result<bool, crate::error::CliError> {
Ok(self.krate(registry, name)?.map(|_| true).unwrap_or(false))
Ok(self
.krate(registry, name, certs_source)?
.map(|_| true)
.unwrap_or(false))
}

/// Determines if the specified crate version exists in the crates.io index
Expand All @@ -33,8 +38,9 @@ impl CratesIoIndex {
registry: Option<&str>,
name: &str,
version: &str,
certs_source: CertsSource,
) -> Result<Option<bool>, crate::error::CliError> {
let krate = self.krate(registry, name)?;
let krate = self.krate(registry, name, certs_source)?;
Ok(krate.map(|ik| ik.versions.iter().any(|iv| iv.version == version)))
}

Expand All @@ -51,6 +57,7 @@ impl CratesIoIndex {
&mut self,
registry: Option<&str>,
name: &str,
certs_source: CertsSource,
) -> Result<Option<IndexKrate>, crate::error::CliError> {
if let Some(registry) = registry {
log::trace!("Cannot connect to registry `{registry}`");
Expand All @@ -64,7 +71,7 @@ impl CratesIoIndex {

if self.index.is_none() {
log::trace!("Connecting to index");
self.index = Some(RemoteIndex::open()?);
self.index = Some(RemoteIndex::open(certs_source)?);
}
let index = self.index.as_mut().unwrap();
log::trace!("Downloading index for {name}");
Expand All @@ -83,13 +90,22 @@ pub struct RemoteIndex {

impl RemoteIndex {
#[inline]
pub fn open() -> Result<Self, crate::error::CliError> {
pub fn open(certs_source: CertsSource) -> Result<Self, crate::error::CliError> {
let index = tame_index::SparseIndex::new(tame_index::IndexLocation::new(
tame_index::IndexUrl::CratesIoSparse,
))?;
let client = tame_index::external::reqwest::blocking::ClientBuilder::new()
.http2_prior_knowledge()
.build()?;

let client = {
let builder = tame_index::external::reqwest::blocking::ClientBuilder::new();

let builder = match certs_source {
CertsSource::Webpki => builder.tls_built_in_webpki_certs(true),
CertsSource::Native => builder.tls_built_in_native_certs(true),
};

builder.build()?
};

let lock = FileLock::unlocked();

Ok(Self {
Expand Down
1 change: 1 addition & 0 deletions src/steps/hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ impl HookStep {
pkg.config.registry(),
crate_name,
&version.full_version_string,
pkg.config.certs_source(),
) {
log::debug!(
"enabled {}, v{} is unpublished",
Expand Down
2 changes: 1 addition & 1 deletion src/steps/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ pub fn verify_rate_limit(
// Note: these rate limits are only known for default registry
if pkg.config.registry().is_none() && pkg.config.publish() {
let crate_name = pkg.meta.name.as_str();
if index.has_krate(None, crate_name)? {
if index.has_krate(None, crate_name, pkg.config.certs_source())? {
existing += 1;
} else {
new += 1;
Expand Down
2 changes: 2 additions & 0 deletions src/steps/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ impl PublishStep {
pkg.config.registry(),
crate_name,
&version.full_version_string,
pkg.config.certs_source(),
) {
let _ = crate::ops::shell::warn(format!(
"disabled due to previous publish ({}), skipping {}",
Expand Down Expand Up @@ -210,6 +211,7 @@ pub fn publish(
&version.full_version_string,
timeout,
dry_run,
pkg.config.certs_source(),
)?;
// HACK: Even once the index is updated, there seems to be another step before the publish is fully ready.
// We don't have a way yet to check for that, so waiting for now in hopes everything is ready
Expand Down
Loading

0 comments on commit 17ec2c5

Please sign in to comment.