Right now, if libraries with hosted installations want copies of the database for backup purposes, they have to request that their vendors provide them with a copy. It would be helpful if it were possible for vendors to easily enable their customers to easily download backups of the database via the Export tool.
Created attachment 10358 [details] [review] Bug 8268: Add database dump to export tool This patch builds on work by Lars Wirzenius for the Koha packages. To date, the only way for a Koha librarian to obtain a complete backup of their system has been to log into the system via SSH (or FTP) to download the mysqldump file. This patch makes it possible for superlibrarians in properly configured systems to download night backups via the staff client's Export tool. Recognizing that this is functionality with potentially very grave security implications, system administrators must manually enable these features in the koha-conf.xml configuration file. The following configuration settings have been added to the koha-conf.xml file: * backupdir => directory where backups should be stored. * backup_db_via_tools => whether to allow superlibrarians to download database backups via the Export tool. The default is disabled, and there is no way -- by design -- to enable this option without manually editing koha-conf.xml. * backup_conf_via_tools => whether to allow superlibrarians to download configuration backups via the Export tool (this may be applicable to packages only). The default is disabled, and there is no way -- by design -- to enable this option without manually editing koha-conf.xml. This commit modifies the following scripts to make use of the new backupdir configuration option: * koha-dump and koha-run-backups in the Debian packages * The sample backup script misc/cronjobs/backup.sh Note that for security reasons, superlibrarians will not be allowed to download files that are not owned by the web server's effective user. This imposes a de facto dependency on ITK (for Apache) or running the web server as the Koha user (as is done with Plack). To test: 1. Apply patch. 2. Go to export page as a superlibrarian. Notice that no additional export options appear because they have not been enabled. 3. Add <backupdir>$KOHADEV/var/spool</backup> to the <config> section of your koha-conf.xml (note that you will need to adjust that so that it is pointing at a logical directory). 4. Create the aforementioned directory. 5. Go to export page as a superlibrarian. Notice that no additional export options appear because they have not been enabled. 6. Add <backup_db_via_tools>1</backup_db_via_tools> to the <config> section of your koha-conf.xml 7. Go to the export page as a superlibrarian. Notice the new tab. 8. Go to the export page as a non-superlibrarian. Notice there is no new tab. 9. Run: mysqldump -u koha -p koha | gzip > $BACKUPDIR/backup.sql.gz (substituting appropriate user, password, and database name) 10. Go to the export page as a superlibrarian, and look at the "Export database" tab. If you are running the web server as your Koha user, and ran the above command as your Koha user, you should now see the file listed as an option for download. 11. If you *did* see the file listed, change the ownership to something else: sudo chown root:root $BACKUPDIR/backup.sql.gz 11a. Confirm that you no longer see the file listed when you look at the "Export database" tab. 12. Change the ownership on the file to your web server (or Koha) user: sudo chown www-data:www-data backup.sql.gz 13. Go to the export page as a superlibrarian, and look at the "Export database" tab. You should now see backup.sql.gz listed. 14. Choose to download backup.sql.gz 15. Confirm that the downloaded file is what you were expecting. If you are interested, you can repeat the above steps but replace <backup_db_via_tools> with <backup_conf_via_tools>, and instead of creating an sql file, create a tar file. To test packaging: run koha-dump, confirm that it still creates a usable backup.
Note that the patch will not apply without bug 8202.
This is incomplete. There should be a way to create a backup in a database independent way. E.g., in the backup.sh script use a switch statement to call whatever dumper is needed.
(Mit Bezug zu comment 3) > This is incomplete. There should be a way to create a backup in a database > independent way. E.g., in the backup.sh script use a switch statement to > call whatever dumper is needed. My comment does not apply to the patch, but to the script backup.sh (which has more problems...), so please ignore my comment for this particular patch.
Applying: Bug 8268: Add database dump to export tool fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/tools/export.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 8268: Add database dump to export tool
(In reply to comment #5) > Applying: Bug 8268: Add database dump to export tool > fatal: sha1 information is lacking or useless > (koha-tmpl/intranet-tmpl/prog/en/modules/tools/export.tt). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 Bug 8268: Add database dump to export tool Did you apply this on top of the patch for bug 8202?
It applies for me: robin@momomoto:~/catalyst/koha$ git bz apply 8202 Bug 8202 - Add authorities to export tool [SIGNED-OFF] Bug 8202: Add authority export to tools/export.pl Apply? [yn] y Applying: Bug 8202: Add authority export to tools/export.pl robin@momomoto:~/catalyst/koha$ git bz apply 8268 Bug 8268 - Koha should offer way to backup entire db Bug 8268: Add database dump to export tool Apply? [yn] y Applying: Bug 8268: Add database dump to export tool /home/robin/catalyst/koha/.git/rebase-apply/patch:117: trailing whitespace. ls "$backupdir/" | /home/robin/catalyst/koha/.git/rebase-apply/patch:307: trailing whitespace. print $query->header( -type => $mimetype, warning: 2 lines add whitespace errors. robin@momomoto:~/catalyst/koha$
Created attachment 10471 [details] [review] Bug 8268: Add database dump to export tool This patch builds on work by Lars Wirzenius for the Koha packages. To date, the only way for a Koha librarian to obtain a complete backup of their system has been to log into the system via SSH (or FTP) to download the mysqldump file. This patch makes it possible for superlibrarians in properly configured systems to download night backups via the staff client's Export tool. Recognizing that this is functionality with potentially very grave security implications, system administrators must manually enable these features in the koha-conf.xml configuration file. The following configuration settings have been added to the koha-conf.xml file: * backupdir => directory where backups should be stored. * backup_db_via_tools => whether to allow superlibrarians to download database backups via the Export tool. The default is disabled, and there is no way -- by design -- to enable this option without manually editing koha-conf.xml. * backup_conf_via_tools => whether to allow superlibrarians to download configuration backups via the Export tool (this may be applicable to packages only). The default is disabled, and there is no way -- by design -- to enable this option without manually editing koha-conf.xml. This commit modifies the following scripts to make use of the new backupdir configuration option: * koha-dump and koha-run-backups in the Debian packages * The sample backup script misc/cronjobs/backup.sh Note that for security reasons, superlibrarians will not be allowed to download files that are not owned by the web server's effective user. This imposes a de facto dependency on ITK (for Apache) or running the web server as the Koha user (as is done with Plack). To test: 1. Apply patch. 2. Go to export page as a superlibrarian. Notice that no additional export options appear because they have not been enabled. 3. Add <backupdir>$KOHADEV/var/spool</backup> to the <config> section of your koha-conf.xml (note that you will need to adjust that so that it is pointing at a logical directory). 4. Create the aforementioned directory. 5. Go to export page as a superlibrarian. Notice that no additional export options appear because they have not been enabled. 6. Add <backup_db_via_tools>1</backup_db_via_tools> to the <config> section of your koha-conf.xml 7. Go to the export page as a superlibrarian. Notice the new tab. 8. Go to the export page as a non-superlibrarian. Notice there is no new tab. 9. Run: mysqldump -u koha -p koha | gzip > $BACKUPDIR/backup.sql.gz (substituting appropriate user, password, and database name) 10. Go to the export page as a superlibrarian, and look at the "Export database" tab. If you are running the web server as your Koha user, and ran the above command as your Koha user, you should now see the file listed as an option for download. 11. If you *did* see the file listed, change the ownership to something else: sudo chown root:root $BACKUPDIR/backup.sql.gz 11a. Confirm that you no longer see the file listed when you look at the "Export database" tab. 12. Change the ownership on the file to your web server (or Koha) user: sudo chown www-data:www-data backup.sql.gz 13. Go to the export page as a superlibrarian, and look at the "Export database" tab. You should now see backup.sql.gz listed. 14. Choose to download backup.sql.gz 15. Confirm that the downloaded file is what you were expecting. If you are interested, you can repeat the above steps but replace <backup_db_via_tools> with <backup_conf_via_tools>, and instead of creating an sql file, create a tar file. To test packaging: run koha-dump, confirm that it still creates a usable backup. ------ This signoff contains two changes: 10-1. If no backup/conf files were present, then the message telling you so doesn't appear and the download button does. Made them behave correctly. 10-2. The test for a file existing required it to be owned by the webserver UID. This change makes it so it only has to be readable. Signed-off-by: Robin Sheat <robin@catalyst.net.nz>
Things noted: 10-1. If you forgot to run the backup command, then there is nothing to choose from but the download button is still there. Not a big issue, just slightly cosmetic. Also, the "no downloads exist" message is missing, I think due to different semantics between T::T and perl about whether an empty array is considered "true." 10-2. This doesn't work with the packages, as it's testing -o (do I own this file) not -r (can I read this file.) Dumps provided by the packages are owned by root, but readable by the webserver due to group permissions. This needs to be fixed in getbackupfilelist and download_backup. My signed off patch fixes these.
Comment on attachment 10358 [details] [review] Bug 8268: Add database dump to export tool patch obsoleted by signed-off patch
QA comment: You add 2 subs: getbackupfilelist and download_backup => bad point, we usually use capitals (GetBackupFileList and DownloadBackup). However, this is not in a package, so I won't fail QA for that. Other than this, I can't find anything wrong in your code, the security triple check (ENV and unix permission, and superlibrarian) is perfect. I'm adding semarie, that is a boss at finding security holes. (Cher frère, si tu trouves un problème, ne te prive pas !) Marking passed QA, I'll delay pushing, I'll wait for some feedback from semarie, just in case
(En réponse au commentaire 11) > > Marking passed QA, I'll delay pushing, I'll wait for some feedback from > semarie, just in case The code seems globally ok: just 4 remarks. Some security remarks (in tools/export.pl): 1. a problem (common in koha): use of user-input in warn. An attacker can forge entries in log file (see CWE-93) (resume: if variable contains CRLF, the log file too... and the next line is forged: so who write in log ? koha or attacker ?) > warn "A suspicious attempt was made to download the db at '$filename' by ..." 2. about the regex for $filename verification > return unless ( ... && not $filename =~ m#(^\.\.|/)# ); The regex could be translated as: $filename should not: - start with '..' OR - contains '/' I don't understand the purpose of "^..", if '/' is forbidden; as you can't escape from directory without '/'. So I think m#/# is suffisent (please correct me if not). Some generals remarks: 3. the directory for backup is not consitent in differents files: in debian/templates/koha-conf-site.xml.in, the directory use for backupdir is: > <backupdir>/var/lib/koha/__KOHASITE__</backupdir> in Makefile.PL, the directory seems to be in /var/spool : > './skel/var/spool/koha' => { target => 'BACKUP_DIR', trimdir => -1 }, in debian/scripts/koha-dump: > [ -z "$backupdir" ] && backupdir="/var/spool/koha/$name" 4. in misc/cronjobs/backup.sh The lasts lines are for mail admin if ok, or mail admin if not ok. > [ -f $KOHA_BACKUP] && echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP || > echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP First, there are *two* lines. I haven't test it, but it should be an error as "cmd_a && cmd_b ||" is not a valid shell expression. Should be one-line "cmd_a && cmd_b || cmd_c" Secondly, I'm not sure that the behaviour is correct. "cmd_a && cmd_b || cmd_c" could be expanded as: if cmd_a ; then if ! cmd_b ; then cmd_c fi else cmd_c fi so if cmd_b failed (echo "backup ok" | mail admin), the cmd_c (echo "backup err" | mail admin) will be run (and expected to failed too, as the first try). Proposed correction: > if [ -f $KOHA_BACKUP] ; then > echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP > else > echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP > fi
Just another error (and that I kept in my "proposed correction", so please correct also my correction :-) ), in 4. "misc/cronjobs/backup.sh" [ -f $KOHA_BACKUP] is not correct, but [ -f $KOHA_BACKUP ] is. -sh: [: missing `]'
Created attachment 10682 [details] [review] Bug 8268 follow-up: incorporate QA comments Fixes the following things: 1. Sanitizes log output to prevent an attacker from using a specially crafted POST to add extra lines to the log 2. Simplify a regular expression since "..file" cannot be used to escape the current directory 3. Makes sure directories are consistent 4. Correct logic issues in misc/cronjobs/backup.sh Thanks to Frère Sébastien Marie for catching these issues.
(In reply to comment #12) > (En réponse au commentaire 11) > > > > Marking passed QA, I'll delay pushing, I'll wait for some feedback from > > semarie, just in case > > The code seems globally ok: just 4 remarks. > > Some security remarks (in tools/export.pl): > 1. a problem (common in koha): use of user-input in warn. An attacker can > forge entries in log file (see CWE-93) (resume: if variable contains CRLF, > the log file too... and the next line is forged: so who write in log ? koha > or attacker ?) > > > warn "A suspicious attempt was made to download the db at '$filename' by ..." > > > 2. about the regex for $filename verification > > return unless ( ... && not $filename =~ m#(^\.\.|/)# ); > > The regex could be translated as: $filename should not: > - start with '..' > OR > - contains '/' > > I don't understand the purpose of "^..", if '/' is forbidden; as you can't > escape from directory without '/'. > > So I think m#/# is suffisent (please correct me if not). > > > > Some generals remarks: > 3. the directory for backup is not consitent in differents files: > > in debian/templates/koha-conf-site.xml.in, the directory use for backupdir > is: > > <backupdir>/var/lib/koha/__KOHASITE__</backupdir> > > in Makefile.PL, the directory seems to be in /var/spool : > > './skel/var/spool/koha' => { target => 'BACKUP_DIR', trimdir => -1 }, > > in debian/scripts/koha-dump: > > [ -z "$backupdir" ] && backupdir="/var/spool/koha/$name" > > > 4. in misc/cronjobs/backup.sh > > The lasts lines are for mail admin if ok, or mail admin if not ok. > > [ -f $KOHA_BACKUP] && echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP || > > echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP > > First, there are *two* lines. I haven't test it, but it should be an error > as "cmd_a && cmd_b ||" is not a valid shell expression. Should be one-line > "cmd_a && cmd_b || cmd_c" > > Secondly, I'm not sure that the behaviour is correct. > "cmd_a && cmd_b || cmd_c" could be expanded as: > > if cmd_a ; then > if ! cmd_b ; then > cmd_c > fi > else > cmd_c > fi > > so if cmd_b failed (echo "backup ok" | mail admin), the cmd_c (echo "backup > err" | mail admin) will be run (and expected to failed too, as the first > try). > > Proposed correction: > > if [ -f $KOHA_BACKUP] ; then > > echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP > > else > > echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP > > fi Thank you for catching those issues. I corrected them all with the follow-up. (In reply to comment #11) > QA comment: > > You add 2 subs: getbackupfilelist and download_backup => bad point, we > usually use capitals (GetBackupFileList and DownloadBackup). However, this > is not in a package, so I won't fail QA for that. Our standard is to use lowercase in .pl files and uppercase in packages, I think. I suppose that makes sense, since it ensures that there will be a visual distinction between local functions and core functions.
Since the follow-up changes sensible things like: + <backupdir>/var/spool/koha/__KOHASITE__</backupdir> I think it's worth a signoff before pushing. Robin, could you please signoff the follow-up ?
Jared, something you did causes this in your patch: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="UTF-8" which causes git to say: fatal: cannot convert from UTF-8UTF-8 to UTF-8 I should be able to work around it easily, but just so you know you did something weird :)
OK, I think there are more issues: $ git am /tmp/Bug-8268-follow-up-incorporate-QA-comments-l9YCje.patch Applying: Bug 8268 follow-up: incorporate QA comments error: patch failed: tools/export.pl:340 error: tools/export.pl: patch does not apply Patch failed at 0001 Bug 8268 follow-up: incorporate QA comments Jared, I expect you can remake this easily, can you do that and I'll have a look and sign it off (looking over it, it seems fine: that said, I didn't spot the path issue that Frère Sébastien caught, so take that for what it's worth :)
Created attachment 10749 [details] [review] Bug 8268 follow-up: incorporate QA comments Fixes the following things: 1. Sanitizes log output to prevent an attacker from using a specially crafted POST to add extra lines to the log 2. Simplify a regular expression since "..file" cannot be used to escape the current directory 3. Makes sure directories are consistent 4. Correct logic issues in misc/cronjobs/backup.sh Thanks to Frere Sebastien Marie for catching these issues.
Created attachment 10751 [details] [review] Bug 8268 follow-up: incorporate QA comments Fixes the following things: 1. Sanitizes log output to prevent an attacker from using a specially crafted POST to add extra lines to the log 2. Simplify a regular expression since "..file" cannot be used to escape the current directory 3. Makes sure directories are consistent 4. Correct logic issues in misc/cronjobs/backup.sh Thanks to Frere Sebastien Marie for catching these issues. Signed-off-by: Robin Sheat <robin@catalyst.net.nz>
QA comments: Frere Sebastien Marie suggestions/concern taken in account, passed QA About > > You add 2 subs: getbackupfilelist and download_backup => bad point, we > > usually use capitals (GetBackupFileList and DownloadBackup). However, this > > is not in a package, so I won't fail QA for that. > > Our standard is to use lowercase in .pl files and uppercase in packages, I > think. I suppose that makes sense, since it ensures that there will be a > visual distinction between local functions and core functions. You're right, sorry for the annoyance.
Patch pushed
I've found a problem with this patch, that you can see when you run plack: Variable "$backupdir" is not available at /home/paul/koha.dev/koha-community/tools/export.pl line 334. => the backup features won't work under plack (problem of variable scoping with Plack that put the script in a sub, making main() variable unaccessible in other subs. Problem solved everywhere with a our instead of my. The best option, that I would prefer -from far- for this new feature, being that we don't use such a our variable, but define backupdir where it's needed (+ it's just reading of a syspref, so not something costly)
Suspecting this recently pushed patch from following: Koha installation crashes on not existent directory: var/spool. This was a dev install. chmod: cannot access `/usr/share/kohadev/newclone/var/spool': No such file or directory make: *** [set_koha_permissions] Error 1
Just for the record (and future cases..): I see changes to Makefile.PL (among others, incl koha-conf.xml) but the test plan does not [even] include testing the various installations. Although the changes are small, it is better to do.
Created attachment 10869 [details] [review] Followup for fixing install Adds a file to skel/var/spool/koha to create subdir
This follow-up is required for new, non-package installations to work.
*** Bug 8467 has been marked as a duplicate of this bug. ***
Trivial and neede follow-up, passed QA
Jared, In a manual dev install I do still come across this line in koha-conf.xml: <backupdir>__BACKUP_DIR__</backupdir> Note that install finished apparently correct..
(In reply to comment #30) > Jared, > In a manual dev install I do still come across this line in koha-conf.xml: > <backupdir>__BACKUP_DIR__</backupdir> > > Note that install finished apparently correct.. I will look into this later today.
Created attachment 11178 [details] [review] Bug 8268 follow-up: rewrite config correctly
Created attachment 11187 [details] [review] Signed followup Problem fixed. Signed off and passed QA
Created attachment 11208 [details] [review] Bug 8268: improve error checking Valid download attempts were being denied thanks to an incorrect regular expression. This patch fixes that, and makes it easier to understand what's going on in the code.
The first patch has been signed off and passed QA, but the second has not.
Created attachment 11262 [details] [review] Bug 8268: improve error checking Valid download attempts were being denied thanks to an incorrect regular expression. This patch fixes that, and makes it easier to understand what's going on in the code. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
both (2) patches signed-off. looks good, passing QA on this mason@xen1:~/git/head$ koha-qa.pl -c 2 * 8374da8 Bug 8268: improve error checking tools/export.pl * 5a8dcc7 Bug 8268 follow-up: rewrite config correctly rewrite-config.PL - perlcritic-progressive tests... OK - perl -c syntax tests... OK - xt/tt_valid.t tests... OK - xt/author/valid-template.t tests... OK
follow-ups http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11187 and http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11262 pushed today (status switched back to "ENH", as this bug describe an enhancement
This feature is now available, though it doesn't play so nicely with Plack (see bug 8437).