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

modif: drop deprecated 'shell' option references #5894

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

glitsj16
Copy link
Collaborator

@glitsj16 glitsj16 commented Jul 10, 2023

The shell option has been removed. Remove stale references.

This does NOT remove shell none-related code comments in:

Relates to #5196.

Suggested by #5891.

@kmk3 kmk3 force-pushed the shell-none-cleaning branch 2 times, most recently from 46a1dc8 to 8c86fae Compare July 11, 2023 05:06
@kmk3
Copy link
Collaborator

kmk3 commented Jul 11, 2023

I fixed the tests and removed a few more references, as I had a branch with
similar changes.

But note that it is still referenced in more places:

$ git grep -In 'shell.none' -- src test
src/firejail/cmdline.c:140:     // index == -1 could happen if we have --shell=none and no program was specified
src/firejail/cmdline.c:167:     // index == -1 could happen if we have --shell=none and no program was specified
src/firejail/fs_lib.c:434://    if (!arg_shell_none) {
src/firejail/join.c:416://      if (!arg_shell_none)
src/firejail/main.c:2967:               fprintf(stderr, "Error: command must be specified when --shell=none used.\n");
src/firejail/profile.c:374:     else if (strcmp(ptr, "shell none") == 0) {
src/firejail/profile.c:375:             fprintf(stderr, "Warning: \"shell none\" command in the profile file is done by default; the command will be deprecated\n");
src/firejail/sandbox.c:416:// we are here because of --shell=none
src/firejail/sandbox.c:551:                     fprintf(stderr, "Error: --shell=none configured, but no program specified\n");
test/environment/shell-none.profile:1:shell none
test/profiles/ignore.profile:3:shell none
test/profiles/ignore2.profile:5:shell none
test/profiles/ignore3.profile:4:shell none

I'm not sure about the source code and the tests should probably be changed to
use another command rather than removing the references, though I wouldn't
consider these as blockers for this PR.

@@ -160,8 +160,6 @@ _firejail_args=(
'*--seccomp.32.keep=-[enable seccomp filter, and whitelist the 32 bit syscalls specified by the command]: :'
# FIXME: Add errnos
'--seccomp-error-action=-[change error code, kill process or log the attempt]: :(kill log)'
'--shell=none[run the program directly without a user shell]'
'--shell=-[set default user shell]: :_values $(cat /etc/shells)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was shell removed too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was shell removed too?

Yes:

$ firejail --quiet --shell=/bin/sh true; echo $?
Warning: --shell feature has been deprecated
1
$ cat test.profile
shell none
shell /bin/sh
$ firejail --quiet --profile=./test.profile true
Warning: "shell none" command in the profile file is done by default; the command will be deprecated
Error: line 2 in ./test.profile is invalid

(The first warning is actually an error)

@kmk3 kmk3 force-pushed the shell-none-cleaning branch 2 times, most recently from 26b5aca to 7714a5b Compare July 14, 2023 07:35
@kmk3 kmk3 changed the title drop deprecated 'shell none' references modif: drop deprecated 'shell' option references Jul 14, 2023
@kmk3 kmk3 force-pushed the shell-none-cleaning branch 2 times, most recently from a2b4d62 to a6d6a39 Compare July 14, 2023 07:45
@kmk3
Copy link
Collaborator

kmk3 commented Jul 14, 2023

(Rebased to master to fix conflicts with #5898 and improved the error messages)

The `shell` option has been removed. Remove stale references.

This does NOT remove `shell none`-related code comments in:

- src/firejail/fs_lib.c (L433-L441)
- src/firejail/join.c (L415-L417)

Relates to netblue30#5196.

Suggested by netblue30#5891.
@glitsj16
Copy link
Collaborator Author

@kmk3 A big thank you for doing the additional work on this!

@glitsj16 glitsj16 merged commit 9863f98 into netblue30:master Jul 19, 2023
10 checks passed
@glitsj16 glitsj16 deleted the shell-none-cleaning branch July 19, 2023 12:54
kmk3 added a commit that referenced this pull request Jul 22, 2023
kmk3 added a commit that referenced this pull request Jul 23, 2023
This adds the `shell` command.  Note that it's still being parsed in
profile.c, even if it's just to return an error.

Commands used to remake them:

    rm contrib/syntax/lists/*
    make syntax

Relates to #5627 #5894.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants