|
Description
Jonathan Druart
2021-07-30 12:02:36 UTC
Created attachment 123375 [details] [review] Bug 28786: Add new syspref TwoFactorAuthentication Sponsored-by: Orex Digital Created attachment 123376 [details] [review] Bug 28786: Add new DB column borrowers.secret Sponsored-by: Orex Digital Created attachment 123377 [details] [review] Bug 28786: Add new DB column borrowers.auth_method Sponsored-by: Orex Digital Created attachment 123378 [details] [review] Bug 28786: Add controller script and template Sponsored-by: Orex Digital 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 Created attachment 123380 [details] [review] Bug 28786: Improve readability in C4::Auth::checkauth Sponsored-by: Orex Digital Created attachment 123381 [details] [review] Bug 28786: DBIC schema changes Sponsored-by: Orex Digital Created attachment 123397 [details] [review] Bug 28786: Add new syspref TwoFactorAuthentication Sponsored-by: Orex Digital Signed-off-by: David Nind <david@davidnind.com> 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> 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> Created attachment 123400 [details] [review] Bug 28786: Add controller script and template Sponsored-by: Orex Digital Signed-off-by: David Nind <david@davidnind.com> 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> 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> Created attachment 123403 [details] [review] Bug 28786: DBIC schema changes Sponsored-by: Orex Digital Signed-off-by: David Nind <david@davidnind.com> 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 Can we add a new table for the 2FA secrets please? (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. (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. 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?
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.
(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? 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. 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... Note that Keycloak db schema view is very old. It's just the first visualization I could find while I was quickly looking. 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. (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. 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. This will be done on bug 28998. 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> 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> 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? +package Koha::Auth::TwoFactorAuth; +=head1 NAME + +Koha::Encryption - Koha class to encrypt or decrypt strings Hmm Template process failed: undef error - The method Koha::Patron->auth_method is not covered by tests! (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. 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. 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. 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> 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> (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! Created attachment 124846 [details] [review] Bug 28786: Correctly inherit from Auth::GoogleAuth Created attachment 124847 [details] [review] Bug 28786: Add tests for checkauth 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 :( (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 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'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. (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. (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 Created attachment 128841 [details] [review] Bug 28786: DO NOT PUSH - DBIx schema changes Added for testing purposes. Created attachment 128842 [details] [review] Bug 28786: (QA follow-up) Upgrade atomicupdate to new style Test plan: Run dbrev again. Starting QA here Created attachment 129383 [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 %]" />) FAIL Koha/Auth/TwoFactorAuth.pm FAIL pod coverage POD is missing for 'new' Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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> 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
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.
Opened bug 29873 (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. 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. (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. -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?
(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. Created attachment 129521 [details] [review] Bug 28786: Don't rely on Auth::GoogleAuth to generate the secret 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! + 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.
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. (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. (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? (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. 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. Hmm Koha::Token was missing too in use 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> 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> 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> 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> 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> 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> Created attachment 129563 [details] [review] Bug 28786: Correctly inherit from Auth::GoogleAuth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 129564 [details] [review] Bug 28786: Add tests for checkauth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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> 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> 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> 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> 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> 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. (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. 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.
(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. 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.
I do agree with the last comments. 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 I am revisiting the Auth part here now. 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> 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> Created attachment 129656 [details] [review] Bug 28786: Correctly inherit from Auth::GoogleAuth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 129657 [details] [review] Bug 28786: Add tests for checkauth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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> 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> 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> 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> 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> Patches rebased on top of bug 29915. Some REST API tests are failing. 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. This will need another QA review 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> 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> Created attachment 131942 [details] [review] Bug 28786: Correctly inherit from Auth::GoogleAuth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 131943 [details] [review] Bug 28786: Add tests for checkauth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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> 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> 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> 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> 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. Patches rebased. 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> 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> Created attachment 131951 [details] [review] Bug 28786: Correctly inherit from Auth::GoogleAuth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 131952 [details] [review] Bug 28786: Add tests for checkauth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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> 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> 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> 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> 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. Created attachment 131958 [details] [review] Bug 28786: DBIC schema Correctly rebased now. 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> 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> 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> 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> 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> 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> Created attachment 132148 [details] [review] Bug 28786: Correctly inherit from Auth::GoogleAuth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 132149 [details] [review] Bug 28786: Add tests for checkauth Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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> 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> 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> 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> 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. Created attachment 132157 [details] [review] Bug 28786: DBIC schema changes Revisiting this one again now No rebase problems found, tested. Passing QA again. 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. If this follow-up doesn't work for everyone please just ignore it. I don't want to hold up the process. This follow-up looks nice. I have a branch prepared for push. Waiting for Mason about perl dep. Created attachment 133516 [details] [review] Bug 28786: add Auth::GoogleAuth module to cpanfile (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 (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 ;) Pushed to master for 22.05, thanks to everybody involved [U+1F984] Created attachment 133522 [details] [review] decrease Auth::GoogleAuth version to 1.02 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 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> 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 + $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?
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> This part in import_patrons is arguable:
else {
$borrower{$key} = '';
}
(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. *** Bug 19887 has been marked as a duplicate of this bug. *** (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. (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. I feel like we should highlight this as a 'new feature' in release notes :) (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. Pushed to master : Bug 28786: Fix Patrons/Import.t Can we remove 'additional_work_needed' ? (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 ;) Created attachment 133643 [details] [review] Bug 28786: Fix 2FA selenium tests Looks like there was a bad rebase at some point. 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> "Fix 2FA selenium tests" pushed to master 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 (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! |
