Make /var/empty immutable (with chattr +i) #18365

Merged
merged 2 commits into from Sep 7, 2016

Projects

None yet

7 participants

@domenkozar
Member

See #18358 and #14910 what bugs these caused.

cc @edolstra

@mention-bot

@domenkozar, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @nathan7 and @peti to be potential reviewers

@domenkozar domenkozar added this to the 16.09 milestone Sep 6, 2016
@edolstra
Member
edolstra commented Sep 6, 2016

No, don't make it a link to the Nix store! Paths in the Nix store can have group = nixbld, which will probably cause sshd to fail. Just do chattr +i /var/empty.

Also, /var/empty is not a GC root, which might cause problems.

@domenkozar
Member

@edolstra updated

@domenkozar
Member

Just a side note: lots of these mkdir -m commands claim to be idempotent, but they're really not since permissions are not reset if directory exists.

@groxxda
Contributor
groxxda commented Sep 6, 2016

Could we use a tmpfs with size=0,mode=000?

@domenkozar
Member

@groxxda what advantage would that have over chattr +i?

@groxxda
Contributor
groxxda commented Sep 6, 2016

@domenkozar it's probably just me not being a fan of chattr 😉
only advantage I can think of is file-system support. But it's probably not relevant because nobody has /var on a tmpfs

btw: Does your patch work when run twice?

@domenkozar domenkozar Make /var/empty immutable
Fixes #14910 and #18358

Deployed to an existing server, restarted sshd and polkit to verify
they don't fail.
3877ec5
@domenkozar
Member

@groxxda it did, but I pushed a fix for rm: cannot remove '/var/empty': Operation not permitted

@grahamc grahamc changed the title from Make /var/empty immutable (symlink to nix store) to Make /var/empty immutable (with chattr +i) Sep 7, 2016
@grahamc
Member
grahamc commented Sep 7, 2016

(Updated the title to reflect the solution)

@domenkozar
Member

@edolstra any objections?

@edolstra edolstra commented on an outdated diff Sep 7, 2016
nixos/modules/system/activation/activation-script.nix
@@ -137,8 +140,13 @@ in
mkdir -m 1777 -p /var/tmp
+ # Make sure it's really empty
+ chattr -i /var/empty
+ rm -rf /var/empty
@edolstra
edolstra Sep 7, 2016 Member

This introduces a race during activation where /var/empty doesn't exist. So for example, if it gets interrupted at that point, you won't be able to log in via ssh anymore.

@edolstra edolstra commented on an outdated diff Sep 7, 2016
nixos/modules/system/activation/activation-script.nix
@@ -137,8 +140,13 @@ in
mkdir -m 1777 -p /var/tmp
+ # Make sure it's really empty
+ chattr -i /var/empty
@edolstra
edolstra Sep 7, 2016 Member

This could be ${e2fsprogs}/bin/chattr to prevent polluting the path with e2fsprogs.

@domenkozar
Member

@edolstra fixed

@domenkozar domenkozar hardcode e2fsprogs, idempotent chmod, remove care condition
8f95e6f
@edolstra edolstra merged commit 70be99c into master Sep 7, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@peti peti deleted the fix-sshd-failure branch Sep 7, 2016
@fpletz
Member
fpletz commented on 3877ec5 Sep 20, 2016

This breaks nixos-containers. Removing containers won't be possible because /var/empty can't be removed and the error message (Permission denied) is not very helpful because the root cause is the immutable flag.

@domenkozar Why is the immutable flag needed? Is something modifying this directory's permissions?

Contributor

I just finished running into this. As @fpletz says, it breaks nixos-container destroy and you have to go in manually and remove the immutable attribute and then cleanup the appropriate /var/lib/containers directory.

@domenkozar
Member

@fpletz I've linked two issues in description of the PR for motivation what issues we fix.

I think it's safe to disable this for containers.

@domenkozar
Member

@fpletz here needs to be added chattr -R -f -i: https://github.com/NixOS/nixpkgs/blob/master/pkgs/tools/virtualization/nixos-container/nixos-container.pl#L248

Any we probably should test if container deletion works in the tests.

@domenkozar
Member

Ah it already is, but doesn't fail: http://hydra.nixos.org/build/40856595/log/raw

@domenkozar domenkozar added a commit that referenced this pull request Sep 30, 2016
@domenkozar domenkozar changelog for #18365 14c16f2
@domenkozar domenkozar added a commit that referenced this pull request Sep 30, 2016
@domenkozar domenkozar changelog for #18365
(cherry picked from commit 14c16f2)
Signed-off-by: Domen Kožar <domen@dev.si>
3781095
@peterhoeg peterhoeg added a commit to peterhoeg/nixpkgs that referenced this pull request Oct 2, 2016
@domenkozar @peterhoeg domenkozar + peterhoeg changelog for #18365
(cherry picked from commit 14c16f2)
Signed-off-by: Domen Kožar <domen@dev.si>
f662742
@bramd bramd added a commit to bramd/nixpkgs that referenced this pull request Oct 22, 2016
@domenkozar @bramd domenkozar + bramd changelog for #18365 90ae900
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment