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.

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

Adding setcap-wrapper functionality to Nix #16654

Merged
merged 78 commits into from Feb 14, 2017

Conversation

@ixmatus
Copy link
Contributor

@ixmatus ixmatus commented Jul 1, 2016

Motivation for this change

Resolves #6768

Setting capabilities on programs is desirable over setuid because it improves the granularity of security. Nix wasn't able to support setcap-wrappers in the style of setuid-wrappers until recently when the Linux 4.4 kernel was added. Linux 4.3 included a capability named Ambient which enables a wrapper program to propagate its capabilities to an execv'ed program.

This PR adds support for setcap-wrappers (with notable improvements to the wrapper program over setuid-wrapper.c) and adds a conditional task that sets ping and ping6 to be setcap wrapped if the kernel is at-least 4.3, if not then we default to the setuid behavior.

Things done
  • Tested using sandboxing
    (nix.useChroot on NixOS,
    or option build-use-chroot in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • 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/), NOTE: only tested ping and ping6
  • Fits CONTRIBUTING.md.

I tested both a sub 4.3 kernel (it defaulted to the setuid'ed ping and ping6) and with a 4.4 kernel (it defaulted to setcap'ed ping and ping6).

@mention-bot
Copy link

@mention-bot mention-bot commented Jul 1, 2016

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

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Jul 1, 2016

Interesting!

I think there are at least 3 commits that are better squashed (MHO).

@bjornfor
bjornfor reviewed Jul 1, 2016
View changes
nixos/modules/security/setcap-wrappers.nix Outdated
pkgs.stdenv.mkDerivation {
name = "setcap-wrapper";
unpackPhase = "true";
buildInputs = [ linuxHeaders_4_4 libcap libcap_ng ];

This comment has been minimized.

@bjornfor

bjornfor Jul 1, 2016
Contributor

It doesn't strike me as a particularly good idea to hardcode the linux header version here (linuxHeaders_4_4). Isn't there an attribute for the "current" kernel header package?

This comment has been minimized.

@ixmatus

ixmatus Jul 1, 2016
Author Contributor

Yes, you're right, that should just be linuxHeaders I believe. I had to specify a hard kernel header version because stable doesn't have the 4.4 headers in it (yet) I had to copy & inject the headers. I'll fix this.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Jul 1, 2016

Yeah a bunch can be squashed but I didn't do it not knowing how you guys
felt. Also we could squash merge the PR once it's approved (unless the
process is merge, not squash)...

On Fri, Jul 1, 2016 at 2:55 PM, Bjørn Forsman notifications@github.com
wrote:

Interesting!

I think there are at least 3 commits that are better squashed (MHO).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16654 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAB3-otwQGsZgiOe9rXYsa5eB82QbCkpks5qRXDFgaJpZM4JDbKB
.

Parnell Springmeyer
parnell@digitalmentat.com | digitalmentat.com | 0xDCCF89258EAD874A
http://pgp.mit.edu/pks/lookup?op=get&search=0xDCCF89258EAD874A

@fpletz
Copy link
Member

@fpletz fpletz commented Jul 2, 2016

Awesome! Thanks a lot for your work! Will test later and give some more feedback.

@fpletz
fpletz reviewed Jul 2, 2016
View changes
nixos/modules/security/setcap-wrappers.nix Outdated
owner = "nobody";
group = "nogroup";
setcap = true;
capabilities = "cap_net_raw+ep";

This comment has been minimized.

@fpletz

fpletz Jul 2, 2016
Member

Could we maybe use a nix list here? Is the format to specify the capabilities documented somewhere? May be worth mentioning where to find it in the description.

This comment has been minimized.

@joachifm

joachifm Jul 2, 2016
Contributor

@fpletz the format is documented in cap_from_text(3)

This comment has been minimized.

@ixmatus

ixmatus Jul 5, 2016
Author Contributor

I'll add some documentation about where one would find the capabilities format cap_from_text and setcap expect, great point.

