Bug 31059

Summary: encryption_key config entry not generated when upgrading
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Bugs List <koha-bugs>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: cslone, david, dcook, m.de.rooy, victor
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28911
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22857
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30649
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33934
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31180
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 28786    
Bug Blocks:    

Description Jonathan Druart 2022-06-28 12:45:21 UTC
It's required for 2FA (from bug 28786) but it's not added to koha-conf.xml when upgrading.
Comment 1 Chris Slone 2022-11-28 21:59:52 UTC
Confirming that this does not get added to the koha-conf.xml. For anyone looking for what to add it would be:

 <!-- Encryption key for crypted password or sensitive data -->
 <encryption_key>__ENCRYPTION_KEY__</encryption_key>
Comment 2 David Cook 2022-11-28 23:37:06 UTC
Yeah, this is an annoyance. The same problem exists for quite a few different koha-conf.xml options :/.
Comment 3 Katrin Fischer 2022-11-29 07:59:50 UTC
We could add something to the about page and future release notes here.
Comment 4 Jonathan Druart 2022-11-29 09:37:10 UTC
We need to script that add missing entries. It could be a simple (perl) misc script that is called at the end of the debian upgrade procedure.
Comment 5 Chris Slone 2022-11-29 14:14:43 UTC
I think a script like that would be great. Documentation would definitely help too, as the tags for wrapping the entry, or even the need for changing the config file at all, wasn't really in the release notes for the new feature (not that I saw, anyhow).
Comment 6 David Cook 2022-12-06 03:11:18 UTC
(In reply to Jonathan Druart from comment #4)
> We need to script that add missing entries. It could be a simple (perl) misc
> script that is called at the end of the debian upgrade procedure.

I added a sample one on bug 28911 that I used as a once-off but it would need work to be used regularly like that.
Comment 7 Jonathan Druart 2024-09-04 07:57:46 UTC
And... Why not simply doing it in a db rev? Fail the upgrade until the key exists in the config: we provide info about how to generate the key and add it to the config. A people doing the Koha upgrade is supposed to know how to run a command and edit a file on the server.
Comment 8 Katrin Fischer 2024-09-04 12:49:47 UTC
(In reply to Jonathan Druart from comment #7)
> And... Why not simply doing it in a db rev? Fail the upgrade until the key
> exists in the config: we provide info about how to generate the key and add
> it to the config. A people doing the Koha upgrade is supposed to know how to
> run a command and edit a file on the server.

Hm, I hope noone does a major update unattended, but they might get stuck on this. Also, you'd need to restart things (Plack, Apache?) would the update continue fine from there?
Comment 9 David Cook 2024-09-05 02:39:50 UTC
(In reply to Jonathan Druart from comment #7)
> And... Why not simply doing it in a db rev? Fail the upgrade until the key
> exists in the config: we provide info about how to generate the key and add
> it to the config. A people doing the Koha upgrade is supposed to know how to
> run a command and edit a file on the server.

I think this would cause a lot of headaches, but it's an interesting point.

I still like the idea of a machine-generated config file that we can update (for example via a db rev). 

Or... koha-conf.xml becomes a generated file. We can have a separate file for local updates, and it can be used with program logic to generate the koha-conf.xml

That could automatically keep koha-conf.xml up-to-date.
Comment 10 David Cook 2024-09-05 02:43:45 UTC
I have a Catalyst app that uses Catalyst::Plugin::ConfigLoader (and Config::JFDI for non-Catalyst purposes).

It's great because I have a base config.yaml file which I manage in version control, and then each instance has its own config_local.yaml file. These then get merged together with the local file overwriting the base file at app startup time.

That said, that doesn't have a mechanism either for auto-generated values. 

But if I wanted to add auto-generated values, I'd probably introduce them via that base config.yaml file. 

--

Anyway, just my 2 cents
Comment 11 David Cook 2024-09-05 03:11:31 UTC
(One last one... I'd love to split out the Zebra config from the rest of koha-conf.xml. The majority of koha-conf.xml is actually just Zebra config. As we embrace Elasticsearch more and more, it's feeling sillier and sillier to have most of koha-conf.xml be dedicated to Zebra...)
Comment 12 Jonathan Druart 2024-09-05 07:05:52 UTC
What I suggested with the db rev was to have a quick solution for this problem. We can of course rethink the whole thing but it's 1 or 2 hours of work vs... X days :)

(In reply to Katrin Fischer from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > And... Why not simply doing it in a db rev? Fail the upgrade until the key
> > exists in the config: we provide info about how to generate the key and add
> > it to the config. A people doing the Koha upgrade is supposed to know how to
> > run a command and edit a file on the server.
> 
> Hm, I hope noone does a major update unattended, but they might get stuck on
> this. Also, you'd need to restart things (Plack, Apache?) would the update
> continue fine from there?

Yes, this would be part of the "info" we will display:
Generate the key using this command, copy the output to the koha-conf into the correct tag, restart everything, try the update again.
Comment 13 David Cook 2024-09-06 01:04:28 UTC
(In reply to Jonathan Druart from comment #12)
> What I suggested with the db rev was to have a quick solution for this
> problem. We can of course rethink the whole thing but it's 1 or 2 hours of
> work vs... X days :)

It's not a practical solution for many Koha implementers though.
Comment 14 Jonathan Druart 2024-09-09 11:38:17 UTC
(In reply to David Cook from comment #13)
> (In reply to Jonathan Druart from comment #12)
> > What I suggested with the db rev was to have a quick solution for this
> > problem. We can of course rethink the whole thing but it's 1 or 2 hours of
> > work vs... X days :)
> 
> It's not a practical solution for many Koha implementers though.

Are we stuck then?

I wanted to suggest an easy to implement solution for bug 35970.
Comment 15 Katrin Fischer 2024-09-09 11:46:17 UTC
I feel having the database update stuck doesn't feel right. Printing and info and adding to the about page would be preferred... in case of the JS files I have commented there. I guess the problem is, that we might break things too much to check the about page?
Comment 16 Victor Grousset/tuxayo 2024-10-03 22:24:28 UTC
IIRC <encryption_key> was outside of the DB so 3rd party service passwords and sensitive data would be safe against a DB only leak (like with an SQL injection).

But for the purpose of asset versioning without leaking the version of Koha, would it work to have another encryption key in the DB for that use? It doesn't matter much if it gets eventually stolen with the DB.

----

Something else:
I might have missed if there is this possibility: can we count for most cases on koha-conf.xml being writable by the update process? So <encryption_key> could be populated there and the rest of the cases without write permission would need manual intervention for the upgrade. Assuming there are, it might not be that bad to have a major upgrade needed a manual intervention. Since switching the update source would already be a manual intervention.

A bonus is that every install would have an out of DB key so that opens the possibility of using it in password storage so an SQL injection would have absolutely no chance in being compromised. Whereas today, even with salt and heavy hashing algorithms, It's feasible to test at least hundreds of thousands of passwords from dictionaries of the most popular ones.

But that whole thing relies on counting for most cases on koha-conf.xml being writable.
There are signs that it might be writable. «Or... koha-conf.xml becomes a generated file»
Comment 17 David Cook 2024-10-09 03:36:26 UTC
(In reply to Victor Grousset/tuxayo from comment #16)
> But that whole thing relies on counting for most cases on koha-conf.xml
> being writable.
> There are signs that it might be writable. «Or... koha-conf.xml becomes a
> generated file»

In koha-testing-docker koha-conf.xml looks like it's writeable by the owner and the owner is the Koha user.

On my servers, it looks like koha-conf.xml is owned by root. 

According to debian/scripts/koha-create it should be owned by root...

So that's fun.
Comment 18 Victor Grousset/tuxayo 2024-10-10 02:46:21 UTC
Thanks for checking David.

> According to debian/scripts/koha-create

So these are regular Debian or Ubuntu package installs?
These are not the RHEL/CentOS installs with homemade packaging you had at some point?
Comment 19 David Cook 2024-10-14 03:07:07 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> Thanks for checking David.
> 
> > According to debian/scripts/koha-create
> 
> So these are regular Debian or Ubuntu package installs?
> These are not the RHEL/CentOS installs with homemade packaging you had at
> some point?

Yeah, recent Debian package installs. (That said, there could've been sysadmin on top that changed things later. I haven't tested actually creating a new branch on a test server.)
Comment 20 David Cook 2024-10-30 23:48:52 UTC
Let's follow this up on bug 31180