Bug 30649 - Vendor EDI account passwords should be encrypted in the database
Summary: Vendor EDI account passwords should be encrypted in the database
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 28998
Blocks: 34033
  Show dependency treegraph
 
Reported: 2022-04-28 18:28 UTC by Kyle M Hall
Modified: 2023-12-28 20:47 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.08,22.11.07


Attachments
Bug 30649 - Add ability to encrypt data in Koha, use for EDI vendor accounts (12.11 KB, patch)
2022-04-28 18:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30649 - Add ability to encrypt data in Koha, use for EDI vendor accounts (12.04 KB, patch)
2022-04-28 18:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30649 - Add ability to encrypt data in Koha, use for EDI vendor accounts (12.02 KB, patch)
2022-04-28 18:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30649 - Add ability to encrypt data in Koha, use for EDI vendor accounts (12.11 KB, patch)
2022-04-28 18:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30649: Vendor EDI account passwords should be encrypted in the database (7.43 KB, patch)
2022-10-18 13:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30649: Vendor EDI account passwords should be encrypted in the database (7.55 KB, patch)
2022-10-18 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30649: Vendor EDI account passwords should be encrypted in the database (7.60 KB, patch)
2022-10-30 19:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 30649: Vendor EDI account passwords should be encrypted in the database (7.67 KB, patch)
2022-11-02 15:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30649: Vendor EDI account passwords should be encrypted in the database (7.51 KB, patch)
2022-11-29 14:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30649: Vendor EDI account passwords should be encrypted in the database (7.51 KB, patch)
2022-11-29 14:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30649: (QA follow-up) Switch password field to mediumtext (1.79 KB, patch)
2022-11-29 14:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30649: (follow-up) Improve database update (2.07 KB, patch)
2023-06-07 11:43 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2022-04-28 18:28:54 UTC
Right now, we store passwords for external services in plain text in the database. It would be good to have the option to encrypt those passwords.
Comment 1 Kyle M Hall 2022-04-28 18:31:54 UTC
Created attachment 134298 [details] [review]
Bug 30649 - Add ability to encrypt data in Koha, use for EDI vendor accounts

Right now, we store passwords for external services in plain text in the database. It would be good to have the option to encrypt those passwords.

Test Plan:
1) prove t/Crypt.t
2) Create a new vendor edi account
3) Note the password is encrypted
4) Update the password for an existing EDI vendor account
5) Note the password is now encrypted in the database
6) Stretch goal, set up an FTP server, send an EDI order to the FTP
   server where the vendor password is encrypted.
Comment 2 Kyle M Hall 2022-04-28 18:33:52 UTC
Created attachment 134300 [details] [review]
Bug 30649 - Add ability to encrypt data in Koha, use for EDI vendor accounts

Right now, we store passwords for external services in plain text in the database. It would be good to have the option to encrypt those passwords.

Test Plan:
1) prove t/Crypt.t
2) Create a new vendor edi account
3) Note the password is encrypted
4) Update the password for an existing EDI vendor account
5) Note the password is now encrypted in the database
6) Stretch goal, set up an FTP server, send an EDI order to the FTP
   server where the vendor password is encrypted.
Comment 3 Kyle M Hall 2022-04-28 18:36:46 UTC
Created attachment 134301 [details] [review]
Bug 30649 - Add ability to encrypt data in Koha, use for EDI vendor accounts

Right now, we store passwords for external services in plain text in the database. It would be good to have the option to encrypt those passwords.

Test Plan:
1) prove t/Crypt.t
2) Create a new vendor edi account
3) Note the password is encrypted
4) Update the password for an existing EDI vendor account
5) Note the password is now encrypted in the database
6) Stretch goal, set up an FTP server, send an EDI order to the FTP
   server where the vendor password is encrypted.
Comment 4 Kyle M Hall 2022-04-28 18:47:07 UTC
Created attachment 134302 [details] [review]
Bug 30649 - Add ability to encrypt data in Koha, use for EDI vendor accounts

Right now, we store passwords for external services in plain text in the database. It would be good to have the option to encrypt those passwords.

Test Plan:
1) prove t/Crypt.t
2) Create a new vendor edi account
3) Note the password is encrypted
4) Update the password for an existing EDI vendor account
5) Note the password is now encrypted in the database
6) Stretch goal, set up an FTP server, send an EDI order to the FTP
   server where the vendor password is encrypted.
