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

Jump to bottom

firejail: local profile handling fixed #77524

Merged
merged 1 commit into from Jan 12, 2020
Merged

firejail: local profile handling fixed #77524

merged 1 commit into from Jan 12, 2020

Conversation

snicket2100
Copy link
Contributor

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.

@snicket2100
firejail: local profile handling fixed
0778f0a 
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 January 11, 2020 20:15
@7c6f434c 7c6f434c merged commit 20e74c9 into NixOS:master Jan 12, 2020
@snicket2100 snicket2100 deleted the firejail-local-profiles-fixed branch January 12, 2020 16:14
@thatsmydoing
Copy link
Contributor

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

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

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

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

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

@thatsmydoing
Copy link
Contributor

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

@snicket2100
Copy link
Contributor Author

@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
Reviewers

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@snicket2100 @thatsmydoing @7c6f434c