From 3b161bfe20ba20703dd850e8538f0d43fd3d578a Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Tue, 20 Apr 2021 09:46:41 +0300
Subject: [PATCH 01/33] experimental client: Add MetadataBundle

MetadataBundle keeps track of current valid set of metadata for the
client, and handles almost every step of the "Detailed client workflow"
in the TUF specification (the remaining steps are download related).

The bundle takes care of persisting valid metadata on disk, loading
local metadata from disk and deleting invalid local metadata. It also
verifies any new metadata (downloaded from remote repository) it is
given.

This is very much a work-in-progress.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 349 +++++++++++++++++++++++++++
 1 file changed, 349 insertions(+)
 create mode 100644 tuf/client_rework/metadata_bundle.py

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
new file mode 100644
index 0000000000..f54335b5ef
--- /dev/null
+++ b/tuf/client_rework/metadata_bundle.py
@@ -0,0 +1,349 @@
+# Copyright the TUF contributors
+# SPDX-License-Identifier: MIT OR Apache-2.0
+
+"""TUF client bundle-of-metadata
+
+MetadataBundle keeps track of current valid set of metadata for the client,
+and handles almost every step of the "Detailed client workflow" in the TUF
+specification (the remaining steps are download related). The bundle takes
+care of persisting valid metadata on disk, loading local metadata from disk
+and deleting invalid local metadata.
+
+New metadata (downloaded from a remote repository) can be loaded using
+'update_metadata()'. The type of accepted metadata depends on bundle state
+(states are "root"/"timestamp"/"snapshot"/"targets"/). Bundle states advances
+to next state on every successful metadata update, except for "root" where state
+only advances when 'root_update_finished()' is called. Exceptions will be thrown
+if metadata fails to load in any way.
+
+Example (with hypothetical download function):
+
+>>> # Load local root
+>>> bundle = MetadataBundle("path/to/metadata")
+>>>
+>>> # state: "root", load more root versions from remote
+>>> with download("root", bundle.root.signed.version + 1) as f:
+>>>     bundle.load_metadata(f.read())
+>>> with download("root", bundle.root.signed.version + 1) as f:
+>>>     bundle.load_metadata(f.read())
+>>>
+>>> # Finally, no more root from remote
+>>> bundle.root_update_finished()
+>>>
+>>> # state: "timestamp", load timestamp
+>>> with download("timestamp") as f:
+>>>     bundle.load_metadata(f.read())
+>>>
+>>> # state: "snapshot", load snapshot (consistent snapshot not shown)
+>>> with download("snapshot") as f:
+>>>     bundle.load_metadata(f.read())
+>>>
+>>> # state: "targets", load targets
+>>> version = bundle.snapshot.signed.meta["targets.json"]["version"]
+>>> with download("snapshot", version + 1) as f:
+>>>     bundle.load_metadata(f.read())
+>>>
+>>> # Top level metadata is now fully loaded and verified
+
+
+TODO:
+ * Delegated targets not implement yet
+ * exceptions are all over the place and not thought out at all
+ * a bit of repetition
+ * No tests!
+ * Naming maybe not final?
+ * some metadata interactions might work better in Metadata itself
+ * Progress through Specification update process should be documented
+   (not sure yet how)
+"""
+
+from collections import abc
+from datetime import datetime
+import logging
+import os
+from typing import Dict
+
+from securesystemslib import keys as sslib_hash
+from securesystemslib import keys as sslib_keys
+
+from tuf import exceptions
+from tuf.api.metadata import Metadata
+
+logger = logging.getLogger(__name__)
+
+# This is a placeholder until ...
+# TODO issue 1306: implement this in Metadata API
+def verify_with_threshold(root: Metadata, role: str, unverified: Metadata):
+    unique_keys = set()
+    for keyid in root.signed.roles[role]["keyids"]:
+        key_metadata = root.signed.keys[keyid]
+        key, _ = sslib_keys.format_metadata_to_key(key_metadata)
+
+        try:
+            if unverified.verify(key):
+                unique_keys.add(key["keyval"]["public"])
+        except:
+            pass
+
+    return len(unique_keys) >= root.signed.roles[role]["threshold"]
+
+
+# TODO issue 1336: implement in metadata api
+from tuf.api.serialization.json import JSONDeserializer
+
+
+def from_string(data: str) -> Metadata:
+    return JSONDeserializer().deserialize(data)
+
+
+class MetadataBundle(abc.Mapping):
+    def __init__(self, path: str):
+        """Initialize by loading existing metadata from disk
+
+        This includes root, timestamp, snapshot and _top-level_ targets .
+        """
+        self._path = path
+        self._bundle = {}  # type: Dict[str: Metadata]
+        self._state = "root"
+        self.reference_time = None
+
+        if not os.path.exists(path):
+            # TODO try to create dir instead?
+            raise exceptions.RepositoryError("Repository does not exist")
+
+        # Load and validate the local root metadata
+        # Valid root metadata is required (but invalid files are not removed)
+        try:
+            with open(os.path.join(self._path, "root.json"), "rb") as f:
+                self._load_intermediate_root(f.read())
+            logger.debug("Loaded local root.json")
+        except:
+            raise exceptions.RepositoryError("Failed to load local root metadata")
+
+    def update_metadata(self, metadata_str: str):
+        logger.debug("Updating %s", self._state)
+        if self._state == "root":
+            self._load_intermediate_root(metadata_str)
+            self.root.to_file(os.path.join(self._path, "root.json"))
+        elif self._state == "timestamp":
+            self._load_timestamp(metadata_str)
+            self.timestamp.to_file(os.path.join(self._path, "timestamp.json"))
+            self._state = "snapshot"
+        elif self._state == "snapshot":
+            self._load_snapshot(metadata_str)
+            self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
+            self._state = "targets"
+        elif self._state == "targets":
+            self._load_targets(metadata_str)
+            self.targets.to_file(os.path.join(self._path, "targets.json"))
+            self._state = ""
+        else:
+            raise NotImplementedError
+
+    def root_update_finished(self):
+        if self._state != "root":
+            # bundle does not support this order of ops
+            raise exceptions.RepositoryError
+
+        self._make_root_permanent(self)
+        self._state = "timestamp"
+
+    # Implement Mapping
+    def __getitem__(self, key: str):
+        return self._bundle[key]
+
+    def __len__(self):
+        return len(self._bundle)
+
+    def __iter__(self):
+        return iter(self._bundle)
+
+    # Helper properties for top level metadata
+    @property
+    def root(self):
+        return self._bundle.get("root")
+
+    @property
+    def timestamp(self):
+        return self._bundle.get("timestamp")
+
+    @property
+    def snapshot(self):
+        return self._bundle.get("snapshot")
+
+    @property
+    def targets(self):
+        return self._bundle.get("targets")
+
+    def _load_intermediate_root(self, data: str):
+        """Verify the new root using current root (if any) and use it as current root
+
+        Raises if root fails verification
+        """
+        new_root = from_string(data)
+        if new_root.signed._type != "root":
+            raise exceptions.RepositoryError
+
+        if self.root is not None:
+            if not verify_with_threshold(self.root, "root", new_root):
+                raise exceptions.UnsignedMetadataError(
+                    "New root is not signed by root", new_root.signed
+                )
+
+            if new_root.signed.version != self.root.signed.version + 1:
+                # TODO not a "Replayed Metadata attack": the version is just not what we expected
+                raise exceptions.ReplayedMetadataError(
+                    "root", new_root.signed.version, self.root.signed.version
+                )
+
+        if not verify_with_threshold(new_root, "root", new_root):
+            raise exceptions.UnsignedMetadataError(
+                "New root is not signed by itself", new_root.signed
+            )
+
+        self._bundle["root"] = new_root
+
+    def _make_root_permanent(self):
+        # Store our reference "now", verify root expiry
+        self.reference_time = datetime.utcnow()
+        if self.root.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError
+
+        logger.debug("Verified final root.json")
+
+        # Load remaning local metadata: this ensures invalid
+        # metadata gets wiped from disk
+        try:
+            with open(os.path.join(self._path, "timestamp.json"), "rb") as f:
+                self._load_timestamp(f.read())
+            logger.debug("Loaded local timestamp.json")
+        except Exception as e:
+            # TODO only handle specific errors
+            logger.debug("Failed to load local timestamp.json")
+            # TODO delete local file
+
+        try:
+            with open(os.path.join(self._path, "snapshot.json"), "rb") as f:
+                self._load_snapshot(f.read())
+            logger.debug("Loaded local snapshot.json")
+        except Exception as e:
+            # TODO only handle specific errors
+            logger.debug("Failed to load local snapshot.json")
+            # TODO delete local file
+
+        try:
+            with open(os.path.join(self._path, "targets.json"), "rb") as f:
+                self._load_targets(f.read())
+            logger.debug("Loaded local targets.json")
+        except Exception as e:
+            # TODO only handle specific errors
+            logger.debug("Failed to load local targets.json")
+            # TODO delete local file
+
+    def _load_timestamp(self, data: str):
+        """Verifies the new timestamp and uses it as current timestamp
+
+        Raises if verification fails
+        """
+        new_timestamp = from_string(data)
+        if new_timestamp.signed._type != "timestamp":
+            raise exceptions.RepositoryError
+
+        if not verify_with_threshold(self.root, "timestamp", new_timestamp):
+            raise exceptions.UnsignedMetadataError(
+                "New timestamp is not signed by root", new_timestamp.signed
+            )
+
+        if self.timestamp is not None:
+            # Prevent rolling back timestamp version
+            if new_timestamp.signed.version < self.timestamp.signed.version:
+                raise exceptions.ReplayedMetadataError(
+                    "timestamp",
+                    new_timestamp.signed.version,
+                    self.timestamp.signed.version,
+                )
+            # Prevent rolling back snapshot version
+            if (
+                new_timestamp.signed.meta["snapshot.json"]["version"]
+                < self.timestamp.signed.meta["snapshot.json"]["version"]
+            ):
+                # TODO not sure about the
+                raise exceptions.ReplayedMetadataError(
+                    "snapshot",
+                    new_timestamp.signed.meta["snapshot.json"]["version"],
+                    self.timestamp.signed.meta["snapshot.json"]["version"],
+                )
+
+        if new_timestamp.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError
+
+        self._bundle["timestamp"] = new_timestamp
+
+    def _load_snapshot(self, data: str):
+        # Verify against the hashes in timestamp, if any
+        meta = self.timestamp.signed.meta["snapshot.json"]
+        hashes = meta.get("hashes") or {}
+        for algo, _hash in meta["hashes"].items():
+            digest_object = sslib_hash.digest(algo)
+            digest_object.update(data)
+            if digest_object.hexdigest() != _hash:
+                raise exceptions.BadHashError()
+        new_snapshot = from_string(data)
+        if new_snapshot.signed._type != "snapshot":
+            raise exceptions.RepositoryError
+
+        if not verify_with_threshold(self.root, "snapshot", new_snapshot):
+            raise exceptions.UnsignedMetadataError(
+                "New snapshot is not signed by root", new_snapshot.signed
+            )
+
+        if (
+            new_snapshot.signed.version
+            != self.timestamp.signed.meta["snapshot.json"]["version"]
+        ):
+            raise exceptions.BadVersionNumberError
+
+        if self.snapshot:
+            for filename, fileinfo in self.snapshot.signed.meta.items():
+                new_fileinfo = new_snapshot.signed.meta.get(filename)
+
+                # Prevent removal of any metadata in meta
+                if new_fileinfo is None:
+                    raise exceptions.ReplayedMetadataError
+
+                # Prevent rollback of any metadata versions
+                if new_fileinfo["version"] < fileinfo["version"]:
+                    raise exceptions.ReplayedMetadataError
+
+        if new_snapshot.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError
+
+        self._bundle["snapshot"] = new_snapshot
+
+    def _load_targets(self, data: str):
+        # Verify against the hashes in snapshot, if any
+        meta = self.snapshot.signed.meta["targets.json"]
+
+        hashes = meta.get("hashes") or {}
+        for algo, _hash in hashes.items():
+            digest_object = sslib_hash.digest(algo)
+            digest_object.update(data)
+            if digest_object.hexdigest() != _hash:
+                raise exceptions.BadHashError()
+
+        new_targets = from_string(data)
+        if new_targets.signed._type != "targets":
+            raise exceptions.RepositoryError
+
+        if not verify_with_threshold(self.root, "targets", new_targets):
+            raise exceptions.UnsignedMetadataError(
+                "New targets is not signed by root", new_targets.signed
+            )
+
+        if new_targets.signed.version != meta["version"]:
+            raise exceptions.BadVersionNumberError
+
+        if new_targets.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError
+
+        self._bundle["targets"] = new_targets

From 5e1fe0d4b501d1ca7acb864e6603b2c0367526af Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Tue, 20 Apr 2021 11:14:43 +0300
Subject: [PATCH 02/33] MetadataBundle: Update outdated docstring

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index f54335b5ef..ecf509168c 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -98,9 +98,7 @@ def from_string(data: str) -> Metadata:
 
 class MetadataBundle(abc.Mapping):
     def __init__(self, path: str):
-        """Initialize by loading existing metadata from disk
-
-        This includes root, timestamp, snapshot and _top-level_ targets .
+        """Initialize by loading root metadata from disk
         """
         self._path = path
         self._bundle = {}  # type: Dict[str: Metadata]

From 3f7c40524cefc737501c5e045cd6b57730120178 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Tue, 20 Apr 2021 21:10:52 +0300
Subject: [PATCH 03/33] MetadataBundle: Modify state handling

A single state variable cannot really handle the case where we may want
load local metadata and may want to updater from remote -- but are not
required to do either.

Instead, make sure two rules apply:
* Metadata can only be loaded/updated if all metadata it depends on is
  loaded
* Metadata becomes immutable when anything that depends on it is loaded

So, loading a new timestamp is possible if root is loaded and snapshot
is not loaded.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 193 +++++++++++++++++----------
 1 file changed, 119 insertions(+), 74 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index ecf509168c..b4e9211d4c 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -10,38 +10,43 @@
 and deleting invalid local metadata.
 
 New metadata (downloaded from a remote repository) can be loaded using
-'update_metadata()'. The type of accepted metadata depends on bundle state
-(states are "root"/"timestamp"/"snapshot"/"targets"/). Bundle states advances
-to next state on every successful metadata update, except for "root" where state
-only advances when 'root_update_finished()' is called. Exceptions will be thrown
-if metadata fails to load in any way.
+'update_metadata()'. The type of accepted metadata depends on bundle state:
+ * Metadata is loadable only if all metadata it depends on is loaded
+ * Metadata is immutable if any metadata depending on it has been loaded 
+
+Exceptions are raised if metadata fails to load in any way (except in the
+case of local loads -- see locad_local_metadata()).
 
 Example (with hypothetical download function):
 
 >>> # Load local root
 >>> bundle = MetadataBundle("path/to/metadata")
 >>>
->>> # state: "root", load more root versions from remote
+>>> # load more root versions from remote
 >>> with download("root", bundle.root.signed.version + 1) as f:
->>>     bundle.load_metadata(f.read())
+>>>     bundle.update_metadata(f.read())
 >>> with download("root", bundle.root.signed.version + 1) as f:
->>>     bundle.load_metadata(f.read())
+>>>     bundle.update_metadata(f.read())
 >>>
->>> # Finally, no more root from remote
+>>> # Finally, no more roots from remote
 >>> bundle.root_update_finished()
 >>>
->>> # state: "timestamp", load timestamp
+>>> # load local timestamp, then update it
+>>> bundle.load_local_metadata("timestamp")
 >>> with download("timestamp") as f:
->>>     bundle.load_metadata(f.read())
+>>>     bundle.update_metadata(f.read())
 >>>
->>> # state: "snapshot", load snapshot (consistent snapshot not shown)
->>> with download("snapshot") as f:
->>>     bundle.load_metadata(f.read())
+>>> # load local snapshot, then update it if needed
+>>> if not bundle.load_local_metadata("snapshot"):
+>>>     # load snapshot (consistent snapshot not shown)
+>>>     with download("snapshot") as f:
+>>>         bundle.update_metadata(f.read())
 >>>
->>> # state: "targets", load targets
->>> version = bundle.snapshot.signed.meta["targets.json"]["version"]
->>> with download("snapshot", version + 1) as f:
->>>     bundle.load_metadata(f.read())
+>>> # load local targets, then update it if needed
+>>> if not bundle.load_local_metadata("targets"):
+>>>     version = bundle.snapshot.signed.meta["targets.json"]["version"]
+>>>     with download("snapshot", version + 1) as f:
+>>>         bundle.update_metadata(f.read())
 >>>
 >>> # Top level metadata is now fully loaded and verified
 
@@ -102,7 +107,6 @@ def __init__(self, path: str):
         """
         self._path = path
         self._bundle = {}  # type: Dict[str: Metadata]
-        self._state = "root"
         self.reference_time = None
 
         if not os.path.exists(path):
@@ -110,41 +114,103 @@ def __init__(self, path: str):
             raise exceptions.RepositoryError("Repository does not exist")
 
         # Load and validate the local root metadata
-        # Valid root metadata is required (but invalid files are not removed)
-        try:
-            with open(os.path.join(self._path, "root.json"), "rb") as f:
-                self._load_intermediate_root(f.read())
-            logger.debug("Loaded local root.json")
-        except:
+        # Valid root metadata is required
+        if not self.load_local_metadata("root"):
             raise exceptions.RepositoryError("Failed to load local root metadata")
 
-    def update_metadata(self, metadata_str: str):
-        logger.debug("Updating %s", self._state)
-        if self._state == "root":
+    def load_local_metadata(self, role_name: str, delegator_name: str = None) -> bool:
+        """Loads metadata from local storage and inserts into bundle
+        
+        If bundle already contains 'role_name', nothing is loaded.
+        Failure to read the file, failure to parse it and failure to
+        load it as valid metadata will not raise exceptions: the function
+        will just fail.
+
+        Returns True if 'role_name' is now in the bundle
+        """
+        if self.get(role_name) is not None:
+            logger.debug("Already loaded %s.json", role_name)
+            return True
+
+        logger.debug("Loading local %s.json", role_name)
+
+        self._raise_on_unsupported_state(role_name)
+
+        try:
+            with open(os.path.join(self._path, f"{role_name}.json"), "rb") as f:
+                data = f.read()
+            
+            if role_name == "root":
+                self._load_intermediate_root(data)
+            elif role_name == "timestamp": 
+                self._load_timestamp(data)
+            elif role_name == "snapshot": 
+                self._load_snapshot(data)
+            elif role_name == "targets":
+                self._load_targets(data)
+            else:
+                self._load_delegate(data, delegator_name)
+
+            return True
+        except Exception as e:
+            # TODO only handle specific errors
+            logger.debug("Failed to load local %s.json", role_name)
+            # TODO delete local file (except probably should not delete root.json?)
+            return False
+
+    def update_metadata(self, metadata_str: str, role_name: str, delegator_name: str = None):
+        logger.debug("Updating %s", role_name)
+
+        self._raise_on_unsupported_state(role_name)
+
+        if role_name == "root":
             self._load_intermediate_root(metadata_str)
             self.root.to_file(os.path.join(self._path, "root.json"))
-        elif self._state == "timestamp":
+        elif role_name == "timestamp":
             self._load_timestamp(metadata_str)
             self.timestamp.to_file(os.path.join(self._path, "timestamp.json"))
-            self._state = "snapshot"
-        elif self._state == "snapshot":
+        elif role_name == "snapshot":
             self._load_snapshot(metadata_str)
             self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
-            self._state = "targets"
-        elif self._state == "targets":
+        elif role_name == "targets":
             self._load_targets(metadata_str)
             self.targets.to_file(os.path.join(self._path, "targets.json"))
-            self._state = ""
         else:
             raise NotImplementedError
 
     def root_update_finished(self):
-        if self._state != "root":
+        if self.timestamp is not None:
             # bundle does not support this order of ops
             raise exceptions.RepositoryError
 
-        self._make_root_permanent(self)
-        self._state = "timestamp"
+        # Store our reference "now", verify root expiry
+        self.reference_time = datetime.utcnow()
+        if self.root.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError
+
+        logger.debug("Verified final root.json")
+
+    def _raise_on_unsupported_state(self, role_name: str):
+        """Raise if updating 'role_name' is not supported at this state"""
+        if role_name == "root":
+            if self.timestamp is not None:
+                raise exceptions.RepositoryError
+        elif role_name == "timestamp":
+            if self.reference_time is None:
+                # root_update_finished() not called
+                raise exceptions.RepositoryError
+            if self.snapshot is not None:
+                raise exceptions.RepositoryError
+        elif role_name == "snapshot":
+            if self.targets is not None:
+                raise exceptions.RepositoryError
+        elif role_name == "targets":
+            if len(self) > 4:
+                # delegates have been loaded already
+                raise exceptions.RepositoryError
+        else:
+            if self.targets is None:
+                raise exceptions.RepositoryError
 
     # Implement Mapping
     def __getitem__(self, key: str):
@@ -201,48 +267,15 @@ def _load_intermediate_root(self, data: str):
 
         self._bundle["root"] = new_root
 
-    def _make_root_permanent(self):
-        # Store our reference "now", verify root expiry
-        self.reference_time = datetime.utcnow()
-        if self.root.signed.is_expired(self.reference_time):
-            raise exceptions.ExpiredMetadataError
-
-        logger.debug("Verified final root.json")
-
-        # Load remaning local metadata: this ensures invalid
-        # metadata gets wiped from disk
-        try:
-            with open(os.path.join(self._path, "timestamp.json"), "rb") as f:
-                self._load_timestamp(f.read())
-            logger.debug("Loaded local timestamp.json")
-        except Exception as e:
-            # TODO only handle specific errors
-            logger.debug("Failed to load local timestamp.json")
-            # TODO delete local file
-
-        try:
-            with open(os.path.join(self._path, "snapshot.json"), "rb") as f:
-                self._load_snapshot(f.read())
-            logger.debug("Loaded local snapshot.json")
-        except Exception as e:
-            # TODO only handle specific errors
-            logger.debug("Failed to load local snapshot.json")
-            # TODO delete local file
-
-        try:
-            with open(os.path.join(self._path, "targets.json"), "rb") as f:
-                self._load_targets(f.read())
-            logger.debug("Loaded local targets.json")
-        except Exception as e:
-            # TODO only handle specific errors
-            logger.debug("Failed to load local targets.json")
-            # TODO delete local file
-
     def _load_timestamp(self, data: str):
         """Verifies the new timestamp and uses it as current timestamp
 
         Raises if verification fails
         """
