Check if the Authorization header for Basic Authentication is valid #1589

lubosmj · 2024-04-13T14:51:24Z

If the header is not valid, DRF returns None when calling the
authenticate() method. This can cause troubles when users are
leveraging the remote authentication because Pulp thinks they
are anonymous users. In the end, authorized users cannot
push or pull content from Pulp. This affects only admin users
in scenarios where the token authentication is disabled.

closes #1577

lubosmj · 2024-04-13T15:59:45Z

I successfully tested image pushing with the following settings (with the enabled basic authentication and remote authentication at the same time):

dynaconf list

AUTHENTICATION_BACKENDS<list> ['django.contrib.auth.backends.ModelBackend', 
 'pulpcore.backends.ObjectRolePermissionBackend',
 'pulpcore.app.authentication.PulpNoCreateRemoteUserBackend'] 

REST_FRAMEWORK<dict> {'DEFAULT_AUTHENTICATION_CLASSES': ['rest_framework.authentication.BasicAuthentication',
                                    'rest_framework.authentication.SessionAuthentication',
                                    'rest_framework.authentication.SessionAuthentication',
                                    'pulpcore.app.authentication.PulpRemoteUserAuthentication'],

REMOTE_USER_ENVIRON_NAME<str> 'HTTP_REMOTE_USER'

Note that when omitting ['django.contrib.auth.backends.ModelBackend', 'pulpcore.backends.ObjectRolePermissionBackend'] in AUTHENTICATION_BACKENDS, Pulp receives AuthenticationFailed on super().authenticate(request) if a user is logged in as admin. Could this be the reason why it worked before? Katello could have used Basic Authentication together with Remote Authentication (#558).

etc/nginx/pulp/pulp_container.conf

location /v2/ {
    ...
    proxy_set_header Remote_User admin;
}

location /extensions/v2/ {
    ...
    proxy_set_header Remote_User admin;
}

location /pulp/container/ {
    ...
    proxy_set_header Remote_User admin;
}

location /token/ {
    ...
    proxy_set_header Remote_User admin;
}

Test with proxy headers being set (using both the basic authentication and remote authentication):

podman logout localhost:5001
podman push localhost:5001/myorg/mygroup/ubuntu:latest --tls-verify=false  # OK, implicit 'Remote-User': 'admin' header being favoured, remote authentication
podman login localhost:5001 -u admin -p password --tls-verify=false
podman push localhost:5001/myorg/mygroup/ubuntu:latest --tls-verify=false  # OK, implicit 'Remote-User': 'admin' header being favoured, podman does not send the Authorization header (running this as an anonymous user by default, podman sends the Authorization header if anonymous pushing does not work), remote authentication

Test without proxy headers being set (using the basic authentication only):

podman logout localhost:5001
podman push localhost:5001/myorg/mygroup/ubuntu:latest --tls-verify=false  # Error: writing manifest: uploading manifest latest to localhost:5001/myorg/mygroup/ubuntu: denied: Access to the requested resource is not authorized, remote authentication not available (missing  'Remote-User': 'admin'  header) and no user is logged in
podman login localhost:5001 -u admin -p password --tls-verify=false
podman push localhost:5001/myorg/mygroup/ubuntu:latest --tls-verify=false  # OK, using basic auth header, 'Authorization': 'Basic YWRtaW46cGFzc3dvcmQ='

lubosmj · 2024-04-13T16:01:48Z

pulp_container/app/token_verification.py

@@ -80,13 +81,18 @@ def authenticate(self, request):
            return (AnonymousUser, None)

        try:
-            return super().authenticate(request)
+            result = super().authenticate(request)


None is returned only when this is evaluated to True. We exploit this check.

That sounds brittle. Even with this explanation it's hard to understand. You should have a comment at the very least. Is the "empty username and password" working at all with remote auth?

What does the empty username and password mean in this context? If there is no Remote_User passed with the request, and no Authorization header is present, Pulp considers this request being anonymous, allowing standard pull operations for public repositories.

What i mean is: When remote auth is configured, and the reverse proxy terminates the authentication process, does it also accept empty for anonymous?

Yes, RemoteUserRegistryAuthentication().authenticate(request) returns None if there is no Remote_User header set. Returning None from the method means that we deal with AnonymousUser, same as with returning (AnonymousUser, None). Maybe we want to return None at all times, @ipanova?

I renamed result into a more explicit variable: authenticated_user.

ianballou

This works well testing container push with Katello.

lubosmj · 2024-04-19T12:33:11Z

After discussing this with Ina, I will try to test this once again with different settings for the remote authentication and return either (AnonymousUser, None) or None from the authenticate() method to get rid of discrepancies.

lubosmj · 2024-04-22T14:11:09Z

According to the rest_framework documentation, we should return either None or (user, auth) from our BaseAuthentication.authenticate() method. In case we would want to return an error response immediately, we would want to raise ApiException or AuthenticationFailed.

I removed return (AnonymousUser, None) from the authenticate() method because AnonymousUser is ultimately initialized by rest_framework, see the framework iterating over the authentication classes and initializing the user and default value of api_settings.UNAUTHENTICATED_USER. This helps us to adhere to the authentication mechanism provided by rest_framework.

Besides that, I tested latest changes with 1. AUTHENTICATION_BACKENDS<list> ['django.contrib.auth.backends.ModelBackend', 'pulpcore.backends.ObjectRolePermissionBackend', 'pulpcore.app.authentication.PulpNoCreateRemoteUserBackend'] , 2. AUTHENTICATION_BACKENDS<list> ['django.contrib.auth.backends.ModelBackend', 'pulpcore.backends.ObjectRolePermissionBackend'], and 3. AUTHENTICATION_BACKENDS<list> ['pulpcore.app.authentication.PulpNoCreateRemoteUserBackend']. Note that there might be users who can use basic authentication when accessing endpoints from localhost; however, access from outside the cluster is through a gateway that uses tokens. Thus, there is exists a scenario where both of the authentication methods can be allowed, as it was supported in the past.

ipanova · 2024-04-22T14:22:17Z

While you listed the combinations you tested of auth backends, did you test auth classes with remote auth only and remote auth + basic auth?

If the header is not valid, DRF returns None when calling the authenticate() method. This can cause troubles when users are leveraging the remote authentication because Pulp thinks they are anonymous users. In the end, authorized users cannot push or pull content from Pulp. This affects only admin users in scenarios where the token authentication is disabled. closes pulp#1577

ianballou

Verified again on Katello latest, still works 👍

patchback · 2024-04-23T17:31:36Z

Backport to 2.19: 💚 backport PR created

✅ Backport PR branch: patchback/backports/2.19/b1c5d706961d07875551283498fb0489343f6da8/pr-1589

Backported as #1600

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

ianballou · 2024-06-27T16:32:08Z

@lubosmj I'd like to request a backport for this on the 2.16 branch if possible. It applied cleanly on my system.

patchback · 2024-06-27T17:07:11Z

Backport to 2.16: 💚 backport PR created

✅ Backport PR branch: patchback/backports/2.16/b1c5d706961d07875551283498fb0489343f6da8/pr-1589

Backported as #1678

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

lubosmj · 2024-06-27T18:42:22Z

@ianballou, I have just released https://github.com/pulp/pulp_container/releases/tag/2.16.8!

ianballou · 2024-06-27T19:13:18Z

Thanks @lubosmj !

lubosmj force-pushed the remote-authentication-fix-1577 branch from 6c313b1 to 3f8f8fa Compare April 13, 2024 14:54

lubosmj marked this pull request as ready for review April 13, 2024 15:59

lubosmj commented Apr 13, 2024

View reviewed changes

lubosmj force-pushed the remote-authentication-fix-1577 branch 2 times, most recently from 51ed1c3 to 2a288fa Compare April 13, 2024 16:23

ianballou approved these changes Apr 15, 2024

View reviewed changes

lubosmj force-pushed the remote-authentication-fix-1577 branch from 2a288fa to ff895fb Compare April 16, 2024 10:56

lubosmj marked this pull request as draft April 19, 2024 12:30

lubosmj force-pushed the remote-authentication-fix-1577 branch from ff895fb to 96c7d4c Compare April 22, 2024 13:52

lubosmj force-pushed the remote-authentication-fix-1577 branch from 96c7d4c to 7b70f29 Compare April 23, 2024 15:01

lubosmj marked this pull request as ready for review April 23, 2024 15:11

ipanova approved these changes Apr 23, 2024

View reviewed changes

ianballou approved these changes Apr 23, 2024

View reviewed changes

lubosmj merged commit b1c5d70 into pulp:main Apr 23, 2024
15 checks passed

lubosmj added the backport-2.19 label Apr 23, 2024

patchback bot mentioned this pull request Apr 23, 2024

[PR #1589/b1c5d706 backport][2.19] Check if the Authorization header for Basic Authentication is valid #1600

Merged

ianballou mentioned this pull request Apr 23, 2024

Fixes #37380 - Container push can fail with a different JSON error Katello/katello#10977

Merged

lubosmj added the backport-2.16 label Jun 27, 2024

patchback bot mentioned this pull request Jun 27, 2024

[PR #1589/b1c5d706 backport][2.16] Check if the Authorization header for Basic Authentication is valid #1678

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if the Authorization header for Basic Authentication is valid #1589

Check if the Authorization header for Basic Authentication is valid #1589

lubosmj commented Apr 13, 2024 •

edited

Loading

lubosmj commented Apr 13, 2024 •

edited

Loading

lubosmj Apr 13, 2024 •

edited

Loading

mdellweg Apr 16, 2024

lubosmj Apr 16, 2024 •

edited

Loading

mdellweg Apr 16, 2024

lubosmj Apr 16, 2024 •

edited

Loading

lubosmj Apr 16, 2024 •

edited

Loading

ianballou left a comment

lubosmj commented Apr 19, 2024

lubosmj commented Apr 22, 2024

ipanova commented Apr 22, 2024

ianballou left a comment

patchback bot commented Apr 23, 2024 •

edited

Loading

ianballou commented Jun 27, 2024

patchback bot commented Jun 27, 2024 •

edited

Loading

lubosmj commented Jun 27, 2024

ianballou commented Jun 27, 2024

Check if the Authorization header for Basic Authentication is valid #1589

Check if the Authorization header for Basic Authentication is valid #1589

Conversation

lubosmj commented Apr 13, 2024 • edited Loading

lubosmj commented Apr 13, 2024 • edited Loading

dynaconf list

etc/nginx/pulp/pulp_container.conf

lubosmj Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

mdellweg Apr 16, 2024

Choose a reason for hiding this comment

lubosmj Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

mdellweg Apr 16, 2024

Choose a reason for hiding this comment

lubosmj Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

lubosmj Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

lubosmj commented Apr 19, 2024

lubosmj commented Apr 22, 2024

ipanova commented Apr 22, 2024

ianballou left a comment

Choose a reason for hiding this comment

patchback bot commented Apr 23, 2024 • edited Loading

Backport to 2.19: 💚 backport PR created

ianballou commented Jun 27, 2024

patchback bot commented Jun 27, 2024 • edited Loading

Backport to 2.16: 💚 backport PR created

lubosmj commented Jun 27, 2024

ianballou commented Jun 27, 2024

lubosmj commented Apr 13, 2024 •

edited

Loading

lubosmj commented Apr 13, 2024 •

edited

Loading

lubosmj Apr 13, 2024 •

edited

Loading

lubosmj Apr 16, 2024 •

edited

Loading

lubosmj Apr 16, 2024 •

edited

Loading

lubosmj Apr 16, 2024 •

edited

Loading

patchback bot commented Apr 23, 2024 •

edited

Loading

patchback bot commented Jun 27, 2024 •

edited

Loading