tough: RFC: More flexible datastores

[iliana]

Hello again! 👋

The datastore, currently:

tough/tough/src/lib.rs

Lines 202 to 208 in 7aa4804

	/// Set a `datastore` directory path. `datastore` is a directory on a persistent filesystem.
	/// This directory's contents store the most recently fetched timestamp, snapshot, and targets
	/// metadata files to detect version rollback attacks.
	///
	/// You may chose to provide a [`PathBuf`] to a directory on a persistent filesystem, which must
	/// exist prior to calling [`RepositoryLoader::load`]. If no datastore is provided, a temporary
	/// directory will be created and cleaned up for for you.

I'm working in an environment where all my easy-to-access filesystems are not persistent, but I'd still like to be able to detect rollback attacks.

I'd like to make Datastore a trait, with more or less the following definition, as well as retaining the current behavior of the persistent and temporary (default) filesystem-based datastores:

trait Datastore {
    /// Stores a role in the datastore.
    ///
    /// The datastore implementation defines what parts of the role are stored. Implementations
    /// that do not persist to trusted storage should store a serialized copy of the full role to
    /// be verified in `load_version`.
    fn store_role(&self, role: Signed<impl Role>) -> Result<()>;

    /// Retrieves a role's version from the datastore.
    ///
    /// Implementations that do not persist to trusted storage should verify the stored role
    /// against the provided trusted root role before returning a version.
    fn load_version(&self, role: RoleType, root: Root) -> Result<NonZeroU64>;
}

struct FilesystemDatastore(PathBuf);

impl Datastore for FilesystemDatastore {
    ..
}

This would also add a Box<dyn Datastore> to Repository and RepositoryLoader.

The only information required for preventing rollback attacks is trusted persistent storage of the versions of the various roles; in my environment storing only the versions to a trusted database would be sufficient here.

Thoughts/comments/concerns before I go implementing? :)

[webern]

This sounds fine to me. The only thought I had in the back of my mind is whether or not the existing Datastore enum could remain more-or-less as is. If the trait had some other name, (Metastore? Rolestore?), and if the .datastore() function in the RepositoryLoader became generic taking Into<Box dyn Rolestore>, and if the we impl Into<Box<dyn Rolestore>> for Datastore then maybe the blast radius becomes very small?

Then again, the blast radius is already probably very small because there is not a lot of code that actually specifies a custom (i.e. non-default) Datastore.

Edit: and maybe a function with the name datastore taking Into<Box<dyn Rolestore>> is dumb anyway.

Edit2: we might need to sternly warn potential self-implementers that taking the Datastore trait into their own hands means they are responsible for the safety of the repo?

[iliana]

The only thought I had in the back of my mind is whether or not the existing Datastore enum could remain more-or-less as is ... then maybe the blast radius becomes very small?

1. Currently the Datastore enum isn't exposed at all, IIRC -- the only exposed API is RepositoryLoader::datastore, which takes an Into<PathBuf>, as well as some error enums (which I'd probably like to rework as just a single variant with a source error?). It'd be reasonable enough to keep the behavior of RepositoryLoader::datastore exactly as it is today, and just rework it to use the proposed FilesystemStore struct. I'd probably add a RepositoryLoader::custom_datastore method, with all the necessary caveats.
2. This is my once-maintainer voice showing :) I realize potential breakage up the stack in Bottlerocket is not ideal but from the perspective of tough, it's not 1.0 yet; any API breakage we cause to make tough a better library to work with is worth it.

we might need to sternly warn potential self-implementers that taking the Datastore trait into their own hands means they are responsible for the safety of the repo?

Yes, extremely. We could also stand to be a lot clearer about what threats exist if you don't provide a datastore today.

[webern]

I'd probably add a RepositoryLoader::custom_datastore method, with all the necessary caveats.

I'm not worried about breakage, I was just mulling it over. One problem is that custom_datastore would be mutually exclusive with the datastore function. You're right that the Datastore enum is private, I forgot that the function signature we have is this:

pub fn datastore<P: Into<PathBuf>>(mut self, datastore: P) -> Self { /* ... */ }

Maybe it could be changed to:

pub fn datastore<D: Into<Box<dyn Datastore>>(mut self, datastore: D) -> Self { /* ... */ }

And we could

impl From<Into<PathBuf>> for YeOldeEnumDatastore  // <- not sure that's a thing feels like something I always wish I could do

(or more likely correct)

impl From<PathBuf> for YeOldeEnumDatastore

And

impl Datastore for YeOldeEnumDatastore

[iliana]

I think Rust forces us to do something slightly more contrived (spoiler: I get to write trait IntoDatastore), but we can get something pretty close to that, yeah.