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

Restrict Security Policies based on route type #13854

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dstufft
Copy link
Member

@dstufft dstufft commented Jun 4, 2023

Provides a mechanism to tag whether a route is an "API" route or not. The effects of tagging a route as an API route is that session basic authentication will no longer work on that route, but Basic and Macaroon authentication will.

Also adds a request.is_api attribute that indicates if the current request is an API request or not.

Currently the only route we tag as an API route is the upload API.

Fixes #7266

@dstufft dstufft requested a review from a team as a code owner June 4, 2023 18:10
Copy link
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

Overall, seems like a reasonable approach.

My one hesitation is around naming - is_api seems to encompass authentication behaviors, rather than a property of the route itself.
We create more than a few APIs - but not all have the same authentication method, so maybe we need a better name for this.

Prior art from Django Rest Framework combines two ideas to accomplish this - APIView and permission_classes - https://www.django-rest-framework.org/api-guide/authentication/#setting-the-authentication-scheme

If the intent is to retire request.is_api once we remove basic_auth, then go for it

@dstufft
Copy link
Member Author

dstufft commented Jun 7, 2023

We create more than a few APIs - but not all have the same authentication method, so maybe we need a better name for this.

I think that all of our APIs technically take the same authentication methods OR they are unauthenticated (and so the authentication methods don't matter anyways). This is mostly a "gimme" though, because the only API that actually takes authentication currently is the upload API :)

If the intent is to retire request.is_api once we remove basic_auth, then go for it

I don't think we'd retire request.is_api once BasicAuthSecurityPolicy is gone, since the underlying desire to ensure that session auth stays only in sessions, and api auth stays only in APIs.

I avoided anything that was associated with the view rather than the route for a few reasons:

  • The way ISecurityPolicy works in Pyramid, it's difficult to reliably know what view is being executed given a request.
    • There is a request.view_name, but that's actually for traversal which we don't use.
    • The trick we do for sessions, is using a view deriver (basically a decorator) to hot swap the request.session , which works but means that anything that runs before the view deriver (higher up in the deriver stack, certain event hooks) can access request.identity before we have a chance to do the hot swap.
  • In theory, whether or not something takes a certain kind of authentication is a property of the URL endpoint, not the particular view function that got called. AuthN mechanisms are public facing API, and are inherently tied to URLs from an end user POV. On the flip side, Permissions may vary per view (can have multiple views per URL, (e.g. for GET vs POST with different permissions to read).

It is kind of crummy that the definition of what AuthN methods for a view is located away from the view itself, but I think it's basically a trade off between making sure that request.identity can't be the "wrong" thing and ergonomics.

With regards to a boolean "Is API" vs "Takes These Authentication Methods", my thinking is:

  • It will be confusing for end users if we have different APIs that accept different authentication methods unless there is some kind of weird special case. Thus we should generally only support two different authentication stacks, at least as the default interface.
  • Right now our API tokens are restricted to upload, but I'm working on fixing that so we can re-use the same API token concept across the entire API surface.

That being said, that's two people who have pushed back on request.is_api, so I'm going to think about this some more to think if there is another solution that can resolve these concerns.

@miketheman miketheman added the security Security-related issues and pull requests label Jan 9, 2024
@warsaw
Copy link
Contributor

warsaw commented Oct 15, 2024

I landed here based on a comment in security_policy.py. I actually have at least two use cases for end-user APIs and the associated security for them, so I'm trying to research what the right way to protect APIs is. The two use cases are:

  • Programmatic yank. We have a package for which we are going to need to yank a bunch of releases, and we'd like to be able to do that programmatically, since doing so through the web ui will be painful. I have a nascent branch in progress to try to implement this.
  • PEP 694 for the Upload 2.0 API. I've resurrected a branch that tries to resolve merge conflicts from a previous attempt, but I am rethinking the implementation from first principles, like it should be an API-only functionality.

The two features will both leverage a fuller, client-accessible JSON-only API, so ensuring that those features are protected with the right security policies are paramount (as the actual functional logic, at least for yanking will likely be pretty simple).

Since this PR is now over a year old, I'm looking for guidance for API security policies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security-related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor authentication mechanisms
3 participants