Issues with multiple arguments passed to setup-sshd

[GunArm]

Since PR #63 is merged, creating new issue to continue discussion for changes proposed by @afischer211 at the end of the discussion on that PR:

Hello, the reason for my investigation is, that the last line of the entrypoint script is not executed. I use the docker-plugin inside jenkins for starting up slave-containers (connected by ssh). Because I want to receive the extended output of ssh with the option -e, I hope on the new version of this script by one of the last pullrequests.
But I must detect, the entrypoint script does not match for the given params of the docker-plugin. They are:
"/usr/sbin/sshd -D -p 22 -o AuthorizedKeysCommand=**** -o AuthorizedKeysCommandUser=****"
So my enhancement with regexp

"elif [[ "$@" =~ /usr/sbin/sshd\ -D\ -p\ 22.* ]]; then"

matches and works like expected (shifting out all arguments until the first -o...), the original version does not match and execute the command in the else-branch (without the -e option).

From reviewing the code it does seem that a minor tweak could improve it. The intention of shifting default sshd arguments in #63 is so that additional arguments would be passed to the SSHD command, but I see that the literal string comparison renders that useless as it will never trap the case that there actually are additional arguments.

So I see that a regex match with a wildcard on the end ought to allow the default sshd command to be trapped and stripped off by shifting, while preserving the remaining arguments to be passed to the final sshd line.

However I am concerned about the next step
exec /usr/sbin/sshd -D -e "${@}"

I am not sure of the intention of quoting the "${@}" on the last line. It comes from commit 49854a6 in #12. The commit message mentions "bash best practices" by which maybe it is referring to the general recommendation in bash to quote all your variables. However there are cases where you explicitly don't want to do that, and this strikes me as one of them. @dduportal any thoughts?

Example:

args="-f2 -d-"
echo a-b-c | cut $args
# returns 'b'
echo a-b-c | cut "$args"
# gives an error

This would seem to be a problem, not just for arguments passed after /usr/sbin/sshd -D -p 22 (in relation to PR #63) but also any multiple of arguments passed even without hitting the pubkey or /usr/shbin/sshd traps.

@afischer I do not have a convenient setup for local testing of passing some extra sshd args like this and verifying that they work, but are you saying you have implemented this change locally and seen that in the last line sshd ok with your -o AuthorizedKeysCommand=**** -o AuthorizedKeysCommandUser=**** being passed as a single quoted token? If it is working for you that would hint to me that sshd does unusual special subparsing of arguments it receives as a single token.

[timja]

I am not sure of the intention of quoting the "${@}" on the last line. It comes from commit 49854a6 in #12. The commit message mentions "bash best practices"

It will be referencing https://www.shellcheck.net/, it's normally recommended to quote variables because of how whitespace is handled in bash, but you can disable a rule by putting a comment on the line above.

PR welcomed if you need this

[dduportal]

Hello! Thanks for reporting and caring!

Sorry that I did miss your mention here and this issue was rotting.

The reason of the quoting around the $@ is explained by the shellcheck rule that is triggered when NOT quoted: https://github.com/koalaman/shellcheck/wiki/SC2068.

It avoids any globbing passed in the argument list to be interpretated (and it also avoid splitting argument if they contains a space, even between quotes).

I realize that I made a mistake back in 2017 though (and I bet that 2021-shellcheck would underline it): we should have elif [[ "${*}" == "/usr/sbin/sshd -D -p 22" ]]; instead, because the $@ array form inside a test double brackets will expand, so the match would be checked only for the first element of the array, while using $* forces the array to be concatenated as a string.

There is no doubt that there might be issues in some cases as underlined in #62 : I propose that we first find a reproduction case to be sure that we can implement a test for these cases, and from there we might want to update the script. Sounds good for you @GunArm @afischer ?

[dduportal]

I realize that I made a mistake back in 2017 though (and I bet that 2021-shellcheck would underline it): we should have elif [[ "${*}" == "/usr/sbin/sshd -D -p 22" ]]; instead, because the $@ array form inside a test double brackets will expand, so the match would be checked only for the first element of the array, while using $* forces the array to be concatenated as a string.

Here is the shellcheck rule that did not exist back in 2017 (when #12 was merged): https://github.com/koalaman/shellcheck/wiki/SC2199