From 5cec2b5e9933905df370bd79f53e0b6e683e6c7f Mon Sep 17 00:00:00 2001 From: Lubos Mjachky Date: Fri, 5 Jul 2024 00:26:51 +0200 Subject: [PATCH] Enforce permission checks while pulling from pull-through distributions closes #1624 --- CHANGES/1624.bugfix | 2 + pulp_container/app/access_policy.py | 4 + pulp_container/app/authorization.py | 104 ++++++++++++------ .../app/global_access_conditions.py | 7 +- .../0041_add_pull_through_pull_permissions.py | 21 ++++ pulp_container/app/models.py | 8 ++ pulp_container/app/registry_api.py | 28 +++-- pulp_container/app/viewsets.py | 25 ++++- .../functional/api/test_pull_through_cache.py | 77 ++++++++++--- pulp_container/tests/functional/constants.py | 3 + staging_docs/admin/learn/rbac.md | 47 +++++++- 11 files changed, 263 insertions(+), 63 deletions(-) create mode 100644 CHANGES/1624.bugfix create mode 100644 pulp_container/app/migrations/0041_add_pull_through_pull_permissions.py diff --git a/CHANGES/1624.bugfix b/CHANGES/1624.bugfix new file mode 100644 index 000000000..e29899403 --- /dev/null +++ b/CHANGES/1624.bugfix @@ -0,0 +1,2 @@ +Permitted users with the `pull_new_distribution` permission to pull data via pull-through +distributions. diff --git a/pulp_container/app/access_policy.py b/pulp_container/app/access_policy.py index aa814bffd..84ac524b3 100644 --- a/pulp_container/app/access_policy.py +++ b/pulp_container/app/access_policy.py @@ -31,6 +31,10 @@ def get_policy_statements(self, request, view): access_policy_obj = AccessPolicyModel.objects.get( viewset_name="distributions/container/container" ) + elif isinstance(view.get_object(), models.ContainerPullThroughDistribution): + access_policy_obj = AccessPolicyModel.objects.get( + viewset_name="distributions/container/pull-through" + ) else: access_policy_obj = AccessPolicyModel.objects.get( viewset_name="pulp_container/namespaces" diff --git a/pulp_container/app/authorization.py b/pulp_container/app/authorization.py index e3791dba7..a945fb2c0 100644 --- a/pulp_container/app/authorization.py +++ b/pulp_container/app/authorization.py @@ -11,12 +11,17 @@ from django.conf import settings from django.http import HttpRequest +from django.db.models import F, Value from rest_framework.request import Request from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import serialization -from pulp_container.app.models import ContainerDistribution, ContainerNamespace +from pulp_container.app.models import ( + ContainerDistribution, + ContainerNamespace, + ContainerPullThroughDistribution, +) from pulp_container.app.access_policy import RegistryAccessPolicy TOKEN_EXPIRATION_TIME = settings.get("TOKEN_EXPIRATION_TIME", 300) @@ -62,14 +67,15 @@ def __init__(self, user, service, scopes): self.user = user self.service = service self.scopes = scopes - self.access_policy = RegistryAccessPolicy() + + permission_checker = PermissionChecker(user) self.actions_permissions = defaultdict( lambda: lambda *args: False, { - "pull": self.has_pull_permissions, - "push": self.has_push_permissions, - "*": self.has_view_catalog_permissions, + "pull": permission_checker.has_pull_permissions, + "push": permission_checker.has_push_permissions, + "*": permission_checker.has_view_catalog_permissions, }, ) @@ -184,6 +190,39 @@ def permit_scope(self, scope): return [{"type": typ, "name": name, "actions": list(permitted_actions)}] + @staticmethod + def generate_claim_set(issuer, issued_at, subject, audience, access): + """ + Generate the claim set that will be signed and dispatched back to the requesting subject. + """ + token_id = str(uuid.UUID(int=random.getrandbits(128), version=4)) + expiration = issued_at + TOKEN_EXPIRATION_TIME + return { + "access": access, + "aud": audience, + "exp": expiration, + "iat": issued_at, + "iss": issuer, + "jti": token_id, + "nbf": issued_at, + "sub": subject, + } + + +def get_pull_through_distribution(path): + return ( + ContainerPullThroughDistribution.objects.annotate(path=Value(path)) + .filter(path__startswith=F("base_path")) + .order_by("-base_path") + .first() + ) + + +class PermissionChecker: + def __init__(self, user): + self.user = user + self.access_policy = RegistryAccessPolicy() + def has_permission(self, obj, method, action, data): """Check if user has permission to perform action.""" @@ -207,13 +246,24 @@ def has_pull_permissions(self, path): try: namespace = ContainerNamespace.objects.get(name=namespace_name) except ContainerNamespace.DoesNotExist: - # Check if user is allowed to create a new namespace + # Check if the user is allowed to create a new namespace return self.has_permission(None, "POST", "create", {"name": namespace_name}) - # Check if user is allowed to view distributions in the namespace - return self.has_permission( - namespace, "GET", "view_distribution", {"name": namespace_name} - ) + if pt_distribution := get_pull_through_distribution(path): + # Check if the user is allowed to create a new distribution + return self.has_modify_pull_through_cache_permissions(pt_distribution) + else: + # Check if the user is allowed to view distributions in the namespace + return self.has_permission( + namespace, "GET", "view_distribution", {"name": namespace_name} + ) + + if pt_distribution := get_pull_through_distribution(path): + # Check if the user is allowed to pull new content via a pull-through distribution + if self.has_modify_pull_through_cache_permissions(pt_distribution, distribution): + return True + + # Check if the user has general pull permissions return self.has_permission(distribution, "GET", "pull", {"base_path": path}) def has_push_permissions(self, path): @@ -241,28 +291,14 @@ def has_view_catalog_permissions(self, path): if path != "catalog": return False - # Fake the request - request = Request(HttpRequest()) - request.method = "GET" - request.user = self.user - # Fake the view - view = FakeViewWithSerializer("catalog", lambda: ContainerDistribution()) - return self.access_policy.has_permission(request, view) + return self.has_permission(ContainerDistribution(), "GET", "catalog", {}) - @staticmethod - def generate_claim_set(issuer, issued_at, subject, audience, access): - """ - Generate the claim set that will be signed and dispatched back to the requesting subject. - """ - token_id = str(uuid.UUID(int=random.getrandbits(128), version=4)) - expiration = issued_at + TOKEN_EXPIRATION_TIME - return { - "access": access, - "aud": audience, - "exp": expiration, - "iat": issued_at, - "iss": issuer, - "jti": token_id, - "nbf": issued_at, - "sub": subject, - } + def has_modify_pull_through_cache_permissions(self, pt_distribution, distribution=None): + has_distro_perms = False + if distribution: + has_distro_perms = self.has_permission(distribution, "GET", "pull_new_content", {}) + return ( + has_distro_perms + or self.has_permission(pt_distribution, "GET", "pull_new_distribution", {}) + or self.has_permission(pt_distribution.namespace, "POST", "create_distribution", {}) + ) diff --git a/pulp_container/app/global_access_conditions.py b/pulp_container/app/global_access_conditions.py index abca8c772..48ef3120b 100644 --- a/pulp_container/app/global_access_conditions.py +++ b/pulp_container/app/global_access_conditions.py @@ -27,10 +27,9 @@ def has_namespace_obj_perms(request, view, action, permission): for dist in obj.distributions.all(): if request.user.has_perm(permission, dist.cast().namespace): return True - elif type(obj) is models.ContainerPushRepositoryVersion: - for dist in obj.repository.distributions.all(): - if request.user.has_perm(permission, dist.cast().namespace): - return True + elif type(obj) is models.ContainerPullThroughDistribution: + namespace = obj.namespace + return request.user.has_perm(permission, namespace) return False diff --git a/pulp_container/app/migrations/0041_add_pull_through_pull_permissions.py b/pulp_container/app/migrations/0041_add_pull_through_pull_permissions.py new file mode 100644 index 000000000..a3232eef7 --- /dev/null +++ b/pulp_container/app/migrations/0041_add_pull_through_pull_permissions.py @@ -0,0 +1,21 @@ +# Generated by Django 4.2.11 on 2024-07-07 21:38 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('container', '0040_add_remote_repo_filter'), + ] + + operations = [ + migrations.AlterModelOptions( + name='containernamespace', + options={'permissions': [('namespace_add_containerdistribution', 'Add any distribution to a namespace'), ('namespace_delete_containerdistribution', 'Delete any distribution from a namespace'), ('namespace_view_containerdistribution', 'View any distribution in a namespace'), ('namespace_pull_containerdistribution', 'Pull from any distribution in a namespace'), ('namespace_push_containerdistribution', 'Push to any distribution in a namespace'), ('namespace_change_containerdistribution', 'Change any distribution in a namespace'), ('namespace_view_containerpushrepository', 'View any push repository in a namespace'), ('namespace_modify_content_containerpushrepository', 'Modify content in any push repository in a namespace'), ('namespace_modify_content_containerrepository', 'Modify content in any repository in a namespace'), ('namespace_change_containerpushrepository', 'Update any existing push repository in a namespace'), ('manage_roles_containernamespace', 'Can manage role assignments on container namespace')]}, + ), + migrations.AlterModelOptions( + name='containerpullthroughdistribution', + options={'default_related_name': '%(app_label)s_%(model_name)s', 'permissions': [('manage_roles_containerpullthroughdistribution', 'Can manage role assignments on pull-through cache distribution'), ('pull_new_containerdistribution', 'Can pull new content via the pull-through cache distribution')]}, + ), + ] diff --git a/pulp_container/app/models.py b/pulp_container/app/models.py index 76efabb15..b96701c92 100644 --- a/pulp_container/app/models.py +++ b/pulp_container/app/models.py @@ -332,6 +332,10 @@ class Meta: "namespace_modify_content_containerpushrepository", "Modify content in any push repository in a namespace", ), + ( + "namespace_modify_content_containerrepository", + "Modify content in any repository in a namespace", + ), ( "namespace_change_containerpushrepository", "Update any existing push repository in a namespace", @@ -711,6 +715,10 @@ class Meta: "manage_roles_containerpullthroughdistribution", "Can manage role assignments on pull-through cache distribution", ), + ( + "pull_new_containerdistribution", + "Can pull new content via the pull-through cache distribution", + ), ] diff --git a/pulp_container/app/registry_api.py b/pulp_container/app/registry_api.py index e1d21e604..1dce3980d 100644 --- a/pulp_container/app/registry_api.py +++ b/pulp_container/app/registry_api.py @@ -51,7 +51,7 @@ from rest_framework.status import HTTP_401_UNAUTHORIZED from pulp_container.app import models, serializers -from pulp_container.app.authorization import AuthorizationService +from pulp_container.app.authorization import AuthorizationService, PermissionChecker from pulp_container.app.cache import ( find_base_path_cached, FlatpakIndexStaticCache, @@ -291,7 +291,9 @@ def get_drv_pull(self, path): Get distribution, repository and repository_version for pull access. """ try: - distribution = models.ContainerDistribution.objects.get(base_path=path) + distribution = models.ContainerDistribution.objects.prefetch_related( + "pull_through_distribution" + ).get(base_path=path) except models.ContainerDistribution.DoesNotExist: # get a pull-through cache distribution whose base_path is a substring of the path return self.get_pull_through_drv(path) @@ -345,6 +347,7 @@ def get_pull_through_drv(self, path): remote=remote, repository=repository, private=pull_through_cache_distribution.private, + namespace=pull_through_cache_distribution.namespace, ) except IntegrityError: # some entities needed to be created, but their keys already exist in the database @@ -1036,10 +1039,13 @@ def handle_safe_method(self, request, path, pk): tag = models.Tag.objects.get(name=pk, pk__in=repository_version.content) except models.Tag.DoesNotExist: distribution = distribution.cast() + permission_checker = PermissionChecker(request.user) if ( distribution.remote - and distribution.pull_through_distribution_id - and request.user.is_authenticated + and distribution.pull_through_distribution + and permission_checker.has_modify_pull_through_cache_permissions( + distribution.pull_through_distribution, distribution + ) ): remote = distribution.remote.cast() # issue a head request first to ensure that the content exists on the remote @@ -1090,10 +1096,13 @@ def handle_safe_method(self, request, path, pk): manifest = None distribution = distribution.cast() + permission_checker = PermissionChecker(request.user) if ( distribution.remote - and distribution.pull_through_distribution_id - and request.user.is_authenticated + and distribution.pull_through_distribution + and permission_checker.has_modify_pull_through_cache_permissions( + distribution.pull_through_distribution, distribution + ) ): remote = distribution.remote.cast() self.fetch_manifest(remote, pk) @@ -1109,7 +1118,7 @@ def get_content_units_to_add(self, manifest, tag): models.MEDIA_TYPE.MANIFEST_LIST, models.MEDIA_TYPE.INDEX_OCI, ): - for listed_manifest in manifest.listed_manifests: + for listed_manifest in manifest.listed_manifests.all(): add_content_units.append(listed_manifest.pk) add_content_units.append(listed_manifest.config_blob_id) add_content_units.extend(listed_manifest.blobs.values_list("pk", flat=True)) @@ -1149,6 +1158,11 @@ def fetch_manifest(self, remote, pk): digest = response.headers.get("docker-content-digest") return models.Manifest.objects.filter(digest=digest).first() + @staticmethod + def has_modify_permissions(user, distribution): + permission_checker = PermissionChecker(user) + return permission_checker.has_permission(distribution, "GET", "pull_new_content", {}) + def put(self, request, path, pk=None): """ Responds with the actual manifest diff --git a/pulp_container/app/viewsets.py b/pulp_container/app/viewsets.py index e42be79f9..8ae0bdcf4 100644 --- a/pulp_container/app/viewsets.py +++ b/pulp_container/app/viewsets.py @@ -1298,6 +1298,14 @@ class ContainerDistributionViewSet(DistributionViewSet, RolesMixin): "has_model_or_obj_perms:container.manage_roles_containerdistribution" ], }, + { + "action": ["pull_new_content"], + "principal": "authenticated", + "effect": "allow", + "condition": [ + "has_namespace_obj_perms:container.namespace_modify_content_containerrepository" + ], + }, ], "creation_hooks": [ { @@ -1453,6 +1461,14 @@ class ContainerPullThroughDistributionViewSet(DistributionViewSet, RolesMixin): "has_model_or_obj_perms:container.manage_roles_containerpullthroughdistribution" ], }, + { + "action": ["pull_new_distribution"], + "principal": "authenticated", + "effect": "allow", + "condition_expression": [ + "has_model_or_obj_perms:container.pull_new_containerdistribution" + ], + }, ], "creation_hooks": [ { @@ -1472,12 +1488,15 @@ class ContainerPullThroughDistributionViewSet(DistributionViewSet, RolesMixin): "container.delete_containerpullthroughdistribution", "container.change_containerpullthroughdistribution", "container.manage_roles_containerpullthroughdistribution", + "container.pull_new_containerdistribution", ], "container.containerpullthroughdistribution_collaborator": [ "container.view_containerpullthroughdistribution", + "container.pull_new_containerdistribution", ], "container.containerpullthroughdistribution_consumer": [ "container.view_containerpullthroughdistribution", + "container.pull_new_containerdistribution", ], } @@ -1544,7 +1563,9 @@ class ContainerNamespaceViewSet( "action": ["view_distribution"], "principal": "authenticated", "effect": "allow", - "condition": "has_model_or_obj_perms:container.namespace_view_containerdistribution", # noqa: E501 + "condition": [ + "has_model_or_obj_perms:container.namespace_view_containerdistribution" + ], }, { "action": ["list_roles", "add_role", "remove_role"], @@ -1579,6 +1600,7 @@ class ContainerNamespaceViewSet( "container.namespace_change_containerdistribution", "container.namespace_view_containerpushrepository", "container.namespace_modify_content_containerpushrepository", + "container.namespace_modify_content_containerrepository", "container.namespace_change_containerpushrepository", "container.manage_roles_containernamespace", ], @@ -1592,6 +1614,7 @@ class ContainerNamespaceViewSet( "container.namespace_change_containerdistribution", "container.namespace_view_containerpushrepository", "container.namespace_modify_content_containerpushrepository", + "container.namespace_modify_content_containerrepository", "container.namespace_change_containerpushrepository", ], "container.containernamespace_consumer": [ diff --git a/pulp_container/tests/functional/api/test_pull_through_cache.py b/pulp_container/tests/functional/api/test_pull_through_cache.py index 5dae32bfe..f4f6ac01d 100644 --- a/pulp_container/tests/functional/api/test_pull_through_cache.py +++ b/pulp_container/tests/functional/api/test_pull_through_cache.py @@ -12,6 +12,7 @@ PULP_HELLO_WORLD_REPO, PULP_HELLO_WORLD_LINUX_AMD64_DIGEST, PULP_FIXTURE_1, + PULP_FIXTURE_1_MANIFEST_A_DIGEST, ) @@ -111,31 +112,46 @@ def test_manifest_pull(delete_orphans_pre, pull_through_distribution, pull_and_v pull_and_verify(images, pull_through_distribution()) -def test_anonymous_pull_by_digest( +def test_pull_by_digest( delete_orphans_pre, add_pull_through_entities_to_cleanup, anonymous_user, local_registry, pull_through_distribution, + gen_user, ): - image = f"{PULP_HELLO_WORLD_REPO}@{PULP_HELLO_WORLD_LINUX_AMD64_DIGEST}" - local_image_path = f"{pull_through_distribution().base_path}/{image}" + image1 = f"{PULP_HELLO_WORLD_REPO}@{PULP_HELLO_WORLD_LINUX_AMD64_DIGEST}" + image2 = f"{PULP_FIXTURE_1}@{PULP_FIXTURE_1_MANIFEST_A_DIGEST}" + local_image_path1 = f"{pull_through_distribution().base_path}/{image1}" + local_image_path2 = f"{pull_through_distribution().base_path}/{image2}" with anonymous_user, pytest.raises(CalledProcessError): - local_registry.pull(local_image_path) + local_registry.pull(local_image_path1) - local_registry.pull(local_image_path) + local_registry.pull(local_image_path1) - add_pull_through_entities_to_cleanup(local_image_path.split("@")[0]) + add_pull_through_entities_to_cleanup(local_image_path1.split("@")[0]) with anonymous_user: - local_registry.pull(local_image_path) + local_registry.pull(local_image_path1) + + with gen_user(): + local_registry.pull(local_image_path1) + + with gen_user(), pytest.raises(CalledProcessError): + local_registry.pull(local_image_path2) + + with gen_user(model_roles=["container.containernamespace_collaborator"]): + local_registry.pull(local_image_path1) + local_registry.pull(local_image_path2) def test_pull_from_private_distribution( delete_orphans_pre, add_pull_through_entities_to_cleanup, anonymous_user, + container_namespace_api, + container_distribution_api, local_registry, pull_through_distribution, gen_user, @@ -143,21 +159,50 @@ def test_pull_from_private_distribution( if settings.TOKEN_AUTH_DISABLED: pytest.skip("RBAC cannot be tested when token authentication is disabled") - image = f"{PULP_HELLO_WORLD_REPO}:latest" - local_image_path = f"{pull_through_distribution(private=True).base_path}/{image}" + pull_through_distribution = pull_through_distribution(private=True) + + image1 = f"{PULP_HELLO_WORLD_REPO}:latest" + image2 = f"{PULP_FIXTURE_1}:manifest_a" + local_image_path1 = f"{pull_through_distribution.base_path}/{image1}" + local_image_path2 = f"{pull_through_distribution.base_path}/{image2}" with anonymous_user, pytest.raises(CalledProcessError): - local_registry.pull(local_image_path) + local_registry.pull(local_image_path1) - local_registry.pull(local_image_path) + local_registry.pull(local_image_path1) - add_pull_through_entities_to_cleanup(local_image_path.split(":")[0]) + namespace = container_namespace_api.list(name=pull_through_distribution.base_path).results[0] + add_pull_through_entities_to_cleanup(local_image_path1.split(":")[0]) with anonymous_user, pytest.raises(CalledProcessError): - local_registry.pull(local_image_path) - - with gen_user(model_roles=["container.containernamespace_collaborator"]): - local_registry.pull(local_image_path) + local_registry.pull(local_image_path1) + + namespace_collaborator = gen_user( + object_roles=[("container.containernamespace_collaborator", namespace.pulp_href)] + ) + with namespace_collaborator: + local_registry.pull(local_image_path1) + + with gen_user(model_roles=["container.containerpullthroughdistribution_consumer"]): + local_registry.pull(local_image_path1) + # test if the user can pull a completely new image through the cache + local_registry.pull(local_image_path2) + + add_pull_through_entities_to_cleanup(local_image_path2.split(":")[0]) + + with gen_user(model_roles=["container.containerdistribution_collaborator"]): + local_registry.pull(local_image_path2) + + distro1_name = local_image_path1.split(":")[0] + distro1 = container_distribution_api.list(name=distro1_name).results[0] + distro_1_collaborator = gen_user( + object_roles=[("container.containerdistribution_collaborator", distro1.pulp_href)] + ) + with distro_1_collaborator: + local_registry.pull(local_image_path1) + + with distro_1_collaborator, pytest.raises(CalledProcessError): + local_registry.pull(local_image_path2) def test_conflicting_names_and_paths( diff --git a/pulp_container/tests/functional/constants.py b/pulp_container/tests/functional/constants.py index cb6c29004..1aaeb6963 100644 --- a/pulp_container/tests/functional/constants.py +++ b/pulp_container/tests/functional/constants.py @@ -37,6 +37,9 @@ # a repository containing 4 manifest lists and 5 manifests PULP_FIXTURE_1 = "pulp/test-fixture-1" +PULP_FIXTURE_1_MANIFEST_A_DIGEST = ( + "sha256:d8fbbbf3fec1857c32c110292a9decf9744f9f97d7247019ae4776c241395221" +) # a dummy repository containing two manifests (index and image) with an arbitrary bootc label PULP_LABELED_FIXTURE = "pulp/bootc-labeled" diff --git a/staging_docs/admin/learn/rbac.md b/staging_docs/admin/learn/rbac.md index 8e93c3b19..a628f1045 100644 --- a/staging_docs/admin/learn/rbac.md +++ b/staging_docs/admin/learn/rbac.md @@ -297,6 +297,7 @@ object permissions for the namespace: "container.namespace_change_containerdistribution" "container.namespace_view_containerpushrepository" "container.namespace_modify_content_containerpushrepository" +"container.namespace_modify_content_containerrepository" ``` The users in the owners group have the permissions to add/remove users from all three groups @@ -321,6 +322,7 @@ following object permissions for the namespace: "container.namespace_change_containerdistribution" "container.namespace_view_containerpushrepository" "container.namespace_modify_content_containerpushrepository" +"container.namespace_modify_content_containerrepository" ``` Users in the Collaborator group can do everything that the owners can, with the exception of @@ -398,7 +400,7 @@ with the Distribution: "container.modify_content_containerpushrepository" ``` -Users in the Collaborator group can do everything that the owners can, with the exception of deleting +Users in the Collaborator group can do everything that the owners can, with the exception for deleting the Distribution. #### Distribution Consumers @@ -422,6 +424,49 @@ Users in the Consumers group can the `pull` the repository. Users should only ne this group if the Distribution has been configured with `private=True`. If the Distribution is public, then anyone can `pull` from the repository associated with the Distribution. + +#### Pull-through Distribution Owners + +This role allows users to manage and pull content from the pull-through cache distribution. + +``` +"container.view_containerpullthroughdistribution" +"container.delete_containerpullthroughdistribution" +"container.change_containerpullthroughdistribution" +"container.manage_roles_containerpullthroughdistribution" +"container.pull_new_containerdistribution" +``` + +#### Pull-through Distribution Collaborators + +Users who have this role assigned can preview and pull new content from the main pull-through cache +distribution. + +``` +"container.view_containerpullthroughdistribution" +"container.pull_new_containerdistribution" +``` + +#### Pull-through Distribution Consumers + +Similarly to the collaborator role, the following set of permissions is set for the consumer role: + +``` +"container.view_containerpullthroughdistribution" +"container.pull_new_containerdistribution" +``` + +It is recommended to assign at least one role with these permissions to allow users to pull new +content from a remote repository: +``` +"container.namespace_modify_content_containerrepository" (e.g., namespace collaborator) +"container.namespace_add_containerdistribution" (e.g., namespace collaborator) +"container.pull_new_containerdistribution" (e.g., pull-through cache consumer) +``` + +Users without the permissions can still pull already cached content from Pulp. This behaviour is +further restricted by flagging a distribution as `private=True`. + ### Private Repositories Users wishing to `pull` from a Container Distribution with `private=True`