Comment 5 David Cook 2022-04-28 23:34:45 UTC
I think Jonathan has already almost gotten an encryption module into Koha with Bug 28998, so this would arguably be a duplicate?
Comment 6 Kyle M Hall 2022-04-29 13:35:50 UTC
(In reply to David Cook from comment #5)
> I think Jonathan has already almost gotten an encryption module into Koha
> with Bug 28998, so this would arguably be a duplicate?

I was not aware of that bug! Once that's pushed, I'll rework this bug to use that instead of making it's own module.
Comment 7 Marcel de Rooy 2022-04-29 14:59:56 UTC
(In reply to Kyle M Hall from comment #6)
> (In reply to David Cook from comment #5)
> > I think Jonathan has already almost gotten an encryption module into Koha
> > with Bug 28998, so this would arguably be a duplicate?
> 
> I was not aware of that bug! Once that's pushed, I'll rework this bug to use
> that instead of making it's own module.

Changing status
Comment 8 Victor Grousset/tuxayo 2022-10-18 03:32:28 UTC
I don't get how to encrypt a password to an external service and still be able to use the external service. Does that mean Koha can in full autonomy decrypt it?
Comment 9 Kyle M Hall 2022-10-18 10:54:25 UTC
(In reply to Victor Grousset/tuxayo from comment #8)
> I don't get how to encrypt a password to an external service and still be
> able to use the external service. Does that mean Koha can in full autonomy
> decrypt it?

Yes, we store a key in the koha konf file for encryption and decryption. I need to rebase this patch to use the work from Bug 28998.
Comment 10 Kyle M Hall 2022-10-18 13:36:47 UTC
Created attachment 142056 [details] [review]
Bug 30649: Vendor EDI account passwords should be encrypted in the database

We are storing edi vendor acccount passwords in clear text in the
database. Now that Koha has the Koha::Encryption module, we should
use that to encrypt passwords for all existing and new EDI accounts.

Test Plan:
1) Apply this patch
2) Create one or more EDI vendor accounts
3) Run a report to view the account passwords, note they are in clear
   text
4) Run updatedatabase.pl
5) Re-run the report, account passwords should be encrypted now
6) Edit a vendor EDI account, note you can still view and update the
   password for an account
Comment 11 Kyle M Hall 2022-10-18 13:45:44 UTC
Created attachment 142057 [details] [review]
Bug 30649: Vendor EDI account passwords should be encrypted in the database

We are storing edi vendor acccount passwords in clear text in the
database. Now that Koha has the Koha::Encryption module, we should
use that to encrypt passwords for all existing and new EDI accounts.

Test Plan:
1) Apply this patch
2) Create one or more EDI vendor accounts
3) Run a report to view the account passwords, note they are in clear
   text
4) Run updatedatabase.pl
5) Re-run the report, account passwords should be encrypted now
6) Edit a vendor EDI account, note you can still view and update the
   password for an account
Comment 12 David Nind 2022-10-30 19:40:09 UTC
Created attachment 142820 [details] [review]
Bug 30649: Vendor EDI account passwords should be encrypted in the database

We are storing edi vendor acccount passwords in clear text in the
database. Now that Koha has the Koha::Encryption module, we should
use that to encrypt passwords for all existing and new EDI accounts.

Test Plan:
1) Apply this patch
2) Create one or more EDI vendor accounts
3) Run a report to view the account passwords, note they are in clear
   text
4) Run updatedatabase.pl
5) Re-run the report, account passwords should be encrypted now
6) Edit a vendor EDI account, note you can still view and update the
   password for an account

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2022-10-30 19:44:17 UTC
Testing notes (using koha-testing-docker):

1. Enable EDIFACT system preference
2. Creating EDI vendor accounts: Administration > Acquisition parameters > EDI accounts
3. Before applying the patch, I added an EDI vendor account (to test that existing accounts are updated)
3. Step 3 - SQL for report: select * from vendor_edi_accounts
4. Step 4 - after the databaseupdate: I ran flush_memcached and restart_all, and cleared the browser cache
Comment 14 Victor Grousset/tuxayo 2022-11-01 05:08:39 UTC
(In reply to Kyle M Hall from comment #9)
> (In reply to Victor Grousset/tuxayo from comment #8)
> > I don't get how to encrypt a password to an external service and still be
> > able to use the external service. Does that mean Koha can in full autonomy
> > decrypt it?
> 
> Yes, we store a key in the koha konf file for encryption and decryption. I
> need to rebase this patch to use the work from Bug 28998.

Ok IIUC the security value doesn't come from encryption but from having the date out of the DB. So a simple SQL injection can't get it.
Is there any gain compared to just storing the passwords into koha-conf.xml directly? 
(hum, maybe Koha can't write to that file and that would need a separate file)
Like is it a plausible attack scenario to be able to read the file but not the DB? That when needing both would help.
Comment 15 Martin Renvoize 2022-11-02 15:45:34 UTC
Created attachment 142989 [details] [review]
Bug 30649: Vendor EDI account passwords should be encrypted in the database

We are storing edi vendor acccount passwords in clear text in the
database. Now that Koha has the Koha::Encryption module, we should
use that to encrypt passwords for all existing and new EDI accounts.

Test Plan:
1) Apply this patch
2) Create one or more EDI vendor accounts
3) Run a report to view the account passwords, note they are in clear
   text
4) Run updatedatabase.pl
5) Re-run the report, account passwords should be encrypted now
6) Edit a vendor EDI account, note you can still view and update the
   password for an account

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-11-02 15:51:09 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> (In reply to Kyle M Hall from comment #9)
> > (In reply to Victor Grousset/tuxayo from comment #8)
> > > I don't get how to encrypt a password to an external service and still be
> > > able to use the external service. Does that mean Koha can in full autonomy
> > > decrypt it?
> > 
> > Yes, we store a key in the koha konf file for encryption and decryption. I
> > need to rebase this patch to use the work from Bug 28998.
> 
> Ok IIUC the security value doesn't come from encryption but from having the
> date out of the DB. So a simple SQL injection can't get it.
> Is there any gain compared to just storing the passwords into koha-conf.xml
> directly? 
> (hum, maybe Koha can't write to that file and that would need a separate
> file)
> Like is it a plausible attack scenario to be able to read the file but not
> the DB? That when needing both would help.

The value does come from the encryption.  If the database is somehow compromised (for example, someone accidentally shares a backup.. it could be as simple as that).. by having the data in the databawse encrypted the nafarious actor doesn't have something useful to them.. They still need to hack the machine to get ahold of the key (from the conf file) and/or read the code to understand what sort of algorithm is used.

So this closes one door.. if they have full access to the server, they have all the elements they need to access the plaintext credentials.. but the improvement here is that they now have to have that full access rather than just a db dump.
Comment 17 Martin Renvoize 2022-11-02 15:55:46 UTC
Still contemplating QA here.. the code works as expected and I'm happy with the implementation as a whole.

However.. I'm not so sure about the in place database upgrade... we tend to try and steer away from referencing Koha modules from within the atomicupdates in case there's a change to said module down the line.  That said.. that's not a blocker for me, just a consideration.  When we upgraded from SHA to BCrypt for user account hashing we added a layer inside the codebase to upgrade the hash on first access I seem to recall.

My other pondering is around what happens if/when an admin wants to change the encryption key for the server.. that's out of scope for this particular bug, but I feel like we should have an option for it somewhere.. either a script to update encrypted data's to use the new key (given the old and new key as input) or a way to defined the keys as an array and upgrade on access or something like that.
Comment 18 Martin Renvoize 2022-11-02 16:06:36 UTC
OK.. I decided to open another bug for my thoughts on key change..

I'll pass this one but highlight to the RM that we may need to rethink the DB update.
Comment 19 David Cook 2022-11-02 23:04:02 UTC
(In reply to Martin Renvoize from comment #17)
> When we upgraded
> from SHA to BCrypt for user account hashing we added a layer inside the
> codebase to upgrade the hash on first access I seem to recall.

We were upgrading from MD5 hashes to BCrypt hashes, which were easy to differentiate, since the BCrypt hashes started with "$2a$08$".

We also had the user input so you could always compare hashes. In this case with the decryption I don't think there's any way to know whether or not you got a valid decrypted value (unless the encryption module throws an exception)...
Comment 20 David Cook 2022-11-02 23:05:20 UTC
(In reply to Martin Renvoize from comment #18)
> OK.. I decided to open another bug for my thoughts on key change..
> 
> I'll pass this one but highlight to the RM that we may need to rethink the
> DB update.

I've commented on Bug 32078 with what I think would be a fairly straightforward approach to allow key rotation.
Comment 21 Victor Grousset/tuxayo 2022-11-07 05:31:42 UTC
(In reply to Martin Renvoize from comment #16)
> The value does come from the encryption.  If the database is somehow
> compromised (for example, someone accidentally shares a backup.. it could be
> as simple as that).. by having the data in the databawse encrypted the
> nafarious actor doesn't have something useful to them.. They still need to
> hack the machine to get ahold of the key (from the conf file) and/or read
> the code to understand what sort of algorithm is used.

That's why I wondered if there was any gain compared to just storing the passwords into koha-conf.xml directly? (or another file)

The question would have been more relevant on bug 28998 now that such a mechanism is implemented, the work is done and it's not very hard to use on any data to be protected from SQL injection or accidental backup publication.
Comment 22 Kyle M Hall 2022-11-07 14:59:13 UTC
(In reply to Victor Grousset/tuxayo from comment #21)
> That's why I wondered if there was any gain compared to just storing the
> passwords into koha-conf.xml directly? (or another file)

Simply put, imo, that would mean librarians could no longer update that data without help from the server administrator, making their jobs more difficult.
Comment 23 Katrin Fischer 2022-11-09 09:56:40 UTC
It might also hinder a a quick desaster recovery to a different server? At least something more to think about for backups etc.
Comment 24 David Cook 2022-11-09 22:48:01 UTC
(In reply to Katrin Fischer from comment #23)
> It might also hinder a a quick desaster recovery to a different server? At
> least something more to think about for backups etc.

With the encryption key in koha-conf.xml, they wouldn't be able to decrypt the encrypted passwords in the database either. 

(In reply to Kyle M Hall from comment #22)
> (In reply to Victor Grousset/tuxayo from comment #21)
> > That's why I wondered if there was any gain compared to just storing the
> > passwords into koha-conf.xml directly? (or another file)
> 
> Simply put, imo, that would mean librarians could no longer update that data
> without help from the server administrator, making their jobs more difficult.

Agreed with Kyle. There needs to be a balance between security and functionality/convenience.
Comment 25 Katrin Fischer 2022-11-09 22:49:24 UTC
(In reply to David Cook from comment #24)
> (In reply to Katrin Fischer from comment #23)
> > It might also hinder a a quick desaster recovery to a different server? At
> > least something more to think about for backups etc.
> 
> With the encryption key in koha-conf.xml, they wouldn't be able to decrypt
> the encrypted passwords in the database either. 

True, so something to keep in mind then for the sysadmins?
Comment 26 David Cook 2022-11-10 00:22:06 UTC
(In reply to Katrin Fischer from comment #25)
> (In reply to David Cook from comment #24)
> > (In reply to Katrin Fischer from comment #23)
> > > It might also hinder a a quick desaster recovery to a different server? At
> > > least something more to think about for backups etc.
> > 
> > With the encryption key in koha-conf.xml, they wouldn't be able to decrypt
> > the encrypted passwords in the database either. 
> 
> True, so something to keep in mind then for the sysadmins?

Yeah, I mean there's other bits of config that this happens for too. I use the "OAI-PMH:ConfFile" syspref, and if you just restore the database dump without including the server-side file, it'll break the OAI.

So sysadmins really need to keep in mind that the database and server-side config need to be restored together.
Comment 27 Victor Grousset/tuxayo 2022-11-27 04:04:48 UTC
(In reply to Kyle M Hall from comment #22)
> (In reply to Victor Grousset/tuxayo from comment #21)
> > That's why I wondered if there was any gain compared to just storing the
> > passwords into koha-conf.xml directly? (or another file)
> 
> Simply put, imo, that would mean librarians could no longer update that data
> without help from the server administrator, making their jobs more difficult.

Hence the earlier «maybe Koha can't write to that file and that would need a separate file»


(In reply to David Cook from comment #26)
> So sysadmins really need to keep in mind that the database and server-side
> config need to be restored together.

Ah yes, so actually encrypting data in the DB does not protect from a backup leak. (I wrongly said that earlier) Since a backup should have the config files.
Comment 28 Jonathan Druart 2022-11-29 09:41:22 UTC
I don't know how this is relevant, but borrowers.secret is MEDIUMTEXT and you are using VARCHAR(256) here.

By the way, 256? Typo for 255?
Comment 29 Kyle M Hall 2022-11-29 14:30:14 UTC
Created attachment 144323 [details] [review]
Bug 30649: Vendor EDI account passwords should be encrypted in the database

We are storing edi vendor acccount passwords in clear text in the
database. Now that Koha has the Koha::Encryption module, we should
use that to encrypt passwords for all existing and new EDI accounts.

Test Plan:
1) Apply this patch
2) Create one or more EDI vendor accounts
3) Run a report to view the account passwords, note they are in clear
   text
4) Run updatedatabase.pl
5) Re-run the report, account passwords should be encrypted now
6) Edit a vendor EDI account, note you can still view and update the
   password for an account

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Kyle M Hall 2022-11-29 14:30:54 UTC
Created attachment 144324 [details] [review]
Bug 30649: Vendor EDI account passwords should be encrypted in the database

We are storing edi vendor acccount passwords in clear text in the
database. Now that Koha has the Koha::Encryption module, we should
use that to encrypt passwords for all existing and new EDI accounts.

Test Plan:
1) Apply this patch
2) Create one or more EDI vendor accounts
3) Run a report to view the account passwords, note they are in clear
   text
4) Run updatedatabase.pl
5) Re-run the report, account passwords should be encrypted now
6) Edit a vendor EDI account, note you can still view and update the
   password for an account

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Kyle M Hall 2022-11-29 14:31:07 UTC
Created attachment 144325 [details] [review]
Bug 30649: (QA follow-up) Switch password field to mediumtext
Comment 32 Kyle M Hall 2022-11-29 14:33:52 UTC
(In reply to Jonathan Druart from comment #28)
> I don't know how this is relevant, but borrowers.secret is MEDIUMTEXT and
> you are using VARCHAR(256) here.
> 
> By the way, 256? Typo for 255?

I like the idea of using mediumtext much better for encrypted data fields than varchar.
Comment 33 Tomás Cohen Arazi 2023-05-15 21:17:01 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 34 Pedro Amorim 2023-06-02 11:53:17 UTC
Nice work everyone!

Pushed to 22.11.x for next release
Comment 35 Jonathan Druart 2023-06-06 06:59:13 UTC
Reported on the ML, it's breaking the upgrade
  ERROR - Exception 'Koha::Exceptions::MissingParameter' thrown 'No encryption_key in koha-conf.xml'

We should not use Koha module in db revs.

Here we have Koha::Database that can be replaced easily with $dbh, however there is no good solution for Koha::Encryption.

We could maybe require the module only if there are rows in vendor_edi_accounts?
Comment 36 David Cook 2023-06-06 23:10:45 UTC
(In reply to Jonathan Druart from comment #35)
> We could maybe require the module only if there are rows in
> vendor_edi_accounts?

Sounds reasonable to me
Comment 37 Martin Renvoize 2023-06-07 11:43:19 UTC
Created attachment 152097 [details] [review]
Bug 30649: (follow-up) Improve database update

This patch implements the proposed switch to use the standard DB handle
and only require Koha::Encryption if necessary.
Comment 38 Martin Renvoize 2023-06-07 11:43:50 UTC
I threw that together.. needs testing though.
Comment 39 David Cook 2023-06-08 01:19:33 UTC
Would this work without the encryption_key being set though? Might need to add a catch for that
Comment 40 Jonathan Druart 2023-06-08 06:53:36 UTC
(In reply to David Cook from comment #39)
> Would this work without the encryption_key being set though? Might need to
> add a catch for that

I don't think we should anything more, with bug 33934 the output of the upgrade process will tell what went wrong.
Comment 41 Tomás Cohen Arazi 2023-06-09 13:29:21 UTC
Follow-up pushed to master. Backporting to 23.05 as well.
Comment 42 Emmi Takkinen 2023-06-16 11:23:58 UTC
There's a typo in atomicupdate file. 

"SELECT * FROM vendor_edit_accounts" should be "SELECT * FROM vendor_edi_accounts"
Comment 43 Emmi Takkinen 2023-06-16 11:25:59 UTC
Also there's no table edi_vendor_accounts.
Comment 44 Emmi Takkinen 2023-06-16 11:30:38 UTC
Oh and also SET password = $password needs guotation marks around $password :D Ran into this while updating my test database. After fixing them manually update proceeded without problems.
Comment 45 Matt Blenkinsop 2023-07-12 09:41:45 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x