1197073 - add support for 2fa using totp (eg. google authenticator)

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect, P1)

Description

[:glob ✱]

add support for 2fa using totp (eg. google authenticator).

Comment 1

[:glob ✱]

Attachment: 1197073_1.patch

adds preliminary 2fa support using totp:
- adds 'two-factor authentication' tab to preferences
- web-based logins using a 2fa enabled account prompts for a totp code
- adds api_key_only preference.  when enabled api-keys become the only supported mechanism for api authentication
- adds ability for admins (true admins, not just editusers) to disable 2fa of other users
- adds 2fa indicator to group_members report (with a 'weakened' indicator if api_key_only is off)

enrolment:
- supports qr and text-only verification
- requires a valid totp code and the user's current password
- api_key_only preference is automatically enabled when 2fa is enabled

disabling:
- requires a valid totp code and the user's current password

limitations:
- only totp is supported (eg. google auth)
- protection is limited to just login
- (see blockers of bug 1196628 for the full list)

Comment 2

[:glob ✱]

Attachment: totp enrollment

Comment 3

[Guillaume Destuynder [:kang] [this account is no longer active]]

spoke with glob, looked at the code and had a vidyo chat prior to that - will look at it when its deployed on bugzilla-dev and puts potential findings back in this bug

Comment 4

[:glob ✱]

Comment on attachment 8653307 [details] [diff] [review]
1197073_1.patch

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

::: template/en/default/account/prefs/mfa.html.tmpl
@@ +9,5 @@
> +[% IF NOT Bugzilla.feature('mfa') %]
> +  <p>
> +    Two-factor Authentication is not available on your account because you are
> +    using an external authentication provider.
> +  </p>

this message should be the one displayed for the next block.
here it should state that 2fa is not available.

Comment 5

[:glob ✱]

Comment on attachment 8653307 [details] [diff] [review]
1197073_1.patch

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

- entering an invalid OTP during login, clicking back then entering a valid one results in:
  The token you submitted does not exist, has expired, or has been canceled.
  (it's complaining about the login token)

::: template/en/default/mfa/totp.html.tmpl
@@ +12,5 @@
> +
> +<h1>Account Verification</h1>
> +
> +<p>
> +  Please enter your verification code from your TOTP application>:

there's a stray > before the :

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8653307 [details] [diff] [review]
1197073_1.patch

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

It's possible to use this with Persona and GitHubAuth, because profiles.mfa is not populated with the MFA provider. 

r-

::: Bugzilla/Auth.pm
@@ +109,5 @@
> +sub mfa_verified {
> +    my ($self, $user, $type) = @_;
> +    require Bugzilla::Auth::Login::CGI;
> +    $self->{_info_getter}->{successful} = Bugzilla::Auth::Login::CGI->new();
> +    $self->_handle_login_result({ user => $user }, $type);

Probably could just call the persister directly here, fwiw. This is fine though.

::: Bugzilla/DB/Schema.pm
@@ +952,4 @@
>              last_seen_date => {TYPE => 'DATETIME'},
>              password_change_required => { TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE' },
>              password_change_reason   => { TYPE => 'varchar(64)' },
> +            mfa            => {TYPE => 'varchar(8)'},

this field is never populated in my experience.

::: editusers.cgi
@@ +270,5 @@
>              ? $cgi->param('password_change_reason')
>              : ''
>          );
> +        if ($user->in_group('admin')) {
> +            $otherUser->set_mfa('') if $cgi->param('mfa') eq '';

Should this be sent to the audit log?
Email the user when their 2fa is turned off?

::: extensions/GitHubAuth/lib/Login.pm
@@ +164,5 @@
> +        return { failure    => AUTH_ERROR,
> +                 user_error => 'github_auth_account_too_powerful' } if $user->in_group('no-github-auth');
> +        return { failure    => AUTH_ERROR,
> +                 user_error => 'mfa_prevents_login',
> +                 details    => { provider => 'GitHub' } } if $user->mfa;

this does not prevent using GitHubAuth with mfa. It is possible to use github auth *and* have a password. A password is required to setup mfa. But after that, I can login with GitHub. It does, however, ask me for an auth code.

::: template/en/default/account/prefs/mfa.html.tmpl
@@ +9,5 @@
> +[% IF NOT Bugzilla.feature('mfa') %]
> +  <p>
> +    Two-factor Authentication is not available on your account because you are
> +    using an external authentication provider.
> +  </p>

This was indeed confusing when I first tested with mfa disabled.

::: template/en/default/mfa/totp.html.tmpl
@@ +12,5 @@
> +
> +<h1>Account Verification</h1>
> +
> +<p>
> +  Please enter your verification code from your TOTP application>:

Fix this ofc.

::: template/en/default/setup/strings.txt.pl
@@ +105,4 @@
>      feature_new_charts        => 'New Charts',
>      feature_old_charts        => 'Old Charts',
>      feature_memcached         => 'Memcached Support',
> +    feature_mfa               => 'Multi-Factor Authentication',

Two-Factor Authentication might be a better choice, considering what we call it in the rest of the UI.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Addendum: it seems the $user->mfa is false, but the actual content of the DB is not. Probably a simple fix?

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8653307 [details] [diff] [review]
1197073_1.patch

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

Comments:

1. When a user disables their own account, IMO we should also clear out their mfa values as well if on.
2. As Dylan mentioned, token error if you mistakenly type in your password or confirmation code.

Need to break for a bit but will continue with review later tonight.

::: Bugzilla/User.pm
@@ +581,5 @@
> +    my ($self) = @_;
> +    my $mfa = $self->{mfa} || return undef;
> +    if ($mfa eq 'TOTP') {
> +        require Bugzilla::MFA::TOTP;
> +        return Bugzilla::MFA::TOTP->new($self);

Should this not be cached in case $user->mfa_provider is called multiple times?

sub mfa_provider {
    my ($self) = @_;
    return $self->{mfa_provider} if $self->{mfa_provider};
    my $mfa = $self->{mfa} || return undef;
    if ($mfa eq 'TOTP') {
        require Bugzilla::MFA::TOTP;
        return $self->{mfa_provider} = Bugzilla::MFA::TOTP->new($self);

::: skins/standard/admin.css
@@ +259,5 @@
> +    padding: 0 0 0 2em;
> +}
> +
> +#mfa-totp-secret {
> +    margin: 1em 0;

Also make the font bigger as I could barely make out some of the letters on my screen when manually entering on my phone.

::: template/en/default/account/prefs/mfa.html.tmpl
@@ +50,5 @@
> +
> +    <button type="button" id="mfa-enable">Enable Two-factor Authentication</button>
> +
> +    <div id="mfa-enable-container" style="display:none">
> +      <b>System:</b>

nit; I think Provider would be a better word?

@@ +53,5 @@
> +    <div id="mfa-enable-container" style="display:none">
> +      <b>System:</b>
> +      <select name="mfa" id="mfa">
> +        <option value="" selected></option>
> +        <option value="TOTP">Time-based One-Time Password (TOTP)</option>

I know we plan to add additional providers (Duo, etc.) but could we add the selector in later and just default to showing TOTP for now? It is annoying to get a blank selector and have to extra click to choose the one choice :)

@@ +110,5 @@
> +
> +  <div id="mfa-confirm" style="display:none">
> +    <p>
> +      Two-factor authentication settings will not be updated until you provide
> +      your current password and <b>Submit Changes</b>.

nit: and click

::: template/en/default/mfa/totp.html.tmpl
@@ +12,5 @@
> +
> +<h1>Account Verification</h1>
> +
> +<p>
> +  Please enter your verification code from your TOTP application>:

remove trailing >:

::: userprefs.cgi
@@ +142,4 @@
>      }
>  
>      $user->set_name($cgi->param('realname'));
> +    $user->set_mfa($cgi->param('mfa'));

Don't see where this is declared in the account.html.tmpl template.

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 8653307 [details] [diff] [review]
1197073_1.patch

Ok weird. My comment reset dylan's review-. Setting back.

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8653307 [details] [diff] [review]
1197073_1.patch

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

feedback+ for general design and functionality. I was able to verify most things worked as expected except for what is listed in mine and Dylan's prior comments. Once those are fixed I feel this will be ready to go.

Comment 11

[:glob ✱]

thanks all for the very quick reviews.

> Email the user when their 2fa is turned off?

note to self: this is an email triggered when an admin disables 2fa of another user.

> this does not prevent using GitHubAuth with mfa. It is possible to use
> github auth *and* have a password. A password is required to setup mfa. But
> after that, I can login with GitHub. It does, however, ask me for an auth
> code.

so you get github + 2fa.  that isn't a huge deal, but it's unexpected.  i'll see what i can dig up.

> 1. When a user disables their own account, IMO we should also clear out
> their mfa values as well if on.

hrm, i swear i wrote that code, but evidently it got deleted (that's why property_delete_all exists). 

> ::: template/en/default/account/prefs/mfa.html.tmpl
> @@ +50,5 @@
> > +
> > +    <button type="button" id="mfa-enable">Enable Two-factor Authentication</button>
> > +
> > +    <div id="mfa-enable-container" style="display:none">
> > +      <b>System:</b>
> 
> nit; I think Provider would be a better word?

i had provider for a while, but TOTP isn't strictly a _provider_ .. there isn't a 'thing' that does the providing.

> I know we plan to add additional providers (Duo, etc.) but could we add the
> selector in later and just default to showing TOTP for now? It is annoying
> to get a blank selector and have to extra click to choose the one choice :)

i don't want to generate secrets and qr codes until they are requested.  without the select acting as a gate this would be the case.

> ::: userprefs.cgi
> @@ +142,4 @@
> >      }
> >  
> >      $user->set_name($cgi->param('realname'));
> > +    $user->set_mfa($cgi->param('mfa'));
> 
> Don't see where this is declared in the account.html.tmpl template.

account/prefs/mfa.html.tmpl:
> <select name="mfa" id="mfa">
>   <option value="" selected></option>
>   <option value="TOTP">Time-based One-Time Password (TOTP)</option>
> </select>

Comment 12

[:glob ✱]

Attachment: 1197073_2.patch

addresses most of the review comments:
- qr and secret stored in a no-cache iframe
- fixes invalid token after bad verification code
- fixes github auth 2fa blocking
  - this was a bug in the github auth ext - there are two paths to find the user, but only one was performing checks
- adds audit line when an admin disables 2fa
- mfa data removed when 2fa is disabled
- provider object caching
- trivial ui fixes

after chatting with dylan we decided an email isn't a requirement at this stage (due to this being available to true admins only, not all editusers)

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 8653914 [details] [diff] [review]
1197073_2.patch

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

Strange. When enabling TOTP for a user, I get a page with this error in the iframe:

file error - cache failed to write enroll.html.tmpl: Insecure dependency in sysopen while running with -T switch at /usr/share/perl5/vendor_perl/File/Temp.pm line 379. at /usr/share/perl5/vendor_perl/File/Temp.pm line 379. File::Temp::_gettemp('template_cache/template/en/default/mfa/totp/XXXXXXXXXX', 'open', 1, 'mkdir', 0, 'unlink_on_close', 0, 'suffixlen', 0, ...) called at /usr/share/perl5/vendor_perl/File/Temp.pm line 1093 File::Temp::tempfile('DIR', 'template_cache/template/en/default/mfa/totp') called at /usr/local/lib64/perl5/Template/Document.pm line 301 eval {...} called at /usr/local/lib64/perl5/Template/Document.pm line 298 Template::Document::write_perl_file('Template::Document', 'template_cache/template/en/default/mfa/totp/enroll.html.tmpl', 'HASH(0x64fd060)') called at /usr/local/lib64/perl5/Template/Provider.pm line 866 Template::Provider::_compile('Template::Provider=HASH(0x5f9c540)', 'HASH(0x61516d8)', 'template_cache/template/en/default/mfa/totp/enroll.html.tmpl') called at /usr/local/lib64/perl5/Template/Provider.pm line 482 Template::Provider::_fetch('Template::Provider=HASH(0x5f9c540)', 'template/en/default/mfa/totp/enroll.html.tmpl', 'mfa/totp/enroll.html.tmpl') called at /usr/local/lib64/perl5/Template/Provider.pm line 529 Template::Provider::_fetch_path('Template::Provider=HASH(0x5f9c540)', 'mfa/totp/enroll.html.tmpl') called at /usr/local/lib64/perl5/Template/Provider.pm line 156 Template::Provider::fetch('Template::Provider=HASH(0x5f9c540)', 'mfa/totp/enroll.html.tmpl', undef) called at /usr/local/lib64/perl5/Template/Context.pm line 140 Template::Context::template('Bugzilla::Template::Context=HASH(0x6060758)', 'mfa/totp/enroll.html.tmpl') called at /usr/local/lib64/perl5/Template/Service.pm line 68 eval {...} called at /usr/local/lib64/perl5/Template/Service.pm line 68 Template::Service::process('Template::Service=HASH(0x5fc40d0)', 'mfa/totp/enroll.html.tmpl', 'HASH(0x1b50710)') called at /usr/local/lib64/perl5/Template.pm line 66 Template::process('Bugzilla::Template=HASH(0x5fba8b8)', 'mfa/totp/enroll.html.tmpl', 'HASH(0x1b50710)') called at Bugzilla/Template.pm line 674 Bugzilla::Template::process('Bugzilla::Template=HASH(0x5fba8b8)', 'mfa/totp/enroll.html.tmpl', 'HASH(0x1b50710)') called at

/home/bugzilla/devel/htdocs/1197073/userprefs.cgi line 589 main::DoMFA() called at /home/bugzilla/devel/htdocs/1197073/userprefs.cgi line 777

If I remove -T from userprefs.cgi it works fine as expected.

All else works as expected once the issues were worked around.

dkl

::: Bugzilla/MFA/TOTP.pm
@@ +57,5 @@
> +        type  => $params->{type},
> +    };
> +
> +    print Bugzilla->cgi->header();
> +    $template->process('mfa/totp.html.tmpl', $vars)

Get an error when logging in:

file error - mfa/totp.html.tmpl: not found 

Needs to be changed to 'mfa/totp/verify.html.tmpl'

::: template/en/default/account/prefs/prefs.html.tmpl
@@ +76,5 @@
> +      name        => "mfa",
> +      label       => "Two-Factor Authentication",
> +      link        => "userprefs.cgi?tab=mfa",
> +      saveable    => "1"
> +    },

In fear of bikeshedding, IMO it would be better to make this a condition of feature('mfa') instead of always showing the tab even if proper modules are not installed. For example after tabs = []:

  IF Bugzilla.feature('mfa');
    tabs.push({
      name        => "mfa",
      label       => "Two-Factor Authentication",
      link        => "userprefs.cgi?tab=mfa",
      saveable    => "1"
    });
  END;

We can use splice() if you really need it in a specific position.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8653914 [details] [diff] [review]
1197073_2.patch

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

r-. I didn't discover anything beyond what dkl reports. I note that one of the issues is another taint error of dubious value.

::: Bugzilla/MFA/TOTP.pm
@@ +58,5 @@
> +    };
> +
> +    print Bugzilla->cgi->header();
> +    $template->process('mfa/totp.html.tmpl', $vars)
> +        || ThrowTemplateError($template->error());

This file doesn't exist any more. Causes an error.

Comment 15

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #13)
> Strange. When enabling TOTP for a user, I get a page with this error in the
> iframe:

hrm, i can't reproduce this issue and it looks like an issue within template::toolkit, not code i added.
will poke around.

> In fear of bikeshedding, IMO it would be better to make this a condition of
> feature('mfa') instead of always showing the tab even if proper modules are
> not installed.

given the only place these modules won't be available is on dev/test installs i don't think the extra work here is warranted.

Comment 16

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #15)
> (In reply to David Lawrence [:dkl] from comment #13)
> > Strange. When enabling TOTP for a user, I get a page with this error in the
> > iframe:
> 
> hrm, i can't reproduce this issue and it looks like an issue within
> template::toolkit, not code i added.
> will poke around.

ah, because i'm using $provider in the path.

Comment 17

[:glob ✱]

Attachment: 1197073_3.patch

- fix taint error (i was never able to reproduce it, but the fix is obvious)
- fix bad template name

Comment 18

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8654715 [details] [diff] [review]
1197073_3.patch

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

r=dylan

I've run out of scenarios to test, and this seems to work reliably. ship it. :-)

Comment 19

[Guillaume Destuynder [:kang] [this account is no longer active]]

tested with :glob over IRC conversation mainly (I thought I posted to this bug with details, but apparently I did not, sorry).
Apart from UX details we made sure the qr code and text version of the base32 secret seed were invalidated by the browser cache (previously weren't)

Another point is that password recovery ("forgotten password" feature) currently bypasses MFA. Glob mentioned this is worked on separately.

so, r+, looks nice too!

Comment 20

[:glob ✱]

schema only:

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   1bcdb37..fcfcdf2  master -> master

Comment 21

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   bcc93f8..421ff7f  master -> master

Comment 22

[:glob ✱]

fix 2fa being disabled when updating the account tab (old code from when 2fa enrolment was on that tab):

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   20eabc1..8a5c7c7  master -> master

Comment 23

[:glob ✱]

fix invalid reporting of 2fa deactivation when an admin edits a user who already has 2fa disabled:

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   91eac6e..3ac155a  master -> master

Comment 24

[Michal Purzynski [:michal`] (use NEEDINFO)]

Can we have a small photos of the devices, right next to the type on 

https://login-dev1.corpdmz.scl3.mozilla.com/duo_enrollments/

It took me half a minute to remember what-is-what (I have two yubikeys not neo and one duo with yubi neo)

For example

Duo Device - a small photo of the phone with a duo app displayed

HOTP8 / HOTP6

A small photo of a yubikey

Comment 25

[Michal Purzynski [:michal`] (use NEEDINFO)]

Ignore comment 24 it was intended for https://bugzilla.mozilla.org/show_bug.cgi?id=1173553