Add simple TUF role metadata model #1112

lukpueh · 2020-08-18T10:26:34Z

This PR copies over the new api/metadata.py module (+ tests) added in #1060, which has shifted focus to implementing a Hashicorp Vault interface.

Co-authorship by @trishankatdatadog, @joshuagl, @sechkova and me plus note-worthy commit message details are preserved in a single squashed commit.

Missing TODOs items for this PR include at least.

add code documentation (docstrings)
finalize signature creation and verification interface
add input validatation

See doc header of api/metadata.py for more details

trishankatdatadog · 2020-08-18T14:52:17Z

tests/test_api.py

+
+# Copyright 2020, New York University and the TUF contributors
+# SPDX-License-Identifier: MIT OR Apache-2.0
+""" Unit tests for api/metdata.py


Suggested change

""" Unit tests for api/metdata.py

""" Unit tests for api/metadata.py

Add metadata module with container classes for TUF role metadata, including methods to read/serialize/write from and to JSON, perform TUF-compliant metadata updates, and create and verify signatures. The 'Metadata' class provides a container for inner TUF metadata objects (Root, Timestamp, Snapshot, Targets) (i.e. OOP composition) The 'Signed' class provides a base class to aggregate common attributes (i.e. version, expires, spec_version) of the inner metadata classes. (i.e. OOP inheritance). The name of the class also aligns with the 'signed' field of the outer metadata container. Based on prior observations in TUF's sister project in-toto, this architecture seems to well represent the metadata model as it is defined in the specification (see in-toto/in-toto#98 and in-toto/in-toto#142 for related discussions). This commits also adds tests. **TODO: See doc header TODO list** **Additional design considerations** (also in regards to prior sketches of this module) - Aims at simplicity, brevity and recognizability of the wireline metadata format. - All attributes that correspond to fields in TUF JSON metadata are public. There doesn't seem to be a good reason to protect them with leading underscores and use setters/getters instead, it just adds more code, and impedes recognizability of the wireline metadata format. - Although, it might be convenient to have short-cuts on the Metadata class that point to methods and attributes that are common to all subclasses of the contained Signed class (e.g. Metadata.version instead of Metadata.signed.version, etc.), this also conflicts with goal of recognizability of the wireline metadata. Thus we won't add such short-cuts for now. See: theupdateframework#1060 (comment) - Signing keys and a 'consistent_snapshot' boolean are not on the targets metadata class. They are a better fit for management code. See: theupdateframework#1060 (comment), and theupdateframework#660. - Does not use sslib schema checks (see TODO notes about validation in doc header) - Does not use existing tuf utils, such as make_metadata_fileinfo, build_dict_conforming_to_schema, if it is easy and more explicit to just re-implement the desired behavior on the metadata classes. - All datetime's are treated as UTC. Since timezone info is not captured in the wireline metadata format it should not be captured in the internal representation either. - Does not use 3rd-party dateutil package, in order to minimize dependency footprint, which is especially important for update clients which often have to vendor their dependencies. However, compatibility between the more advanced dateutil.relativedelta (e.g handles leap years automatically) and timedelta is tested. - Uses PEP8 indentation (4 space) and Google-style doc string instead of sslab-style. See secure-systems-lab/code-style-guidelines#20 - Does not support Python =< 3.5 Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> Co-authored-by: Joshua Lock <jlock@vmware.com> Co-authored-by: Teodora Sechkova <tsechkova@vmware.com> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

sechkova · 2020-08-21T16:05:49Z

I will be leaving some thoughts here while looking at the way the client side (Updater) may use the api/metadata.

There are several places in the code where Metadata needs to be loaded by passing a file object instead of a filename, in particular when Updater downloads temporary metadata files and verifies them. Should we plan for adding another method like read_from_json_object or we may use the Metadata() constructor directly by passing the file object already serialised as a dictionary and not complicating this api?

lukpueh · 2020-08-31T10:37:48Z

There are several places in the code where Metadata needs to be loaded by passing a file object instead of a filename, in particular when Updater downloads temporary metadata files and verifies them. Should we plan for adding another method like read_from_json_object or we may use the Metadata() constructor directly by passing the file object already serialised as a dictionary and not complicating this api?

Great idea! What about the following interface?

# Classmethod on all classes
def from_dict(json_dict):
    """
    Initialize object from Python dictionary, and call from_dict for any
    inner complex attribute that is represented by a class.

    In the case of Metadata.from_dict(), it must dispatch to the corresponding
    from_dict of the base class of Signed, identified by
    json_dict['signed']['_type'].

    NOTE: This is basically a mix of the constructor and load_from_json and
    the inverse of `as_dict` in the current version of metadata.py. """

# Classmethods on Metadata (and optionally on Signed)
def from_json(json_object):
    """Deserialize a json string and pass to from_dict(). """

def from_json_file(filename):
    """Read file from storage and pass to from_json(). """

lukpueh · 2020-09-03T15:18:52Z

Hey @sechkova, I added an Metadata.from_json(metadat_json_string) in a7a33ce. Let me know if this is what you had in mind. 1cdae24 generally cleans up the deserialization interface as I proposed in #1112 (comment) (see detailed commit message).

@woodruffw, @joshuagl, @trishankatdatadog, I'd say this is ready for review aka. throwing rocks at it. It still misses some features but what's there is tested, documented and should serve our purposes for PEP 458. I think we can work off the todo list from metadata.py's doc header independently in follow-up PRs.

lukpueh · 2020-09-03T15:23:42Z

tuf/api/metadata.py

+        version: The metadata version number.
+        spec_version: The TUF specification version number (semver) the
+            metadata format adheres to.
+        expires: The metadata expiration date in 'YYYY-MM-DDTHH:MM:SSZ' format.


Now a datetimeobject.

tuf/api/metadata.py

lukpueh · 2020-09-04T07:53:25Z

tuf/api/metadata.py

+     and assign attributes later on.
+   - Fail on bad json metadata in write_to_json method, but with option to
+     disable check as there might be a justified reason to write WIP
+     metadata to json.


Btw. what do others think about the ValidationMixin methods we use in in-toto? (also see Layout._validate_* methods for example usage)

I think it's a commendable way of doing this (credits to @SantiagoTorres)

That ValidationMixin looks nice! I'd be happy to see it used here in tuf

trishankatdatadog

Great job, @lukpueh! 🎉

I don't have strong opinions about this PR: it looks largely like the Metadata API I originally proposed, except for changes like removing KeyRing and Threshold so on (we can argue about that later). As long as @woodruffw and @joshuagl also approve, I'm 👍🏽

joshuagl

Very nice streamlining of the metadata API PR. Thanks @lukpueh! I have a few minor questions and comments inline. I'm also curious to understand why version and spec_version have different types? version: int and spec_version: str, is there any reason spec_version can't be an int?

tuf/api/metadata.py

joshuagl · 2020-09-01T12:51:42Z

tuf/api/metadata.py

+
+            FIXME: Is this behavior expected? An alternative approach would be
+            to raise an exception if no signature is found for the keyid,
+            and/or if more than one sigantures are found for the keyid.


Suggested change

and/or if more than one sigantures are found for the keyid.

and/or if more than one signatures are found for the keyid.

I prefer the alternative approach of raising an exception if no signature is found, or more than one signature is found, for the keyid.

I think I do too. Any advice on the exception type? At some point we should discuss exception taxonomy guidelines. But we shouldn't block on that.

We don't seem to have an exception in tuf.exceptions or securesystemslib.exceptions that would fit, we could either throw a generic exception or define a new one. Either way, it will probably need some cleanup once guidelines are defined.

joshuagl · 2020-09-07T15:12:46Z

tests/test_api.py

+        cls.temporary_directory = tempfile.mkdtemp(dir=os.getcwd())
+
+        test_repo_data = os.path.join(
+                os.path.dirname(os.path.realpath(__file__)), 'repository_data')


8 space indentation on a continuation?

Hm. Good question. I always thought PEP 8 required double-indentation for hanging indent, but it is only a suggestion, and also only to distinguish it from a subsequent nested block.

IMO it's a good idea to always differentiate line continuation indentation from functional indentation, even if there is no subsequent nested block. But I'm fine either way.

FYI, the google style guide does not say anything about double indent but has some examples that could be interpreted that way.

IMO it's a good idea to always differentiate line continuation indentation from functional indentation, even if there is no subsequent nested block. But I'm fine either way.

That seems like a reasonable coding style for us to adopt. We should document it and teach the linter about it 😃

joshuagl · 2020-09-07T15:21:11Z

tuf/api/metadata.py

+     and assign attributes later on.
+   - Fail on bad json metadata in write_to_json method, but with option to
+     disable check as there might be a justified reason to write WIP
+     metadata to json.


That ValidationMixin looks nice! I'd be happy to see it used here in tuf

joshuagl · 2020-09-07T15:27:13Z

tuf/api/metadata.py

+        # Convert 'expires' TUF metadata string to a datetime object, which is
+        # what the constructor expects and what we store. The inverse operation
+        # is implemented in 'to_dict'.
+        signed_dict['expires'] = iso8601.parse_date(


Do we still need an iso8601 dependency if we're only targeting Python 3.5+, can we just use datetime.fromisoformat() here?

joshuagl · 2020-09-07T15:34:40Z

tests/test_api.py

@@ -3,7 +3,12 @@
 # Copyright 2020, New York University and the TUF contributors
 # SPDX-License-Identifier: MIT OR Apache-2.0
 """ Unit tests for api/metdata.py
+
+Skipped on Python < 3.6.


I presume the goal would be to revert this patch once all Python 2.7 supporting code has been removed?

tuf/api/metadata.py

lukpueh · 2020-09-08T13:33:32Z

version: int and spec_version: str, is there any reason spec_version can't be an int?

Yes, spec_version is in semver format.

lukpueh · 2020-09-08T15:41:25Z

Thanks for the reviews, @joshuagl and @trishankatdatadog!. I addressed the minor comments in 0a9fc93, which I'm happy to squash with previous commits before merging. The more serious change of Metadata.verify behaviour is in a separate commit, to make review easier. :)

joshuagl

Thanks @lukpueh, this looks solid. I'm very happy to see this long-discussed model implemented.

One minor typo in a comment, but otherwise this looks good to rebase and merge.

joshuagl · 2020-09-09T10:36:45Z

tests/test_api.py

@@ -19,6 +17,7 @@
 from datetime import datetime, timedelta
 from dateutil.relativedelta import relativedelta

+# TODO: Remove case handling when fully dropping support for versions >= 3.6


Suggested change

# TODO: Remove case handling when fully dropping support for versions >= 3.6

# TODO: Remove case handling when fully dropping support for versions < 3.6

🤦 thanks for catching this!

joshuagl · 2020-09-09T10:37:34Z

tuf/api/metadata.py


-import iso8601


joshuagl · 2020-09-09T10:37:51Z

tuf/api/metadata.py

+        signed_dict['expires'] = datetime.strptime(
+                signed_dict['expires'],
+                "%Y-%m-%dT%H:%M:%SZ").replace(tzinfo=None)


Awesome, thanks for adding this!

We have three uses of iso8601.parse_date in the current codebase, so this might be a good candidate for a helper function in future.

The new metadata module uses constructs that are only available on Python >= 3.6 (typing, f-format strings, etc.). Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add generic read from json class method that returns a Metadata object with a signed field that contains the appropriate Signed subclass, based on the signed._type field of the read metadata. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add simple as_json Metadata method and use it instead of repository lib's internal _get_written_metadata function in write_to_json. This commit further adds code documentation and the possibility to write compact json by excluding whitespace to write_to_json, and also removes a call to the sign method from write_to_json. The commit also adds tests. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add simple methods to create or verify signatures of the canonical_signed property of a Metadata object. See corresponding docstrings for behavior and design considerations. The commit also adds tests and updates the test setup to load some test keys into memory. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Remove metadata factory on Signed class, for the sake of API simplicity/non-ambiguity, i.e. it's enough to have one way of loading any Metadata, that is: Metadata.read_from_json Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Consistenly rename de/serialization interface methods, using a 'from_' and 'to_' prefix. read_from_json -> from_json_file write_to_json -> to_json_file as_json -> to_json as_dict -> to_dict signed_bytes -> to_canonical_bytes The latter is also changed from a property to a method for consistency with the other serialization methods. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

