Skip to content

Refactor authentication mechanisms #7266

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

Open
ewdurbin opened this issue Jan 19, 2020 · 3 comments · May be fixed by #13854
Open

Refactor authentication mechanisms #7266

ewdurbin opened this issue Jan 19, 2020 · 3 comments · May be fixed by #13854
Labels
2FA needs discussion a product management/policy issue maintainers and users should discuss security Security-related issues and pull requests

Comments

@ewdurbin
Copy link
Member

Motivation

Two authenticate issues were reported by @ewjoachim which both came down to our global authentication policy.

Web UI Authentication and 2FA bypass via API Tokens (Macaroons)

API tokens are advertised as only being valid for uploads, however by setting the appropriate header, Authorization: token pypi-....., requests for arbitrary actions could be made with the equivalent of a standard session.

Thus leaked API tokens regardless of scope may have had a much bigger impact than advertised (uploading rogue releases vs deleting releases/projects or modifying user account components)

Initially resolved in: #7184

Web UI 2FA bypass via Basic Auth

Similar to above, constructing and setting the appropriate header, Authorization: Basic <base64>, requests for arbitrary actions could be made with the equivalent of a standard session.

Thus, 2FA bypass was possible if an attacker had the username and password for a user.

Initially resolved in: #7186

Initial Discussion

After report, while verifying and implementing the above fixes @dstufft some longer term solutions to avoid these kinds of issues:

A view deriver that can be applied to the view_config that flags it as accepting specific authentication types:

def api_view(view, info):
    if info.options.get("is_api"):
        @functools.wraps(view)
        def wrapped(context, request):
            # Somehow check the authentication mechanism and reject if not Macaroon or Basic Auth
            return view(context, request)
    else:
        @functools.wraps(view)
        def wrapped(context, request):
            # Same as above, but the inverse
            return view(context, request)
    return wrapped

Another suggestion was running API/upload hosts with specific configurations to enable the appropriate methods.

Ultimately this issue is being opened to discuss alternative approaches and decide on a path forward.

@brainwane brainwane added the needs discussion a product management/policy issue maintainers and users should discuss label Jan 21, 2020
@ewjoachim
Copy link
Contributor

ewjoachim commented Mar 1, 2020

Disclaimer: I'm new to Pyramid.

Given that a user visiting a web page with a session cookie, basic auth or macaroon is not the same thing, I wonder if it wouldn't be much more appropriate to describe them as different principals... This way, the Authentication part would say "This is user A with a session" / "This is user B with a macaroon valid for projects C" etc, and then the authorization part would be much closer to doing the job the authorization part should do: answering whether principal has permission on context.

This looks relevant given Authorization recieves request, and authentication doesn't. Currently, we have to rely on a "hackish way" to get the request in the authentication part.

Also, this would solve a few concerning problems around authorization and authentication:

  • Authentication currently doesn't check for macaroon signature (I believe it's not a bug per se: I havent found a way to use Macaroon Authentication without Macaroon Authorization which does check)
  • Macaroon Autorization doesn't use Macaroon Authentication's results, meaning we could use a session authentication, and our macaroon would still be examined (but for nothing).

I would be much more at ease if macaroons were read once on the request in Authentication, and then somehow used as principal. And this would easily solve the question of limiting some pages to some auth methods (we would change the __acl__ methods for project and user)

Does that make sense @ewdurbin ?

I'd like to start a proof of concept PR, and this will probably require working on #7183 too.

@woodruffw
Copy link
Member

Now that we've refactored Warehouse's authn/z in terms of Pyramid's security policies, I'm going to take another look at this. In particular, I think this is mostly resolved by the combination of the new security policy APIs + the AuthenticationMechanism enum, but I'll comb through and make sure.

@dstufft dstufft linked a pull request Jun 4, 2023 that will close this issue
@miketheman miketheman added security Security-related issues and pull requests 2FA labels Jul 6, 2023
@merwok
Copy link
Contributor

merwok commented Nov 29, 2024

There were projects to handle this cleanly, but they haven’t been active in a long time:

They may serve as inspiration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2FA needs discussion a product management/policy issue maintainers and users should discuss security Security-related issues and pull requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants