Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle users' permissions when pulling data via pull-through distributions #1659

Merged

Conversation

lubosmj
Copy link
Member

@lubosmj lubosmj commented Jun 11, 2024

closes #1657
closes #1623
closes #1624

@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch 2 times, most recently from 275c623 to b46d85d Compare June 11, 2024 20:18
@@ -0,0 +1 @@
Disallowed anonymous users to pull new content via a pull-through caching distribution.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add that anonymous users could still pull 'cached' content?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not read through the code, but how this scenario will be handled:
Anonymos user pulls content (from existing repo) but there is an updated content remotely? We should not be allowing this, because as a result of such operation a new repo version will be created and more bits will be pulled down into pulp

Copy link
Member Author

@lubosmj lubosmj Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anonymos user pulls content (from existing repo) but there is an updated content remotely?

Good catch! I may want to introduce another check in

if distribution.remote and distribution.pull_through_distribution_id:
and
if distribution.remote_id and distribution.pull_through_distribution_id:
. Is there a guarantee that the same headers are passed during the redirect? With BasicAuth, we would want to be sure that the user headers remain the same inside content-app to prevent pulling.

can you add that anonymous users could still pull 'cached' content?

Is "new content" not explicit enough?

@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch from b46d85d to 5742557 Compare June 12, 2024 09:23
@lubosmj lubosmj marked this pull request as draft June 12, 2024 09:31
@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch from 5742557 to 59939fb Compare June 25, 2024 17:28
@lubosmj
Copy link
Member Author

lubosmj commented Jun 25, 2024

I am still trying to figure out how to restrict the access to content-app for anonymous users. Checking the basic auth header is not enough.

@lubosmj
Copy link
Member Author

lubosmj commented Jul 1, 2024

Actually, we do not need to restrict the access to content-app. Similarly, we do not pass a token to content-app and check the access scope. Everything is tested inside api-app. So, I am making this PR ready for review.

@lubosmj lubosmj marked this pull request as ready for review July 1, 2024 13:36
@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch 6 times, most recently from 4a02891 to 84b9f36 Compare July 3, 2024 20:23
@lubosmj lubosmj changed the title Disallow anonymous users to pull new data via pull-through distributions Handle anonymous users pulling data via pull-through distributions Jul 3, 2024
@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch 3 times, most recently from 24a9188 to 67809ea Compare July 4, 2024 19:47
@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch from 67809ea to 78ca84c Compare July 4, 2024 19:50
@lubosmj lubosmj marked this pull request as draft July 4, 2024 22:27
@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch 4 times, most recently from 9ea4125 to dd6c2a4 Compare July 5, 2024 11:39
@lubosmj lubosmj changed the title Handle anonymous users pulling data via pull-through distributions Handle users' permissions when pulling data via pull-through distributions Jul 7, 2024
@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch from dd6c2a4 to 5174376 Compare July 7, 2024 23:23
@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch 5 times, most recently from 2d37cc3 to 3ccfc8f Compare July 8, 2024 09:27
@lubosmj lubosmj marked this pull request as ready for review July 8, 2024 09:28
@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch from 3ccfc8f to 4e40f43 Compare July 8, 2024 09:38
@lubosmj lubosmj force-pushed the 1657-fix-token-auth-pulling-pushing-name-clash branch from 4e40f43 to cdd5575 Compare July 8, 2024 09:40
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see anything out of the odinary.

Only looked at: "Enforce permission checks while pulling from pull-through distributions"

Comment on lines -30 to -33
elif type(obj) is models.ContainerPushRepositoryVersion:
for dist in obj.repository.distributions.all():
if request.user.has_perm(permission, dist.cast().namespace):
return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this never triggered?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. And we do not have such a model.

and distribution.pull_through_distribution_id
and request.user.is_authenticated
and distribution.pull_through_distribution
and permission_checker.has_pull_through_permissions(distribution)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and a similar one below) is the core of the change, right?

Copy link
Member Author

@lubosmj lubosmj Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, once we pass through the registry token steps and a user can access the particular endpoint, we should finally ensure that users without valid permissions cannot pull new content into a repository. But, they can still pull existing content.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This granularity cannot be achieved purely with token authentication. That is why I had to introduce this check.

@git-hyagi
Copy link
Contributor

When I was testing this change, I was receiving errors like: "Repository not found", but the remote repository was available. Then, I realized the error was because of the lack of authentication.
What if we return 401 for
https://github.com/pulp/pulp_container/pull/1659/files#diff-1b944e15118f82233aa03a435e9e05c188ad0d371b71be43260506b4a5eeddffR315 (not self.request.user.is_authenticated)
https://github.com/pulp/pulp_container/pull/1659/files#diff-1b944e15118f82233aa03a435e9e05c188ad0d371b71be43260506b4a5eeddffR1046 (not permission_checker.has_pull_through_permissions(distribution))
https://github.com/pulp/pulp_container/pull/1659/files#diff-1b944e15118f82233aa03a435e9e05c188ad0d371b71be43260506b4a5eeddffR1101 (not permission_checker.has_pull_through_permissions(distribution))
?

for example:

    if (
        distribution.remote
        and distribution.pull_through_distribution_id
        and request.user.is_authenticated
    ):
        <logic block>
    elif not request.user.is_authenticated:
        raise UnauthorizedException()
    else:
        raise ManifestNotFound(reference=pk)

Trying to pull from a private pull-through distribution, it returned:

requested access to the resource is denied

instead of "repository not found".

@lubosmj
Copy link
Member Author

lubosmj commented Jul 14, 2024

The idea is to not change the default behaviour. Once there is no such a repository existing in Pulp, we should return 404 in case an unauthorized user is trying to pull it. If the repository exists, and it is private, we should return 401 when the unauthorized user is accessing it. This whole workflow should be transparent to pull-through caching. I think more granular error handling is not needed in this case.

@lubosmj lubosmj merged commit 35c6dee into pulp:main Jul 19, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants