From 4efd9496dc18dbfecfd8d3a322871028546b291e Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Mon, 31 Jan 2022 13:43:34 +0200 Subject: [PATCH 1/5] ngclient: Make DownloadErrors consistent Fetcher interface should only raise DownloadErrors, regardless of the implementation. * Make sure fetch() wraps non-DownloadError errors in a DownloadError * Make the abstract function private _fetch() * Try to be more consistent in doscstrings This now makes the example client more sensible (when server does not respond): $ ./client_example.py download qwerty ... Failed to download target qwerty: Failed to download url http://127.0.0.1:8000/metadata/2.root.json (here the latter part of the error string comes from DownloadError raised by FetcherInterface.fetch()) Signed-off-by: Jussi Kukkonen --- examples/client_example/client_example.py | 6 +-- tests/repository_simulator.py | 2 +- tuf/ngclient/_internal/requests_fetcher.py | 3 +- tuf/ngclient/fetcher.py | 50 +++++++++++++++++----- 4 files changed, 45 insertions(+), 16 deletions(-) diff --git a/examples/client_example/client_example.py b/examples/client_example/client_example.py index bed32b9dce..e747abef99 100755 --- a/examples/client_example/client_example.py +++ b/examples/client_example/client_example.py @@ -10,7 +10,7 @@ import shutil from pathlib import Path -from tuf.api.exceptions import RepositoryError +from tuf.api.exceptions import DownloadError, RepositoryError from tuf.ngclient import Updater # constants @@ -73,8 +73,8 @@ def download(target: str) -> bool: path = updater.download_target(info) print(f"Target downloaded and available in {path}") - except (OSError, RepositoryError) as e: - print(str(e)) + except (OSError, RepositoryError, DownloadError) as e: + print(f"Failed to download target {target}: {e}") return False return True diff --git a/tests/repository_simulator.py b/tests/repository_simulator.py index 5988101c51..ed6e51f647 100644 --- a/tests/repository_simulator.py +++ b/tests/repository_simulator.py @@ -205,7 +205,7 @@ def publish_root(self) -> None: self.signed_roots.append(self.md_root.to_bytes(JSONSerializer())) logger.debug("Published root v%d", self.root.version) - def fetch(self, url: str) -> Iterator[bytes]: + def _fetch(self, url: str) -> Iterator[bytes]: """Fetches data from the given url and returns an Iterator (or yields bytes). """ diff --git a/tuf/ngclient/_internal/requests_fetcher.py b/tuf/ngclient/_internal/requests_fetcher.py index 5b95e2bfb7..53c25999b8 100644 --- a/tuf/ngclient/_internal/requests_fetcher.py +++ b/tuf/ngclient/_internal/requests_fetcher.py @@ -51,7 +51,7 @@ def __init__(self) -> None: self.socket_timeout: int = 4 # seconds self.chunk_size: int = 400000 # bytes - def fetch(self, url: str) -> Iterator[bytes]: + def _fetch(self, url: str) -> Iterator[bytes]: """Fetches the contents of HTTP/HTTPS url from a remote server Arguments: @@ -61,7 +61,6 @@ def fetch(self, url: str) -> Iterator[bytes]: exceptions.SlowRetrievalError: A timeout occurs while receiving data. exceptions.FetcherHTTPError: An HTTP error code is received. - exceptions.DownloadError: When there is a problem parsing the url. Returns: A bytes iterator diff --git a/tuf/ngclient/fetcher.py b/tuf/ngclient/fetcher.py index a56c66dc78..0de9883380 100644 --- a/tuf/ngclient/fetcher.py +++ b/tuf/ngclient/fetcher.py @@ -28,38 +28,66 @@ class FetcherInterface: __metaclass__ = abc.ABCMeta @abc.abstractmethod - def fetch(self, url: str) -> Iterator[bytes]: + def _fetch(self, url: str) -> Iterator[bytes]: """Fetches the contents of HTTP/HTTPS url from a remote server. + Implementations must raise FetcherHTTPError if they receive an + HTTP error code. + + Implementations may raise any errors but the ones that are not + DownloadErrors will be wrapped in a DownloadError by fetch(). + Arguments: url: A URL string that represents a file location. Raises: - exceptions.SlowRetrievalError: A timeout occurs while receiving - data. - exceptions.FetcherHTTPError: An HTTP error code is received. + exceptions.FetcherHTTPError: An HTTP error code was received. Returns: A bytes iterator """ raise NotImplementedError # pragma: no cover + def fetch(self, url: str) -> Iterator[bytes]: + """Fetches the contents of HTTP/HTTPS url from a remote server. + + Arguments: + url: A URL string that represents a file location. + + Raises: + exceptions.DownloadError: An error occurred during download. + exceptions.FetcherHTTPError: An HTTP error code was received. + + Returns: + A bytes iterator + """ + # Ensure that fetch() only raises DownloadErrors, regardless of the + # fetcher implementation + try: + return self._fetch(url) + except Exception as e: + if isinstance(e, exceptions.DownloadError): + raise e + raise exceptions.DownloadError(f"Failed to download {url}") from e + @contextmanager def download_file(self, url: str, max_length: int) -> Iterator[IO]: """Opens a connection to 'url' and downloads the content up to 'max_length'. Args: - url: a URL string that represents the location of the file. - max_length: an integer value representing the length of - the file or an upper bound. + url: a URL string that represents the location of the file. + max_length: an integer value representing the length of + the file or an upper bound. Raises: - exceptions.DownloadLengthMismatchError: downloaded bytes exceed - 'max_length'. + exceptions.DownloadError: An error occurred during download. + exceptions.DownloadLengthMismatchError: downloaded bytes exceed + 'max_length'. + exceptions.FetcherHTTPError: An HTTP error code was received. Yields: - A TemporaryFile object that points to the contents of 'url'. + A TemporaryFile object that points to the contents of 'url'. """ logger.debug("Downloading: %s", url) @@ -96,8 +124,10 @@ def download_bytes(self, url: str, max_length: int) -> bytes: max_length: upper bound of data size in bytes. Raises: + exceptions.DownloadError: An error occurred during download. exceptions.DownloadLengthMismatchError: downloaded bytes exceed 'max_length'. + exceptions.FetcherHTTPError: An HTTP error code was received. Returns: The content of the file in bytes. From 17f2ddff02654216fea8d3a740274c74af6b01c2 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Mon, 31 Jan 2022 13:56:46 +0200 Subject: [PATCH 2/5] exceptions: rename FetcherHTTPError I've not supported many renames but I'm suggesting this one: FetcherHTTPError was created because we needed to signal 403/404 from the fetcher to updater. At that time the download error hierarchy in general was not thought out. Now we have a couple of different errors all derived from DownloadError. I believe it does not make sense to point out "Fetcher" in one of their names: DownloadHTTPError makes it clearer this is a specific type of DownloadError. Signed-off-by: Jussi Kukkonen --- tests/repository_simulator.py | 12 ++++++------ tests/test_fetcher_ng.py | 2 +- tuf/api/exceptions.py | 2 +- tuf/ngclient/_internal/requests_fetcher.py | 4 ++-- tuf/ngclient/fetcher.py | 10 +++++----- tuf/ngclient/updater.py | 2 +- 6 files changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/repository_simulator.py b/tests/repository_simulator.py index ed6e51f647..e50886d781 100644 --- a/tests/repository_simulator.py +++ b/tests/repository_simulator.py @@ -56,7 +56,7 @@ from securesystemslib.keys import generate_ed25519_key from securesystemslib.signer import SSlibSigner -from tuf.api.exceptions import FetcherHTTPError +from tuf.api.exceptions import DownloadHTTPError from tuf.api.metadata import ( SPECIFICATION_VERSION, TOP_LEVEL_ROLE_NAMES, @@ -238,7 +238,7 @@ def _fetch(self, url: str) -> Iterator[bytes]: yield self.fetch_target(target_path, prefix) else: - raise FetcherHTTPError(f"Unknown path '{path}'", 404) + raise DownloadHTTPError(f"Unknown path '{path}'", 404) def fetch_target( self, target_path: str, target_hash: Optional[str] @@ -251,12 +251,12 @@ def fetch_target( repo_target = self.target_files.get(target_path) if repo_target is None: - raise FetcherHTTPError(f"No target {target_path}", 404) + raise DownloadHTTPError(f"No target {target_path}", 404) if ( target_hash and target_hash not in repo_target.target_file.hashes.values() ): - raise FetcherHTTPError(f"hash mismatch for {target_path}", 404) + raise DownloadHTTPError(f"hash mismatch for {target_path}", 404) logger.debug("fetched target %s", target_path) return repo_target.data @@ -273,7 +273,7 @@ def fetch_metadata(self, role: str, version: Optional[int] = None) -> bytes: if role == Root.type: # return a version previously serialized in publish_root() if version is None or version > len(self.signed_roots): - raise FetcherHTTPError(f"Unknown root version {version}", 404) + raise DownloadHTTPError(f"Unknown root version {version}", 404) logger.debug("fetched root version %d", version) return self.signed_roots[version - 1] @@ -289,7 +289,7 @@ def fetch_metadata(self, role: str, version: Optional[int] = None) -> bytes: md = self.md_delegates.get(role) if md is None: - raise FetcherHTTPError(f"Unknown role {role}", 404) + raise DownloadHTTPError(f"Unknown role {role}", 404) md.signatures.clear() for signer in self.signers[role].values(): diff --git a/tests/test_fetcher_ng.py b/tests/test_fetcher_ng.py index 425dd2cc30..4c87ed2b00 100644 --- a/tests/test_fetcher_ng.py +++ b/tests/test_fetcher_ng.py @@ -101,7 +101,7 @@ def test_url_parsing(self) -> None: # File not found error def test_http_error(self) -> None: - with self.assertRaises(exceptions.FetcherHTTPError) as cm: + with self.assertRaises(exceptions.DownloadHTTPError) as cm: self.url = f"{self.url_prefix}/non-existing-path" self.fetcher.fetch(self.url) self.assertEqual(cm.exception.status_code, 404) diff --git a/tuf/api/exceptions.py b/tuf/api/exceptions.py index cb92694524..18fe43711d 100644 --- a/tuf/api/exceptions.py +++ b/tuf/api/exceptions.py @@ -52,7 +52,7 @@ class SlowRetrievalError(DownloadError): """Indicate that downloading a file took an unreasonably long time.""" -class FetcherHTTPError(DownloadError): +class DownloadHTTPError(DownloadError): """ Returned by FetcherInterface implementations for HTTP errors. diff --git a/tuf/ngclient/_internal/requests_fetcher.py b/tuf/ngclient/_internal/requests_fetcher.py index 53c25999b8..289c8e37ce 100644 --- a/tuf/ngclient/_internal/requests_fetcher.py +++ b/tuf/ngclient/_internal/requests_fetcher.py @@ -60,7 +60,7 @@ def _fetch(self, url: str) -> Iterator[bytes]: Raises: exceptions.SlowRetrievalError: A timeout occurs while receiving data. - exceptions.FetcherHTTPError: An HTTP error code is received. + exceptions.DownloadHTTPError: An HTTP error code is received. Returns: A bytes iterator @@ -88,7 +88,7 @@ def _fetch(self, url: str) -> Iterator[bytes]: except requests.HTTPError as e: response.close() status = e.response.status_code - raise exceptions.FetcherHTTPError(str(e), status) + raise exceptions.DownloadHTTPError(str(e), status) return self._chunks(response) diff --git a/tuf/ngclient/fetcher.py b/tuf/ngclient/fetcher.py index 0de9883380..6bcd5b8e4d 100644 --- a/tuf/ngclient/fetcher.py +++ b/tuf/ngclient/fetcher.py @@ -31,7 +31,7 @@ class FetcherInterface: def _fetch(self, url: str) -> Iterator[bytes]: """Fetches the contents of HTTP/HTTPS url from a remote server. - Implementations must raise FetcherHTTPError if they receive an + Implementations must raise DownloadHTTPError if they receive an HTTP error code. Implementations may raise any errors but the ones that are not @@ -41,7 +41,7 @@ def _fetch(self, url: str) -> Iterator[bytes]: url: A URL string that represents a file location. Raises: - exceptions.FetcherHTTPError: An HTTP error code was received. + exceptions.DownloadHTTPError: An HTTP error code was received. Returns: A bytes iterator @@ -56,7 +56,7 @@ def fetch(self, url: str) -> Iterator[bytes]: Raises: exceptions.DownloadError: An error occurred during download. - exceptions.FetcherHTTPError: An HTTP error code was received. + exceptions.DownloadHTTPError: An HTTP error code was received. Returns: A bytes iterator @@ -84,7 +84,7 @@ def download_file(self, url: str, max_length: int) -> Iterator[IO]: exceptions.DownloadError: An error occurred during download. exceptions.DownloadLengthMismatchError: downloaded bytes exceed 'max_length'. - exceptions.FetcherHTTPError: An HTTP error code was received. + exceptions.DownloadHTTPError: An HTTP error code was received. Yields: A TemporaryFile object that points to the contents of 'url'. @@ -127,7 +127,7 @@ def download_bytes(self, url: str, max_length: int) -> bytes: exceptions.DownloadError: An error occurred during download. exceptions.DownloadLengthMismatchError: downloaded bytes exceed 'max_length'. - exceptions.FetcherHTTPError: An HTTP error code was received. + exceptions.DownloadHTTPError: An HTTP error code was received. Returns: The content of the file in bytes. diff --git a/tuf/ngclient/updater.py b/tuf/ngclient/updater.py index f5cbd89b2c..743777a837 100644 --- a/tuf/ngclient/updater.py +++ b/tuf/ngclient/updater.py @@ -315,7 +315,7 @@ def _load_root(self) -> None: self._trusted_set.update_root(data) self._persist_metadata(Root.type, data) - except exceptions.FetcherHTTPError as exception: + except exceptions.DownloadHTTPError as exception: if exception.status_code not in {403, 404}: raise # 404/403 means current root is newest available From 6718620d607a35cb66d58802d8d183daa18a4814 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Mon, 31 Jan 2022 14:36:21 +0200 Subject: [PATCH 3/5] fetcher: docstring fix Make the dosctring match the similar argument in download_bytes() Signed-off-by: Jussi Kukkonen --- tuf/ngclient/fetcher.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tuf/ngclient/fetcher.py b/tuf/ngclient/fetcher.py index 6bcd5b8e4d..002572131b 100644 --- a/tuf/ngclient/fetcher.py +++ b/tuf/ngclient/fetcher.py @@ -77,8 +77,7 @@ def download_file(self, url: str, max_length: int) -> Iterator[IO]: Args: url: a URL string that represents the location of the file. - max_length: an integer value representing the length of - the file or an upper bound. + max_length: upper bound of file size in bytes. Raises: exceptions.DownloadError: An error occurred during download. From 6b079eefec38a5f7ca91a35989d9a80caa55843d Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Tue, 1 Feb 2022 16:48:58 +0200 Subject: [PATCH 4/5] ngclient: Add missing f to an f-string Signed-off-by: Jussi Kukkonen --- tuf/ngclient/_internal/requests_fetcher.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tuf/ngclient/_internal/requests_fetcher.py b/tuf/ngclient/_internal/requests_fetcher.py index 289c8e37ce..76081808c3 100644 --- a/tuf/ngclient/_internal/requests_fetcher.py +++ b/tuf/ngclient/_internal/requests_fetcher.py @@ -121,7 +121,7 @@ def _get_session(self, url: str) -> requests.Session: parsed_url = parse.urlparse(url) if not parsed_url.scheme or not parsed_url.hostname: - raise exceptions.DownloadError("Failed to parse URL {url}") + raise exceptions.DownloadError(f"Failed to parse URL {url}") session_index = f"{parsed_url.scheme}+{parsed_url.hostname}" session = self._sessions.get(session_index) From e6f363273f5d36a2fdcce18a5a6846bef8801f7a Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Thu, 3 Feb 2022 17:04:56 +0200 Subject: [PATCH 5/5] ngclient: Small refactor, avoid isinstance Signed-off-by: Jussi Kukkonen --- tuf/ngclient/fetcher.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tuf/ngclient/fetcher.py b/tuf/ngclient/fetcher.py index 002572131b..83900bf300 100644 --- a/tuf/ngclient/fetcher.py +++ b/tuf/ngclient/fetcher.py @@ -65,9 +65,9 @@ def fetch(self, url: str) -> Iterator[bytes]: # fetcher implementation try: return self._fetch(url) + except exceptions.DownloadError as e: + raise e except Exception as e: - if isinstance(e, exceptions.DownloadError): - raise e raise exceptions.DownloadError(f"Failed to download {url}") from e @contextmanager