Bug 40349 - Use pbkdf instead of the default openssl1 for Crypt::CBC
Summary: Use pbkdf instead of the default openssl1 for Crypt::CBC
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
: 34697 (view as bug list)
Depends on: 40348
Blocks:
  Show dependency treegraph
 
Reported: 2025-07-11 12:35 UTC by Jonathan Druart
Modified: 2025-07-18 10:10 UTC (History)
3 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-07-11 12:35:56 UTC
02:28:36 koha_1       | WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better.
02:28:36 koha_1       | Pass -nodeprecate=>1 to inhibit this message.
02:28:36 koha_1       |  at /kohadevbox/koha/Koha/Patron.pm line 3216.

From the doc
https://metacpan.org/pod/Crypt::CBC

-pbkdf         The passphrase-based key derivation function used to derive
                  the encryption key and initialization vector from the
                  provided passphrase. For backward compatibility, Crypt::CBC
                  will default to "opensslv1", but it is recommended to use
                  the standard "pbkdf2"algorithm instead. If you wish to interoperate
                  with OpenSSL, be aware that different versions of the software
                  support a series of derivation functions.
Comment 1 David Cook 2025-07-14 00:18:45 UTC
Sounds good.

Of course, it means that we'll have to re-encrypt all the database contents. 

But that could be a blessing in disguise!

At the moment, we just *know* which fields are encrypted based on the code, and how they're encrypted, and we have a single "encryption_key" in koha-conf.xml. But the more time goes on the harder this is going to get to manage.

We should update the database schema to store JSON metadata alongside the encrypted values. This is a normal practice these days. I'd suggest things like "kid" (or key_id), "algorithm" (e.g. AES-256-CBC in the case of Koha::Encryption currently), "pbkdf" (e.g. opensslv1 or pbkdf2), "iterations" (e.g. 10,000 - current default value of Crypt::CBC). In theory, we'd want IV and salt as well, but Crypt::CBC stores them as headers in the ciphertext/encrypted value, so we probably don't need to worry about those yet. 

Instead of "algorithm", we could break it down into "cipher" (e.g. AES), "keysize" (e.g. 256 bits which is a default value from Crypt::Cipher::AES), and "mode" (e.g. CBC). 

I'd then say we should add an attribute to "encryption_key" in koha-conf.xml. Since we don't currently programmatically update koha-conf.xml, I'd say we just store a value of "default" for kid for existing data.
Comment 2 David Cook 2025-07-14 00:21:57 UTC
Once we've updated the database schema to reflect the reality of the encryption options being used, we can then do scripted re-encryptions of the data. 

By storing key ID, we also create the opportunity to rotate encryption keys and add the ability to use separate keys for separate tasks.
Comment 3 David Cook 2025-07-14 00:25:04 UTC
(In reply to David Cook from comment #1)
> We should update the database schema to store JSON metadata alongside the
> encrypted values.

Recently, I've been involved in a number of Keycloak system upgrades which have involved re-hashing of passwords, and storing the crypto options next to the values is how they're able to do it. It's also how I was able to work out some bugs in their re-hashing process heh.
Comment 4 David Cook 2025-07-15 02:53:58 UTC
Noticed that I've been set as the assignee but I don't think that I'll have time to work on it at this time.
Comment 5 Jonathan Druart 2025-07-18 10:10:44 UTC
*** Bug 34697 has been marked as a duplicate of this bug. ***