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

Stop warning on safe supplementary group clean #5114

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

kmk3
Copy link
Collaborator

@kmk3 kmk3 commented Apr 22, 2022

When nogroups is used, the following warning may be issued (potentially
multiple times, as drop_privs may be called more than once):

Warning: cleaning all supplementary groups

But the warning is being shown even when it seems that all supplementary
groups can be safely dropped (and are thus dropped), which is likely a
common scenario. This commit prevents the warning from being printed in
that case, making it so that it is only shown in the non-happy paths (as
was the case on firejail 0.9.66).

Misc: The added code was copied from drop_privs.

This amends commit 7abce0b ("Fix keeping certain groups with
nogroups", 2021-11-30) / PR #4732.

Kind of relates to #4930.

When nogroups is used, the following warning may be issued (potentially
multiple times, as drop_privs may be called more than once):

    Warning: cleaning all supplementary groups

But the warning is being shown even when it seems that all supplementary
groups can be safely dropped (and are thus dropped), which is likely a
common scenario.  This commit prevents the warning from being printed in
that case, making it so that it is only shown in the non-happy paths (as
was the case on firejail 0.9.66).

Misc: The added code was copied from drop_privs.

This amends commit 7abce0b ("Fix keeping certain groups with
nogroups", 2021-11-30) / PR netblue30#4732.

Kind of relates to netblue30#4930.
@kmk3 kmk3 added this to In progress in Release 0.9.70 via automation Apr 22, 2022
Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

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

Can't really be sure on the coding side of this PR, but it definately deserves full credit for the effort to do something about these superfluous (hence potentially confusing) warnings: +1!

@kmk3
Copy link
Collaborator Author

kmk3 commented Apr 22, 2022

@glitsj16 left a comment:

Can't really be sure on the coding side of this PR, but it definately
deserves full credit for the effort to do something about these superfluous
(hence potentially confusing) warnings: +1!

Thanks; sorry for taking so long to fix this.

This is the one part of the code that I changed without having a good enough
understanding of it and now it's rather messy, but I'm working on fixing that.

There's one more notable bugfix to post and a WIP refactor/unification of group
handling, which should simplify some things a lot.

Though I can't see an easy fix for one key part, which is the fact that
clean_supplementary_groups may be called during argument parsing but
(since #4725/#4732) it depends on arg_ variables.

If all arguments/profile entries were parsed before doing anything "important",
I think it would make using arg_ variables a lot safer/more robust. I
remember someone suggesting something like this in an issue (or maybe in a
comment), but I can't find it at the moment.

@netblue30 netblue30 merged commit f35ac46 into netblue30:master Apr 25, 2022
@netblue30
Copy link
Owner

all in, thanks!

@kmk3 kmk3 deleted the stop-warn-group-clean branch April 26, 2022 01:12
kmk3 added a commit that referenced this pull request Jun 8, 2022
@kmk3 kmk3 moved this from In progress to Done (on RELNOTES) in Release 0.9.70 Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release 0.9.70
  
Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

None yet

3 participants