PoC(tap13): User Selection of the Top-Level Target Files Through Mapping Metadata #1103

mnm678 · 2020-08-06T22:13:08Z

This pr adds support for TAP 13. It adds support for targets mapping metadata that is defined by the client. This mapping metadata allows the client to specify which targets metadata file on the repository they would like to use as top-level targets. This mechanism allows repository managers to split a repository into multiple namespaces and for clients to specify which of these namespaces to trust.

lukpueh

Thanks for taking a stab at TAP13, @mnm678! Could you please take a look why the tests fail>

Also, my review is mostly about programming- and code-style. I admit that I had a hard time to know if the patch indeed implements TAP 13. Would you mind adding a few more code comments to make the intentions clearer? More details in the commit message would also help. :)

One more style-nit: Please try to stay within 80 characters per line.

lukpueh · 2020-08-20T14:09:47Z

tests/repository_data/client/targets_map.json

+   "scheme": "ed25519"
+  }
+ }
+}


Is there an agreement about the targets map metadata format? AFAIR TAP 12 still misses the definition.

There isn't. I based this format on a suggestion from the comments of the TAP 13 pr. It would probably be good to get some more consensus about this or a different format.

cc @joshuagl, @trishankatdatadog, @JustinCappos

Some of the other metadata files refer to other metadata files as well: they use the full name "targets.json" so targets_filename should probably do that too

in the intervening 1 year we've discovered multiple reasons not to use the full filename.

lukpueh · 2020-08-20T14:23:54Z

tuf/keydb.py

+
+      else:
+        logger.warning('Root Metadata file contains a key with an invalid keytype.')
+