+        if self.root is None:
+            # bundle does not support this order of ops
+            raise exceptions.RepositoryError
+
         new_timestamp = from_string(data)
         if new_timestamp.signed._type != "timestamp":
             raise exceptions.RepositoryError
@@ -278,6 +311,10 @@ def _load_timestamp(self, data: str):
         self._bundle["timestamp"] = new_timestamp
 
     def _load_snapshot(self, data: str):
+        if self.root is None or self.timestamp is None:
+            # bundle does not support this order of ops
+            raise exceptions.RepositoryError
+
         # Verify against the hashes in timestamp, if any
         meta = self.timestamp.signed.meta["snapshot.json"]
         hashes = meta.get("hashes") or {}
@@ -319,6 +356,10 @@ def _load_snapshot(self, data: str):
         self._bundle["snapshot"] = new_snapshot
 
     def _load_targets(self, data: str):
+        if self.root is None or self.snapshot is None:
+            # bundle does not support this order of ops
+            raise exceptions.RepositoryError
+
         # Verify against the hashes in snapshot, if any
         meta = self.snapshot.signed.meta["targets.json"]
 
@@ -345,3 +386,7 @@ def _load_targets(self, data: str):
             raise exceptions.ExpiredMetadataError
 
         self._bundle["targets"] = new_targets
+
+
+    def _load_delegate(self, data: str, delegator_name: str = None):
+        pass
\ No newline at end of file

From 62284549eeab1d75ddb2a01d4a32e16a09ed3175 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Wed, 21 Apr 2021 15:41:12 +0300
Subject: [PATCH 04/33] MetadataBundle: implement delegates support

Also make sure we always try loading local data for the metadata
that gets security benefits from it.

Also update the threshold verification (placeholder) to support
targets as well as root.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 80 +++++++++++++++++++---------
 1 file changed, 56 insertions(+), 24 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index b4e9211d4c..f965ec9460 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -78,19 +78,34 @@
 
 # This is a placeholder until ...
 # TODO issue 1306: implement this in Metadata API
-def verify_with_threshold(root: Metadata, role: str, unverified: Metadata):
+def verify_with_threshold(delegator: Metadata, role_name: str, unverified: Metadata):
+    if delegator.signed._type == 'root':
+        keys = delegator.signed.keys
+        role = delegator.signed.roles.get(role_name)
+    elif delegator.signed._type == 'targets':
+        keys = delegator.signed.delegations["keys"]
+        # role names are unique: first match is enough
+        roles = delegator.signed.delegations["roles"]
+        role = next((role for role in roles if role["name"] == role_name), None)
+    else:
+        raise ValueError('Call is valid only on delegator metadata')
+
+    if role is None:
+        raise exceptions.UnknownRoleError
+
+    # verify that delegate is signed by correct threshold of unique keys
     unique_keys = set()
-    for keyid in root.signed.roles[role]["keyids"]:
-        key_metadata = root.signed.keys[keyid]
-        key, _ = sslib_keys.format_metadata_to_key(key_metadata)
+    for keyid in role["keyids"]:
+        key_metadata = keys[keyid]
+        key, dummy = sslib_keys.format_metadata_to_key(key_metadata)
 
         try:
             if unverified.verify(key):
                 unique_keys.add(key["keyval"]["public"])
-        except:
+        except:  # TODO specify the Exceptions
             pass
 
-    return len(unique_keys) >= root.signed.roles[role]["threshold"]
+    return len(unique_keys) >= role["threshold"]
 
 
 # TODO issue 1336: implement in metadata api
@@ -149,7 +164,7 @@ def load_local_metadata(self, role_name: str, delegator_name: str = None) -> boo
             elif role_name == "targets":
                 self._load_targets(data)
             else:
-                self._load_delegate(data, delegator_name)
+                self._load_delegated_targets(data, role_name, delegator_name)
 
             return True
         except Exception as e:
@@ -164,19 +179,23 @@ def update_metadata(self, metadata_str: str, role_name: str, delegator_name: str
         self._raise_on_unsupported_state(role_name)
 
         if role_name == "root":
+            self.load_local_metadata("root")
             self._load_intermediate_root(metadata_str)
             self.root.to_file(os.path.join(self._path, "root.json"))
         elif role_name == "timestamp":
+            self.load_local_metadata("timestamp")
             self._load_timestamp(metadata_str)
             self.timestamp.to_file(os.path.join(self._path, "timestamp.json"))
         elif role_name == "snapshot":
+            self.load_local_metadata("snapshot")
             self._load_snapshot(metadata_str)
             self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
         elif role_name == "targets":
             self._load_targets(metadata_str)
             self.targets.to_file(os.path.join(self._path, "targets.json"))
         else:
-            raise NotImplementedError
+            self._load_delegated_targets(metadata_str, role_name, delegator_name)
+            self[role_name].to_file(os.path.join(self._path, f"{role_name}.json"))
 
     def root_update_finished(self):
         if self.timestamp is not None:
@@ -202,13 +221,17 @@ def _raise_on_unsupported_state(self, role_name: str):
             if self.snapshot is not None:
                 raise exceptions.RepositoryError
         elif role_name == "snapshot":
+            if self.timestamp is None:
+                raise exceptions.RepositoryError
             if self.targets is not None:
                 raise exceptions.RepositoryError
         elif role_name == "targets":
+            if self.snapshot is None:
+                raise exceptions.RepositoryError
             if len(self) > 4:
                 # delegates have been loaded already
                 raise exceptions.RepositoryError
-        else:
+        else: # delegated role
             if self.targets is None:
                 raise exceptions.RepositoryError
 
@@ -266,6 +289,7 @@ def _load_intermediate_root(self, data: str):
             )
 
         self._bundle["root"] = new_root
+        logger.debug("Loaded root")
 
     def _load_timestamp(self, data: str):
         """Verifies the new timestamp and uses it as current timestamp
@@ -309,6 +333,7 @@ def _load_timestamp(self, data: str):
             raise exceptions.ExpiredMetadataError
 
         self._bundle["timestamp"] = new_timestamp
+        logger.debug("Loaded timestamp")
 
     def _load_snapshot(self, data: str):
         if self.root is None or self.timestamp is None:
@@ -316,7 +341,10 @@ def _load_snapshot(self, data: str):
             raise exceptions.RepositoryError
 
         # Verify against the hashes in timestamp, if any
-        meta = self.timestamp.signed.meta["snapshot.json"]
+        meta = self.timestamp.signed.meta.get("snapshot.json")
+        if meta is None:
+            raise exceptions.RepositoryError
+
         hashes = meta.get("hashes") or {}
         for algo, _hash in meta["hashes"].items():
             digest_object = sslib_hash.digest(algo)
@@ -354,14 +382,21 @@ def _load_snapshot(self, data: str):
             raise exceptions.ExpiredMetadataError
 
         self._bundle["snapshot"] = new_snapshot
+        logger.debug("Loaded snapshot")
 
     def _load_targets(self, data: str):
-        if self.root is None or self.snapshot is None:
-            # bundle does not support this order of ops
+        self._load_delegated_targets(data, "targets", "root")
+
+    def _load_delegated_targets(self, data: str, role_name: str, delegator_name: str):
+        logger.debug(f"Loading {role_name} delegated by {delegator_name}")
+        delegator = self.get(delegator_name)
+        if delegator == None:
             raise exceptions.RepositoryError
 
         # Verify against the hashes in snapshot, if any
-        meta = self.snapshot.signed.meta["targets.json"]
+        meta = self.snapshot.signed.meta.get(f"{role_name}.json")
+        if meta is None:
+            raise exceptions.RepositoryError
 
         hashes = meta.get("hashes") or {}
         for algo, _hash in hashes.items():
@@ -370,23 +405,20 @@ def _load_targets(self, data: str):
             if digest_object.hexdigest() != _hash:
                 raise exceptions.BadHashError()
 
-        new_targets = from_string(data)
-        if new_targets.signed._type != "targets":
+        new_delegate = from_string(data)
+        if new_delegate.signed._type != "targets":
             raise exceptions.RepositoryError
 
-        if not verify_with_threshold(self.root, "targets", new_targets):
+        if not verify_with_threshold(delegator, role_name, new_delegate):
             raise exceptions.UnsignedMetadataError(
-                "New targets is not signed by root", new_targets.signed
+                f"New {role_name} is not signed by {delegator_name}"
             )
 
-        if new_targets.signed.version != meta["version"]:
+        if new_delegate.signed.version != meta["version"]:
             raise exceptions.BadVersionNumberError
 
-        if new_targets.signed.is_expired(self.reference_time):
+        if new_delegate.signed.is_expired(self.reference_time):
             raise exceptions.ExpiredMetadataError
 
-        self._bundle["targets"] = new_targets
-
-
-    def _load_delegate(self, data: str, delegator_name: str = None):
-        pass
\ No newline at end of file
+        self._bundle[role_name] = new_delegate
+        logger.debug("Loaded {role_name}")

From f503ebeb17326d3d224c6a571e35edf19f6d1dfc Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Wed, 21 Apr 2021 15:52:19 +0300
Subject: [PATCH 05/33] MetadataBundle: use Metadata.from_bytes()

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 40 +++++++++++-----------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index f965ec9460..978d12c9ed 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -108,14 +108,6 @@ def verify_with_threshold(delegator: Metadata, role_name: str, unverified: Metad
     return len(unique_keys) >= role["threshold"]
 
 
-# TODO issue 1336: implement in metadata api
-from tuf.api.serialization.json import JSONDeserializer
-
-
-def from_string(data: str) -> Metadata:
-    return JSONDeserializer().deserialize(data)
-
-
 class MetadataBundle(abc.Mapping):
     def __init__(self, path: str):
         """Initialize by loading root metadata from disk
@@ -144,7 +136,7 @@ def load_local_metadata(self, role_name: str, delegator_name: str = None) -> boo
         Returns True if 'role_name' is now in the bundle
         """
         if self.get(role_name) is not None:
-            logger.debug("Already loaded %s.json", role_name)
+            logger.debug("Already loaded local %s.json", role_name)
             return True
 
         logger.debug("Loading local %s.json", role_name)
@@ -173,28 +165,28 @@ def load_local_metadata(self, role_name: str, delegator_name: str = None) -> boo
             # TODO delete local file (except probably should not delete root.json?)
             return False
 
-    def update_metadata(self, metadata_str: str, role_name: str, delegator_name: str = None):
+    def update_metadata(self, data: bytes, role_name: str, delegator_name: str = None):
         logger.debug("Updating %s", role_name)
 
         self._raise_on_unsupported_state(role_name)
 
         if role_name == "root":
             self.load_local_metadata("root")
-            self._load_intermediate_root(metadata_str)
+            self._load_intermediate_root(data)
             self.root.to_file(os.path.join(self._path, "root.json"))
         elif role_name == "timestamp":
             self.load_local_metadata("timestamp")
-            self._load_timestamp(metadata_str)
+            self._load_timestamp(data)
             self.timestamp.to_file(os.path.join(self._path, "timestamp.json"))
         elif role_name == "snapshot":
             self.load_local_metadata("snapshot")
-            self._load_snapshot(metadata_str)
+            self._load_snapshot(data)
             self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
         elif role_name == "targets":
-            self._load_targets(metadata_str)
+            self._load_targets(data)
             self.targets.to_file(os.path.join(self._path, "targets.json"))
         else:
-            self._load_delegated_targets(metadata_str, role_name, delegator_name)
+            self._load_delegated_targets(data, role_name, delegator_name)
             self[role_name].to_file(os.path.join(self._path, f"{role_name}.json"))
 
     def root_update_finished(self):
@@ -262,12 +254,12 @@ def snapshot(self):
     def targets(self):
         return self._bundle.get("targets")
 
-    def _load_intermediate_root(self, data: str):
+    def _load_intermediate_root(self, data: bytes):
         """Verify the new root using current root (if any) and use it as current root
 
         Raises if root fails verification
         """
-        new_root = from_string(data)
+        new_root = Metadata.from_bytes(data)
         if new_root.signed._type != "root":
             raise exceptions.RepositoryError
 
@@ -291,7 +283,7 @@ def _load_intermediate_root(self, data: str):
         self._bundle["root"] = new_root
         logger.debug("Loaded root")
 
-    def _load_timestamp(self, data: str):
+    def _load_timestamp(self, data: bytes):
         """Verifies the new timestamp and uses it as current timestamp
 
         Raises if verification fails
@@ -300,7 +292,7 @@ def _load_timestamp(self, data: str):
             # bundle does not support this order of ops
             raise exceptions.RepositoryError
 
-        new_timestamp = from_string(data)
+        new_timestamp = Metadata.from_bytes(data)
         if new_timestamp.signed._type != "timestamp":
             raise exceptions.RepositoryError
 
@@ -335,7 +327,7 @@ def _load_timestamp(self, data: str):
         self._bundle["timestamp"] = new_timestamp
         logger.debug("Loaded timestamp")
 
-    def _load_snapshot(self, data: str):
+    def _load_snapshot(self, data: bytes):
         if self.root is None or self.timestamp is None:
             # bundle does not support this order of ops
             raise exceptions.RepositoryError
@@ -351,7 +343,7 @@ def _load_snapshot(self, data: str):
             digest_object.update(data)
             if digest_object.hexdigest() != _hash:
                 raise exceptions.BadHashError()
-        new_snapshot = from_string(data)
+        new_snapshot = Metadata.from_bytes(data)
         if new_snapshot.signed._type != "snapshot":
             raise exceptions.RepositoryError
 
@@ -384,10 +376,10 @@ def _load_snapshot(self, data: str):
         self._bundle["snapshot"] = new_snapshot
         logger.debug("Loaded snapshot")
 
-    def _load_targets(self, data: str):
+    def _load_targets(self, data: bytes):
         self._load_delegated_targets(data, "targets", "root")
 
-    def _load_delegated_targets(self, data: str, role_name: str, delegator_name: str):
+    def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: str):
         logger.debug(f"Loading {role_name} delegated by {delegator_name}")
         delegator = self.get(delegator_name)
         if delegator == None:
@@ -405,7 +397,7 @@ def _load_delegated_targets(self, data: str, role_name: str, delegator_name: str
             if digest_object.hexdigest() != _hash:
                 raise exceptions.BadHashError()
 
-        new_delegate = from_string(data)
+        new_delegate = Metadata.from_bytes(data)
         if new_delegate.signed._type != "targets":
             raise exceptions.RepositoryError
 

From c1afe57ae846e34237d4acef7e1fa9a88ab9504f Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Thu, 22 Apr 2021 10:13:02 +0300
Subject: [PATCH 06/33] MetadataBundle: Require load_local_metadata()

Change the rules a bit: require calling load_local_metadata() before
update_metadata() is legal. This removes the need to run
load_local_metadata() "just to be sure" in update_metadata()

Improve comments

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 76 ++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 21 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 978d12c9ed..65645d67dd 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -9,13 +9,23 @@
 care of persisting valid metadata on disk, loading local metadata from disk
 and deleting invalid local metadata.
 
-New metadata (downloaded from a remote repository) can be loaded using
-'update_metadata()'. The type of accepted metadata depends on bundle state:
- * Metadata is loadable only if all metadata it depends on is loaded
- * Metadata is immutable if any metadata depending on it has been loaded 
+Loaded metadata can be accessed via the index access with rolename as key
+or, in the case of top-level metadata using the helper properties like
+'MetadataBundle.root'
+
+Metadata can be loaded into bundle by two means:
+ * loading from local storage: load_local_metadata()
+   (and, in the case of root metadata, the constuctor)
+ * updating from remote repository: update_metadata()
+
+The rules for top-level metadata are
+ * Metadata is loadable only if metadata it depends on is loaded
+ * Metadata is immutable if any metadata depending on it has been loaded
+ * Loading from local storage must be attempted before updating from remote
+ * Updating from remote is never required
 
 Exceptions are raised if metadata fails to load in any way (except in the
-case of local loads -- see locad_local_metadata()).
+case of local loads -- see load_local_metadata()).
 
 Example (with hypothetical download function):
 
@@ -52,14 +62,24 @@
 
 
 TODO:
- * Delegated targets not implement yet
+ * Delegated targets are implemented but they are not covered
+   by same immutability guarantees: the top-level metadata is handled
+   by hard-coded rules (can't update root if snapshot is loaded)
+   but delegations would require storing the delegation tree ...
  * exceptions are all over the place and not thought out at all
+ * usefulness of root_update_finished() can be debated: it could be done
+   in the beginning of _load_timestamp()...
+ * there are some divergences from spec:
+   * 5.3.11: timestamp and snapshot are not deleted right away (only on next load):
+     the load functions will refuse to load the files when they are not signed by
+     current root keys. Deleting at the specified point is possible but means additional
+     code with some quirks..
  * a bit of repetition
  * No tests!
  * Naming maybe not final?
  * some metadata interactions might work better in Metadata itself
  * Progress through Specification update process should be documented
-   (not sure yet how)
+   (not sure yet how: maybe a spec_logger that logs specification events?)
 """
 
 from collections import abc
@@ -79,16 +99,16 @@
 # This is a placeholder until ...
 # TODO issue 1306: implement this in Metadata API
 def verify_with_threshold(delegator: Metadata, role_name: str, unverified: Metadata):
-    if delegator.signed._type == 'root':
+    if delegator.signed._type == "root":
         keys = delegator.signed.keys
         role = delegator.signed.roles.get(role_name)
-    elif delegator.signed._type == 'targets':
+    elif delegator.signed._type == "targets":
         keys = delegator.signed.delegations["keys"]
         # role names are unique: first match is enough
         roles = delegator.signed.delegations["roles"]
         role = next((role for role in roles if role["name"] == role_name), None)
     else:
-        raise ValueError('Call is valid only on delegator metadata')
+        raise ValueError("Call is valid only on delegator metadata")
 
     if role is None:
         raise exceptions.UnknownRoleError
@@ -110,10 +130,10 @@ def verify_with_threshold(delegator: Metadata, role_name: str, unverified: Metad
 
 class MetadataBundle(abc.Mapping):
     def __init__(self, path: str):
-        """Initialize by loading root metadata from disk
-        """
+        """Initialize by loading root metadata from disk"""
         self._path = path
         self._bundle = {}  # type: Dict[str: Metadata]
+        self._local_load_attempted = {}
         self.reference_time = None
 
         if not os.path.exists(path):
@@ -127,12 +147,14 @@ def __init__(self, path: str):
 
     def load_local_metadata(self, role_name: str, delegator_name: str = None) -> bool:
         """Loads metadata from local storage and inserts into bundle
-        
+
         If bundle already contains 'role_name', nothing is loaded.
         Failure to read the file, failure to parse it and failure to
         load it as valid metadata will not raise exceptions: the function
         will just fail.
 
+        Raises if 'role_name' cannot be loaded from local storage at this state
+
         Returns True if 'role_name' is now in the bundle
         """
         if self.get(role_name) is not None:
@@ -142,16 +164,17 @@ def load_local_metadata(self, role_name: str, delegator_name: str = None) -> boo
         logger.debug("Loading local %s.json", role_name)
 
         self._raise_on_unsupported_state(role_name)
+        self._local_load_attempted[role_name] = True
 
         try:
             with open(os.path.join(self._path, f"{role_name}.json"), "rb") as f:
                 data = f.read()
-            
+
             if role_name == "root":
                 self._load_intermediate_root(data)
-            elif role_name == "timestamp": 
+            elif role_name == "timestamp":
                 self._load_timestamp(data)
-            elif role_name == "snapshot": 
+            elif role_name == "snapshot":
                 self._load_snapshot(data)
             elif role_name == "targets":
                 self._load_targets(data)
@@ -166,20 +189,26 @@ def load_local_metadata(self, role_name: str, delegator_name: str = None) -> boo
             return False
 
     def update_metadata(self, data: bytes, role_name: str, delegator_name: str = None):
+        """Takes new metadata (from remote repository) and loads it into bundle
+
+        Raises if 'role_name' cannot be update from remote at this state
+        Raises if 'data' cannot be parsed or validated
+        Raises if the new metadata cannot be verified by the bundle
+        """
         logger.debug("Updating %s", role_name)
 
         self._raise_on_unsupported_state(role_name)
 
+        if not self._local_load_attempted.get(role_name):
+            raise exceptions.RepositoryError
+
         if role_name == "root":
-            self.load_local_metadata("root")
             self._load_intermediate_root(data)
             self.root.to_file(os.path.join(self._path, "root.json"))
         elif role_name == "timestamp":
-            self.load_local_metadata("timestamp")
             self._load_timestamp(data)
             self.timestamp.to_file(os.path.join(self._path, "timestamp.json"))
         elif role_name == "snapshot":
-            self.load_local_metadata("snapshot")
             self._load_snapshot(data)
             self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
         elif role_name == "targets":
@@ -190,6 +219,11 @@ def update_metadata(self, data: bytes, role_name: str, delegator_name: str = Non
             self[role_name].to_file(os.path.join(self._path, f"{role_name}.json"))
 
     def root_update_finished(self):
+        """Marks root update as finished, validates the root metadata
+
+        Raises if root update is not a valid operation at this state
+        Raises if validation fails
+        """
         if self.timestamp is not None:
             # bundle does not support this order of ops
             raise exceptions.RepositoryError
@@ -223,7 +257,7 @@ def _raise_on_unsupported_state(self, role_name: str):
             if len(self) > 4:
                 # delegates have been loaded already
                 raise exceptions.RepositoryError
-        else: # delegated role
+        else:  # delegated role
             if self.targets is None:
                 raise exceptions.RepositoryError
 
@@ -413,4 +447,4 @@ def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: s
             raise exceptions.ExpiredMetadataError
 
         self._bundle[role_name] = new_delegate
-        logger.debug("Loaded {role_name}")
+        logger.debug(f"Loaded {role_name}")

From e7a0febe1bfe28cccb18fb65037a00dfa1050b16 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Thu, 22 Apr 2021 18:42:33 +0300
Subject: [PATCH 07/33] Improve loading rules checks

Make sure we do not load any metadata if delegates of
that metadata are already loaded.

Also remove duplicate checks.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 65645d67dd..e11c45e13f 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -237,9 +237,12 @@ def root_update_finished(self):
 
     def _raise_on_unsupported_state(self, role_name: str):
         """Raise if updating 'role_name' is not supported at this state"""
+
+        # Special rules for top-level roles. We want to enforce a strict order
+        # root->snapshot->timestamp->targets where loading a metadata is no
+        # longer allowed when the next metadata in the order has been loaded
         if role_name == "root":
-            if self.timestamp is not None:
-                raise exceptions.RepositoryError
+            pass
         elif role_name == "timestamp":
             if self.reference_time is None:
                 # root_update_finished() not called
@@ -254,13 +257,19 @@ def _raise_on_unsupported_state(self, role_name: str):
         elif role_name == "targets":
             if self.snapshot is None:
                 raise exceptions.RepositoryError
-            if len(self) > 4:
-                # delegates have been loaded already
-                raise exceptions.RepositoryError
         else:  # delegated role
             if self.targets is None:
                 raise exceptions.RepositoryError
 
+        # Generic rule: Updating a role is not allowed if
+        #  * role is already loaded AND
+        #  * role has a delegate that is already loaded
+        role = self.get(role_name)
+        if role is not None and role.signed.delegations is not None:
+            for delegate in role.signed.delegations["roles"]:
+                if self.get(delegate["name"]) is not None:
+                    raise exceptions.RepositoryError
+
     # Implement Mapping
     def __getitem__(self, key: str):
         return self._bundle[key]
@@ -322,10 +331,6 @@ def _load_timestamp(self, data: bytes):
 
         Raises if verification fails
         """
-        if self.root is None:
-            # bundle does not support this order of ops
-            raise exceptions.RepositoryError
-
         new_timestamp = Metadata.from_bytes(data)
         if new_timestamp.signed._type != "timestamp":
             raise exceptions.RepositoryError
@@ -362,9 +367,6 @@ def _load_timestamp(self, data: bytes):
         logger.debug("Loaded timestamp")
 
     def _load_snapshot(self, data: bytes):
-        if self.root is None or self.timestamp is None:
-            # bundle does not support this order of ops
-            raise exceptions.RepositoryError
 
         # Verify against the hashes in timestamp, if any
         meta = self.timestamp.signed.meta.get("snapshot.json")

From cadbd844500822965346bb09814f37ef43ca7654 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Thu, 22 Apr 2021 19:08:50 +0300
Subject: [PATCH 08/33] Comment and variable name improvements

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index e11c45e13f..dcd16ca99a 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -62,10 +62,6 @@
 
 
 TODO:
- * Delegated targets are implemented but they are not covered
-   by same immutability guarantees: the top-level metadata is handled
-   by hard-coded rules (can't update root if snapshot is loaded)
-   but delegations would require storing the delegation tree ...
  * exceptions are all over the place and not thought out at all
  * usefulness of root_update_finished() can be debated: it could be done
    in the beginning of _load_timestamp()...
@@ -129,14 +125,14 @@ def verify_with_threshold(delegator: Metadata, role_name: str, unverified: Metad
 
 
 class MetadataBundle(abc.Mapping):
-    def __init__(self, path: str):
+    def __init__(self, repository_path: str):
         """Initialize by loading root metadata from disk"""
-        self._path = path
+        self._path = repository_path
         self._bundle = {}  # type: Dict[str: Metadata]
         self._local_load_attempted = {}
         self.reference_time = None
 
-        if not os.path.exists(path):
+        if not os.path.exists(self._path):
             # TODO try to create dir instead?
             raise exceptions.RepositoryError("Repository does not exist")
 
@@ -353,7 +349,7 @@ def _load_timestamp(self, data: bytes):
                 new_timestamp.signed.meta["snapshot.json"]["version"]
                 < self.timestamp.signed.meta["snapshot.json"]["version"]
             ):
-                # TODO not sure about the
+                # TODO not sure about the correct exception here
                 raise exceptions.ReplayedMetadataError(
                     "snapshot",
                     new_timestamp.signed.meta["snapshot.json"]["version"],

From 7e457ec98c0b9f831f9504f5398311759a08d830 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Tue, 27 Apr 2021 11:26:42 +0300
Subject: [PATCH 09/33] Exceptions refactoring

Make more exceptions derive from RepositoryError:
The idea is that a client can just handle RepositoryError and will know
that any problems resulting from unexpected metadata will end up there.

Also fix some wildly misleading variable naming in ReplayedMetadataError

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/exceptions.py | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/tuf/exceptions.py b/tuf/exceptions.py
index baed5446eb..c5f795e0c6 100755
--- a/tuf/exceptions.py
+++ b/tuf/exceptions.py
@@ -89,12 +89,6 @@ def __repr__(self):
     #     repr(self.observed_hash) + ')')
 
 
-
-
-class BadVersionNumberError(Error):
-  """Indicate an error for metadata that contains an invalid version number."""
-
-
 class BadPasswordError(Error):
   """Indicate an error after encountering an invalid password."""
 
@@ -107,6 +101,10 @@ class RepositoryError(Error):
   """Indicate an error with a repository's state, such as a missing file."""
 
 
+class BadVersionNumberError(RepositoryError):
+  """Indicate an error for metadata that contains an invalid version number."""
+
+
 class MissingLocalRepositoryError(RepositoryError):
   """Raised when a local repository could not be found."""
 
@@ -119,36 +117,29 @@ class ForbiddenTargetError(RepositoryError):
   """Indicate that a role signed for a target that it was not delegated to."""
 
 
-class ExpiredMetadataError(Error):
+class ExpiredMetadataError(RepositoryError):
   """Indicate that a TUF Metadata file has expired."""
 
 
 class ReplayedMetadataError(RepositoryError):
   """Indicate that some metadata has been replayed to the client."""
 
-  def __init__(self, metadata_role, previous_version, current_version):
+  def __init__(self, metadata_role, downloaded_version, current_version):
     super(ReplayedMetadataError, self).__init__()
 
     self.metadata_role = metadata_role
-    self.previous_version = previous_version
+    self.downloaded_version = downloaded_version
     self.current_version = current_version
 
-
   def __str__(self):
     return (
         'Downloaded ' + repr(self.metadata_role) + ' is older (' +
-        repr(self.previous_version) + ') than the version currently '
+        repr(self.downloaded_version) + ') than the version currently '
         'installed (' + repr(self.current_version) + ').')
 
   def __repr__(self):
     return self.__class__.__name__ + ' : ' + str(self)
 
-    # # Directly instance-reproducing:
-    # return (
-    #     self.__class__.__name__ + '(' + repr(self.metadata_role) + ', ' +
-    #     repr(self.previous_version) + ', ' + repr(self.current_version) + ')')
-
-
 
 class CryptoError(Error):
   """Indicate any cryptography-related errors."""
@@ -250,7 +241,7 @@ class InvalidNameError(Error):
   """Indicate an error while trying to validate any type of named object."""
 
 
-class UnsignedMetadataError(Error):
+class UnsignedMetadataError(RepositoryError):
   """Indicate metadata object with insufficient threshold of signatures."""
 
   def __init__(self, message, signable):

From 53f5ccb58a752dc6917b1c57ecb44a24cc8eefdd Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Tue, 27 Apr 2021 12:20:20 +0300
Subject: [PATCH 10/33] MetadataBundle: Exception refactor

The goal is that the bundle only raises two kinds of errors:
* user errors (ValueError/KeyError) that can be avoided by
  the caller
* RepositoryErrors that are a result of unacceptable metadata:
  The requested action cannot succeed because of the metadata.
  These typically cannot be avoided by the caller.

File open and serialization errors are handled internally.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 134 ++++++++++++++++++---------
 1 file changed, 92 insertions(+), 42 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index dcd16ca99a..f41b1d8e1d 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -89,6 +89,7 @@
 
 from tuf import exceptions
 from tuf.api.metadata import Metadata
+from tuf.api.serialization import SerializationError
 
 logger = logging.getLogger(__name__)
 
@@ -178,9 +179,8 @@ def load_local_metadata(self, role_name: str, delegator_name: str = None) -> boo
                 self._load_delegated_targets(data, role_name, delegator_name)
 
             return True
-        except Exception as e:
-            # TODO only handle specific errors
-            logger.debug("Failed to load local %s.json", role_name)
+        except (OSError, exceptions.RepositoryError) as e:
+            logger.debug("Failed to load local %s: %s", role_name, e)
             # TODO delete local file (except probably should not delete root.json?)
             return False
 
@@ -196,7 +196,9 @@ def update_metadata(self, data: bytes, role_name: str, delegator_name: str = Non
         self._raise_on_unsupported_state(role_name)
 
         if not self._local_load_attempted.get(role_name):
-            raise exceptions.RepositoryError
+            raise ValueError(
+                f"Cannot update {role_name} before loading local metadata"
+            )
 
         if role_name == "root":
             self._load_intermediate_root(data)
@@ -220,14 +222,13 @@ def root_update_finished(self):
         Raises if root update is not a valid operation at this state
         Raises if validation fails
         """
-        if self.timestamp is not None:
-            # bundle does not support this order of ops
-            raise exceptions.RepositoryError
+        if self.timestamp is None:
+            raise ValueError("Root update is already finished")
 
         # Store our reference "now", verify root expiry
         self.reference_time = datetime.utcnow()
         if self.root.signed.is_expired(self.reference_time):
-            raise exceptions.ExpiredMetadataError
+            raise exceptions.ExpiredMetadataError("New root.json is expired")
 
         logger.debug("Verified final root.json")
 
@@ -242,20 +243,22 @@ def _raise_on_unsupported_state(self, role_name: str):
         elif role_name == "timestamp":
             if self.reference_time is None:
                 # root_update_finished() not called
-                raise exceptions.RepositoryError
+                raise ValueError("Cannot load timestamp before root")
             if self.snapshot is not None:
-                raise exceptions.RepositoryError
+                raise ValueError("Cannot load timestamp after snapshot")
         elif role_name == "snapshot":
             if self.timestamp is None:
-                raise exceptions.RepositoryError
+                raise ValueError("Cannot load snapshot before timestamp")
             if self.targets is not None:
-                raise exceptions.RepositoryError
+                raise ValueError("Cannot load snapshot after targets")
         elif role_name == "targets":
             if self.snapshot is None:
-                raise exceptions.RepositoryError
-        else:  # delegated role
+                raise ValueError("Cannot load targets before snapshot")
+        else:
             if self.targets is None:
-                raise exceptions.RepositoryError
+                raise ValueError(
+                    "Cannot load delegated targets before targets"
+                )
 
         # Generic rule: Updating a role is not allowed if
         #  * role is already loaded AND
@@ -263,8 +266,12 @@ def _raise_on_unsupported_state(self, role_name: str):
         role = self.get(role_name)
         if role is not None and role.signed.delegations is not None:
             for delegate in role.signed.delegations["roles"]:
-                if self.get(delegate["name"]) is not None:
-                    raise exceptions.RepositoryError
+                delegate_name = delegate["name"]
+                if self.get(delegate_name) is not None:
+                    raise ValueError(
+                        f"Cannot load {role_name} after delegate"
+                        f"{delegate_name}"
+                    )
 
     # Implement Mapping
     def __getitem__(self, key: str):
@@ -298,9 +305,15 @@ def _load_intermediate_root(self, data: bytes):
 
         Raises if root fails verification
         """
-        new_root = Metadata.from_bytes(data)
+        try:
+            new_root = Metadata.from_bytes(data)
+        except SerializationError as e:
+            raise exceptions.RepositoryError("Failed to load root") from e
+
         if new_root.signed._type != "root":
-            raise exceptions.RepositoryError
+            raise exceptions.RepositoryError(
+                f"Expected 'root', got '{new_root.signed._type}'"
+            )
 
         if self.root is not None:
             if not verify_with_threshold(self.root, "root", new_root):
@@ -327,9 +340,15 @@ def _load_timestamp(self, data: bytes):
 
         Raises if verification fails
         """
-        new_timestamp = Metadata.from_bytes(data)
+        try:
+            new_timestamp = Metadata.from_bytes(data)
+        except SerializationError as e:
+            raise exceptions.RepositoryError("Failed to load timestamp") from e
+
         if new_timestamp.signed._type != "timestamp":
-            raise exceptions.RepositoryError
+            raise exceptions.RepositoryError(
+                f"Expected 'timestamp', got '{new_timestamp.signed._type}'"
+            )
 
         if not verify_with_threshold(self.root, "timestamp", new_timestamp):
             raise exceptions.UnsignedMetadataError(
@@ -357,27 +376,33 @@ def _load_timestamp(self, data: bytes):
                 )
 
         if new_timestamp.signed.is_expired(self.reference_time):
-            raise exceptions.ExpiredMetadataError
+            raise exceptions.ExpiredMetadataError("New timestamp is expired")
 
         self._bundle["timestamp"] = new_timestamp
         logger.debug("Loaded timestamp")
 
     def _load_snapshot(self, data: bytes):
 
-        # Verify against the hashes in timestamp, if any
-        meta = self.timestamp.signed.meta.get("snapshot.json")
-        if meta is None:
-            raise exceptions.RepositoryError
+        meta = self.timestamp.signed.meta["snapshot.json"]
 
+        # Verify against the hashes in timestamp, if any
         hashes = meta.get("hashes") or {}
         for algo, _hash in meta["hashes"].items():
             digest_object = sslib_hash.digest(algo)
             digest_object.update(data)
-            if digest_object.hexdigest() != _hash:
-                raise exceptions.BadHashError()
-        new_snapshot = Metadata.from_bytes(data)
+            observed_hash = digest_object.hexdigest()
+            if observed_hash != _hash:
+                raise exceptions.BadHashError(_hash, observed_hash)
+
+        try:
+            new_snapshot = Metadata.from_bytes(data)
+        except SerializationError as e:
+            raise exceptions.RepositoryError("Failed to load snapshot") from e
+
         if new_snapshot.signed._type != "snapshot":
-            raise exceptions.RepositoryError
+            raise exceptions.RepositoryError(
+                f"Expected 'snapshot', got '{new_snapshot.signed._type}'"
+            )
 
         if not verify_with_threshold(self.root, "snapshot", new_snapshot):
             raise exceptions.UnsignedMetadataError(
@@ -388,7 +413,11 @@ def _load_snapshot(self, data: bytes):
             new_snapshot.signed.version
             != self.timestamp.signed.meta["snapshot.json"]["version"]
         ):
-            raise exceptions.BadVersionNumberError
+            raise exceptions.BadVersionNumberError(
+                f"Expected snapshot version"
+                f"{self.timestamp.signed.meta['snapshot.json']['version']},"
+                f"got {new_snapshot.signed.version}"
+            )
 
         if self.snapshot:
             for filename, fileinfo in self.snapshot.signed.meta.items():
@@ -396,14 +425,19 @@ def _load_snapshot(self, data: bytes):
 
                 # Prevent removal of any metadata in meta
                 if new_fileinfo is None:
-                    raise exceptions.ReplayedMetadataError
+                    raise exceptions.RepositoryError(
+                        f"New snapshot is missing info for '{filename}'"
+                    )
 
                 # Prevent rollback of any metadata versions
                 if new_fileinfo["version"] < fileinfo["version"]:
-                    raise exceptions.ReplayedMetadataError
+                    raise exceptions.BadVersionNumberError(
+                        f"Expected {filename} version"
+                        f"{new_fileinfo['version']}, got {fileinfo['version']}"
+                    )
 
         if new_snapshot.signed.is_expired(self.reference_time):
-            raise exceptions.ExpiredMetadataError
+            raise exceptions.ExpiredMetadataError("New snapshot is expired")
 
         self._bundle["snapshot"] = new_snapshot
         logger.debug("Loaded snapshot")
@@ -413,25 +447,38 @@ def _load_targets(self, data: bytes):
 
     def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: str):
         logger.debug(f"Loading {role_name} delegated by {delegator_name}")
+
         delegator = self.get(delegator_name)
+        # TODO this check could maybe be done in _raise_on_unspported_state
         if delegator == None:
-            raise exceptions.RepositoryError
+            raise exceptions.ValueError(
+                "Cannot load delegated target before delegator"
+            )
 
         # Verify against the hashes in snapshot, if any
         meta = self.snapshot.signed.meta.get(f"{role_name}.json")
         if meta is None:
-            raise exceptions.RepositoryError
+            raise exceptions.RepositoryError(
+                f"Snapshot does not contain information for '{role_name}'"
+            )
 
         hashes = meta.get("hashes") or {}
         for algo, _hash in hashes.items():
             digest_object = sslib_hash.digest(algo)
             digest_object.update(data)
-            if digest_object.hexdigest() != _hash:
-                raise exceptions.BadHashError()
+            observed_hash = digest_object.hexdigest()
+            if observed_hash != _hash:
+                raise exceptions.BadHashError(_hash, observed_hash)
+
+        try:
+            new_delegate = Metadata.from_bytes(data)
+        except SerializationError as e:
+            raise exceptions.RepositoryError("Failed to load snapshot") from e
 
-        new_delegate = Metadata.from_bytes(data)
         if new_delegate.signed._type != "targets":
-            raise exceptions.RepositoryError
+            raise exceptions.RepositoryError(
+                f"Expected 'targets', got '{new_delegate.signed._type}'"
+            )
 
         if not verify_with_threshold(delegator, role_name, new_delegate):
             raise exceptions.UnsignedMetadataError(
@@ -439,10 +486,13 @@ def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: s
             )
 
         if new_delegate.signed.version != meta["version"]:
-            raise exceptions.BadVersionNumberError
+            raise exceptions.BadVersionNumberError(
+                        f"Expected {role_name} version"
+                        f"{meta['version']}, got {new_delegate.signed.version}"
+            )
 
         if new_delegate.signed.is_expired(self.reference_time):
-            raise exceptions.ExpiredMetadataError
+            raise exceptions.ExpiredMetadataError(f"New {role_name} is expired")
 
         self._bundle[role_name] = new_delegate
         logger.debug(f"Loaded {role_name}")

From 71793b61ecfda8c53d8c828d31c354d22ebc132a Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Tue, 4 May 2021 19:23:36 +0300
Subject: [PATCH 11/33] Update MetadataBundle with named methods

Use named methods for the top-level metadata (e.g.
load_local_timestamp() instead of load_local_metadata("timestamp")).
This can now happen after the mirrors are no longer in play (so
updater can easily call named methods) and makes the bundle
implementation more straight-forward.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 316 +++++++++++++--------------
 1 file changed, 151 insertions(+), 165 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index f41b1d8e1d..3ebb187d5b 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -13,63 +13,65 @@
 or, in the case of top-level metadata using the helper properties like
 'MetadataBundle.root'
 
-Metadata can be loaded into bundle by two means:
- * loading from local storage: load_local_metadata()
-   (and, in the case of root metadata, the constuctor)
- * updating from remote repository: update_metadata()
-
 The rules for top-level metadata are
  * Metadata is loadable only if metadata it depends on is loaded
  * Metadata is immutable if any metadata depending on it has been loaded
- * Loading from local storage must be attempted before updating from remote
- * Updating from remote is never required
+ * Caller must load/update these in order:
+   root -> timestamp -> snapshot -> targets -> (other delegated targets)
+ * Caller should try loading local file before updating metadata from remote
 
-Exceptions are raised if metadata fails to load in any way (except in the
-case of local loads -- see load_local_metadata()).
+Exceptions are raised if metadata fails to load in any way. The exception
+to this is local loads -- only local root metadata needs to be valid:
+other local metadata is allowed to be invalid (e.g. no longer signed):
+it won't be loaded but there will not be an exception.
 
 Example (with hypothetical download function):
 
 >>> # Load local root
 >>> bundle = MetadataBundle("path/to/metadata")
 >>>
->>> # load more root versions from remote
->>> with download("root", bundle.root.signed.version + 1) as f:
->>>     bundle.update_metadata(f.read())
+>>> # update root until no more are available from remote
 >>> with download("root", bundle.root.signed.version + 1) as f:
->>>     bundle.update_metadata(f.read())
->>>
->>> # Finally, no more roots from remote
+>>>     bundle.update_root(f.read())
+>>> # ...
 >>> bundle.root_update_finished()
 >>>
->>> # load local timestamp, then update it
->>> bundle.load_local_metadata("timestamp")
+>>> # load timestamp, then update from remote
+>>> bundle.load_local_timestamp()
 >>> with download("timestamp") as f:
->>>     bundle.update_metadata(f.read())
+>>>     bundle.update_timestamp(f.read())
 >>>
->>> # load local snapshot, then update it if needed
->>> if not bundle.load_local_metadata("snapshot"):
->>>     # load snapshot (consistent snapshot not shown)
->>>     with download("snapshot") as f:
->>>         bundle.update_metadata(f.read())
+>>> # load snapshot, update from remote if needed
+>>> if not bundle.load_local_snapshot():
+>>>     # TODO get version from timestamp
+>>>     with download("snapshot", version) as f:
+>>>         bundle.update_snapshot(f.read())
 >>>
->>> # load local targets, then update it if needed
->>> if not bundle.load_local_metadata("targets"):
->>>     version = bundle.snapshot.signed.meta["targets.json"]["version"]
->>>     with download("snapshot", version + 1) as f:
->>>         bundle.update_metadata(f.read())
+>>> # load local targets, update from remote if needed
+>>> if not bundle.load_local_targets():
+>>>     # TODO get version from snapshot
+>>>     with download("targets", version) as f:
+>>>         bundle.update_targets(f.read())
 >>>
->>> # Top level metadata is now fully loaded and verified
+>>> # load local delegated role, update from remote if needed
+>>> if not bundle.load_local_delegated_targets("rolename", "targets"):
+>>>     # TODO get version from snapshot
+>>>     with download("rolename", version) as f:
+>>>         bundle.update_targets(f.read(), "rolename", "targets")
 
 
 TODO:
- * exceptions are all over the place and not thought out at all
+ * exceptions are all over the place: the idea is that client could just handle a
+   generic RepositoryError that covers every issue that server provided metadata
+   could inflict (other errors would be user errors), but this is not yet the case
  * usefulness of root_update_finished() can be debated: it could be done
-   in the beginning of _load_timestamp()...
+   in the beginning of load_timestamp()...
  * there are some divergences from spec:
    * 5.3.11: timestamp and snapshot are not deleted right away (only on next load):
      the load functions will refuse to load the files when they are not signed by
      current root keys. Deleting at the specified point is possible but means additional
      code with some quirks..
+   * in general local metadata files are not deleted (they just won't succesfully load)
  * a bit of repetition
  * No tests!
  * Naming maybe not final?
@@ -130,7 +132,6 @@ def __init__(self, repository_path: str):
         """Initialize by loading root metadata from disk"""
         self._path = repository_path
         self._bundle = {}  # type: Dict[str: Metadata]
-        self._local_load_attempted = {}
         self.reference_time = None
 
         if not os.path.exists(self._path):
@@ -139,90 +140,49 @@ def __init__(self, repository_path: str):
 
         # Load and validate the local root metadata
         # Valid root metadata is required
-        if not self.load_local_metadata("root"):
+        logger.debug("Loading local root")
+        try:
+            with open(os.path.join(self._path, "root.json"), "rb") as f:
+                self._load_intermediate_root(f.read())
+        except (OSError, exceptions.RepositoryError) as e:
             raise exceptions.RepositoryError("Failed to load local root metadata")
 
-    def load_local_metadata(self, role_name: str, delegator_name: str = None) -> bool:
-        """Loads metadata from local storage and inserts into bundle
-
-        If bundle already contains 'role_name', nothing is loaded.
-        Failure to read the file, failure to parse it and failure to
-        load it as valid metadata will not raise exceptions: the function
-        will just fail.
-
-        Raises if 'role_name' cannot be loaded from local storage at this state
-
-        Returns True if 'role_name' is now in the bundle
-        """
-        if self.get(role_name) is not None:
-            logger.debug("Already loaded local %s.json", role_name)
-            return True
-
-        logger.debug("Loading local %s.json", role_name)
+    # Implement Mapping
+    def __getitem__(self, key: str):
+        return self._bundle[key]
 
-        self._raise_on_unsupported_state(role_name)
-        self._local_load_attempted[role_name] = True
+    def __len__(self):
+        return len(self._bundle)
 
-        try:
-            with open(os.path.join(self._path, f"{role_name}.json"), "rb") as f:
-                data = f.read()
-
-            if role_name == "root":
-                self._load_intermediate_root(data)
-            elif role_name == "timestamp":
-                self._load_timestamp(data)
-            elif role_name == "snapshot":
-                self._load_snapshot(data)
-            elif role_name == "targets":
-                self._load_targets(data)
-            else:
-                self._load_delegated_targets(data, role_name, delegator_name)
+    def __iter__(self):
+        return iter(self._bundle)
 
-            return True
-        except (OSError, exceptions.RepositoryError) as e:
-            logger.debug("Failed to load local %s: %s", role_name, e)
-            # TODO delete local file (except probably should not delete root.json?)
-            return False
+    # Helper properties for top level metadata
+    @property
+    def root(self):
+        return self._bundle.get("root")
 
-    def update_metadata(self, data: bytes, role_name: str, delegator_name: str = None):
-        """Takes new metadata (from remote repository) and loads it into bundle
+    @property
+    def timestamp(self):
+        return self._bundle.get("timestamp")
 
-        Raises if 'role_name' cannot be update from remote at this state
-        Raises if 'data' cannot be parsed or validated
-        Raises if the new metadata cannot be verified by the bundle
-        """
-        logger.debug("Updating %s", role_name)
+    @property
+    def snapshot(self):
+        return self._bundle.get("snapshot")
 
-        self._raise_on_unsupported_state(role_name)
+    @property
+    def targets(self):
+        return self._bundle.get("targets")
 
-        if not self._local_load_attempted.get(role_name):
-            raise ValueError(
-                f"Cannot update {role_name} before loading local metadata"
-            )
+    # Public methods
+    def update_root(self, data: bytes):
+        logger.debug("Updating root")
 
-        if role_name == "root":
-            self._load_intermediate_root(data)
-            self.root.to_file(os.path.join(self._path, "root.json"))
-        elif role_name == "timestamp":
-            self._load_timestamp(data)
-            self.timestamp.to_file(os.path.join(self._path, "timestamp.json"))
-        elif role_name == "snapshot":
-            self._load_snapshot(data)
-            self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
-        elif role_name == "targets":
-            self._load_targets(data)
-            self.targets.to_file(os.path.join(self._path, "targets.json"))
-        else:
-            self._load_delegated_targets(data, role_name, delegator_name)
-            self[role_name].to_file(os.path.join(self._path, f"{role_name}.json"))
+        self._load_intermediate_root(data)
+        self.root.to_file(os.path.join(self._path, "root.json"))
 
     def root_update_finished(self):
-        """Marks root update as finished, validates the root metadata
-
-        Raises if root update is not a valid operation at this state
-        Raises if validation fails
-        """
-        if self.timestamp is None:
+        if self.reference_time is not None:
             raise ValueError("Root update is already finished")
 
         # Store our reference "now", verify root expiry
@@ -232,79 +192,82 @@ def root_update_finished(self):
 
         logger.debug("Verified final root.json")
 
-    def _raise_on_unsupported_state(self, role_name: str):
-        """Raise if updating 'role_name' is not supported at this state"""
+    def load_local_timestamp(self):
+        logger.debug("Loading local timestamp")
 
-        # Special rules for top-level roles. We want to enforce a strict order
-        # root->snapshot->timestamp->targets where loading a metadata is no
-        # longer allowed when the next metadata in the order has been loaded
-        if role_name == "root":
-            pass
-        elif role_name == "timestamp":
-            if self.reference_time is None:
-                # root_update_finished() not called
-                raise ValueError("Cannot load timestamp before root")
-            if self.snapshot is not None:
-                raise ValueError("Cannot load timestamp after snapshot")
-        elif role_name == "snapshot":
-            if self.timestamp is None:
-                raise ValueError("Cannot load snapshot before timestamp")
-            if self.targets is not None:
-                raise ValueError("Cannot load snapshot after targets")
-        elif role_name == "targets":
-            if self.snapshot is None:
-                raise ValueError("Cannot load targets before snapshot")
-        else:
-            if self.targets is None:
-                raise ValueError(
-                    "Cannot load delegated targets before targets"
-                )
+        try:
+            with open(os.path.join(self._path, "timestamp.json"), "rb") as f:
+                self._load_timestamp(f.read())
+            return True
+        except (OSError, exceptions.RepositoryError) as e:
+            logger.debug("Failed to load local timestamp: %s", e)
+            return False
 
-        # Generic rule: Updating a role is not allowed if
-        #  * role is already loaded AND
-        #  * role has a delegate that is already loaded
-        role = self.get(role_name)
-        if role is not None and role.signed.delegations is not None:
-            for delegate in role.signed.delegations["roles"]:
-                delegate_name = delegate["name"]
-                if self.get(delegate_name) is not None:
-                    raise ValueError(
-                        f"Cannot load {role_name} after delegate"
-                        f"{delegate_name}"
-                    )
+    def update_timestamp(self, data: bytes):
+        logger.debug("Updating timestamp")
 
-    # Implement Mapping
-    def __getitem__(self, key: str):
-        return self._bundle[key]
+        self._load_timestamp(data)
+        self.timestamp.to_file(os.path.join(self._path, "timestamp.json"))
 
-    def __len__(self):
-        return len(self._bundle)
+    def load_local_snapshot(self):
+        logger.debug("Loading local snapshot")
 
-    def __iter__(self):
-        return iter(self._bundle)
+        try:
+            with open(os.path.join(self._path, "snapshot.json"), "rb") as f:
+                self._load_snapshot(f.read())
+            return True
+        except (OSError, exceptions.RepositoryError) as e:
+            logger.debug("Failed to load local snapshot: %s", e)
+            return False
 
-    # Helper properties for top level metadata
-    @property
-    def root(self):
-        return self._bundle.get("root")
+    def update_snapshot(self, data: bytes):
+        logger.debug("Updating snapshot")
 
-    @property
-    def timestamp(self):
-        return self._bundle.get("timestamp")
+        self._load_snapshot(data)
+        self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
 
-    @property
-    def snapshot(self):
-        return self._bundle.get("snapshot")
+    def load_local_targets(self):
+        logger.debug("Loading local targets")
 
-    @property
-    def targets(self):
-        return self._bundle.get("targets")
+        try:
+            with open(os.path.join(self._path, "targets.json"), "rb") as f:
+                self._load_targets(f.read())
+            return True
+        except (OSError, exceptions.RepositoryError) as e:
+            logger.debug("Failed to load local targets: %s", e)
+            return False
+
+    def update_targets(self, data: bytes):
+        logger.debug("Updating targets")
+
+        self._load_targets(data)
+        self.targets.to_file(os.path.join(self._path, "targets.json"))
+
+    def load_local_delegated_targets(self, role_name: str, delegator_name: str):
+        logger.debug("Loading local %s", role_name)
+
+        try:
+            with open(os.path.join(self._path, f"{role_name}.json"), "rb") as f:
+                self._load_delegated_targets(f.read(), role_name, delegator_name)
+            return True
+        except (OSError, exceptions.RepositoryError) as e:
+            logger.debug("Failed to load local %s: %s", role_name, e)
+            return False
+
+    def update_delegated_targets(self, data: bytes, role_name: str, delegator_name: str = None):
+        logger.debug("Updating %s", role_name)
+
+        self._load_delegated_targets(data, role_name, delegator_name)
+        self[role_name].to_file(os.path.join(self._path, f"{role_name}.json"))
 
     def _load_intermediate_root(self, data: bytes):
         """Verify the new root using current root (if any) and use it as current root
 
         Raises if root fails verification
         """
+        if self.reference_time is not None:
+            raise ValueError("Cannot update root after root update is finished")
+
         try:
             new_root = Metadata.from_bytes(data)
         except SerializationError as e:
@@ -340,6 +303,12 @@ def _load_timestamp(self, data: bytes):
 
         Raises if verification fails
         """
+        if self.reference_time is None:
+            # root_update_finished() not called
+            raise ValueError("Cannot update timestamp before root")
+        if self.snapshot is not None:
+            raise ValueError("Cannot update timestamp after snapshot")
+
         try:
             new_timestamp = Metadata.from_bytes(data)
         except SerializationError as e:
@@ -382,6 +351,15 @@ def _load_timestamp(self, data: bytes):
         logger.debug("Loaded timestamp")
 
     def _load_snapshot(self, data: bytes):
+        """Verifies the new snapshot and uses it as current snapshot
+
+        Raises if verification fails
+        """
+
+        if self.timestamp is None:
+            raise ValueError("Cannot update snapshot before timestamp")
+        if self.targets is not None:
+            raise ValueError("Cannot update snapshot after targets")
 
         meta = self.timestamp.signed.meta["snapshot.json"]
 
@@ -443,13 +421,21 @@ def _load_snapshot(self, data: bytes):
         logger.debug("Loaded snapshot")
 
     def _load_targets(self, data: bytes):
+        """Verifies the new targets and uses it as current targets
+
+        Raises if verification fails
+        """
+        if self.snapshot is None:
+            raise ValueError("Cannot load targets before snapshot")
+
         self._load_delegated_targets(data, "targets", "root")
 
     def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: str):
-        logger.debug(f"Loading {role_name} delegated by {delegator_name}")
+        """Verifies the new delegated 'role_name' and uses it as current 'role_name'
 
+        Raises if verification fails
+        """
         delegator = self.get(delegator_name)
-        # TODO this check could maybe be done in _raise_on_unspported_state
         if delegator == None:
             raise exceptions.ValueError(
                 "Cannot load delegated target before delegator"
@@ -495,4 +481,4 @@ def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: s
             raise exceptions.ExpiredMetadataError(f"New {role_name} is expired")
 
         self._bundle[role_name] = new_delegate
-        logger.debug(f"Loaded {role_name}")
+        logger.debug(f"Loaded {role_name} delegated by {delegator_name}")

From 5596f777f897ad081319f09b7adfd1f787671d96 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Wed, 5 May 2021 21:44:20 +0300
Subject: [PATCH 12/33] MetadataBundle: Handle targets like delegated targets

The public load_local_targets() and update_targets() can just call the
versions for delegated targets to remove duplication.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 3ebb187d5b..dcc52ce3b3 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -227,21 +227,10 @@ def update_snapshot(self, data: bytes):
         self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
 
     def load_local_targets(self):
-        logger.debug("Loading local targets")
-
-        try:
-            with open(os.path.join(self._path, "targets.json"), "rb") as f:
-                self._load_targets(f.read())
-            return True
-        except (OSError, exceptions.RepositoryError) as e:
-            logger.debug("Failed to load local targets: %s", e)
-            return False
+        return self.load_local_delegated_targets("targets", "root")
 
     def update_targets(self, data: bytes):
-        logger.debug("Updating targets")
-
-        self._load_targets(data)
-        self.targets.to_file(os.path.join(self._path, "targets.json"))
+        self.update_delegated_targets(data, "targets", "root")
 
     def load_local_delegated_targets(self, role_name: str, delegator_name: str):
         logger.debug("Loading local %s", role_name)
@@ -420,21 +409,14 @@ def _load_snapshot(self, data: bytes):
         self._bundle["snapshot"] = new_snapshot
         logger.debug("Loaded snapshot")
 
-    def _load_targets(self, data: bytes):
-        """Verifies the new targets and uses it as current targets
+    def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: str):
+        """Verifies the new delegated 'role_name' and uses it as current 'role_name'
 
         Raises if verification fails
         """
         if self.snapshot is None:
             raise ValueError("Cannot load targets before snapshot")
 
-        self._load_delegated_targets(data, "targets", "root")
-
-    def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: str):
-        """Verifies the new delegated 'role_name' and uses it as current 'role_name'
-
-        Raises if verification fails
-        """
         delegator = self.get(delegator_name)
         if delegator == None:
             raise exceptions.ValueError(

From 766f4948d4279d9c92a17b19b7fdd1a163a42854 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Wed, 5 May 2021 21:52:08 +0300
Subject: [PATCH 13/33] MetadataBundle: Avoid loading targets twice

If role is loaded already, skip loading local version (this is an
optimization since there's no case where the loaded version and the
on-disk version should differ).

This could be done for top-level metadata as well but the situation
doesn't really come up there: there's no good reasons to try loading
top-level local metadata multiple times, unlike in the targets case
where same delegates may be loaded when looking up different target
files.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index dcc52ce3b3..b4881eb880 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -233,6 +233,10 @@ def update_targets(self, data: bytes):
         self.update_delegated_targets(data, "targets", "root")
 
     def load_local_delegated_targets(self, role_name: str, delegator_name: str):
+        if self.get(role_name):
+            logger.debug("Local %s already loaded", role_name)
+            return True
+
         logger.debug("Loading local %s", role_name)
 
         try:

From 9051358ff3d2f1c80ea59f639c3b45676b7bd2bb Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Thu, 6 May 2021 10:01:16 +0300
Subject: [PATCH 14/33] MetadataBundle: fix import name

Also use only lookup dictionary once if necessary.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index b4881eb880..cf4868191b 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -86,7 +86,7 @@
 import os
 from typing import Dict
 
-from securesystemslib import keys as sslib_hash
+from securesystemslib import hash as sslib_hash
 from securesystemslib import keys as sslib_keys
 
 from tuf import exceptions
@@ -358,7 +358,7 @@ def _load_snapshot(self, data: bytes):
 
         # Verify against the hashes in timestamp, if any
         hashes = meta.get("hashes") or {}
-        for algo, _hash in meta["hashes"].items():
+        for algo, _hash in hashes.items():
             digest_object = sslib_hash.digest(algo)
             digest_object.update(data)
             observed_hash = digest_object.hexdigest()

From 1d22d5aedca75dc6b54a40550f3ed23231fe9811 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Thu, 6 May 2021 10:24:19 +0300
Subject: [PATCH 15/33] MetadataBundle: Improve hints and docs

Complete the type hints for MetadataBundle.
Slightly improve documentation.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 66 +++++++++++++++++-----------
 1 file changed, 41 insertions(+), 25 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index cf4868191b..1f38250191 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -84,7 +84,7 @@
 from datetime import datetime
 import logging
 import os
-from typing import Dict
+from typing import Dict, Iterator, Optional
 
 from securesystemslib import hash as sslib_hash
 from securesystemslib import keys as sslib_keys
@@ -97,7 +97,7 @@
 
 # This is a placeholder until ...
 # TODO issue 1306: implement this in Metadata API
-def verify_with_threshold(delegator: Metadata, role_name: str, unverified: Metadata):
+def verify_with_threshold(delegator: Metadata, role_name: str, unverified: Metadata) -> bool:
     if delegator.signed._type == "root":
         keys = delegator.signed.keys
         role = delegator.signed.roles.get(role_name)
@@ -148,40 +148,42 @@ def __init__(self, repository_path: str):
             raise exceptions.RepositoryError("Failed to load local root metadata")
 
     # Implement Mapping
-    def __getitem__(self, key: str):
+    def __getitem__(self, key: str) -> Metadata:
         return self._bundle[key]
 
-    def __len__(self):
+    def __len__(self) -> int:
         return len(self._bundle)
 
-    def __iter__(self):
+    def __iter__(self) -> Iterator[Metadata]:
         return iter(self._bundle)
 
     # Helper properties for top level metadata
     @property
-    def root(self):
+    def root(self) -> Optional[Metadata]:
         return self._bundle.get("root")
 
     @property
-    def timestamp(self):
+    def timestamp(self) -> Optional[Metadata]:
         return self._bundle.get("timestamp")
 
     @property
-    def snapshot(self):
+    def snapshot(self) -> Optional[Metadata]:
         return self._bundle.get("snapshot")
 
     @property
-    def targets(self):
+    def targets(self) -> Optional[Metadata]:
         return self._bundle.get("targets")
 
     # Public methods
     def update_root(self, data: bytes):
+        """Update root metadata with data from remote repository."""
         logger.debug("Updating root")
 
         self._load_intermediate_root(data)
         self.root.to_file(os.path.join(self._path, "root.json"))
 
     def root_update_finished(self):
+        """Mark root metadata as final."""
         if self.reference_time is not None:
             raise ValueError("Root update is already finished")
 
@@ -192,7 +194,10 @@ def root_update_finished(self):
 
         logger.debug("Verified final root.json")
 
-    def load_local_timestamp(self):
+    def load_local_timestamp(self) -> bool:
+        """Load cached timestamp metadata from local storage.
+
+        Returns True if timestamp was succesfully loaded"""
         logger.debug("Loading local timestamp")
 
         try:
@@ -204,12 +209,16 @@ def load_local_timestamp(self):
             return False
 
     def update_timestamp(self, data: bytes):
+        """Update timestamp metadata with data from remote repository."""
         logger.debug("Updating timestamp")
 
         self._load_timestamp(data)
         self.timestamp.to_file(os.path.join(self._path, "timestamp.json"))
 
-    def load_local_snapshot(self):
+    def load_local_snapshot(self) -> bool:
+        """Load cached snapshot metadata from local storage.
+
+        Returns True if snapshot was succesfully loaded"""
         logger.debug("Loading local snapshot")
 
         try:
@@ -221,18 +230,28 @@ def load_local_snapshot(self):
             return False
 
     def update_snapshot(self, data: bytes):
+        """Update snapshot metadata with data from remote repository."""
         logger.debug("Updating snapshot")
 
         self._load_snapshot(data)
         self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
 
-    def load_local_targets(self):
+    def load_local_targets(self) -> bool:
+        """Load cached targets metadata from local storage.
+
+        Returns True if targets was succesfully loaded"""
         return self.load_local_delegated_targets("targets", "root")
 
     def update_targets(self, data: bytes):
+        """Update targets metadata with data from remote repository."""
         self.update_delegated_targets(data, "targets", "root")
 
-    def load_local_delegated_targets(self, role_name: str, delegator_name: str):
+    def load_local_delegated_targets(self, role_name: str, delegator_name: str) -> bool:
+        """Load cached metadata for 'role_name' from local storage.
+
+        Metadata for 'delegator_name' must be loaded already.
+
+        Returns True if metadata was succesfully loaded"""
         if self.get(role_name):
             logger.debug("Local %s already loaded", role_name)
             return True
@@ -248,16 +267,19 @@ def load_local_delegated_targets(self, role_name: str, delegator_name: str):
             return False
 
     def update_delegated_targets(self, data: bytes, role_name: str, delegator_name: str = None):
+        """Update 'rolename' metadata with data from remote repository.
+
+        Metadata for 'delegator_name' must be loaded already."""
         logger.debug("Updating %s", role_name)
 
         self._load_delegated_targets(data, role_name, delegator_name)
         self[role_name].to_file(os.path.join(self._path, f"{role_name}.json"))
 
     def _load_intermediate_root(self, data: bytes):
-        """Verify the new root using current root (if any) and use it as current root
+        """Verifies and loads 'data' as new root metadata.
 
-        Raises if root fails verification
-        """
+        Note that an expired intermediate root is considered valid: expiry is
+        only checked for the final root in root_update_finished()."""
         if self.reference_time is not None:
             raise ValueError("Cannot update root after root update is finished")
 
@@ -292,10 +314,7 @@ def _load_intermediate_root(self, data: bytes):
         logger.debug("Loaded root")
 
     def _load_timestamp(self, data: bytes):
-        """Verifies the new timestamp and uses it as current timestamp
-
-        Raises if verification fails
-        """
+        """Verifies and loads 'data' as new timestamp metadata."""
         if self.reference_time is None:
             # root_update_finished() not called
             raise ValueError("Cannot update timestamp before root")
@@ -344,10 +363,7 @@ def _load_timestamp(self, data: bytes):
         logger.debug("Loaded timestamp")
 
     def _load_snapshot(self, data: bytes):
-        """Verifies the new snapshot and uses it as current snapshot
-
-        Raises if verification fails
-        """
+        """Verifies and loads 'data' as new snapshot metadata."""
 
         if self.timestamp is None:
             raise ValueError("Cannot update snapshot before timestamp")
@@ -414,7 +430,7 @@ def _load_snapshot(self, data: bytes):
         logger.debug("Loaded snapshot")
 
     def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: str):
-        """Verifies the new delegated 'role_name' and uses it as current 'role_name'
+        """Verifies and loads 'data' as new metadata for delegated target 'role_name'.
 
         Raises if verification fails
         """

From e26772cc6d5b06e697faf4c611f366411087008d Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Thu, 6 May 2021 11:33:34 +0300
Subject: [PATCH 16/33] Remove unnecessary directory check at startup

* Loading root.json will fail just as descriptively
* As long as Bundle doesn't implement bootstrapping
  the local repo, there's also no need to create
  missing directories

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 1f38250191..6093efdf72 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -134,10 +134,6 @@ def __init__(self, repository_path: str):
         self._bundle = {}  # type: Dict[str: Metadata]
         self.reference_time = None
 
-        if not os.path.exists(self._path):
-            # TODO try to create dir instead?
-            raise exceptions.RepositoryError("Repository does not exist")
-
         # Load and validate the local root metadata
         # Valid root metadata is required
         logger.debug("Loading local root")

From 800b0882123bb3e15d2ea4ca7d125902b46b321d Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Thu, 6 May 2021 14:46:34 +0300
Subject: [PATCH 17/33] MetadataBundle: Fix loads of linting issues

Lots of fixes, mostly obvious ones. The trickier ones and pylint
disables have comments added to explain them.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 95 +++++++++++++++++-----------
 1 file changed, 59 insertions(+), 36 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 6093efdf72..a5fdb33836 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -61,17 +61,14 @@
 
 
 TODO:
- * exceptions are all over the place: the idea is that client could just handle a
-   generic RepositoryError that covers every issue that server provided metadata
-   could inflict (other errors would be user errors), but this is not yet the case
+ * exceptions are all over the place: the idea is that client could just handle
+   a generic RepositoryError that covers every issue that server provided
+   metadata could inflict (other errors would be user errors), but this is not
+   yet the case
  * usefulness of root_update_finished() can be debated: it could be done
    in the beginning of load_timestamp()...
- * there are some divergences from spec:
-   * 5.3.11: timestamp and snapshot are not deleted right away (only on next load):
-     the load functions will refuse to load the files when they are not signed by
-     current root keys. Deleting at the specified point is possible but means additional
-     code with some quirks..
-   * in general local metadata files are not deleted (they just won't succesfully load)
+ * there are some divergences from spec: in general local metadata files are
+   not deleted (they just won't succesfully load)
  * a bit of repetition
  * No tests!
  * Naming maybe not final?
@@ -80,10 +77,10 @@
    (not sure yet how: maybe a spec_logger that logs specification events?)
 """
 
-from collections import abc
-from datetime import datetime
 import logging
 import os
+from collections import abc
+from datetime import datetime
 from typing import Dict, Iterator, Optional
 
 from securesystemslib import hash as sslib_hash
@@ -93,11 +90,21 @@
 from tuf.api.metadata import Metadata
 from tuf.api.serialization import SerializationError
 
+# TODO: Either enaable old-style logging in pylintc (issue #1334)
+# or change this file to use f-strings for logging
+# pylint: disable=logging-too-many-args
+
+# TODO: signed._type really does not work issue #1375:
+# pylint: disable=protected-access
+
 logger = logging.getLogger(__name__)
 
 # This is a placeholder until ...
 # TODO issue 1306: implement this in Metadata API
-def verify_with_threshold(delegator: Metadata, role_name: str, unverified: Metadata) -> bool:
+def verify_with_threshold(
+    delegator: Metadata, role_name: str, unverified: Metadata
+) -> bool:
+    """Verify 'unverified' with keys and treshold defined in delegator"""
     if delegator.signed._type == "root":
         keys = delegator.signed.keys
         role = delegator.signed.roles.get(role_name)
@@ -121,13 +128,20 @@ def verify_with_threshold(delegator: Metadata, role_name: str, unverified: Metad
         try:
             if unverified.verify(key):
                 unique_keys.add(key["keyval"]["public"])
-        except:  # TODO specify the Exceptions
-            pass
+        except Exception as e:  # pylint: disable=broad-except
+            # TODO specify the Exceptions (see issue #1351)
+            logger.info("verify failed: %s", e)
 
     return len(unique_keys) >= role["threshold"]
 
 
 class MetadataBundle(abc.Mapping):
+    """Internal class to keep track of valid metadata in Updater
+
+    MetadataBundle ensures that metadata is valid. It provides easy ways to
+    update the metadata with the caller making decisions on what is updated.
+    """
+
     def __init__(self, repository_path: str):
         """Initialize by loading root metadata from disk"""
         self._path = repository_path
@@ -141,7 +155,9 @@ def __init__(self, repository_path: str):
             with open(os.path.join(self._path, "root.json"), "rb") as f:
                 self._load_intermediate_root(f.read())
         except (OSError, exceptions.RepositoryError) as e:
-            raise exceptions.RepositoryError("Failed to load local root metadata")
+            raise exceptions.RepositoryError(
+                "Failed to load local root metadata"
+            ) from e
 
     # Implement Mapping
     def __getitem__(self, key: str) -> Metadata:
@@ -242,7 +258,9 @@ def update_targets(self, data: bytes):
         """Update targets metadata with data from remote repository."""
         self.update_delegated_targets(data, "targets", "root")
 
-    def load_local_delegated_targets(self, role_name: str, delegator_name: str) -> bool:
+    def load_local_delegated_targets(
+        self, role_name: str, delegator_name: str
+    ) -> bool:
         """Load cached metadata for 'role_name' from local storage.
 
         Metadata for 'delegator_name' must be loaded already.
@@ -256,13 +274,17 @@ def load_local_delegated_targets(self, role_name: str, delegator_name: str) -> b
 
         try:
             with open(os.path.join(self._path, f"{role_name}.json"), "rb") as f:
-                self._load_delegated_targets(f.read(), role_name, delegator_name)
+                self._load_delegated_targets(
+                    f.read(), role_name, delegator_name
+                )
             return True
         except (OSError, exceptions.RepositoryError) as e:
             logger.debug("Failed to load local %s: %s", role_name, e)
             return False
 
-    def update_delegated_targets(self, data: bytes, role_name: str, delegator_name: str = None):
+    def update_delegated_targets(
+        self, data: bytes, role_name: str, delegator_name: str = None
+    ):
         """Update 'rolename' metadata with data from remote repository.
 
         Metadata for 'delegator_name' must be loaded already."""
@@ -296,7 +318,6 @@ def _load_intermediate_root(self, data: bytes):
                 )
 
             if new_root.signed.version != self.root.signed.version + 1:
-                # TODO not a "Replayed Metadata attack": the version is just not what we expected
                 raise exceptions.ReplayedMetadataError(
                     "root", new_root.signed.version, self.root.signed.version
                 )
@@ -358,7 +379,8 @@ def _load_timestamp(self, data: bytes):
         self._bundle["timestamp"] = new_timestamp
         logger.debug("Loaded timestamp")
 
-    def _load_snapshot(self, data: bytes):
+    # TODO: remove pylint disable once the hash verification is in metadata.py
+    def _load_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
         """Verifies and loads 'data' as new snapshot metadata."""
 
         if self.timestamp is None:
@@ -370,12 +392,12 @@ def _load_snapshot(self, data: bytes):
 
         # Verify against the hashes in timestamp, if any
         hashes = meta.get("hashes") or {}
-        for algo, _hash in hashes.items():
+        for algo, stored_hash in hashes.items():
             digest_object = sslib_hash.digest(algo)
             digest_object.update(data)
             observed_hash = digest_object.hexdigest()
-            if observed_hash != _hash:
-                raise exceptions.BadHashError(_hash, observed_hash)
+            if observed_hash != stored_hash:
+                raise exceptions.BadHashError(stored_hash, observed_hash)
 
         try:
             new_snapshot = Metadata.from_bytes(data)
@@ -425,8 +447,10 @@ def _load_snapshot(self, data: bytes):
         self._bundle["snapshot"] = new_snapshot
         logger.debug("Loaded snapshot")
 
-    def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: str):
-        """Verifies and loads 'data' as new metadata for delegated target 'role_name'.
+    def _load_delegated_targets(
+        self, data: bytes, role_name: str, delegator_name: str
+    ):
+        """Verifies and loads 'data' as new metadata for target 'role_name'.
 
         Raises if verification fails
         """
@@ -434,10 +458,8 @@ def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: s
             raise ValueError("Cannot load targets before snapshot")
 
         delegator = self.get(delegator_name)
-        if delegator == None:
-            raise exceptions.ValueError(
-                "Cannot load delegated target before delegator"
-            )
+        if delegator is None:
+            raise ValueError("Cannot load targets before delegator")
 
         # Verify against the hashes in snapshot, if any
         meta = self.snapshot.signed.meta.get(f"{role_name}.json")
@@ -447,12 +469,12 @@ def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: s
             )
 
         hashes = meta.get("hashes") or {}
-        for algo, _hash in hashes.items():
+        for algo, stored_hash in hashes.items():
             digest_object = sslib_hash.digest(algo)
             digest_object.update(data)
             observed_hash = digest_object.hexdigest()
-            if observed_hash != _hash:
-                raise exceptions.BadHashError(_hash, observed_hash)
+            if observed_hash != stored_hash:
+                raise exceptions.BadHashError(stored_hash, observed_hash)
 
         try:
             new_delegate = Metadata.from_bytes(data)
@@ -466,17 +488,18 @@ def _load_delegated_targets(self, data: bytes, role_name: str, delegator_name: s
 
         if not verify_with_threshold(delegator, role_name, new_delegate):
             raise exceptions.UnsignedMetadataError(
-                f"New {role_name} is not signed by {delegator_name}"
+                f"New {role_name} is not signed by {delegator_name}",
+                new_delegate,
             )
 
         if new_delegate.signed.version != meta["version"]:
             raise exceptions.BadVersionNumberError(
-                        f"Expected {role_name} version"
-                        f"{meta['version']}, got {new_delegate.signed.version}"
+                f"Expected {role_name} version"
+                f"{meta['version']}, got {new_delegate.signed.version}"
             )
 
         if new_delegate.signed.is_expired(self.reference_time):
             raise exceptions.ExpiredMetadataError(f"New {role_name} is expired")
 
         self._bundle[role_name] = new_delegate
-        logger.debug(f"Loaded {role_name} delegated by {delegator_name}")
+        logger.debug("Loaded %s delegated by %s", role_name, delegator_name)

From b6817886ccd545e983bfef0a988846e919419cab Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Tue, 11 May 2021 15:09:14 +0300
Subject: [PATCH 18/33] Improve documentation

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index a5fdb33836..fc5b2272d2 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -4,10 +4,11 @@
 """TUF client bundle-of-metadata
 
 MetadataBundle keeps track of current valid set of metadata for the client,
-and handles almost every step of the "Detailed client workflow" in the TUF
-specification (the remaining steps are download related). The bundle takes
-care of persisting valid metadata on disk, loading local metadata from disk
-and deleting invalid local metadata.
+and handles almost every step of the "Detailed client workflow" (
+https://theupdateframework.github.io/specification/latest#detailed-client-workflow)
+in the TUF specification (the remaining steps are download related). The
+bundle takes care of persisting valid metadata on disk and loading local
+metadata from disk.
 
 Loaded metadata can be accessed via the index access with rolename as key
 or, in the case of top-level metadata using the helper properties like
@@ -16,9 +17,12 @@
 The rules for top-level metadata are
  * Metadata is loadable only if metadata it depends on is loaded
  * Metadata is immutable if any metadata depending on it has been loaded
- * Caller must load/update these in order:
+ * Metadata must be loaded/updated in order:
    root -> timestamp -> snapshot -> targets -> (other delegated targets)
- * Caller should try loading local file before updating metadata from remote
+ * For each metadata either local load or the remote update must succeed
+ * Caller should try loading local version before updating metadata from remote
+   (the exception is root where local data is loaded at MetadataBundle
+   initialization: the initialization fails if local data cannot be loaded)
 
 Exceptions are raised if metadata fails to load in any way. The exception
 to this is local loads -- only local root metadata needs to be valid:
@@ -104,7 +108,7 @@
 def verify_with_threshold(
     delegator: Metadata, role_name: str, unverified: Metadata
 ) -> bool:
-    """Verify 'unverified' with keys and treshold defined in delegator"""
+    """Verify 'unverified' with keys and threshold defined in delegator"""
     if delegator.signed._type == "root":
         keys = delegator.signed.keys
         role = delegator.signed.roles.get(role_name)

From 66fa37b259ba91a4913ce24497216fd6d40dba6a Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 11:36:32 +0300
Subject: [PATCH 19/33] MetadataBundle: Update to API changes

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index fc5b2272d2..1f7b5d2236 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -91,7 +91,7 @@
 from securesystemslib import keys as sslib_keys
 
 from tuf import exceptions
-from tuf.api.metadata import Metadata
+from tuf.api.metadata import Metadata, Root, Targets
 from tuf.api.serialization import SerializationError
 
 # TODO: Either enaable old-style logging in pylintc (issue #1334)
@@ -109,14 +109,17 @@ def verify_with_threshold(
     delegator: Metadata, role_name: str, unverified: Metadata
 ) -> bool:
     """Verify 'unverified' with keys and threshold defined in delegator"""
-    if delegator.signed._type == "root":
+    role = None
+    keys = {}
+    if isinstance(delegator.signed, Root):
         keys = delegator.signed.keys
         role = delegator.signed.roles.get(role_name)
-    elif delegator.signed._type == "targets":
-        keys = delegator.signed.delegations["keys"]
-        # role names are unique: first match is enough
-        roles = delegator.signed.delegations["roles"]
-        role = next((role for role in roles if role["name"] == role_name), None)
+    elif isinstance(delegator.signed, Targets):
+        if delegator.signed.delegations:
+            keys = delegator.signed.delegations.keys
+            # role names are unique: first match is enough
+            roles = delegator.signed.delegations.roles
+            role = next((r for r in roles if r.name == role_name), None)
     else:
         raise ValueError("Call is valid only on delegator metadata")
 
@@ -125,9 +128,9 @@ def verify_with_threshold(
 
     # verify that delegate is signed by correct threshold of unique keys
     unique_keys = set()
-    for keyid in role["keyids"]:
-        key_metadata = keys[keyid]
-        key, dummy = sslib_keys.format_metadata_to_key(key_metadata)
+    for keyid in role.keyids:
+        key_dict = keys[keyid].to_dict()
+        key, dummy = sslib_keys.format_metadata_to_key(key_dict)
 
         try:
             if unverified.verify(key):
@@ -136,7 +139,7 @@ def verify_with_threshold(
             # TODO specify the Exceptions (see issue #1351)
             logger.info("verify failed: %s", e)
 
-    return len(unique_keys) >= role["threshold"]
+    return len(unique_keys) >= role.threshold
 
 
 class MetadataBundle(abc.Mapping):

From 0bbfe038cfb5286f1cb0fcbd19cc7c6afd744504 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 12:32:45 +0300
Subject: [PATCH 20/33] tests: Add minimal test case for Bundle

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_metadata_bundle.py | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 tests/test_metadata_bundle.py

diff --git a/tests/test_metadata_bundle.py b/tests/test_metadata_bundle.py
new file mode 100644
index 0000000000..7d12e0cc7f
--- /dev/null
+++ b/tests/test_metadata_bundle.py
@@ -0,0 +1,29 @@
+import logging
+import os
+import sys
+import unittest
+
+from tuf.api import metadata
+from tuf.client_rework.metadata_bundle import MetadataBundle
+
+from tests import utils
+
+logger = logging.getLogger(__name__)
+
+class TestMetadataBundle(unittest.TestCase):
+    def test_local_load(self):
+        repo_dir = os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata')
+
+        bundle = MetadataBundle(repo_dir)
+        bundle.root_update_finished()
+
+        self.assertTrue(bundle.load_local_timestamp())
+        self.assertTrue(bundle.load_local_snapshot())
+        self.assertTrue(bundle.load_local_targets())
+        self.assertTrue(bundle.load_local_delegated_targets('role1','targets'))
+        self.assertTrue(bundle.load_local_delegated_targets('role2','role1'))
+
+
+if __name__ == '__main__':
+  utils.configure_test_logging(sys.argv)
+  unittest.main()

From 112b333bba2c420cddb53e4e0b7bc31d00173d83 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 14:02:29 +0300
Subject: [PATCH 21/33] Metadata API: Fix DelegatedRole serialization issue

A DelegatedRole with paths=[] fails to serialize correctly (paths is not
included in the output json).

Fix the issue, modify tests to notice a regression.

Fixes #1389

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_api.py   | 18 ++++++++++--------
 tuf/api/metadata.py |  6 +++---
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/tests/test_api.py b/tests/test_api.py
index 6e591a9eeb..a3b2381e13 100755
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -450,21 +450,23 @@ def test_delegated_role_class(self):
             with self.assertRaises(ValueError):
                 DelegatedRole.from_dict(role.copy())
 
-            # Test creating DelegatedRole only with "path_hash_prefixes"
+            # Test creating DelegatedRole only with "path_hash_prefixes" (an empty one)
             del role["paths"]
-            DelegatedRole.from_dict(role.copy())
-            role["paths"] = "foo"
+            role["path_hash_prefixes"] = []
+            role_obj = DelegatedRole.from_dict(role.copy())
+            self.assertEqual(role_obj.to_dict(), role)
 
-            # Test creating DelegatedRole only with "paths"
+            # Test creating DelegatedRole only with "paths" (now an empty one)
             del role["path_hash_prefixes"]
-            DelegatedRole.from_dict(role.copy())
-            role["path_hash_prefixes"] = "foo"
+            role["paths"] = []
+            role_obj = DelegatedRole.from_dict(role.copy())
+            self.assertEqual(role_obj.to_dict(), role)
 
             # Test creating DelegatedRole without "paths" and
             # "path_hash_prefixes" set
             del role["paths"]
-            del role["path_hash_prefixes"]
-            DelegatedRole.from_dict(role)
+            role_obj = DelegatedRole.from_dict(role.copy())
+            self.assertEqual(role_obj.to_dict(), role)
 
 
     def test_delegation_class(self):
diff --git a/tuf/api/metadata.py b/tuf/api/metadata.py
index 361630ef25..6cb654b937 100644
--- a/tuf/api/metadata.py
+++ b/tuf/api/metadata.py
@@ -770,7 +770,7 @@ def __init__(
         super().__init__(keyids, threshold, unrecognized_fields)
         self.name = name
         self.terminating = terminating
-        if paths and path_hash_prefixes:
+        if paths is not None and path_hash_prefixes is not None:
             raise ValueError(
                 "Only one of the attributes 'paths' and"
                 "'path_hash_prefixes' can be set!"
@@ -806,9 +806,9 @@ def to_dict(self) -> Dict[str, Any]:
             "terminating": self.terminating,
             **base_role_dict,
         }
-        if self.paths:
+        if self.paths is not None:
             res_dict["paths"] = self.paths
-        elif self.path_hash_prefixes:
+        elif self.path_hash_prefixes is not None:
             res_dict["path_hash_prefixes"] = self.path_hash_prefixes
         return res_dict
 

From f8b714d1675a857ef72d545f5d693edb62e7278d Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 14:26:26 +0300
Subject: [PATCH 22/33] Metadata API: Don't do equality comparisons on
 containers

Use either "if X is not None:" or a try-except instead of a "if X:".

I believe Targets.from_dict() was not really broken with previous code
but it looks suspicious and did fail the added test with a strange
exception: I expect the from_dict() methods to mainly fail with
KeyErrors, ValueErrors or AttributeErrors if file format structure
is incorrect.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_api.py   | 15 +++++++++++++++
 tuf/api/metadata.py | 11 +++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/tests/test_api.py b/tests/test_api.py
index a3b2381e13..3f95459e93 100755
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -496,6 +496,21 @@ def test_delegation_class(self):
         delegations = Delegations.from_dict(copy.deepcopy(delegations_dict))
         self.assertEqual(delegations_dict, delegations.to_dict())
 
+        # empty keys and roles
+        delegations_dict = {"keys":{}, "roles":[]}
+        delegations = Delegations.from_dict(delegations_dict.copy())
+        self.assertEqual(delegations_dict, delegations.to_dict())
+
+        # Test some basic missing or broken input
+        invalid_delegations_dicts = [
+            {},
+            {"keys":None, "roles":None},
+            {"keys":{"foo":0}, "roles":[]},
+            {"keys":{}, "roles":["foo"]},
+        ]
+        for d in invalid_delegations_dicts:
+            with self.assertRaises((KeyError, AttributeError)):
+                Delegations.from_dict(d)
 
     def test_metadata_targets(self):
         targets_path = os.path.join(
diff --git a/tuf/api/metadata.py b/tuf/api/metadata.py
index 6cb654b937..2aeb80f6cd 100644
--- a/tuf/api/metadata.py
+++ b/tuf/api/metadata.py
@@ -911,9 +911,12 @@ def from_dict(cls, targets_dict: Dict[str, Any]) -> "Targets":
         """Creates Targets object from its dict representation."""
         common_args = cls._common_fields_from_dict(targets_dict)
         targets = targets_dict.pop("targets")
-        delegations = targets_dict.pop("delegations", None)
-        if delegations:
-            delegations = Delegations.from_dict(delegations)
+        try:
+            delegations_dict = targets_dict.pop("delegations")
+        except KeyError:
+            delegations = None
+        else:
+            delegations = Delegations.from_dict(delegations_dict)
         # All fields left in the targets_dict are unrecognized.
         return cls(*common_args, targets, delegations, targets_dict)
 
@@ -921,7 +924,7 @@ def to_dict(self) -> Dict[str, Any]:
         """Returns the dict representation of self."""
         targets_dict = self._common_fields_to_dict()
         targets_dict["targets"] = self.targets
-        if self.delegations:
+        if self.delegations is not None:
             targets_dict["delegations"] = self.delegations.to_dict()
         return targets_dict
 

From 2d155faae69d845da929ad1637a67fa6ba7438c0 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 16:25:01 +0300
Subject: [PATCH 23/33] MetadataBundle: Change ValueErrors to RuntimeErrors

As the metadata type is no longer an argument, these are not
ValueErrors.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_metadata_bundle.py        | 20 ++++++++++++++++++++
 tuf/client_rework/metadata_bundle.py | 16 ++++++++--------
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/tests/test_metadata_bundle.py b/tests/test_metadata_bundle.py
index 7d12e0cc7f..be30e6ba83 100644
--- a/tests/test_metadata_bundle.py
+++ b/tests/test_metadata_bundle.py
@@ -14,12 +14,32 @@ class TestMetadataBundle(unittest.TestCase):
     def test_local_load(self):
         repo_dir = os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata')
 
+        # test loading all local metadata succesfully
         bundle = MetadataBundle(repo_dir)
         bundle.root_update_finished()
+        self.assertTrue(bundle.load_local_timestamp())
+        self.assertTrue(bundle.load_local_snapshot())
+        self.assertTrue(bundle.load_local_targets())
+        self.assertTrue(bundle.load_local_delegated_targets('role1','targets'))
+        self.assertTrue(bundle.load_local_delegated_targets('role2','role1'))
+
+        # Make sure loading metadata without its "dependencies" fails
+        bundle = MetadataBundle(repo_dir)
 
+        with self.assertRaises(RuntimeError):
+            bundle.load_local_timestamp()
+        bundle.root_update_finished()
+        with self.assertRaises(RuntimeError):
+            bundle.load_local_snapshot()
         self.assertTrue(bundle.load_local_timestamp())
+        with self.assertRaises(RuntimeError):
+            bundle.load_local_targets()
         self.assertTrue(bundle.load_local_snapshot())
+        with self.assertRaises(RuntimeError):
+            bundle.load_local_delegated_targets('role1','targets')
         self.assertTrue(bundle.load_local_targets())
+        with self.assertRaises(RuntimeError):
+            bundle.load_local_delegated_targets('role2','role1')
         self.assertTrue(bundle.load_local_delegated_targets('role1','targets'))
         self.assertTrue(bundle.load_local_delegated_targets('role2','role1'))
 
diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 1f7b5d2236..3ea73809aa 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -204,7 +204,7 @@ def update_root(self, data: bytes):
     def root_update_finished(self):
         """Mark root metadata as final."""
         if self.reference_time is not None:
-            raise ValueError("Root update is already finished")
+            raise RuntimeError("Root update is already finished")
 
         # Store our reference "now", verify root expiry
         self.reference_time = datetime.utcnow()
@@ -306,7 +306,7 @@ def _load_intermediate_root(self, data: bytes):
         Note that an expired intermediate root is considered valid: expiry is
         only checked for the final root in root_update_finished()."""
         if self.reference_time is not None:
-            raise ValueError("Cannot update root after root update is finished")
+            raise RuntimeError("Cannot update root after root update is finished")
 
         try:
             new_root = Metadata.from_bytes(data)
@@ -341,9 +341,9 @@ def _load_timestamp(self, data: bytes):
         """Verifies and loads 'data' as new timestamp metadata."""
         if self.reference_time is None:
             # root_update_finished() not called
-            raise ValueError("Cannot update timestamp before root")
+            raise RuntimeError("Cannot update timestamp before root")
         if self.snapshot is not None:
-            raise ValueError("Cannot update timestamp after snapshot")
+            raise RuntimeError("Cannot update timestamp after snapshot")
 
         try:
             new_timestamp = Metadata.from_bytes(data)
@@ -391,9 +391,9 @@ def _load_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
         """Verifies and loads 'data' as new snapshot metadata."""
 
         if self.timestamp is None:
-            raise ValueError("Cannot update snapshot before timestamp")
+            raise RuntimeError("Cannot update snapshot before timestamp")
         if self.targets is not None:
-            raise ValueError("Cannot update snapshot after targets")
+            raise RuntimeError("Cannot update snapshot after targets")
 
         meta = self.timestamp.signed.meta["snapshot.json"]
 
@@ -462,11 +462,11 @@ def _load_delegated_targets(
         Raises if verification fails
         """
         if self.snapshot is None:
-            raise ValueError("Cannot load targets before snapshot")
+            raise RuntimeError("Cannot load targets before snapshot")
 
         delegator = self.get(delegator_name)
         if delegator is None:
-            raise ValueError("Cannot load targets before delegator")
+            raise RuntimeError("Cannot load targets before delegator")
 
         # Verify against the hashes in snapshot, if any
         meta = self.snapshot.signed.meta.get(f"{role_name}.json")

From eb648d19bc340ebbe3a60d57eae5ddcff76f6b26 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 17:32:02 +0300
Subject: [PATCH 24/33] MetadataBundle: Save original files on disk

Don't use the serialized format as that won't match any hashes in
"meta".

Add basic tests for updating metadata.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_metadata_bundle.py        | 59 ++++++++++++++++++++++++++--
 tuf/client_rework/metadata_bundle.py | 12 ++++--
 2 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/tests/test_metadata_bundle.py b/tests/test_metadata_bundle.py
index be30e6ba83..b566a7a27b 100644
--- a/tests/test_metadata_bundle.py
+++ b/tests/test_metadata_bundle.py
@@ -1,6 +1,8 @@
 import logging
 import os
+import shutil
 import sys
+import tempfile
 import unittest
 
 from tuf.api import metadata
@@ -11,11 +13,26 @@
 logger = logging.getLogger(__name__)
 
 class TestMetadataBundle(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        cls.temporary_directory = tempfile.mkdtemp(dir=os.getcwd())
+
+    @classmethod
+    def tearDownClass(cls):
+        shutil.rmtree(cls.temporary_directory)
+
+    def setUp(self):
+        # copy metadata to "local repo"
+        shutil.copytree(
+            os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata'),
+            self.temporary_directory,
+            dirs_exist_ok=True
+        )
+
     def test_local_load(self):
-        repo_dir = os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata')
 
         # test loading all local metadata succesfully
-        bundle = MetadataBundle(repo_dir)
+        bundle = MetadataBundle(self.temporary_directory)
         bundle.root_update_finished()
         self.assertTrue(bundle.load_local_timestamp())
         self.assertTrue(bundle.load_local_snapshot())
@@ -24,7 +41,7 @@ def test_local_load(self):
         self.assertTrue(bundle.load_local_delegated_targets('role2','role1'))
 
         # Make sure loading metadata without its "dependencies" fails
-        bundle = MetadataBundle(repo_dir)
+        bundle = MetadataBundle(self.temporary_directory)
 
         with self.assertRaises(RuntimeError):
             bundle.load_local_timestamp()
@@ -43,6 +60,42 @@ def test_local_load(self):
         self.assertTrue(bundle.load_local_delegated_targets('role1','targets'))
         self.assertTrue(bundle.load_local_delegated_targets('role2','role1'))
 
+    def test_update(self):
+        remote_dir = os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata')
+
+        # remove all but root.json from local repo
+        os.remove(os.path.join(self.temporary_directory, "timestamp.json"))
+        os.remove(os.path.join(self.temporary_directory, "snapshot.json"))
+        os.remove(os.path.join(self.temporary_directory, "targets.json"))
+        os.remove(os.path.join(self.temporary_directory, "role1.json"))
+        os.remove(os.path.join(self.temporary_directory, "role2.json"))
+
+        # test updating metadata succesfully
+        bundle = MetadataBundle(self.temporary_directory)
+        bundle.root_update_finished()
+
+        with open(os.path.join(remote_dir, "timestamp.json"), "rb") as f:
+            bundle.update_timestamp(f.read())
+        with open(os.path.join(remote_dir, "snapshot.json"), "rb") as f:
+            bundle.update_snapshot(f.read())
+        with open(os.path.join(remote_dir, "targets.json"), "rb") as f:
+            bundle.update_targets(f.read())
+        with open(os.path.join(remote_dir, "role1.json"), "rb") as f:
+            bundle.update_delegated_targets(f.read(), "role1", "targets")
+        with open(os.path.join(remote_dir, "role2.json"), "rb") as f:
+            bundle.update_delegated_targets(f.read(), "role2", "role1")
+
+        # test loading the metadata (that should now be locally available)
+        bundle = MetadataBundle(self.temporary_directory)
+        bundle.root_update_finished()
+        self.assertTrue(bundle.load_local_timestamp())
+        self.assertTrue(bundle.load_local_snapshot())
+        self.assertTrue(bundle.load_local_targets())
+        self.assertTrue(bundle.load_local_delegated_targets('role1','targets'))
+        self.assertTrue(bundle.load_local_delegated_targets('role2','role1'))
+
+        # TODO test loading one version, then updating to new versions of each metadata
+
 
 if __name__ == '__main__':
   utils.configure_test_logging(sys.argv)
diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 3ea73809aa..91f0b39de3 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -199,7 +199,8 @@ def update_root(self, data: bytes):
         logger.debug("Updating root")
 
         self._load_intermediate_root(data)
-        self.root.to_file(os.path.join(self._path, "root.json"))
+        with open(os.path.join(self._path, "root.json"), "wb") as f:
+            f.write(data)
 
     def root_update_finished(self):
         """Mark root metadata as final."""
@@ -232,7 +233,8 @@ def update_timestamp(self, data: bytes):
         logger.debug("Updating timestamp")
 
         self._load_timestamp(data)
-        self.timestamp.to_file(os.path.join(self._path, "timestamp.json"))
+        with open(os.path.join(self._path, "timestamp.json"), "wb") as f:
+            f.write(data)
 
     def load_local_snapshot(self) -> bool:
         """Load cached snapshot metadata from local storage.
@@ -253,7 +255,8 @@ def update_snapshot(self, data: bytes):
         logger.debug("Updating snapshot")
 
         self._load_snapshot(data)
-        self.snapshot.to_file(os.path.join(self._path, "snapshot.json"))
+        with open(os.path.join(self._path, "snapshot.json"), "wb") as f:
+            f.write(data)
 
     def load_local_targets(self) -> bool:
         """Load cached targets metadata from local storage.
@@ -298,7 +301,8 @@ def update_delegated_targets(
         logger.debug("Updating %s", role_name)
 
         self._load_delegated_targets(data, role_name, delegator_name)
-        self[role_name].to_file(os.path.join(self._path, f"{role_name}.json"))
+        with open(os.path.join(self._path, f"{role_name}.json"), "wb") as f:
+            f.write(data)
 
     def _load_intermediate_root(self, data: bytes):
         """Verifies and loads 'data' as new root metadata.

From 3b30d085ee896ccf20cd31acf34fef87d163ac78 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 17:52:51 +0300
Subject: [PATCH 25/33] MetadataBundle: Store reference time earlier

Spec says reference time should be the beginning of the process: do
that.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 91f0b39de3..67ad2df450 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -153,7 +153,8 @@ def __init__(self, repository_path: str):
         """Initialize by loading root metadata from disk"""
         self._path = repository_path
         self._bundle = {}  # type: Dict[str: Metadata]
-        self.reference_time = None
+        self.reference_time = datetime.utcnow()
+        self._root_update_finished = False
 
         # Load and validate the local root metadata
         # Valid root metadata is required
@@ -204,14 +205,13 @@ def update_root(self, data: bytes):
 
     def root_update_finished(self):
         """Mark root metadata as final."""
-        if self.reference_time is not None:
+        if self._root_update_finished:
             raise RuntimeError("Root update is already finished")
 
-        # Store our reference "now", verify root expiry
-        self.reference_time = datetime.utcnow()
         if self.root.signed.is_expired(self.reference_time):
             raise exceptions.ExpiredMetadataError("New root.json is expired")
 
+        self._root_update_finished = True
         logger.debug("Verified final root.json")
 
     def load_local_timestamp(self) -> bool:
@@ -309,7 +309,7 @@ def _load_intermediate_root(self, data: bytes):
 
         Note that an expired intermediate root is considered valid: expiry is
         only checked for the final root in root_update_finished()."""
-        if self.reference_time is not None:
+        if self._root_update_finished:
             raise RuntimeError("Cannot update root after root update is finished")
 
         try:
@@ -343,7 +343,7 @@ def _load_intermediate_root(self, data: bytes):
 
     def _load_timestamp(self, data: bytes):
         """Verifies and loads 'data' as new timestamp metadata."""
-        if self.reference_time is None:
+        if not self._root_update_finished:
             # root_update_finished() not called
             raise RuntimeError("Cannot update timestamp before root")
         if self.snapshot is not None:

From 8d0245ab30dfc3d2cdd1c9109391e70853de1131 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 17:58:53 +0300
Subject: [PATCH 26/33] MetadataBundle: Use type, not _type

Signed now has "type" attribute, use that.

Also remove another pylint disable that is no longer needed (logging is
now old style).

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 67ad2df450..12ef50e4d3 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -94,13 +94,6 @@
 from tuf.api.metadata import Metadata, Root, Targets
 from tuf.api.serialization import SerializationError
 
-# TODO: Either enaable old-style logging in pylintc (issue #1334)
-# or change this file to use f-strings for logging
-# pylint: disable=logging-too-many-args
-
-# TODO: signed._type really does not work issue #1375:
-# pylint: disable=protected-access
-
 logger = logging.getLogger(__name__)
 
 # This is a placeholder until ...
@@ -317,9 +310,9 @@ def _load_intermediate_root(self, data: bytes):
         except SerializationError as e:
             raise exceptions.RepositoryError("Failed to load root") from e
 
-        if new_root.signed._type != "root":
+        if new_root.signed.type != "root":
             raise exceptions.RepositoryError(
-                f"Expected 'root', got '{new_root.signed._type}'"
+                f"Expected 'root', got '{new_root.signed.type}'"
             )
 
         if self.root is not None:
@@ -354,9 +347,9 @@ def _load_timestamp(self, data: bytes):
         except SerializationError as e:
             raise exceptions.RepositoryError("Failed to load timestamp") from e
 
-        if new_timestamp.signed._type != "timestamp":
+        if new_timestamp.signed.type != "timestamp":
             raise exceptions.RepositoryError(
-                f"Expected 'timestamp', got '{new_timestamp.signed._type}'"
+                f"Expected 'timestamp', got '{new_timestamp.signed.type}'"
             )
 
         if not verify_with_threshold(self.root, "timestamp", new_timestamp):
@@ -415,9 +408,9 @@ def _load_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
         except SerializationError as e:
             raise exceptions.RepositoryError("Failed to load snapshot") from e
 
-        if new_snapshot.signed._type != "snapshot":
+        if new_snapshot.signed.type != "snapshot":
             raise exceptions.RepositoryError(
-                f"Expected 'snapshot', got '{new_snapshot.signed._type}'"
+                f"Expected 'snapshot', got '{new_snapshot.signed.type}'"
             )
 
         if not verify_with_threshold(self.root, "snapshot", new_snapshot):
@@ -492,9 +485,9 @@ def _load_delegated_targets(
         except SerializationError as e:
             raise exceptions.RepositoryError("Failed to load snapshot") from e
 
-        if new_delegate.signed._type != "targets":
+        if new_delegate.signed.type != "targets":
             raise exceptions.RepositoryError(
-                f"Expected 'targets', got '{new_delegate.signed._type}'"
+                f"Expected 'targets', got '{new_delegate.signed.type}'"
             )
 
         if not verify_with_threshold(delegator, role_name, new_delegate):

From 876fda1bb26423518a2e1399403cc5c87ff27505 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 18:02:53 +0300
Subject: [PATCH 27/33] MetadataBundle: Add comments about the process

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 12ef50e4d3..54d5e02a42 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -204,6 +204,9 @@ def root_update_finished(self):
         if self.root.signed.is_expired(self.reference_time):
             raise exceptions.ExpiredMetadataError("New root.json is expired")
 
+        # We skip specification step 5.3.11: deleting timestamp and snapshot
+        # with rotated keys is not needed as they will be invalid, are not
+        # loaded and cannot be loaded
         self._root_update_finished = True
         logger.debug("Verified final root.json")
 
@@ -316,6 +319,7 @@ def _load_intermediate_root(self, data: bytes):
             )
 
         if self.root is not None:
+            # We are not loading initial trusted root: verify the new one
             if not verify_with_threshold(self.root, "root", new_root):
                 raise exceptions.UnsignedMetadataError(
                     "New root is not signed by root", new_root.signed

From 112f3b6a0334be4a68f4d5e72067b832285fba09 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 20:35:12 +0300
Subject: [PATCH 28/33] MetadataBundle: Handle Deserialization errors

because we are deserializing, not serializing.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 54d5e02a42..8e0e9e1b94 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -92,7 +92,7 @@
 
 from tuf import exceptions
 from tuf.api.metadata import Metadata, Root, Targets
-from tuf.api.serialization import SerializationError
+from tuf.api.serialization import DeserializationError
 
 logger = logging.getLogger(__name__)
 
@@ -310,7 +310,7 @@ def _load_intermediate_root(self, data: bytes):
 
         try:
             new_root = Metadata.from_bytes(data)
-        except SerializationError as e:
+        except DeserializationError as e:
             raise exceptions.RepositoryError("Failed to load root") from e
 
         if new_root.signed.type != "root":
@@ -348,7 +348,7 @@ def _load_timestamp(self, data: bytes):
 
         try:
             new_timestamp = Metadata.from_bytes(data)
-        except SerializationError as e:
+        except DeserializationError as e:
             raise exceptions.RepositoryError("Failed to load timestamp") from e
 
         if new_timestamp.signed.type != "timestamp":
@@ -409,7 +409,7 @@ def _load_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
 
         try:
             new_snapshot = Metadata.from_bytes(data)
-        except SerializationError as e:
+        except DeserializationError as e:
             raise exceptions.RepositoryError("Failed to load snapshot") from e
 
         if new_snapshot.signed.type != "snapshot":
@@ -486,7 +486,7 @@ def _load_delegated_targets(
 
         try:
             new_delegate = Metadata.from_bytes(data)
-        except SerializationError as e:
+        except DeserializationError as e:
             raise exceptions.RepositoryError("Failed to load snapshot") from e
 
         if new_delegate.signed.type != "targets":

From b86d1f733fe09930c5f810998aeb210b6b385a2e Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 20:36:51 +0300
Subject: [PATCH 29/33] MetadataBundle: Raise instead of returning bool

The bundle should now raise
* derivatives of RepositoryError on failures that are likely a result
  of server error or a malicious server
* RuntimeErrors if calls were made when they are not possible
* ValueErrors if arguments are invalid

last two are callers errors and avoidable.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_metadata_bundle.py        | 140 +++++++++++++++++++++------
 tuf/client_rework/metadata_bundle.py |  58 +++++------
 2 files changed, 136 insertions(+), 62 deletions(-)

diff --git a/tests/test_metadata_bundle.py b/tests/test_metadata_bundle.py
index b566a7a27b..27b35daa2f 100644
--- a/tests/test_metadata_bundle.py
+++ b/tests/test_metadata_bundle.py
@@ -1,3 +1,4 @@
+import json
 import logging
 import os
 import shutil
@@ -5,7 +6,8 @@
 import tempfile
 import unittest
 
-from tuf.api import metadata
+from tuf import exceptions
+from tuf.api.metadata import Metadata
 from tuf.client_rework.metadata_bundle import MetadataBundle
 
 from tests import utils
@@ -15,65 +17,67 @@
 class TestMetadataBundle(unittest.TestCase):
     @classmethod
     def setUpClass(cls):
-        cls.temporary_directory = tempfile.mkdtemp(dir=os.getcwd())
+        cls.temp_dir = tempfile.mkdtemp(dir=os.getcwd())
 
     @classmethod
     def tearDownClass(cls):
-        shutil.rmtree(cls.temporary_directory)
+        shutil.rmtree(cls.temp_dir)
 
     def setUp(self):
         # copy metadata to "local repo"
         shutil.copytree(
             os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata'),
-            self.temporary_directory,
+            self.temp_dir,
             dirs_exist_ok=True
         )
 
     def test_local_load(self):
 
         # test loading all local metadata succesfully
-        bundle = MetadataBundle(self.temporary_directory)
+        bundle = MetadataBundle(self.temp_dir)
         bundle.root_update_finished()
-        self.assertTrue(bundle.load_local_timestamp())
-        self.assertTrue(bundle.load_local_snapshot())
-        self.assertTrue(bundle.load_local_targets())
-        self.assertTrue(bundle.load_local_delegated_targets('role1','targets'))
-        self.assertTrue(bundle.load_local_delegated_targets('role2','role1'))
+        bundle.load_local_timestamp()
+        bundle.load_local_snapshot()
+        bundle.load_local_targets()
+        bundle.load_local_delegated_targets('role1','targets')
+        bundle.load_local_delegated_targets('role2','role1')
 
         # Make sure loading metadata without its "dependencies" fails
-        bundle = MetadataBundle(self.temporary_directory)
+        bundle = MetadataBundle(self.temp_dir)
 
         with self.assertRaises(RuntimeError):
             bundle.load_local_timestamp()
         bundle.root_update_finished()
         with self.assertRaises(RuntimeError):
             bundle.load_local_snapshot()
-        self.assertTrue(bundle.load_local_timestamp())
+        bundle.load_local_timestamp()
         with self.assertRaises(RuntimeError):
             bundle.load_local_targets()
-        self.assertTrue(bundle.load_local_snapshot())
+        bundle.load_local_snapshot()
         with self.assertRaises(RuntimeError):
             bundle.load_local_delegated_targets('role1','targets')
-        self.assertTrue(bundle.load_local_targets())
+        bundle.load_local_targets()
         with self.assertRaises(RuntimeError):
             bundle.load_local_delegated_targets('role2','role1')
-        self.assertTrue(bundle.load_local_delegated_targets('role1','targets'))
-        self.assertTrue(bundle.load_local_delegated_targets('role2','role1'))
+        bundle.load_local_delegated_targets('role1','targets')
+        bundle.load_local_delegated_targets('role2','role1')
 
     def test_update(self):
         remote_dir = os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata')
 
         # remove all but root.json from local repo
-        os.remove(os.path.join(self.temporary_directory, "timestamp.json"))
-        os.remove(os.path.join(self.temporary_directory, "snapshot.json"))
-        os.remove(os.path.join(self.temporary_directory, "targets.json"))
-        os.remove(os.path.join(self.temporary_directory, "role1.json"))
-        os.remove(os.path.join(self.temporary_directory, "role2.json"))
-
-        # test updating metadata succesfully
-        bundle = MetadataBundle(self.temporary_directory)
+        os.remove(os.path.join(self.temp_dir, "timestamp.json"))
+        os.remove(os.path.join(self.temp_dir, "snapshot.json"))
+        os.remove(os.path.join(self.temp_dir, "targets.json"))
+        os.remove(os.path.join(self.temp_dir, "role1.json"))
+        os.remove(os.path.join(self.temp_dir, "role2.json"))
+
+        bundle = MetadataBundle(self.temp_dir)
         bundle.root_update_finished()
 
+        # test local load failure, then updating metadata succesfully
+        with self.assertRaises(exceptions.RepositoryError):
+            bundle.load_local_timestamp()
         with open(os.path.join(remote_dir, "timestamp.json"), "rb") as f:
             bundle.update_timestamp(f.read())
         with open(os.path.join(remote_dir, "snapshot.json"), "rb") as f:
@@ -86,16 +90,92 @@ def test_update(self):
             bundle.update_delegated_targets(f.read(), "role2", "role1")
 
         # test loading the metadata (that should now be locally available)
-        bundle = MetadataBundle(self.temporary_directory)
+        bundle = MetadataBundle(self.temp_dir)
         bundle.root_update_finished()
-        self.assertTrue(bundle.load_local_timestamp())
-        self.assertTrue(bundle.load_local_snapshot())
-        self.assertTrue(bundle.load_local_targets())
-        self.assertTrue(bundle.load_local_delegated_targets('role1','targets'))
-        self.assertTrue(bundle.load_local_delegated_targets('role2','role1'))
+        bundle.load_local_timestamp()
+        bundle.load_local_snapshot()
+        bundle.load_local_targets()
+        bundle.load_local_delegated_targets('role1','targets')
+        bundle.load_local_delegated_targets('role2','role1')
 
         # TODO test loading one version, then updating to new versions of each metadata
 
+    def test_local_load_with_invalid_data(self):
+        # Test root and one of the top-level metadata files
+
+        with tempfile.TemporaryDirectory() as tempdir:
+            # Missing root.json
+            with self.assertRaises(exceptions.RepositoryError):
+                MetadataBundle(tempdir)
+
+            # root.json not a json file at all
+            with open(os.path.join(tempdir, "root.json"), "w") as f:
+                f.write("")
+            with self.assertRaises(exceptions.RepositoryError):
+                MetadataBundle(tempdir)
+
+            # root.json does not validate
+            md = Metadata.from_file(os.path.join(self.temp_dir, "root.json"))
+            md.signed.version += 1
+            md.to_file(os.path.join(tempdir, "root.json"))
+            with self.assertRaises(exceptions.RepositoryError):
+                MetadataBundle(tempdir)
+
+            md.signed.version -= 1
+            md.to_file(os.path.join(tempdir, "root.json"))
+            bundle = MetadataBundle(tempdir)
+            bundle.root_update_finished()
+
+            # Missing timestamp.json
+            with self.assertRaises(exceptions.RepositoryError):
+                bundle.load_local_timestamp()
+
+            # timestamp not a json file at all
+            with open(os.path.join(tempdir, "timestamp.json"), "w") as f:
+                f.write("")
+            with self.assertRaises(exceptions.RepositoryError):
+                bundle.load_local_timestamp()
+
+            # timestamp does not validate
+            md = Metadata.from_file(os.path.join(self.temp_dir, "timestamp.json"))
+            md.signed.version += 1
+            md.to_file(os.path.join(tempdir, "timestamp.json"))
+            with self.assertRaises(exceptions.RepositoryError):
+                bundle.load_local_timestamp()
+
+            md.signed.version -= 1
+            md.to_file(os.path.join(tempdir, "timestamp.json"))
+            bundle.load_local_timestamp()
+
+    def test_update_with_invalid_data(self):
+        # Test on of the top level metadata files
+
+        timestamp_md = Metadata.from_file(os.path.join(self.temp_dir, "timestamp.json"))
+
+        # remove all but root.json from local repo
+        os.remove(os.path.join(self.temp_dir, "timestamp.json"))
+        os.remove(os.path.join(self.temp_dir, "snapshot.json"))
+        os.remove(os.path.join(self.temp_dir, "targets.json"))
+        os.remove(os.path.join(self.temp_dir, "role1.json"))
+        os.remove(os.path.join(self.temp_dir, "role2.json"))
+
+        bundle = MetadataBundle(self.temp_dir)
+        bundle.root_update_finished()
+
+        # timestamp not a json file at all
+        with self.assertRaises(exceptions.RepositoryError):
+            bundle.update_timestamp(b"")
+
+        # timestamp does not validate
+        timestamp_md.signed.version += 1
+        data = timestamp_md.to_dict()
+        with self.assertRaises(exceptions.RepositoryError):
+            bundle.update_timestamp(json.dumps(data).encode())
+
+        timestamp_md.signed.version -= 1
+        data = timestamp_md.to_dict()
+        bundle.update_timestamp(json.dumps(data).encode())
+
 
 if __name__ == '__main__':
   utils.configure_test_logging(sys.argv)
diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 8e0e9e1b94..2a102d6094 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -210,19 +210,17 @@ def root_update_finished(self):
         self._root_update_finished = True
         logger.debug("Verified final root.json")
 
-    def load_local_timestamp(self) -> bool:
-        """Load cached timestamp metadata from local storage.
-
-        Returns True if timestamp was succesfully loaded"""
+    def load_local_timestamp(self):
+        """Load cached timestamp metadata from local storage."""
         logger.debug("Loading local timestamp")
 
         try:
             with open(os.path.join(self._path, "timestamp.json"), "rb") as f:
                 self._load_timestamp(f.read())
-            return True
-        except (OSError, exceptions.RepositoryError) as e:
-            logger.debug("Failed to load local timestamp: %s", e)
-            return False
+        except OSError as e:
+            raise exceptions.RepositoryError(
+                "Failed to load local timestamp"
+            ) from e
 
     def update_timestamp(self, data: bytes):
         """Update timestamp metadata with data from remote repository."""
@@ -232,19 +230,17 @@ def update_timestamp(self, data: bytes):
         with open(os.path.join(self._path, "timestamp.json"), "wb") as f:
             f.write(data)
 
-    def load_local_snapshot(self) -> bool:
-        """Load cached snapshot metadata from local storage.
-
-        Returns True if snapshot was succesfully loaded"""
+    def load_local_snapshot(self):
+        """Load cached snapshot metadata from local storage."""
         logger.debug("Loading local snapshot")
 
         try:
             with open(os.path.join(self._path, "snapshot.json"), "rb") as f:
                 self._load_snapshot(f.read())
-            return True
-        except (OSError, exceptions.RepositoryError) as e:
-            logger.debug("Failed to load local snapshot: %s", e)
-            return False
+        except OSError as e:
+            raise exceptions.RepositoryError(
+                "Failed to load local snapshot"
+            ) from e
 
     def update_snapshot(self, data: bytes):
         """Update snapshot metadata with data from remote repository."""
@@ -254,27 +250,21 @@ def update_snapshot(self, data: bytes):
         with open(os.path.join(self._path, "snapshot.json"), "wb") as f:
             f.write(data)
 
-    def load_local_targets(self) -> bool:
-        """Load cached targets metadata from local storage.
-
-        Returns True if targets was succesfully loaded"""
-        return self.load_local_delegated_targets("targets", "root")
+    def load_local_targets(self):
+        """Load cached targets metadata from local storage."""
+        self.load_local_delegated_targets("targets", "root")
 
     def update_targets(self, data: bytes):
         """Update targets metadata with data from remote repository."""
         self.update_delegated_targets(data, "targets", "root")
 
-    def load_local_delegated_targets(
-        self, role_name: str, delegator_name: str
-    ) -> bool:
+    def load_local_delegated_targets(self, role_name: str, delegator_name: str):
         """Load cached metadata for 'role_name' from local storage.
 
         Metadata for 'delegator_name' must be loaded already.
-
-        Returns True if metadata was succesfully loaded"""
+        """
         if self.get(role_name):
             logger.debug("Local %s already loaded", role_name)
-            return True
 
         logger.debug("Loading local %s", role_name)
 
@@ -283,10 +273,10 @@ def load_local_delegated_targets(
                 self._load_delegated_targets(
                     f.read(), role_name, delegator_name
                 )
-            return True
-        except (OSError, exceptions.RepositoryError) as e:
-            logger.debug("Failed to load local %s: %s", role_name, e)
-            return False
+        except OSError as e:
+            raise exceptions.RepositoryError(
+                f"Failed to load local {role_name}"
+            ) from e
 
     def update_delegated_targets(
         self, data: bytes, role_name: str, delegator_name: str = None
@@ -306,7 +296,9 @@ def _load_intermediate_root(self, data: bytes):
         Note that an expired intermediate root is considered valid: expiry is
         only checked for the final root in root_update_finished()."""
         if self._root_update_finished:
-            raise RuntimeError("Cannot update root after root update is finished")
+            raise RuntimeError(
+                "Cannot update root after root update is finished"
+            )
 
         try:
             new_root = Metadata.from_bytes(data)
@@ -405,6 +397,7 @@ def _load_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
             digest_object.update(data)
             observed_hash = digest_object.hexdigest()
             if observed_hash != stored_hash:
+                # TODO: Error should derive from RepositoryError
                 raise exceptions.BadHashError(stored_hash, observed_hash)
 
         try:
@@ -482,6 +475,7 @@ def _load_delegated_targets(
             digest_object.update(data)
             observed_hash = digest_object.hexdigest()
             if observed_hash != stored_hash:
+                # TODO: Error should derive from RepositoryError
                 raise exceptions.BadHashError(stored_hash, observed_hash)
 
         try:

From a371258be8d043502ba511a37b3c8af5e9b1a9bb Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Fri, 14 May 2021 20:43:08 +0300
Subject: [PATCH 30/33] MetadataBundle: Use builtin errors when possible

There's on value in using custom errors when builtins work.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 2a102d6094..7976a2c536 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -117,7 +117,7 @@ def verify_with_threshold(
         raise ValueError("Call is valid only on delegator metadata")
 
     if role is None:
-        raise exceptions.UnknownRoleError
+        raise ValueError(f"Delegated role {role_name} not found")
 
     # verify that delegate is signed by correct threshold of unique keys
     unique_keys = set()

From 6b53ac78d07a8e1dbc4c77ad53c812cce3310dd3 Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Sun, 16 May 2021 12:16:55 +0300
Subject: [PATCH 31/33] Make BadHashError derive from RepositoryError

This is backwards-compatible and means that most (all?) errors resulting
from suspicious or broken metadata are now RepositoryErrors.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/exceptions.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tuf/exceptions.py b/tuf/exceptions.py
index c5f795e0c6..af82c92a89 100755
--- a/tuf/exceptions.py
+++ b/tuf/exceptions.py
@@ -66,7 +66,11 @@ class UnsupportedAlgorithmError(Error):
   """Indicate an error while trying to identify a user-specified algorithm."""
 
 
-class BadHashError(Error):
+class RepositoryError(Error):
+  """Indicate an error with a repository's state, such as a missing file."""
+
+
+class BadHashError(RepositoryError):
   """Indicate an error while checking the value of a hash object."""
 
   def __init__(self, expected_hash, observed_hash):
@@ -97,10 +101,6 @@ class UnknownKeyError(Error):
   """Indicate an error while verifying key-like objects (e.g., keyids)."""
 
 
-class RepositoryError(Error):
-  """Indicate an error with a repository's state, such as a missing file."""
-
-
 class BadVersionNumberError(RepositoryError):
   """Indicate an error for metadata that contains an invalid version number."""
 

From f2cff951a6ebd0834089ad53f64abc44a650d8fd Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Sun, 16 May 2021 12:24:17 +0300
Subject: [PATCH 32/33] MetadataBundle: Don't do any file IO

Remove file IO from MetadataBundle:
* This make the bundle API very clear and easy to understand
* This means caller must now read from and persist data to disk
  but initial prototypes suggest this won't make Updater too
  complex

This change is something we can still back out from if it turns out to
be the wrong decision: the file-persisting MetadataBundle has been tested
and works fine.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tests/test_metadata_bundle.py        | 206 +++++++++----------------
 tuf/client_rework/metadata_bundle.py | 222 +++++++--------------------
 2 files changed, 133 insertions(+), 295 deletions(-)

diff --git a/tests/test_metadata_bundle.py b/tests/test_metadata_bundle.py
index 27b35daa2f..e758e6e7bd 100644
--- a/tests/test_metadata_bundle.py
+++ b/tests/test_metadata_bundle.py
@@ -15,166 +15,108 @@
 logger = logging.getLogger(__name__)
 
 class TestMetadataBundle(unittest.TestCase):
-    @classmethod
-    def setUpClass(cls):
-        cls.temp_dir = tempfile.mkdtemp(dir=os.getcwd())
-
-    @classmethod
-    def tearDownClass(cls):
-        shutil.rmtree(cls.temp_dir)
-
-    def setUp(self):
-        # copy metadata to "local repo"
-        shutil.copytree(
-            os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata'),
-            self.temp_dir,
-            dirs_exist_ok=True
-        )
-
-    def test_local_load(self):
-
-        # test loading all local metadata succesfully
-        bundle = MetadataBundle(self.temp_dir)
-        bundle.root_update_finished()
-        bundle.load_local_timestamp()
-        bundle.load_local_snapshot()
-        bundle.load_local_targets()
-        bundle.load_local_delegated_targets('role1','targets')
-        bundle.load_local_delegated_targets('role2','role1')
-
-        # Make sure loading metadata without its "dependencies" fails
-        bundle = MetadataBundle(self.temp_dir)
-
-        with self.assertRaises(RuntimeError):
-            bundle.load_local_timestamp()
-        bundle.root_update_finished()
-        with self.assertRaises(RuntimeError):
-            bundle.load_local_snapshot()
-        bundle.load_local_timestamp()
-        with self.assertRaises(RuntimeError):
-            bundle.load_local_targets()
-        bundle.load_local_snapshot()
-        with self.assertRaises(RuntimeError):
-            bundle.load_local_delegated_targets('role1','targets')
-        bundle.load_local_targets()
-        with self.assertRaises(RuntimeError):
-            bundle.load_local_delegated_targets('role2','role1')
-        bundle.load_local_delegated_targets('role1','targets')
-        bundle.load_local_delegated_targets('role2','role1')
 
     def test_update(self):
-        remote_dir = os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata')
-
-        # remove all but root.json from local repo
-        os.remove(os.path.join(self.temp_dir, "timestamp.json"))
-        os.remove(os.path.join(self.temp_dir, "snapshot.json"))
-        os.remove(os.path.join(self.temp_dir, "targets.json"))
-        os.remove(os.path.join(self.temp_dir, "role1.json"))
-        os.remove(os.path.join(self.temp_dir, "role2.json"))
+        repo_dir = os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata')
 
-        bundle = MetadataBundle(self.temp_dir)
+        with open(os.path.join(repo_dir, "root.json"), "rb") as f:
+            bundle = MetadataBundle(f.read())
         bundle.root_update_finished()
 
-        # test local load failure, then updating metadata succesfully
-        with self.assertRaises(exceptions.RepositoryError):
-            bundle.load_local_timestamp()
-        with open(os.path.join(remote_dir, "timestamp.json"), "rb") as f:
+        with open(os.path.join(repo_dir, "timestamp.json"), "rb") as f:
             bundle.update_timestamp(f.read())
-        with open(os.path.join(remote_dir, "snapshot.json"), "rb") as f:
+        with open(os.path.join(repo_dir, "snapshot.json"), "rb") as f:
             bundle.update_snapshot(f.read())
-        with open(os.path.join(remote_dir, "targets.json"), "rb") as f:
+        with open(os.path.join(repo_dir, "targets.json"), "rb") as f:
             bundle.update_targets(f.read())
-        with open(os.path.join(remote_dir, "role1.json"), "rb") as f:
+        with open(os.path.join(repo_dir, "role1.json"), "rb") as f:
             bundle.update_delegated_targets(f.read(), "role1", "targets")
-        with open(os.path.join(remote_dir, "role2.json"), "rb") as f:
+        with open(os.path.join(repo_dir, "role2.json"), "rb") as f:
             bundle.update_delegated_targets(f.read(), "role2", "role1")
 
-        # test loading the metadata (that should now be locally available)
-        bundle = MetadataBundle(self.temp_dir)
-        bundle.root_update_finished()
-        bundle.load_local_timestamp()
-        bundle.load_local_snapshot()
-        bundle.load_local_targets()
-        bundle.load_local_delegated_targets('role1','targets')
-        bundle.load_local_delegated_targets('role2','role1')
-
-        # TODO test loading one version, then updating to new versions of each metadata
+    def test_out_of_order_ops(self):
+        repo_dir = os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata')
+        data={}
+        for md in ["root", "timestamp", "snapshot", "targets", "role1"]:
+            with open(os.path.join(repo_dir, f"{md}.json"), "rb") as f:
+                data[md] = f.read()
 
-    def test_local_load_with_invalid_data(self):
-        # Test root and one of the top-level metadata files
+        bundle = MetadataBundle(data["root"])
 
-        with tempfile.TemporaryDirectory() as tempdir:
-            # Missing root.json
-            with self.assertRaises(exceptions.RepositoryError):
-                MetadataBundle(tempdir)
+        # Update timestamp before root is finished
+        with self.assertRaises(RuntimeError):
+            bundle.update_timestamp(data["timestamp"])
 
-            # root.json not a json file at all
-            with open(os.path.join(tempdir, "root.json"), "w") as f:
-                f.write("")
-            with self.assertRaises(exceptions.RepositoryError):
-                MetadataBundle(tempdir)
+        bundle.root_update_finished()
+        with self.assertRaises(RuntimeError):
+            bundle.root_update_finished()
 
-            # root.json does not validate
-            md = Metadata.from_file(os.path.join(self.temp_dir, "root.json"))
-            md.signed.version += 1
-            md.to_file(os.path.join(tempdir, "root.json"))
-            with self.assertRaises(exceptions.RepositoryError):
-                MetadataBundle(tempdir)
+        # Update snapshot before timestamp
+        with self.assertRaises(RuntimeError):
+            bundle.update_snapshot(data["snapshot"])
 
-            md.signed.version -= 1
-            md.to_file(os.path.join(tempdir, "root.json"))
-            bundle = MetadataBundle(tempdir)
-            bundle.root_update_finished()
+        bundle.update_timestamp(data["timestamp"])
 
-            # Missing timestamp.json
-            with self.assertRaises(exceptions.RepositoryError):
-                bundle.load_local_timestamp()
+        # Update targets before snapshot
+        with self.assertRaises(RuntimeError):
+            bundle.update_targets(data["targets"])
 
-            # timestamp not a json file at all
-            with open(os.path.join(tempdir, "timestamp.json"), "w") as f:
-                f.write("")
-            with self.assertRaises(exceptions.RepositoryError):
-                bundle.load_local_timestamp()
+        bundle.update_snapshot(data["snapshot"])
 
-            # timestamp does not validate
-            md = Metadata.from_file(os.path.join(self.temp_dir, "timestamp.json"))
-            md.signed.version += 1
-            md.to_file(os.path.join(tempdir, "timestamp.json"))
-            with self.assertRaises(exceptions.RepositoryError):
-                bundle.load_local_timestamp()
+        #update timestamp after snapshot
+        with self.assertRaises(RuntimeError):
+            bundle.update_timestamp(data["timestamp"])
 
-            md.signed.version -= 1
-            md.to_file(os.path.join(tempdir, "timestamp.json"))
-            bundle.load_local_timestamp()
+        # Update delegated targets before targets
+        with self.assertRaises(RuntimeError):
+            bundle.update_delegated_targets(data["role1"], "role1", "targets")
 
-    def test_update_with_invalid_data(self):
-        # Test on of the top level metadata files
+        bundle.update_targets(data["targets"])
+        bundle.update_delegated_targets(data["role1"], "role1", "targets")
 
-        timestamp_md = Metadata.from_file(os.path.join(self.temp_dir, "timestamp.json"))
+    def test_update_with_invalid_json(self):
+        repo_dir = os.path.join(os.getcwd(), 'repository_data', 'repository', 'metadata')
+        data={}
+        for md in ["root", "timestamp", "snapshot", "targets", "role1"]:
+            with open(os.path.join(repo_dir, f"{md}.json"), "rb") as f:
+                data[md] = f.read()
 
-        # remove all but root.json from local repo
-        os.remove(os.path.join(self.temp_dir, "timestamp.json"))
-        os.remove(os.path.join(self.temp_dir, "snapshot.json"))
-        os.remove(os.path.join(self.temp_dir, "targets.json"))
-        os.remove(os.path.join(self.temp_dir, "role1.json"))
-        os.remove(os.path.join(self.temp_dir, "role2.json"))
+        # root.json not a json file at all
+        with self.assertRaises(exceptions.RepositoryError):
+            MetadataBundle(b"")
+        # root.json is invalid
+        root = Metadata.from_bytes(data["root"])
+        root.signed.version += 1
+        with self.assertRaises(exceptions.RepositoryError):
+            MetadataBundle(json.dumps(root.to_dict()).encode())
 
-        bundle = MetadataBundle(self.temp_dir)
+        bundle = MetadataBundle(data["root"])
         bundle.root_update_finished()
 
-        # timestamp not a json file at all
-        with self.assertRaises(exceptions.RepositoryError):
-            bundle.update_timestamp(b"")
+        top_level_md = [
+            (data["timestamp"], bundle.update_timestamp),
+            (data["snapshot"], bundle.update_snapshot),
+            (data["targets"], bundle.update_targets),
+        ]
+        for metadata, update_func in top_level_md:
+            # metadata is not json
+            with self.assertRaises(exceptions.RepositoryError):
+                update_func(b"")
+            # metadata is invalid
+            md = Metadata.from_bytes(metadata)
+            md.signed.version += 1
+            with self.assertRaises(exceptions.RepositoryError):
+                update_func(json.dumps(md.to_dict()).encode())
+
+            # metadata is of wrong type
+            with self.assertRaises(exceptions.RepositoryError):
+                update_func(data["root"])
+
+            update_func(metadata)
 
-        # timestamp does not validate
-        timestamp_md.signed.version += 1
-        data = timestamp_md.to_dict()
-        with self.assertRaises(exceptions.RepositoryError):
-            bundle.update_timestamp(json.dumps(data).encode())
 
-        timestamp_md.signed.version -= 1
-        data = timestamp_md.to_dict()
-        bundle.update_timestamp(json.dumps(data).encode())
+    # TODO test updating over initial metadata (new keys, newer timestamp, etc)
+    # TODO test the actual specification checks
 
 
 if __name__ == '__main__':
diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 7976a2c536..470f750ebc 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -6,83 +6,66 @@
 MetadataBundle keeps track of current valid set of metadata for the client,
 and handles almost every step of the "Detailed client workflow" (
 https://theupdateframework.github.io/specification/latest#detailed-client-workflow)
-in the TUF specification (the remaining steps are download related). The
-bundle takes care of persisting valid metadata on disk and loading local
-metadata from disk.
+in the TUF specification: the remaining steps are related to filesystem and
+network IO which is not handled here.
 
 Loaded metadata can be accessed via the index access with rolename as key
-or, in the case of top-level metadata using the helper properties like
-'MetadataBundle.root'
+(bundle["root"]) or, in the case of top-level metadata using the helper
+properties (bundle.root).
 
 The rules for top-level metadata are
  * Metadata is loadable only if metadata it depends on is loaded
  * Metadata is immutable if any metadata depending on it has been loaded
  * Metadata must be loaded/updated in order:
    root -> timestamp -> snapshot -> targets -> (other delegated targets)
- * For each metadata either local load or the remote update must succeed
- * Caller should try loading local version before updating metadata from remote
-   (the exception is root where local data is loaded at MetadataBundle
-   initialization: the initialization fails if local data cannot be loaded)
 
-Exceptions are raised if metadata fails to load in any way. The exception
-to this is local loads -- only local root metadata needs to be valid:
-other local metadata is allowed to be invalid (e.g. no longer signed):
-it won't be loaded but there will not be an exception.
+Exceptions are raised if metadata fails to load in any way.
 
-Example (with hypothetical download function):
+Example of loading root, timestamp and snapshot:
 
->>> # Load local root
->>> bundle = MetadataBundle("path/to/metadata")
+>>> # Load local root (RepositoryErrors here stop the update)
+>>> with open(root_path, "rb") as f:
+>>>     bundle = MetadataBundle(f.read())
 >>>
->>> # update root until no more are available from remote
+>>> # update root from remote until no more are available
 >>> with download("root", bundle.root.signed.version + 1) as f:
 >>>     bundle.update_root(f.read())
 >>> # ...
 >>> bundle.root_update_finished()
 >>>
->>> # load timestamp, then update from remote
->>> bundle.load_local_timestamp()
+>>> # load local timestamp, then update from remote
+>>> try:
+>>>     with open(timestamp_path, "rb") as f:
+>>>         bundle.update_timestamp(f.read())
+>>> except (RepositoryError, OSError):
+>>>     pass # failure to load a local file is ok
+>>>
 >>> with download("timestamp") as f:
 >>>     bundle.update_timestamp(f.read())
 >>>
->>> # load snapshot, update from remote if needed
->>> if not bundle.load_local_snapshot():
->>>     # TODO get version from timestamp
+>>> # load local snapshot, then update from remote if needed
+>>> try:
+>>>     with open(snapshot_path, "rb") as f:
+>>>         bundle.update_snapshot(f.read())
+>>> except (RepositoryError, OSError):
+>>>     # local snapshot is not valid, load from remote
+>>>     # (RepositoryErrors here stop the update)
 >>>     with download("snapshot", version) as f:
 >>>         bundle.update_snapshot(f.read())
->>>
->>> # load local targets, update from remote if needed
->>> if not bundle.load_local_targets():
->>>     # TODO get version from snapshot
->>>     with download("targets", version) as f:
->>>         bundle.update_targets(f.read())
->>>
->>> # load local delegated role, update from remote if needed
->>> if not bundle.load_local_delegated_targets("rolename", "targets"):
->>>     # TODO get version from snapshot
->>>     with download("rolename", version) as f:
->>>         bundle.update_targets(f.read(), "rolename", "targets")
-
 
 TODO:
- * exceptions are all over the place: the idea is that client could just handle
+ * exceptions are not final: the idea is that client could just handle
    a generic RepositoryError that covers every issue that server provided
    metadata could inflict (other errors would be user errors), but this is not
    yet the case
  * usefulness of root_update_finished() can be debated: it could be done
    in the beginning of load_timestamp()...
- * there are some divergences from spec: in general local metadata files are
-   not deleted (they just won't succesfully load)
- * a bit of repetition
- * No tests!
- * Naming maybe not final?
  * some metadata interactions might work better in Metadata itself
  * Progress through Specification update process should be documented
    (not sure yet how: maybe a spec_logger that logs specification events?)
 """
 
 import logging
-import os
 from collections import abc
 from datetime import datetime
 from typing import Dict, Iterator, Optional
@@ -142,23 +125,16 @@ class MetadataBundle(abc.Mapping):
     update the metadata with the caller making decisions on what is updated.
     """
 
-    def __init__(self, repository_path: str):
-        """Initialize by loading root metadata from disk"""
-        self._path = repository_path
+    def __init__(self, data: bytes):
+        """Initialize by loading trusted root metadata"""
         self._bundle = {}  # type: Dict[str: Metadata]
         self.reference_time = datetime.utcnow()
         self._root_update_finished = False
 
         # Load and validate the local root metadata
         # Valid root metadata is required
-        logger.debug("Loading local root")
-        try:
-            with open(os.path.join(self._path, "root.json"), "rb") as f:
-                self._load_intermediate_root(f.read())
-        except (OSError, exceptions.RepositoryError) as e:
-            raise exceptions.RepositoryError(
-                "Failed to load local root metadata"
-            ) from e
+        logger.debug("Updating initial trusted root")
+        self.update_root(data)
 
     # Implement Mapping
     def __getitem__(self, key: str) -> Metadata:
@@ -187,110 +163,8 @@ def snapshot(self) -> Optional[Metadata]:
     def targets(self) -> Optional[Metadata]:
         return self._bundle.get("targets")
 
-    # Public methods
+    # Methods for updating metadata
     def update_root(self, data: bytes):
-        """Update root metadata with data from remote repository."""
-        logger.debug("Updating root")
-
-        self._load_intermediate_root(data)
-        with open(os.path.join(self._path, "root.json"), "wb") as f:
-            f.write(data)
-
-    def root_update_finished(self):
-        """Mark root metadata as final."""
-        if self._root_update_finished:
-            raise RuntimeError("Root update is already finished")
-
-        if self.root.signed.is_expired(self.reference_time):
-            raise exceptions.ExpiredMetadataError("New root.json is expired")
-
-        # We skip specification step 5.3.11: deleting timestamp and snapshot
-        # with rotated keys is not needed as they will be invalid, are not
-        # loaded and cannot be loaded
-        self._root_update_finished = True
-        logger.debug("Verified final root.json")
-
-    def load_local_timestamp(self):
-        """Load cached timestamp metadata from local storage."""
-        logger.debug("Loading local timestamp")
-
-        try:
-            with open(os.path.join(self._path, "timestamp.json"), "rb") as f:
-                self._load_timestamp(f.read())
-        except OSError as e:
-            raise exceptions.RepositoryError(
-                "Failed to load local timestamp"
-            ) from e
-
-    def update_timestamp(self, data: bytes):
-        """Update timestamp metadata with data from remote repository."""
-        logger.debug("Updating timestamp")
-
-        self._load_timestamp(data)
-        with open(os.path.join(self._path, "timestamp.json"), "wb") as f:
-            f.write(data)
-
-    def load_local_snapshot(self):
-        """Load cached snapshot metadata from local storage."""
-        logger.debug("Loading local snapshot")
-
-        try:
-            with open(os.path.join(self._path, "snapshot.json"), "rb") as f:
-                self._load_snapshot(f.read())
-        except OSError as e:
-            raise exceptions.RepositoryError(
-                "Failed to load local snapshot"
-            ) from e
-
-    def update_snapshot(self, data: bytes):
-        """Update snapshot metadata with data from remote repository."""
-        logger.debug("Updating snapshot")
-
-        self._load_snapshot(data)
-        with open(os.path.join(self._path, "snapshot.json"), "wb") as f:
-            f.write(data)
-
-    def load_local_targets(self):
-        """Load cached targets metadata from local storage."""
-        self.load_local_delegated_targets("targets", "root")
-
-    def update_targets(self, data: bytes):
-        """Update targets metadata with data from remote repository."""
-        self.update_delegated_targets(data, "targets", "root")
-
-    def load_local_delegated_targets(self, role_name: str, delegator_name: str):
-        """Load cached metadata for 'role_name' from local storage.
-
-        Metadata for 'delegator_name' must be loaded already.
-        """
-        if self.get(role_name):
-            logger.debug("Local %s already loaded", role_name)
-
-        logger.debug("Loading local %s", role_name)
-
-        try:
-            with open(os.path.join(self._path, f"{role_name}.json"), "rb") as f:
-                self._load_delegated_targets(
-                    f.read(), role_name, delegator_name
-                )
-        except OSError as e:
-            raise exceptions.RepositoryError(
-                f"Failed to load local {role_name}"
-            ) from e
-
-    def update_delegated_targets(
-        self, data: bytes, role_name: str, delegator_name: str = None
-    ):
-        """Update 'rolename' metadata with data from remote repository.
-
-        Metadata for 'delegator_name' must be loaded already."""
-        logger.debug("Updating %s", role_name)
-
-        self._load_delegated_targets(data, role_name, delegator_name)
-        with open(os.path.join(self._path, f"{role_name}.json"), "wb") as f:
-            f.write(data)
-
-    def _load_intermediate_root(self, data: bytes):
         """Verifies and loads 'data' as new root metadata.
 
         Note that an expired intermediate root is considered valid: expiry is
@@ -299,6 +173,7 @@ def _load_intermediate_root(self, data: bytes):
             raise RuntimeError(
                 "Cannot update root after root update is finished"
             )
+        logger.debug("Updating root")
 
         try:
             new_root = Metadata.from_bytes(data)
@@ -328,9 +203,23 @@ def _load_intermediate_root(self, data: bytes):
             )
 
         self._bundle["root"] = new_root
-        logger.debug("Loaded root")
+        logger.debug("Updated root")
+
+    def root_update_finished(self):
+        """Mark root metadata as final."""
+        if self._root_update_finished:
+            raise RuntimeError("Root update is already finished")
+
+        if self.root.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError("New root.json is expired")
 
-    def _load_timestamp(self, data: bytes):
+        # We skip specification step 5.3.11: deleting timestamp and snapshot
+        # with rotated keys is not needed as they will be invalid, are not
+        # loaded and cannot be loaded
+        self._root_update_finished = True
+        logger.debug("Verified final root.json")
+
+    def update_timestamp(self, data: bytes):
         """Verifies and loads 'data' as new timestamp metadata."""
         if not self._root_update_finished:
             # root_update_finished() not called
@@ -377,16 +266,17 @@ def _load_timestamp(self, data: bytes):
             raise exceptions.ExpiredMetadataError("New timestamp is expired")
 
         self._bundle["timestamp"] = new_timestamp
-        logger.debug("Loaded timestamp")
+        logger.debug("Updated timestamp")
 
     # TODO: remove pylint disable once the hash verification is in metadata.py
-    def _load_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
+    def update_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
         """Verifies and loads 'data' as new snapshot metadata."""
 
         if self.timestamp is None:
             raise RuntimeError("Cannot update snapshot before timestamp")
         if self.targets is not None:
             raise RuntimeError("Cannot update snapshot after targets")
+        logger.debug("Updating snapshot")
 
         meta = self.timestamp.signed.meta["snapshot.json"]
 
@@ -446,9 +336,13 @@ def _load_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
             raise exceptions.ExpiredMetadataError("New snapshot is expired")
 
         self._bundle["snapshot"] = new_snapshot
-        logger.debug("Loaded snapshot")
+        logger.debug("Updated snapshot")
 
-    def _load_delegated_targets(
+    def update_targets(self, data: bytes):
+        """Verifies and loads 'data' as new top-level targets metadata."""
+        self.update_delegated_targets(data, "targets", "root")
+
+    def update_delegated_targets(
         self, data: bytes, role_name: str, delegator_name: str
     ):
         """Verifies and loads 'data' as new metadata for target 'role_name'.
@@ -462,6 +356,8 @@ def _load_delegated_targets(
         if delegator is None:
             raise RuntimeError("Cannot load targets before delegator")
 
+        logger.debug("Updating %s delegated by %s", role_name, delegator_name)
+
         # Verify against the hashes in snapshot, if any
         meta = self.snapshot.signed.meta.get(f"{role_name}.json")
         if meta is None:
@@ -504,4 +400,4 @@ def _load_delegated_targets(
             raise exceptions.ExpiredMetadataError(f"New {role_name} is expired")
 
         self._bundle[role_name] = new_delegate
-        logger.debug("Loaded %s delegated by %s", role_name, delegator_name)
+        logger.debug("Updated %s delegated by %s", role_name, delegator_name)

From 377eac18f6313181c77b609dda3e8309d97da7aa Mon Sep 17 00:00:00 2001
From: Jussi Kukkonen <jkukkonen@vmware.com>
Date: Mon, 17 May 2021 18:46:05 +0300
Subject: [PATCH 33/33] MetadataBundle: Improve docstrings

Document arguments and exceptions, improve prose in general. Remove
mention of local file deletion now that file IO is not done here.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
---
 tuf/client_rework/metadata_bundle.py | 96 ++++++++++++++++++++++------
 1 file changed, 77 insertions(+), 19 deletions(-)

diff --git a/tuf/client_rework/metadata_bundle.py b/tuf/client_rework/metadata_bundle.py
index 470f750ebc..87d4d0e62c 100644
--- a/tuf/client_rework/metadata_bundle.py
+++ b/tuf/client_rework/metadata_bundle.py
@@ -19,6 +19,7 @@
  * Metadata must be loaded/updated in order:
    root -> timestamp -> snapshot -> targets -> (other delegated targets)
 
+
 Exceptions are raised if metadata fails to load in any way.
 
 Example of loading root, timestamp and snapshot:
@@ -121,46 +122,63 @@ def verify_with_threshold(
 class MetadataBundle(abc.Mapping):
     """Internal class to keep track of valid metadata in Updater
 
-    MetadataBundle ensures that metadata is valid. It provides easy ways to
-    update the metadata with the caller making decisions on what is updated.
+    MetadataBundle ensures that the collection of metadata in the bundle is
+    valid. It provides easy ways to update the metadata with the caller making
+    decisions on what is updated.
     """
 
-    def __init__(self, data: bytes):
-        """Initialize by loading trusted root metadata"""
+    def __init__(self, root_data: bytes):
+        """Initialize bundle by loading trusted root metadata
+
+        Args:
+            root_data: Trusted root metadata as bytes. Note that this metadata
+                will only be verified by itself: it is the source of trust for
+                all metadata in the bundle.
+
+        Raises:
+            RepositoryError: Metadata failed to load or verify. The actual
+                error type and content will contain more details.
+        """
         self._bundle = {}  # type: Dict[str: Metadata]
         self.reference_time = datetime.utcnow()
         self._root_update_finished = False
 
-        # Load and validate the local root metadata
-        # Valid root metadata is required
+        # Load and validate the local root metadata. Valid initial trusted root
+        # metadata is required
         logger.debug("Updating initial trusted root")
-        self.update_root(data)
+        self.update_root(root_data)
 
-    # Implement Mapping
-    def __getitem__(self, key: str) -> Metadata:
-        return self._bundle[key]
+    def __getitem__(self, role: str) -> Metadata:
+        """Returns current Metadata for 'role'"""
+        return self._bundle[role]
 
     def __len__(self) -> int:
+        """Returns number of Metadata objects in bundle"""
         return len(self._bundle)
 
     def __iter__(self) -> Iterator[Metadata]:
+        """Returns iterator over all Metadata objects in bundle"""
         return iter(self._bundle)
 
     # Helper properties for top level metadata
     @property
     def root(self) -> Optional[Metadata]:
+        """Current root Metadata or None"""
         return self._bundle.get("root")
 
     @property
     def timestamp(self) -> Optional[Metadata]:
+        """Current timestamp Metadata or None"""
         return self._bundle.get("timestamp")
 
     @property
     def snapshot(self) -> Optional[Metadata]:
+        """Current snapshot Metadata or None"""
         return self._bundle.get("snapshot")
 
     @property
     def targets(self) -> Optional[Metadata]:
+        """Current targets Metadata or None"""
         return self._bundle.get("targets")
 
     # Methods for updating metadata
@@ -168,7 +186,15 @@ def update_root(self, data: bytes):
         """Verifies and loads 'data' as new root metadata.
 
         Note that an expired intermediate root is considered valid: expiry is
-        only checked for the final root in root_update_finished()."""
+        only checked for the final root in root_update_finished().
+
+        Args:
+            data: unverified new root metadata as bytes
+
+        Raises:
+            RepositoryError: Metadata failed to load or verify. The actual
+                error type and content will contain more details.
+        """
         if self._root_update_finished:
             raise RuntimeError(
                 "Cannot update root after root update is finished"
@@ -206,21 +232,30 @@ def update_root(self, data: bytes):
         logger.debug("Updated root")
 
     def root_update_finished(self):
-        """Mark root metadata as final."""
+        """Marks root metadata as final and verifies it is not expired
+
+        Raises:
+            ExpiredMetadataError: The final root metadata is expired.
+        """
         if self._root_update_finished:
             raise RuntimeError("Root update is already finished")
 
         if self.root.signed.is_expired(self.reference_time):
             raise exceptions.ExpiredMetadataError("New root.json is expired")
 
-        # We skip specification step 5.3.11: deleting timestamp and snapshot
-        # with rotated keys is not needed as they will be invalid, are not
-        # loaded and cannot be loaded
         self._root_update_finished = True
         logger.debug("Verified final root.json")
 
     def update_timestamp(self, data: bytes):
-        """Verifies and loads 'data' as new timestamp metadata."""
+        """Verifies and loads 'data' as new timestamp metadata.
+
+        Args:
+            data: unverified new timestamp metadata as bytes
+
+        Raises:
+            RepositoryError: Metadata failed to load or verify. The actual
+                error type and content will contain more details.
+        """
         if not self._root_update_finished:
             # root_update_finished() not called
             raise RuntimeError("Cannot update timestamp before root")
@@ -270,7 +305,15 @@ def update_timestamp(self, data: bytes):
 
     # TODO: remove pylint disable once the hash verification is in metadata.py
     def update_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
-        """Verifies and loads 'data' as new snapshot metadata."""
+        """Verifies and loads 'data' as new snapshot metadata.
+
+        Args:
+            data: unverified new snapshot metadata as bytes
+
+        Raises:
+            RepositoryError: Metadata failed to load or verify. The actual
+                error type and content will contain more details.
+        """
 
         if self.timestamp is None:
             raise RuntimeError("Cannot update snapshot before timestamp")
@@ -339,7 +382,15 @@ def update_snapshot(self, data: bytes):  # pylint: disable=too-many-branches
         logger.debug("Updated snapshot")
 
     def update_targets(self, data: bytes):
-        """Verifies and loads 'data' as new top-level targets metadata."""
+        """Verifies and loads 'data' as new top-level targets metadata.
+
+        Args:
+            data: unverified new targets metadata as bytes
+
+        Raises:
+            RepositoryError: Metadata failed to load or verify. The actual
+                error type and content will contain more details.
+        """
         self.update_delegated_targets(data, "targets", "root")
 
     def update_delegated_targets(
@@ -347,7 +398,14 @@ def update_delegated_targets(
     ):
         """Verifies and loads 'data' as new metadata for target 'role_name'.
 
-        Raises if verification fails
+        Args:
+            data: unverified new metadata as bytes
+            role_name: The role name of the new metadata
+            delegator_name: The name of the role delegating the new metadata
+
+        Raises:
+            RepositoryError: Metadata failed to load or verify. The actual
+                error type and content will contain more details.
         """
         if self.snapshot is None:
             raise RuntimeError("Cannot load targets before snapshot")