refactor: API for fetching / using the trust root#69
refactor: API for fetching / using the trust root#69wolfv wants to merge 3 commits intosigstore:mainfrom
Conversation
Signed-off-by: Wolf Vollprecht <w.vollprecht@gmail.com>
There was a problem hiding this comment.
I've not tried this yet myself but the code looks fine to me, thanks for doing all the work.
- The main feature of supporting any TUF url looks good, will be useful immediately in root-signing
- I think it would be good if the default and most obvious offline mechanism was "use TUF material but offline" (instead of, "use embedded material") -- I'm not quite sure if this is the case now (and I know the TUF libraries might not make this as useful as it should be) so I don't think this PR needs to handle all of it
| // Check TUF metadata expiration before serving cached data. | ||
| // The timestamp.json file has the shortest expiration in TUF | ||
| // (typically 1 day) and is the primary freshness indicator. | ||
| match self.check_cache_expiration(&cache_dir).await { | ||
| Ok(()) => return Ok(bytes), | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "Cached TUF metadata has expired ({}), falling back to embedded data", | ||
| e | ||
| ); | ||
| // Fall through to embedded data | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure about this code. Why would the embedded content suddenly be better than cached TUF content if timestamp has expired?
I think there is an argument to be made that TUF cached content (when not verified by TUF) may not be as "trusted" as the embedded data -- but that has nothing to do with timestamp expiry
also comment on docstring: timestamp expiry is not really 1 day: timestamp gets published with 7 day expiry and a new one appears every 24 hours.
| /// For the most up-to-date endpoints, use `from_tuf_config()` with a TUF-fetched config. | ||
| pub fn production() -> Self { | ||
| Self::from_tuf_config(&TufSigningConfig::production().expect("embedded config is valid")) | ||
| Self::from_tuf_config( | ||
| &TufSigningConfig::from_json(SIGSTORE_PRODUCTION_SIGNING_CONFIG) | ||
| .expect("embedded config is valid"), | ||
| ) |
There was a problem hiding this comment.
The method still feels a bit like like foot gun. It will keep working correctly for a long time... until it doesn't.
That said, the docstring is fine and I don't have immediate suggestions for something better that wouldn't have its own problems.
| /// Embedded production trusted root from <https://tuf-repo-cdn.sigstore.dev/> | ||
| /// This is the default trusted root for Sigstore's public production instance. | ||
| pub const SIGSTORE_PRODUCTION_TRUSTED_ROOT: &str = include_str!("trusted_root.json"); | ||
|
|
||
| /// Embedded staging trusted root from <https://tuf-repo-cdn.sigstage.dev/> | ||
| /// This is the trusted root for Sigstore's staging/testing instance. | ||
| pub const SIGSTORE_STAGING_TRUSTED_ROOT: &str = include_str!("trusted_root_staging.json"); |
There was a problem hiding this comment.
could rename these EMBEDDED_PRODUCTION_TRUSTED_ROOT etc to make it clearer that they may not be the current ones... but maybe that's not worth the churn
(to be clear, some work may be needed to make sure that at least a testing CLI exposes custom url / custom root.json but the feature is there now) |
| /// remaining human-readable. URLs are expected to be ASCII. | ||
| fn url_to_dirname(url: &str) -> String { |
There was a problem hiding this comment.
One more comment based on some experience:
- Trying to absolutely canonicalize the URL does not seem critical... but
- Handling the slash at end (making sure
"https://tuf-repo-cdn.sigstore.dev"and"https://tuf-repo-cdn.sigstore.dev/"end up at same local directory) may make some debugging later on a bit faster
2 participants
With this refactor we strongly prefer to use TUF for the sigstore trust root instead of the embedded one as we did before.