Bug 29873 - 2FA: Generate QR code without exposing secret via HTTP GET
Summary: 2FA: Generate QR code without exposing secret via HTTP GET
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 28786
Blocks: 29894 32011
  Show dependency treegraph
 
Reported: 2022-01-13 12:32 UTC by Marcel de Rooy
Modified: 2023-06-08 22:26 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Instead of calling the (deprecated) Google Charts API and exposing our secret, we create the image ourself and push it as a data uri in the src attribute of the image (inline image).
Version(s) released in:
22.05.00


Attachments
Bug 29873: Create QR code (3.00 KB, patch)
2022-01-13 14:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: Create QR code (3.61 KB, patch)
2022-01-18 15:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: Add unit test (2.60 KB, patch)
2022-01-18 15:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: Adjust QR image size (1.10 KB, patch)
2022-01-18 15:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: Add unit test (2.66 KB, patch)
2022-01-18 15:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: Adjust QR image size (1.16 KB, patch)
2022-01-18 15:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: Update cpanfile (752 bytes, patch)
2022-01-18 15:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: (follow-up) Rename qr_dataurl (5.10 KB, patch)
2022-01-19 12:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: Create QR code (4.01 KB, patch)
2022-04-12 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: Add unit test (2.68 KB, patch)
2022-04-12 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29873: Create QR code (4.03 KB, patch)
2022-04-21 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29873: Add unit test (2.71 KB, patch)
2022-04-21 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29873: Create QR code (4.11 KB, patch)
2022-04-22 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29873: Add unit test (2.78 KB, patch)
2022-04-22 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-01-13 12:32:45 UTC
See bug 28786. The Google Charts API shows the secret in the src of the image.
Comment 1 Marcel de Rooy 2022-01-13 13:52:08 UTC
Will give it a go
Comment 2 Marcel de Rooy 2022-01-13 14:41:44 UTC
Created attachment 129402 [details] [review]
Bug 29873: Create QR code
Comment 3 Marcel de Rooy 2022-01-13 14:46:06 UTC
Work in progress. But should give you an idea. Works for me. But needs some POD, unit testing, dependency registering.

It uses Imager::QRCode, packaged for Debian in libimager-qrcode-perl.
Comment 4 Marcel de Rooy 2022-01-18 15:04:06 UTC
Created attachment 129576 [details] [review]
Bug 29873: Create QR code

Instead of using deprecated Google Charts API, and exposing our
secret in a GET parameter, we generate QR data ourselves.

Test plan:
[1] Enable two factor authentication in the prefs.
[2] Login in staff. Go to account. Select Manage 2FA.
[3] Verify that QR code is displayed.
[4] Register the QR in your authenticator app and test 2FA
    by logging in again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with Google Authenticator and FreeOTP.
Comment 5 Marcel de Rooy 2022-01-18 15:04:10 UTC
Created attachment 129577 [details] [review]
Bug 29873: Add unit test

Test plan:
Run t/db_dependent/Koha/Auth/TwoFactorAuth.t
Comment 6 Marcel de Rooy 2022-01-18 15:04:52 UTC
Still looking at size of QR image (but it works). And impact on Selenium test.
Comment 7 Marcel de Rooy 2022-01-18 15:31:17 UTC
Created attachment 129578 [details] [review]
Bug 29873: Adjust QR image size

Size parameter 8 results in too big images.
Size parameter 4 seems fine. Although I have seen variations
in the number of pixels from about 116x116 to 180x180.
The documentation is a bit obsure.

Test plan:
Register again with smaller QR code and logout/login.
Comment 8 Marcel de Rooy 2022-01-18 15:34:32 UTC
(In reply to Marcel de Rooy from comment #6)
> Still looking at size of QR image (but it works). And impact on Selenium
> test.

t/db_dependent/selenium/authentication_2fa.t seems fine to me still.
Could you check, Jonathan?
Comment 9 Marcel de Rooy 2022-01-18 15:35:13 UTC
Created attachment 129579 [details] [review]
Bug 29873: Add unit test

Test plan:
Run t/db_dependent/Koha/Auth/TwoFactorAuth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2022-01-18 15:35:17 UTC
Created attachment 129580 [details] [review]
Bug 29873: Adjust QR image size

Size parameter 8 results in too big images.
Size parameter 4 seems fine. Although I have seen variations
in the number of pixels from about 116x116 to 180x180.
The documentation is a bit obsure.

Test plan:
Register again with smaller QR code and logout/login.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2022-01-18 15:50:15 UTC
Created attachment 129581 [details] [review]
Bug 29873: Update cpanfile

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Jonathan Druart 2022-01-18 16:12:34 UTC
I would rename ->qr_dataurl with ->qr_code as it actually generates an image.
Comment 13 Marcel de Rooy 2022-01-19 12:59:34 UTC
Created attachment 129605 [details] [review]
Bug 29873: (follow-up) Rename qr_dataurl

As requested by a QA team member.
We're moving to qr_code as method name. This is the same name as
the method in the underlying base class.
Apart from one sed statement, changing to self->SUPER on one line.

Test plan:
Can you still register, logout and login?
Run t/db_dependent/Koha/Auth/TwoFactorAuth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2022-01-19 13:00:10 UTC
(In reply to Jonathan Druart from comment #12)
> I would rename ->qr_dataurl with ->qr_code as it actually generates an image.

Renamed. Added a few POD lines now too.
Comment 15 Marcel de Rooy 2022-02-01 12:33:40 UTC
Hmm. Should we have done something like:

$image = GD::Barcode->new('QRcode', $barcode, { Ecc => "M", ModuleSize => $qrcode_modulesize } )->plot->png();
Comment 16 Marcel de Rooy 2022-02-01 12:44:03 UTC
GD::Barcode::QRcode
Comment 17 Marcel de Rooy 2022-02-01 12:50:24 UTC
Back to ASSIGNED. If all the stuff that comes before this one, gets clearer, I will resubmit.
Comment 18 Marcel de Rooy 2022-04-12 13:28:16 UTC
Second try
Comment 19 Marcel de Rooy 2022-04-12 14:31:00 UTC
Created attachment 133216 [details] [review]
Bug 29873: Create QR code

Instead of using deprecated Google Charts API, and exposing our
secret in a GET parameter, we generate QR data ourselves.

Test plan:
[1] Enable two factor authentication in the prefs.
[2] Login in staff. Go to account. Select Manage 2FA.
[3] Verify that QR code is displayed.
[4] Register the QR in your authenticator app and test 2FA
    by logging in again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with Google Authenticator and FreeOTP.

Bug 29873: (follow-up) Rename qr_dataurl

As requested by a QA team member.
We're moving to qr_code as method name. This is the same name as
the method in the underlying base class.
Apart from one sed statement, changing to self->SUPER on one line.

Test plan:
Can you still register, logout and login?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 29873: (follow-up) Switch to GD

We do not need a new module, we could use GD instead.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2022-04-12 14:31:04 UTC
Created attachment 133217 [details] [review]
Bug 29873: Add unit test

Test plan:
Run t/db_dependent/Koha/Auth/TwoFactorAuth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended to reflect requested changes.
Comment 21 Martin Renvoize 2022-04-21 13:15:44 UTC
Created attachment 133558 [details] [review]
Bug 29873: Create QR code

Instead of using deprecated Google Charts API, and exposing our
secret in a GET parameter, we generate QR data ourselves.

Test plan:
[1] Enable two factor authentication in the prefs.
[2] Login in staff. Go to account. Select Manage 2FA.
[3] Verify that QR code is displayed.
[4] Register the QR in your authenticator app and test 2FA
    by logging in again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with Google Authenticator and FreeOTP.

Bug 29873: (follow-up) Rename qr_dataurl

As requested by a QA team member.
We're moving to qr_code as method name. This is the same name as
the method in the underlying base class.
Apart from one sed statement, changing to self->SUPER on one line.

Test plan:
Can you still register, logout and login?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 29873: (follow-up) Switch to GD

We do not need a new module, we could use GD instead.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2022-04-21 13:15:49 UTC
Created attachment 133559 [details] [review]
Bug 29873: Add unit test

Test plan:
Run t/db_dependent/Koha/Auth/TwoFactorAuth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended to reflect requested changes.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2022-04-21 13:21:23 UTC
Good call.. works as expected. Signing of..

Must admit, I still wonder if we aught to have gone with Authen::OATH rather than Auth::GoogleAuth for 2FA.. but that's another question entirely.
Comment 24 Jonathan Druart 2022-04-21 13:34:54 UTC
(In reply to Martin Renvoize from comment #23)
> Must admit, I still wonder if we aught to have gone with Authen::OATH rather
> than Auth::GoogleAuth for 2FA.. but that's another question entirely.

Discussed extensively on bug 28786 already.

tldr; With this patch there is no bad reason to not switch (but do we have a good reason to switch?).
Comment 25 Marcel de Rooy 2022-04-22 06:12:15 UTC
(In reply to Jonathan Druart from comment #24)
> (In reply to Martin Renvoize from comment #23)
> > Must admit, I still wonder if we aught to have gone with Authen::OATH rather
> > than Auth::GoogleAuth for 2FA.. but that's another question entirely.
> 
> Discussed extensively on bug 28786 already.
> 
> tldr; With this patch there is no bad reason to not switch (but do we have a
> good reason to switch?).

Lets not do that now anymore. Go ahead, Joubu. QA time!
Comment 26 Jonathan Druart 2022-04-22 10:28:50 UTC
Created attachment 133659 [details] [review]
Bug 29873: Create QR code

Instead of using deprecated Google Charts API, and exposing our
secret in a GET parameter, we generate QR data ourselves.

Test plan:
[1] Enable two factor authentication in the prefs.
[2] Login in staff. Go to account. Select Manage 2FA.
[3] Verify that QR code is displayed.
[4] Register the QR in your authenticator app and test 2FA
    by logging in again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with Google Authenticator and FreeOTP.

Bug 29873: (follow-up) Rename qr_dataurl

As requested by a QA team member.
We're moving to qr_code as method name. This is the same name as
the method in the underlying base class.
Apart from one sed statement, changing to self->SUPER on one line.

Test plan:
Can you still register, logout and login?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 29873: (follow-up) Switch to GD

We do not need a new module, we could use GD instead.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2022-04-22 10:28:56 UTC
Created attachment 133660 [details] [review]
Bug 29873: Add unit test

Test plan:
Run t/db_dependent/Koha/Auth/TwoFactorAuth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended to reflect requested changes.
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Fridolin Somers 2022-05-02 21:24:12 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