New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mysqlBackup service: Grant privileges to backup user #29703
Conversation
Do you want to write the test in this pull request? |
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. |
@@ -9,7 +9,7 @@ let | |||
cfg = config.services.mysqlBackup ; | |||
location = cfg.location ; | |||
mysqlBackupCron = db : '' | |||
${cfg.period} ${cfg.user} ${mysql}/bin/mysqldump ${if cfg.singleTransaction then "--single-transaction" else ""} ${db} | ${gzip}/bin/gzip -c > ${location}/${db}.gz | |||
${cfg.period} ${cfg.user} ${mysql}/bin/mysqldump -u ${cfg.user} ${if cfg.singleTransaction then "--single-transaction" else ""} ${db} | ${gzip}/bin/gzip -c > ${location}/${db}.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does authentication works in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unix socket authentication, through the new services.mysql.ensureUsers
option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. ❓
The Actually, I decided I will after all do a basic nixos test which can then be enhanced once this service uses systemd timers. |
OK @Mic92 , I have added the test :-) |
Does the backup work for you? The directory is still empty in my case. Also changing the |
I never tried changing period. It worked for me last night with the default time. |
My system crontab seems empty. |
630b52d
to
c2b3f90
Compare
Interesting! I have updated the test to check that the system crontab is not empty. I'm wondering if
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
privs = "SELECT, SHOW VIEW, TRIGGER, LOCK TABLES"; | ||
grant = db: nameValuePair "${db}.*" privs; | ||
in | ||
listToAttrs (map grant config.services.mysqlBackup.databases); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 👍
@@ -9,7 +9,7 @@ let | |||
cfg = config.services.mysqlBackup ; | |||
location = cfg.location ; | |||
mysqlBackupCron = db : '' | |||
${cfg.period} ${cfg.user} ${mysql}/bin/mysqldump ${if cfg.singleTransaction then "--single-transaction" else ""} ${db} | ${gzip}/bin/gzip -c > ${location}/${db}.gz | |||
${cfg.period} ${cfg.user} ${mysql}/bin/mysqldump -u ${cfg.user} ${if cfg.singleTransaction then "--single-transaction" else ""} ${db} | ${gzip}/bin/gzip -c > ${location}/${db}.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. ❓
Hi @florianjacob, thanks for the comments.
|
@@ -8,3 +8,4 @@ insert into tests values (1, 'a'); | |||
insert into tests values (2, 'b'); | |||
insert into tests values (3, 'c'); | |||
insert into tests values (4, 'd'); | |||
insert into tests values (5, 'hello'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this interfere with other tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used by mysql and mysqlReplication, which still pass.
Just tested with systemd-timers, works for me! 👍 Thanks!
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. 🙃
Yes. I forgot there's also a |
Can squash the commits a little bit for easier backporting? |
431c351
to
e76b3eb
Compare
OK @Mic92, have rebased on latest nixos-unstable channel and squashed. I also removed usage of the Two other tangentially related things:
|
e76b3eb
to
4f30221
Compare
I rebased it to solve merge conflicts. |
nixos/tests/mysql-backup.nix
Outdated
}; | ||
in { | ||
one = mkNode { package = pkgs.mysql; user = "mysqlbackup"; }; | ||
two = mkNode { package = pkgs.mariadb; user = "mysqlbackup"; }; |
There was a problem hiding this comment.
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
nixos/tests/mysql-backup.nix
Outdated
one = mkNode { package = pkgs.mysql; user = "mysqlbackup"; }; | ||
two = mkNode { package = pkgs.mariadb; user = "mysqlbackup"; }; | ||
three = mkNode { package = pkgs.mysql; user = "mybackup"; }; | ||
four = mkNode { package = pkgs.mariadb; user = "mybackup"; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is their a usecase for having the username configureable? Would simplify our assumptions a little bit, which is a good thing for backup code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 themysqlbackup
user as is, and have a generalbackup
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot do the first option. And the second option also don't require to change the username.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. then just remove the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, done, tests simplified.
* 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
4f30221
to
9bd932a
Compare
Grants enough privileges to the configured user so that it can run
mysqldump
.Uses the new declarative users options.
Resolves #24728.
/cc @davidak @florianjacob @fadenb
Things done
build-use-sandbox
innix.conf
on non-NixOS)