Bug 28786 - Two-factor authentication for staff client - TOTP
Summary: Two-factor authentication for staff client - TOTP
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords: dependency
: 19887 (view as bug list)
Depends on: 29957 28785 29915
Blocks: 20476 28787 29835 29836 30586 30588 30724 31059 31119 29873 30842 31247
  Show dependency treegraph
 
Reported: 2021-07-30 12:02 UTC by Jonathan Druart
Modified: 2022-09-29 16:44 UTC (History)
16 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/578
Text to go in the release notes:
This new feature adds an initial optional implementation of two-factor authentication (2FA) to improve security when logging into the staff interface. This implementation uses time-based, one-time passwords (TOTP) as the second factor, letting librarians use an application to handle it and provide them the code they need when logging in. It is enabled using the new system preference "TwoFactorAuthentication". Librarians can then enable 2FA for their account from More > Manage Two-Factor authentication. To setup: 1) Scan the QR code with an authenticator app. 2) Enter the one time code generated. For future logins, librarians are prompted to enter the authenticator code after entering their normal login credentials. Any authenticator app, such as Google Authenticator, andOTP, and many others can be used. Applications that enable backup of their 2FA accounts (either cloud-based or automatic) are recommended.
Version(s) released in:
22.05.00


Attachments
Bug 28786: Add new syspref TwoFactorAuthentication (2.95 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.secret (3.37 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.auth_method (3.41 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add controller script and template (10.15 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (25.98 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.59 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: DBIC schema changes (3.44 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new syspref TwoFactorAuthentication (2.99 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.secret (3.42 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.auth_method (3.46 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Add controller script and template (10.19 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (26.01 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.64 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: DBIC schema changes (3.49 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Example UI (108.87 KB, image/png)
2021-09-07 09:40 UTC, Tomás Cohen Arazi
Details
Bug 28786: Two-factor authentication for staff client - TOTP (27.10 KB, patch)
2021-09-10 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.64 KB, patch)
2021-09-10 15:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (27.11 KB, patch)
2021-09-13 20:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.64 KB, patch)
2021-09-13 20:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Correctly inherit from Auth::GoogleAuth (2.61 KB, patch)
2021-09-13 21:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add tests for checkauth (5.12 KB, patch)
2021-09-13 21:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: DO NOT PUSH - DBIx schema changes (3.48 KB, patch)
2021-12-21 14:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style (4.06 KB, patch)
2021-12-21 15:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Fix qa warnings (3.30 KB, patch)
2022-01-13 07:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Fix qa warnings (3.73 KB, patch)
2022-01-13 08:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Don't pass unecessary secret to constructor (1.95 KB, patch)
2022-01-14 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Don't rely on Auth::GoogleAuth to generate the secret (1.07 KB, patch)
2022-01-17 10:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new syspref TwoFactorAuthentication (3.08 KB, patch)
2022-01-18 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.secret (3.51 KB, patch)
2022-01-18 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.auth_method (3.55 KB, patch)
2022-01-18 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Add controller script and template (10.29 KB, patch)
2022-01-18 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (27.19 KB, patch)
2022-01-18 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.73 KB, patch)
2022-01-18 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Correctly inherit from Auth::GoogleAuth (2.70 KB, patch)
2022-01-18 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Add tests for checkauth (5.21 KB, patch)
2022-01-18 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: DO NOT PUSH - DBIx schema changes (3.53 KB, patch)
2022-01-18 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style (4.12 KB, patch)
2022-01-18 12:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Fix qa warnings (3.74 KB, patch)
2022-01-18 12:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Do not call generate_secret32 (1.93 KB, patch)
2022-01-18 12:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Check the returned CSRF token (2.00 KB, patch)
2022-01-18 12:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (27.14 KB, patch)
2022-01-20 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.43 KB, patch)
2022-01-20 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Correctly inherit from Auth::GoogleAuth (2.67 KB, patch)
2022-01-20 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add tests for checkauth (5.17 KB, patch)
2022-01-20 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: DO NOT PUSH - DBIx schema changes (3.50 KB, patch)
2022-01-20 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style (4.09 KB, patch)
2022-01-20 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Fix qa warnings (3.70 KB, patch)
2022-01-20 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Do not call generate_secret32 (1.89 KB, patch)
2022-01-20 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Check the returned CSRF token (1.96 KB, patch)
2022-01-20 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Don't expose secret and auth_method to the REST API (771 bytes, patch)
2022-01-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (28.05 KB, patch)
2022-03-21 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.38 KB, patch)
2022-03-21 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Correctly inherit from Auth::GoogleAuth (2.67 KB, patch)
2022-03-21 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add tests for checkauth (5.17 KB, patch)
2022-03-21 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style (4.09 KB, patch)
2022-03-21 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Fix qa warnings (3.70 KB, patch)
2022-03-21 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Do not call generate_secret32 (1.89 KB, patch)
2022-03-21 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Check the returned CSRF token (1.96 KB, patch)
2022-03-21 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Don't expose secret and auth_method to the REST API (771 bytes, patch)
2022-03-21 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (27.31 KB, patch)
2022-03-21 10:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.38 KB, patch)
2022-03-21 10:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Correctly inherit from Auth::GoogleAuth (2.67 KB, patch)
2022-03-21 10:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add tests for checkauth (5.17 KB, patch)
2022-03-21 10:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style (4.09 KB, patch)
2022-03-21 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Fix qa warnings (3.70 KB, patch)
2022-03-21 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Do not call generate_secret32 (1.89 KB, patch)
2022-03-21 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Check the returned CSRF token (1.96 KB, patch)
2022-03-21 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Don't expose secret and auth_method to the REST API (771 bytes, patch)
2022-03-21 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: DBIC schema (4.70 KB, patch)
2022-03-21 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new syspref TwoFactorAuthentication (3.05 KB, patch)
2022-03-24 13:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.secret (3.48 KB, patch)
2022-03-24 13:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.auth_method (3.52 KB, patch)
2022-03-24 13:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add controller script and template (10.26 KB, patch)
2022-03-24 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (27.26 KB, patch)
2022-03-24 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.80 KB, patch)
2022-03-24 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Correctly inherit from Auth::GoogleAuth (2.67 KB, patch)
2022-03-24 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add tests for checkauth (5.17 KB, patch)
2022-03-24 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style (4.09 KB, patch)
2022-03-24 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Fix qa warnings (3.70 KB, patch)
2022-03-24 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Do not call generate_secret32 (1.89 KB, patch)
2022-03-24 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (QA follow-up) Check the returned CSRF token (1.96 KB, patch)
2022-03-24 13:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Don't expose secret and auth_method to the REST API (771 bytes, patch)
2022-03-24 13:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: DBIC schema changes (3.40 KB, patch)
2022-03-24 13:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: (follow-up) Improve style of 2FA code input (3.40 KB, patch)
2022-04-12 16:41 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28786: add Auth::GoogleAuth module to cpanfile (669 bytes, patch)
2022-04-21 04:22 UTC, Mason James
Details | Diff | Splinter Review
decrease Auth::GoogleAuth version to 1.02 (716 bytes, patch)
2022-04-21 07:22 UTC, Mason James
Details | Diff | Splinter Review
Bug 28786: Don't set auth_method to NULL in ->anonymize (1.27 KB, patch)
2022-04-21 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Don't set auth_method to NULL in ->anonymize (1.36 KB, patch)
2022-04-21 09:50 UTC, Mason James
Details | Diff | Splinter Review
Bug 28786: Fix Patrons/Import.t (1.02 KB, patch)
2022-04-21 11:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Fix Patrons/Import.t (1.12 KB, patch)
2022-04-21 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28786: Fix 2FA selenium tests (1.96 KB, patch)
2022-04-22 08:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Fix 2FA selenium tests (2.02 KB, patch)
2022-04-22 14:40 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-07-30 12:02:36 UTC
This enhancement will provide a first step for Two-factor authentication (2FA).

This implementation will use time-based, one-time passwords (TOTP) as the second factor, letting the librarians use an application to handle it and provide them the code they need to provide at a second login step.

The goal is to let librarians choose if then want to enable the feature, not to force them.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password
Comment 1 Jonathan Druart 2021-08-02 13:14:15 UTC
Created attachment 123375 [details] [review]
Bug 28786: Add new syspref TwoFactorAuthentication

Sponsored-by: Orex Digital
Comment 2 Jonathan Druart 2021-08-02 13:14:19 UTC
Created attachment 123376 [details] [review]
Bug 28786: Add new DB column borrowers.secret

Sponsored-by: Orex Digital
Comment 3 Jonathan Druart 2021-08-02 13:14:22 UTC
Created attachment 123377 [details] [review]
Bug 28786: Add new DB column borrowers.auth_method

Sponsored-by: Orex Digital
Comment 4 Jonathan Druart 2021-08-02 13:14:26 UTC
Created attachment 123378 [details] [review]
Bug 28786: Add controller script and template

Sponsored-by: Orex Digital
Comment 5 Jonathan Druart 2021-08-02 13:14:30 UTC
Created attachment 123379 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital
Comment 6 Jonathan Druart 2021-08-02 13:14:34 UTC
Created attachment 123380 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital
Comment 7 Jonathan Druart 2021-08-02 13:14:38 UTC
Created attachment 123381 [details] [review]
Bug 28786: DBIC schema changes

Sponsored-by: Orex Digital
Comment 8 David Nind 2021-08-02 19:53:05 UTC
Created attachment 123397 [details] [review]
Bug 28786: Add new syspref TwoFactorAuthentication

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2021-08-02 19:53:09 UTC
Created attachment 123398 [details] [review]
Bug 28786: Add new DB column borrowers.secret

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2021-08-02 19:53:14 UTC
Created attachment 123399 [details] [review]
Bug 28786: Add new DB column borrowers.auth_method

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2021-08-02 19:53:20 UTC
Created attachment 123400 [details] [review]
Bug 28786: Add controller script and template

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2021-08-02 19:53:25 UTC
Created attachment 123401 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2021-08-02 19:53:30 UTC
Created attachment 123402 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2021-08-02 19:53:35 UTC
Created attachment 123403 [details] [review]
Bug 28786: DBIC schema changes

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 David Nind 2021-08-02 19:58:12 UTC
Testing notes (koha-testing-docker):
- Tested with Google Authenticator app
- Tested with andOTP app (https://github.com/andOTP/andOTP)
- Everything work as per the test plan
Comment 16 Tomás Cohen Arazi 2021-09-03 14:27:09 UTC
Can we add a new table for the 2FA secrets please?
Comment 17 Jonathan Druart 2021-09-06 09:19:00 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> Can we add a new table for the 2FA secrets please?

Why?
Adding it means that the code needs to be adjusted as well, to deal with 1-N relationship at DB level. IMO the DB structure should reflect the UI. If we need it later it will be easy to move it to a separate table.
Comment 18 Tomás Cohen Arazi 2021-09-06 12:16:46 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Tomás Cohen Arazi from comment #16)
> > Can we add a new table for the 2FA secrets please?
> 
> Why?
> Adding it means that the code needs to be adjusted as well, to deal with 1-N
> relationship at DB level. IMO the DB structure should reflect the UI. If we
> need it later it will be easy to move it to a separate table.

2FA secrets (and the same for overdrive tokens, etc) are not part of the 'patron' entity. Changing the model like this to avoid a new table has another side effects: you will need to remove it in Koha::Patron::to_api_mapping, and if we want to add routes for dealing with them, the code will look weird.
Comment 19 Jonathan Druart 2021-09-07 09:33:15 UTC
Are you suggesting a one-column table with a list of secrets or a ('secret', 'auth_type') that would contain ('my_secret_passphrase', 'totp') with a uniq on auth_type?
Comment 20 Tomás Cohen Arazi 2021-09-07 09:40:11 UTC
Created attachment 124569 [details]
Example UI

I'm not sure the 'app' should be only one or not, I guess the answer is yes. But  I like Github's page, and it points us towards this 'several 2FA methods' thinking.
Comment 21 Jonathan Druart 2021-09-08 09:56:33 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> Created attachment 124569 [details]
> Example UI
> 
> I'm not sure the 'app' should be only one or not, I guess the answer is yes.
> But  I like Github's page, and it points us towards this 'several 2FA
> methods' thinking.

The "Security keys" is not implemented in this development, I can understand the 1-N you would need in this case (you can have several keys), but not with the TOTP. You have only one TOTP secret that you could regenerate, but that will invalidate the existing one. How is the secret different than the password? Are you suggesting we should have the password in this new table?
Comment 24 David Cook 2021-09-09 02:16:50 UTC
If we look at Red Hat's FOSS Identity Management system Keycloak, we can see that they do store credentials separately to the user_entity table.

https://www.keycloak.org/docs/latest/server_admin/#_user-credentials

http://htmlpreview.github.io/?https://gist.githubusercontent.com/thomasdarimont/b1c19da5e8df747b8596e6ddcda7e36f/raw/29309467f4ea07519cf614fd74943272e7d939f4/keycloak_db_overview_4.0.0.CR1-SNAPSHOT.svg

They store both passwords and one-time passwords using that table.

I don't know about Keycloak for sure, but it's common for applications to store multiple OTPs to give users a margin of error. For instance, with AWS and Google Authenticator, you can use the current code or the past 1-2 codes I believe. Using a separate credential table would make that easier.
Comment 25 David Cook 2021-09-09 04:15:26 UTC
Note having a credential table could also make it easier to add "temporary" passwords. 

For instance, it would be great if staff could reset a password, tell the patron the password, and then the patron would be forced to reset that password on first login. I'm going to raise a ticket for that and link it to here...
Comment 26 David Cook 2021-09-09 04:18:23 UTC
Note that Keycloak db schema view is very old. It's just the first visualization I could find while I was quickly looking.
Comment 27 Martin Renvoize 2021-09-09 13:57:46 UTC
I've finally found a moment to look at this.

Whilst I like Tomas's proposal to normalise the database by having a distinct table.. I think that can certainly come as a followup later.

I do wonder why you've chosen a super new cpan module for this, Auth::GoogleAuth. I had considered Authen::OAth myself, because although it's a little smaller (you'de need something to generate QR codes on top), it's further up the CPAN river and is written by a trusted author... might even already have a debian package.

Finally, and what I would consider a QA failure.. the secret is stored in plain text in the database. I feel this should be stored encrypted using the a passphrase stored in the config file (we could re-use api_secret_passphrase or add a new field to the config.
Comment 28 Jonathan Druart 2021-09-10 09:08:58 UTC
(In reply to Martin Renvoize from comment #27)
> I've finally found a moment to look at this.
> 
> Whilst I like Tomas's proposal to normalise the database by having a
> distinct table.. I think that can certainly come as a followup later.

If we are talking about 3NF, I don't think having a separate table with a single column (different than the FK) is what can be called normalisation.

If that is what you have in mind, I think it's wrong:
borrowers < 1-1 > secrets
                  - borrowernumber (FK borrowers.borrowernumber)
                  - secret


> I do wonder why you've chosen a super new cpan module for this,
> Auth::GoogleAuth. I had considered Authen::OAth myself, because although
> it's a little smaller (you'de need something to generate QR codes on top),
> it's further up the CPAN river and is written by a trusted author... might
> even already have a debian package.

I've compared them all.
There is a recent version of Auth::GoogleAuth, which show it's still an active project:
https://metacpan.org/pod/Auth::GoogleAuth
1.03      2021-01-10
1.02      2017-12-26
1.01      2015-08-03
1.00      2015-08-03

Note that 1.03 is only in bullseye.

https://metacpan.org/pod/Authen::OATH
2.0.1     2017-04-12
1.0.0     2010-05-25

The author is active in different other projects: https://metacpan.org/author/GRYPHON

Also, it can generate QR codes out-of-the-box.

> Finally, and what I would consider a QA failure.. the secret is stored in
> plain text in the database. I feel this should be stored encrypted using the
> a passphrase stored in the config file (we could re-use
> api_secret_passphrase or add a new field to the config.

How would you do that?
You encrypt when you don't want to know the secret, and you will never know it again.
Here we need the secret (and not one of its hashed version) as it's an external module that is doing the comparison job. We could eventually generate a new secret containing borrowers.secret, like we concatenate bcrypt_settings (or part of it) with borrowers.secret. That could work but feel weird.
Comment 29 Martin Renvoize 2021-09-10 10:46:39 UTC
Not all encryption is one way ;). I was suggesting for these fields we use encrypt/decrypt where we have the key stored outside of the database, but on the server.  That way, we can decrypt the secret as needed on the server to validate the top. But the key secret itself can't easily be leaked via reports.
Comment 30 Jonathan Druart 2021-09-10 15:16:18 UTC
This will be done on bug 28998.
Comment 31 Jonathan Druart 2021-09-10 15:29:54 UTC
Created attachment 124770 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 32 Jonathan Druart 2021-09-10 15:30:14 UTC
Created attachment 124771 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 33 Martin Renvoize 2021-09-10 16:05:12 UTC
QA looking here.

So far I'm reasonably happy. I think I would have prefered more isolation of the verification step myself, but I can follow on with that in a follow-up bug.

So, personally, I would pass around a 'varified' state linked to the session (as you do I believe). Then, for any get_template_and_user calls I'd have checked the verification status and redirected to a self-contained verification controller for the MFA check... rather than folding the check into Auth.pm and the login pages themselves.  In this way you open up the option to invalidate the verification without invalidating the session entirely for things like patron modification for example (when we add this to the opac.. I can see it being most helpful to not require the verification step at first login but rather upon taking higher privilege actions).

Anywho.. I'll continue down the QA route but wanted to flag it in case you had any feedback as to why you took this particular route rather than any others?
Comment 34 Marcel de Rooy 2021-09-13 09:31:43 UTC
+package Koha::Auth::TwoFactorAuth;
+=head1 NAME
+
+Koha::Encryption - Koha class to encrypt or decrypt strings

Hmm
Comment 35 Marcel de Rooy 2021-09-13 09:44:19 UTC
Template process failed: undef error - The method Koha::Patron->auth_method is not covered by tests!
Comment 36 Marcel de Rooy 2021-09-13 09:46:16 UTC
(In reply to Marcel de Rooy from comment #35)
> Template process failed: undef error - The method Koha::Patron->auth_method
> is not covered by tests!

No DBIx schema change.
Comment 37 Marcel de Rooy 2021-09-13 12:05:56 UTC
Not a complete QA, but at least some remarks:

Nice development! Would like to see 2FA in Koha.

There was discussion about moving the secret to another table. I tend to follow Tomas here. Two factor authentication now only includes TOTP, but we could extend that. If we have several methods, they would (probably) have their own secrets. So yes a separate table would be better.

In terms of security I wonder if we should let the user choose to enable 2FA. If the library switches 2FA on, I would opt for enforcing it. How would you let a user register at that point? Might be that you need some verification mail mechanism here to allow access to the register page exposing the shared key (QR).

As for code, Koha/Auth/TwoFactorAuth.pm should be a folder or base class. And the TOTP code should move deeper then?
There is a Selenium test, but not a regular one?

The "Improve readability" patch triggers this remark ;) The code in C4::Auth is very essential, but already a pain. The maintenance of it by adding the 2FA will be even harder. No one volunteers to rewrite it, but wouldnt this be a great opportunity? Just hoping.. The current changes with a nice "ugly trick" are not the greatest base for confidence.
Comment 38 Jonathan Druart 2021-09-13 20:03:59 UTC
Martin and Marcel, thank you for your interest in this patch set!

As you noted, C4::Auth is not an easy place, and we all know that. It was hard to write code that followed the different "check auth" methods, I reworked the code to make the code isolated enough to be located in only one place (checkauth). And I wrote it, it was the best solution I found without having to rewrite more parts.

There are more to implement but, as said in the commit message, the idea was to provide a first step, without reworking the whole C4::Auth code.

I would like to get more funding to work on follow-up bugs, but as of today I have already exploded the time I could dedicate to this bug.

(In reply to Martin Renvoize from comment #33)
> So, personally, I would pass around a 'varified' state linked to the session
> (as you do I believe). Then, for any get_template_and_user calls I'd have
> checked the verification status and redirected to a self-contained
> verification controller for the MFA check... rather than folding the check
> into Auth.pm and the login pages themselves.  In this way you open up the
> option to invalidate the verification without invalidating the session
> entirely for things like patron modification for example (when we add this
> to the opac.. I can see it being most helpful to not require the
> verification step at first login but rather upon taking higher privilege
> actions).
> 
> Anywho.. I'll continue down the QA route but wanted to flag it in case you
> had any feedback as to why you took this particular route rather than any
> others?

I don't follow what you suggest. A check in get_template_and_user is not enough, we need to catch other auth calls as well.

(In reply to Marcel de Rooy from comment #37)
> There was discussion about moving the secret to another table. I tend to
> follow Tomas here. Two factor authentication now only includes TOTP, but we
> could extend that. If we have several methods, they would (probably) have
> their own secrets. So yes a separate table would be better.

IMO it's out of the scope. That would put this bug in a dead-end situation.

> In terms of security I wonder if we should let the user choose to enable
> 2FA. If the library switches 2FA on, I would opt for enforcing it. How would
> you let a user register at that point? Might be that you need some
> verification mail mechanism here to allow access to the register page
> exposing the shared key (QR).

It's part of the enhancement I suggested in the commit message :)
"* Force 2FA for librarians"

> As for code, Koha/Auth/TwoFactorAuth.pm should be a folder or base class.
> And the TOTP code should move deeper then?

It's a first step, no need to make it more complex. We can still create the base class we will need it.

> There is a Selenium test, but not a regular one?

What tests would make you happy? I could add tests for Koha::Auth::TwoFactorAuth but it's based on Auth::GoogleAuth and only overwrite the constructor.
Or are you asking for C4::Auth::checkauth tests?

With selenium tests we are testing the whole workflow, they are robust and I am very happy with them as they helped me a lot during the development process.

> The "Improve readability" patch triggers this remark ;) The code in C4::Auth
> is very essential, but already a pain. The maintenance of it by adding the
> 2FA will be even harder. No one volunteers to rewrite it, but wouldnt this
> be a great opportunity? Just hoping.. The current changes with a nice "ugly
> trick" are not the greatest base for confidence.

I am open to suggestions to improve the code I've added, and isolate more the code. But I spent a lot of time and tried different approaches before ending with that one.
Comment 39 Jonathan Druart 2021-09-13 20:06:25 UTC
Created attachment 124844 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 Jonathan Druart 2021-09-13 20:06:29 UTC
Created attachment 124845 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 41 Jonathan Druart 2021-09-13 20:06:46 UTC
(In reply to Marcel de Rooy from comment #34)
> +package Koha::Auth::TwoFactorAuth;
> +=head1 NAME
> +
> +Koha::Encryption - Koha class to encrypt or decrypt strings
> 
> Hmm

Oops, fixed!
Comment 42 Jonathan Druart 2021-09-13 21:21:07 UTC
Created attachment 124846 [details] [review]
Bug 28786: Correctly inherit from Auth::GoogleAuth
Comment 43 Jonathan Druart 2021-09-13 21:21:14 UTC
Created attachment 124847 [details] [review]
Bug 28786: Add tests for checkauth
Comment 44 Katrin Fischer 2021-10-21 19:50:54 UTC
I am sure I am missing something obvious, but:

sudo apt-get install libauth-googleauth-perl
Reading package lists... Done
Building dependency tree       
Reading state information... Done
E: Unable to locate package libauth-googleauth-perl

:(
Comment 45 David Cook 2021-10-21 22:13:39 UTC
(In reply to Katrin Fischer from comment #44)
> I am sure I am missing something obvious, but:
> 
> sudo apt-get install libauth-googleauth-perl
> Reading package lists... Done
> Building dependency tree       
> Reading state information... Done
> E: Unable to locate package libauth-googleauth-perl
> 
> :(

Looks like it was added in Buster so maybe you're using an older OS?

https://packages.debian.org/buster/libauth-googleauth-perl
Comment 46 Jonathan Druart 2021-10-22 07:29:55 UTC
Indeed, it's not in D9
"The current LTS version is Debian 9 stretch and will be supported from 2020-07-06 to 2022-06-30"

My bad I was sure I checked the availability of the package.

So, some options:
- wait one more release to push (22.05 released one month before EOL of D9 and we drop D9 support)
- Try to package it for D9 (Mason, how ease would it be?)
- Flag the module has optional and replace use with require
Comment 47 Katrin Fischer 2021-10-22 07:32:06 UTC
I'd be ok with 2 or 3.

I am not sure I can easily update my kohadevbox to a newer Debian - if someone can help me get the module sorted, I am happy to have another go at this.
Comment 48 Jonathan Druart 2021-10-22 07:52:24 UTC
(In reply to Katrin Fischer from comment #47)
> I'd be ok with 2 or 3.
> 
> I am not sure I can easily update my kohadevbox to a newer Debian - if
> someone can help me get the module sorted, I am happy to have another go at
> this.

Install via CPAN.
Comment 49 Mason James 2021-10-22 08:18:52 UTC
(In reply to Jonathan Druart from comment #46)
> Indeed, it's not in D9
> "The current LTS version is Debian 9 stretch and will be supported from
> 2020-07-06 to 2022-06-30"
> 
> My bad I was sure I checked the availability of the package.
> 
> So, some options:
> - wait one more release to push (22.05 released one month before EOL of D9
> and we drop D9 support)
> - Try to package it for D9 (Mason, how ease would it be?)
> - Flag the module has optional and replace use with require

i've added libauth-googleauth-perl to the kc.org staging repo, for testing

root@kohadevbox:/kohadevbox#  apt-cache policy libauth-googleauth-perl
libauth-googleauth-perl:
  Installed: 1.03-1
  Candidate: 1.03-1
  Version table:
 *** 1.03-1 500
        500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages
        100 /var/lib/dpkg/status

root@kohadevbox:/kohadevbox# lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description:    Debian GNU/Linux 9.13 (stretch)
Release:        9.13
Codename:       stretch
Comment 50 Marcel de Rooy 2021-12-21 14:56:47 UTC
Created attachment 128841 [details] [review]
Bug 28786: DO NOT PUSH - DBIx schema changes

Added for testing purposes.
Comment 51 Marcel de Rooy 2021-12-21 15:12:42 UTC
Created attachment 128842 [details] [review]
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style

Test plan:
Run dbrev again.
Comment 52 Marcel de Rooy 2022-01-12 15:17:03 UTC
Starting QA here
Comment 53 Marcel de Rooy 2022-01-13 07:57:55 UTC Comment hidden (obsolete)
Comment 54 Marcel de Rooy 2022-01-13 08:02:43 UTC
Created attachment 129384 [details] [review]
Bug 28786: (QA follow-up) Fix qa warnings

FAIL   members/two_factor_auth.pl
   FAIL   file permissions
                File must have the exec flag
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/members/two_factor_auth.tt
   FAIL   filters
                missing_filter at line 42 (                        <p>Account: [% issuer %]</p>)
                missing_filter at line 43 (                        <p>Key: [% key_id %]</p>)
                missing_filter at line 54 (                            <input type="hidden" name="secret32" value="[% secret32 %]" />)
                missing_filter at line 58 (                                    <img id="qr_code" src="[% qr_code_url %]" />)
 FAIL   Koha/Auth/TwoFactorAuth.pm
   FAIL   pod coverage
                POD is missing for 'new'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Marcel de Rooy 2022-01-13 08:04:02 UTC
Skipping this one (no big deal):
 FAIL   installer/data/mysql/atomicupdate/two-fa.perl
   FAIL   git manipulation
                The file has been added and deleted in the same patchset
Comment 56 Marcel de Rooy 2022-01-13 10:58:32 UTC
There are several issues. Some are blockers, some could be dealt with on follow-up reports, as is already the case for storing the secret.

[1] This code pattern looks wrong:
    my $secret = Koha::AuthUtils::generate_salt( 'weak', 16 );
    my $auth = Koha::Auth::TwoFactorAuth->new( { patron => $logged_in_user, secret => $secret } );
    my $secret32 = $auth->generate_secret32;
What you do with generate_secret32 is effectively wasting your own secret.
Note that Auth::GoogleAuth uses 16 random numbers 0-31 to create a new secret.
If you pass a secret, GoogleAuth will convert it to base32 internally.
The synopsis in the module is wrong too.
This is a blocker. But simple to correct.

[2] The randomizer (which we should not use) depends on Math::Random::MT.
This module says: This algorithm has a very uniform distribution and is good for modelling purposes but do not use it for cryptography.
As long as we pass our own stronger secrets, no worries. See [1].

[3] Martin refers earlier to Authen::OATH. I follow him there, but no blocker.

[4] The argument used before "Also, it can generate QR codes out-of-the-box" is currently actually an issue. It creates a link like https://chart.googleapis.com/chart?&cht=qr with the secret as a URL GET parameter. That's obviously unsafe!
Furthermore, this Google API has been deprecated for some years already.
https://developers.google.com/chart/infographics/docs/qr_codes

We could use the $return_otpauth parameter to create the QR code ourselves.
Personally, I would not use this in its current form. But I would propose to solve this issue on a follow-up report.

[5] This one I did not yet fully tackle. But I have the impression that we have an issue in C4/Auth now. A few times already I had to login twice.
This MIGHT be a blocker. Needs still investigation.
Comment 57 Marcel de Rooy 2022-01-13 12:33:04 UTC
Opened bug 29873
Comment 58 Jonathan Druart 2022-01-14 12:08:00 UTC
(In reply to Marcel de Rooy from comment #56)
> There are several issues. Some are blockers, some could be dealt with on
> follow-up reports, as is already the case for storing the secret.
> 
> [1] This code pattern looks wrong:
>     my $secret = Koha::AuthUtils::generate_salt( 'weak', 16 );
>     my $auth = Koha::Auth::TwoFactorAuth->new( { patron => $logged_in_user,
> secret => $secret } );
>     my $secret32 = $auth->generate_secret32;
> What you do with generate_secret32 is effectively wasting your own secret.
> Note that Auth::GoogleAuth uses 16 random numbers 0-31 to create a new
> secret.
> If you pass a secret, GoogleAuth will convert it to base32 internally.
> The synopsis in the module is wrong too.
> This is a blocker. But simple to correct.

Indeed, good catch. I think it's a leftover of a first version.

> [2] The randomizer (which we should not use) depends on Math::Random::MT.
> This module says: This algorithm has a very uniform distribution and is good
> for modelling purposes but do not use it for cryptography.
> As long as we pass our own stronger secrets, no worries. See [1].

Sorry, you lost me here. Which randomizer module?

> [3] Martin refers earlier to Authen::OATH. I follow him there, but no
> blocker.

Why? Did you see my answer on comment 23?
On the other hand if we do not longer use the QR code generator from Auth::GoogleAuth there is no bad reason to not switch. But what are your good reasons to switch then? :)

> [4] The argument used before "Also, it can generate QR codes out-of-the-box"
> is currently actually an issue. It creates a link like
> https://chart.googleapis.com/chart?&cht=qr with the secret as a URL GET
> parameter. That's obviously unsafe!
> Furthermore, this Google API has been deprecated for some years already.
> https://developers.google.com/chart/infographics/docs/qr_codes
> 
> We could use the $return_otpauth parameter to create the QR code ourselves.
> Personally, I would not use this in its current form. But I would propose to
> solve this issue on a follow-up report.

Agreed, I will be happy to test it.

> [5] This one I did not yet fully tackle. But I have the impression that we
> have an issue in C4/Auth now. A few times already I had to login twice.
> This MIGHT be a blocker. Needs still investigation.

Hum, I didn't experience it.
Comment 59 Jonathan Druart 2022-01-14 12:14:26 UTC
Created attachment 129474 [details] [review]
Bug 28786: Don't pass unecessary secret to constructor

This is a leftover what a misunderstanding I had at first reading the module's pod,
generate_secret32 does not generate the base-32 version of the secret we passed in
parameter to the constructor.
Comment 60 Marcel de Rooy 2022-01-17 07:51:33 UTC
(In reply to Jonathan Druart from comment #58)

> > [2] The randomizer (which we should not use) depends on Math::Random::MT.
> > This module says: This algorithm has a very uniform distribution and is good
> > for modelling purposes but do not use it for cryptography.
> > As long as we pass our own stronger secrets, no worries. See [1].
> 
> Sorry, you lost me here. Which randomizer module?

See Auth::GoogleAuth:
use Math::Random::MT 'rand';
sub generate_secret32
[...]
return $self->secret32( join( '', @chars[ map { rand( scalar(@chars) ) } 1 .. 16 ] ) );
Please reread earlier comment.

> > [3] Martin refers earlier to Authen::OATH. I follow him there, but no
> > blocker.
> 
> Why? Did you see my answer on comment 23?
> On the other hand if we do not longer use the QR code generator from
> Auth::GoogleAuth there is no bad reason to not switch. But what are your
> good reasons to switch then? :)

Martin mentions them already. I could only add: the fact that it potentially exposes the secret in a GET parameter, could be reason enough?

> > [5] This one I did not yet fully tackle. But I have the impression that we
> > have an issue in C4/Auth now. A few times already I had to login twice.
> > This MIGHT be a blocker. Needs still investigation.
> 
> Hum, I didn't experience it.

I will be testing it still a bit. Will report later.
Comment 61 Marcel de Rooy 2022-01-17 07:55:00 UTC
-my $secret = Koha::AuthUtils::generate_salt( 'weak', 16 );
-my $auth = Koha::Auth::TwoFactorAuth->new(
-    { patron => $patron, secret => $secret } );
+my $auth = Koha::Auth::TwoFactorAuth->new({ patron => $patron });
 my $secret32 = $auth->generate_secret32;
 my $ok = $auth->verify($pin_code, 1, $secret32);

Actually, this is not what I meant.
You let the weak randomizer of Auth::GoogleAuth create a secret by calling generate_secret32.
I had in mind to just remove that call. In the assumption that we pass our own secret (hopefully better) from generate_salt?
Comment 62 Jonathan Druart 2022-01-17 10:10:06 UTC
(In reply to Marcel de Rooy from comment #60)
> (In reply to Jonathan Druart from comment #58)
> 
> > > [2] The randomizer (which we should not use) depends on Math::Random::MT.
> > > This module says: This algorithm has a very uniform distribution and is good
> > > for modelling purposes but do not use it for cryptography.
> > > As long as we pass our own stronger secrets, no worries. See [1].
> > 
> > Sorry, you lost me here. Which randomizer module?
> 
> See Auth::GoogleAuth:
> use Math::Random::MT 'rand';
> sub generate_secret32
> [...]
> return $self->secret32( join( '', @chars[ map { rand( scalar(@chars) ) } 1
> .. 16 ] ) );
> Please reread earlier comment.

Not sure this is valid, but I will provide a patch anyway.

> > > [3] Martin refers earlier to Authen::OATH. I follow him there, but no
> > > blocker.
> > 
> > Why? Did you see my answer on comment 23?
> > On the other hand if we do not longer use the QR code generator from
> > Auth::GoogleAuth there is no bad reason to not switch. But what are your
> > good reasons to switch then? :)
> 
> Martin mentions them already. I could only add: the fact that it potentially
> exposes the secret in a GET parameter, could be reason enough?

I've answered Martin's concerns, they were not valid IMO.
I am not willing to rewrite the base of the feature here.
Comment 63 Jonathan Druart 2022-01-17 10:10:41 UTC
Created attachment 129521 [details] [review]
Bug 28786: Don't rely on Auth::GoogleAuth to generate the secret
Comment 64 Marcel de Rooy 2022-01-17 13:36:22 UTC
Too bad that this does not work too with Microsoft Authenticator:

https://social.technet.microsoft.com/Forums/en-US/17f742d7-5596-443e-ac1b-0850c393df5e/add-otpauth-protocol-support-to-microsoft-authenticator?forum=MicrosoftAuthenticatorApp

Although widely used!
Comment 65 Marcel de Rooy 2022-01-17 13:58:01 UTC
+    my $require_2FA = ( C4::Context->preference('TwoFactorAuthentication') && $type ne "OPAC" ) ? 1 : 0;

Seems okay, but just intuitively had some doubts if we block too much with $type not equal.
Comment 66 Marcel de Rooy 2022-01-17 14:03:09 UTC
Well, I am still seeing some things that I would like to change. But I cant justify them with our coding guidelines.
So, I will probably pass QA shortly. Will add a follow-up to replace the last two patches.
And experiment a bit further on bug 28984.
Comment 67 Marcel de Rooy 2022-01-17 14:04:06 UTC
(In reply to Marcel de Rooy from comment #60)
> > > [5] This one I did not yet fully tackle. But I have the impression that we
> > > have an issue in C4/Auth now. A few times already I had to login twice.
> > > This MIGHT be a blocker. Needs still investigation.
> > 
> > Hum, I didn't experience it.
> 
> I will be testing it still a bit. Will report later.

Not able to reproduce now.
Comment 68 Jonathan Druart 2022-01-17 14:26:46 UTC
(In reply to Marcel de Rooy from comment #66)
> Well, I am still seeing some things that I would like to change. But I cant
> justify them with our coding guidelines.

What do you have in mind?
Comment 69 Marcel de Rooy 2022-01-17 15:02:56 UTC
(In reply to Jonathan Druart from comment #68)
> (In reply to Marcel de Rooy from comment #66)
> > Well, I am still seeing some things that I would like to change. But I cant
> > justify them with our coding guidelines.
> 
> What do you have in mind?

Lets do that on the other report.
Comment 70 Marcel de Rooy 2022-01-18 12:40:21 UTC
While still thinking a bit about the secret going back and forth, I discovered a tiny but serious thing overlooked: the check of the CSRF token in the return.
Will add it on a follow-up too.
Comment 71 Marcel de Rooy 2022-01-18 12:53:04 UTC
Hmm Koha::Token was missing too in use
Comment 72 Marcel de Rooy 2022-01-18 12:57:19 UTC
Created attachment 129557 [details] [review]
Bug 28786: Add new syspref TwoFactorAuthentication

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 73 Marcel de Rooy 2022-01-18 12:57:24 UTC
Created attachment 129558 [details] [review]
Bug 28786: Add new DB column borrowers.secret

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 74 Marcel de Rooy 2022-01-18 12:57:29 UTC
Created attachment 129559 [details] [review]
Bug 28786: Add new DB column borrowers.auth_method

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 75 Marcel de Rooy 2022-01-18 12:57:33 UTC
Created attachment 129560 [details] [review]
Bug 28786: Add controller script and template

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 76 Marcel de Rooy 2022-01-18 12:57:39 UTC
Created attachment 129561 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 77 Marcel de Rooy 2022-01-18 12:57:44 UTC
Created attachment 129562 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 78 Marcel de Rooy 2022-01-18 12:57:49 UTC
Created attachment 129563 [details] [review]
Bug 28786: Correctly inherit from Auth::GoogleAuth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 79 Marcel de Rooy 2022-01-18 12:57:54 UTC
Created attachment 129564 [details] [review]
Bug 28786: Add tests for checkauth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 80 Marcel de Rooy 2022-01-18 12:57:58 UTC
Created attachment 129565 [details] [review]
Bug 28786: DO NOT PUSH - DBIx schema changes

Added for testing purposes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 81 Marcel de Rooy 2022-01-18 12:58:03 UTC
Created attachment 129566 [details] [review]
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style

Test plan:
Run dbrev again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 82 Marcel de Rooy 2022-01-18 12:58:07 UTC
Created attachment 129567 [details] [review]
Bug 28786: (QA follow-up) Fix qa warnings

 FAIL   members/two_factor_auth.pl
   FAIL   file permissions
                File must have the exec flag
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/members/two_factor_auth.tt
   FAIL   filters
                missing_filter at line 42 (                        <p>Account: [% issuer %]</p>)
                missing_filter at line 43 (                        <p>Key: [% key_id %]</p>)
                missing_filter at line 54 (                            <input type="hidden" name="secret32" value="[% secret32 %]" />)
                missing_filter at line 58 (                                    <img id="qr_code" src="[% qr_code_url %]" />)
 FAIL   Koha/Auth/TwoFactorAuth.pm
   FAIL   pod coverage
                POD is missing for 'new'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 83 Marcel de Rooy 2022-01-18 12:58:12 UTC
Created attachment 129568 [details] [review]
Bug 28786: (QA follow-up) Do not call generate_secret32

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 84 Marcel de Rooy 2022-01-18 12:58:16 UTC
Created attachment 129569 [details] [review]
Bug 28786: (QA follow-up) Check the returned CSRF token

We're doing this in the registering stage and at disabling.

Test plan:
Ënable two-factor auth but logout/login on another tab. You should
get the Wrong CSRF token when submitting.
Do similar thing while disabling.
Verify that you can register / disable when in the same session.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 85 Marcel de Rooy 2022-01-18 13:04:29 UTC
Great! We have a base for 2FA in staff herewith.
Imo we could push this while keeping in mind that several follow-up reports are in the pipeline including encrypting the secret, not exposing the secret via a Google Charts URL, and others.
Early push will make us aware of problems in connection to changes in C4/Auth done here.

Note: The secret being passed in again has the protection of the CSRF token. It would be nice however to discuss the expiry time of these tokens. We have 8 hours still. Should we be more strict? Open a new report and propose something.
Comment 86 Jonathan Druart 2022-01-18 14:34:44 UTC
(In reply to Jonathan Druart from comment #63)
> Created attachment 129521 [details] [review] [review]
> Bug 28786: Don't rely on Auth::GoogleAuth to generate the secret

Marcel, apparently you removed this change but it was needed IMO. Passing secret to the constructor is useless.
Comment 87 Marcel de Rooy 2022-01-18 14:57:14 UTC
Yes, I replaced that patch.

-    my $secret32 = $auth->generate_secret32;
+    my $secret32 = encode_base32(Koha::AuthUtils::generate_salt("strong", 32));

Base32 encoding is done by the module.
You should not use strong in the AuthUtils call! It is too STRONG.
Note:

    if( $strength eq "strong" ){
        $source = '/dev/random'; # blocking

You dont want it to block on a VM or so when there is not enough randomness/entropy.
Comment 88 Marcel de Rooy 2022-01-18 14:59:23 UTC
(In reply to Jonathan Druart from comment #86)
> Passing
> secret to the constructor is useless.

Please look at the changes in my generate_secret32 patch.
You need to pass the secret, but it is just a Class::Accessor. It will only do something when you call the other methods.
Comment 89 Marcel de Rooy 2022-01-18 15:02:18 UTC
     my $auth = Koha::Auth::TwoFactorAuth->new(
         { patron => $logged_in_user, secret => $secret } );
 
-    my $secret32 = $auth->generate_secret32;
     my $qr_code_url =
-      $auth->qr_code( $secret32, $auth->key_id, $auth->issuer, );
+      $auth->qr_code( undef, $auth->key_id, $auth->issuer ); # no need to pass secret32

Do note that this qr code call will be replaced too on 29873. Almost ready.
Comment 90 Jonathan Druart 2022-01-19 12:40:31 UTC
I do agree with the last comments.
Comment 91 Marcel de Rooy 2022-01-20 08:28:21 UTC
While working on follow-up reports, I still find an issue. Sorry :)

The following scheme shows a different response with this patch set and without:

[1] Login with a good or bad userid and wrong password (staff).
You should get the error Invalid username or password.
[2] On this form with the error, login again with the same wrong credentials.
You do not get the previous error. Which you should have got!
But you will crash on line 87 of mainpage.
Can't use an undefined value as a HASH reference at /usr/share/koha/mainpage.pl line 87

Changing status
Comment 92 Marcel de Rooy 2022-01-20 12:56:05 UTC
I am revisiting the Auth part here now.
Comment 93 Jonathan Druart 2022-01-20 14:10:07 UTC
Created attachment 129654 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 94 Jonathan Druart 2022-01-20 14:10:14 UTC
Created attachment 129655 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 95 Jonathan Druart 2022-01-20 14:10:21 UTC
Created attachment 129656 [details] [review]
Bug 28786: Correctly inherit from Auth::GoogleAuth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 96 Jonathan Druart 2022-01-20 14:10:30 UTC
Created attachment 129657 [details] [review]
Bug 28786: Add tests for checkauth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 97 Jonathan Druart 2022-01-20 14:10:42 UTC
Created attachment 129658 [details] [review]
Bug 28786: DO NOT PUSH - DBIx schema changes

Added for testing purposes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 98 Jonathan Druart 2022-01-20 14:10:53 UTC
Created attachment 129659 [details] [review]
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style

Test plan:
Run dbrev again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 99 Jonathan Druart 2022-01-20 14:11:03 UTC
Created attachment 129660 [details] [review]
Bug 28786: (QA follow-up) Fix qa warnings

 FAIL   members/two_factor_auth.pl
   FAIL   file permissions
                File must have the exec flag
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/members/two_factor_auth.tt
   FAIL   filters
                missing_filter at line 42 (                        <p>Account: [% issuer %]</p>)
                missing_filter at line 43 (                        <p>Key: [% key_id %]</p>)
                missing_filter at line 54 (                            <input type="hidden" name="secret32" value="[% secret32 %]" />)
                missing_filter at line 58 (                                    <img id="qr_code" src="[% qr_code_url %]" />)
 FAIL   Koha/Auth/TwoFactorAuth.pm
   FAIL   pod coverage
                POD is missing for 'new'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 100 Jonathan Druart 2022-01-20 14:11:14 UTC
Created attachment 129661 [details] [review]
Bug 28786: (QA follow-up) Do not call generate_secret32

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 101 Jonathan Druart 2022-01-20 14:11:25 UTC
Created attachment 129662 [details] [review]
Bug 28786: (QA follow-up) Check the returned CSRF token

We're doing this in the registering stage and at disabling.

Test plan:
Ënable two-factor auth but logout/login on another tab. You should
get the Wrong CSRF token when submitting.
Do similar thing while disabling.
Verify that you can register / disable when in the same session.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 102 Jonathan Druart 2022-01-20 14:12:33 UTC
Patches rebased on top of bug 29915.
Comment 103 Jonathan Druart 2022-01-20 14:44:39 UTC
Some REST API tests are failing.
Comment 104 Jonathan Druart 2022-01-20 14:47:38 UTC
Created attachment 129665 [details] [review]
Bug 28786: Don't expose secret and auth_method to the REST API

This can be done later if needed.
Comment 105 Marcel de Rooy 2022-02-04 07:54:07 UTC
This will need another QA review
Comment 106 Jonathan Druart 2022-03-21 09:50:38 UTC
Created attachment 131940 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 107 Jonathan Druart 2022-03-21 09:50:44 UTC
Created attachment 131941 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 108 Jonathan Druart 2022-03-21 09:50:49 UTC
Created attachment 131942 [details] [review]
Bug 28786: Correctly inherit from Auth::GoogleAuth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 109 Jonathan Druart 2022-03-21 09:50:55 UTC
Created attachment 131943 [details] [review]
Bug 28786: Add tests for checkauth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 110 Jonathan Druart 2022-03-21 09:51:00 UTC
Created attachment 131944 [details] [review]
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style

Test plan:
Run dbrev again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 111 Jonathan Druart 2022-03-21 09:51:05 UTC
Created attachment 131945 [details] [review]
Bug 28786: (QA follow-up) Fix qa warnings

 FAIL   members/two_factor_auth.pl
   FAIL   file permissions
                File must have the exec flag
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/members/two_factor_auth.tt
   FAIL   filters
                missing_filter at line 42 (                        <p>Account: [% issuer %]</p>)
                missing_filter at line 43 (                        <p>Key: [% key_id %]</p>)
                missing_filter at line 54 (                            <input type="hidden" name="secret32" value="[% secret32 %]" />)
                missing_filter at line 58 (                                    <img id="qr_code" src="[% qr_code_url %]" />)
 FAIL   Koha/Auth/TwoFactorAuth.pm
   FAIL   pod coverage
                POD is missing for 'new'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 112 Jonathan Druart 2022-03-21 09:51:10 UTC
Created attachment 131946 [details] [review]
Bug 28786: (QA follow-up) Do not call generate_secret32

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 113 Jonathan Druart 2022-03-21 09:51:15 UTC
Created attachment 131947 [details] [review]
Bug 28786: (QA follow-up) Check the returned CSRF token

We're doing this in the registering stage and at disabling.

Test plan:
Ënable two-factor auth but logout/login on another tab. You should
get the Wrong CSRF token when submitting.
Do similar thing while disabling.
Verify that you can register / disable when in the same session.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 114 Jonathan Druart 2022-03-21 09:51:20 UTC
Created attachment 131948 [details] [review]
Bug 28786: Don't expose secret and auth_method to the REST API

This can be done later if needed.
Comment 115 Jonathan Druart 2022-03-21 09:52:04 UTC
Patches rebased.
Comment 116 Jonathan Druart 2022-03-21 10:14:40 UTC
Created attachment 131949 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 117 Jonathan Druart 2022-03-21 10:14:47 UTC
Created attachment 131950 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 118 Jonathan Druart 2022-03-21 10:14:52 UTC
Created attachment 131951 [details] [review]
Bug 28786: Correctly inherit from Auth::GoogleAuth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 119 Jonathan Druart 2022-03-21 10:14:59 UTC
Created attachment 131952 [details] [review]
Bug 28786: Add tests for checkauth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 120 Jonathan Druart 2022-03-21 10:15:05 UTC
Created attachment 131953 [details] [review]
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style

Test plan:
Run dbrev again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 121 Jonathan Druart 2022-03-21 10:15:11 UTC
Created attachment 131954 [details] [review]
Bug 28786: (QA follow-up) Fix qa warnings

 FAIL   members/two_factor_auth.pl
   FAIL   file permissions
                File must have the exec flag
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/members/two_factor_auth.tt
   FAIL   filters
                missing_filter at line 42 (                        <p>Account: [% issuer %]</p>)
                missing_filter at line 43 (                        <p>Key: [% key_id %]</p>)
                missing_filter at line 54 (                            <input type="hidden" name="secret32" value="[% secret32 %]" />)
                missing_filter at line 58 (                                    <img id="qr_code" src="[% qr_code_url %]" />)
 FAIL   Koha/Auth/TwoFactorAuth.pm
   FAIL   pod coverage
                POD is missing for 'new'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 122 Jonathan Druart 2022-03-21 10:15:18 UTC
Created attachment 131955 [details] [review]
Bug 28786: (QA follow-up) Do not call generate_secret32

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 123 Jonathan Druart 2022-03-21 10:15:24 UTC
Created attachment 131956 [details] [review]
Bug 28786: (QA follow-up) Check the returned CSRF token

We're doing this in the registering stage and at disabling.

Test plan:
Ënable two-factor auth but logout/login on another tab. You should
get the Wrong CSRF token when submitting.
Do similar thing while disabling.
Verify that you can register / disable when in the same session.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 124 Jonathan Druart 2022-03-21 10:15:30 UTC
Created attachment 131957 [details] [review]
Bug 28786: Don't expose secret and auth_method to the REST API

This can be done later if needed.
Comment 125 Jonathan Druart 2022-03-21 10:15:36 UTC
Created attachment 131958 [details] [review]
Bug 28786: DBIC schema
Comment 126 Jonathan Druart 2022-03-21 10:15:48 UTC
Correctly rebased now.
Comment 127 Jonathan Druart 2022-03-24 13:29:41 UTC
Created attachment 132142 [details] [review]
Bug 28786: Add new syspref TwoFactorAuthentication

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 128 Jonathan Druart 2022-03-24 13:29:49 UTC
Created attachment 132143 [details] [review]
Bug 28786: Add new DB column borrowers.secret

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 129 Jonathan Druart 2022-03-24 13:29:55 UTC
Created attachment 132144 [details] [review]
Bug 28786: Add new DB column borrowers.auth_method

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 130 Jonathan Druart 2022-03-24 13:30:03 UTC
Created attachment 132145 [details] [review]
Bug 28786: Add controller script and template

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 131 Jonathan Druart 2022-03-24 13:30:11 UTC
Created attachment 132146 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 132 Jonathan Druart 2022-03-24 13:30:17 UTC
Created attachment 132147 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 133 Jonathan Druart 2022-03-24 13:30:24 UTC
Created attachment 132148 [details] [review]
Bug 28786: Correctly inherit from Auth::GoogleAuth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 134 Jonathan Druart 2022-03-24 13:30:30 UTC
Created attachment 132149 [details] [review]
Bug 28786: Add tests for checkauth

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 135 Jonathan Druart 2022-03-24 13:30:42 UTC
Created attachment 132150 [details] [review]
Bug 28786: (QA follow-up) Upgrade atomicupdate to new style

Test plan:
Run dbrev again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 136 Jonathan Druart 2022-03-24 13:30:50 UTC
Created attachment 132153 [details] [review]
Bug 28786: (QA follow-up) Fix qa warnings

 FAIL   members/two_factor_auth.pl
   FAIL   file permissions
                File must have the exec flag
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/members/two_factor_auth.tt
   FAIL   filters
                missing_filter at line 42 (                        <p>Account: [% issuer %]</p>)
                missing_filter at line 43 (                        <p>Key: [% key_id %]</p>)
                missing_filter at line 54 (                            <input type="hidden" name="secret32" value="[% secret32 %]" />)
                missing_filter at line 58 (                                    <img id="qr_code" src="[% qr_code_url %]" />)
 FAIL   Koha/Auth/TwoFactorAuth.pm
   FAIL   pod coverage
                POD is missing for 'new'

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 137 Jonathan Druart 2022-03-24 13:30:57 UTC
Created attachment 132154 [details] [review]
Bug 28786: (QA follow-up) Do not call generate_secret32

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 138 Jonathan Druart 2022-03-24 13:31:04 UTC
Created attachment 132155 [details] [review]
Bug 28786: (QA follow-up) Check the returned CSRF token

We're doing this in the registering stage and at disabling.

Test plan:
Ënable two-factor auth but logout/login on another tab. You should
get the Wrong CSRF token when submitting.
Do similar thing while disabling.
Verify that you can register / disable when in the same session.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 139 Jonathan Druart 2022-03-24 13:31:09 UTC
Created attachment 132156 [details] [review]
Bug 28786: Don't expose secret and auth_method to the REST API

This can be done later if needed.
Comment 140 Jonathan Druart 2022-03-24 13:31:16 UTC
Created attachment 132157 [details] [review]
Bug 28786: DBIC schema changes
Comment 141 Marcel de Rooy 2022-04-12 12:15:26 UTC
Revisiting this one again now
Comment 142 Marcel de Rooy 2022-04-12 12:55:51 UTC
No rebase problems found, tested. Passing QA again.
Comment 143 Owen Leonard 2022-04-12 16:41:50 UTC
Created attachment 133222 [details] [review]
Bug 28786: (follow-up) Improve style of 2FA code input

This patch cleans up login.css a little bit so that the same style rules
which apply to the login screen apply to the 2FA input form as well.

The patch also changes the "Log out" link on the 2FA form to a "Cancel"
link alongside the "Verify" button.

To test, apply the patch and start the process of logging in to the
staff client using an account with 2FA enabled.

On both the login form and 2FA code views, confirm that everything is
styled consistently.
Comment 144 Owen Leonard 2022-04-12 16:42:43 UTC
If this follow-up doesn't work for everyone please just ignore it. I don't want to hold up the process.
Comment 145 Fridolin Somers 2022-04-20 19:33:59 UTC
This follow-up looks nice.
I have a branch prepared for push.
Waiting for Mason about perl dep.
Comment 146 Mason James 2022-04-21 04:22:42 UTC
Created attachment 133516 [details] [review]
Bug 28786: add Auth::GoogleAuth module to cpanfile
Comment 147 Mason James 2022-04-21 04:24:10 UTC
(In reply to Fridolin Somers from comment #145)
> This follow-up looks nice.
> I have a branch prepared for push.
> Waiting for Mason about perl dep.

hi Frido, i've added a patch for the dep
Comment 148 Fridolin Somers 2022-04-21 06:32:16 UTC
(In reply to Mason James from comment #147)
> (In reply to Fridolin Somers from comment #145)
> > This follow-up looks nice.
> > I have a branch prepared for push.
> > Waiting for Mason about perl dep.
> 
> hi Frido, i've added a patch for the dep

Super fast, thanks ;)
Comment 149 Fridolin Somers 2022-04-21 06:43:56 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 150 Mason James 2022-04-21 07:22:49 UTC
Created attachment 133522 [details] [review]
decrease Auth::GoogleAuth version to 1.02
Comment 151 Jonathan Druart 2022-04-21 09:43:39 UTC
Created attachment 133531 [details] [review]
Bug 28786: Don't set auth_method to NULL in ->anonymize

DBI Exception: DBD::mysql::st execute failed: Column 'auth_method' cannot be null

caught by t/db_dependent/Koha/Patrons.t
Comment 152 Mason James 2022-04-21 09:50:11 UTC
Created attachment 133533 [details] [review]
Bug 28786: Don't set auth_method to NULL in ->anonymize

DBI Exception: DBD::mysql::st execute failed: Column 'auth_method' cannot be null

caught by t/db_dependent/Koha/Patrons.t

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 153 Jonathan Druart 2022-04-21 11:52:42 UTC
Created attachment 133541 [details] [review]
Bug 28786: Fix Patrons/Import.t

DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'auth_method' at row 1 at /kohadevbox/koha/Koha/Object.pm line 170
Comment 154 Marcel de Rooy 2022-04-21 12:02:48 UTC
+        $borrower{auth_method} = undef if $borrower{auth_method} eq "";

`auth_method` ENUM('password', 'two-factor') NOT NULL DEFAULT 'password' COMMENT 'Authentication method',

Are you sure?
Comment 155 Marcel de Rooy 2022-04-21 12:18:15 UTC
Created attachment 133548 [details] [review]
Bug 28786: Fix Patrons/Import.t

DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'auth_method' at row 1 at /kohadevbox/koha/Koha/Object.pm line 170

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 156 Marcel de Rooy 2022-04-21 12:19:13 UTC
This part in import_patrons is arguable:

                else {
                    $borrower{$key} = '';
                }
Comment 157 Jonathan Druart 2022-04-21 13:25:30 UTC
(In reply to Marcel de Rooy from comment #154)
> +        $borrower{auth_method} = undef if $borrower{auth_method} eq "";
> 
> `auth_method` ENUM('password', 'two-factor') NOT NULL DEFAULT 'password'
> COMMENT 'Authentication method',
> 
> Are you sure?

If set to null the DBMS will use the default value.
Comment 158 Martin Renvoize 2022-04-21 16:42:29 UTC
*** Bug 19887 has been marked as a duplicate of this bug. ***
Comment 159 Tomás Cohen Arazi 2022-04-21 17:09:52 UTC
(In reply to Jonathan Druart from comment #28)
> (In reply to Martin Renvoize from comment #27)
> > I've finally found a moment to look at this.
> > 
> > Whilst I like Tomas's proposal to normalise the database by having a
> > distinct table.. I think that can certainly come as a followup later.
> 
> If we are talking about 3NF, I don't think having a separate table with a
> single column (different than the FK) is what can be called normalisation.
> 
> If that is what you have in mind, I think it's wrong:
> borrowers < 1-1 > secrets
>                   - borrowernumber (FK borrowers.borrowernumber)
>                   - secret

Look at bug 29924 and it will become obvious that we are just delaying the inevitable discussion. Which is fine. But I hope we solve on the next cycle.
Comment 160 Tomás Cohen Arazi 2022-04-21 17:12:30 UTC
(In reply to David Cook from comment #25)
> Note having a credential table could also make it easier to add "temporary"
> passwords. 
> 
> For instance, it would be great if staff could reset a password, tell the
> patron the password, and then the patron would be forced to reset that
> password on first login. I'm going to raise a ticket for that and link it to
> here...

And that'd be yet another attribute for the credential (needs_reset?)... which shouldn't belong to the patron object itself.
Comment 161 Katrin Fischer 2022-04-21 20:28:20 UTC
I feel like we should highlight this as a 'new feature' in release notes :)
Comment 162 Fridolin Somers 2022-04-21 22:58:50 UTC
(In reply to Katrin Fischer from comment #161)
> I feel like we should highlight this as a 'new feature' in release notes :)

Indeed. I changed the start of release notes.
Comment 163 Fridolin Somers 2022-04-21 23:00:47 UTC
Pushed to master :
  Bug 28786: Fix Patrons/Import.t

Can we remove 'additional_work_needed' ?
Comment 164 Marcel de Rooy 2022-04-22 06:07:31 UTC
(In reply to Tomás Cohen Arazi from comment #160)
> (In reply to David Cook from comment #25)
> > Note having a credential table could also make it easier to add "temporary"
> > passwords. 
> > 
> > For instance, it would be great if staff could reset a password, tell the
> > patron the password, and then the patron would be forced to reset that
> > password on first login. I'm going to raise a ticket for that and link it to
> > here...
> 
> And that'd be yet another attribute for the credential (needs_reset?)...
> which shouldn't belong to the patron object itself.

Opened bug 30592 for you, Tomas. Feel free to take assignee ;)
Comment 165 Jonathan Druart 2022-04-22 08:55:16 UTC
Created attachment 133643 [details] [review]
Bug 28786: Fix 2FA selenium tests

Looks like there was a bad rebase at some point.
Comment 166 Martin Renvoize 2022-04-22 14:40:53 UTC
Created attachment 133686 [details] [review]
Bug 28786: Fix 2FA selenium tests

Looks like there was a bad rebase at some point.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 167 Fridolin Somers 2022-04-22 18:41:04 UTC
"Fix 2FA selenium tests" pushed to master
Comment 168 Caroline Cyr La Rose 2022-07-26 19:43:20 UTC
I added a section in the manual. This type of more technical thing is a bit outside my expertise and I'm not 100% sure I have the right vocabulary. So please let me know of any corrections I need to make. 

https://gitlab.com/koha-community/koha-manual/-/merge_requests/578
Comment 169 Jonathan Druart 2022-07-27 07:58:16 UTC
(In reply to Caroline Cyr La Rose from comment #168)
> I added a section in the manual. This type of more technical thing is a bit
> outside my expertise and I'm not 100% sure I have the right vocabulary. So
> please let me know of any corrections I need to make. 
> 
> https://gitlab.com/koha-community/koha-manual/-/merge_requests/578

Read good, thanks!