This commit better separates the Metadata class model from the Metadata wireline format, by tailoring the constructors towards class-based parameters and adding an additional factory classmethod that creates Metadata objects based on the wireline json/dictionary metadata representation. (pythonic way of constructor overloading). This 'from_dict' factory method recurses into the 'from_dict' methods of each contained complex field/attribute that is also represented by a class. Currently 'signed' is the only such attribute. This commit further: - Changes optional constructor keyword arguments to mandatory positional arguments: Reduces code and simplifies usage by restricting it. For now, users are unlikely to call constructor directly anyway, but the 'from_dict' factory (or its 'from_json_file' wrapper) instead. - Removes Signed.__expiration (datetime) vs. Signed.expires (datestring) dichotomy: Keeping only one representation of the same attribute in memory makes the interface simpler and less ambiguous. We choose the datetime object, because it is more convenient to modify. Transformation from and to the string format required by the tuf wireline format is performed in the corresponding metadata de/serialization methods, i.e. ('to_dict' and 'from_dict'). Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add convenience wrapper that takes a json string and passes it to from_dict to create a Metadata object. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Change Metadata.verify(key) behavior to raise an exception if none or multiple signatures for the passed key are found on the Metadata object. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

See: Add root metadata class to new TUF metadata model theupdateframework#1137 Add classes for complex metadata fields theupdateframework#1139 Add input validation to simple metadata api theupdateframework#1140 Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Use builtin datetime instead of external iso6801 for simple datetime string parsing. Also see theupdateframework#1065 Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

lukpueh · 2020-09-10T14:27:35Z

Thanks again for the reviews! I just cleaned up the git history, fixed a minor inconsistency and removed the todo list after created GitHub issues for each item.
The diff of the new force-pushed tip to your last review can be seen here: https://github.com/lukpueh/tuf/compare/simple-tuf-api-before-rebase..simple-tuf-api

lukpueh force-pushed the simple-tuf-api branch from 3bac769 to bf805b0 Compare August 18, 2020 13:03

trishankatdatadog reviewed Aug 18, 2020

View reviewed changes

lukpueh force-pushed the simple-tuf-api branch 3 times, most recently from be19a11 to ae23d32 Compare August 19, 2020 08:13

lukpueh force-pushed the simple-tuf-api branch from e530af3 to 6732705 Compare August 20, 2020 10:21

lukpueh force-pushed the simple-tuf-api branch 3 times, most recently from a71b03e to 520bc20 Compare September 3, 2020 14:26

lukpueh changed the title ~~Add simple TUF role metadata model (WIP)~~ Add simple TUF role metadata model Sep 3, 2020

lukpueh marked this pull request as ready for review September 3, 2020 14:30

lukpueh commented Sep 3, 2020

View reviewed changes

lukpueh commented Sep 4, 2020

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

lukpueh commented Sep 4, 2020

View reviewed changes

trishankatdatadog approved these changes Sep 4, 2020

View reviewed changes

joshuagl reviewed Sep 7, 2020

View reviewed changes

joshuagl mentioned this pull request Sep 8, 2020

support updating individual metadata upon addition of target file #1048

Closed

lukpueh force-pushed the simple-tuf-api branch from e06e278 to 0a9fc93 Compare September 8, 2020 15:35

joshuagl approved these changes Sep 9, 2020

View reviewed changes

This was referenced Sep 10, 2020

Decide on Python version #1125

Closed

Decide where to develop TUF 1.0.0 #1126

Closed

Decide on style guideline #1128

Closed

Decide on extent of OOP in metadata model #1133

Closed

How much metadata behaviour should be implemented on their classes? #1134

Open

This was referenced Sep 10, 2020

Design client library #1135

Closed

Design repository library #1136

