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 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 Jun 21, 2024
1 parent 6d1d0b3 commit 589ea78
Show file tree
Hide file tree
Showing 11 changed files with 140 additions and 41 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
42 changes: 22 additions & 20 deletions docs/reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,27 @@ Arguments:
values: major, minor, patch, release, rc, beta, alpha]

Options:
--manifest-path <PATH> Path to Cargo.toml
-p, --package <SPEC> Package to process (see `cargo help pkgid`)
--workspace Process all packages in the workspace
--exclude <SPEC> Exclude packages from being processed
--unpublished Process all packages whose current version is unpublished
-m, --metadata <METADATA> Semver metadata
-x, --execute Actually perform a release. Dry-run mode is the default
--no-confirm Skip release confirmation and version preview
--prev-tag-name <NAME> The name of tag for the previous release
-c, --config <PATH> Custom config file
--isolated Ignore implicit configuration files
--sign Sign both git commit and tag
--dependent-version <ACTION> Specify how workspace dependencies on this crate should be
handed [possible values: upgrade, fix]
--allow-branch <GLOB[,...]> Comma-separated globs of branch names a release can happen from
-q, --quiet... Pass many times for less log output
-v, --verbose... Pass many times for more log output
-h, --help Print help (see more with '--help')
-V, --version Print version
--manifest-path <PATH> Path to Cargo.toml
-p, --package <SPEC> Package to process (see `cargo help pkgid`)
--workspace Process all packages in the workspace
--exclude <SPEC> Exclude packages from being processed
--unpublished Process all packages whose current version is unpublished
-m, --metadata <METADATA> Semver metadata
-x, --execute Actually perform a release. Dry-run mode is the default
--no-confirm Skip release confirmation and version preview
--prev-tag-name <NAME> The name of tag for the previous release
-c, --config <PATH> Custom config file
--isolated Ignore implicit configuration files
--sign Sign both git commit and tag
--dependent-version <ACTION> Specify how workspace dependencies on this crate should be
handed [possible values: upgrade, fix]
--allow-branch <GLOB[,...]> Comma-separated globs of branch names a release can happen from
--certs-source <CERTS_SOURCE> Indicate what certificate store to use for web requests
[possible values: webpki, native]
-q, --quiet... Pass many times for less log output
-v, --verbose... Pass many times for more log output
-h, --help Print help (see more with '--help')
-V, --version Print version

Commit:
--sign-commit Sign git commit
Expand Down Expand Up @@ -140,7 +142,7 @@ Workspace configuration is read from the following (in precedence order)
| `metadata` | \- | `optional`, `required`, `ignore`, `persistent` | `optional` | Policy for presence of absence of `--metadata` flag when changing the version |
| `rate-limit.new-packages` | \- | integer | `5` | `optional` | Rate limit for publishing new packages |
| `rate-limit.existing-packages` | \- | integer | `30` | `optional` | Rate limit for publishing existing packages |

| `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
27 changes: 26 additions & 1 deletion src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub struct Config {
pub metadata: Option<MetadataPolicy>,
pub target: Option<String>,
pub rate_limit: RateLimit,
pub certs_source: Option<CertsSource>,
}

impl Config {
Expand Down Expand Up @@ -87,6 +88,7 @@ impl Config {
metadata: Some(empty.metadata()),
target: None,
rate_limit: RateLimit::from_defaults(),
certs_source: Some(empty.certs_source()),
}
}

Expand Down Expand Up @@ -166,8 +168,10 @@ impl Config {
if let Some(target) = source.target.as_deref() {
self.target = Some(target.to_owned());
}

self.rate_limit.update(&source.rate_limit);
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 @@ -303,6 +307,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 @@ -346,6 +354,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 certs from Mozilla's root certificate store.
#[default]
Webpki,
/// Use 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 @@ -590,6 +610,10 @@ pub struct ConfigArgs {
#[arg(long, value_delimiter = ',', value_name = "GLOB[,...]")]
pub allow_branch: Option<Vec<String>>,

/// Indicate what certificate store to use for web requests.
#[arg(long)]
pub certs_source: Option<CertsSource>,

#[command(flatten)]
pub commit: CommitArgs,

Expand All @@ -610,6 +634,7 @@ impl ConfigArgs {
sign_commit: self.sign(),
sign_tag: self.sign(),
dependent_version: self.dependent_version,
certs_source: self.certs_source,
..Default::default()
};
config.update(&self.commit.to_config());
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 @@ -229,7 +229,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 @@ -215,6 +216,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 589ea78

Please sign in to comment.