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

Revert "move whitelist/blacklist to allow/deny" #4410

Merged
merged 2 commits into from
Jul 25, 2021

Conversation

kmk3
Copy link
Collaborator

@kmk3 kmk3 commented Jul 18, 2021

This reverts commit fe0f975.

Note: This only reverts the changes from etc.

The 4 aliases introduced on commit 45f2ba5 are mere, well, aliases.
That is, they fail to address the different usability problems discussed
on #3447 and in fact only make things more confusing (as has
already been mentioned on this and later comments). The main
reason is that the aliases do not meaningfully map to the original
commands. For example, the commands from each pair below seem like they
would do the exact same thing:

  • allow and nodeny
  • deny and noallow

Additionally, if these aliases are not the final commands, but only a
test/work-in-progress, then keeping the wide-scale search/replace
changes made on commit fe0f975 would only serve to cause confusion, as
users of firejail-git, contributors and downstream projects might start
changing the commands used on their profiles, only to later have to
change them again, potentially to completely different commands.

The sooner this is undone the better, as (besides the above reasons) the
more profile changes there are between the original commit and the
revert, the harder it is to e.g.: git diff versions of files across
the following revision ranges: before the commit, after the commit but
before the revert and after the revert. Note: This is still the case
even if a commit is ignored by git blame.

So let us revert fe0f975 and only reapply similar large-scale changes
once we have discussed and settled on better commands.

How the revert was applied: Despite using the auto-generated message
from git revert, to ensure correctness and to avoid conflicts the
changes were reverted in different steps: Firstly, revert the files
which can be safely reverted directly ("filestorevert"):

# Find out which files have been changed on fe0f975f44, but have not
# been changed afterwards and list them on "filestorevert"
git show --pretty='' --name-only fe0f975f44 -- etc | LC_ALL=C sort >allfiles
git diff --name-only fe0f975f44..master -- etc | LC_ALL=C sort >filestoignore
comm -2 -3 allfiles filestoignore >filestorevert

# Note: There are 3 extra files on filestoignore because they were
# added after commit fe0f975f44
wc -l allfiles filestoignore filestorevert | head -n 3
#   797 allfiles
#     8 filestoignore
#   792 filestorevert

# Automatically revert files in "filestorevert"
# See https://stackoverflow.com/a/23401018/10095231
tr '\n' '\000' <filestorevert | xargs -0 git show fe0f975f44 -- |
git apply --reverse

printf 'Total files reverted:\n'
git diff --name-only | wc -l
# 792

Secondly, do some search/replace on the rest:

tr '\n' '\000' <filestoignore | xargs -0 sed -i.bak \
  -e 's/allow  /whitelist /' -e 's/noallow  /nowhitelist /' \
  -e 's/deny  /blacklist /' -e 's/nodeny  /noblacklist /' \
  -e 's/deny-nolog  /blacklist-nolog /'

find etc -name '*.bak' -print0 | xargs -0 rm

Thirdly, verify the result. The following command shows the difference
between all the changes in etc from before fe0f975 and this commit
(inclusive):

git diff fe0f975f44~1 -- etc

From the output, it looks like all alias changes are fully reverted and
that the other changes to etc (from after fe0f975) remain, so the
revert seems to be done correctly.

This reverts commit fe0f975.

Note: This only reverts the changes from etc.

The 4 aliases introduced on commit 45f2ba5 are mere, well, aliases.
That is, they fail to address the different usability problems discussed
on [netblue30#3447][3447] and in fact only make things more confusing (as has
already been mentioned on [this][4379] and later comments).  The main
reason is that the aliases do not meaningfully map to the original
commands.  For example, the commands from each pair below seem like they
would do the exact same thing:

* `allow` and `nodeny`
* `deny` and `noallow`

Additionally, if these aliases are not the final commands, but only a
test/work-in-progress, then keeping the wide-scale search/replace
changes made on commit fe0f975 would only serve to cause confusion, as
users of firejail-git, contributors and downstream projects might start
changing the commands used on their profiles, only to later have to
change them again, potentially to completely different commands.

The sooner this is undone the better, as (besides the above reasons) the
more profile changes there are between the original commit and the
revert, the harder it is to e.g.: `git diff` versions of files across
the following revision ranges: before the commit, after the commit but
before the revert and after the revert.  Note: This is still the case
even if a commit is [ignored by `git blame`][4390].

So let us revert fe0f975 and only reapply similar large-scale changes
once we have discussed and settled on better commands.

How the revert was applied: Despite using the auto-generated message
from `git revert`, to ensure correctness and to avoid conflicts the
changes were reverted in different steps: Firstly, revert the files
which can be safely reverted directly ("filestorevert"):

    # Find out which files have been changed on fe0f975, but have not
    # been changed afterwards and list them on "filestorevert"
    git show --pretty='' --name-only fe0f975 -- etc | LC_ALL=C sort >allfiles
    git diff --name-only fe0f975..master -- etc | LC_ALL=C sort >filestoignore
    comm -2 -3 allfiles filestoignore >filestorevert

    # Note: There are 3 extra files on filestoignore because they were
    # added after commit fe0f975
    wc -l allfiles filestoignore filestorevert | head -n 3
    #   797 allfiles
    #     8 filestoignore
    #   792 filestorevert

    # Automatically revert files in "filestorevert"
    # See https://stackoverflow.com/a/23401018/10095231
    tr '\n' '\000' <filestorevert | xargs -0 git show fe0f975 -- |
    git apply --reverse

    printf 'Total files reverted:\n'
    git diff --name-only | wc -l
    # 792

Secondly, do some search/replace on the rest:

    tr '\n' '\000' <filestoignore | xargs -0 sed -i.bak \
      -e 's/allow  /whitelist /' -e 's/noallow  /nowhitelist /' \
      -e 's/deny  /blacklist /' -e 's/nodeny  /noblacklist /' \
      -e 's/deny-nolog  /blacklist-nolog /'

    find etc -name '*.bak' -print0 | xargs -0 rm

Thirdly, verify the result.  The following command shows the difference
between all the changes in etc from before fe0f975 and this commit
(inclusive):

    git diff fe0f975~1 -- etc

From the output, it looks like all alias changes are fully reverted and
that the other changes to etc (from after fe0f975) remain, so the
revert seems to be done correctly.

[3447]: netblue30#3447
[4379]: netblue30#4379 (comment)
[4390]: netblue30#4390
@kmk3 kmk3 requested a review from netblue30 July 18, 2021 23:51
@kmk3
Copy link
Collaborator Author

kmk3 commented Jul 19, 2021

Thirdly, verify the result. The following command shows the difference
between all the changes in etc from before fe0f975 and this commit
(inclusive):

git diff fe0f975f44~1 -- etc

GitHub version:

https://github.com/netblue30/firejail/compare/fe0f975f44~1..f43382f1e9707b4fd5e63c7bfe881912aa4ee994

Note: There is no way to diff by path on GitHub, so the above also shows
changes in unrelated dirs.

Note2: I plan to add the revert commit to .git-blame-ignore-revs, like in
#4390, but I'd rather do it after this PR (i.e.: on a follow-up PR), to
be sure of the final commit hash.

Note3: While another large-scale search/replace may very well be done on most
profiles, it is also possible that it would be done individually or in batches
instead, for two reasons:

  1. In order to fix the "whitelisting requires 3 commands" annoyance, a simple
    search/replace won't do; whitelisting profiles would require at least some
    other additional changes (like deleting the lines involving the other 2
    commands), which is not as simple to get right by scripting (without
    mangling the profiles, that is).

  2. I have another WIP proposal from months ago (which will likely be posted a
    while after the first one, on a separate issue), regarding the ordering of
    some whitelist-related paths on profile.template (to fix some
    inconsistencies which have come up during some PR reviews), which would
    require splitting and sorting the relevant lines. If this ends up being
    agreed upon and if most profiles are going to be changed anyway, it could be
    helpful to apply these changes at the same time, in order to avoid rework
    and to avoid more noise in the history. Note: This sorting issue is
    probably less widely known and not as crucial as the first reason, but I
    thought I'd leave it here for completeness.

