diff --git a/CHANGES/+pull-through-signed-urls.bugfix b/CHANGES/+pull-through-signed-urls.bugfix new file mode 100644 index 00000000..804b9268 --- /dev/null +++ b/CHANGES/+pull-through-signed-urls.bugfix @@ -0,0 +1 @@ +Fixed pull-through downloads trusting the upstream url given by the client; pull-through links are now signed and downloads are checked against the upstream sha256. diff --git a/CHANGES/1428.bugfix b/CHANGES/1428.bugfix new file mode 100644 index 00000000..b5bf381f --- /dev/null +++ b/CHANGES/1428.bugfix @@ -0,0 +1 @@ +Fixed pip intermittently receiving a pull-through wheel instead of its metadata, or the reverse, when Redis caching is enabled. diff --git a/pulp_python/app/models.py b/pulp_python/app/models.py index 902bf5a8..fc57cd6e 100644 --- a/pulp_python/app/models.py +++ b/pulp_python/app/models.py @@ -34,12 +34,15 @@ from .provenance import Provenance from .utils import ( + PULL_THROUGH_PARAM, PYPI_LAST_SERIAL, PYPI_SERIAL_CONSTANT, artifact_to_metadata_artifact, artifact_to_python_content_data, canonicalize_name, + lookup_pull_through_entry, python_content_to_json, + unsign_pull_through_token, ) log = getLogger(__name__) @@ -357,30 +360,44 @@ class PythonRemote(Remote, AutoAddObjPermsMixin): vulnerabilities = models.BooleanField(default=False) def get_remote_artifact_url(self, relative_path=None, request=None): - """Get url for remote_artifact""" - if request and (url := request.query.get("redirect")): - # This is a special case for pull-through caching - # To handle PEP 658, it states that if the package has metadata available then it - # should be found at the download URL + ".metadata". Thus if the request url ends with - # ".metadata" then we need to add ".metadata" to the redirect url if not present. - if relative_path: - if relative_path.endswith(".metadata") and not url.endswith(".metadata"): - url += ".metadata" - # Handle special case for bug in pip (TODO file issue in pip) where it appends - # ".metadata" to the redirect url instead of the request url - if url.endswith(".metadata") and not relative_path.endswith(".metadata"): - setattr(self, "_real_relative_path", url.rsplit("/", 1)[1]) + """Get url for remote_artifact, content app requests need a stored or signed entry.""" + if request is None: + return super().get_remote_artifact_url(relative_path) + filename = PurePath(relative_path or "").name + if token := request.query.get(PULL_THROUGH_PARAM): + # pip adds ".metadata" to the end of the url (the token) instead of the url path + if token.endswith(".metadata"): + token = token.removesuffix(".metadata") + if not filename.endswith(".metadata"): + filename = f"{filename}.metadata" + setattr(self, "_real_relative_path", filename) + resolved = unsign_pull_through_token(self, token, filename) + else: + resolved = lookup_pull_through_entry(self, filename) + if resolved: + url, sha256 = resolved + if sha256: + self.__dict__.setdefault("_pull_through_digests", {})[url] = sha256 return url - return super().get_remote_artifact_url(relative_path, request=request) + return None def get_remote_artifact_content_type(self, relative_path=None): """Return PythonPackageContent, except for metadata artifacts.""" if hasattr(self, "_real_relative_path"): relative_path = getattr(self, "_real_relative_path") - if relative_path and relative_path.endswith(".whl.metadata"): + if relative_path and relative_path.endswith(".metadata"): return None return PythonPackageContent + def get_downloader(self, remote_artifact=None, url=None, download_factory=None, **kwargs): + """Validate pull-through downloads against the sha256 from their pull-through entry.""" + digests = self.__dict__.get("_pull_through_digests", {}) + if remote_artifact and not remote_artifact.sha256: + remote_artifact.sha256 = digests.get(remote_artifact.url) + return super().get_downloader( + remote_artifact=remote_artifact, url=url, download_factory=download_factory, **kwargs + ) + class Meta: default_related_name = "%(app_label)s_%(model_name)s" permissions = [ diff --git a/pulp_python/app/pypi/views.py b/pulp_python/app/pypi/views.py index 8d580a2f..782d8325 100644 --- a/pulp_python/app/pypi/views.py +++ b/pulp_python/app/pypi/views.py @@ -56,6 +56,8 @@ get_remote_package_filter, get_remote_simple_page, python_content_to_json, + sign_pull_through_path, + store_pull_through_entries, write_simple_detail, write_simple_detail_json, write_simple_index, @@ -368,11 +370,22 @@ def list(self, request, path): def pull_through_package_simple(self, package, path, remote): """Gets the package's simple page from remote.""" - def parse_package(release_package): + def upstream_url(release_package): parsed = urlparse(release_package.url) - stripped_url = urlunsplit(chain(parsed[:3], ("", ""))) - redirect_path = f"{path}/{release_package.filename}?redirect={stripped_url}" - d_url = urljoin(self.base_content_url, redirect_path) + return urlunsplit(chain(parsed[:3], ("", ""))) + + def parse_package(release_package, stored): + if stored: + pt_path = release_package.filename + else: + pt_path = sign_pull_through_path( + remote, + release_package.filename, + upstream_url(release_package), + sha256=release_package.digests.get("sha256"), + metadata_sha256=(release_package.metadata_digests or {}).get("sha256"), + ) + d_url = urljoin(self.base_content_url, f"{path}/{pt_path}") return { "filename": release_package.filename, "url": d_url, @@ -395,11 +408,19 @@ def parse_package(release_package): if not page: log.info(f"Failed to fetch {package} simple page from {remote.url}") return {} - return { - p.filename: parse_package(p) - for p in page.packages - if rfilter.filter_release(package, p.version) - } + packages = [p for p in page.packages if rfilter.filter_release(package, p.version)] + stored = store_pull_through_entries( + remote, + { + p.filename: { + "url": upstream_url(p), + "sha256": p.digests.get("sha256"), + "metadata_sha256": (p.metadata_digests or {}).get("sha256"), + } + for p in packages + }, + ) + return {p.filename: parse_package(p, stored) for p in packages} @extend_schema(operation_id="pypi_simple_package_read", summary="Get package simple page") @method_decorator(cache_control(max_age=600, public=True)) diff --git a/pulp_python/app/utils.py b/pulp_python/app/utils.py index 8838fa51..06c10592 100644 --- a/pulp_python/app/utils.py +++ b/pulp_python/app/utils.py @@ -12,6 +12,7 @@ import pkginfo from aiohttp.client_exceptions import ClientError from django.conf import settings +from django.core import signing from django.db.models import F, FilteredRelation, Q from django.db.utils import IntegrityError from jinja2 import Template @@ -20,6 +21,7 @@ from packaging.version import InvalidVersion, parse from pypi_simple import ACCEPT_JSON_PREFERRED, ProjectPage +from pulpcore.plugin.cache import SyncContentCache from pulpcore.plugin.exceptions import TimeoutException from pulpcore.plugin.models import Artifact, Remote, VulnerabilityReport from pulpcore.plugin.util import get_domain @@ -35,6 +37,12 @@ SUPPORTED_METADATA_VERSIONS = ("1.0", "1.1", "1.2", "2.0", "2.1", "2.2", "2.3", "2.4", "2.5") SIMPLE_API_VERSION = "1.1" + +PULL_THROUGH_PARAM = "pt" +PULL_THROUGH_SALT = "pulp_python.pull_through" +# Outlive both the cached simple pages and their max-age in client caches +PULL_THROUGH_TTL = settings.CACHE_SETTINGS["EXPIRES_TTL"] + 600 + PYPI_SIMPLE_V1_HTML = "application/vnd.pypi.simple.v1+html" PYPI_SIMPLE_V1_JSON = "application/vnd.pypi.simple.v1+json" @@ -768,3 +776,86 @@ async def aget_remote_simple_page(package, remote, max_retries=1): else: page = ProjectPage.from_html(package, open(d.path, "rb").read(), base_url=url) return page + + +def sign_pull_through_path(remote, filename, url, sha256=None, metadata_sha256=None): + """Returns the content app path of a pull-through package, carrying its signed upstream url.""" + data = {"r": str(remote.pk), "f": filename, "u": url} + if sha256: + data["s"] = sha256 + if metadata_sha256: + data["m"] = metadata_sha256 + token = signing.Signer(salt=PULL_THROUGH_SALT).sign_object(data, compress=True) + return f"{filename}?{PULL_THROUGH_PARAM}={token}" + + +def unsign_pull_through_token(remote, token, filename): + """ + Returns the upstream (url, sha256) for a file of a signed pull-through token. + + filename can be the signed package's filename or its PEP 658 metadata file. Returns None if + the token is invalid, was signed for another remote or doesn't cover filename. + """ + try: + data = signing.Signer(salt=PULL_THROUGH_SALT).unsign_object(token) + except signing.BadSignature: + return None + if data.get("r") != str(remote.pk): + return None + if filename == data["f"]: + return data["u"], data.get("s") + if filename == f"{data['f']}.metadata" and data.get("m"): + return f"{data['u']}.metadata", data["m"] + return None + + +def _pull_through_key(remote): + # Editing the remote invalidates its entries + return f"pulp_python:pull_through:{remote.pk}:{remote.pulp_last_updated.timestamp()}" + + +def store_pull_through_entries(remote, entries): + """ + Stores filename -> {url, sha256, metadata_sha256} entries of a remote in Redis. + + Returns True if the entries were stored, so plain filename links can be used for them. + """ + if not settings.CACHE_ENABLED: + return False + if not entries: + return True + key = _pull_through_key(remote) + try: + redis = SyncContentCache().redis + redis.hset(key, mapping={f: json.dumps(e) for f, e in entries.items()}) + redis.expire(key, PULL_THROUGH_TTL) + except Exception as e: + log.warning("Failed to store pull-through entries: %s", e) + return False + return True + + +def lookup_pull_through_entry(remote, filename): + """ + Returns the upstream (url, sha256) for a file stored by store_pull_through_entries. + + filename can be a stored package's filename or its PEP 658 metadata file. + """ + if not settings.CACHE_ENABLED: + return None + is_metadata = filename.endswith(".metadata") + try: + raw = SyncContentCache().redis.hget( + _pull_through_key(remote), filename.removesuffix(".metadata") + ) + except Exception as e: + log.warning("Failed to look up pull-through entry: %s", e) + return None + if not raw: + return None + entry = json.loads(raw) + if not is_metadata: + return entry["url"], entry.get("sha256") + if entry.get("metadata_sha256"): + return f"{entry['url']}.metadata", entry["metadata_sha256"] + return None diff --git a/pulp_python/tests/functional/api/test_full_mirror.py b/pulp_python/tests/functional/api/test_full_mirror.py index e89cdac2..c942038e 100644 --- a/pulp_python/tests/functional/api/test_full_mirror.py +++ b/pulp_python/tests/functional/api/test_full_mirror.py @@ -16,6 +16,12 @@ ) +@pytest.fixture +def signed_links(pulp_settings): + """Pull-through links carry a signed token only when Redis is not available.""" + return not pulp_settings.CACHE_ENABLED + + def test_pull_through_install( python_bindings, python_remote_factory, python_distribution_factory, delete_orphans_pre ): @@ -43,7 +49,9 @@ def test_pull_through_install( @pytest.mark.parallel -def test_pull_through_simple(python_remote_factory, python_distribution_factory, pulp_content_url): +def test_pull_through_simple( + python_remote_factory, python_distribution_factory, pulp_content_url, signed_links +): """Tests that the simple page is properly modified when requesting a pull-through.""" remote = python_remote_factory(url=PYPI_URL, includes=["shelf-reader"]) distro = python_distribution_factory(remote=remote.pulp_href) @@ -54,8 +62,11 @@ def test_pull_through_simple(python_remote_factory, python_distribution_factory, assert len(project_page.packages) == 2 for package in project_page.packages: assert package.filename in PYTHON_XS_FIXTURE_CHECKSUMS - relative_path = f"{distro.base_path}/{package.filename}?redirect=" - assert urljoin(pulp_content_url, relative_path) in package.url + expected = urljoin(pulp_content_url, f"{distro.base_path}/{package.filename}") + if signed_links: + assert package.url.startswith(f"{expected}?pt=") + else: + assert package.url == expected assert PYTHON_XS_FIXTURE_CHECKSUMS[package.filename] == package.digests["sha256"] @@ -124,6 +135,7 @@ def test_pull_through_with_repo( pulpcore_bindings, monitor_task, has_pulp_plugin, + signed_links, ): """Tests that content is saved to repository.""" remote = python_remote_factory(includes=[]) @@ -134,7 +146,7 @@ def test_pull_through_with_repo( url = urljoin(distro.base_url, "simple/aiohttp/") project_page = ProjectPage.from_response(requests.get(url), "aiohttp") for package in sample(project_page.packages, 3): - assert "?redirect=" in package.url + assert ("?pt=" in package.url) == signed_links r = requests.get(package.url) assert r.status_code == 200 @@ -161,7 +173,7 @@ def test_pull_through_with_repo( @pytest.mark.parallel def test_pull_through_local_only( - python_remote_factory, python_distribution_factory, python_repo_with_sync + python_remote_factory, python_distribution_factory, python_repo_with_sync, signed_links ): """Tests that pull-through checks the repository if the package is not present on the remote.""" remote = python_remote_factory(url=PYPI_URL, includes=["pulpcore"]) @@ -172,12 +184,13 @@ def test_pull_through_local_only( url = f"{distro.base_url}simple/pulpcore/" r = requests.get(url) assert r.status_code == 200 - assert "?redirect=" not in r.text + assert "?pt=" not in r.text url = f"{distro.base_url}simple/shelf-reader/" r = requests.get(url) assert r.status_code == 200 - assert "?redirect=" in r.text + assert "shelf-reader-0.1.tar.gz" in r.text + assert ("?pt=" in r.text) == signed_links url = f"{distro.base_url}simple/pulp_python/" r = requests.get(url) @@ -210,9 +223,8 @@ def test_pull_through_metadata(python_remote_factory, python_distribution_factor """ Tests that metadata is correctly served when using pull-through. - So when requesting the metadata url according to PEP 658 you should just need to add .metadata - to the end of the url path. Since pull-through includes a redirect query parameter we need to - test adding .metadata to the end of the url path vs adding it to the end of redirect query. + According to PEP 658 the metadata url is the package url with .metadata added to its path. + pip and uv build it differently (string vs path append), both have to work. """ remote = python_remote_factory(includes=["pytz"]) distro = python_distribution_factory(remote=remote.pulp_href) @@ -226,26 +238,101 @@ def test_pull_through_metadata(python_remote_factory, python_distribution_factor assert package1.has_metadata assert package2.has_metadata - # The correct way to get the metadata url: add to path (uv does this) + # Add to path (uv does this) parts1 = urlsplit(package1.url) url1 = urlunsplit((parts1[0], parts1[1], parts1[2] + ".metadata", parts1[3], parts1[4])) r = requests.get(url1) assert r.status_code == 200 assert sha256(r.content).hexdigest() == package1.metadata_digests["sha256"] - # The incorrect way to get the metadata url: add to end of string (pip does this) + # Add to end of string (pip does this) url2 = package2.url + ".metadata" r = requests.get(url2) assert r.status_code == 200 assert sha256(r.content).hexdigest() == package2.metadata_digests["sha256"] +@pytest.mark.parallel +def test_pull_through_rejects_unknown_files( + python_remote_factory, python_distribution_factory, signed_links +): + """Tests that pull-through only downloads files listed on a simple page of the distro's remote.""" + if signed_links: + pytest.skip("Pull-through links are signed when Redis is not available") + remote = python_remote_factory(includes=["shelf-reader"]) + distro = python_distribution_factory(remote=remote.pulp_href) + remote2 = python_remote_factory(includes=["shelf-reader"]) + distro2 = python_distribution_factory(remote=remote2.pulp_href) + + # Not listed on any simple page yet + r = requests.get(f"{distro.base_url}shelf-reader-0.1.tar.gz") + assert r.status_code == 404 + + url = f"{distro.base_url}simple/shelf-reader/" + package = ProjectPage.from_response(requests.get(url), "shelf-reader").packages[0] + + # Listed on a simple page of another remote + r = requests.get(package.url.replace(f"/{distro.base_path}/", f"/{distro2.base_path}/")) + assert r.status_code == 404 + + # Old unsigned redirect links + upstream_url = urljoin(remote.url, "simple/shelf-reader/") + upstream = ProjectPage.from_response(requests.get(upstream_url), "shelf-reader").packages[0] + r = requests.get(f"{distro.base_url}shelf_reader-9.9.tar.gz?redirect={upstream.url}") + assert r.status_code == 404 + + r = requests.get(package.url) + assert r.status_code == 200 + assert sha256(r.content).hexdigest() == package.digests["sha256"] + + +@pytest.mark.parallel +def test_pull_through_rejects_bad_links( + python_remote_factory, python_distribution_factory, signed_links +): + """Tests that pull-through only downloads files from links signed for the distro's remote.""" + if not signed_links: + pytest.skip("Pull-through links are only signed when Redis is not available") + remote = python_remote_factory(includes=["shelf-reader"]) + distro = python_distribution_factory(remote=remote.pulp_href) + remote2 = python_remote_factory(includes=["shelf-reader"]) + distro2 = python_distribution_factory(remote=remote2.pulp_href) + + url = f"{distro.base_url}simple/shelf-reader/" + package1, package2 = ProjectPage.from_response(requests.get(url), "shelf-reader").packages + token1 = package1.url.split("?pt=")[1] + + # Tampered token + tampered = token1[:-1] + ("A" if token1[-1] != "A" else "B") + r = requests.get(package1.url.replace(token1, tampered)) + assert r.status_code == 404 + + # Token used for another file + r = requests.get(package2.url.replace(package2.url.split("?pt=")[1], token1)) + assert r.status_code == 404 + + # Token used on a distribution with another remote + r = requests.get(package1.url.replace(f"/{distro.base_path}/", f"/{distro2.base_path}/")) + assert r.status_code == 404 + + # Old unsigned redirect links + upstream_url = urljoin(remote.url, "simple/shelf-reader/") + upstream = ProjectPage.from_response(requests.get(upstream_url), "shelf-reader").packages[0] + r = requests.get(f"{distro.base_url}{upstream.filename}?redirect={upstream.url}") + assert r.status_code == 404 + + r = requests.get(package1.url) + assert r.status_code == 200 + assert sha256(r.content).hexdigest() == package1.digests["sha256"] + + @pytest.mark.parallel def test_pull_through_metadata_with_repo( python_repo_factory, python_remote_factory, python_distribution_factory, pulpcore_bindings, + signed_links, ): """Tests that metadata is correctly saved when using pull-through with a repository.""" remote = python_remote_factory(url=PYPI_URL, includes=["pip"]) @@ -257,7 +344,7 @@ def test_pull_through_metadata_with_repo( filename = "pip-26.0.1-py3-none-any.whl" package = next(p for p in project_page.packages if p.filename == filename) assert package.has_metadata - assert "?redirect=" in package.url + assert ("?pt=" in package.url) == signed_links # Retrieve the metadata and assert the content was not saved to the repository parts = urlsplit(package.url) @@ -268,11 +355,12 @@ def test_pull_through_metadata_with_repo( project_page = ProjectPage.from_response(requests.get(pip_url), "pip") package = next(p for p in project_page.packages if p.filename == filename) assert package.has_metadata - assert "?redirect=" in package.url + assert ("?pt=" in package.url) == signed_links # Now retrieve the package and assert the content was saved with metadata r = requests.get(package.url) assert r.status_code == 200 + assert sha256(r.content).hexdigest() == package.digests["sha256"] pa = pulpcore_bindings.ArtifactsApi.list(sha256=package.digests["sha256"]) assert pa.count == 1 ma = pulpcore_bindings.ArtifactsApi.list(sha256=package.metadata_digests["sha256"]) @@ -281,7 +369,7 @@ def test_pull_through_metadata_with_repo( # Check the simple page is updated to point to the local repository project_page = ProjectPage.from_response(requests.get(pip_url), "pip") package = next(p for p in project_page.packages if p.filename == filename) - assert "?redirect=" not in package.url + assert "?pt=" not in package.url r = requests.get(package.metadata_url) assert r.status_code == 200 assert sha256(r.content).hexdigest() == package.metadata_digests["sha256"] diff --git a/pulp_python/tests/unit/test_pull_through.py b/pulp_python/tests/unit/test_pull_through.py new file mode 100644 index 00000000..9520d45c --- /dev/null +++ b/pulp_python/tests/unit/test_pull_through.py @@ -0,0 +1,87 @@ +from datetime import datetime, timedelta +from types import SimpleNamespace +from uuid import uuid4 + +import pytest +from django.conf import settings + +from pulp_python.app.utils import ( + PULL_THROUGH_PARAM, + lookup_pull_through_entry, + sign_pull_through_path, + store_pull_through_entries, + unsign_pull_through_token, +) + +FILENAME = "shelf_reader-0.1-py2-none-any.whl" +URL = f"https://files.example.com/packages/ab/cd/{FILENAME}" + + +def _token(remote, **kwargs): + path = sign_pull_through_path(remote, FILENAME, URL, **kwargs) + filename, token = path.split(f"?{PULL_THROUGH_PARAM}=") + assert filename == FILENAME + return token + + +def test_round_trip(): + remote = SimpleNamespace(pk=uuid4()) + token = _token(remote, sha256="a" * 64, metadata_sha256="b" * 64) + + assert unsign_pull_through_token(remote, token, FILENAME) == (URL, "a" * 64) + assert unsign_pull_through_token(remote, token, f"{FILENAME}.metadata") == ( + f"{URL}.metadata", + "b" * 64, + ) + + +def test_metadata_requires_signed_digest(): + remote = SimpleNamespace(pk=uuid4()) + token = _token(remote, sha256="a" * 64) + + assert unsign_pull_through_token(remote, token, f"{FILENAME}.metadata") is None + + +def test_rejects_other_remote(): + token = _token(SimpleNamespace(pk=uuid4()), sha256="a" * 64) + + assert unsign_pull_through_token(SimpleNamespace(pk=uuid4()), token, FILENAME) is None + + +def test_rejects_other_filename(): + remote = SimpleNamespace(pk=uuid4()) + token = _token(remote, sha256="a" * 64) + + assert unsign_pull_through_token(remote, token, "evil-1.0-py3-none-any.whl") is None + + +def test_rejects_tampered_token(): + remote = SimpleNamespace(pk=uuid4()) + token = _token(remote, sha256="a" * 64) + tampered = token[:-1] + ("A" if token[-1] != "A" else "B") + + assert unsign_pull_through_token(remote, tampered, FILENAME) is None + assert unsign_pull_through_token(remote, "not-a-token", FILENAME) is None + + +@pytest.mark.skipif(not settings.CACHE_ENABLED, reason="CACHE_ENABLED is not set") +def test_stored_entries(): + remote = SimpleNamespace(pk=uuid4(), pulp_last_updated=datetime.now()) + entry = {"url": URL, "sha256": "a" * 64, "metadata_sha256": None} + assert store_pull_through_entries(remote, {FILENAME: entry}) + + assert lookup_pull_through_entry(remote, FILENAME) == (URL, "a" * 64) + assert lookup_pull_through_entry(remote, f"{FILENAME}.metadata") is None + assert lookup_pull_through_entry(remote, "evil-1.0-py3-none-any.whl") is None + + entry["metadata_sha256"] = "b" * 64 + assert store_pull_through_entries(remote, {FILENAME: entry}) + assert lookup_pull_through_entry(remote, f"{FILENAME}.metadata") == ( + f"{URL}.metadata", + "b" * 64, + ) + + other = SimpleNamespace(pk=uuid4(), pulp_last_updated=remote.pulp_last_updated) + assert lookup_pull_through_entry(other, FILENAME) is None + edited = SimpleNamespace(pk=remote.pk, pulp_last_updated=datetime.now() + timedelta(1)) + assert lookup_pull_through_entry(edited, FILENAME) is None