diff --git a/CHANGES/28.feature b/CHANGES/28.feature new file mode 100644 index 0000000..b9b7847 --- /dev/null +++ b/CHANGES/28.feature @@ -0,0 +1 @@ +A repository can no longer be used for both pull-through caching and uploads at the same time. diff --git a/docs/user/guides/private-registry.md b/docs/user/guides/private-registry.md index 8df92d2..2e488a9 100644 --- a/docs/user/guides/private-registry.md +++ b/docs/user/guides/private-registry.md @@ -10,6 +10,11 @@ Rust packages. pulp rust repository create --name my-crates ``` +!!! note + A repository may be used for pull-through caching **or** for uploads, but not both. Pulp + rejects attempts to attach an upload distribution to a repository that is already used for + caching (and vice versa). Use separate repositories for caching and publishing. + ## Create a Distribution A distribution makes the repository's content available to Cargo over HTTP. Set `--allow-uploads` @@ -166,10 +171,10 @@ publishing `my-crate` when `my_crate` already exists in the same repository is r duplicate. Yank and unyank operations use the same matching. !!! tip "Separate Registries" - Keep private registries and public pull-through caches as separate distributions (and - preferably separate repositories). This makes it easy to audit which registries have - upstream access and reduces the risk of accidental misconfiguration. For additional - isolation or access control, they could be kept on entirely separate domains. + Pulp enforces that private registries and pull-through caches use separate repositories. + A repository that is a pull-through cache (targeted by a distribution with a `remote`, or + with its own `remote` set) cannot also accept uploads, and vice versa. This prevents + dependency-confusion attacks where uploaded content could override cached upstream content. ## Further Reading diff --git a/pulp_rust/app/serializers.py b/pulp_rust/app/serializers.py index f734f07..510b1ca 100755 --- a/pulp_rust/app/serializers.py +++ b/pulp_rust/app/serializers.py @@ -261,6 +261,9 @@ def validate(self, data): allow_uploads = data.get( "allow_uploads", self.instance.allow_uploads if self.instance else False ) + repository = data.get("repository", self.instance.repository if self.instance else None) + + # A single distribution cannot both cache from a remote and accept uploads. if remote and allow_uploads: raise serializers.ValidationError( _( @@ -268,6 +271,40 @@ def validate(self, data): "Use separate distributions for pull-through caching and publishing." ) ) + + # A single repository can't be used for both pull-through caching and uploads. + if repository: + repo = repository.cast() + sibling_distributions = models.RustDistribution.objects.filter(repository=repo) + if self.instance: + sibling_distributions = sibling_distributions.exclude(pk=self.instance.pk) + + if allow_uploads: + if repo.remote_id: + raise serializers.ValidationError( + _( + "This repository has a remote set for caching and cannot also be " + "used for uploads. Use separate repositories for pull-through " + "caching and publishing." + ) + ) + if sibling_distributions.exclude(remote=None).exists(): + raise serializers.ValidationError( + _( + "This repository is already used for pull-through caching by " + "another distribution and cannot also be used for uploads. Use " + "separate repositories for pull-through caching and publishing." + ) + ) + + if remote and sibling_distributions.filter(allow_uploads=True).exists(): + raise serializers.ValidationError( + _( + "This repository is already used for uploads by another distribution " + "and cannot also be used for pull-through caching. Use separate " + "repositories for pull-through caching and publishing." + ) + ) return data class Meta: diff --git a/pulp_rust/app/views.py b/pulp_rust/app/views.py index 0f81e0d..10ead26 100644 --- a/pulp_rust/app/views.py +++ b/pulp_rust/app/views.py @@ -54,6 +54,32 @@ BASE_CONTENT_URL = urljoin(settings.CONTENT_ORIGIN, settings.CONTENT_PATH_PREFIX) +def cargo_error(detail, status=400): + """Build a Cargo-style JSON error response.""" + return HttpResponse( + json.dumps({"errors": [{"detail": detail}]}), + content_type="application/json", + status=status, + ) + + +def repository_write_error(distro): + """Return a Cargo error response if the distribution can't be written to, else None. + + Write operations (publish, yank, unyank) require the distribution to point at a + repository. A distribution serving a fixed repository_version is read-only, and one + with neither has nothing to write to. + """ + if distro.repository: + return None + if distro.repository_version: + return cargo_error( + "This distribution serves a fixed repository version and cannot be modified.", + status=400, + ) + return cargo_error("No repository associated with this distribution", status=404) + + class PlainTextRenderer(BaseRenderer): """Renderer for text/plain responses (Cargo sends Accept: text/plain).""" @@ -320,14 +346,6 @@ def get_distribution(self): RustDistribution, base_path=self.kwargs["repo"], pulp_domain=get_domain() ) - @staticmethod - def _error_response(detail, status=400): - return HttpResponse( - json.dumps({"errors": [{"detail": detail}]}), - content_type="application/json", - status=status, - ) - def put(self, request, **kwargs): """ Handle ``cargo publish`` requests. @@ -339,33 +357,31 @@ def put(self, request, **kwargs): distro = self.get_distribution() if not request.user.has_perm("rust.publish_rustdistribution", distro): - return self._error_response("insufficient permissions", status=403) + return cargo_error("insufficient permissions", status=403) if not distro.allow_uploads: - return self._error_response("this registry does not allow uploads", status=403) + return cargo_error("this registry does not allow uploads", status=403) - if not distro.repository: - return self._error_response( - "no repository associated with this distribution", status=404 - ) + if error := repository_write_error(distro): + return error try: metadata, crate_bytes = parse_cargo_publish_body(request.body) except (struct.error, json.JSONDecodeError, UnicodeDecodeError): - return self._error_response("invalid publish request body") + return cargo_error("invalid publish request body") name = metadata.get("name") vers = metadata.get("vers") if not name or not vers: - return self._error_response("missing required fields: name, vers") + return cargo_error("missing required fields: name, vers") error = validate_crate_name(name) if error: - return self._error_response(error) + return cargo_error(error) error = validate_crate_version(vers) if error: - return self._error_response(error) + return cargo_error(error) # Check for duplicates using canonical name form to prevent confusable # packages (e.g. "my-crate" vs "my_crate" or "MyCrate" vs "mycrate"). @@ -377,7 +393,7 @@ def put(self, request, **kwargs): if RustContent.objects.filter( pk__in=repo_version.content, canonical_name=canonical, vers=vers_base ).exists(): - return self._error_response(f"crate version `{name}@{vers}` is already uploaded") + return cargo_error(f"crate version `{name}@{vers}` is already uploaded") # Write the .crate bytes to a temp file — raw bytes can't be passed # through dispatch() because task kwargs are stored as JSON. @@ -427,14 +443,6 @@ def get_permissions(self): return [IsAuthenticated()] return [] - @staticmethod - def _error_response(detail, status=400): - return HttpResponse( - json.dumps({"errors": [{"detail": detail}]}), - content_type="application/json", - status=status, - ) - def get_full_path(self, base_path, pulp_domain=None): if settings.DOMAIN_ENABLED: domain = pulp_domain or get_domain() @@ -481,9 +489,9 @@ def delete(self, request, name, version, rest, **kwargs): distro = self.get_distribution() if not request.user.has_perm("rust.yank_rustdistribution", distro): - return self._error_response("insufficient permissions", status=403) - if not distro.repository: - raise Http404("No repository associated with this distribution") + return cargo_error("insufficient permissions", status=403) + if error := repository_write_error(distro): + return error canonical = canonicalize_crate_name(name) repo_version = distro.repository.latest_version() @@ -523,9 +531,9 @@ def put(self, request, name, version, rest, **kwargs): distro = self.get_distribution() if not request.user.has_perm("rust.yank_rustdistribution", distro): - return self._error_response("insufficient permissions", status=403) - if not distro.repository: - raise Http404("No repository associated with this distribution") + return cargo_error("insufficient permissions", status=403) + if error := repository_write_error(distro): + return error task = dispatch( aunyank_package, diff --git a/pulp_rust/tests/functional/api/test_mixed_content.py b/pulp_rust/tests/functional/api/test_mixed_content.py new file mode 100644 index 0000000..83633ca --- /dev/null +++ b/pulp_rust/tests/functional/api/test_mixed_content.py @@ -0,0 +1,108 @@ +"""Tests preventing a repository from mixing pull-through cached and uploaded content. + +A repository must be used either for pull-through caching or for uploads, never both. +""" + +import pytest + +from pulpcore.client.pulp_rust.exceptions import ApiException + +from pulp_rust.tests.functional.utils import CRATES_IO_URL + + +def test_single_distribution_remote_and_uploads_rejected( + rust_repo_factory, + rust_remote_factory, + rust_distribution_factory, +): + """A single distribution cannot have both a remote and allow_uploads.""" + repo = rust_repo_factory() + remote = rust_remote_factory(url=CRATES_IO_URL) + + with pytest.raises(ApiException) as exc: + rust_distribution_factory( + repository=repo.pulp_href, remote=remote.pulp_href, allow_uploads=True + ) + assert exc.value.status == 400 + + +def test_upload_distribution_on_pull_through_repo_rejected( + rust_repo_factory, + rust_remote_factory, + rust_distribution_factory, +): + """A repo already used for pull-through caching cannot get an upload distribution.""" + repo = rust_repo_factory() + remote = rust_remote_factory(url=CRATES_IO_URL) + + # First distribution caches into the repo via a remote. + rust_distribution_factory(repository=repo.pulp_href, remote=remote.pulp_href) + + # A second distribution accepting uploads into the same repo must be rejected. + with pytest.raises(ApiException) as exc: + rust_distribution_factory(repository=repo.pulp_href, allow_uploads=True) + assert exc.value.status == 400 + + +def test_pull_through_distribution_on_upload_repo_rejected( + rust_repo_factory, + rust_remote_factory, + rust_distribution_factory, +): + """A repo already used for uploads cannot get a pull-through distribution.""" + repo = rust_repo_factory() + remote = rust_remote_factory(url=CRATES_IO_URL) + + # First distribution accepts uploads into the repo. + rust_distribution_factory(repository=repo.pulp_href, allow_uploads=True) + + # A second distribution caching into the same repo must be rejected. + with pytest.raises(ApiException) as exc: + rust_distribution_factory(repository=repo.pulp_href, remote=remote.pulp_href) + assert exc.value.status == 400 + + +def test_upload_distribution_on_repo_with_remote_rejected( + rust_repo_factory, + rust_remote_factory, + rust_distribution_factory, +): + """A repo with its own remote (for syncing) cannot get an upload distribution.""" + remote = rust_remote_factory(url=CRATES_IO_URL) + repo = rust_repo_factory(remote=remote.pulp_href) + + with pytest.raises(ApiException) as exc: + rust_distribution_factory(repository=repo.pulp_href, allow_uploads=True) + assert exc.value.status == 400 + + +def test_separate_repos_for_cache_and_uploads_succeed( + rust_repo_factory, + rust_remote_factory, + rust_distribution_factory, +): + """Using separate repositories for caching and uploads is allowed.""" + remote = rust_remote_factory(url=CRATES_IO_URL) + + cache_repo = rust_repo_factory() + cache_distro = rust_distribution_factory( + repository=cache_repo.pulp_href, remote=remote.pulp_href + ) + assert cache_distro is not None + + upload_repo = rust_repo_factory() + upload_distro = rust_distribution_factory(repository=upload_repo.pulp_href, allow_uploads=True) + assert upload_distro is not None + + +def test_multiple_upload_distributions_on_one_repo_succeed( + rust_repo_factory, + rust_distribution_factory, +): + """Multiple upload distributions may share one repo (no caching involved).""" + repo = rust_repo_factory() + + first = rust_distribution_factory(repository=repo.pulp_href, allow_uploads=True) + second = rust_distribution_factory(repository=repo.pulp_href, allow_uploads=True) + assert first is not None + assert second is not None diff --git a/pulp_rust/tests/functional/api/test_rbac.py b/pulp_rust/tests/functional/api/test_rbac.py index c64d1ae..36845a7 100644 --- a/pulp_rust/tests/functional/api/test_rbac.py +++ b/pulp_rust/tests/functional/api/test_rbac.py @@ -21,9 +21,27 @@ def test_basic_crud(self, gen_users, rust_repo_api_client, try_action): assert (b_list.count, c_list.count) == (0, 0) # Create — only creator (bob) can create - try_action(alice, rust_repo_api_client, "create", 403, {"name": str(uuid.uuid4())}) - repo = try_action(bob, rust_repo_api_client, "create", 201, {"name": str(uuid.uuid4())}) - try_action(charlie, rust_repo_api_client, "create", 403, {"name": str(uuid.uuid4())}) + try_action( + alice, + rust_repo_api_client, + "create", + 403, + {"name": str(uuid.uuid4())}, + ) + repo = try_action( + bob, + rust_repo_api_client, + "create", + 201, + {"name": str(uuid.uuid4())}, + ) + try_action( + charlie, + rust_repo_api_client, + "create", + 403, + {"name": str(uuid.uuid4())}, + ) # Read — alice has model-level viewer, bob is owner (creation hook), charlie has nothing try_action(alice, rust_repo_api_client, "read", 200, repo.pulp_href)