Add script to generate metadata for use in compatibility testing. #237

alpearce · 2019-11-22T01:31:32Z

(Also check in generated metadata.)

We want to be explicit about making changes to rust-tuf that change the metadata it generates. go-tuf accomplishes this by containing checked-in keys and metadata generated by go-tuf and, in a unit test, asserts that it produces byte-for-byte equivalent metadata. That way, changes to the library that change generated metadata must include corresponding changes to the checked-in metadata to indicate the change in behavior.

This patch includes the metadata generator and the first set of generated metadata. It uses the same keys as the go-tuf generator so that this metadata can also be used for comparison across implementations.

Feel free to go wild on the review; this is the biggest hunk of Rust I've written and I'd appreciate a lot of comments.

erickt

A few things other things:

First, it looks like you hit a test failure. You can ignore the appveyor, that's going away.
Darn, I thought I added a mode FileSystemBuilder that lets us pretty print the metadata. That makes it much easier to review. I filed Extend FileSystemRepositoryBuilder to support pretty printing metadata #238 to track implementing this.

erickt · 2019-11-22T16:36:24Z

tests/metadata/generate.rs

+use tuf::repository::{FileSystemRepository, FileSystemRepositoryBuilder, Repository};
+
+// TODO parametrize this better/find it within the build?
+const KEYS_PATH: &str = "./keys.json";


This should be okay for now. I've been thinking about re-laying out the directory structure, so we could handle improving discovery of the keys.json file then.

tests/metadata/generate.rs

erickt · 2019-11-22T16:55:46Z

tests/metadata/generate.rs

+        .join((step - 1).to_string())
+        .join("repository");
+    let dst = Path::new(dir).join(step.to_string());
+    Command::new("cp")


Could you use /bin/cp, just to make sure we don't pick up any old cp executable in the current directory? It should be okay if this only is runnable on a posix-y system, since this is a test script.

erickt · 2019-11-22T16:58:26Z

tests/metadata/generate.rs

+    keys
+}
+
+fn copy_repo(dir: &str, step: u8) {


Generally speaking I'd prefer this call some rust function to copy the directories, since we might not guarantee that cp is available. Eventually I'd like to turn this into a test library, so that we can add a test that will complain if we accidentally change the metadata format without regenerating the test metadata, like we're doing in go-tuf.

Unfortunately, I'm guessing you discovered there's no standard library way for copying directories. We might be able to pull in a crate for this (fs_extra seems popular, but we need to check out it's quality), but for now I say we punt for now and solve this later for when we create this identity test.

So in short, could you add a // TODO replace this with pure rust library so it's portable across OSes?

Added TODO.

erickt · 2019-11-22T17:00:24Z

tests/metadata/generate.rs

+}
+
+// adds a target and updates the non-root metadata files.
+async fn add_target(


More evidence we need a helper library for generating metadata.

@alpearce is it ok for you if I attempt at the library part or is it something you would like to do?

Fine with me!

erickt · 2019-11-22T17:04:02Z

tests/metadata/generate.rs

+
+async fn generate_repos(dir: &str, consistent_snapshot: bool) -> tuf::Result<()> {
+    // Create initial repo.
+    // TODO: @erickt, this impl is very tied to the json keys file. Do we need it to be more flexible?


This should be fine for now.

erickt · 2019-11-22T18:29:36Z

tests/metadata/generate.rs

+            .unwrap();
+        copy_repo(dir, i);
+
+        let root_signer: Option<PrivateKey> = match r {


Nit: I think you can leave off Option<PrivateKey>.

alpearce · 2019-11-22T21:28:49Z

Updated:

@erickt's comments
use a constant expiration so metadata will be static
regenerate metadata with the above

heartsucker · 2019-11-23T07:41:55Z

Nice! We should fix #238 before merging this one otherwise reviews are going to be impossible.

alpearce · 2019-11-26T01:56:31Z

Borrowed the "linkify-metadata" script from go-tuf and regenerated symlinked metadata.

This adds an interchange called `JsonPretty`, which makes it easier for humans to review metadata changes. It is identical to `tuf::interchange::Json`, except for the implementation of `to_writer`. Instead of writing out the canonical form, it writes out a pretty printed form of the data. In addition, it changes `SnapshotMetadataBuilder::insert_metadata_with_path` and `TimestampMetadataBuilder::from_snapshot` to use `D::to_writer`. This makes sure that the hash of these metadata files are computed from the format written to disk, instead of the canonical format.

erickt · 2019-11-27T02:14:03Z

A couple more things:

I uploaded a pretty printing interchange in Add a json pretty printing interchange #241, could you rebase your patch on top of this one and use it to regenerate the metadata? While you do that, can you squash all the patches together, then factor out the metadata into it's own patch? That should make it easier to review the metadata generation script from the actual metadata.
I used your PR to test out that JsonPretty worked (thanks!) I was able to successfully parse step 0, but I ran into a bug in step 1. It is missing the 0 target. Instead, it should look like this.
You don't need to do this now, but it'd be nice if we could migrate this script into a separate independent cargo toml. I'd be fine if you want us to do this in a future PR though to get this landed.

alpearce · 2019-11-28T02:37:52Z

Prettified, squashed, separated metadata, fixed bug, added TODO in Cargo.toml.

erickt self-requested a review November 22, 2019 16:21

erickt mentioned this pull request Nov 22, 2019

Extend FileSystemRepositoryBuilder to support pretty printing metadata #238

Closed

erickt requested changes Nov 22, 2019

View reviewed changes

Allison Pearce added 2 commits November 27, 2019 18:35

Add script to generate metadata for use in compatibility testing.

f929bee

upload generated metadata

0ca0d64

alpearce force-pushed the generator branch from 21404fd to 0ca0d64 Compare November 28, 2019 02:36

erickt approved these changes Dec 2, 2019

View reviewed changes

erickt merged commit de06c66 into theupdateframework:develop Dec 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script to generate metadata for use in compatibility testing. #237

Add script to generate metadata for use in compatibility testing. #237

alpearce commented Nov 22, 2019

erickt left a comment

erickt Nov 22, 2019

alpearce Nov 22, 2019

erickt Nov 22, 2019

alpearce Nov 22, 2019

erickt Nov 22, 2019

alpearce Nov 22, 2019

erickt Nov 22, 2019

cavokz Nov 26, 2019

alpearce Nov 27, 2019

erickt Nov 22, 2019

alpearce Nov 22, 2019

erickt Nov 22, 2019

alpearce Nov 22, 2019

alpearce commented Nov 22, 2019

heartsucker commented Nov 23, 2019

alpearce commented Nov 26, 2019

erickt commented Nov 27, 2019

alpearce commented Nov 28, 2019

Add script to generate metadata for use in compatibility testing. #237

Add script to generate metadata for use in compatibility testing. #237

Conversation

alpearce commented Nov 22, 2019

erickt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpearce commented Nov 22, 2019

heartsucker commented Nov 23, 2019

alpearce commented Nov 26, 2019

erickt commented Nov 27, 2019

alpearce commented Nov 28, 2019