Revert "Add Landlock support to Firejail" #5347
Conversation
|
By the way, please wait for this pull request before merging anything else, as |
|
Yes, lots of issues! I'll put a write-up shortly. We go with it as experimental for the next release, disabled by default at compile time. Meanwhile, you guys go in and fix whatever doesn't look right - I know both you and @rusty-snake had some problems with it. It is to early to get any consensus on any particular issue, so we just commit in whatever we think makes sense, and we reconcile them later. Probably we'll end up changing also the names of the commands - the code is not compiled by default anyway. |
|
@kmk3 what do you think about a landlock project (https://github.com/netblue30/firejail/projects)? |
- --enable-landlock Landlock self-restriction support
+ --enable-landlock Landlock self-restriction support [EXPERIMENTAL]We should make it clear as the MACs (AppArmor and SELinux) are opt-in too. |
|
@netblue30 Thank you for invitation. I'm sorry for my absence, I was busy with finding a job. I would also like to say, I doubt that merging the #5315 to master before it was ready (code style fixes, error message corrections, building on systems that don't support Landlock and other changes were planned) was the best idea.
How does it work on systems that don't support Landlock (means it's not in the kernel, and there is no userspace support for it)? I'd like to ask you to avoid merging pull requests with changes requested before proposing fixes, or at least contacting the PR author -- I was working to introduce requested changes, and was a bit surprised to find out that #5315 was already merged. I'd also like to ask to re-open the #5315 so I could introduce the proposed changes for review. |
|
@ChrysoliteAzalea: Welcome to the club! Don''t bother reopening the old PR, commit the suggestions or any other changes directly in git. It will be marked as experimental in the next release, so the user knows what to expect. At some point we will have to detect the kernel version at runtime and bring in the functionality. We've done this on several other features, it won't be a problem. |
We should use feature detection. |
How? We have at compile time a "#include <linux/landlock.h>" that will break if the file is not there. |
|
Did you mean a "runtime check" in |
|
@netblue30 commented on Aug 29:
Why merge it as is then? What is the hurry? This adds technical debt and creates needless rework for anyone that may Disregarding the fact that a pull request is actively being discussed and Additionally, even if fixed later, the merged code (and all of the issues in Also, note that fixing unmerged code is trivial (just amend/rebase and All of that said, this pull request was created so that the WIP code can be Now looking at the current master, I see that my comment about
There appeared to be enough consensus on #5315 for at least the raised issues
I was going to suggest on the pull request to only add the most basic support I had already posted many review comments and I never would have expected a
That is basically what was already being done on the pull request. The main difference is that there was a thread for each issue in the pull Why merge a pull request full of issues that weren't done being fixed and then
It would likely have been much easier to just checkout the source branch, make The problem is that doing so precludes collaboration. Firstly, I'm not necessarily 100% sure of every suggestion that I make and I And secondly, working with the author helps them (and we) better understand the Also, note that by engaging in the discussions, the author shares the burden of And indeed, considering @rusty-snake's approval and @ChrysoliteAzalea's See also #4517. |
|
Updated and force-pushed to fix conflict with commit 6e687c3 ("tracelog |
|
Updated and force-pushed to also revert changes from a couple of commits that |
|
Sorry guys, I have to reject this PR. @kmk3: What you say is correct, and that's basically what we've been doing so far. It works fine for released features where it is critical to keep an eye on the history. However, landlock has no history, it is just starting up. As new landlock features become available in the kernel, we will move a lot of the existing functionality to landlock. I am talking about our blacklist/whitelist implementation based on SUID and mount/bind. As soon as the kernel implements landlock blacklists, we phase out ours and move it to landlock. So, it is much better to put landlock in early, and fix all the integration problems we run into. I've done some tests on Fedora (kernel 5.19), landlock is working fine as implemented by @ChrysoliteAzalea. Probably we should drop the experimental flag and go live with it in Arch, Fedora and Gentoo - this way we get more testing from outside people. |
This reverts commit 2f3c19a. Part of reverting commits with Landlock-related changes.
This reverts commit c5a052f. Part of reverting commits with Landlock-related changes.
This reverts commit 2d885e5. Part of reverting commits with Landlock-related changes.
This reverts commit 0594c5d. Part of reverting commits with Landlock-related changes.
This reverts commit 796fa09. Part of reverting commits with Landlock-related changes.
… file" This reverts commit 6e687c3. Part of reverting commits with Landlock-related changes.
This reverts commit 836ffe3. Part of reverting commits with Landlock-related changes.
…ock" This reverts commit 54cb3e7, reversing changes made to 97b1e02. There were many issues and requests for changes raised in the pull request (both code-wise and design-wise) and most of them are still unresolved[1]. [1] netblue30#5315
Committer note: This is the same as commit 6e687c3 ("tracelog disabled by default in /etc/firejail/firejail.config file", 2022-08-29) but without the Landlock-related changes.
|
@netblue30 commented on Sep 4:
I'm not sure what you mean by this. To clarify, I previously mentioned The first main issue is with the development process. That is, with unilaterally merging an unfinished pull request that was actively The second main issue is with the result of the merge. To summarize the main reasons for doing the revert:
This pull request is about fixing the above problems (which AFAIK can only be Also, note that Landlock-related commits made after #5315 can later be I don't think that it would take a substantial amount of time overall for us to Any clarifications would be appreciated. |
kmk3
force-pushed
the
revert-landlock
branch
from
8bdb7ac
to
d900d53
Compare
|
(Rebased to master to fix conflicts) |
|
OK, let's do it as you suggested, You run the show, git revert/rebase/forced push - whatever you think is necessary. We need a place to keep the discussion going, maybe the issue #5315 open by @rusty-snake? |
|
@netblue30 commented on Sep 5:
Alright, thanks!
I assume you meant #5354, in which case sure. I'll merge this, then put together a new @ChrysoliteAzalea after that you can change it if needed, squash it, open a new |
Commands used to find and print the relevant commits:
$ TZ=UTC0 git log --date='format-local:%Y-%m-%d' \
--pretty='%H # %cd | %s' 60db9f7~1..60db9f7 |
grep 'Revert "';
git log --reverse --pretty=%b 60db9f7~1..60db9f7 |
sed -E -n 's/^This reverts commit ([0-9a-z]+).*/\1/p' |
TZ=UTC0 xargs git show --date='format-local:%Y-%m-%d' \
--pretty='%H # %cd | %s' -s | grep -v 'Merge pull request';
TZ=UTC0 git log --no-merges --date='format-local:%Y-%m-%d' \
--pretty='%H # %cd | %s' 54cb3e7~1..54cb3e7
Explanation: The first `git log` basically takes the revision range from
one commit before a given merge commit until the merge commit itself and
prints the commits in that range (which should usually mean all commits
that were in the branch that was merged). In this case, it's the
commits that do the revert.
The second `git log` finds the hash of all commits that were reverted
and prints them.
The `grep -v` and third `git log` are only needed because the merge
commit of the original branch was reverted directly (on commit
97874c3), rather than reverting each individual commit on that branch.
So these commands are used to print all of the commits in the original
branch.
Relates to netblue30#5315 netblue30#5347.
This reverts commit 836ffe3.
This reverts commit 54cb3e7, reversing
changes made to 97b1e02.
There were many issues and requests for changes raised in the pull
request (both code-wise and design-wise) and most of them are still
unresolved[1].
[1] #5315
Cc: @ChrysoliteAzalea