Skip to content

Support fetching intermediates - #103

Open
ctz wants to merge 9 commits into
mainfrom
jbp-intermediate-fetch
Open

Support fetching intermediates#103
ctz wants to merge 9 commits into
mainfrom
jbp-intermediate-fetch

Conversation

@ctz

@ctz ctz commented Apr 15, 2026

Copy link
Copy Markdown
Member

This is currently behind a config flag. So existing uses won't change.

@ctz
ctz force-pushed the jbp-intermediate-fetch branch from 72868f8 to d299fac Compare April 15, 2026 19:04
@codspeed-hq

codspeed-hq Bot commented Apr 15, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 4 untouched benchmarks


Comparing jbp-intermediate-fetch (816722c) with main (c503a2e)

Open in CodSpeed

@ctz
ctz force-pushed the jbp-intermediate-fetch branch from d299fac to 43acafd Compare April 15, 2026 19:12
@ctz
ctz force-pushed the jbp-intermediate-fetch branch from 43acafd to c9a5dc8 Compare July 13, 2026 14:53
@ctz

ctz commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

(periodic rebase, nothing to see here)

@ctz
ctz force-pushed the jbp-intermediate-fetch branch from c9a5dc8 to e116a42 Compare August 5, 2026 09:03
@ctz
ctz force-pushed the jbp-intermediate-fetch branch from e116a42 to f9070ba Compare August 5, 2026 18:44
@ctz

ctz commented Aug 5, 2026

Copy link
Copy Markdown
Member Author
  • openssl symlink farming for intermediates

not doing this as there doesn't appear to be a way to give openssl an X509_LOOKUP for untrusted certs

@ctz
ctz force-pushed the jbp-intermediate-fetch branch from f9070ba to 816722c Compare August 5, 2026 20:06
@ctz
ctz marked this pull request as ready for review August 5, 2026 20:06

@djc djc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

Comment thread upki/src/revocation/fetch.rs Outdated
remote_url: &str,
local: &Path,
) -> Result<Self, Error> {
pub(crate) fn construct(manifest: &Manifest, ctx: &FetchContext<'_>) -> Result<Self, Error> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: I think the idiomatic abbrevation for "context" in Rust is cx.

Comment on lines +132 to +138
fn should_clean_up_file_name(&self, name: &str) -> bool {
match self.typ {
FetchType::Revocation => name.ends_with(".filter") || name.ends_with(".delta"),
}
}

fn requires_revocation_index(&self) -> bool {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Feels like these could/should be methods on FetchType?

Also I'm inclined to say that this could/should be a trait instead, so that we can decentralize this setup (that is, collect revocation parameters for fetching in revocation and other stuff elsewhere), rather than centralizing it in fetch.

Comment thread upki/src/data.rs
use serde::{Deserialize, Serialize};
use tracing::info;

use crate::revocation::Error;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: this seems a little wrong?

pub files: Vec<ManifestFile>,
}
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Manifest(data::Manifest);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure this type still makes sense? The remainder of from_config() seems like it could be generic to data::Manifest and verify() could be a free function?

Comment thread upki/src/lib.rs
pub mod revocation;

/// Common data storage formats.
pub mod data;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the revocation mod declaration and the import from it should be kept together.

fetch-url = ""

[intermediates]
enabled = false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of enabled, I think we should make intermediates (and revocation) Option in the top-level Config?

Comment on lines +476 to +479
config_write: impl FnOnce(&TempDir, &str),
) -> (TempDir, PathBuf, SettingsBindDropGuard) {
let temp = TempDir::new().unwrap();
write_config(&temp, fetch_url);
config_write(&temp, fetch_url);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The difference between config_write and write_config seems too subtle to be both useful and not confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants