security-wrapper: Don't remove the old paths yet as that can create migration pain

[ixmatus]

Motivation for this change

As reported in #19862, removing the old wrapper dirs was a bad idea (on me) because users may be using sudo to nixos-rebuild switch which will break afterwards unless the re-source; but it's not intuitive to do that.

Leaving the old paths doesn't hurt anything and on their next login, will use the new wrapper paths correctly. The next upgrade, 17.09, can remove these paths from the system safely.

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 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/)
• Fits CONTRIBUTING.md.

---

[mention-bot]

@ixmatus, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abbradar, @bjornfor and @globin to be potential reviewers.

[globin]

What about linking the /var/setuid-wrappers to the new location?

[ixmatus]

@globin that will not work I think because the wrappers check the dir from which they are being run.

[ixmatus]

CC: @teh @domenkozar

[domenkozar]

I like it. If this gets merged, we should open an issue for 17.09 not to forget the cleanup.

[ixmatus]

Interestingly, I think the initial issue experienced by @bsoudan had to do with the changes introduced prior to my own wrapper work that made the activation script more atomic using tmpfs. Then I believe @qknight and @resonator reporting in #19862, upgrading from 16.09 to master, experienced a similar migration issue to @bsoudan as a result of my own changes in the security wrappers activation script that removes the old wrapper dirs.

So, I believe this solves the migration problem from 16.09 to 17.03 with my setcap wrapper changes, but it doesn't resolve @bsoudan's original problem which I think was separate (though, similar in character) to the issue introduced by my own work.

I tested this on my EC2 instance by switching to a 16.09 system. Logging out. I checked all the wrappers pointed to the path setup by a 16.09 system, switched the config to my branch, checked I could use sudo, su, ping, etc. and that their paths were pointing at the 16.09 wrapper path. I then logged out, logged back in and verified that the new wrapper path was in my $PATH and all of the wrappers I used were pointing to the ones contained in that path.

[abbradar]

You've outrun me :) One potential problem is that new sudo and other setuids won't be used with old environment but that's okay I guess. Thanks!

[globin]

It definitely is better than the current state. :)

[abbradar]

A small idea which should improve this: let's leave this code but wrap it in if [ "$(readlink -f /run/current-system)" = "$(readlink -f /run/booted-system)" ]. This way it'd be removed on the next boot.

[domenkozar]

We should probably merge this for 17.03

[globin]

Yes, It sounded someone was going to improve this but if not I will merge this soon

[ixmatus]

@globin, @abbradar's suggestion is good but I haven't taken the time to think it through and the safest option (to me) is to not remove anything across this specific upgrade boundary, so that we reduce possible mistakes, since we don't have any really good automated tests for this.

If you disagree with that, I'm open to guidance on what would be the preferred integration path.

I will create an issue ticket when you merge this if there aren't additional changes requested.

[abbradar]

Let's just be conservative then; we need this patch in one way or another and this can be improved upon later.

[globin]

Fine :)

[abbradar]

I have my version of the patch but I still need to test it extensively; if I'm on time maybe we'll get the cleanup working too.

[ixmatus]

Okay, I created issue #24164 tracking cleanup work.

CC: @domenkozar @abbradar @globin