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

Relocate firecfg.config to /etc/firejail/ #4669

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

hlein
Copy link
Contributor

@hlein hlein commented Nov 6, 2021

This should make it easier for users, and distributions, to customize
which programs they want firejail to wrap.

Signed-off-by: Hank Leininger [email protected]
Closes: #408
Bug: #2097
Bug: #2829
Bug: #3665

@hlein
Copy link
Contributor Author

hlein commented Nov 6, 2021

N.B. my testing and initial patch was actually against release 0.9.66, but I think this does the right things.

Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

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

There are a few more places to change:

diff --git a/src/firecfg/main.c b/src/firecfg/main.c
index b8fdbcda0..1a05efd29 100644
--- a/src/firecfg/main.c
+++ b/src/firecfg/main.c
@@ -181,7 +181,7 @@ static void set_links_firecfg(void) {
        if (asprintf(&firejail_exec, "%s/bin/firejail", PREFIX) == -1)
                errExit("asprintf");

-       // parse /usr/lib/firejail/firecfg.cfg file
+       // parse /etc/firejail/firecfg.config file
        FILE *fp = fopen(cfgfile, "r");
        if (!fp) {
                perror("fopen");
@@ -440,7 +440,7 @@ int main(int argc, char **argv) {
        // clear all symlinks
        clean();

-       // set new symlinks based on /usr/lib/firejail/firecfg.cfg
+       // set new symlinks based on /etc/firejail/firecfg.config
        set_links_firecfg();

        if (getuid() == 0) {

Misc: The filename is misspelled in these comments as well.

@@ -171,10 +171,10 @@ static void set_file(const char *name, const char *firejail_exec) {
free(fname);
}

// parse /usr/lib/firejail/firecfg.cfg file
// parse /etc/firejail/firecfg.cfg file
Copy link
Collaborator

@kmk3 kmk3 Nov 6, 2021

Choose a reason for hiding this comment

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

Suggested change
// parse /etc/firejail/firecfg.cfg file
// parse /etc/firejail/firecfg.config file

The filename is currently misspelled.

Misc: I noticed it by searching with git grep -F 'firecfg.'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, good catch, I will clean up these misspellings of firecfg.config and update their paths too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, good catch, I will clean up these misspellings of firecfg.config and update their paths too.

Done, I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hlein on Nov 6:

Aha, good catch, I will clean up these misspellings of firecfg.config and
update their paths too.

Thanks.

From the new commit message:

Also fixed some firecfg.cfg -> firecfg.config references.

One minor non-blocking nitpick: I'd s/fixed/fix/ to make it more similar to
this:

This should make it easier for users, and distributions, to customize
which programs they want firejail to wrap. Also fixed some
firecfg.cfg -> firecfg.config references.

Signed-off-by: Hank Leininger <[email protected]>
Closes: netblue30#408
Bug: netblue30#2097
Bug: netblue30#2829
Bug: netblue30#3665
@topimiettinen
Copy link
Collaborator

This reminds me that Firejails should support this kind of administrative hierarchy:

  • /usr/lib/firejail/profiles or /usr/share/firejail/profiles is for distros, based on our set of profiles
  • /etc/firejail/profiles can be used by local system admin to override distro defaults, initially completely empty
  • $XDG_CONFIG_HOME/firejail/profiles (or $HOME/.config/firejail/profiles) can be used by users to override system admin when allowed, initially doesn't exist

Finally, users can override their own config files and other configs with command line switches and environment variables, again when allowed.

In theory, there could be another element for dynamic config generation in /run/firejail/profiles, for example in case the version of the kernel or loading of kernel modules could influence the configs somehow (optimize allowed system calls in seccomp lists? only allow some apps when a USB key is plugged in, kill the app when unplugged?).

Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

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

LGTM.

@netblue30 netblue30 merged commit ba78abd into netblue30:master Nov 11, 2021
@netblue30
Copy link
Owner

Thanks!

kmk3 added a commit that referenced this pull request Nov 15, 2021
@kmk3
Copy link
Collaborator

kmk3 commented Nov 15, 2021

Note: This could be considered a breaking change and is probably relevant to
packagers either way, so I added it to the RELNOTES on commit c374c0c
("RELNOTES: mention move of firecfg.config to /etc/firejail/").

By the way, on the note I included the PR number to make it easier to find.
Let me know if this is undesirable for some reason (and feel free to edit the
note). If not, maybe this could be done by default (where applicable) on the
notes of future releases, but that's probably worth its own discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

firecfg.config location
4 participants