Skip to content
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.

Sign up for GitHub

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

Jump to bottom

nixos/postgresqlBackup: set to umask to 0077 #49840

Merged
merged 1 commit into from Nov 14, 2018
Merged

nixos/postgresqlBackup: set to umask to 0077 #49840

merged 1 commit into from Nov 14, 2018

Conversation

markuskowa
Copy link
Member

@markuskowa markuskowa commented Nov 6, 2018 •
edited

Motivation for this change

The backup file now is world readable (0644). The mkdir in the service file creates the backup directory with mode 0700. However, if the directory already exists the permissions are not changed and
the backup file is exposed. Setting the umask during creation of the dump to 0077 ensures
that the backup itself is not exposed and existing settings are not changed (i.e. the directory permissions).

This patch also needs a backport to 18.09

Things done
  • Ensure that the backup file is only readable by the owner by setting umask 0077
  • Add file permission test to nixos/tests/postgresql.nix
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@markuskowa
nixos/postgresqlBackup: set to umask to 0077
a0371d4 
* Ensure that the backup file is only readable by the owner
* Add file permission test to tests
@markuskowa
Copy link
Member Author

@GrahamcOfBorg test postgresql

@GrahamcOfBorg
Copy link

Success on aarch64-linux

Attempted: tests.postgresql

No partial log is available.

@GrahamcOfBorg
Copy link

Success on x86_64-linux

Attempted: tests.postgresql

No partial log is available.

aanderse
aanderse reviewed Nov 9, 2018
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Could you not use the systemd UMask setting here?

@markuskowa
Copy link
Member Author

I can't get it to work with the systemd UMask setting. If I set UMask=0077 or UMask=0007 in the service config, then the service script fails with Permission denied when it tries to write the backup. I do not understand why it behaves different to setting the umask directly in the shell script.

@aanderse
Copy link
Member

@markuskowa I realize my mistake:

UMask is being applied to both the preStart and the script code being executed. /var/backup doesn't exist so the UMask is being applied to that folder as root and hence unreadable by postgres in the script block of your code, hence the permission denied error.

I was incorrect about using the systemd UMask here and what you were doing was perfectly correct. Sorry for that.

Great patch, and given the security aspect this should definitely get back ported to 18.09 as well. Just need to find someone to merge now.

@samueldr samueldr merged commit 58c0c25 into NixOS:master Nov 14, 2018
@samueldr
Copy link
Member

samueldr commented Nov 14, 2018 •
edited

Backported:

[release-18.09 4c72d0c] nixos/postgresqlBackup: set to umask to 0077
Date: Tue Nov 6 21:59:29 2018 +0100
2 files changed, 3 insertions(+)

@markuskowa markuskowa deleted the fix-pgBackup branch November 14, 2018 09:38
@markuskowa
Copy link
Member Author

Thanks for the clarification! That makes perfect sense now. It is rather subtle how umask is applied here. The systemd man pages are not very clear in that respect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse aanderse left review comments

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice thoughtpolice is a code owner

Assignees
No one assigned
Labels
1.severity: security 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@markuskowa @GrahamcOfBorg @aanderse @samueldr