mysqlBackup service: Grant privileges to backup user by rvl · Pull Request #29703 · NixOS/nixpkgs

rvl · 2017-09-23T15:24:00Z

Grants enough privileges to the configured user so that it can run mysqldump.

Uses the new declarative users options.

Resolves #24728.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Fits CONTRIBUTING.md.

mention-bot · 2017-09-23T15:24:03Z

@rvl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rbvermaa, @edolstra and @peti to be potential reviewers.

Mic92 · 2017-09-24T14:48:56Z

Do you want to write the test in this pull request?

rvl · 2017-09-24T14:55:37Z

I would rather do it after the backup service is changed to use a systemd timer. I think there has already been some work in that direction, so I could use that as part of a separate PR.

Mic92 · 2017-09-24T15:02:43Z

nixos/modules/services/backup/mysql-backup.nix

How does authentication works in this case?

Unix socket authentication, through the new services.mysql.ensureUsers option.

Did you test whether -u is necessary? In theory it should default to the executing user, which should be changed from root to ${cfg.user} by cron. But I noticed that it seems to be required to explicitly specify when sudoing as another user than root, so this might be the case with cron as well. ❓

rvl · 2017-09-24T15:23:20Z

The ensureUsers option comes from #29387 and is on the 17.09 branch as well.

Actually, I decided I will after all do a basic nixos test which can then be enhanced once this service uses systemd timers.

rvl · 2017-09-24T16:03:54Z

OK @Mic92 , I have added the test :-)

Mic92 · 2017-09-24T16:04:01Z

Does the backup work for you? The directory is still empty in my case. Also changing the mysqlBackup.period does not seem to rebuild configuration.

rvl · 2017-09-24T16:07:34Z

I never tried changing period. It worked for me last night with the default time.

Mic92 · 2017-09-24T16:14:59Z

My system crontab seems empty.

rvl · 2017-09-24T16:26:26Z

Interesting! I have updated the test to check that the system crontab is not empty.

I'm wondering if /etc/crontab has been previously edited by the user, will NixOS overwrite it? My test is:

echo lah > /etc/crontab
edit configuration.nix and add services.mysqlBackup.period = "*/5 * * * *";
nixos-rebuild switch
cat /etc/crontab
Seems OK to me. What output do you get from nixos-option services.cron.enable ?

florianjacob

Happy to see ensureUsers in action! 😄

My main concern is that it doesn't make much sense to reduce the permissions of the backup user, which is still set by the script to default to the mysql user – which has, by definition, total control of the database regardless of its mysql permissions.

I'd vote for creating a special backup user by default and use that, and do it now when the script is unbroken for making this a “non-breaking” change. 🤔 This would also have the benefit of being able to allow ssh logins for that backup user to pull the backups to a remote location, without giving them any permissions over the mysql daemon.

I'd also avoid to invest too much time into the cron stuff, as fadenb@4c90ea3 (from #29031) is essentially ready for being merged from my point of view.

florianjacob · 2017-09-24T19:15:29Z

nixos/modules/services/backup/mysql-backup.nix

Didn't think of that use case to reduce permissions to a dynamic set of tables, but I think it's nicely expressed and can serve as a template for other cases where this might be required. 👍

florianjacob · 2017-09-24T19:16:39Z

nixos/modules/services/backup/mysql-backup.nix

Did you test whether -u is necessary? In theory it should default to the executing user, which should be changed from root to ${cfg.user} by cron. But I noticed that it seems to be required to explicitly specify when sudoing as another user than root, so this might be the case with cron as well. ❓

rvl · 2017-09-25T18:24:58Z

Hi @florianjacob, thanks for the comments.

I do not know why the -u option is required, but at least it won't harm anything.
If I understand correctly, the privileges granted to the backup user will be added to what they already have.
I have implemented your suggestion to use a different default user for backups.
I have included @fadenb's systemd timer change and made improvements to the error handling.

Mic92 · 2017-09-25T20:24:10Z

nixos/tests/testdb.sql

Does this interfere with other tests?

It's used by mysql and mysqlReplication, which still pass.

florianjacob · 2017-09-25T21:47:11Z

Just tested with systemd-timers, works for me! 👍 Thanks!

I do not know why the -u option is required, but at least it won't harm anything.

Just tested it, seems like it isn't necessary anymore when the script is started by systemd instead of cron. But of course, won't harm anything. 🙃

If I understand correctly, the privileges granted to the backup user will be added to what they already have.

Yes. I forgot there's also a mysql mysql user by default.

Mic92 · 2017-09-25T23:24:16Z

Can squash the commits a little bit for easier backporting?

rvl · 2017-09-26T06:30:19Z

OK @Mic92, have rebased on latest nixos-unstable channel and squashed.

I also removed usage of the mysqldump -u option, and changed the test to use the mysqlbackup user.

Two other tangentially related things:

In the next version, we could consider not using a bash script for database backups (shell script mistakes have been known to cause data loss for some people).
I think the /etc/crontab issues seen are related to services.mysqlBackup: Backup job might break due to garbage collection #29031. @Mic92, would you have anything to add about that?

Mic92 · 2017-09-26T10:33:13Z

I rebased it to solve merge conflicts.

Mic92 · 2017-09-26T10:41:13Z

nixos/tests/mysql-backup.nix

Turns out mysql == mariadb in nixpkgs

Mic92 · 2017-09-26T10:53:13Z

nixos/tests/mysql-backup.nix

Is their a usecase for having the username configureable? Would simplify our assumptions a little bit, which is a good thing for backup code.

Good point. 🤔 What I can think of:

Someone creates mutliple instances of this (via custom derivations or something? Is that possible?) to have a different set of databases backed up with separate users to separate locations.

Someone backs up multiple things to /var/backup apart from databases and wants to use the same user for everything, to be able to take all what's there to another host via SSH. But in that case, I'd say it would be better to leave the mysqlbackup user as is, and have a general backup group to which all of /var/backup belongs to or something.

Especially in case it's not easily possible to have multiple instances, I think I'd prefer to see mysqlBackup as encapsulated service that always has that own mysqlbackup user which is only dumping databases to disk, and nothing else.

You cannot do the first option. And the second option also don't require to change the username.

Personally, I don't have a usecase for having the username configureable. However I'm reluctant to remove it because it was there before. What do you recommend?

In either case, considering that mysql == mariadb in nixpkgs, we could fairly safely collapse the four test cases into one to simplify and speed up the test.

ok. then just remove the tests.

OK, done, tests simplified.

@fadenb

* Grants enough privileges to the configured user so that it can run mysqldump. * Adds a nixos test. * Use systemd timers instead of a cronjob (by @fadenb). * Creates a new user for backups by default, instead of using mysql user. * Ensures that backup user has write permissions on backup location. * Write backup to a temporary file before renaming so that a failed backup won't overwrite the previous backup, and so that the backup location will never contain a partial backup. Breaking changes: * Renamed period to calendar to reflect the change in how to configure the backup time. * A failed backup will no longer result in cron sending an e-mail -- users' monitoring systems must be updated. Resolves NixOS#24728

Mic92 changed the title ~~mysqlBackup service: Grant privileges to backup user~~ [WIP] mysqlBackup service: Grant privileges to backup user Sep 24, 2017

Mic92 changed the title ~~[WIP] mysqlBackup service: Grant privileges to backup user~~ mysqlBackup service: Grant privileges to backup user Sep 24, 2017

Mic92 reviewed Sep 24, 2017

View reviewed changes

Mic92 added the 9.needs: port to stable A PR needs a backport to the stable release. label Sep 24, 2017

rvl changed the title ~~mysqlBackup service: Grant privileges to backup user~~ [WIP] mysqlBackup service: Grant privileges to backup user Sep 24, 2017

rvl changed the title ~~[WIP] mysqlBackup service: Grant privileges to backup user~~ mysqlBackup service: Grant privileges to backup user Sep 24, 2017

rvl force-pushed the mysql-backup-privileges branch from 630b52d to c2b3f90 Compare September 24, 2017 16:24

florianjacob reviewed Sep 24, 2017

View reviewed changes

florianjacob mentioned this pull request Sep 24, 2017

services.mysqlBackup: Backup job might break due to garbage collection #29031

Closed

rvl changed the title ~~mysqlBackup service: Grant privileges to backup user~~ [WIPmysqlBackup service: Grant privileges to backup user Sep 25, 2017

rvl changed the title ~~[WIPmysqlBackup service: Grant privileges to backup user~~ [WIP] mysqlBackup service: Grant privileges to backup user Sep 25, 2017

rvl changed the title ~~[WIP] mysqlBackup service: Grant privileges to backup user~~ mysqlBackup service: Grant privileges to backup user Sep 25, 2017

Mic92 reviewed Sep 25, 2017

View reviewed changes

rvl force-pushed the mysql-backup-privileges branch from 431c351 to e76b3eb Compare September 26, 2017 06:20

Mic92 force-pushed the mysql-backup-privileges branch from e76b3eb to 4f30221 Compare September 26, 2017 10:32

Mic92 reviewed Sep 26, 2017

View reviewed changes

nixos/tests/mysql-backup.nix Outdated

Copy link

Member

Mic92 Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out mysql == mariadb in nixpkgs

rvl reacted with laugh emoji

Mic92 reviewed Sep 26, 2017

View reviewed changes

rvl added 2 commits September 27, 2017 06:25

nixos/release-notes: MySQL backup service breaking changes

9bd932a

rvl force-pushed the mysql-backup-privileges branch from 4f30221 to 9bd932a Compare September 27, 2017 05:37

fpletz added this to the 17.09 milestone Sep 27, 2017

globin merged commit 34eefdf into NixOS:master Sep 27, 2017

samueldr removed the 9.needs: port to stable A PR needs a backport to the stable release. label Apr 17, 2019

Uh oh!

Conversation

rvl commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

mention-bot commented Sep 23, 2017

Uh oh!

Mic92 commented Sep 24, 2017

Uh oh!

rvl commented Sep 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvl commented Sep 24, 2017

Uh oh!

rvl commented Sep 24, 2017

Uh oh!

Mic92 commented Sep 24, 2017

Uh oh!

rvl commented Sep 24, 2017

Uh oh!

Mic92 commented Sep 24, 2017

Uh oh!

rvl commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

florianjacob left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvl commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

florianjacob commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mic92 commented Sep 25, 2017

Uh oh!

rvl commented Sep 26, 2017

Uh oh!

Mic92 commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mic92 Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

florianjacob Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mic92 Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

rvl commented Sep 23, 2017 •

edited

Loading

rvl commented Sep 24, 2017 •

edited

Loading

florianjacob left a comment •

edited

Loading

florianjacob commented Sep 25, 2017 •

edited

Loading

Mic92 Sep 26, 2017 •

edited

Loading

florianjacob Sep 26, 2017 •

edited

Loading

Mic92 Sep 26, 2017 •

edited

Loading