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

firejail: local profile handling fixed #77524

Merged
merged 1 commit into from Jan 12, 2020

Conversation

@snicket2100
Copy link
Contributor

@snicket2100 snicket2100 commented Jan 11, 2020

The sed expression wasn't really catching anything (as local profiles are included in the provided set of profiles by include aaa.local and not by include xx/firejail/aaa.local as the sed expression used to expect). As a result, it was not possible to create local profiles in any accessible location. This fix makes it possible to create them in /etc/firejail/ which seems pretty standard.

Motivation for this change

as above

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
The sed expression wasn't really catching anything (as local profiles are
included in the provided set of profiles by `include aaa.local` and not by
`include xx/firejail/aaa.local` as the sed expression used to expect).
As a result, it was not possible to create local profiles in any
accessible location. This fix makes it possible to create them in
`/etc/firejail/` which seems pretty standard.
@ofborg ofborg bot requested a review from 7c6f434c Jan 11, 2020
@7c6f434c 7c6f434c merged commit 20e74c9 into NixOS:master Jan 12, 2020
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
firejail on aarch64-linux Success
Details
firejail on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@snicket2100 snicket2100 deleted the snicket2100:firejail-local-profiles-fixed branch Jan 12, 2020
@thatsmydoing
Copy link
Contributor

@thatsmydoing thatsmydoing commented Mar 26, 2020

Prior to this fix, was configuration in /etc/firejail not being picked up? I'm not really sure with how firejail handles includes like these, but this actually broke local profile handling for me from 19.09 to nixos-unstable. I put my customization files in ~/.config/firejail and that used to work, but now with includes hardcoded to /etc/firejail, they don't anymore and I have to move my configuration there instead.

@thatsmydoing
Copy link
Contributor

@thatsmydoing thatsmydoing commented Mar 26, 2020

Looking at the firejail code, https://github.com/netblue30/firejail/blob/e4cb6b42743ad18bd11d07fd32b51e8576239318/src/firejail/profile.c#L68-L83

It only looks in either SYSCONFDIR, which in this case is the nix store, and the user configuration ~/.config/firejail directories. So to preserve the original behavior of supporting both /etc/firejail and ~/.config/firejail, it would be necessary to either patch the code or symlink the main files into /etc

@snicket2100
Copy link
Contributor Author

@snicket2100 snicket2100 commented Mar 27, 2020

Aaah, damn, I didn't even realize that this piece of code handling ~/.config/firejail is there :/ Wanted to put my local customizations into /etc/firejail as they seem easier to manage this way. No idea how to easily make both of these folders work in parallel, patching firejail in the NixOS build seems a bit over the top, would rather push it to the firejail upstream if anything but not sure that I can justify it.

What do you think?

We can also try duplicating each include .*.local line so that it tries bots /etc/firejail as well as the relative path (which will end up looking in ~/.config/firejail). The disadvantage is that when you will be adding your own includes you will still need to be specific.

@thatsmydoing
Copy link
Contributor

@thatsmydoing thatsmydoing commented Mar 27, 2020

Oh, if it looks in the nix store by default instead of /etc/firejail what about creating a corresponding program.local file inside the nix store that includes /etc/firejail/program.local? That way we don't have to modify the existing files and it should be able to find both maybe?

@snicket2100
Copy link
Contributor Author

@snicket2100 snicket2100 commented Mar 27, 2020

Yeah, that could work! So you are suggesting automatically generating a set of program.local files in the firejail derivation?

@thatsmydoing
Copy link
Contributor

@thatsmydoing thatsmydoing commented Mar 27, 2020

Yes, if we can support both locations, that would be ideal.

@snicket2100
Copy link
Contributor Author

@snicket2100 snicket2100 commented Mar 27, 2020

@thatsmydoing I think this should fix your problem #83515, I have checked that it still uses local profiles from /etc/firejail. Would be great if you could confirm that it fixes the problem for you.

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

3 participants
You can’t perform that action at this time.