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

security.wrappers: init #890

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

Samasaur1
Copy link
Contributor

This PR adds security.wrappers to nix-darwin, closing #618. It's heavily modeled after how NixOS implements the same option. On Darwin, it allows setting ownership, permissions, setuid/setgid, and (not yet implemented) codesigning. I've tested this PR, and it does meet the minimal use case of providing a setuid wrapper to a given program.

However, I have some concerns that should be addressed before it is merged:

  • Is the generated wrapper "secure"? The NixOS implementation took a lot of steps to ensure security, some of which I could not replicate on Darwin. For example, multiple lists of unsecure environment variables — loaded from other derivations, not defined as constants — were combined into the wrapper. I've copied what the env vars were at the time of writing, but this isn't great for maintenance
  • Supporting codesigning. I wasn't exactly clear on what the goal was here, only that it was mentioned in feature request: security.wrappers #618. I left a stub in this implementation which should hopefully allow implementing codesigning without a huge amount of reworking.
  • Am I correct that setting capabilities is not a thing on Darwin?
  • NixOS does not let you use setcap and setuid on the same wrapper. Although I didn't implement setcap at all, I do allow codesigning and setuid/setgid on the same wrapper. Is there a reason to disallow this?

These are just the concerns that I have — if others have concerns (about the concept as a whole or about my implementation), please leave a comment.

@Samasaur1
Copy link
Contributor Author

Is it possible that codesigning is desired so that entitlements can be applied to binaries?

@Samasaur1
Copy link
Contributor Author

Okay, I have removed codesigning from this PR, so it now only supports setuid/setgid. I've left the commented-out codesigning code in place for now, but I can remove it if desired.

I'm marking this PR as ready for review, and hopefully reviewers can look at wrapper.c/wrapper.nix in this PR versus wrapper.c and wrapper.nix in nixpkgs, with a focus on the security of the wrappers.

@Samasaur1 Samasaur1 marked this pull request as ready for review February 28, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant