Can dependency on /bin/sh be removed?

>   [credential-helper]
>   helper = /bin/myhelper
>
> In that case, the shell is only being used to tokenize 'myhelper get'.
>
> Is there a solution that I'm missing here?

> Having said that, I do not think you can avoid /bin/sh if your goal
> is "minimal image *to run git*", as there are many things we run,
> starting from the editor and the pager and end-user hooks.  The
> credential helper is probably the least of your problems.  What's a
> minimum /bin/dash image cost these days?

> Even though the code path starting from start_command() is what
> run_credential_helper() does use, what is run is NOT a simple
> command "/bin/myhelper".  It will receive arguments, like
> 
> 	/bin/myhelper erase
> 	/bin/myhelper get
> 	/bin/myhelper store
> 
> etc., because the caller appends these operation verbs to the value
> of the configuration variable.  And as you found out, to tokenize them
> into two, we need shell.

> We may be able to teach credential.c:credential_do() not to paste
> the operation verb to the command line so early.  Instead you could
> teach the function to send the command line and operation verb
> separately down to run_credential_helper() though.  That way, we
> might be able to avoid the shell in this particular case.  That is,
> if we can 
> 
>  * Have start_command() -> prepare_cmd() -> prepare_shell_cmd()
>    codepath to take the usual route _without_ the operation verb
>    tucked to the command line, we would get cmd->args.v[] that does
>    not rely on the shell;
> 
>  * Then before the prepared command is executed, if we can somehow
>    _append_ to cmd->args.v[] the operation verb (after all, that
>    wants to become the argv[1] to the spawned command) before
>    start_command() exec's it
> 
> then we are done.

> Having said that, I do not think you can avoid /bin/sh if your goal
> is "minimal image *to run git*", as there are many things we run,
> starting from the editor and the pager and end-user hooks.  The
> credential helper is probably the least of your problems.  What's a
> minimum /bin/dash image cost these days?

> Yes, I think this is reasonable. You'd also perhaps want to have it set
> child->git_cmd as appropriate (though really, I do not think that does
> anything except stick "git" into child.args[0], so we could just do that
> ourselves).
>
> I'm actually a little surprised it was not written this way in the first
> place.

> > Having said that, I do not think you can avoid /bin/sh if your goal
> > is "minimal image *to run git*", as there are many things we run,
> > starting from the editor and the pager and end-user hooks.  The
> > credential helper is probably the least of your problems.  What's a
> > minimum /bin/dash image cost these days?

>
> On Mon, Jul 15, 2024 at 01:18:52PM -0700, Junio C Hamano wrote:
>
> > Even though the code path starting from start_command() is what
> > run_credential_helper() does use, what is run is NOT a simple
> > command "/bin/myhelper".  It will receive arguments, like
> >
> >       /bin/myhelper erase
> >       /bin/myhelper get
> >       /bin/myhelper store
> >
> > etc., because the caller appends these operation verbs to the value
> > of the configuration variable.  And as you found out, to tokenize them
> > into two, we need shell.
>
> It is also usually "git myhelper get", and so on (though you can
> configure a shell command).
>
> > We may be able to teach credential.c:credential_do() not to paste
> > the operation verb to the command line so early.  Instead you could
> > teach the function to send the command line and operation verb
> > separately down to run_credential_helper() though.  That way, we
> > might be able to avoid the shell in this particular case.  That is,
> > if we can
> >
> >  * Have start_command() -> prepare_cmd() -> prepare_shell_cmd()
> >    codepath to take the usual route _without_ the operation verb
> >    tucked to the command line, we would get cmd->args.v[] that does
> >    not rely on the shell;
> >
> >  * Then before the prepared command is executed, if we can somehow
> >    _append_ to cmd->args.v[] the operation verb (after all, that
> >    wants to become the argv[1] to the spawned command) before
> >    start_command() exec's it
> >
> > then we are done.
>
> Yes, I think this is reasonable. You'd also perhaps want to have it set
> child->git_cmd as appropriate (though really, I do not think that does
> anything except stick "git" into child.args[0], so we could just do that
> ourselves).
>
> I'm actually a little surprised it was not written this way in the first
> place. In the non-!, non-absolute-path case we are pasting together a
> string that will be passed to the shell, and it includes the "helper"
> argument without further quoting. I don't think you could smuggle a
> semicolon into there (due to our protocol restrictions), but it does
> seem like a possible shell injection route.
>
> I think it probably goes all the way back to my abca927dbe (introduce
> credentials API, 2011-12-10).
>
> > Having said that, I do not think you can avoid /bin/sh if your goal
> > is "minimal image *to run git*", as there are many things we run,
> > starting from the editor and the pager and end-user hooks.  The
> > credential helper is probably the least of your problems.  What's a
> > minimum /bin/dash image cost these days?
>
> Right. The bigger issue to me is that the credential helper "!" syntax
> is defined to the end user as running a shell (and ditto for all of
> those other spots). So any platform where we can't run a shell would not
> be fully compatible with git on other platforms.
>
> That may be an OK limitation as long as it is advertised clearly, but
> the use of a shell here is not just an implementation detail, but an
> intentional user-facing design.
>
> -Peff

