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.

Sign up for GitHub

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

Jump to bottom

One time use project upload token #6935

Closed
wants to merge 114 commits into from
Closed

One time use project upload token #6935

wants to merge 114 commits into from

Conversation

rachelcipkins
Copy link

Created a one time user scoped token for project uploads. Implemented as a caveat. #6378

@rachelcipkins
Psuedo code added for issue pypi#6255.
34f5e96
@rachelcipkins
Added caveats for project version and macaroon expiration time.
545dea0
@rachelcipkins
Added tests for expiration time.
cd4e8a2
@rachelcipkins
Updated caveats for expiration time and release.
03ea2ce
@rachelcipkins
Added macaroon fields for expiration time and release.
1447ed7
@rachelcipkins
Added expiration field to macaroon.
0d2733d
@rachelcipkins
Added form fields for expiration and release.
a89a25e
@rachelcipkins
Moved release caveat.
8826d39
@rachelcipkins
Added potential new Macaroon fields.
d3cb195
@rachelcipkins
Added scope, release, and expiration fields to Macaroon object.
71f7c1b
@rachelcipkins
Added scope, release, and expiration fields to create_macaroon.
2e62cfc
@rachelcipkins
Changed release to be a text input.
29dcb8c
@rachelcipkins
Updated validate_release.
0de8289
@rachelcipkins
Added verification for release.
e96b1b0
@rachelcipkins
Updated timezone info to GMT for POC.
5cbadb0
@rachelcipkins
Removed macaroon db fields for scope, release, and expiration. Added …
1913b28 
…these in as caveat permissions instead.
@rachelcipkins
Updated functionality for release caveat.
b5bb8ce
@rachelcipkins
Added validation for releases.
63cfeee
@rachelcipkins
Added property to macaroon views to get all of a user's projects.
77e80b0
@rachelcipkins
Removed testing lines.
ece4724
@rachelcipkins
Removed note.
c9feca4
@rachelcipkins
Merge branch 'tob-6255'
8631895
@rachelcipkins
Started adding tests for release and expiration caveats.
b63290a
@rachelcipkins
Started adding tests for release and expiration additions to the Crea…
8ffb416 
…teMacaroonForm.
@rachelcipkins
Changed wording on error messages added check to see if scope is user.
683448b
@rachelcipkins
Fixed logic error and typo.
b140178
@rachelcipkins
Changed release to releases.
7b3bd1e
@rachelcipkins
Fixed errors in existing tests.
ad8f16c
@rachelcipkins
Added all_projects attribute to default_response.
77183f7
@rachelcipkins
Revert "Added all_projects attribute to default_response."
f9d2fb5 
This reverts commit 77183f7.
@woodruffw woodruffw mentioned this pull request Nov 18, 2019
Closed
woodruffw
woodruffw reviewed Nov 26, 2019
View reviewed changes
warehouse/legacy/api/pypi.py Outdated
except InvalidMacaroon:
raise HTTPUnauthorized()
user = macaroon_service.find_userid(
request.master_key
Copy link
Member

Choose a reason for hiding this comment

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

I see that this master_key attribute gets mocked in the tests, but I can't find where it actually appears in the (real) request object. Does it get set anywhere, or is this WIP?

@woodruffw
warehouse: Refactor token creation endpoint
312f2e5 
Uses the auth framework and leans on the session/active request
user where possible, reuses the macaroon creation form used in the
UI, adds appropriate event recording, and propagates errors
as a JSON response.

Removes "one-time" tokens pending a refactor.
@woodruffw
tests: Update macaroon caveat tests
280144e
@woodruffw
warehouse/routes: Auto-format
6769844
@woodruffw
tests/unit: Begin rewriting tests
8fbb669
@woodruffw
tests/api: Add more token creation tests
7865c10
@woodruffw
tests: Update management form strings
634e807
@woodruffw
tests: Remove one-time-token tests
e262de7
@woodruffw
tests: Remove unused import
e24fe65
@woodruffw
warehouse/api: Fix import orders
7f33a34
@woodruffw
Merge branch 'master' into tob-one-time-token
66bd850
@woodruffw
warehouse, tests: Re-add one-time tokens
37051e9
@woodruffw
warehouse/macaroons: Remove unused import
87ca35a
@woodruffw
Copy link
Member

This PR now contains a few different features:

@woodruffw
Copy link
Member

Some notes:

  • The API endpoint should return an HTTP 400 (or 403, as appropriate) + JSON body on errors, but it currently returns HTML when authentication fails (e.g., when a macaroon fails caveat checks). This is probably happening somewhere further up in the auth policy and needs to be special cased; I need to look into this.

  • The semantics around one-time tokens and project version tokens need some discussion: the project/file upload API is currently designed to take one upload at a time, meaning that a version release that includes multiple distributions (one sdist, a couple of bdist-*) will need N one-time tokens for N uploads. Similarly, a release is considered "created" once the first file associated with it is uploaded, meaning that macaroons bound to a single release will only work for the first uploaded file.

@brainwane
Copy link
Contributor

@rcipkins @woodruffw I suspect this is the kind of feature that could use a sentence of explanation in https://pypi.org/help/#apitoken .

@brainwane brainwane mentioned this pull request Jan 17, 2020
Closed
@merwok
Copy link
Contributor

merwok commented Jan 17, 2020

The API endpoint should return an HTTP 400 (or 403, as appropriate) + JSON body on errors, but it currently returns HTML when authentication fails […]. This is probably happening somewhere further up in the auth policy and needs to be special cased

Could be coming from the generic forbidden view:
https://github.com/pypa/warehouse/blob/master/warehouse/views.py#L94

A more specific view_config (using a specific exception as context or if not possible a custom predicate) could do what you want.

alex
alex reviewed Aug 28, 2020
View reviewed changes
warehouse/legacy/api/json.py
macaroon_service = request.find_service(IMacaroonService, context=None)

form = CreateMacaroonForm(
**payload,
Copy link
Member

Choose a reason for hiding this comment

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

Passing ** from attacker provided JSON is a risky move. It lets the attacker control which args are provided. If CreateMacaroonForm adds a new optional argument this can turn into a vulnerability. It ought to be passed as data=payload or explicitly unpacked.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, excellent point. This PR needs a decent amount of reworking (and discussion, w/r/t to the semantics of "one time use" for uploads of multiple files per release).

Base automatically changed from master to main January 21, 2021 18:39
@ewjoachim ewjoachim mentioned this pull request Mar 11, 2021
Open
@woodruffw woodruffw mentioned this pull request Feb 18, 2022
Closed
@woodruffw
Copy link
Member

I'm going to pick the core functionality out of this PR and include it in #10888. Closing.

@woodruffw woodruffw closed this Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woodruffw woodruffw woodruffw left review comments

@alex alex alex left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rachelcipkins @woodruffw @brainwane @merwok @alex