|
Description
Kyle M Hall (khall)
2022-04-28 18:28: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. 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. 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. 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. I think Jonathan has already almost gotten an encryption module into Koha with Bug 28998, so this would arguably be a duplicate? (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. (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 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? (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. 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 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 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> 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 (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. 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> (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. 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. 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. (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)... (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. (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. (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. It might also hinder a a quick desaster recovery to a different server? At least something more to think about for backups etc. (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. (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? (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. (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. 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? 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> 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> Created attachment 144325 [details] [review] Bug 30649: (QA follow-up) Switch password field to mediumtext (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. Pushed to master for 23.05. Nice work everyone, thanks! Nice work everyone! Pushed to 22.11.x for next release 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? (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 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. I threw that together.. needs testing though. Would this work without the encryption_key being set though? Might need to add a catch for that (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. Follow-up pushed to master. Backporting to 23.05 as well. There's a typo in atomicupdate file. "SELECT * FROM vendor_edit_accounts" should be "SELECT * FROM vendor_edi_accounts" Also there's no table edi_vendor_accounts. 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. Nice work everyone! Pushed to oldstable for 22.11.x |