Remove root from snapshot #988

joshuagl · 2020-02-27T15:21:04Z

Fixes issue #: #933

Description of the changes being introduced by the pull request:
Per the updated spec in theupdateframework/specification#31 root.json should no longer be listed in snapshot.json.

Remove the listing of root.json from snapshot.json and update the tests
Update the docs/METADATA.md entry on snapshot.json
Update docstrings and code comments that refer to the order of operations for top-level roles to reflect the workflow in the latest spec
Modify the test repository generator to remove a warning, update a comment and pass relative paths to the target files in add_target() calls
Regenerate the repository_data for the snapshot.json change

Please verify and check that the pull request fulfills the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

tests/repository_data/generate.py

tests/test_updater.py

joshuagl

The PR is marked as draft as I'm sharing before I've finished the changes.

joshuagl · 2020-02-28T11:33:33Z

I believe this PR is ready for review

mnm678 · 2020-03-03T20:25:40Z

tests/repository_data/client/test_repository1/metadata/current/targets.json

@@ -40,7 +40,7 @@
  "targets": {
   "file1.txt": {
    "custom": {
-     "file_permissions": "0644"
+     "file_permissions": "0664"


Why did the write bit get added to these?

Good catch, Marina! Also, do we discuss anywhere why we set file permissions here at all? Is it show what the "custom" attribute can do? If so, we really ought to document it somewhere.

Why did the write bit get added to these?

Nice catch!

664 is the default file permissions for user-created files on Fedora (my host OS) and generate.py does not explicitly set permissions on the target files at creation time. Presumably this differs from wherever the original files were generated.

I can edit the patch to drop this change?

Default file permissions on Ubuntu and Debian are 644 (umask 0022 vs umask 0002 on Fedora), which I believe would account for the discrepancy. I'll edit the patch to drop this change.

I added an extra patch efae583 to set the permissions on file1.txt so that we don't generate different data on different Linux host OS.

tests/repository_data/client/test_repository1/metadata/current/targets.json

trishankatdatadog · 2020-03-03T20:43:20Z

Other than @mnm678 and my comments, this PR LGTM!

trishankatdatadog · 2020-03-04T15:44:06Z

@joshuagl Yeah, unfortunately the tuf logger is very noisy, and sometimes misleading...

lukpueh · 2020-03-11T10:43:28Z

docs/METADATA.md

@@ -64,7 +64,7 @@ of delegated Targets metadata and [example](https://raw.githubusercontent.com/th

 Signed by: Snapshot role.

-The snapshot.json metadata file lists hashes and sizes of all metadata files other than timestamp.json. This file ensures that clients will see a consistent view of the files on the repository. That is, metadata files (and thus target file) that existed on the repository at different times cannot be combined and presented to clients by an attacker.
+The snapshot.json metadata file lists the version, and optionally the file hashes and sizes, of all metadata files other than root.json and timestamp.json. This file ensures that clients will see a consistent view of the files on the repository. That is, metadata files (and thus target file) that existed on the repository at different times cannot be combined and presented to clients by an attacker.


Minor nit: The "all metadata files other" wording is absolutely correct, but wouldn't it be more helpful to tell the reader what files to include, i.e. "top-level targets metadata and all delegated targets metadata" (instead of what files not to include)?
What do others think? This is definitely not a blocker for this PR.

I proposed a similar change in the spec (see theupdateframework/specification@8dc300f and the newer theupdateframework/specification@a5848e7)

That would be much more helpful indeed. I've force-pushed an update to this patch which changes this to:

The snapshot.json metadata file lists the version, and optionally the file hashes and sizes, of the top-level targets metadata and all delegated targets metadata.

lukpueh · 2020-03-11T11:01:15Z

tests/repository_data/generate.py


 repository.targets.delegate('role1', [delegation_public],
    [os.path.basename(target3_filepath)])
-repository.targets('role1').add_target(target3_filepath)
+repository.targets('role1').add_target(os.path.basename(target3_filepath))
 repository.targets('role1').load_signing_key(delegation_private)

 repository.targets('role1').delegate('role2', [delegation_public], [])


Thanks for taking the time to actually fixing this. It makes more sense than adding a dirty patch to a commit message. :)

In PR #40 aginst the specification "root.json" has been removed from the meta dictionary in "snapshot.json". Update generate_snapshot_metadata() to no longer add an entry for root.json to root.json Signed-off-by: Joshua Lock <jlock@vmware.com>

Signed-off-by: Joshua Lock <jlock@vmware.com>

The specification was updated in PR #40 to remove root.json from snapshot.json Signed-off-by: Joshua Lock <jlock@vmware.com>

The workflow for downloading metadata for top-level roles has changed. Root is now updated and verified by stepping through a chain of trust based on the currently available root metadata. For that reason root.json is no longer needed in snapshot and has been dropped from there per theupdateframework/specification#40 Update docstrings and comments in the Updater object to reflect the correct flow of metadata updates: root (if necessary) -> timestamp -> snapshot -> targets Signed-off-by: Joshua Lock <jlock@vmware.com>

* Fix the path referenced in the Purpose * Change add_target() calls to pass file paths relative to targets dir Signed-off-by: Joshua Lock <jlock@vmware.com>

One of the created target files has its file permissions encoded in the targets metadata via the custom attribute of the add_target() function. On Linux-based OS the umask value of the environment the script is run in can result in different octal permissions for the created file, i.e. on Fedora the default umask is 0002 (default permissions 664) whereas on Debian/Ubuntu the default umask is 0022 (default permissions 644). Explicitly chown 'file1' to octal permissions 644 so that the generated data has the same custom attributes for targets regardless of which Linux host they are generated on. Signed-off-by: Joshua Lock <jlock@vmware.com>

Re-generate metadata to adopt the change that root.json is no longer listed in snapshot.json ``` # Remove repository and client data cd tests/repository_data && rm -rf repository client # Generate metadata python generate.py # Duplicate metadata files cp -r client/test_repository1 client/test_repository2 # Recover non-signed file git checkout client/map.json ``` Signed-off-by: Joshua Lock <jlock@vmware.com>

lukpueh

Excellent work and exemplary commit discipline, @joshuagl! I have one tiny request (see inline comment) otherwise it looks great to me.

By the way, I also tested your updates with the old metadata and tests still pass, which is great!

lukpueh · 2020-03-11T11:36:35Z

tuf/client/updater.py

      roles are updated by this method.  The 'timestamp' role is always
      downloaded from a mirror without first checking if it has been updated;
      it is updated in refresh() by calling _update_metadata('timestamp').
+      The 'root' role is always updated first and verified based on the trusted
+      root metadata file the client already has a copy of; it is updated in
+      refresh() by calling _update_root_metadata().


Thanks for making the effort of updating the docstrings. If I understand this function correctly, then it should only be called for "referenced metadata" (On an unrelated side-note: I think the parameter should be called referencing_metadata).

This also means that that the function should no longer be called for root metadata as it is not referenced by any other metadata. Would you mind removing the elif metadata_role == 'root':-logic and TODO: If 'metadata_role' is root -comment in the function body?

I've pushed an additional patch to address the latter part of this comment

Remove logic for handling of root metadata in _update_metadata_if_changed() as root metadata is no longer fetched with this function, instead _update_root_metadata() serves this purpose. Additionally remove redundant mention of root metadata in a TODO comment. Signed-off-by: Joshua Lock <jlock@vmware.com>

joshuagl · 2020-03-11T13:02:03Z

Thank you for the diligent and exhaustive review, as always, @lukpueh ! Contributing to tuf is always a pleasure.

By the way, I also tested your updates with the old metadata and tests still pass, which is great!

I should have thought to test that, happy to read it! 😄

joshuagl commented Feb 27, 2020

View reviewed changes

tests/repository_data/generate.py Show resolved Hide resolved

joshuagl commented Feb 27, 2020

View reviewed changes

tests/repository_data/generate.py Show resolved Hide resolved

tests/test_updater.py Outdated Show resolved Hide resolved

joshuagl commented Feb 27, 2020

View reviewed changes

joshuagl force-pushed the joshuagl/issue-933 branch from 1a91534 to 3c6c61f Compare February 28, 2020 11:27

joshuagl marked this pull request as ready for review February 28, 2020 11:29

joshuagl changed the title ~~WIP: Remove root from snapshot~~ Remove root from snapshot Feb 28, 2020

mnm678 reviewed Mar 3, 2020

View reviewed changes

trishankatdatadog reviewed Mar 3, 2020

View reviewed changes

tests/repository_data/client/test_repository1/metadata/current/targets.json Outdated Show resolved Hide resolved

joshuagl force-pushed the joshuagl/issue-933 branch from 3c6c61f to df2690e Compare March 4, 2020 10:25

lukpueh reviewed Mar 11, 2020

View reviewed changes

joshuagl added 7 commits March 11, 2020 11:15

Remove root from snapshot

0c97271

In PR #40 aginst the specification "root.json" has been removed from the meta dictionary in "snapshot.json". Update generate_snapshot_metadata() to no longer add an entry for root.json to root.json Signed-off-by: Joshua Lock <jlock@vmware.com>

Update tests for removal of root.json from snapshot.json

8f13fe5

Signed-off-by: Joshua Lock <jlock@vmware.com>

Update docs/METADATA.md to reflect recent spec change

8dfee1a

The specification was updated in PR #40 to remove root.json from snapshot.json Signed-off-by: Joshua Lock <jlock@vmware.com>

Update test repository data generator

a134db0

* Fix the path referenced in the Purpose * Change add_target() calls to pass file paths relative to targets dir Signed-off-by: Joshua Lock <jlock@vmware.com>

joshuagl force-pushed the joshuagl/issue-933 branch from df2690e to 3720b23 Compare March 11, 2020 11:36

lukpueh requested changes Mar 11, 2020

View reviewed changes

lukpueh approved these changes Mar 11, 2020

View reviewed changes

lukpueh merged commit 1cf085a into theupdateframework:develop Mar 11, 2020

joshuagl deleted the joshuagl/issue-933 branch March 11, 2020 13:59

joshuagl mentioned this pull request Mar 11, 2020

Remove root from snapshot #933

Closed

joshuagl mentioned this pull request Apr 8, 2020

Problems running tuf unit tests against local version of securesyslib #1010

Closed

effulgentsia mentioned this pull request Aug 14, 2020

Update required version of library in requirements.txt and regenerate test fixtures php-tuf/php-tuf#29

Closed

ethan-lowman-dd mentioned this pull request Jul 15, 2021

July 14 changes raphaelgavache/go-tuf#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove root from snapshot #988

Remove root from snapshot #988

joshuagl commented Feb 27, 2020 •

edited

Loading

joshuagl left a comment

joshuagl commented Feb 28, 2020

mnm678 Mar 3, 2020

trishankatdatadog Mar 3, 2020

joshuagl Mar 3, 2020 •

edited

Loading

joshuagl Mar 4, 2020

joshuagl Mar 4, 2020

trishankatdatadog commented Mar 3, 2020

trishankatdatadog commented Mar 4, 2020

lukpueh Mar 11, 2020

joshuagl Mar 11, 2020

lukpueh Mar 11, 2020

lukpueh left a comment

lukpueh Mar 11, 2020

joshuagl Mar 11, 2020

joshuagl commented Mar 11, 2020

Remove root from snapshot #988

Remove root from snapshot #988

Conversation

joshuagl commented Feb 27, 2020 • edited Loading

joshuagl left a comment

Choose a reason for hiding this comment

joshuagl commented Feb 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuagl Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trishankatdatadog commented Mar 3, 2020

trishankatdatadog commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuagl commented Mar 11, 2020

joshuagl commented Feb 27, 2020 •

edited

Loading

joshuagl Mar 3, 2020 •

edited

Loading