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

Fall back to BasicAuth if token is disabled #234

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

mdellweg
Copy link
Member

@mdellweg mdellweg commented Feb 8, 2021

@pulpbot
Copy link
Member

pulpbot commented Feb 8, 2021

Attached issue: https://pulp.plan.io/issues/8074

@mdellweg mdellweg force-pushed the token_vs_basic_auth branch 2 times, most recently from 3fa9a83 to 0c3cb86 Compare February 9, 2021 09:23
@mdellweg mdellweg marked this pull request as ready for review February 9, 2021 09:25
@mdellweg mdellweg force-pushed the token_vs_basic_auth branch 2 times, most recently from 6e07a87 to eb2cb06 Compare February 9, 2021 10:46
@property
def authentication_classes(self):
if settings.get("TOKEN_AUTH_DISABLED", False):
return api_settings.DEFAULT_AUTHENTICATION_CLASSES
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided that this is the better way, as we can fallback to the list of configured auth-classes.
Also it does not mess with the class creation process as __new__ does.

Copy link
Member

Choose a reason for hiding this comment

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

this changed fixed the direct api calls but still not for client push
return [api_settings.DEFAULT_AUTHENTICATION_CLASSES[1]]

Copy link
Member

Choose a reason for hiding this comment

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

i get this "Attempting next endpoint for push after error: unauthorized: Authentication credentials were not provided."
it feels like the client does not send the basic auth creds in the calls?

Copy link
Member Author

Choose a reason for hiding this comment

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

I used podman, and push with logged in as admin worked for me.

Copy link
Member

Choose a reason for hiding this comment

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

  Applying rpm.0029_rpmpublication_sqlite_metadata... OK
  Applying sessions.0001_initial... OK
Initialize missing access policies for core.
Initialize missing access policies for container.
Initialize missing access policies for python.
Initialize missing access policies for rpm.
Initialize missing access policies for file.
Initialize missing access policies for pulp_2to3_migration.
pulp [None]: pulp_2to3_migration.app.plugin:INFO: Plugin pulp_deb is not installed in pulp3 therefore it will not be migrated from pulp2
Successfully set password for "admin" user.
pulp [None]: pulp_2to3_migration.app.plugin:INFO: Plugin pulp_deb is not installed in pulp3 therefore it will not be migrated from pulp2

0 static files symlinked to '/var/lib/pulp/assets', 161 unmodified.
(pulp) [vagrant@pulp3-source-fedora32 ~]$ prestart
systemctl restart pulpcore-content pulpcore-worker@1 pulpcore-worker@2 pulpcore-resource-manager pulpcore-api
(pulp) [vagrant@pulp3-source-fedora32 ~]$ podman logout localhost:24817
Removed login credentials for localhost:24817
(pulp) [vagrant@pulp3-source-fedora32 ~]$ podman login localhost:24817
Username: invalid-user         
Password: 
Login Succeeded!
(pulp) [vagrant@pulp3-source-fedora32 ~]$ podman logout localhost:24817
Removed login credentials for localhost:24817
(pulp) [vagrant@pulp3-source-fedora32 ~]$ podman login localhost:24817
Username: admin
Password: 
Login Succeeded!
(pulp) [vagrant@pulp3-source-fedora32 ~]$ podman push localhost:24817/test/this:mytag1.8
Getting image source signatures
Copying blob 210dda196ec1 [--------------------------------------] 8.0b / 2.0KiB
Error: error copying image to the remote destination: Error writing blob: Error initiating layer upload to /v2/test/this/blobs/uploads/ in localhost:24817: unauthorized: Authentication credentials were not provided.
(pulp) [vagrant@pulp3-source-fedora32 ~]$ 

Copy link
Member

Choose a reason for hiding this comment

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

podman-2.2.1-1.fc32.x86_64

@mdellweg mdellweg force-pushed the token_vs_basic_auth branch 4 times, most recently from c7a5a8e to b9896b2 Compare February 9, 2021 16:45
Decide upon permission based on user.
"""
# TODO RBAC goes here
if request.user.is_staff:
Copy link
Member

Choose a reason for hiding this comment

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

I have tested this pr, user here is anonymours no matter what i am doing

  1. login into the registry permits to login even with wrong credentials
  2. if i'm accessing api directly this codepath is niot executed and i can perform get/post/put call without any auth provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. As long as we don't force "/v2/" to need credentials, podman will never try to provide them.
But we break anonymous pull if we do, because he will still try to find that and fallback to /v1ping/ instead.
This needs more investigation.

This will allow push for admin and pull for everyone including
AnonymousUser, if TOKEN_AUTH_DISABLED=True.

fixes #8074
https://pulp.plan.io/issues/8074
Copy link
Member

@ipanova ipanova left a comment

Choose a reason for hiding this comment

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

with this pr i was able to push with admin credentials and to pull whether with a valid logged in user or not being logged in( anonymous pull). I can confirm that push is possible only with admin credentials.
This PR however does does not take into account whether a distribution is private or not during pull operation because policy is defined on the distribution/namespace viewsets which are not used whenever token_auth is disabled.

@mdellweg mdellweg merged commit 3823647 into pulp:master Feb 16, 2021
@mdellweg mdellweg deleted the token_vs_basic_auth branch February 16, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants