Skip to content

upki: add support for system-wide configuration/caching #106

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.

Sign up for GitHub

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

Closed
djc wants to merge 3 commits into from
Closed

upki: add support for system-wide configuration/caching #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
496dd44
upki: inline user dir helper functions
djc Apr 17, 2026
7e7c945
upki: introduce new PathKind enum
djc Apr 17, 2026
8741184
upki: take PathKind for Config construction
djc Apr 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions revoke-test/benches/bench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,20 @@ use criterion::{Criterion, criterion_group, criterion_main};
use revoke_test::RevocationTestSites;
use rustls_pki_types::CertificateDer;
use upki::revocation::{Index, Manifest, RevocationCheckInput, RevocationStatus};
use upki::{Config, ConfigPath};
use upki::{Config, ConfigPath, PathKind};

fn revocation(c: &mut Criterion) {
c.bench_function("load-config", |b| {
b.iter(|| {
Config::from_file_or_user_default(&ConfigPath::Specified(PathBuf::from(
Config::from_file_or_default(&ConfigPath::Specified(PathBuf::from(
"benches/data/config.toml",
)))
.unwrap()
})
});

c.bench_function("load-manifest", |b| {
let config = Config::from_file_or_user_default(&ConfigPath::Specified(PathBuf::from(
let config = Config::from_file_or_default(&ConfigPath::Specified(PathBuf::from(
"benches/data/config.toml",
)))
.unwrap();
Expand All @@ -43,7 +43,8 @@ fn revocation(c: &mut Criterion) {
});

c.bench_function("revocation-check", |b| {
let config = Config::from_file_or_user_default(&ConfigPath::new(None).unwrap()).unwrap();
let config =
Config::from_file_or_default(&ConfigPath::new(None, PathKind::User).unwrap()).unwrap();
let revoked_certs = certificates_for_test_site(BENCHMARK_CASE);

b.iter(|| {
Expand Down
6 changes: 3 additions & 3 deletions rustls-upki/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use upki::revocation::{
CertSerial, CtTimestamp, Index, IssuerSpkiHash, Manifest, RevocationCheckInput,
RevocationStatus,
};
use upki::{self, Config, ConfigPath};
use upki::{self, Config, ConfigPath, PathKind};
use webpki::{EndEntityCert, ExtendedKeyUsage, InvalidNameContext, VerifiedPath};

/// A [`ServerCertVerifier`] that uses upki to check revocation status of server certificates.
Expand Down Expand Up @@ -64,8 +64,8 @@ impl ServerVerifier {
})
.expect("no cipher suites supported with SHA256");

let config = ConfigPath::new(config_path)
.and_then(|path| Config::from_file_or_user_default(&path))
let config = ConfigPath::new(config_path, PathKind::User)
.and_then(|path| Config::from_file_or_default(&path))
.map_err(|report| rustls::Error::General(report.to_string()))?;

// Pre-roll storage to check it works, and bring (eg. permanent configuration) errors
Expand Down
8 changes: 4 additions & 4 deletions upki-cli/src/bin/upki.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use eyre::{Context, Report};
use rustls_pki_types::CertificateDer;
use rustls_pki_types::pem::PemObject;
use upki::revocation::{Index, Manifest, RevocationCheckInput, fetch};
use upki::{Config, ConfigPath};
use upki::{Config, ConfigPath, PathKind};

#[tokio::main(flavor = "current_thread")]
async fn main() -> Result<ExitCode, Report> {
Expand All @@ -22,15 +22,15 @@ async fn main() -> Result<ExitCode, Report> {
.init();
}

let config_path =
ConfigPath::new(args.config_file).wrap_err("cannot find configuration path")?;
let config_path = ConfigPath::new(args.config_file, PathKind::User)
Copy link
Copy Markdown
Member

@ctz ctz Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is the right level for this declaration? Because if args.config_file is none, then PathKind::User implies the default should ignore the system-level one?

Copy link
Copy Markdown
Member Author

@djc djc Apr 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the current behavior, too, right? This PR intentionally punts on heuristics about when we should try what paths in what order (because it is a little bit tricky), in favor of at least encoding in the library the ability to have system-level config and cache.

Copy link
Copy Markdown
Member

@ctz ctz Apr 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, yes, but the desired and previous behaviour was to find a user-controlled config file (if one exists at a well-known location) before falling back to a system-wide config file. And that should happen without each call site having to do that work individually, else we end with some upki functions ignoring one location or the other.

Copy link
Copy Markdown
Member Author

@djc djc Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. But then I think that means we have to reintroduce the for_read vs for_write distinction, right? I think it's clear that check() should first try the user config file and then the system-wide config file, but it's not obvious to me what the least surprising behavior for fetch() is. Maybe it would be better for fetch() config to require a specific path or level?

Copy link
Copy Markdown
Member

@ctz ctz Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if someone doing fetch() as a normal user, using a system-level config, should pretty quickly fail with a clear permission error. I think that's fine. eg, this is what apt-get update does in that situation:

$ apt-get update
Reading package lists... Done
E: Could not open lock file /var/lib/apt/lists/lock - open (13: Permission denied)
E: Unable to lock directory /var/lib/apt/lists/
W: Problem unlinking the file /var/cache/apt/pkgcache.bin - RemoveCaches (13: Permission denied)
W: Problem unlinking the file /var/cache/apt/srcpkgcache.bin - RemoveCaches (13: Permission denied)

.wrap_err("cannot find configuration path")?;

if let Command::ShowConfigPath = args.command {
println!("{}", config_path.as_ref().display());
return Ok(ExitCode::SUCCESS);
}

let config = Config::from_file_or_user_default(&config_path)?;
let config = Config::from_file_or_default(&config_path)?;

Ok(match args.command {
Command::Fetch { dry_run } => fetch(dry_run, &config).await?,
Expand Down
4 changes: 2 additions & 2 deletions upki-ffi/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::slice;

use rustls_pki_types::CertificateDer;
use upki::revocation::{self, Index, RevocationCheckInput, RevocationStatus};
use upki::{Config, Error};
use upki::{Config, Error, PathKind};

/// Check the revocation status of a certificate.
///
Expand Down Expand Up @@ -119,7 +119,7 @@ pub unsafe extern "C" fn upki_config_new_user(out: *mut *mut upki_config) -> upk
return upki_result::UPKI_ERR_NULL_POINTER;
}

match Config::try_user_default() {
match Config::try_default(PathKind::User) {
Ok(config) => {
unsafe { *out = Box::into_raw(Box::new(upki_config(config))) };
upki_result::UPKI_OK
Expand Down
49 changes: 30 additions & 19 deletions upki/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ impl Config {
/// Load the configuration data from a file at `path`.
///
/// If no file exists at `path`, a default configuration is returned.
pub fn from_file_or_user_default(path: &ConfigPath) -> Result<Self, Error> {
pub fn from_file_or_default(path: &ConfigPath) -> Result<Self, Error> {
match path {
ConfigPath::Default(path) if path.exists() => Self::from_file(path),
ConfigPath::Default(_) => Self::try_user_default(),
ConfigPath::Default(path, _) if path.exists() => Self::from_file(path),
ConfigPath::Default(_, kind) => Self::try_default(*kind),
ConfigPath::Specified(path) => Self::from_file(path),
}
}
Expand All @@ -49,9 +49,9 @@ impl Config {
}

/// Return a sensible default configuration.
pub fn try_user_default() -> Result<Self, Error> {
pub fn try_default(kind: PathKind) -> Result<Self, Error> {
Ok(Self {
cache_dir: user_cache_dir()?,
cache_dir: kind.cache_dir()?,
revocation: RevocationConfig::default(),
})
}
Expand All @@ -66,7 +66,7 @@ pub enum ConfigPath {
/// The path was directly specified by a user.
Specified(PathBuf),
/// The path was determined automatically.
Default(PathBuf),
Default(PathBuf, PathKind),
}

impl ConfigPath {
Expand All @@ -81,10 +81,10 @@ impl ConfigPath {
///
/// This function fails for platform-specific reasons, typically if `$HOME` is not
/// set, or another XDG environment variable is malformed.
pub fn new(specified: Option<PathBuf>) -> Result<Self, Error> {
pub fn new(specified: Option<PathBuf>, kind: PathKind) -> Result<Self, Error> {
match specified {
Some(f) => Ok(Self::Specified(f)),
None => user_config_file().map(ConfigPath::Default),
None => Ok(Self::Default(kind.config_dir()?.join(CONFIG_FILE), kind)),
}
}
}
Expand All @@ -93,11 +93,32 @@ impl AsRef<Path> for ConfigPath {
fn as_ref(&self) -> &Path {
match self {
Self::Specified(path) => path.as_ref(),
Self::Default(path) => path.as_ref(),
Self::Default(path, _) => path.as_ref(),
}
}
}

/// What kind of path is being determined.
#[derive(Clone, Copy, Debug)]
pub enum PathKind {
Copy link
Copy Markdown
Member Author

@djc djc Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the name... Scope, ConfigSource, ..?

/// User-relative configuration and data.
User,
}

impl PathKind {
fn config_dir(self) -> Result<PathBuf, Error> {
Ok(match self {
Self::User => project_dirs()?.config_dir().to_owned(),
})
}

fn cache_dir(self) -> Result<PathBuf, Error> {
Ok(match self {
Self::User => project_dirs()?.cache_dir().to_owned(),
})
}
}

/// Errors for the upki library API.
#[non_exhaustive]
#[derive(Debug)]
Expand Down Expand Up @@ -156,16 +177,6 @@ impl fmt::Display for Error {
}
}

fn user_config_file() -> Result<PathBuf, Error> {
Ok(project_dirs()?
.config_dir()
.join(CONFIG_FILE))
}

fn user_cache_dir() -> Result<PathBuf, Error> {
Ok(project_dirs()?.cache_dir().to_owned())
}

fn project_dirs() -> Result<ProjectDirs, Error> {
ProjectDirs::from("dev", "rustls", PREFIX).ok_or(Error::NoValidHomeDirectory)
}
Expand Down