> > We may be able to teach credential.c:credential_do() not to paste
> > the operation verb to the command line so early.  Instead you could
> > teach the function to send the command line and operation verb
> > separately down to run_credential_helper() though.  That way, we
> > might be able to avoid the shell in this particular case.  That is,
> > if we can 
> > 
> >  * Have start_command() -> prepare_cmd() -> prepare_shell_cmd()
> >    codepath to take the usual route _without_ the operation verb
> >    tucked to the command line, we would get cmd->args.v[] that does
> >    not rely on the shell;
> > 
> >  * Then before the prepared command is executed, if we can somehow
> >    _append_ to cmd->args.v[] the operation verb (after all, that
> >    wants to become the argv[1] to the spawned command) before
> >    start_command() exec's it
> > 
> > then we are done.
> 
> Yes, I think this is reasonable. You'd also perhaps want to have it set
> child->git_cmd as appropriate (though really, I do not think that does
> anything except stick "git" into child.args[0], so we could just do that
> ourselves).
> 
> I'm actually a little surprised it was not written this way in the first
> place. In the non-!, non-absolute-path case we are pasting together a
> string that will be passed to the shell, and it includes the "helper"
> argument without further quoting. I don't think you could smuggle a
> semicolon into there (due to our protocol restrictions), but it does
> seem like a possible shell injection route.
> 
> I think it probably goes all the way back to my abca927dbe (introduce
> credentials API, 2011-12-10).

>   [credential]
>   helper = cache --socket=/path/to/socket --timeout=123
>
> Arguably we could have gotten away with word-splitting ourselves,
> sticking the result in child_process.args, and avoided the shell. But
> the use of the shell is documented in gitcredentials(7):
>
>   helper
>     The name of an external credential helper, and any associated
>     options. If the helper name is not an absolute path, then the string
>     git credential- is prepended. The resulting string is executed by
>     the shell (so, for example, setting this to foo --option=bar will
>     execute git credential-foo --option=bar via the shell. See the
>     manual of specific helpers for examples of their use.
>
> So users may be depending on that to do "--socket=$HOME/.foo", or even
> more exotic shell constructs.
>
> Again, it's possible that we could detect that no shell metacharacters
> are in play and do the word-splitting ourselves. But at that point I
> think it should go into run-command's prepare_shell_cmd(). That is, I I
> think it could take space out of the list of metachars that force us to
> invoke the shell, and do the word-splitting there. But not having
> thought very hard about it, there are probably corner cases where that
> optimization is detectable by the user (presumably unusual IFS, but
> maybe more?).

> > Again, it's possible that we could detect that no shell metacharacters
> > are in play and do the word-splitting ourselves. But at that point I
> > think it should go into run-command's prepare_shell_cmd(). That is, I I
> > think it could take space out of the list of metachars that force us to
> > invoke the shell, and do the word-splitting there. But not having
> > thought very hard about it, there are probably corner cases where that
> > optimization is detectable by the user (presumably unusual IFS, but
> > maybe more?).
> 
> Well, I strongly object to an approach for us to "parse" anything.

> But even then it would be sensible to formulate:
> 
> 	argv[0] = sh
> 	argv[1] = -c
> 	argv[2] = git-credential-cache --socket=/path/	--timeout=123 "$@"
> 	argv[3] = -
> 	argv[4] = NULL
> 
> and if there is an argument say "get", extend it to
> 
> 	argv[0] = sh
> 	argv[1] = -c
> 	argv[2] = git-credential-cache --socket=/path/	--timeout=123 "$@"
> 	argv[3] = -
> 	argv[4] = get
> 	argv[5] = NULL
> 
> before passing the array to execv(), no?

> And with the metacharacter optimization to drop .use_shell we
> already have, a single-token /bin/myhelper case would then become
> 
> 	argv[0] = /bin/myhelper
> 	argv[1] = get
> 	argv[2] = NULL
> 
> naturally.

> On Jul 16 2024, Jeff King wrote:
> 
> > Again, it's possible that we could detect that no shell
> > metacharacters are in play and do the word-splitting ourselves. But
> > at that point I think it should go into run-command's
> > prepare_shell_cmd().
> 
> This is what GNU make does (see construct_command_argv_internal), for
> performance reason.  But run_command is probably not performance
> critical.

> On Jul 16 2024, Jeff King wrote:
> 
> > Again, it's possible that we could detect that no shell metacharacters
> > are in play and do the word-splitting ourselves. But at that point I
> > think it should go into run-command's prepare_shell_cmd().
> 
> This is what GNU make does (see construct_command_argv_internal), for
> performance reason.  But run_command is probably not performance
> critical.