-
Notifications
You must be signed in to change notification settings - Fork 557
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
feature: add notpm command & keep tpm devices in private-dev #6390
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There has to be an follow-up PR that adds notpm to all profiles with private-dev.
@rusty-snake Added a commit that adds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
After this is committed, we should also update the Wiki: https://github.com/netblue30/firejail/wiki/Comparison-of-firejail-and-systemd's-hardening-options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Some ordering nitpicks...
Amended and force-pushed with alphabetical ordering. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorting.
There are some other missing changes:
See commit 760f50f ("landlock: move commands into profile and add |
0a57213
to
6d50555
Compare
It looks like force-pushing is making other updates obsolete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR already makes substantial changes by adding the new command; leave the
profile changes (other than the ones for default.profile and profile.template)
for after this PR to make reviewing easier.
In the parts of the code that are mostly alphabetically sorted (such as in the
man pages), it makes sense to put notpm
before notv
.
But in some parts of the code, for whatever reason the multimedia-related
options (nosound
, noautopulse
, no3d
, notv
, nodvd
) are sorted
together, so for consistency put notpḿ
before nou2f
, as was suggested in
the previous review.
That is:
notv
,nou2f
->notpm
,notv
,nou2f
nodvd
,nou2f
->nodvd
,notpm
,nou2f
Also, avoid sorting things in this PR (especially in the same commit).
After this PR I might try to make these parts more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misc
OK, reverted the long list of profile changes. I stored it in another branch to propose later.
OK, I see.
This sentence confuses me, because I interpreted the two previous paragraphs as a ask to actually sort these options in a certain way. Or do you want to merge this PR, and do the sorting in a different one? I'm happy either way.
|
4da75ef
to
1d80642
Compare
Sorry, I meant avoid sorting/moving existing lines while adding new lines at
In general it seems better to avoid refactoring in PRs that just add a new
I fixed the sorting, squashed the commits, edited the commit message and Let me know if there are any issues. |
An ssh private key may be stored in a Trusted Platform Module (TPM) device and `private-dev` in ssh.profile currently breaks this use-case, as it does not keep tpm devices (see netblue30#6379). So add a new `notpm` command and keep tpm devices in /dev by default with `private-dev` unless `notpm` is used.
This command is deprecated and may be confused for a hardening option. This amends commit 5a61202 ("rename noautopulse to keep-config-pulse", 2021-05-13) / PR netblue30#4278. This is a follow-up to netblue30#6390.
An ssh private key may be stored in a Trusted Platform Module (TPM)
device and
private-dev
in ssh.profile currently breaks this use-case,as it does not keep tpm devices (see #6379).
So add a new
notpm
command and keep tpm devices in /dev by defaultwith
private-dev
unlessnotpm
is used.Tested locally with:
Fixes #6379.