Rewrite auth delegation to use a server-side POST instead of a client-side GET to delegate API Key

RESOLVED FIXED in Bugzilla 6.0

Status

()

Bugzilla
User Accounts
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcote, Assigned: dylan)

Tracking

({bmo-goal})

Bugzilla 6.0
bmo-goal
Bug Flags:
approval +

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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)

Updated

3 years ago
Keywords: bmo-goal
(Assignee)

Comment 1

3 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

3 years ago
Bug 1045145 comment #10 strongly is where I infer that a 16 byte token is not secure enough.

Comment 3

3 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

3 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

3 years ago
Sessions tokens are one-time tokens already.
(Assignee)

Comment 7

3 years ago
Created attachment 8638636 [details] [diff] [review]
1175643_1.patch

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

3 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

Updated

3 years ago
Duplicate of this bug: 1187504
I have just updated BzDeck to support POST requests. https://github.com/bzdeck/bzdeck/issues/297
(Assignee)

Updated

3 years ago
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+

Updated

3 years ago
Flags: approval?

Updated

3 years ago
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
(Assignee)

Comment 12

3 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   5fd2b62..07a68c6  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Target Milestone: --- → Bugzilla 6.0
You need to log in before you can comment on or make changes to this bug.