Bug 20929 - koha-dump should not pass a --password argument to mysqldump
Summary: koha-dump should not pass a --password argument to mysqldump
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: Main
Hardware: All Linux
: P5 - low normal (vote)
Assignee: Samir Shah
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-13 08:31 UTC by Samir Shah
Modified: 2019-05-23 16:07 UTC (History)
6 users (show)

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


Attachments
Use env variable instead of --password argument for mysqldump (1.41 KB, patch)
2018-06-13 08:31 UTC, Samir Shah
Details | Diff | Splinter Review
Use env variable instead of --password argument for mysqldump (without spurious white space changes) (667 bytes, patch)
2018-06-13 08:34 UTC, Samir Shah
Details | Diff | Splinter Review
Use env variable instead of --password argument for mysqldump (667 bytes, patch)
2018-06-13 08:35 UTC, Samir Shah
Details | Diff | Splinter Review
Bug 20929: Create and Use CNF file (2.24 KB, patch)
2018-09-18 13:42 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20929: Create and Use CNF file (2.34 KB, patch)
2019-04-03 14:17 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Samir Shah 2018-06-13 08:31:29 UTC Comment hidden (obsolete)
Comment 1 Samir Shah 2018-06-13 08:34:56 UTC Comment hidden (obsolete)
Comment 2 Samir Shah 2018-06-13 08:35:25 UTC
Created attachment 76019 [details] [review]
Use env variable instead of --password argument for mysqldump
Comment 3 Jonathan Druart 2018-06-14 13:35:46 UTC
We should create a .my.cnf, but this is much better than passing the password in the options.
Comment 4 Mark Tompsett 2018-09-18 12:44:27 UTC
(In reply to Jonathan Druart from comment #3)
> We should create a .my.cnf, but this is much better than passing the
> password in the options.

I agree. The environment variable is documented here:
https://dev.mysql.com/doc/refman/5.5/en/environment-variables.html
Let me attempt a counter-patch.
Comment 5 Mark Tompsett 2018-09-18 13:42:39 UTC
Created attachment 79052 [details] [review]
Bug 20929: Create and Use CNF file

Rather than have hostname, username, and passwords
on the command line, visible with a ps, use a
CNF file so that mysqldump knows them more securely.

TEST PLAN
----------
1) start kohadevbox
2) reset_all
3) sudo systemctl deamon-reload
4) git bz apply 20929
   -- choose just this patch
5) restart_all
6) perl /home/vagrant/misc4dev/cp_debian_files.pl
7) sudo koha-dump kohadev
8) ls -la /var/spool/koha/kohadev
   -- there is a koha-dump.cnf file with rw owner only perms
9) sudo cat /var/spool/koha/kohadev/koha-dump.cnf
   -- the host, username, and password should match what
      are in the koha-conf.xml file.
10) zcat the backup to make sure the backup has meaningful
    data in it.

BONUS TEST: Change the kohadev DB password, make the
            koha-conf.xml file match, confirm your kohadev
            still works, and that a retest has the new
            username/password in the koha-dump.cnf file.
Comment 6 Mark Tompsett 2018-09-18 13:44:45 UTC
My counter patch does NOT apply on top of the first patch. Choose one, sign it off. Obsolete both.
Comment 7 Tomás Cohen Arazi 2019-03-17 20:56:53 UTC
I prefer the simplicity of using ENV. If I was to use the .cnf file I would touch and chmod before putting information there just in case something goes wrong before the chmod.
Comment 8 Liz Rea 2019-04-03 14:17:27 UTC
Created attachment 87356 [details] [review]
Bug 20929: Create and Use CNF file

Rather than have hostname, username, and passwords
on the command line, visible with a ps, use a
CNF file so that mysqldump knows them more securely.

TEST PLAN
----------
1) start kohadevbox
2) reset_all
3) sudo systemctl deamon-reload
4) git bz apply 20929
   -- choose just this patch
5) restart_all
6) perl /home/vagrant/misc4dev/cp_debian_files.pl
7) sudo koha-dump kohadev
8) in another terminal window: while true; do ls -lah /var/spool/koha/kohadev; sleep 1; done
   -- there is a koha-dump.cnf file with rw owner only perms in one of the ls runs
9) zcat the backup to make sure the backup has meaningful
    data in it.

BONUS TEST: Change the kohadev DB password, make the
            koha-conf.xml file match, confirm your kohadev
            still works, and that a retest of koha-dump still works.
Test plan mostly written by Mark Tompsett.
Comment 9 Liz Rea 2019-04-03 14:28:50 UTC
This counter-counter patch creates and chmod the file before the dump begins, and removes it when we are done with the file. It will be new each day, in case the password changes on the database. If the file exists for some unforeseen reason, the contents will be overwritten (and it'll still be deleted at the end).

The only thing I can think of that might make this better, would be to put the temp .cnf file in /etc/koha/sites/<instance>/ instead of in /var/spool. 

Having sent this patch in, now I'm wondering if we shouldn't just create this config file per instance when we koha-create, and keep it in /etc/koha/sites/<instance> 

I think I'll do a counter patch for that approach as well, I don't like having the file with passwords even temporarily in a directory that could potentially have dodgy permissions (because people [no one I know though] do strange things with backups sometimes).
I suppose if they've got in there, they've got the DB anyway so eh, not sure it matters buuuut... /etc/ will be safer and more persistent than /var/spool anyway.
Comment 10 Liz Rea 2019-04-05 14:46:31 UTC
Just noting that using ENV is decidedly insecure, and we should definitely not go about this that way: 
https://dev.mysql.com/doc/refman/5.5/en/password-security-user.html
Comment 11 Magnus Enger 2019-05-23 13:33:10 UTC
(In reply to Liz Rea from comment #9)
> The only thing I can think of that might make this better, would be to put
> the temp .cnf file in /etc/koha/sites/<instance>/ instead of in /var/spool. 
> 
> Having sent this patch in, now I'm wondering if we shouldn't just create
> this config file per instance when we koha-create, and keep it in
> /etc/koha/sites/<instance> 

If you plan to work on that, the status should perhaps not be "Needs signoff"? :-)