Closed

joshuagl mentioned this pull request Sep 10, 2020

Remove iso8601 dependency #1065

Closed

lukpueh added 15 commits September 10, 2020 15:59

Skip api tests on Python < 3.6

b1dd3d6

The new metadata module uses constructs that are only available on Python >= 3.6 (typing, f-format strings, etc.). Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Simplifies Timestamp.update method

0d7e268

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add metadata model class and method docstrings

5cc7335

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Remove comments and unify quotes in api tests

21de660

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Remove Signed.read_from_json metadata method

e61ae1b

Remove metadata factory on Signed class, for the sake of API simplicity/non-ambiguity, i.e. it's enough to have one way of loading any Metadata, that is: Metadata.read_from_json Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add from_json metadata convenience wrapper

387169f

Add convenience wrapper that takes a json string and passes it to from_dict to create a Metadata object. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Re-order metadata methods logically and add vspace

f9a4ebe

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Raise on bad signature count in Metadata.verify

73dd72d

Change Metadata.verify(key) behavior to raise an exception if none or multiple signatures for the passed key are found on the Metadata object. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Remove iso8601 dependency from simple metadata api

f106435

Use builtin datetime instead of external iso6801 for simple datetime string parsing. Also see theupdateframework#1065 Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

lukpueh force-pushed the simple-tuf-api branch from 0a9fc93 to f106435 Compare September 10, 2020 14:21

lukpueh merged commit eda46e1 into theupdateframework:develop Sep 10, 2020

lukpueh mentioned this pull request Sep 11, 2020

Add metadata container classes for signed metadata secure-systems-lab/securesystemslib#272

Closed

joshuagl mentioned this pull request Sep 11, 2020

PoC(tap): Snapshot merkle tree #1113

Closed

joshuagl mentioned this pull request Nov 26, 2020

A simple, per-file metadata CRUD API #1060

Closed

3 tasks

di mentioned this pull request Feb 1, 2022

Roadmap for PEP 458 pypi/warehouse#10672

Open

52 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple TUF role metadata model #1112

Add simple TUF role metadata model #1112

lukpueh commented Aug 18, 2020 •

edited

Loading

trishankatdatadog Aug 18, 2020

sechkova commented Aug 21, 2020

lukpueh commented Aug 31, 2020

lukpueh commented Sep 3, 2020

lukpueh Sep 3, 2020

lukpueh Sep 4, 2020

joshuagl Sep 7, 2020

trishankatdatadog left a comment

joshuagl left a comment

joshuagl Sep 1, 2020

joshuagl Sep 7, 2020

lukpueh Sep 8, 2020

joshuagl Sep 8, 2020

joshuagl Sep 7, 2020

lukpueh Sep 8, 2020

joshuagl Sep 8, 2020

joshuagl Sep 7, 2020

joshuagl Sep 7, 2020

joshuagl Sep 7, 2020

lukpueh commented Sep 8, 2020

lukpueh commented Sep 8, 2020

joshuagl left a comment

joshuagl Sep 9, 2020

lukpueh Sep 10, 2020

joshuagl Sep 9, 2020

joshuagl Sep 9, 2020

joshuagl Sep 9, 2020

lukpueh commented Sep 10, 2020

	""" Unit tests for api/metdata.py
	""" Unit tests for api/metadata.py

	and/or if more than one sigantures are found for the keyid.
	and/or if more than one signatures are found for the keyid.

	# TODO: Remove case handling when fully dropping support for versions >= 3.6
	# TODO: Remove case handling when fully dropping support for versions < 3.6

Add simple TUF role metadata model #1112

Add simple TUF role metadata model #1112

Conversation

lukpueh commented Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

sechkova commented Aug 21, 2020

lukpueh commented Aug 31, 2020

lukpueh commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trishankatdatadog left a comment

Choose a reason for hiding this comment

joshuagl 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

lukpueh commented Sep 8, 2020

lukpueh commented Sep 8, 2020

joshuagl 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

lukpueh commented Sep 10, 2020

lukpueh commented Aug 18, 2020 •

edited

Loading