Skip to content
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.

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

Cleaning up API surface before releasing 0.3.0 #355

Open
8 tasks
erickt opened this issue Feb 4, 2022 · 1 comment
Open
8 tasks

Cleaning up API surface before releasing 0.3.0 #355

erickt opened this issue Feb 4, 2022 · 1 comment

Comments

@erickt
Copy link
Collaborator

erickt commented Feb 4, 2022

We're getting close to releasing 0.3.0. Are there any other papercuts on the API surface we want to clean up before release? Here are some ideas:

  • MetadataPath and TargetPath own a String, but are mainly passed around as a reference. Should we instead replicate std::path::Path and use a dynamically sized type? Downside of this is that we'd have to use unsafe in order to construct these values.
  • Should we create client::ClientBuilder and move the client::Config into it? Then people who just want the default wouldn't have to deal with passing around Config::default().
  • The TUF spec refers to different formats as "POUFs". Should we rename tuf::interchange to tuf::pouf, and tuf::interchange::Json to tuf::pouf::Pouf1?
  • Should we get rid of the HashAlgorithm enum, and use a dyn HashAlgorithm instead? That might allow better customization of the hash algorithm type.
  • Should we drop support for the legacy hash_key_algorithm?
  • The tuf::error::Error is a little messy. Is there a better way to clean it up?
  • should tuf::verify::verify_signature be public?
  • Change TargetsMetadata to store targets in a BTreeMap rather than a HashMap to make ordering stable.
@znewman01
Copy link

Misc. notes below from a cursory read without actually using the library, so reader beware.

Re: ideas in issue

  • MetadataPath and TargetPath: There's a lot of reflexive revulsion against libraries that use unsafe, fair or not. IMO in a security-focused library it's not worth the consternation.
  • +1 to ClientBuilder
  • +1 to tuf::pouf, maybe alias tuf::pouf::Pouf1 to tuf::pouf::Cjson?
  • verify_signature public? I don't see a need for it.

High-level

  • Would be nice to have more direction towards what a user would actually want to use.
    • E.g. quickstart, more examples, re-exporting important structs/traits at the top level
    • Or an overview of how the parts fit together.
    • What's a store vs. a database vs. a repository? IME every TUF library suffers from this confusion, so don't consider it a criticism.
    • Database is kind of a funny name but not sure what else to call it
  • How do you go from zero to root keys setup? a la https://blog.sigstore.dev/sigstore-bring-your-own-stuf-with-tuf-40febfd2badd
    • could be a nice quickstart

Nitpicks/idle thoughts/bikeshedding (feel free to totally ignore)

  • ThingToBuild::build()/Builder::finish() as methods are a little surprising to me vs. ThingToBuild::builder()/Builder::build()
  • repo_builder::RepoBuilder -> repository::MetadataBuilder?
  • RepoBuilder::create -> RepoBuilder::new? Is there a reason for calling it one vs. the other?
  • Implement EphemeralRepository::new() in terms of EphemeralRepository::default()?
  • Delegations::new(HashMap<KeyId, Key>, Vec<Delegation>)
  • IIRC chrono is more-or-less unmaintained and it's generally better to prefer time-rs: CVE-2020-26235 advisory for time 0.1 dependency chronotope/chrono#602
  • RepositoryProvider -> RepositoryReader etc.?
  • Lots of API stutter: repository::RepositoryStorage etc. I know this is not totally a settled issue in Rust but I tend to prefer repository::Storage or even repo::Storage

Positives

  • Overall looks great! Very thoughtfully designed
  • Love the in-memory implementations! Really makes great tests and docs.
  • Like the Verified wrapper a lot.

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

No branches or pull requests

2 participants