@netblue30 netblue30 merged commit 8b50039 into netblue30:master Jul 25, 2021
@netblue30
Copy link
Owner

merged, sorry for delay!

@kmk3
Copy link
Collaborator Author

kmk3 commented Jul 26, 2021

merged, sorry for delay!

That's alright; I'm just glad it's finally done now.

@kmk3 kmk3 deleted the revert-allow-deny-etc branch July 26, 2021 19:11
Fred-Barclay added a commit that referenced this pull request Jul 28, 2021
kmk3 added a commit to kmk3/firejail that referenced this pull request Aug 27, 2021
Add commit f43382f ("Revert "move whitelist/blacklist to allow/deny"")
from PR netblue30#4410.

As mentioned on commit b023b9a ("Exclude allow/deny move in profile
from git blame") / PR netblue30#4390, commit fe0f975 ("move whitelist/blacklist
to allow/deny") "is just a huge rename", and so is the revert of it.

Note that there is a follow-up to f43382f: commit 2e4d52e ("Revert
allow/deny additional files") (sort of related to netblue30#4421).  It renames a
bit too much, which is later fixed by commit 3836131 ("Fix zim and
rednotebook").  Since these are small changes and since they involve
regressions, neither commit is added.
kmk3 added a commit to kmk3/firejail that referenced this pull request Sep 2, 2021
This reverts commit a11707e.

The man pages currently direct users to use the aliases instead of the
commands, which some users of firejail-git may end up doing.  Example:

netblue30#4496

So revert the man page changes as well to avoid confusion.

Note: This is not a full revert.  The commit in question also contains
some string formatting fixes on src/firejail/usage.c (related to dbus
and netmask), which are left intact.

Relates to netblue30#4410.
kmk3 added a commit to kmk3/firejail that referenced this pull request Oct 5, 2021
This reverts commit 4438f14.

Also, partially revert related commit e4307b4 ("fix whitelist/allow in
make test-utils") to keep tests working.

The profiles are being generated using aliases, which are not documented
nor used on the profiles in the repository.  So generate them using the
normal commands for consistency.  See also commit dd13595 ("Revert
"allow/deny help and man pages"") / PR netblue30#4502.

Relates to netblue30#4410.

Misc: I noticed this on issue netblue30#4592.
kmk3 added a commit to kmk3/firejail that referenced this pull request Oct 6, 2021
This reverts commit 4438f14.

Also, partially revert related commit e4307b4 ("fix whitelist/allow in
make test-utils") to keep the tests working.

The profiles are being generated using aliases, which are not used on
the profiles in the repository.  So generate them using the normal
commands for consistency.  See also commit dd13595 ("Revert
"allow/deny help and man pages"") / PR netblue30#4502.

Relates to netblue30#4410.

Misc: I noticed this on issue netblue30#4592.
kmk3 added a commit to kmk3/firejail that referenced this pull request Dec 10, 2021
kmk3 added a commit to kmk3/firejail that referenced this pull request Dec 10, 2021
…lacklist/noblacklist"

This reverts commit 45f2ba5.

Note: This is not a clean revert.

Note2: This also reverts the changes to src/firejail/profile.c from
commit fe0f975 ("move whitelist/blacklist to allow/deny", 2021-07-05).

Relates to netblue30#4410.
kmk3 added a commit to kmk3/firejail that referenced this pull request Dec 10, 2021
As of this commit, these are not of much use.  Though later if a generic
profile search/replace tool with built-in rules is to be added, the
tools in question could be used as a starting point.

src/tools/profcleaner.c was added on commit fe0f975 ("move
whitelist/blacklist to allow/deny", 2021-07-05).

src/tools/profcleaner.sh was added on commit ed02ab5 ("Create
profcleaner.sh", 2021-07-07) / PR netblue30#4389.

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

Successfully merging this pull request may close these issues.

2 participants