@fpletz
fpletz reviewed Jul 2, 2016
View changes
nixos/modules/security/setcap-wrappers.nix Outdated
{
options = {
security.setcapCapabilities = mkOption {
type = types.listOf types.attrs;

This comment has been minimized.

@fpletz

fpletz Jul 2, 2016
Member

To validate the the attribute sets you could to use types.listOf types.submodule here.

@edolstra
Copy link
Member

@edolstra edolstra commented Jul 4, 2016

This should be merged with the setuid wrapper so that we don't have to maintain two very similar programs, and bloat $PATH with yet another directory. Capabilities could be specified as an attribute in security.setuidOwners (which should probably be renamed).

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Jul 5, 2016

@edolstra that's a great idea. It was separated originally simply because we needed the feature and just wanted to inject it into our modules tree but now that you're saying this I definitely agree it should be unified.

Additionally, we made significant improvements to the C wrapper program that the setuid C wrapper program should benefit from so I think it's very reasonable to merge these two features together.

@edolstra would you like me to begin making those changes in this PR? Or close this one and open a new PR with the two features merged? I could also do them in a separate branch and open a PR to this branch so we can review the changes required to merge to the features, then once approved approve this PR.

@ttuegel
Copy link
Member

@ttuegel ttuegel commented Jul 15, 2016

Just adding my 👍 for KDE, which could use a setcap wrapper (currently using a setuid wrapper).

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Jul 15, 2016

Okay cool. Because I haven't had a response on how maintainers want the PR for merging setuid-wrapper and setcap-wrapper I'm just going to branch off of this branch and submit a PR for THIS branch.

@fpletz
Copy link
Member

@fpletz fpletz commented Jul 15, 2016

@ixmatus Both options seem fine to me. Please just go ahead. Thanks a lot for your work. 👍

@grahamc
Copy link
Member

@grahamc grahamc commented Jul 21, 2016

Just want to put a bump on this and say I look forward to having it in mainline.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Jul 21, 2016

Thanks 😃 I'm almost done with merging the two wrapper functionalities
(what's left is testing, actually) so soon!

On Jul 20, 2016 8:33 PM, "Graham Christensen" notifications@github.com
wrote:

Just want to put a bump on this and say I look forward to having it in
mainline.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16654 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAB3-ljiLeUZ8ox_kqN-V9VRuTRUez-oks5qXsyGgaJpZM4JDbKB
.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Aug 1, 2016

Sorry I've been MIA everyone - I have this 90% complete, just haven't found a few hours to work on this amongst all of the travel for work I've been doing. I should be able to finish this week so everyone can review.

@joachifm joachifm added this to the 16.09 milestone Aug 19, 2016
@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Aug 25, 2016

@joachifm how much time do I have before the 16.09 release? We've been swamped here but would love to see this in 16.09 so I can prioritize time for this if it's coming soon.

@joachifm
Copy link
Contributor

@joachifm joachifm commented Aug 25, 2016

@ixmatus the branch-off is scheduled for next week (I don't recall if branch-off is at last day of the month or 1st of the next one, so either wednesday or thursday). Is there anything we can do to help move things along?

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Aug 26, 2016

@joachifm okay thanks. I just saw the email message hiding in my folder from a month ago!

Well, as this branch stands, it works. But I'm right in the middle of addressing @edolstra's feedback; I should be able to get to that this weekend. Though I will say that there's some production mileage on our own systems of the current formulation that his PR represents.

As a suggestion, it might be "safer" to merge this one and run with it, then come back in after release and refactor. Though I know that's not preferable because it's easy to forget to do that...

I'm proceeding with addressing his feedback, will try to knock it out over the weekend, and merging the two functionalities but if you feel like the above strategy (merging what is here now and works) is better, just let me know.

@joachifm
Copy link
Contributor

@joachifm joachifm commented Aug 26, 2016

@ixmatus so the only thing that's currently missing is unifying setcap and setuid? If so, my preference would be to merge it, but that's not my call to make.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Aug 28, 2016

It's hard to make that trade-off.

I've seen many times such refactorings don't happen in future, because bugs are just fixed and there's never enough initiative to rewrite/merge.

This PR should follow the boyscout rule and refactor setuid-wrapper to introduce new functionality.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Aug 28, 2016

👍 I'm working on finishing up the merged functionality today.

On Aug 28, 2016 7:00 AM, "Domen Kožar" notifications@github.com wrote:

It's hard to make that tradeoff.

I've seen many times such refactorings don't happen in future, because
bugs are just fixed and there's never enough initiative to rewrite/merge.

This PR should follow the boyscout rule
http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule
and refactor setuid-wrapper to introduce new functionality.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16654 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAB3-sVa2Lk-FQYqj9nxj4XvkoGVRSmuks5qkXhRgaJpZM4JDbKB
.

@grahamc
Copy link
Member

@grahamc grahamc commented Aug 29, 2016

This looks really great. How did the merging go?


Reviewed 6 of 7 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Aug 29, 2016

Honestly, very slowly. I also moved to a new permanent residence this
weekend so I'm hoping to tackle it tomorrow night.

On Sun, Aug 28, 2016 at 9:40 PM, Graham Christensen <
notifications@github.com> wrote:

This looks really great. How did the merging go?

Reviewed 6 of 7 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved

discussions.

Comments from Reviewable
https://reviewable.io:443/reviews/nixos/nixpkgs/16654#-:-KQJ9Tt9zoI8Px3GH06L:b-cgdg9z


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#16654 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAB3-vygx_AhQAFsbe_VofLNz9_IypI8ks5qkkaTgaJpZM4JDbKB
.

Parnell Springmeyer
parnell@digitalmentat.com | digitalmentat.com | 0xDCCF89258EAD874A
http://pgp.mit.edu/pks/lookup?op=get&search=0xDCCF89258EAD874A

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Aug 30, 2016

I'm being given a day by my employer tomorrow (Wednesday) to knock this out, I'll be working on it then. Sorry for the delay everyone.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Sep 1, 2016

Well, the 1st of this month has come - I made progress but I don't feel confident yet in the merged functionality, I want to refactor the code a bit more and do more testing. I think unless the freeze gets extended, this will have to wait until the next release or make it into a patch release.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Sep 1, 2016

I'm continuing to work on it today so hopefully I can get something I feel confident about in today - but not sure if that's cutting it too close for the freeze.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Sep 1, 2016

If the feature is complete and you're willing to fix any bugs in upcoming month, we can merge tgis today.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Sep 1, 2016

@domenkozar yeah I am willing, I'll push to this branch in a few hours after a bit more testing.

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Feb 15, 2017

Why did ping change permissions?

New:

-r-x--x--x 1 root root       17680 Feb 15 08:36 ping

vs old (on release-16.09):

-r-s--x--x 1 root root       12856 Feb 15 08:36 ping

I guess this reflects a change in the defaults of the wrappers?

@globin
Copy link
Member

@globin globin commented Feb 15, 2017

Probably ping is using the raw network capability now instead of setuid before or similar

@teh
Copy link
Contributor

@teh teh commented Feb 15, 2017

@bjornfor

Yea, what Robin said. Removing the s bit when possible is the main idea behind this branch.

You can set before running ping WRAPPER_DEBUG=1 and it should print some debug info on what caps it sets.

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Feb 15, 2017

Thanks for the info.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Feb 15, 2017

@bjornfor thank you for catching those mistakes! Is the test suite on Hydra something that can be run, say, on my own EC2 machine so that I could catch these mistakes next time while developing? Do we have developer documentation outlining how to run such a test suite? It would be nice to catch these things earlier.

The list to attrset improvement was suggested by @edolstra in this PR and I like it better than the list-based approach as it keeps it consistent with the other APIs in NixOS that expose a similar interface.

And yes, the only two programs I updated to use the new setcap feature are ping and ping6 since those two are the canonical example of using Linux capabilities. This PR paved the way and I expect more of the programs currently being setuid wrapped need to be setcap wrapped with the appropriate capabilities on them.

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Feb 15, 2017

Is the test suite on Hydra something that can be run, say, on my own EC2 machine [...]

I know very little about how Hydra works, but I know a bunch of nixos tests exist in the nixos/tests directory. A single test can be run with nix-build nixos/tests/<suite>. Not sure if the test coverage is sufficient to catch the errors I faced. (I was simply doing nixos-rebuild boot on my system, unfortunately.)

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Feb 15, 2017

Okay, I'll investigate using the test suite next time, thanks. Hmm I was also running nixos-rebuild boot on my EC2 instance, did you have a different configuration.nix with some things enabled that a stock configuration.nix might not?

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Feb 15, 2017

@ixmatus: Yes, I have enabled extra services. And because I don't have enabled "all", there may very well be some places in nixos that are still broken due to this change... (Hence I wish this API change would have been handled a bit more carefully...)

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Feb 15, 2017

@edolstra, @ixmatus: The documentation for security.wrappers say that 'source' is optional, and if unset the program will be looked up in $PATH. That is not true. If 'source' is not set nixos-rebuild fails...

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Feb 15, 2017

@bjornfor this API change / PR has been around for more than six months and I'm completely engaged in helping to fix things. What do you suggest I should do different now or next time to make this more graceful or careful?

Yes that documentation is stale, I will fix it ASAP. Thanks for pointing it out.

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Feb 15, 2017

Already done: ce0a52f

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Feb 15, 2017

Also, maintainers appeared unphased that travis reports a red X on builds of PRs so I was assuming that the builds are broken somewhere and it is acknowledged but not blocking merges, or builds are happening elsewhere (on hydra?) that validate work to be merged.

I'm very open to helping with project short-comings (to make merges safer) and receiving constructive feedback on how I could have gone about something better or different.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Feb 15, 2017

I will try enabling all services and building on my EC2 instance to see if other places fail; I'm going to ag for the wrapper interface and then enable all the services I find and build to see if there are more failures.

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Feb 15, 2017

Also, I'm sorry my change is breaking a few things. I will try a wider build and clean up whatever fails, I will take ownership of not having thought to do that entirely for all my dev test builds :)

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Feb 15, 2017

@ixmatus: Unfortunately Hydra doesn't build PRs (yet). So the (real) CI only catches problems after the fact. (Travis CI doesn't check much compared to Hydra.) This is IMHO one of the biggest problems with nixpkgs development. (Along with "who decides what"...) There is no automation to prevent untested features entering master, and people generally don't care enough (IMHO) about stability on master. (And there are many committers -- both good and bad!)

I think NixOS/hydra#95 is very important. And whatever we need to get to a place where Hydra is more or less a "gateway" for code to enter master branch.

Some might argue that problems are caught within "reasonable" time with the current setup. But I think we should strive for better quality on master branch :-)

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Feb 15, 2017

@bjornfor: okay I ran a nixos-rebuild build on my EC2 machine with this configuration.nix:

{
  imports = [ <nixpkgs/nixos/modules/virtualisation/amazon-image.nix> ];
  ec2.hvm = true;

  virtualisation.virtualbox.host.enable = true;
  services.locate.enable = true;
  services.exim.enable = true;
  services.xserver.desktopManager.kde5.enable = true;
  services.xserver.desktopManager.enlightenment.enable = true;
  services.cron.enable = true;
  services.fcron.enable = true;
  services.atd.enable = true;

# trace: warning: You must run gale-install in order to generate a domain key.
#  services.gale.enable = true;
  services.smokeping.enable = true;

# - PAM support is currently not implemented.
#  security.duosec.pam.enable = true;
  security.chromiumSuidSandbox.enable = true;

# A Nix error is happening here unrelated to security wrappers I think
#  security.pam.enableEcryptfs.enable = true;
  security.polkit.enable = true;
  security.pam.usb.enable = true;
  programs.light.enable = true;
  programs.kbdlight.enable = true;
}

My build succeeded with those lines commented out and I left a comment with the failure. I believe gale can be fixed so I can enable it and test that. I'm not positive what the PAM support is currently not implemented assertion message is, I will try digging into that to see what's going on there.

And finally pam.enableEcryptfs.enable = true; generated an error unrelated to the wrapper API change, I'll put a PR up to fix that ASAP.

I think I'm covering the build pretty well here. I will switch my system to this config and check all of the executables being wrapped to make sure it went the way it was supposed to. A couple of modules have some tricky logic for conditionally including certain executables in the wrappers attrset so I'll to spend some time tonight picking that apart and making sure those code paths are exercised.

Also, yes, I think better CI would help NixOS a lot; I have experience working with CI for NixOS systems because we've had to make it as smooth as possible at work where we're using NixOS. I'm more than happy to help with this but not quite sure where to start or if I have permissions to do this kind of work or what is currently being worked on.

@rycee
Copy link
Member

@rycee rycee commented Feb 15, 2017

Umm, for future merges I would appreciate some kind of commit cleanup. Ideally into a sequence of reasonably small commits that can be individually reviewed and with clear descriptive commit messages.

I happened to think of this while browsing the log since my last pull to keep myself up to date. Was having a difficult time with commits such as

a26a796 Merging against master - updating smokingpig, rebase was going to be messy
ad8fde5 Andddd more derp
ce36b58 Derp
f64b06a Hmmm
fd97408 It's clearly quite late
61fe8de Silly, should just have one activation script
48a0c5a More fixing
21368c4 Hmm, unnecessary
a4f905a Enhhh I think compile time macros are gross
785684f Ahhh, my compile-time macros confused me...of course they did...
1ad5411 Hmm
e8bec4c Implicit declared function...
a20e657 Fixing
@teh
Copy link
Contributor

@teh teh commented Feb 15, 2017

+1 for flatting commit history next time, this is also hard to back out

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Feb 15, 2017

I'm more than happy to help with this but not quite sure where to start or if I have permissions to do this kind of work or what is currently being worked on.

Good! I don't know where to start either. IMHO, another big pain point of NixOS is exactly the lack of knowing who has power to do what, and who has the final say in matters... and that some core people are overloaded (or so I've heard).

@ixmatus
Copy link
Contributor Author

@ixmatus ixmatus commented Feb 15, 2017

Yeah, do we squash-merge PRs (or is there a community guideline against that?) That would have at least cleaned it up going into master...

That feedback for general PR hygiene is totally fair and I completely understand the reason, it's painful for me when I have to review PRs that aren't cleaned up or broken into smaller pieces.

This PR has seen a lot of churn (the initial PR had a single clean commit that was squashed) and my personal and work life are such that I don't always have the time I want to devote to OSS projects and I was worried I'd lose momentum again if I started juggling multiple PRs and maintaining those PRs against master.

So, I appreciate people's patience with the messy PR and commit history and I'll try to do better next time, free-time and momentum permitting.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 15, 2017

@bjornfor
Copy link
Contributor

@bjornfor bjornfor commented Feb 16, 2017

I take offense with the statement that any of the committers can be
classified into either of these categories! (well, except Eelco,
obviously, he is good by definition)

Oh, that's not what I meant! (Sorry for bad wording.) I mean that many committers to the main repo is both a blessing and a curse. (I think a hierarchical approach, like the topic specific sub-trees of Linux development is a better model. AFAIK, only Linus has commit access to the main repo.)

Hm, what will be the live non-gridlocked branch then? Some updates have
large but rarely used reverse dependencies…

You mean where to push updates that will cause some regressions that negatively affect some, however few, people? Difficult question, but I think we have to start by knowing the effects of a change before it enters master. Then we can decide on whether that change should go in or not.

We already have a definition of a "working" master branch: it's where Hydra builds pass and a new channel is produced. And this "working" master branch still has many packages in a broken state, but those packages are not blockers.

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Feb 16, 2017

globin added a commit that referenced this pull request Feb 16, 2017
The replacements matched to much due to wrapperDir having `/bin` in its
path now.

cc #16654
@Mic92 Mic92 mentioned this pull request Feb 21, 2017
4 of 7 tasks complete
globin added a commit that referenced this pull request Mar 23, 2017
This makes setuid wrappers not fail after upgrading.

references #23641, #22914, #19862, #16654
globin added a commit that referenced this pull request Mar 23, 2017
This makes setuid wrappers not fail after upgrading.

references #23641, #22914, #19862, #16654

(cherry picked from commit e82baf0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.