-
Notifications
You must be signed in to change notification settings - Fork 995
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
Macaroon-based API keys #6084
Macaroon-based API keys #6084
Conversation
@webknjaz, did you see @woodruffw's request above?
|
This comment has been minimized.
This comment has been minimized.
87a0f70
to
77c7018
Compare
This should be ready for initial testing, for the adventurous. Some quick notes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope this helps!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a maintainer of multiple projects: econ-ark
and Forms990-analysis
. I created an API token that was scoped to econ-ark
and then tried to use it to upload a Forms990-analysis
package, and got:
HTTPError: 403 Client Error: The user 'brainwane' isn't allowed to upload to project 'Forms990-analysis'. See http://localhost/help/#project-name for more information. for url: http://localhost/legacy/
This is not technically accurate -- this user account is allowed to upload to that project, but the token isn't. And maybe we don't want to leak the username in this error message. If we see that it's a token being used, can/should we change the error message in this case to something like "This token isn't allowed to upload to project 'name'"?
In the Docker logs, just for completeness: web_1 | [IP address] - @token [18/Jul/2019:01:03:01 +0000] "POST /legacy/ HTTP/1.1" 403 933 "-" "twine/1.13.0 pkginfo/1.5.0.1 requests/2.18.4 setuptools/41.0.1 requests-toolbelt/0.9.1 tqdm/4.32.2 CPython/3.7.3"
Yep, this needs to be changed here: It might be difficult to parametrize the error message in this context, so I could replace "user" with "credential owned by user" -- thoughts? Re: leaking the username: this check happens after an authorization check, so we already have a valid credential (just not one for this package). As such it isn't an infoleak per se (and might be useful to users doing complicated deployments, since their error messages will tell them which underlying user identity failed). |
Based on dstufft#2.
TODO: