| Message ID | pull.1930.v3.git.git.1742743771108.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] shell: allow overriding built-in commands | expand |
On Sun, Mar 23, 2025 at 03:29:30PM +0000, Ayman Bagabas via GitGitGadget wrote: > From: Ayman Bagabas <ayman.bagabas@gmail.com> > > This patch allows overriding the shell built-in commands by placing a > script with the same name under git-shell-commands directory. > > This is useful for users who want to extend the shell built-in commands > without replacing the original command binary. For instance, a user > wanting to allow only a subset of users to run the git-receive-pack can > override the command with a script that checks the user and calls the > original command if the user is allowed. OK. We do not allow users to override normal Git commands with aliases, etc. But in the case of git-shell, those names are really a well-known API that a client is using, and this is the only opportunity an admin has to plug in between the client request and Git just running the command. So it seems like a reasonable goal. A more restricted approach might be to provide a more formal hook/plugin interface. E.g., to run a hook script with the command name and arguments, and have it return success/failure to allow the to proceed. That's not quite as flexible (in your approach I could replace what upload-pack is doing entirely, cache its output, and so on). But it might be harder for admins to screw up. I dunno. Let's look at the patch... > diff --git a/shell.c b/shell.c > index 76333c80686..8c7f4388bd5 100644 > --- a/shell.c > +++ b/shell.c > @@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv) > /* Accept "git foo" as if the caller said "git-foo". */ > prog[3] = '-'; > > + cd_to_homedir(); > for (cmd = cmd_list ; cmd->name ; cmd++) { Hmm, so we have moved the cd_to_homedir() call up, which used to happen after this loop. This means that when running a builtin command found in the loop, our working directory will potentially be different now than it was before your patch. That seems like an unintended side effect. Though I admit I am not sure why git-shell would be running in anything but the user's homedir in the first place. > + char *full_cmd; > if (strncmp(cmd->name, prog, len)) > continue; > arg = NULL; > @@ -210,10 +212,15 @@ int cmd_main(int argc, const char **argv) > default: > continue; > } > + /* Allow overriding built-in commands */ > + full_cmd = make_cmd(cmd->name); > + if (!access(full_cmd, X_OK)) { > + const char *argv[3] = { cmd->name, arg, NULL }; > + return execv(full_cmd, (char *const *) argv); > + } > return cmd->exec(cmd->name, arg); This leaks full_cmd if the exec call fails, I'd think? > + const char *argv[3] = { cmd->name, arg, NULL }; > + return execv(full_cmd, (char *const *) argv); So we just stuff "arg" into the argv we pass to the script. But isn't it supposed to be a shell command, that could have quoted arguments? For user-defined commands, we call split_cmdline() to get the real array, and pass it to the sub-program. For the built-in commands, we seem to cheat a little and just assume it is a single string, which we pick apart with sq_dequote(). But either way what your patch is doing seems wrong. Your custom git-upload-pack (or whatever) script will get passed the quoted value, and have to unquote itself. I guess if that were documented it _could_ be the right thing, but it seems rather unfriendly and unlike how the other user-defined commands work (and of course it's not actually documented). You also miss out on the option-injection protections from 3ec804490a (shell: disallow repo names beginning with dash, 2017-04-29). We skip those for user-defined commands, but I think you'd probably want them for something meant to be a wrapper around the built-in command. Likewise the setup_path() magic done by do_generic_cmd(). So it seems like rather than running execv() ourselves here, this should probably do one of: a. Break out of the loop, skipping the built-in command, so that we can run it as a regular user-defined command. b. Hook into do_generic_cmd() instead, after we've done our de-quoting and checked for option injection. Of the two, I think (b) is probably the least surprising in terms of what the wrapper script has to do. If this were just a hook that asked "can we run this command", then none of this would matter. Running it would be a separate step. -Peff
Jeff King <peff@peff.net> writes: > So it seems like a reasonable goal. A more restricted approach might be > to provide a more formal hook/plugin interface. E.g., to run a hook > script with the command name and arguments, and have it return > success/failure to allow the to proceed. > > That's not quite as flexible (in your approach I could replace what > upload-pack is doing entirely, cache its output, and so on). But it > might be harder for admins to screw up. I dunno. Yeah, we usually try not to be overly flexible for that reason, but given that "git shell" is so limited that rewriting its services wholesale is not all that much of a deal, I think it is OK. I however wonder if it is worth admins' time and effort to add features to "git-shell" using this new facility, or if they are better off using something more established like gitolite once they want to go fancier beyond what the basic "git-shell" offers.
On Sun, Mar 23, 2025 at 10:27:32PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > So it seems like a reasonable goal. A more restricted approach might be > > to provide a more formal hook/plugin interface. E.g., to run a hook > > script with the command name and arguments, and have it return > > success/failure to allow the to proceed. > > > > That's not quite as flexible (in your approach I could replace what > > upload-pack is doing entirely, cache its output, and so on). But it > > might be harder for admins to screw up. I dunno. > > Yeah, we usually try not to be overly flexible for that reason, but > given that "git shell" is so limited that rewriting its services > wholesale is not all that much of a deal, I think it is OK. > > I however wonder if it is worth admins' time and effort to add > features to "git-shell" using this new facility, or if they are > better off using something more established like gitolite once they > want to go fancier beyond what the basic "git-shell" offers. Yeah, I left my general opinions on git-shell unspoken. ;) For features, I think you are probably better off with something like gitolite (which I think does have some access control). For security, I'd be a little scared of git-shell, just because it's not run all that frequently and we've had issues with it before (e.g., integer overflows). If you're taking requests from untrusted clients, you're probably better off configuring http service. I also imagine there may be restricted shell implementations that are more general and more battle-hardened, that you could configure to only run a few commands. But I haven't looked at that space (because again, I'd suggest just git-over-http). -Peff
> On Mar 24, 2025, at 11:28 PM, Jeff King <peff@peff.net> wrote: > > On Sun, Mar 23, 2025 at 10:27:32PM -0700, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >>> So it seems like a reasonable goal. A more restricted approach might be >>> to provide a more formal hook/plugin interface. E.g., to run a hook >>> script with the command name and arguments, and have it return >>> success/failure to allow the to proceed. >>> >>> That's not quite as flexible (in your approach I could replace what >>> upload-pack is doing entirely, cache its output, and so on). But it >>> might be harder for admins to screw up. I dunno. >> >> Yeah, we usually try not to be overly flexible for that reason, but >> given that "git shell" is so limited that rewriting its services >> wholesale is not all that much of a deal, I think it is OK. >> >> I however wonder if it is worth admins' time and effort to add >> features to "git-shell" using this new facility, or if they are >> better off using something more established like gitolite once they >> want to go fancier beyond what the basic "git-shell" offers. > > Yeah, I left my general opinions on git-shell unspoken. ;) > > For features, I think you are probably better off with something like > gitolite (which I think does have some access control). Gitolite is a great software, but it also has its limitations. It couples authentication and authorization in the same system. However, I'm looking for something more flexible that I can plug whatever authentication or authorization system to the mix similar to git-http-backend paired with apache/nginx/h2o/etc. > > For security, I'd be a little scared of git-shell, just because it's not > run all that frequently and we've had issues with it before (e.g., > integer overflows). If you're taking requests from untrusted clients, > you're probably better off configuring http service. That's a fair point. Perhaps writing my own restricted shell might be the best solution for what I'm looking for :/ > > I also imagine there may be restricted shell implementations that are > more general and more battle-hardened, that you could configure to only > run a few commands. But I haven't looked at that space (because again, > I'd suggest just git-over-http). If you know any general restricted shell implementations please do tell. I'm looking for an SSH solution something pluggable like git-http-backend that I can build on top of. Honestly, git-shell's simplicity is what got my interest at first. The fact that it's not secure and not run frequently can change and be improved. - Ayman
diff --git a/shell.c b/shell.c index 76333c80686..8c7f4388bd5 100644 --- a/shell.c +++ b/shell.c @@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv) /* Accept "git foo" as if the caller said "git-foo". */ prog[3] = '-'; + cd_to_homedir(); for (cmd = cmd_list ; cmd->name ; cmd++) { int len = strlen(cmd->name); char *arg; + char *full_cmd; if (strncmp(cmd->name, prog, len)) continue; arg = NULL; @@ -210,10 +212,15 @@ int cmd_main(int argc, const char **argv) default: continue; } + /* Allow overriding built-in commands */ + full_cmd = make_cmd(cmd->name); + if (!access(full_cmd, X_OK)) { + const char *argv[3] = { cmd->name, arg, NULL }; + return execv(full_cmd, (char *const *) argv); + } return cmd->exec(cmd->name, arg); } - cd_to_homedir(); count = split_cmdline(prog, &user_argv); if (count >= 0) { if (is_valid_cmd_name(user_argv[0])) {