Please try not to copy-paste huge blocks like these (especially if they include dubious keyid_hash_algorithm-code that is prone to change .. see secure-systems-lab/securesystemslib#227). :)

Why not just merge the two dictionaries targets_map_file['keys'] and root_metadata['keys'] into a new dict and then iterate over that one? We need to be careful though and define (and implement) the desired behavior for when there are matching keyids in both dicts.

I removed the repeated block in the latest commit. When merging the dictionaries, I chose to prioritize keys from root metadata if there is a conflict. With the current implementation's use of sha2-256 for all keyids, this conflict should not occur, but it is something that might require some more thought.

lukpueh · 2020-08-20T14:24:25Z

tuf/roledb.py

@@ -76,7 +76,7 @@
 TOP_LEVEL_ROLES = ['root', 'targets', 'snapshot', 'timestamp']


-def create_roledb_from_root_metadata(root_metadata, repository_name='default'):
+def create_roledb_from_root_metadata(root_metadata, repository_name='default', targets_map_file = None):


Style nit: Please remove space around operator.

lukpueh · 2020-08-20T14:25:08Z

tuf/keydb.py

@@ -60,7 +60,7 @@
 _keydb_dict['default'] = {}


-def create_keydb_from_root_metadata(root_metadata, repository_name='default'):
+def create_keydb_from_root_metadata(root_metadata, repository_name='default', targets_map_file = None):


Style nit: Please remove space around operator.

lukpueh · 2020-08-20T14:26:06Z

tuf/roledb.py

@@ -140,6 +144,11 @@ def create_roledb_from_root_metadata(root_metadata, repository_name='default'):
      roleinfo['previous_keyids'] = roleinfo['keyids']
      roleinfo['previous_threshold'] = roleinfo['threshold']

+    if rolename == 'targets' and targets_map_file is not None:


Minor nit: Could be an elif, right?

lukpueh · 2020-08-20T14:34:17Z

tuf/client/updater.py

+    if targets_map_filename is not None:
+      # Is 'targets_map_file' a path?  If not, raise
+      # 'securesystemslib.exceptions.FormatError'.  The actual content of the map
+      # file is validated later on in this method.


Please don't copy those comments. They are already everywhere (not your fault) and IMO not particularly helpful.

lukpueh · 2020-08-20T14:35:20Z

tuf/client/updater.py

+    self.targets_map_filename = None
+
+    if targets_map_filename is not None:
+      # Is 'targets_map_file' a path?  If not, raise
+      # 'securesystemslib.exceptions.FormatError'.  The actual content of the map
+      # file is validated later on in this method.
+      securesystemslib.formats.PATH_SCHEMA.check_match(targets_map_filename)
+      self.targets_map_filename = targets_map_filename


Minor style suggestion:

Suggested change

self.targets_map_filename = None

if targets_map_filename is not None:

# Is 'targets_map_file' a path? If not, raise

# 'securesystemslib.exceptions.FormatError'. The actual content of the map

# file is validated later on in this method.

securesystemslib.formats.PATH_SCHEMA.check_match(targets_map_filename)

self.targets_map_filename = targets_map_filename

if targets_map_filename is not None:

securesystemslib.formats.PATH_SCHEMA.check_match(targets_map_filename)

self.targets_map_filename = targets_map_filename

lukpueh · 2020-08-20T14:35:43Z

tuf/client/updater.py

@@ -626,7 +641,7 @@ class Updater(object):
    http://www.python.org/dev/peps/pep-0008/#method-names-and-instance-variables
  """

-  def __init__(self, repository_name, repository_mirrors):
+  def __init__(self, repository_name, repository_mirrors, targets_map_file = None):


Style nit: Please remove space around operator.

tuf/client/updater.py

lukpueh · 2020-08-20T14:37:48Z

tuf/client/updater.py

+    if targets_map_file is not None:
+      # Is 'targets_map_file' a path?  If not, raise
+      # 'securesystemslib.exceptions.FormatError'.  The actual content of the map
+      # file is validated later on in this method.


See comment about this comment block above.

lukpueh

Thanks for quickly addressing my comments, @mnm678. The patch looks quite good to me (modulo some minor nits in my inline comments). I will need to take another look to say for sure that this fully implements TAP13. It would be nice if other TAP authors could take a look too (especially in regards to the metadata format).

Also, please git rid of the merge commit in the PRs commit history.

lukpueh · 2020-08-21T07:42:32Z

tuf/client/updater.py

+      securesystemslib.formats.PATH_SCHEMA.check_match(targets_map_filename)
+
+      try:
+        self.targets_map_file = securesystemslib.util.load_json_file(\


No need to escape the newline, there is implied line continuation inside parentheses.

lukpueh · 2020-08-21T07:44:56Z

tests/repository_data/client/targets_map.json

+   "scheme": "ed25519"
+  }
+ }
+}


cc @joshuagl, @trishankatdatadog, @JustinCappos

lukpueh · 2020-08-21T07:52:55Z

tuf/keydb.py

+    # If any keyids from root metadata match those from targets_map_metadata,
+    # only the root metadata keys will be added. Add all keys to root_metadata
+    keys_to_add = dict(list(targets_map_file['keys'].items()) +
+        list(root_metadata['keys'].items()))


Minor nit: This technique is discouraged (see e.g. https://stackoverflow.com/questions/38987/how-do-i-merge-two-dictionaries-in-a-single-expression-in-python-taking-union-o). I suggest to use the copy + update technique.

mnm678 · 2020-08-21T17:43:13Z

Thanks @lukpueh . I force pushed to clean up the merge commit. It looks like the linter is failing due to #1115.

jku · 2020-09-17T07:26:08Z

It would be great if commits (and the PR cover letter as well) answered the question "why?", even if the answer is just a single sentence.

I understand the TAP exists but

Once this is committed and no longer part of a PR the context isn't as clear and
Going from no explanation at all to the somewhat academic TAP is big jump

jku · 2020-09-17T07:45:24Z

tuf/client/updater.py

+            targets_map_filename)
+
+      except (securesystemslib.exceptions.Error) as e:
+        raise tuf.exceptions.Error('Cannot load the targets map file: ' + str(e))


Please avoid generic exceptions in public API if possible.

Also document the exception (alternatively remove the 'Exceptions' section from the docs if it's known to be outdated).

Do you think a FormatError would make more sense here (to indicate that the targets map file specified an invalid filename)? Or something else?

Good question: securesystemslib seems to have a wide variety of errors here (some of which clearly aren't formaterrors) so maybe you can't avoid a generic one. In that case at least document the exceptions that may be raised.

By the way, the check_match() call just before this seems to not be needed as securesystemslib does that (unless the point was to trigger the FormatError specifically before the try-except)

This commit adds support for TAP 13 to the Updater class. TAP 13 allows a client to specify which targets metadata on the repository they would like to use as the top-level targets metadata. This mechanism allows repository managers to split a repository into multiple namespaces and for clients to specify which of these namespaces to trust. This commit implements this feature by reading the targets map file and using the indicated metadata as top-level targets metadata. Signed-off-by: marinamoore <[email protected]>

The pr adds test targets mapping metadata using the existing delegated role from the test repository. It also adds an initial test for TAP 13 functionality Signed-off-by: marinamoore <[email protected]>

Signed-off-by: marinamoore <[email protected]>

…itories. This assumes that the targets map file will be the same on each repository listed in the map file. Signed-off-by: marinamoore <[email protected]>

Signed-off-by: marinamoore <[email protected]>

…quest. This commit also adds some additional comments. Signed-off-by: marinamoore <[email protected]>

If targets map is used, use the referenced metadata instead of the top level metadata. This makes _update_metadata_if_changed consistent with _update_metadata. Signed-off-by: marinamoore <[email protected]>

If the targets map file cannot be loaded using load_json_file, a tuf.exceptions.Error exception will be raised. Signed-off-by: marinamoore <[email protected]>

mnm678 · 2020-09-21T19:11:47Z

It would be great if commits (and the PR cover letter as well) answered the question "why?", even if the answer is just a single sentence.

I just force-pushed to update the commit messages.

joshuagl · 2020-11-26T16:27:41Z

As this is a PoC for the TAP process I've converted it to a draft.

joshuagl

I'd suggest "targets_filename" -> "targets_rolename" as we want to move away from being file-centric in the spec

joshuagl · 2021-12-10T16:28:26Z

tests/repository_data/client/targets_map.json

+   "scheme": "ed25519"
+  }
+ }
+}


in the intervening 1 year we've discovered multiple reasons not to use the full filename.

tests/repository_data/client/targets_map.json

Signed-off-by: Marina Moore <[email protected]>

lukpueh · 2022-02-17T10:53:07Z

Now with the legacy code gone this PR will need quite a re-write.

Since most of above review comments and discussion don't seem to be relevant anymore, I suggest to close here and submit a fresh PR for TAP13 ... it should be a lot easier to build on top of the new APIs.

mnm678 marked this pull request as ready for review August 7, 2020 19:31

joshuagl mentioned this pull request Aug 10, 2020

Refactor to use changed format metadata to key #1016

Merged

3 tasks

mnm678 requested a review from lukpueh August 10, 2020 16:51

lukpueh reviewed Aug 20, 2020

View reviewed changes

lukpueh reviewed Aug 21, 2020

View reviewed changes

mnm678 force-pushed the tap13 branch 3 times, most recently from 5490e2d to fde296b Compare August 21, 2020 17:38

mnm678 mentioned this pull request Aug 21, 2020

TAP13 draft theupdateframework/taps#118

Merged

jku reviewed Sep 17, 2020

View reviewed changes

mnm678 force-pushed the tap13 branch from 73b5488 to facb897 Compare September 21, 2020 19:07

mnm678 added 8 commits September 21, 2020 12:09

Add initial TAP 13 tests

b2fe6e3

The pr adds test targets mapping metadata using the existing delegated role from the test repository. It also adds an initial test for TAP 13 functionality Signed-off-by: marinamoore <[email protected]>

Add tests for the targets map file

b47e474

Signed-off-by: marinamoore <[email protected]>

This commit adds support for the targets map file with multiple repos…

4ceda62

…itories. This assumes that the targets map file will be the same on each repository listed in the map file. Signed-off-by: marinamoore <[email protected]>

Update docstrings for targets map file

9b6f0bc

Signed-off-by: marinamoore <[email protected]>

Fix style issues and remove repeated code as discussed in the pull re…

4238496

…quest. This commit also adds some additional comments. Signed-off-by: marinamoore <[email protected]>

Use targets map metadata in _update_metadata_if_changed

dce7e4a

If targets map is used, use the referenced metadata instead of the top level metadata. This makes _update_metadata_if_changed consistent with _update_metadata. Signed-off-by: marinamoore <[email protected]>

Document the exception when the target map file is not loaded.

8eb0c5d

If the targets map file cannot be loaded using load_json_file, a tuf.exceptions.Error exception will be raised. Signed-off-by: marinamoore <[email protected]>

mnm678 force-pushed the tap13 branch from facb897 to 8eb0c5d Compare September 21, 2020 19:10

joshuagl changed the title ~~Tap13~~ PoC(tap13): User Selection of the Top-Level Target Files Through Mapping Metadata Nov 26, 2020

joshuagl marked this pull request as draft November 26, 2020 16:27

mnm678 mentioned this pull request Mar 18, 2021

client refactor: consider API changes #1317

Closed

mnm678 mentioned this pull request Apr 29, 2021

Discussion of TAP 13: User Selection of the Top-Level Target Files Through Mapping Metadata theupdateframework/taps#137

Open

joshuagl reviewed Dec 10, 2021

View reviewed changes

mnm678 commented Dec 10, 2021

View reviewed changes

tests/repository_data/client/targets_map.json Outdated Show resolved Hide resolved

filename -> rolename

a6d6cdc

Signed-off-by: Marina Moore <[email protected]>

lukpueh closed this Feb 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC(tap13): User Selection of the Top-Level Target Files Through Mapping Metadata #1103

PoC(tap13): User Selection of the Top-Level Target Files Through Mapping Metadata #1103

mnm678 commented Aug 6, 2020 •

edited

Loading

lukpueh left a comment

lukpueh Aug 20, 2020

mnm678 Aug 20, 2020

lukpueh Aug 21, 2020

jku Sep 17, 2020

joshuagl Dec 10, 2021

lukpueh Aug 20, 2020

mnm678 Aug 20, 2020

lukpueh Aug 20, 2020

lukpueh Aug 20, 2020

lukpueh Aug 20, 2020

lukpueh Aug 20, 2020

lukpueh Aug 20, 2020

lukpueh Aug 20, 2020

lukpueh Aug 20, 2020

lukpueh left a comment

lukpueh Aug 21, 2020

lukpueh Aug 21, 2020

lukpueh Aug 21, 2020

mnm678 commented Aug 21, 2020

jku commented Sep 17, 2020

jku Sep 17, 2020

mnm678 Sep 18, 2020

jku Sep 21, 2020

mnm678 commented Sep 21, 2020

joshuagl commented Nov 26, 2020

joshuagl left a comment

joshuagl Dec 10, 2021

lukpueh commented Feb 17, 2022


		else:
		logger.warning('Root Metadata file contains a key with an invalid keytype.')

PoC(tap13): User Selection of the Top-Level Target Files Through Mapping Metadata #1103

PoC(tap13): User Selection of the Top-Level Target Files Through Mapping Metadata #1103

Conversation

mnm678 commented Aug 6, 2020 • edited Loading

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

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

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

Choose a reason for hiding this comment

mnm678 commented Aug 21, 2020

jku commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnm678 commented Sep 21, 2020

joshuagl commented Nov 26, 2020

joshuagl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Feb 17, 2022

mnm678 commented Aug 6, 2020 •

edited

Loading