Closed
Bug 1175643
Opened 9 years ago
Closed 9 years ago
Rewrite auth delegation to use a server-side POST instead of a client-side GET to delegate API Key
Categories
(Bugzilla :: User Accounts, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 6.0
People
(Reporter: mcote, Assigned: dylan)
References
Details
(Keywords: bmo-goal)
Attachments
(1 file)
3.01 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
Right now the login and API key are passed to the app's callback in the query string of a GET request. This means there's a high likelihood for the API key to exist in logs, which is not great. Instead, let's generate a one-time token and pass that in the query string (username is probably still okay to include). Instead of calling verify_login(), the client would call a new endpoint that would return the API key for the given token (and verify the username?) and then delete the token. This would serve the purpose of verify_login(), so no extra calls on the client.
Assignee | ||
Comment 1•9 years ago
|
||
Ugh. I realized the implementation of this isn't good enough to pass a security review. Tokens have a length of 16 bytes. API keys have a length 40, which was a result of the security review for API keys (as I remember reading in the comments for that bug). Even though the tokens are single use, the search space is much too shallow at 16 bytes. This leaves a few options, all of which have downsides: 1) Extend the tokens table to use 40-byte tokens. This has unknown consequences on existing code and is my least favorable option... 2) Add another column to user_api_keys, that is nullable. api_key_one_time or api_key_coupon. Once this "coupon" is redeemed, set it to null. 3) Add another table for these api key coupons (or whatever we end up calling this). I'll do either 2 or 3. glob/dkl, thoughts?
Status: NEW → ASSIGNED
Flags: needinfo?(glob)
Flags: needinfo?(dkl)
Assignee | ||
Comment 2•9 years ago
|
||
Bug 1045145 comment #10 strongly is where I infer that a 16 byte token is not secure enough.
Comment 3•9 years ago
|
||
I'm personally really worried about the number of ways to authenticate a session. login+password, API tokens, one-time tokens, session tokens, hash tokens. I suspect Bugzilla will shoot itself in the foot sooner or later.
(In reply to Dylan William Hardison [:dylan] from comment #1) > 1) Extend the tokens table to use 40-byte tokens. This has unknown > consequences on existing code and is my least favorable option... > 2) Add another column to user_api_keys, that is nullable. api_key_one_time > or api_key_coupon. Once this "coupon" is redeemed, set it to null. > 3) Add another table for these api key coupons (or whatever we end up > calling this). > > I'll do either 2 or 3. glob/dkl, thoughts? 1 is my preferred option, by a long way. extending the size of the token column from 16 to 40 chars wouldn't impact any existing code (ideally we should increase the default length of tokens too, that's bug 1179856).
Flags: needinfo?(glob)
Reporter | ||
Comment 5•9 years ago
|
||
(In reply to Frédéric Buclin from comment #3) > I'm personally really worried about the number of ways to authenticate a > session. login+password, API tokens, one-time tokens, session tokens, hash > tokens. I suspect Bugzilla will shoot itself in the foot sooner or later. You raise a very good point. I proposed introducing this new token type only because communicating an API key via GET query string is a bad idea due to the probability of them ending up in logs. I am completely open to other suggestions for this communication. I don't think a simple POST request will work because the callback is a redirect... although more complex, maybe there could be a redirect followed by a direct API-level POST with the key? Argh it really feels like we're reinventing OAuth.
Comment 6•9 years ago
|
||
Sessions tokens are one-time tokens already.
Assignee | ||
Comment 7•9 years ago
|
||
First version of the patch. Documentation will need to be updated, but I wanted to get eyeballs on this sooner than later. Simple (bug-ridden) consumer side written in python: https://gist.github.com/dylanwh/9cf0468d1ca4944e1c80
Attachment #8638636 -
Flags: review?(dkl)
Assignee | ||
Updated•9 years ago
|
Summary: Use a one-time token in auth delegation → Rewrite auth delegation to use a server-side POST instead of a client-side GET to delegate API Key
Comment 9•9 years ago
|
||
I have just updated BzDeck to support POST requests. https://github.com/bzdeck/bzdeck/issues/297
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(dkl)
Comment 10•9 years ago
|
||
Comment on attachment 8638636 [details] [diff] [review] 1175643_1.patch Review of attachment 8638636 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Nits on checkin. r=dkl ::: auth.cgi @@ +95,5 @@ > + $ua->protocols_allowed(['http', 'https']); > + # If the URL of the proxy is given, use it, else get this information > + # from the environment variable. > + my $proxy_url = Bugzilla->params->{'proxy_url'}; > + if ($proxy_url) { nit if (my $proxy_url = Bugzilla->params->{'proxy_url'}) { $ua->proxy(['http', 'https'], $proxy_url); } else ... @@ +117,5 @@ > + ThrowUserError('auth_delegation_json_error', { json_text => $resp->content }); > + } > + else { > + print $cgi->redirect($callback_uri); > + } Nit: can shorten to $@ && ThrowUserError('auth_delegation_json_error', { json_text => $resp->content }); print $cgi->redirect($callback_uri);
Attachment #8638636 -
Flags: review?(dkl) → review+
Updated•9 years ago
|
Flags: approval?
Updated•9 years ago
|
Flags: approval? → approval+
Comment 11•9 years ago
|
||
Is the patch final? I'm not familiar with Perl but it looks different from what :dylan explained in https://github.com/bzdeck/bzdeck/issues/298
Assignee | ||
Comment 12•9 years ago
|
||
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 5fd2b62..07a68c6 master -> master
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Target Milestone: --- → Bugzilla 6.0
You need to log in
before you can comment on or make changes to this bug.
Description
•