Bug 28998 - Encrypt borrowers.secret
Summary: Encrypt borrowers.secret
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 29894
Blocks: 30592 30594 33934 30649
  Show dependency treegraph
 
Reported: 2021-09-10 15:15 UTC by Jonathan Druart
Modified: 2023-06-07 07:28 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Sponsored-by: Orex Digital
Version(s) released in:
22.05.00


Attachments
Bug 28998: Store encrypted secret (8.11 KB, patch)
2021-09-10 15:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28998: Add encryption_key to the config file (3.10 KB, patch)
2021-09-10 15:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28998: Add Crypt::CBC dependency (696 bytes, patch)
2021-09-10 15:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28998: Introduce Koha::Encryption (2.74 KB, patch)
2022-04-14 08:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Introduce Koha::Encryption (2.82 KB, patch)
2022-04-14 14:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret (2.94 KB, patch)
2022-04-14 14:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script (4.06 KB, patch)
2022-04-14 14:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Adjust Selenium test (2.79 KB, patch)
2022-04-14 14:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test (1.75 KB, patch)
2022-04-14 14:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Add encryption_key to the config file (3.19 KB, patch)
2022-04-14 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Add Crypt::CBC dependency (791 bytes, patch)
2022-04-14 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Introduce Koha::Encryption (2.82 KB, patch)
2022-04-21 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret (2.80 KB, patch)
2022-04-21 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script (4.06 KB, patch)
2022-04-21 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Adjust Selenium test (2.79 KB, patch)
2022-04-21 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test (1.75 KB, patch)
2022-04-21 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Add encryption_key to the config file (3.19 KB, patch)
2022-04-21 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Add Crypt::CBC dependency (791 bytes, patch)
2022-04-21 08:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Introduce Koha::Encryption (2.90 KB, patch)
2022-04-21 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret (2.82 KB, patch)
2022-04-21 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script (4.08 KB, patch)
2022-04-21 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Adjust Selenium test (2.81 KB, patch)
2022-04-21 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test (1.78 KB, patch)
2022-04-21 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Add encryption_key to the config file (3.21 KB, patch)
2022-04-21 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Add Crypt::CBC dependency (816 bytes, patch)
2022-04-21 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (QA follow-up) POD Fix (1.44 KB, patch)
2022-04-21 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Introduce Koha::Encryption (2.94 KB, patch)
2022-04-22 07:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret (2.86 KB, patch)
2022-04-22 07:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script (3.67 KB, patch)
2022-04-22 07:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Adjust Selenium test (2.85 KB, patch)
2022-04-22 07:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test (1.82 KB, patch)
2022-04-22 07:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Add encryption_key to the config file (3.25 KB, patch)
2022-04-22 07:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Add Crypt::CBC dependency (856 bytes, patch)
2022-04-22 07:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (QA follow-up) POD Fix (1.48 KB, patch)
2022-04-22 07:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (follow-up) Add warning on about for missing key (1.99 KB, patch)
2022-04-22 07:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Check missing encryption key in script and module (2.65 KB, patch)
2022-04-22 08:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (follow-up) Check missing encryption key in script and module (2.67 KB, patch)
2022-04-22 08:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Introduce Koha::Encryption (2.90 KB, patch)
2022-04-22 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret (2.82 KB, patch)
2022-04-22 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script (3.63 KB, patch)
2022-04-22 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Adjust Selenium test (2.81 KB, patch)
2022-04-22 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test (1.78 KB, patch)
2022-04-22 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Add encryption_key to the config file (3.21 KB, patch)
2022-04-22 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Add Crypt::CBC dependency (816 bytes, patch)
2022-04-22 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (QA follow-up) POD Fix (1.44 KB, patch)
2022-04-22 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Add warning on about for missing key (2.01 KB, patch)
2022-04-22 08:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Check missing encryption key in script and module (2.69 KB, patch)
2022-04-22 08:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Check missing encryption key in script and module (3.71 KB, patch)
2022-04-22 08:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: (follow-up) Check missing encryption key in script and module (3.78 KB, patch)
2022-04-22 08:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28998: Introduce Koha::Encryption (2.90 KB, patch)
2022-04-22 09:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret (2.98 KB, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script (3.63 KB, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Adjust Selenium test (2.81 KB, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test (1.78 KB, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Add encryption_key to the config file (3.21 KB, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Add Crypt::CBC dependency (816 bytes, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (QA follow-up) POD Fix (1.44 KB, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Add warning on about for missing key (2.01 KB, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (follow-up) Check missing encryption key in script and module (3.74 KB, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: (QA follow-up) Improve output on error (15.04 KB, patch)
2022-04-22 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28998: Fix Auth. (1.02 KB, patch)
2022-05-05 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28998: (QA follow-up) Silence some useless warnings (2.12 KB, patch)
2022-05-05 11:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28998: (follow-up) Adjust the Auth.t fix (2.66 KB, patch)
2022-05-06 06:36 UTC, Marcel de Rooy
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-09-10 15:15:55 UTC
We don't want the secret to be available from the reports admin and so we must encrypt them.
Comment 1 Jonathan Druart 2021-09-10 15:31:08 UTC
Created attachment 124772 [details] [review]
Bug 28998: Store encrypted secret
Comment 2 Jonathan Druart 2021-09-10 15:31:13 UTC
Created attachment 124773 [details] [review]
Bug 28998: Add encryption_key to the config file
Comment 3 Jonathan Druart 2021-09-10 15:31:17 UTC
Created attachment 124774 [details] [review]
Bug 28998: Add Crypt::CBC dependency
Comment 4 Jonathan Druart 2021-09-10 15:32:29 UTC
How do we generate the new config entry on upgrade?
Comment 5 Martin Renvoize 2021-09-10 16:06:47 UTC
This is looking great.. thanks for taking the time on it.. I'll swing back round to this once I've completed working through the 2FA bug.
Comment 6 Martin Renvoize 2021-09-10 16:09:24 UTC
242722e99df77be9ce0e4c6d52ff402e0a77d604 might help with generating the random key at koha-create.. need to dig out how we did it for upgrades
Comment 7 Marcel de Rooy 2022-01-17 14:35:40 UTC
This is what we do with the API secret using Crypt::Eksblowfish::Bcrypt :

        $self->set(
            {   secret    => Koha::AuthUtils::hash_password( $self->{_plain_text_secret} ),
                client_id => $self->_generate_unused_uuid('client_id'),
            }

Just reading a bit, it seems that AES is preferred over Blowfish. But this is theoretical and about larger texts to encrypt.
Since we are only crypting a short secret here, I wonder if we should not stick to Blowfish here and use what we already have instead of yet another module?
Comment 8 Tomás Cohen Arazi 2022-01-17 14:43:50 UTC
Question: shouldn't we entirely move the 'secret' column to a separate table and add the table to a deny-list?
Comment 9 Marcel de Rooy 2022-04-14 06:48:02 UTC
Looking here. Might propose some changes?
Comment 10 Marcel de Rooy 2022-04-14 07:55:04 UTC
(In reply to Marcel de Rooy from comment #7)
> Since we are only crypting a short secret here, I wonder if we should not
> stick to Blowfish here and use what we already have instead of yet another
> module?

Using Crypt::Eksblowfish is a bit of a pain (you need to chunk yourself, add padding etc.) So just scratch that.
Comment 11 Marcel de Rooy 2022-04-14 08:45:06 UTC
Created attachment 133291 [details] [review]
Bug 28998: Introduce Koha::Encryption

Test plan:
Run t/db_dependent/Koha/Encryption.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2022-04-14 14:28:33 UTC
Created attachment 133317 [details] [review]
Bug 28998: Introduce Koha::Encryption

Test plan:
Run t/db_dependent/Koha/Encryption.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[AMENDED] Added copyright line to module.
Comment 13 Marcel de Rooy 2022-04-14 14:28:38 UTC
Created attachment 133318 [details] [review]
Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2022-04-14 14:28:44 UTC
Created attachment 133319 [details] [review]
Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script

Test plan:
Run t/db_dependent/Koha/Auth/TwoFactorAuth.t
Walk thru cycle register - logout/login - deregister - logout/login.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2022-04-14 14:28:48 UTC
Created attachment 133320 [details] [review]
Bug 28998: Adjust Selenium test

Test plan:
Run t/db_dependent/selenium/authentication_2fa.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2022-04-14 14:28:53 UTC
Created attachment 133321 [details] [review]
Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test

This patch can be squashed with the previous one, if all is ok.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2022-04-14 14:31:09 UTC
Created attachment 133322 [details] [review]
Bug 28998: Add encryption_key to the config file

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2022-04-14 14:31:13 UTC
Created attachment 133323 [details] [review]
Bug 28998: Add Crypt::CBC dependency

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2022-04-14 14:32:04 UTC
(In reply to Marcel de Rooy from comment #17)
> Created attachment 133322 [details] [review] [review]
> Bug 28998: Add encryption_key to the config file
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Recently a QAer insisted that I should adjust koha-create when adding koha-conf.xml entries :)
Comment 20 Marcel de Rooy 2022-04-14 14:36:21 UTC
Reworked this a bit and tested on top of 28786 - 29873 - 29894.
Main thing is splitting the first patch and moving some code to Koha::Patron.
Taking the liberty to set it to Signed Off with one request: Please run the Selenium test. And if you agree, squash patch 4 and patch 5.
If you would not agree, feel free to change status and tell me about it :)
Comment 21 Marcel de Rooy 2022-04-21 08:41:12 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2022-04-21 08:41:17 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2022-04-21 08:41:21 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2022-04-21 08:41:26 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2022-04-21 08:41:30 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2022-04-21 08:41:34 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2022-04-21 08:41:39 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2022-04-21 08:42:26 UTC
Trivial rebase on Patron
Comment 29 Martin Renvoize 2022-04-21 16:01:15 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize 2022-04-21 16:01:20 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize 2022-04-21 16:01:25 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize 2022-04-21 16:01:29 UTC Comment hidden (obsolete)
Comment 33 Martin Renvoize 2022-04-21 16:01:34 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2022-04-21 16:01:38 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize 2022-04-21 16:01:43 UTC Comment hidden (obsolete)
Comment 36 Martin Renvoize 2022-04-21 16:07:04 UTC
This all makes a lot of sense to me.. I can see a case in the future for a distinct table, but I'm not aware of an allow-list on reports yet so that can come later in my opinion.

I wonder slightly about the accessor names in Koha::Patron.. we could just silently encrypt/descrypt and continue to rely on 'secret' as our accessor name.. but that's not a big issue.  I also wonder if the field in general should be clearer.. this secret us specific to this form of on time password for 2FA.. calling is 'secret' is very generic.. but again.. that's from the earlier bug.

As is it.. PQA
Comment 37 Martin Renvoize 2022-04-21 16:16:12 UTC Comment hidden (obsolete)
Comment 38 Martin Renvoize 2022-04-21 16:43:25 UTC
Still working here.. I just found it not working on my checkout and I'm not sure why.. setting back to give me time to work backwards.
Comment 39 Martin Renvoize 2022-04-21 16:47:58 UTC
OK, I spotted it..

I had dropped the config between first and second test.

So.. I think this highlights an issue.. we should probably try/catch to give a better error than the 500 we currently throw on lack of configuration.. or fallback to unencrypted secrets when the key is missing.. but that re-introduces the security issue.

Might even be worth a script to quickly encrypt un-encrypted secrets at upgrade if we don't get this pushed prior to release
Comment 40 Marcel de Rooy 2022-04-22 07:11:34 UTC
Looking here
Comment 41 Marcel de Rooy 2022-04-22 07:36:26 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2022-04-22 07:36:31 UTC Comment hidden (obsolete)
Comment 43 Marcel de Rooy 2022-04-22 07:36:36 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2022-04-22 07:36:41 UTC Comment hidden (obsolete)
Comment 45 Marcel de Rooy 2022-04-22 07:36:47 UTC Comment hidden (obsolete)
Comment 46 Marcel de Rooy 2022-04-22 07:36:51 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2022-04-22 07:36:58 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2022-04-22 07:37:04 UTC Comment hidden (obsolete)
Comment 49 Marcel de Rooy 2022-04-22 07:37:10 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2022-04-22 08:07:26 UTC Comment hidden (obsolete)
Comment 51 Marcel de Rooy 2022-04-22 08:08:14 UTC Comment hidden (obsolete)
Comment 52 Marcel de Rooy 2022-04-22 08:10:47 UTC
Added two follow-ups to address your concerns.
Comment 53 Marcel de Rooy 2022-04-22 08:25:08 UTC
(In reply to Martin Renvoize from comment #36)
> I wonder slightly about the accessor names in Koha::Patron.. we could just
> silently encrypt/descrypt and continue to rely on 'secret' as our accessor
> name.. but that's not a big issue.  I also wonder if the field in general
> should be clearer.. this secret us specific to this form of on time password
> for 2FA.. calling is 'secret' is very generic.. but again.. that's from the
> earlier bug.

Actually, I did that at first and decided to go for another accessor name. This is more transparent.
We could address that (if needed) when we move it out of borrowers.
Comment 54 Martin Renvoize 2022-04-22 08:36:18 UTC Comment hidden (obsolete)
Comment 55 Martin Renvoize 2022-04-22 08:36:23 UTC Comment hidden (obsolete)
Comment 56 Martin Renvoize 2022-04-22 08:36:28 UTC Comment hidden (obsolete)
Comment 57 Martin Renvoize 2022-04-22 08:36:32 UTC Comment hidden (obsolete)
Comment 58 Martin Renvoize 2022-04-22 08:36:37 UTC Comment hidden (obsolete)
Comment 59 Martin Renvoize 2022-04-22 08:36:42 UTC Comment hidden (obsolete)
Comment 60 Martin Renvoize 2022-04-22 08:36:47 UTC Comment hidden (obsolete)
Comment 61 Martin Renvoize 2022-04-22 08:36:51 UTC Comment hidden (obsolete)
Comment 62 Martin Renvoize 2022-04-22 08:36:57 UTC Comment hidden (obsolete)
Comment 63 Martin Renvoize 2022-04-22 08:37:02 UTC Comment hidden (obsolete)
Comment 64 Marcel de Rooy 2022-04-22 08:37:03 UTC Comment hidden (obsolete)
Comment 65 Marcel de Rooy 2022-04-22 08:49:53 UTC Comment hidden (obsolete)
Comment 66 Martin Renvoize 2022-04-22 09:48:58 UTC Comment hidden (obsolete)
Comment 67 Martin Renvoize 2022-04-22 09:49:03 UTC Comment hidden (obsolete)
Comment 68 Martin Renvoize 2022-04-22 09:49:08 UTC Comment hidden (obsolete)
Comment 69 Martin Renvoize 2022-04-22 09:49:12 UTC Comment hidden (obsolete)
Comment 70 Martin Renvoize 2022-04-22 09:49:17 UTC Comment hidden (obsolete)
Comment 71 Martin Renvoize 2022-04-22 09:49:23 UTC Comment hidden (obsolete)
Comment 72 Martin Renvoize 2022-04-22 09:49:27 UTC Comment hidden (obsolete)
Comment 73 Martin Renvoize 2022-04-22 09:49:32 UTC Comment hidden (obsolete)
Comment 74 Martin Renvoize 2022-04-22 09:49:38 UTC Comment hidden (obsolete)
Comment 75 Martin Renvoize 2022-04-22 09:49:43 UTC Comment hidden (obsolete)
Comment 76 Martin Renvoize 2022-04-22 09:49:48 UTC Comment hidden (obsolete)
Comment 77 Martin Renvoize 2022-04-22 09:50:33 UTC
Right, I'm happy with this all now.. PQA.. (except it'll be blocked until the dependency bugs are all PQA too)
Comment 78 Martin Renvoize 2022-04-22 09:57:06 UTC
Selenium tests failing further down the tree so blocked until those have been fixed and PQA'd
Comment 79 Martin Renvoize 2022-04-22 16:05:34 UTC
Hmm, OK.. the selenium failures are now introduced by this bug..

Is there any way to mock the config (encrpytion_key) for a selenium test?
Comment 80 Martin Renvoize 2022-04-25 06:50:54 UTC
OK, so the tests do pass.. However, you need to have the encryption_key config variable set.

We can't easily set this using our Mock libs, as it won't work with the selenium tests.. so I think the solution is to just ensure k-t-d has that variable always set.
Comment 81 Fridolin Somers 2022-05-02 21:15:47 UTC
(In reply to Martin Renvoize from comment #80)
> OK, so the tests do pass.. However, you need to have the encryption_key
> config variable set.
> 
> We can't easily set this using our Mock libs, as it won't work with the
> selenium tests.. so I think the solution is to just ensure k-t-d has that
> variable always set.

Create a pull request on KTD ?
Then I push master right ?
Comment 82 Marcel de Rooy 2022-05-03 11:55:45 UTC
(In reply to Fridolin Somers from comment #81)
> (In reply to Martin Renvoize from comment #80)
> > OK, so the tests do pass.. However, you need to have the encryption_key
> > config variable set.
> > 
> > We can't easily set this using our Mock libs, as it won't work with the
> > selenium tests.. so I think the solution is to just ensure k-t-d has that
> > variable always set.
> 
> Create a pull request on KTD ?
> Then I push master right ?

I do not follow this. As soon as we push this patch, and packaging has been updated accordingly, a new build should be fine imo.
Why do we change the order here and wait for ktd instead??
Comment 83 Martin Renvoize 2022-05-03 13:21:05 UTC
I think Marcel is correct, once pushed k-t-d should get new packages magically and we should get the config option filled.

So, go for it.. :)
Comment 84 Marcel de Rooy 2022-05-03 13:38:22 UTC
Personally, I would not find it an improvement if we should(!) wait for ktd updates on a local koha-conf copy while master would be fine.
Btw the implicit assumption that every Koha dev uses ktd is just not true ;)
Comment 85 Jonathan Druart 2022-05-03 14:24:30 UTC
ktd has its own koha-conf. If the entry is mandatory then it needs to be added there.

ktd is used by Jenkins and so patches have to not break the CI.

I am afraid that at least the 2FA selenium tests will fail.

Note that we are talking about a one-line change patch... If we need it I can write a patch for that.
Comment 86 Marcel de Rooy 2022-05-03 14:32:20 UTC
(In reply to Jonathan Druart from comment #85)
> ktd has its own koha-conf. If the entry is mandatory then it needs to be
> added there.
> 
> ktd is used by Jenkins and so patches have to not break the CI.
> 
> I am afraid that at least the 2FA selenium tests will fail.
> 
> Note that we are talking about a one-line change patch... If we need it I
> can write a patch for that.

My problem is not that one line. It is the principle behind it. ktd should be smarter than this (imo).
Comment 88 Jonathan Druart 2022-05-03 14:33:41 UTC
(In reply to Marcel de Rooy from comment #86)
> (In reply to Jonathan Druart from comment #85)
> > ktd has its own koha-conf. If the entry is mandatory then it needs to be
> > added there.
> > 
> > ktd is used by Jenkins and so patches have to not break the CI.
> > 
> > I am afraid that at least the 2FA selenium tests will fail.
> > 
> > Note that we are talking about a one-line change patch... If we need it I
> > can write a patch for that.
> 
> My problem is not that one line. It is the principle behind it. ktd should
> be smarter than this (imo).

Yes, it's a known problem, see https://gitlab.com/koha-community/koha-testing-docker/-/issues/281
Comment 89 Marcel de Rooy 2022-05-03 14:35:59 UTC
(In reply to Jonathan Druart from comment #88)
> (In reply to Marcel de Rooy from comment #86)
> > (In reply to Jonathan Druart from comment #85)
> > > ktd has its own koha-conf. If the entry is mandatory then it needs to be
> > > added there.
> > > 
> > > ktd is used by Jenkins and so patches have to not break the CI.
> > > 
> > > I am afraid that at least the 2FA selenium tests will fail.
> > > 
> > > Note that we are talking about a one-line change patch... If we need it I
> > > can write a patch for that.
> > 
> > My problem is not that one line. It is the principle behind it. ktd should
> > be smarter than this (imo).
> 
> Yes, it's a known problem, see
> https://gitlab.com/koha-community/koha-testing-docker/-/issues/281

Ha, now we're talking :)
Comment 90 Fridolin Somers 2022-05-03 18:38:49 UTC
Thanks a lot, I push.
Sorry for not knowing much from our process.
Comment 91 Fridolin Somers 2022-05-03 18:42:22 UTC
I've squashed :
  Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test
With
  Bug 28998: Adjust Selenium test
Comment 92 Fridolin Somers 2022-05-03 18:55:51 UTC
Needs to add Crypt::CBC dependency
Comment 93 Martin Renvoize 2022-05-04 12:30:50 UTC
Dependency is actually OK... we've already got it listed in the cpanfile on this bug and the version we're pinned to is the latest version that all our debians ship.

There's another bug requesting a newer version which would allow us to tighten up security a little more.. but we don't need anything else doing here and now :)
Comment 94 Fridolin Somers 2022-05-04 15:20:36 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 95 Jonathan Druart 2022-05-05 11:28:45 UTC
Created attachment 134636 [details] [review]
Bug 28998: Fix Auth.
Comment 96 Tomás Cohen Arazi 2022-05-05 11:41:39 UTC
Created attachment 134637 [details] [review]
Bug 28998: (QA follow-up) Silence some useless warnings

This patch fixes the tests and also silences some useless warnings about
REMOTE_ADDRESS not being defined.

A test is added for a legit warning that is thrown.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 97 Tomás Cohen Arazi 2022-05-05 11:43:52 UTC
@RM: Follow-ups pushed to fix Jenkins.
Comment 98 Martin Renvoize 2022-05-05 12:23:07 UTC
Comment on attachment 134637 [details] [review]
Bug 28998: (QA follow-up) Silence some useless warnings

Review of attachment 134637 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Auth.t
@@ +151,4 @@
>          }
>  
>          t::lib::Mocks::mock_preference( 'TwoFactorAuthentication', 0 );
> +        $patron->encode_secret('one_secret');

It feels weird here to require a secret set when TwoFactorAuthentication is disabled?
Comment 99 Marcel de Rooy 2022-05-06 06:32:35 UTC
(In reply to Martin Renvoize from comment #98)
> Comment on attachment 134637 [details] [review] [review]
> Bug 28998: (QA follow-up) Silence some useless warnings
> 
> Review of attachment 134637 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: t/db_dependent/Auth.t
> @@ +151,4 @@
> >          }
> >  
> >          t::lib::Mocks::mock_preference( 'TwoFactorAuthentication', 0 );
> > +        $patron->encode_secret('one_secret');
> 
> It feels weird here to require a secret set when TwoFactorAuthentication is
> disabled?

Thats true. I will move it a bit lower. Submitting a follow-up.
Comment 100 Marcel de Rooy 2022-05-06 06:36:03 UTC
Created attachment 134671 [details] [review]
Bug 28998: (follow-up) Adjust the Auth.t fix

Move the encode_secret call where we need it (when pref is enabled
and patron switches to 2FA).
Mock the koha-conf encryption_key.
Disable 2FA when exiting subtest.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 101 Marcel de Rooy 2022-05-06 06:39:07 UTC
Martin: Please have a look again.
@RM: Push last patch only.
Comment 102 Martin Renvoize 2022-05-06 09:00:06 UTC
Looking good, tests passing perfectly still.

Add my SO line and pushed to master.
Comment 103 Tomás Cohen Arazi 2022-06-06 17:40:35 UTC
Hi, on U22 I found this warnings:

14:36:09 koha_1       | WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better.
14:36:09 koha_1       | Pass -nodeprecate=>1 to inhibit this message.
14:36:09 koha_1       |  at /kohadevbox/koha/Koha/Patron.pm line 2167.
14:36:09 koha_1       | WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better.
14:36:09 koha_1       | Pass -nodeprecate=>1 to inhibit this message.
14:36:09 koha_1       |  at /kohadevbox/koha/Koha/Patron.pm line 2167.
14:36:09 koha_1       | WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better.
14:36:09 koha_1       | Pass -nodeprecate=>1 to inhibit this message.
14:36:09 koha_1       |  at /kohadevbox/koha/Koha/Patron.pm line 2167.
Comment 104 Martin Renvoize 2022-06-06 18:56:22 UTC
See bug 30594 ;)
Comment 105 David Cook 2023-05-18 06:33:52 UTC
(In reply to Jonathan Druart from comment #4)
> How do we generate the new config entry on upgrade?

I don't think this was ever done... which has been driving me a little crazy lately 😅