Stop trusting the client's redirect url for pull-through downloads

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.

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.

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 = [

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))

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