Closed Bug 1045145 Opened 6 years ago Closed 5 years ago

backport upstream bug 726696 to bmo/4.2 to allow use of api keys for authentication

Categories

(bugzilla.mozilla.org :: API, defect)

Production
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dkl, Assigned: dkl)

References

Details

Attachments

(1 file, 3 obsolete files)

SSIA
Attached patch 1045145_1.patch (obsolete) — Splinter Review
Attachment #8463588 - Flags: review?(glob)
Comment on attachment 8463588 [details] [diff] [review]
1045145_1.patch

we should wait for all issues caused by the upstream bug to be fixed before we backport it (current 3 bugs).
Attachment #8463588 - Flags: review?(glob) → review-
Attached patch 1045145_2.patch (obsolete) — Splinter Review
Remaining upstream issues fixed and committed. New patch with those changes.
Attachment #8463588 - Attachment is obsolete: true
Attachment #8486181 - Flags: review?(glob)
we'd like a security review on this before it lands on BMO.
the discussion and upstream implementation of this feature is in bug 726696.

dkl: can you please provide a high level summary of the changes here?
Flags: sec-review?
Flags: needinfo?(dkl)
Dan can you take a look at this?
Flags: sec-review?(dveditz)
Flags: sec-review?
Flags: needinfo?(dveditz)
(In reply to Byron Jones ‹:glob› from comment #4)
> we'd like a security review on this before it lands on BMO.
> the discussion and upstream implementation of this feature is in bug 726696.
> 
> dkl: can you please provide a high level summary of the changes here?

Sure. Should we do this in a separate security review review bug as we have done with other features in the past or keep it here?

Security Assurance Review Request

1. Who is/are the point of contact(s) for this review?

dkl@mozilla.com, glob@mozilla.com

2. Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):

Bugzilla has up until recently allowed cookie authentication over WebService API calls which was determined to not be very secure way of authenticating users. Alternatively users can pass their username and password as part of the request parameters to allow making changes to bugzilla data or to view secure bugs but this is also not ideal in that the information can be visible in logs, etc.

Upstream cookie support for API calls has been removed and eventually we would like to do the same for BMO. A new method of webservice authentication was created upstream using API tokens and API keys.

API tokens are used for embedded methods in Bugzilla pages, such as User.get({match => "foo"}) used for autocompletion. A token will be automatically generated by Bugzilla and included in the list of arguments passed to the method. This token + login cookies will then be used to authenticate the user. Tokens have a limited lifetime, and are therefore more suitable for web pages. As a token is now required to validate login cookies, external scripts using login cookies only to authenticate users will no longer work as they have no way to access tokens (which are generated using the site_wide_secret key stored in localconfig).

For external API calls and if the user doesn't want to pass his credentials (login + password), an API key is required. An API key is bound to a user account and a user can create several API keys so that he can use one distinct key per external application if desired (such as an external dashboard wanting to interact with a Bugzilla installation). Allowing several API keys per user will let the user revoke them on a one-by-one basis in case he has concerns with a given application, without compromising other applications or having to update the API key in all other applications.

There will also be a new tab in the Preferences section to revoke, update the description and create new API keys. When a key is created, we will send the user an e-mail for security reasons. Otherwise an attacker that knew a user’s password could create an API key and remain undetected for a long time.  API keys remain valid forever, or until they are revoked. If a user is disabled, then their API keys will not work.

3. Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:

https://bugzilla.mozilla.org/show_bug.cgi?id=726696

4. Does this request block another bug? If so, please indicate the bug number. 

No

5. This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?

ASAP

6. To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list? If so, which goal?

No

7. Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.)

7.1 Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users? 

It will affect all users of bugzilla.mozilla.org via the web UI and WebServices API.

7.2 Are there any portions of the project that interact with 3rd party services?

No

7.3 Will your application/service collect user data? If so, please describe.

Same data that is normally collected by using bugzilla.mozilla.org

8. If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):

None

9. Desired Date of review (if known from https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html) and whom to invite.

ASAP

dkl
Flags: needinfo?(dkl)
Ping dveditz for sec review. Timeframe?
My review of web authentication wouldn't be all that strong. Yvan, is this something your team (what's left of it) could take on? Or maybe rforbes or mgoodwin would have the bandwidth?
Flags: sec-review?(yboily)
Flags: sec-review?(dveditz)
Flags: needinfo?(yboily)
Flags: needinfo?(dveditz)
I have pinged rforbes and mgoodwin about possibly taking a look at this sec review. Adding them to needinfo.

dkl
Flags: needinfo?(rforbes)
Flags: needinfo?(mgoodwin)
This looks sane.

The only question I have is on token size. Is there any reason these need to be so small? I'd be much happier if the API keys were somewhat larger (they won't change often, and we don't have the concern of users needing to type them). Would you be happy making this change?
Flags: needinfo?(yboily)
Flags: needinfo?(rforbes)
Flags: needinfo?(mgoodwin)
Flags: needinfo?(dkl)
(In reply to Mark Goodwin [:mgoodwin] from comment #10)
> The only question I have is on token size. Is there any reason these need to
> be so small? I'd be much happier if the API keys were somewhat larger (they
> won't change often, and we don't have the concern of users needing to type
> them). Would you be happy making this change?

Do you have any documentation on the best practice for API key management? Or have you seen in past experience that 40 character key length is easily breakable?

Would 48 length be sufficient? changing the key size this late upstream is not trivial as we would need to add some special code to migrate people from 40 to the new size for those who have already installed but not impossible.

dkl
Flags: needinfo?(dkl) → needinfo?(mgoodwin)
(In reply to David Lawrence [:dkl] from comment #11)
> (In reply to Mark Goodwin [:mgoodwin] from comment #10)
> > The only question I have is on token size. Is there any reason these need to
> > be so small? I'd be much happier if the API keys were somewhat larger (they
> > won't change often, and we don't have the concern of users needing to type
> > them). Would you be happy making this change?
> 
> Do you have any documentation on the best practice for API key management?
> Or have you seen in past experience that 40 character key length is easily
> breakable?
> 
> Would 48 length be sufficient? changing the key size this late upstream is
> not trivial as we would need to add some special code to migrate people from
> 40 to the new size for those who have already installed but not impossible.
> 
> dkl

40 is, in fact, fine; I'd missed we were overriding the default token length.
Flags: needinfo?(mgoodwin)
Flags: sec-review?(yboily) → sec-review+
Thanks mgoodwin!

Glob, if you want to push this this week then feel free if the code review checks out. Otherwise we will wait two weeks when you have recovered from the all-hands.

dkl
(In reply to David Lawrence [:dkl] from comment #13)
> Glob, if you want to push this this week then feel free if the code review
> checks out. Otherwise we will wait two weeks when you have recovered from
> the all-hands.

i would prefer to wait until after the all-hands to deploy this - it ensures we are available to catch any potential issues.
Comment on attachment 8486181 [details] [diff] [review]
1045145_2.patch

Review of attachment 8486181 [details] [diff] [review]:
-----------------------------------------------------------------

the patch from bug 1076155 needs to be included here.

i loaded a restricted bug, tried comment tagging, and got:
>  The token 'BVXUEzVlW9' is not valid. It could be because you loaded this page more than 3 days ago
aside from the obvious issue of the broken functionality, there's no reason to display the token to the user in this error message.

it looks like there's changes to Bugzilla::Token::GetTokenData which aren't part of this backport (and possibly other changes).

::: Bugzilla/Token.pm
@@ +60,5 @@
> +    my ($token) = $dbh->selectrow_array("
> +        SELECT token FROM tokens
> +         WHERE userid = ? AND tokentype = 'api_token'
> +               AND (" . $dbh->sql_date_math('issuedate', '+', (MAX_TOKEN_AGE * 24 - 12), 'HOUR') . ") > NOW()",
> +        undef, $user->id);

this is in conflict with CleanTokenTable, which deletes tokens with issuedate > now().
as a result api_tokens don't live as long as expected.

CleanTokenTable needs to be updated to give a longer lifetime to api_token entries.

::: js/bug.js
@@ +142,5 @@
>                  method: 'BugUserLastVisit.update',
> +                params: {
> +                    Bugzilla_api_token: BUGZILLA.api_token,
> +                    ids: bug_id
> +                },

there's a merge conflict here, but it's trivial to fix

::: template/en/default/account/prefs/apikey.html.tmpl
@@ +15,5 @@
> +  API keys are used to authenticate WebService API calls. You can create more than
> +  one API key if required. Each API key has an optional description which can help
> +  you record what each key is used for. Documentation on how to log in is
> +  available from
> +  <a href="[% docs_urlbase FILTER html %]api/Bugzilla/WebService.html#LOGGING_IN">here</a>.

because our docs_urlbase is 4.2, this link won't work.
use the full url, not one based from docs_urlbase

@@ +77,5 @@
> +generated for you.</p>
> +
> +<p>
> +  <input type="checkbox" name="new_key" id="new_key">
> +  Generate a new API key with optional description

this text should be a <label>, so clicking on it checks #new_key

::: userprefs.cgi
@@ +558,5 @@
> +    # Was a new api key requested
> +    if ($cgi->param('new_key')) {
> +        my $new_key = Bugzilla::User::APIKey->create({
> +            user_id     => $user->id,
> +            description => $cgi->param('new_description'),

t/002goodperl.t ...... 1/2232
#   Failed test 'userprefs.cgi incorrectly passes a CGI argument to a hash --ERROR
# description => $cgi->param('new_description'), on line 562'
#   at t/002goodperl.t line 149.
# Looks like you failed 1 test of 2232.
Attachment #8486181 - Flags: review?(glob) → review-
Comment on attachment 8486181 [details] [diff] [review]
1045145_2.patch

Review of attachment 8486181 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/email/new-api-key.txt.tmpl
@@ +12,5 @@
> +  #%]
> +
> +From: [% Param('mailfrom') %]
> +To: [% user.email %]
> +Subject: [% terms.Bugzilla %]: New API key created

$terms hasn't been loaded, so the summary generated is ": New API key created".
Attached patch 1045145_3.patch (obsolete) — Splinter Review
Thanks for the review long ago :) I will probably need to push the CleanTokenTable changes upstream as well. Comment tagging and others work better as well.

dkl
Attachment #8486181 - Attachment is obsolete: true
Attachment #8546766 - Flags: review?(glob)
This is going to be really useful after bug 1118365 lands, so that we can use API keys with MozReview and not have the "I log into Bugzilla with GitHub; how do I log into MozReview?" problem as we do with Persona (the current solution being to use cookies, but it's not terribly obvious).
I'll support this API key auth in my BzDeck app.
Comment on attachment 8546766 [details] [diff] [review]
1045145_3.patch

Review of attachment 8546766 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Token.pm
@@ +255,5 @@
> +                     " - " . $dbh->sql_to_days('issuedate') . " >= ?) " .
> +                    "OR (tokentype = 'api_token' AND " . $dbh->sql_to_days('NOW()') . " - " . 
> +                     $dbh->sql_to_days($dbh->sql_date_math('issuedate', '-', '12', 'HOUR')) .
> +                     " >= ?)",
> +             undef, MAX_TOKEN_AGE, MAX_TOKEN_AGE);

sorry to do this, but i don't totally agree with glob'2014.  i think there's still an issue here, but not what i thought it was.

api tokens are reused up until 12 hours before they expire - this happens in issue_api_token where the number of hours between the issue date and now().

this should mean that CleanTokenTable doesn't need to be touched, as it deletes tokens older than MAX_TOKEN_AGE.  however it uses sql_to_days to perform this math, which (on mysql at least) completely ignores the time component.

ie.
select to_days('2015-01-23 23:59:59') - to_days('2015-01-20 00:00:00') = 3
select to_days('2015-01-24 00:00:00') - to_days('2015-01-20 00:00:00') = 4

this disparity may cause issues, so we should change CleanTokenTable to use HOURS as a unit and delete tokens older than MAX_TOKEN_AGE * 24 hours.

::: js/bug.js
@@ +142,5 @@
>                  method: 'BugUserLastVisit.update',
> +                params: {
> +                    Bugzilla_api_token: BUGZILLA.api_token,
> +                    ids: bug_ids
> +                },

remove trailing ,

@@ +163,5 @@
>                  version: "1.1",
>                  method: 'BugUserLastVisit.get',
> +                params: {
> +                    Bugzilla_api_token: BUGZILLA.api_token
> +                },

remove trailing ,

::: template/en/default/email/new-api-key.txt.tmpl
@@ +12,5 @@
> +  #%]
> +
> +From: [% Param('mailfrom') %]
> +To: [% user.email %]
> +Subject: [% terms.Bugzilla %]: New API key created

terms.Bugzilla is still empty here, resulting in a bad subject for the email.

::: template/en/default/global/user-error.html.tmpl
@@ +246,5 @@
>  
> +  [% ELSIF error == "auth_invalid_token" %]
> +    [% title = 'A token error occurred' %]
> +    The token is not valid. It could be because you loaded this page more than
> +    3 days ago.

the "3" shouldn't be hard coded here, it's the MAX_TOKEN_AGE constant.
Attachment #8546766 - Flags: review?(glob) → review-
Attached patch 1045145_4.patchSplinter Review
Attachment #8546766 - Attachment is obsolete: true
Attachment #8556035 - Flags: review?(glob)
Comment on attachment 8556035 [details] [diff] [review]
1045145_4.patch

Review of attachment 8556035 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob, with issues to be a fixed on commit.

::: Bugzilla/Token.pm
@@ +248,5 @@
>  
>  sub CleanTokenTable {
>      my $dbh = Bugzilla->dbh;
> +    # API tokens should be also be deleted if they are within 12 hours
> +    # of reaching MAX_TOKEN_AGE to be consistent with issue_api_token()

this comment doesn't make sense.  the code doesn't do this, and i don't think it would be correct for it to do so as the end result would be making api tokens live 12 hours less than other tokens.

please remove this comment.

::: template/en/default/account/prefs/apikey.html.tmpl
@@ +15,5 @@
> +  API keys are used to authenticate WebService API calls. You can create more than
> +  one API key if required. Each API key has an optional description which can help
> +  you record what each key is used for. Documentation on how to log in is available from
> +  <a href="https://bugzilla.readthedocs.org/en/latest/using/preferences.html#api-keys">
> +    here</a>.

that link doesn't contain any information about how to log in with an api-key.
we should be linking to https://bugzilla.readthedocs.org/en/latest/api/core/v1/general.html#authentication
Attachment #8556035 - Flags: review?(glob) → review+
Neato!

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   89d3199..c8447e9  master -> master

dkl
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
This is awesome.

Sent from my BzDeck
You need to log in before you can comment on or make changes to this bug.