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/weechat: add setuid wrapper for `screen' to ensure true multiuser capabilities #48131

Merged
merged 1 commit into from Oct 17, 2018
Merged

nixos/weechat: add setuid wrapper for `screen' to ensure true multiuser capabilities #48131

merged 1 commit into from Oct 17, 2018

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Oct 9, 2018 •
edited

Things done

Previously you either had to set the setuid bit yourself or workaround
isSystemUser = true (for a loginable shell) to access the weechat
screen.

programs.screen shouldn't do this by default to avoid taking too much
assumptions about the setup, however services.weechat explicitly
requires tihs.

See #45728

  • 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.

andrew-d
andrew-d reviewed Oct 9, 2018
View reviewed changes
nixos/modules/services/misc/weechat.nix Outdated
@@ -46,10 +46,12 @@ in
Group = "weechat";
RemainAfterExit = "yes";
};
script = "exec ${pkgs.screen}/bin/screen -Dm -S ${cfg.sessionName} ${cfg.binary}";
script = "exec /run/wrappers/bin/screen -Dm -S ${cfg.sessionName} ${cfg.binary}";
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want this to be ${config.security.wrapperDir}/bin/screen? It appears that a lot of places hard-code the path /run/wrappers, so not super-clear that this is a huge deal 😃

@Ma27
nixos/weechat: add setuid wrapper for `screen' to ensure true multius…
018573b 
…er capabilities

Previously you either had to set the setuid bit yourself or workaround
`isSystemUser = true` (for a loginable shell) to access the weechat
screen.

`programs.screen` shouldn't do this by default to avoid taking too much
assumptions about the setup, however `services.weechat` explicitly
requires tihs.

See NixOS#45728
@Ma27
Copy link
Member Author

Ma27 commented Oct 10, 2018

@andrew-d thanks, fixed!:)

@teto
Copy link
Member

teto commented Oct 11, 2018

Would it be possible to make it so that it can work with tmux too ? I thought screen was not maintained anymore.

@Ma27
Copy link
Member Author

Ma27 commented Oct 11, 2018

I thought screen was not maintained anymore.

do you have a source for that? I'm still seeing screen used fairly often.

IIRC the original weechat PR had a similar discussion, in the end I used to original change and implemented some minor improvements, so feel free to add tmux support :D

In this PR it's IMHO out of scope though.

@Ma27
Copy link
Member Author

Ma27 commented Oct 16, 2018

Are there any people against this change? Otherwise I'd merge this in about 24 hours :)

@Ma27
Copy link
Member Author

Ma27 commented Oct 17, 2018

A review and the approval of another maintainer should be fine I guess :)

@Ma27 Ma27 merged commit 13e4110 into NixOS:master Oct 17, 2018
@Ma27 Ma27 deleted the weechat-multiuser-support branch October 17, 2018 21:39
@Ma27
Copy link
Member Author

Ma27 commented Oct 17, 2018

this properly fixes multi-user support, so I backported this as bfb61a7

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

@andrew-d andrew-d andrew-d left review comments

@matthewbauer matthewbauer matthewbauer approved these changes

Assignees
No one assigned
Labels
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
6 participants
@Ma27 @teto @zimbatm @matthewbauer @andrew-d @GrahamcOfBorg