:glob ✱ Assignee
	Description • 9 years ago

add duo-security as a second 2fa provider, integrating with the existing moco account.

	Sylvie Veilleux [:sylvieV]
	Comment 1 • 9 years ago

Hi- would like to check in to see when we plan to enable Duo and if it will be replacing the current solution or used as an alternative only?

	:glob ✱ Assignee
	Comment 2 • 9 years ago

(In reply to SylvieV from comment #1)
> Hi- would like to check in to see when we plan to enable Duo

i haven't started work on this yet so i'm unable to provide a time frame.  i had to fix up the 2fa implementation first (bug 1199087) and last week was consumed with travel and tribe.  

> and if it will be replacing the current solution or used as an alternative only?

it will be an alternative - users will be able to use TOTP or duo (but not both).

	:glob ✱ Assignee
	Comment 3 • 9 years ago

this is now code complete.  i'm waiting for BMO to be configured under moco's duo account so i can test it with the production config before putting it up for review (i've been testing with my own free duo account).

demo: https://dl.dropboxusercontent.com/u/16292140/duo.mp4

	Dylan Hardison [:dylan] (he/him)
	Comment 5 • 9 years ago

Comment on attachment 8670247 [details] [diff] [review]
1199089_1.patch

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

Missing dependency: DuoWeb

	:glob ✱ Assignee
	Comment 6 • 9 years ago

Comment on attachment 8670247 [details] [diff] [review]
1199089_1.patch

ah, my bz script ignores lib/.  new patch soon.

	:glob ✱ Assignee
	Comment 7 • 9 years ago

moved DuoWeb.pm outside of a gitignored directory

	Dylan Hardison [:dylan] (he/him)
	Comment 8 • 9 years ago

Comment on attachment 8670286 [details] [diff] [review]
1199089_2.patch

You'll need to exclude it from the pod tests too.

t/011pod.t ........... 1/607 
#   Failed test 'DuoWeb.pm has incorrect POD syntax --ERROR'
#   at t/011pod.t line 56.

	:glob ✱ Assignee
	Comment 9 • 9 years ago

DuoWeb.pm is now excluded from tests

	David Lawrence [:dkl]
	Comment 10 • 9 years ago

Comment on attachment 8670341 [details] [diff] [review]
1199089_3.patch

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

1) Nit: Any reason we cannot move DuoWeb.pm to Bugzilla/DuoWeb.pm like we have done with others in the past? I would prefer to keep the document root free of *.pm except for Bugzilla.pm of course.

2) Unable to disable 2FA using printed recovery codes (happens with both TOTP and Duo):
  a) Set up Duo as a MFA provider on account successfully.
  b) Print out a list of recovery codes
  c) Log out
  d) Log in with correct username and password.
  e) Instead of using Duo push, choose to use a recovery code for verification
  f) Choosing first code in list, log in works properly
  g) Go to Preferences -> Two-Factor Authentication -> Disable Two-Factor Authentication
  h) Enter current password, and hit Submit
  i) Again choose to use a recovery code for verification using the next code in the list
  j) get the error "Invalid verification code." Observe that the code just used has also
     been removed from profile_mfa as if it were used. So multiple failures the user
     would run out of recovery codes would need manual intervention in the future.

Other actions work as expected and code changes look good overall.

::: template/en/default/admin/params/auth.html.tmpl
@@ +160,5 @@
> +    "Duo 2FA.",
> +
> +  duo_akey =>
> +    "The 'integration secret key' for Duo 2FA. This is automatically " _
> +    "generated by Bugzilla.",

nit: 

"The 'integration secret key' for Duo 2FA. This is automatically " _
"generated by running checksetup.pl."

	:glob ✱ Assignee
	Comment 11 • 9 years ago

(In reply to David Lawrence [:dkl] from comment #10)
> 1) Nit: Any reason we cannot move DuoWeb.pm to Bugzilla/DuoWeb.pm like we
> have done with others in the past? I would prefer to keep the document root
> free of *.pm except for Bugzilla.pm of course.

it's quite different from the rest of the bugzilla codebase (unlike patchreader, the only module we've inlined), so i don't think it belongs under our namespace.

	:glob ✱ Assignee
	Comment 12 • 9 years ago

- don't check recovery codes twice

	:glob ✱ Assignee
	Comment 13 • 9 years ago

forgot to fix the duo_akey param description

	David Lawrence [:dkl]
	Comment 14 • 9 years ago

Comment on attachment 8672483 [details] [diff] [review]
1199089_5.patch

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

Verified recovery code issue fixed and all other final checks that I checked all checked out. r=dkl

	:glob ✱ Assignee
	Comment 15 • 9 years ago

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   07791e2..d69cebd  master -> master

once pushed, we'll be capable of supporting duo on bmo but it won't be available to users until the duo licenses are sorted out (bug 1210870).