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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 6.0

People

(Reporter: mcote, Assigned: dylan)

References

Details

(Keywords: bmo-goal)

Attachments

(1 file)

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.
Keywords: bmo-goal
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)
Bug 1045145 comment #10 strongly is where I infer that a 16 byte token is not secure enough.
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)
(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.
Sessions tokens are one-time tokens already.
Attached patch 1175643_1.patchSplinter Review
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)
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
I have just updated BzDeck to support POST requests. https://github.com/bzdeck/bzdeck/issues/297
Flags: needinfo?(dkl)
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+
Flags: approval?
Flags: approval? → approval+
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
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   5fd2b62..07a68c6  master -> master
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Bugzilla 6.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: