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

Jump to bottom

nixos/firejail: add wrappedPackages option #45469

Closed

dotlambda wants to merge 1 commit into NixOS:master from dotlambda:firejail-wrapped-packages

Member

dotlambda commented Aug 22, 2018 •

edited

Motivation for this change

Using this, not only the wrapped binaries but also all other directories from the derivations are put into /run/current-system/sw. This is useful if you need desktop entries, icons, etc.

My buildCommand is certainly far from perfect because I'm not good at programming bash. I'm open for suggestions.

Things done

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.

cc @peterhoeg @adisbladis

dotlambda requested a review from peterhoeg

August 22, 2018 15:51

dotlambda force-pushed the firejail-wrapped-packages branch from 0fc54a0 to 1b9e616 Compare

August 22, 2018 15:53

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels

dotlambda mentioned this pull request

Vulnerabilities in ghostscript and imagemagick #45458

Closed

Member Author

dotlambda commented Aug 26, 2018 •

edited

This is a reminder for myself: I should try to do this using symlinkJoin. See e.g. https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/video/mpv/wrapper.nix

timokau reviewed

View reviewed changes

nixos/modules/programs/firejail.nix Outdated

    
                    name = "firejail-" + pkg.name;

                    buildCommand = ''

                      mkdir -p $out

                      ln -s ${pkg}/* $out

Member

timokau Aug 26, 2018

You should quote variables like ${pkg} and $out as they may contain spaces (in case of a store path with spaces) and confuse bash.

Member

timokau Aug 26, 2018

Be careful not to include the glob in the quote though, so "${pkg}"/*

timokau reviewed

View reviewed changes

nixos/modules/programs/firejail.nix Outdated

    
                    buildCommand = ''

                      mkdir -p $out

                      ln -s ${pkg}/* $out

                      rm $out/bin

Member

timokau Aug 26, 2018

What if the package doesn't have a bin directory? Doesn't make much sense in combination with firejail, but might happen. In that case there should probably be a useful error message.

timokau reviewed

View reviewed changes

nixos/modules/programs/firejail.nix Outdated

    
                      ln -s ${pkg}/* $out

                      rm $out/bin

                      mkdir $out/bin

                      for bin in ${pkg}/bin/*; do

Member

timokau Aug 26, 2018

I think the variable name bin is confusing here. Maybe dir instead?

Member Author

dotlambda Aug 31, 2018

These are actual extecutables.

Member

timokau Aug 31, 2018

Oh nevermind, I don't know why I thought they were subdirectories.

timokau reviewed

View reviewed changes

nixos/modules/programs/firejail.nix Outdated

    
                      rm $out/bin

                      mkdir $out/bin

                      for bin in ${pkg}/bin/*; do

                      newbin="$out/bin/$(basename $bin)"

Member

timokau Aug 26, 2018

$bin should also be quoted. $() allows nested quotes, so

newbin="$out/bin/$( basename "$bin")"

Although why is basename even needed here?

nixos/modules/programs/firejail.nix Outdated

    
                      mkdir $out/bin

                      for bin in ${pkg}/bin/*; do

                      newbin="$out/bin/$(basename $bin)"

                      cat <<_EOF >$newbin

Member

timokau Aug 26, 2018

Why the underscore? $newbin needs to be quoted as well.

nixos/modules/programs/firejail.nix Outdated

    
                      newbin="$out/bin/$(basename $bin)"

                      cat <<_EOF >$newbin

                      #!${pkgs.stdenv.shell} -e

                      /run/wrappers/bin/firejail $bin "\$@"

Member

timokau Aug 26, 2018

quote

nixos/modules/programs/firejail.nix Outdated

    
                      #!${pkgs.stdenv.shell} -e

                      /run/wrappers/bin/firejail $bin "\$@"

                      _EOF

                      chmod 0755 $newbin

Member

timokau Aug 26, 2018

quote


          nixos/firejail: add wrappedPackages option

a85e19b

Using this, not only the wrapped binaries but also all other
directories from the derivations are put into /run/current-system/sw.
This is useful if you need desktop entries, icons, etc.

dotlambda force-pushed the firejail-wrapped-packages branch from 1b9e616 to a85e19b Compare

August 31, 2018 12:34

matthewbauer reviewed

View reviewed changes

nixos/modules/programs/firejail.nix

+                  pkgs.symlinkJoin {
+                    name = "firejail-" + pkg.name;
+                    paths = [ pkg ];
+                    buildInputs = with pkgs; [ tree ];

Member

matthewbauer Sep 29, 2018

I don't think this should be needed as a build input. All of the below commands are included in stdenv.

matthewbauer reviewed

View reviewed changes

nixos/modules/programs/firejail.nix

+                    buildInputs = with pkgs; [ tree ];
+                    postBuild = ''
+                      for bin in $(find "$out/bin" -type l); do
+                      oldbin="$(readlink "$bin")"

Member

matthewbauer Sep 29, 2018

Should be indented here.

matthewbauer reviewed

View reviewed changes

nixos/modules/programs/firejail.nix

@@ @@ -20,13 +19,37 @@ let @@
                   '';
                 };
+                wrappedPkgs = map (pkg:

Member

matthewbauer Sep 29, 2018

I think naming here is a little confusing. wrappedPkgs vs wrappedPackages seems like they should be the same thing (although I suppose it's not any worse than wrappedBins vs wrappedBinaries).

peterhoeg mentioned this pull request

firejail: override files should be in /etc/firejail #47718

Merged

9 tasks

peterhoeg reviewed

View reviewed changes

nixos/modules/programs/firejail.nix

+                      for bin in $(find "$out/bin" -type l); do
+                      oldbin="$(readlink "$bin")"
+                      rm "$bin"
+                      cat <<_EOF >"$bin"

Member

peterhoeg Oct 3, 2018

Instead of manually creating shell scripts (they are admittedly basic), just use makeWrapper.

  makeWrapper $bin /run/wrappers/bin/firejail \
    --argv0 $bin \
    --add-flags $oldbin

Contributor

ryneeverett Jan 2, 2021

I tried this before analyzing it and it failed with mkdir: cannot create directory '/run': Permission denied because the second argument to makeWrapper is the destination it tries to create. I don't see how makeWrapper could be made to work here.

Member

mmahut commented Aug 9, 2019

Any updates on this pull request, please?

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the
related code and @ mention them in a comment.
Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
irc.freenode.net.

stale bot added the 2.status: stale label

Member

KamilaBorowska commented Jun 15, 2020

still important to me

stale bot removed the 2.status: stale label

Member Author

dotlambda commented Jun 15, 2020

@xfix Feel free to finish this based on my work.

I think we should mention in the docs that desktop files stating an absolute path will start non-wrapped binaries. Maybe we could implement some check & warning for that.

Member

peterhoeg commented Jun 15, 2020 via email

Or how about a wrapper for use by .desktop files which could then be used for firejail, bubblewrap, systemd-run, etc?

ryantm added 2.status: merge conflict and removed 2.status: merge conflict labels

ryantm added the 2.status: merge conflict label

ryneeverett mentioned this pull request

nixos/firejail: wrappedPackages #108204

Open

10 tasks

Member

SuperSandro2000 commented Jan 2, 2021

Replaced by #108204

SuperSandro2000 closed this

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