From 38b85cb8dbffa94bde24edb889a49c5559abc2f1 Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Fri, 9 Jul 2021 14:06:36 +0300 Subject: [PATCH 1/9] Add ngclient to mypy Extend mypy to include all files under ngclient. Signed-off-by: Teodora Sechkova --- setup.cfg | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index 26c0d092eb..f75bf17900 100644 --- a/setup.cfg +++ b/setup.cfg @@ -15,7 +15,10 @@ warn_unreachable = True strict_equality = True disallow_untyped_defs = True disallow_untyped_calls = True -files = tuf/api/, tuf/exceptions.py +files = + tuf/api/, + tuf/ngclient, + tuf/exceptions.py [mypy-securesystemslib.*] ignore_missing_imports = True From 191a1e5e3d63876b9ef60f436a6f7dda889fde43 Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Fri, 9 Jul 2021 14:10:24 +0300 Subject: [PATCH 2/9] Add type annotations to download.py and fetcher.py Add the stub for the requests package (types-requests) to requirements-tests.txt. Add urllib3 to the ignored imports. The project seems to have added type annotations already but has not released a version including them yet. Signed-off-by: Teodora Sechkova --- requirements-test.txt | 1 + setup.cfg | 3 +++ tuf/ngclient/_internal/requests_fetcher.py | 8 ++++---- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/requirements-test.txt b/requirements-test.txt index 692d53953e..fed1a6de24 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -13,3 +13,4 @@ isort pylint mypy bandit +types-requests diff --git a/setup.cfg b/setup.cfg index f75bf17900..5baa2d2173 100644 --- a/setup.cfg +++ b/setup.cfg @@ -22,3 +22,6 @@ files = [mypy-securesystemslib.*] ignore_missing_imports = True + +[mypy-urllib3.*] +ignore_missing_imports = True diff --git a/tuf/ngclient/_internal/requests_fetcher.py b/tuf/ngclient/_internal/requests_fetcher.py index a26231c5bb..d2f24ef25d 100644 --- a/tuf/ngclient/_internal/requests_fetcher.py +++ b/tuf/ngclient/_internal/requests_fetcher.py @@ -7,7 +7,7 @@ import logging import time -from typing import Iterator, Optional +from typing import Dict, Iterator, Optional from urllib import parse # Imports @@ -31,7 +31,7 @@ class RequestsFetcher(FetcherInterface): session per scheme+hostname combination. """ - def __init__(self): + def __init__(self) -> None: # http://docs.python-requests.org/en/master/user/advanced/#session-objects: # # "The Session object allows you to persist certain parameters across @@ -46,7 +46,7 @@ def __init__(self): # improve efficiency, but avoiding sharing state between different # hosts-scheme combinations to minimize subtle security issues. # Some cookies may not be HTTP-safe. - self._sessions = {} + self._sessions: Dict[str, requests.Session] = {} # Default settings self.socket_timeout: int = 4 # seconds @@ -146,7 +146,7 @@ def _chunks( finally: response.close() - def _get_session(self, url): + def _get_session(self, url: str) -> requests.Session: """Returns a different customized requests.Session per schema+hostname combination. """ From b6e02bde4713c184c61fbfa6a0c0d6b79d345126 Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Fri, 9 Jul 2021 14:16:40 +0300 Subject: [PATCH 3/9] Replace BinaryIO with IO[bytes] in metadata.py Needed in order to be compatible with the return type of download_file (TemporaryFile is typed as IO[bytes]). BinaryIO is a subclass of IO[bytes]. Signed-off-by: Teodora Sechkova --- tuf/api/metadata.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tuf/api/metadata.py b/tuf/api/metadata.py index 8f5f48299d..6d1de28477 100644 --- a/tuf/api/metadata.py +++ b/tuf/api/metadata.py @@ -24,8 +24,8 @@ from collections import OrderedDict from datetime import datetime, timedelta from typing import ( + IO, Any, - BinaryIO, ClassVar, Dict, Generic, @@ -753,7 +753,7 @@ class BaseFile: @staticmethod def _verify_hashes( - data: Union[bytes, BinaryIO], expected_hashes: Dict[str, str] + data: Union[bytes, IO[bytes]], expected_hashes: Dict[str, str] ) -> None: """Verifies that the hash of 'data' matches 'expected_hashes'""" is_bytes = isinstance(data, bytes) @@ -782,7 +782,7 @@ def _verify_hashes( @staticmethod def _verify_length( - data: Union[bytes, BinaryIO], expected_length: int + data: Union[bytes, IO[bytes]], expected_length: int ) -> None: """Verifies that the length of 'data' matches 'expected_length'""" if isinstance(data, bytes): @@ -867,7 +867,7 @@ def to_dict(self) -> Dict[str, Any]: return res_dict - def verify_length_and_hashes(self, data: Union[bytes, BinaryIO]) -> None: + def verify_length_and_hashes(self, data: Union[bytes, IO[bytes]]) -> None: """Verifies that the length and hashes of "data" match expected values. Args: @@ -1182,7 +1182,7 @@ def to_dict(self) -> Dict[str, Any]: **self.unrecognized_fields, } - def verify_length_and_hashes(self, data: Union[bytes, BinaryIO]) -> None: + def verify_length_and_hashes(self, data: Union[bytes, IO[bytes]]) -> None: """Verifies that length and hashes of "data" match expected values. Args: From ea76a30d903a2cb519c76169004c134c44177d11 Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Fri, 9 Jul 2021 14:21:05 +0300 Subject: [PATCH 4/9] Add missing type annotations to updater Add missing annotations and partially resolve mypy errors in updater.py and trusted_metadata_set.py Signed-off-by: Teodora Sechkova --- .../_internal/trusted_metadata_set.py | 16 ++++++------ tuf/ngclient/updater.py | 25 ++++++++----------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/tuf/ngclient/_internal/trusted_metadata_set.py b/tuf/ngclient/_internal/trusted_metadata_set.py index 608889fb2a..62832c51b0 100644 --- a/tuf/ngclient/_internal/trusted_metadata_set.py +++ b/tuf/ngclient/_internal/trusted_metadata_set.py @@ -92,7 +92,7 @@ def __init__(self, root_data: bytes): RepositoryError: Metadata failed to load or verify. The actual error type and content will contain more details. """ - self._trusted_set = {} # type: Dict[str: Metadata] + self._trusted_set: Dict[str, Metadata] = {} self.reference_time = datetime.utcnow() # Load and validate the local root metadata. Valid initial trusted root @@ -134,7 +134,7 @@ def targets(self) -> Optional[Metadata]: return self._trusted_set.get("targets") # Methods for updating metadata - def update_root(self, data: bytes): + def update_root(self, data: bytes) -> None: """Verifies and loads 'data' as new root metadata. Note that an expired intermediate root is considered valid: expiry is @@ -175,7 +175,7 @@ def update_root(self, data: bytes): self._trusted_set["root"] = new_root logger.debug("Updated root") - def update_timestamp(self, data: bytes): + def update_timestamp(self, data: bytes) -> None: """Verifies and loads 'data' as new timestamp metadata. Note that an expired intermediate timestamp is considered valid so it @@ -237,7 +237,7 @@ def update_timestamp(self, data: bytes): self._trusted_set["timestamp"] = new_timestamp logger.debug("Updated timestamp") - def update_snapshot(self, data: bytes): + def update_snapshot(self, data: bytes) -> None: """Verifies and loads 'data' as new snapshot metadata. Note that intermediate snapshot is considered valid even if it is @@ -314,7 +314,9 @@ def update_snapshot(self, data: bytes): self._trusted_set["snapshot"] = new_snapshot logger.debug("Updated snapshot") - def _check_final_snapshot(self): + def _check_final_snapshot(self) -> None: + """Check snapshot expiry and version before targets is updated""" + if self.snapshot.signed.is_expired(self.reference_time): raise exceptions.ExpiredMetadataError("snapshot.json is expired") @@ -328,7 +330,7 @@ def _check_final_snapshot(self): f"got {self.snapshot.signed.version}" ) - def update_targets(self, data: bytes): + def update_targets(self, data: bytes) -> None: """Verifies and loads 'data' as new top-level targets metadata. Args: @@ -342,7 +344,7 @@ def update_targets(self, data: bytes): def update_delegated_targets( self, data: bytes, role_name: str, delegator_name: str - ): + ) -> None: """Verifies and loads 'data' as new metadata for target 'role_name'. Args: diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index 1acf2c40bd..d0e2db1f53 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -112,12 +112,7 @@ def __init__( # Read trusted local root metadata data = self._load_local_metadata("root") self._trusted_set = trusted_metadata_set.TrustedMetadataSet(data) - - if fetcher is None: - self._fetcher = requests_fetcher.RequestsFetcher() - else: - self._fetcher = fetcher - + self._fetcher = fetcher or requests_fetcher.RequestsFetcher() self.config = config or UpdaterConfig() def refresh(self) -> None: @@ -225,7 +220,7 @@ def download_target( targetinfo: TargetFile, destination_directory: str, target_base_url: Optional[str] = None, - ): + ) -> None: """Downloads the target file specified by 'targetinfo'. Args: @@ -241,12 +236,14 @@ def download_target( TODO: download-related errors TODO: file write errors """ - if target_base_url is None and self._target_base_url is None: - raise ValueError( - "target_base_url must be set in either download_target() or " - "constructor" - ) + if target_base_url is None: + if self._target_base_url is None: + raise ValueError( + "target_base_url must be set in either " + "download_target() or constructor" + ) + target_base_url = self._target_base_url else: target_base_url = _ensure_trailing_slash(target_base_url) @@ -289,7 +286,7 @@ def _load_local_metadata(self, rolename: str) -> bytes: with open(os.path.join(self._dir, f"{rolename}.json"), "rb") as f: return f.read() - def _persist_metadata(self, rolename: str, data: bytes): + def _persist_metadata(self, rolename: str, data: bytes) -> None: with open(os.path.join(self._dir, f"{rolename}.json"), "wb") as f: f.write(data) @@ -450,6 +447,6 @@ def _preorder_depth_first_walk( return None -def _ensure_trailing_slash(url: str): +def _ensure_trailing_slash(url: str) -> str: """Return url guaranteed to end in a slash""" return url if url.endswith("/") else f"{url}/" From 018364d16a460ad97fa106513a01136f16d8a81c Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Wed, 28 Jul 2021 17:33:07 +0300 Subject: [PATCH 5/9] Use assert to check for None values This is done only for hinting 'mypy' that we have ensured these values cannot be None. 'Bandit' raises warnings for assert usage in the code but we are disabling them since we do not rely upon 'asserts' for any runtime checks but only for type checking. Signed-off-by: Teodora Sechkova --- tuf/ngclient/_internal/trusted_metadata_set.py | 2 ++ tuf/ngclient/updater.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/tuf/ngclient/_internal/trusted_metadata_set.py b/tuf/ngclient/_internal/trusted_metadata_set.py index 62832c51b0..e6c1be74f4 100644 --- a/tuf/ngclient/_internal/trusted_metadata_set.py +++ b/tuf/ngclient/_internal/trusted_metadata_set.py @@ -317,6 +317,8 @@ def update_snapshot(self, data: bytes) -> None: def _check_final_snapshot(self) -> None: """Check snapshot expiry and version before targets is updated""" + assert self.snapshot is not None # nosec + assert self.timestamp is not None # nosec if self.snapshot.signed.is_expired(self.reference_time): raise exceptions.ExpiredMetadataError("snapshot.json is expired") diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index d0e2db1f53..a3c7189d75 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -341,6 +341,7 @@ def _load_snapshot(self) -> None: # Local snapshot does not exist or is invalid: update from remote logger.debug("Failed to load local snapshot %s", e) + assert self._trusted_set.timestamp is not None # nosec metainfo = self._trusted_set.timestamp.signed.meta["snapshot.json"] length = metainfo.length or self.config.snapshot_max_length version = None @@ -361,6 +362,7 @@ def _load_targets(self, role: str, parent_role: str) -> None: # Local 'role' does not exist or is invalid: update from remote logger.debug("Failed to load local %s: %s", role, e) + assert self._trusted_set.snapshot is not None # nosec metainfo = self._trusted_set.snapshot.signed.meta[f"{role}.json"] length = metainfo.length or self.config.targets_max_length version = None From 8c9534b29ca7eceb8cfe491d0130716c9e751c96 Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Tue, 20 Jul 2021 12:55:59 +0300 Subject: [PATCH 6/9] Make TrustedMetadataSet.root non-optional The 'root' property is guaranteed to be set after init. Signed-off-by: Teodora Sechkova --- tuf/ngclient/_internal/trusted_metadata_set.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tuf/ngclient/_internal/trusted_metadata_set.py b/tuf/ngclient/_internal/trusted_metadata_set.py index e6c1be74f4..c33970aa57 100644 --- a/tuf/ngclient/_internal/trusted_metadata_set.py +++ b/tuf/ngclient/_internal/trusted_metadata_set.py @@ -114,9 +114,9 @@ def __iter__(self) -> Iterator[Metadata]: # Helper properties for top level metadata @property - def root(self) -> Optional[Metadata]: - """Current root Metadata or None""" - return self._trusted_set.get("root") + def root(self) -> Metadata: + """Current root Metadata""" + return self._trusted_set["root"] @property def timestamp(self) -> Optional[Metadata]: @@ -161,7 +161,7 @@ def update_root(self, data: bytes) -> None: f"Expected 'root', got '{new_root.signed.type}'" ) - if self.root is not None: + if self._trusted_set.get("root") is not None: # We are not loading initial trusted root: verify the new one self.root.verify_delegate("root", new_root) From cd096ba5c610dfb70b8b5e1ce1c228e93e1b8d7d Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Wed, 11 Aug 2021 18:36:27 +0300 Subject: [PATCH 7/9] Make SlowRetrievalError parameter optional SlowRetrievalError is raised from RequestsFetcher where average_download_speed is not calculated. Signed-off-by: Teodora Sechkova --- tuf/exceptions.py | 11 +++++++---- tuf/ngclient/_internal/requests_fetcher.py | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/tuf/exceptions.py b/tuf/exceptions.py index 2a24a0429e..8ebc92c7d1 100755 --- a/tuf/exceptions.py +++ b/tuf/exceptions.py @@ -24,7 +24,7 @@ from urllib import parse -from typing import Any, Dict +from typing import Any, Dict, Optional import logging logger = logging.getLogger(__name__) @@ -206,16 +206,19 @@ def __repr__(self) -> str: class SlowRetrievalError(DownloadError): """"Indicate that downloading a file took an unreasonably long time.""" - def __init__(self, average_download_speed: int): + def __init__(self, average_download_speed: Optional[int] = None): super(SlowRetrievalError, self).__init__() self.__average_download_speed = average_download_speed #bytes/second def __str__(self) -> str: - return ( - 'Download was too slow. Average speed: ' + + msg = 'Download was too slow.' + if self.__average_download_speed is not None: + msg = ('Download was too slow. Average speed: ' + repr(self.__average_download_speed) + ' bytes per second.') + return msg + def __repr__(self) -> str: return self.__class__.__name__ + ' : ' + str(self) diff --git a/tuf/ngclient/_internal/requests_fetcher.py b/tuf/ngclient/_internal/requests_fetcher.py index d2f24ef25d..ae68f1a3be 100644 --- a/tuf/ngclient/_internal/requests_fetcher.py +++ b/tuf/ngclient/_internal/requests_fetcher.py @@ -141,7 +141,7 @@ def _chunks( ) except urllib3.exceptions.ReadTimeoutError as e: - raise exceptions.SlowRetrievalError(str(e)) + raise exceptions.SlowRetrievalError from e finally: response.close() From 4f57ae43f8322a40be2b084268e9e72a435b108d Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Wed, 28 Jul 2021 17:14:54 +0300 Subject: [PATCH 8/9] Denote expected type of Metadata.signed By explicitly denoting the expected type of Metadata.signed we help mypy understand our intentions and correctly figure out types. This is entirely a typing feature and has no runtime effect. Modify the return type of Metadata.from_dict to match the other factory methods (from_*). Signed-off-by: Teodora Sechkova --- tuf/api/metadata.py | 2 +- tuf/ngclient/_internal/trusted_metadata_set.py | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tuf/api/metadata.py b/tuf/api/metadata.py index 6d1de28477..ba16a68a17 100644 --- a/tuf/api/metadata.py +++ b/tuf/api/metadata.py @@ -98,7 +98,7 @@ def __init__(self, signed: T, signatures: "OrderedDict[str, Signature]"): self.signatures = signatures @classmethod - def from_dict(cls, metadata: Dict[str, Any]) -> "Metadata": + def from_dict(cls, metadata: Dict[str, Any]) -> "Metadata[T]": """Creates Metadata object from its dict representation. Arguments: diff --git a/tuf/ngclient/_internal/trusted_metadata_set.py b/tuf/ngclient/_internal/trusted_metadata_set.py index c33970aa57..234429fba7 100644 --- a/tuf/ngclient/_internal/trusted_metadata_set.py +++ b/tuf/ngclient/_internal/trusted_metadata_set.py @@ -66,7 +66,7 @@ from typing import Dict, Iterator, Optional from tuf import exceptions -from tuf.api.metadata import Metadata +from tuf.api.metadata import Metadata, Root, Snapshot, Targets, Timestamp from tuf.api.serialization import DeserializationError logger = logging.getLogger(__name__) @@ -114,22 +114,22 @@ def __iter__(self) -> Iterator[Metadata]: # Helper properties for top level metadata @property - def root(self) -> Metadata: + def root(self) -> Metadata[Root]: """Current root Metadata""" return self._trusted_set["root"] @property - def timestamp(self) -> Optional[Metadata]: + def timestamp(self) -> Optional[Metadata[Timestamp]]: """Current timestamp Metadata or None""" return self._trusted_set.get("timestamp") @property - def snapshot(self) -> Optional[Metadata]: + def snapshot(self) -> Optional[Metadata[Snapshot]]: """Current snapshot Metadata or None""" return self._trusted_set.get("snapshot") @property - def targets(self) -> Optional[Metadata]: + def targets(self) -> Optional[Metadata[Targets]]: """Current targets Metadata or None""" return self._trusted_set.get("targets") @@ -152,7 +152,7 @@ def update_root(self, data: bytes) -> None: logger.debug("Updating root") try: - new_root = Metadata.from_bytes(data) + new_root = Metadata[Root].from_bytes(data) except DeserializationError as e: raise exceptions.RepositoryError("Failed to load root") from e @@ -199,7 +199,7 @@ def update_timestamp(self, data: bytes) -> None: # timestamp/snapshot can not yet be loaded at this point try: - new_timestamp = Metadata.from_bytes(data) + new_timestamp = Metadata[Timestamp].from_bytes(data) except DeserializationError as e: raise exceptions.RepositoryError("Failed to load timestamp") from e @@ -276,7 +276,7 @@ def update_snapshot(self, data: bytes) -> None: ) from e try: - new_snapshot = Metadata.from_bytes(data) + new_snapshot = Metadata[Snapshot].from_bytes(data) except DeserializationError as e: raise exceptions.RepositoryError("Failed to load snapshot") from e @@ -387,7 +387,7 @@ def update_delegated_targets( ) from e try: - new_delegate = Metadata.from_bytes(data) + new_delegate = Metadata[Targets].from_bytes(data) except DeserializationError as e: raise exceptions.RepositoryError("Failed to load snapshot") from e From 6ada96cf90845e27326f5fc6fa715cfad82d126b Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Thu, 26 Aug 2021 16:37:01 +0300 Subject: [PATCH 9/9] Load trusted root in a separate private method Add an additional private method for loading the initial trusted root metadata. The public method update_root() is now used only externally for updating the intiial root. The 'root' property is used only after its initialization in the constructor and is not longer optional which makes mypy happy. This split results in cleaner code and the ability to annotate the 'root' property as non-optional at the cost of some code duplication. Signed-off-by: Teodora Sechkova --- .../_internal/trusted_metadata_set.py | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/tuf/ngclient/_internal/trusted_metadata_set.py b/tuf/ngclient/_internal/trusted_metadata_set.py index 234429fba7..be1b0b44ed 100644 --- a/tuf/ngclient/_internal/trusted_metadata_set.py +++ b/tuf/ngclient/_internal/trusted_metadata_set.py @@ -98,7 +98,7 @@ def __init__(self, root_data: bytes): # Load and validate the local root metadata. Valid initial trusted root # metadata is required logger.debug("Updating initial trusted root") - self.update_root(root_data) + self._load_trusted_root(root_data) def __getitem__(self, role: str) -> Metadata: """Returns current Metadata for 'role'""" @@ -161,15 +161,15 @@ def update_root(self, data: bytes) -> None: f"Expected 'root', got '{new_root.signed.type}'" ) - if self._trusted_set.get("root") is not None: - # We are not loading initial trusted root: verify the new one - self.root.verify_delegate("root", new_root) + # Verify that new root is signed by trusted root + self.root.verify_delegate("root", new_root) - if new_root.signed.version != self.root.signed.version + 1: - raise exceptions.ReplayedMetadataError( - "root", new_root.signed.version, self.root.signed.version - ) + if new_root.signed.version != self.root.signed.version + 1: + raise exceptions.ReplayedMetadataError( + "root", new_root.signed.version, self.root.signed.version + ) + # Verify that new root is signed by itself new_root.verify_delegate("root", new_root) self._trusted_set["root"] = new_root @@ -409,3 +409,24 @@ def update_delegated_targets( self._trusted_set[role_name] = new_delegate logger.debug("Updated %s delegated by %s", role_name, delegator_name) + + def _load_trusted_root(self, data: bytes) -> None: + """Verifies and loads 'data' as trusted root metadata. + + Note that an expired initial root is considered valid: expiry is + only checked for the final root in update_timestamp(). + """ + try: + new_root = Metadata[Root].from_bytes(data) + except DeserializationError as e: + raise exceptions.RepositoryError("Failed to load root") from e + + if new_root.signed.type != "root": + raise exceptions.RepositoryError( + f"Expected 'root', got '{new_root.signed.type}'" + ) + + new_root.verify_delegate("root", new_root) + + self._trusted_set["root"] = new_root + logger.debug("Loaded trusted root")