Git prompt

Git prompt

[Ethan Reesor]

I have a git user set up on my server. It's prompt is set to
git-prompt and it's git-shell-commands is empty. The server works as a
git remote using ssh and the git user. When I `ssh git@server` I get a
prompt where I can do nothing. When you ssh to github.com, you recieve
this message: "Hi username! You've successfully authenticated, but
GitHub does not provide shell access."

How do I make the git user work like github where, upon attempting to
get a prompt, the connection is closed?

--
Ethan Reesor

Re: Git prompt

[Jonathan Nieder]

Ethan Reesor wrote:

> I have a git user set up on my server. It's prompt is set to
> git-prompt and it's git-shell-commands is empty.
[...]
> How do I make the git user work like github where, upon attempting to
> get a prompt, the connection is closed?

I assume you mean that the user's login shell is git-shell.

You can disable interactive logins by removing the
~/git-shell-commands/ directory.  Unfortunately that doesn't let you
customize the message.  Perhaps it would make sense to teach shell.c
to look for a

	[shell]
		greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.'

setting in git's config file.  What do you think?

Thanks,
Jonathan

Re: Git prompt

[Ethan Reesor]

That would be perfect. (And I did mean I set the login shell to
git-prompt. Additionally, the git user does not have permissions to
run any other shell.) However, when I remove the git-shell-commands
directory I get (on the local end):

fatal: Interactive git shell is not enabled.
hint: ~/git-shell-commands should exist and have read and execute access.

If no one with more experience has the time to look into your
suggestion, I will try.

Thanks,
Ethan

On Sun, Feb 10, 2013 at 4:25 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ethan Reesor wrote:
>
>> I have a git user set up on my server. It's prompt is set to
>> git-prompt and it's git-shell-commands is empty.
> [...]
>> How do I make the git user work like github where, upon attempting to
>> get a prompt, the connection is closed?
>
> I assume you mean that the user's login shell is git-shell.
>
> You can disable interactive logins by removing the
> ~/git-shell-commands/ directory.  Unfortunately that doesn't let you
> customize the message.  Perhaps it would make sense to teach shell.c
> to look for a
>
>         [shell]
>                 greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.'
>
> setting in git's config file.  What do you think?
>
> Thanks,
> Jonathan



--
Ethan Reesor (Gmail)

Re: Git prompt

[Jeff King]

On Sun, Feb 10, 2013 at 01:25:38PM -0800, Jonathan Nieder wrote:

> Ethan Reesor wrote:
> 
> > I have a git user set up on my server. It's prompt is set to
> > git-prompt and it's git-shell-commands is empty.
> [...]
> > How do I make the git user work like github where, upon attempting to
> > get a prompt, the connection is closed?
> 
> I assume you mean that the user's login shell is git-shell.
> 
> You can disable interactive logins by removing the
> ~/git-shell-commands/ directory.  Unfortunately that doesn't let you
> customize the message.  Perhaps it would make sense to teach shell.c
> to look for a
> 
> 	[shell]
> 		greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.'
> 
> setting in git's config file.  What do you think?

I think something like that makes sense. To my knowledge there is no way
with stock git to customize git-shell's output (at GitHub, that message
comes from our front-end routing process before you even hit git-shell
on our backend machines).

The "username" in our version of the message comes from a database
mapping public keys to GitHub users, not the Unix username.  But I
suspect sites running stock Git would be happy enough to have
%(username) map to the actual Unix username.

-Peff

Re: Git prompt

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Feb 10, 2013 at 01:25:38PM -0800, Jonathan Nieder wrote:
>
>> Ethan Reesor wrote:
>> 
>> > I have a git user set up on my server. It's prompt is set to
>> > git-prompt and it's git-shell-commands is empty.
>> [...]
>> > How do I make the git user work like github where, upon attempting to
>> > get a prompt, the connection is closed?
>> 
>> I assume you mean that the user's login shell is git-shell.
>> 
>> You can disable interactive logins by removing the
>> ~/git-shell-commands/ directory.  Unfortunately that doesn't let you
>> customize the message.  Perhaps it would make sense to teach shell.c
>> to look for a
>> 
>> 	[shell]
>> 		greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.'
>> 
>> setting in git's config file.  What do you think?
>
> I think something like that makes sense. To my knowledge there is no way
> with stock git to customize git-shell's output (at GitHub, that message
> comes from our front-end routing process before you even hit git-shell
> on our backend machines).
>
> The "username" in our version of the message comes from a database
> mapping public keys to GitHub users, not the Unix username.  But I
> suspect sites running stock Git would be happy enough to have
> %(username) map to the actual Unix username.

Yeah, that greeting is cute---I like it ;-)

Re: Git prompt

[Sitaram Chamarty]

On Mon, Feb 11, 2013 at 4:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Sun, Feb 10, 2013 at 01:25:38PM -0800, Jonathan Nieder wrote:
>>
>>> Ethan Reesor wrote:
>>>
>>> > I have a git user set up on my server. It's prompt is set to
>>> > git-prompt and it's git-shell-commands is empty.
>>> [...]
>>> > How do I make the git user work like github where, upon attempting to
>>> > get a prompt, the connection is closed?
>>>
>>> I assume you mean that the user's login shell is git-shell.
>>>
>>> You can disable interactive logins by removing the
>>> ~/git-shell-commands/ directory.  Unfortunately that doesn't let you
>>> customize the message.  Perhaps it would make sense to teach shell.c
>>> to look for a
>>>
>>>      [shell]
>>>              greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.'
>>>
>>> setting in git's config file.  What do you think?
>>
>> I think something like that makes sense. To my knowledge there is no way
>> with stock git to customize git-shell's output (at GitHub, that message
>> comes from our front-end routing process before you even hit git-shell
>> on our backend machines).
>>
>> The "username" in our version of the message comes from a database
>> mapping public keys to GitHub users, not the Unix username.  But I
>> suspect sites running stock Git would be happy enough to have
>> %(username) map to the actual Unix username.
>
> Yeah, that greeting is cute---I like it ;-)

Indeed!  In gitolite, I borrowed that idea added to it by making it
print a list of repos you have access to, along with what permissions
(R or RW) you have :-)

I'm not suggesting git should do that, but instead of a fixed string,
a default command to be executed would be better.  That command could
do anything the local site wanted to make it do, including something
eqvt to what I just said.

This of course now means that the ~/git-shell-commands should not be
empty, since that is where this default command also will be present.

[RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

If I disable git-shell's interactive mode by removing the
~/git-shell-commands directory, then attempts to use 'ssh' with the
git account interactively produce an error message intended for the
administrator:

	$ ssh git@myserver
	fatal: Interactive git shell is not enabled.
	hint: ~/git-shell-commands should exist and have read and execute access.
	$

It is better to give the user a friendly hint that she is on the
right track, like GitHub does:

	Hi <username>! You've successfully authenticated, but
	GitHub does not provide shell access.

An appropriate greeting might even include more complex information,
like a list of repositories the user has access to.  A
git-shell-commands directory with only a "help" script can get us most
of the way there, but it unfortunately it produces a "git>" prompt
where the user can do nothing but ask for more help or exit.  So allow
the "help" script to abort the shell by exiting with nonzero status.

Downside: this will prevent interactive git-shell logins in existing
setups where the "help" script exits with nonzero status by mistake.
Hopefully those are rare enough to not cause much trouble in practice.

Reported-by: Ethan Reesor <firelizzard@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Sitaram Chamarty wrote:

> Indeed!  In gitolite, I borrowed that idea added to it by making it
> print a list of repos you have access to, along with what permissions
> (R or RW) you have :-)
>
> I'm not suggesting git should do that, but instead of a fixed string,
> a default command to be executed would be better.

Good call.

[...]
> This of course now means that the ~/git-shell-commands should not be
> empty, since that is where this default command also will be present.

How about this?

A patch on top could change the default "git-shell-commands is not
present" message if that seems worthwhile.

 Documentation/git-shell.txt | 26 ++++++++++++++++++++++++++
 shell.c                     | 10 ++++++++--
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 9b925060..758083ff 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -29,6 +29,32 @@ read and execute permissions to the directory in order to execute the
 programs in it. The programs are executed with a cwd of $HOME, and
 <argument> is parsed as a command-line string.
 
+When run interactively (with no arguments), 'git-shell' will
+automatically run `~/git-shell-commands/help` on startup, provided it
+exists.  If the 'help' command fails then the interactive shell is
+aborted.
+
+EXAMPLE
+-------
+
+To disable interactive logins, displaying a greeting instead:
++
+----------------
+$ chsh -s /usr/bin/git-shell
+$ mkdir $HOME/git-shell-commands
+$ cat >$HOME/git-shell-commands/help <<\EOF
+#!/bin/sh
+printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"
+printf '%s\n' "provide interactive shell access."
+exit 128
+EOF
+$ chmod +x $HOME/git-shell-commands/help
+----------------
+
+SEE ALSO
+--------
+contrib/git-shell-commands/README
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/shell.c b/shell.c
index 84b237fe..3abc2b84 100644
--- a/shell.c
+++ b/shell.c
@@ -63,10 +63,16 @@ static void cd_to_homedir(void)
 
 static void run_shell(void)
 {
-	int done = 0;
+	int done = 0, status;
 	static const char *help_argv[] = { HELP_COMMAND, NULL };
 	/* Print help if enabled */
-	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+	status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+	if (!status)
+		; /* success */
+	else if (status == -1 && errno == ENOENT)
+		; /* help disabled */
+	else
+		exit(status);
 
 	do {
 		struct strbuf line = STRBUF_INIT;
-- 
1.8.1.3

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> How about this?
>
> A patch on top could change the default "git-shell-commands is not
> present" message if that seems worthwhile.

Hmph.

I wonder if rewording the message when git-shell-commmands directory
is not there may be a better first step (which actually could be the
last step)?

That is, showing something like this,

> +printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"
> +printf '%s\n' "provide interactive shell access."

but rephrased with a reference to "git help shell" for people
preparing their own server when ~/git-shell-commands/ in good order?

Something like

diff --git a/shell.c b/shell.c
index 84b237f..71ff04f 100644
--- a/shell.c
+++ b/shell.c
@@ -162,9 +162,11 @@ int main(int argc, char **argv)
 		/* Allow the user to run an interactive shell */
 		cd_to_homedir();
 		if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
-			die("Interactive git shell is not enabled.\n"
+			die("The user has been recognized as '%s' but "
+			    "interactive git shell is not enabled.\n"
 			    "hint: ~/" COMMAND_DIR " should exist "
-			    "and have read and execute access.");
+			    "and have read and execute access.",
+			    get_user_name());
 		}
 		run_shell();
 		exit(0);

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> How about this?
>>
>> A patch on top could change the default "git-shell-commands is not
>> present" message if that seems worthwhile.
>
> Hmph.
>
> I wonder if rewording the message when git-shell-commmands directory
> is not there may be a better first step (which actually could be the
> last step)?

Maybe, but it's not a step that I'm interested in.  I don't think it
changes the desirability of the patch I sent.  They are independent.

[...]
> --- a/shell.c
> +++ b/shell.c
> @@ -162,9 +162,11 @@ int main(int argc, char **argv)
>  		/* Allow the user to run an interactive shell */
>  		cd_to_homedir();
>  		if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
> -			die("Interactive git shell is not enabled.\n"
> +			die("The user has been recognized as '%s' but "
> +			    "interactive git shell is not enabled.\n"
>  			    "hint: ~/" COMMAND_DIR " should exist "
> -			    "and have read and execute access.");
> +			    "and have read and execute access.",
> +			    get_user_name());

Personally I don't think the hint should be here at all (it should be
obvious that git-shell(1) is the place to read about the login
behavior of an account with shell set to git-shell), but I don't mind
as long as it's possible to override the message.

Thanks,
Jonathan

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> How about this?
>>>
>>> A patch on top could change the default "git-shell-commands is not
>>> present" message if that seems worthwhile.
>>
>> Hmph.
>>
>> I wonder if rewording the message when git-shell-commmands directory
>> is not there may be a better first step (which actually could be the
>> last step)?
>
> Maybe, but it's not a step that I'm interested in.  I don't think it
> changes the desirability of the patch I sent.  They are independent.

What I thought I read in the log message was that you wanted to give
a better message telling the users that the site does _not_ allow an
interactive shell access.  I do not see how that is independent from
a message given from this codepath, where the side has forbidden
shell access by not having ~/git-shell-commands directory in the
first place.  Are you shooting for customizability?

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Junio C Hamano wrote:

>               Are you shooting for customizability?

Yes, and the ability to generate the message dynamically.

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jeff King]

On Sun, Feb 10, 2013 at 08:32:47PM -0800, Jonathan Nieder wrote:

> Junio C Hamano wrote:
> 
> >               Are you shooting for customizability?
> 
> Yes, and the ability to generate the message dynamically.

As far as the default goes, I think the current one is OK, provided
there is an option to customize it (e.g., like your patch). Right now it
is just nonsensical to random users ("What? What in the world is
~/git-shell-commands?"). But once it is customizable, the main consumer
of the message is admins who say "What? Why isn't the git-shell I just
set up working?". The current message helps them diagnose the problem,
and when they are ready to accept connections from random users, they'll
want something customizable anyway.

-Peff

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>>               Are you shooting for customizability?
>
> Yes, and the ability to generate the message dynamically.

Hmph, if that is the case, wouldn't it be a better direction to give
a better help for majority of the case where git-shell is used as
the login shell to allow push and fetch but not for interactive
access at all?

The first step in that direction may be to give a better canned
message, followed by a mechanism (perhaps a hook) that lets a
message customized for the site's needs, no?  Why should a site
administrator create an otherwise empty directory for each and every
user and add an executable in there that shows an error message,
only to improve the default message because it is not friendly
enough?

I may be being slower than usual, but I am still not convinced...

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Ethan Reesor]

On Mon, Feb 11, 2013 at 12:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Hmph, if that is the case, wouldn't it be a better direction to give
> a better help for majority of the case where git-shell is used as
> the login shell to allow push and fetch but not for interactive
> access at all?
>
> The first step in that direction may be to give a better canned
> message, followed by a mechanism (perhaps a hook) that lets a
> message customized for the site's needs, no?  Why should a site
> administrator create an otherwise empty directory for each and every
> user and add an executable in there that shows an error message,
> only to improve the default message because it is not friendly
> enough?

Jonathan made the following comment on the thread I started that lead
to this RFC:
> You can disable interactive logins by removing the
> ~/git-shell-commands/ directory.  Unfortunately that doesn't let you
> customize the message.  Perhaps it would make sense to teach shell.c
> to look for a
>
>        [shell]
>                greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.'
>
> setting in git's config file.

How is this for an alternative? Have shell.c look for
        [shell]
                missing_commands_directory = "Stuff is broke."
setting. If the setting is missing, then it prints the default message
(the current message). That way, there's a default setting, there can
be a system-wide message, there can be a user specific message, and
those messages can be set via `git-commit`.

--
Ethan Reesor

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Ethan Reesor]

I noticed a typo I made. I meant `git-config` rather than
`git-commit`. Sorry for my mistake.

On Mon, Feb 11, 2013 at 12:57 AM, Ethan Reesor <firelizzard@gmail.com> wrote:
> On Mon, Feb 11, 2013 at 12:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Hmph, if that is the case, wouldn't it be a better direction to give
>> a better help for majority of the case where git-shell is used as
>> the login shell to allow push and fetch but not for interactive
>> access at all?
>>
>> The first step in that direction may be to give a better canned
>> message, followed by a mechanism (perhaps a hook) that lets a
>> message customized for the site's needs, no?  Why should a site
>> administrator create an otherwise empty directory for each and every
>> user and add an executable in there that shows an error message,
>> only to improve the default message because it is not friendly
>> enough?
>
> Jonathan made the following comment on the thread I started that lead
> to this RFC:
>> You can disable interactive logins by removing the
>> ~/git-shell-commands/ directory.  Unfortunately that doesn't let you
>> customize the message.  Perhaps it would make sense to teach shell.c
>> to look for a
>>
>>        [shell]
>>                greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.'
>>
>> setting in git's config file.
>
> How is this for an alternative? Have shell.c look for
>         [shell]
>                 missing_commands_directory = "Stuff is broke."
> setting. If the setting is missing, then it prints the default message
> (the current message). That way, there's a default setting, there can
> be a system-wide message, there can be a user specific message, and
> those messages can be set via `git-commit`.
>
> --
> Ethan Reesor



--
Ethan Reesor (Gmail)

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Ethan Reesor wrote:

>                        That way, there's a default setting, there can
> be a system-wide message, there can be a user specific message, and
> those messages can be set via `git-commit`.

That won't let me imitate gitolite's behavior without a lot of
config file churn:

	$ ssh git@localhost
	Hello, jrn.  This is git@elie running git-shell 1.8.1.3.

	 R W	path/to/one/repo
	 R	path/to/another/repo
	$

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Ethan Reesor]

Why not have both? That way there is a way to get a customizable
response that avoids Junio's complaints and there is a way to do what
you are trying to achieve.

On Mon, Feb 11, 2013 at 1:09 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ethan Reesor wrote:
>
>>                        That way, there's a default setting, there can
>> be a system-wide message, there can be a user specific message, and
>> those messages can be set via `git-commit`.
>
> That won't let me imitate gitolite's behavior without a lot of
> config file churn:
>
>         $ ssh git@localhost
>         Hello, jrn.  This is git@elie running git-shell 1.8.1.3.
>
>          R W    path/to/one/repo
>          R      path/to/another/repo
>         $



--
Ethan Reesor (Gmail)

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

[administrivia: please don't top-post]
Ethan Reesor wrote:

> Why not have both? That way there is a way to get a customizable
> response that avoids Junio's complaints and there is a way to do what
> you are trying to achieve.

What was Junio's complaint?

Jonathan

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Ethan Reesor]

On Mon, Feb 11, 2013 at 1:15 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> [administrivia: please don't top-post]
> Ethan Reesor wrote:
>
>> Why not have both? That way there is a way to get a customizable
>> response that avoids Junio's complaints and there is a way to do what
>> you are trying to achieve.
>
> What was Junio's complaint?

I was referring to the one you recently addressed:

On Mon, Feb 11, 2013 at 1:14 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>> Junio C Hamano wrote:
>
>>>>               Are you shooting for customizability?
>>>
>>> Yes, and the ability to generate the message dynamically.
>>
>> Hmph, if that is the case, wouldn't it be a better direction to give
>> a better help for majority of the case where git-shell is used as
>> the login shell to allow push and fetch but not for interactive
>> access at all?
>>
>> The first step in that direction may be to give a better canned
>> message, followed by a mechanism (perhaps a hook) that lets a
>> message customized for the site's needs, no?
>
> The trouble is that I can't imagine a canned message that everyone
> will like.  (For example, I quite dislike the current one.)  That's
> exactly the situation in which some configurability is helpful.
>
> Some configurability is nice for other situations, anyway.  For
> example, sites serving a multilingual audience may want the message to
> vary based on the user's language (or even source IP).  The message
> can include a list of available repositories or extra information that
> changes over time.  And so on.
>
> Hope that helps,
> Jonathan

When I made my suggestion, I was tempted to say that both methods
(having help return non-zero and allowing a git-configurable response)
should be included, but I couldn't think of a reason to include both
until you brought your use case back up.

--
Ethan Reesor (Gmail)

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Junio C Hamano wrote:

>>>               Are you shooting for customizability?
>>
>> Yes, and the ability to generate the message dynamically.
>
> Hmph, if that is the case, wouldn't it be a better direction to give
> a better help for majority of the case where git-shell is used as
> the login shell to allow push and fetch but not for interactive
> access at all?
>
> The first step in that direction may be to give a better canned
> message, followed by a mechanism (perhaps a hook) that lets a
> message customized for the site's needs, no?

The trouble is that I can't imagine a canned message that everyone
will like.  (For example, I quite dislike the current one.)  That's
exactly the situation in which some configurability is helpful.

Some configurability is nice for other situations, anyway.  For
example, sites serving a multilingual audience may want the message to
vary based on the user's language (or even source IP).  The message
can include a list of available repositories or extra information that
changes over time.  And so on.

Hope that helps,
Jonathan

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>> Junio C Hamano wrote:
>
>>>>               Are you shooting for customizability?
>>>
>>> Yes, and the ability to generate the message dynamically.
>>
>> Hmph, if that is the case, wouldn't it be a better direction to give
>> a better help for majority of the case where git-shell is used as
>> the login shell to allow push and fetch but not for interactive
>> access at all?
>>
>> The first step in that direction may be to give a better canned
>> message, followed by a mechanism (perhaps a hook) that lets a
>> message customized for the site's needs, no?
>
> The trouble is that I can't imagine a canned message that everyone
> will like.  (For example, I quite dislike the current one.)  That's
> exactly the situation in which some configurability is helpful.

I am not saying we should have a perfect canned message everybody
likes and not have any configurability.  I however think we can aim
to come up with a message that covers 80% of site administrators who
do not care too much and just want git-shell to allow the standard
services without giving any custom command.

And for the remaining 20% of those who do not like the canned
message but still do not need any custom command, I think it is way
suboptimal to force them to create git-shell-commands directory for
47 users his host gives git-shell access to, and copy the "help"
script to all of them, only to get a customized message.  It would
help them quite a lot if you just called /etc/git/shell-disabled or
some hook that generates a customized message; then there is no need
to add any git-shell-commands directory and a "help" script every
time he gets one new user, no?

For those who _do_ want to give customized commands to their users,
they can already have "help" script to give a friendly message.  It
just felt silly to force sites to create the directory only to
refuse an access to the "custom commands" feature, especially when
the existence of that directory is a signal that the site may want
to give its users an acess to that feature.

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> The trouble is that I can't imagine a canned message that everyone
>> will like.  (For example, I quite dislike the current one.)  That's
>> exactly the situation in which some configurability is helpful.
>
> I am not saying we should have a perfect canned message everybody
> likes and not have any configurability.  I however think we can aim
> to come up with a message that covers 80% of site administrators who
> do not care too much and just want git-shell to allow the standard
> services without giving any custom command.

Isn't the current message meant to be that?  Just removing the "hint:"
line would be enough to leave me happy with it.

> And for the remaining 20% of those who do not like the canned
> message but still do not need any custom command, I think it is way
> suboptimal to force them to create git-shell-commands directory for
> 47 users his host gives git-shell access to, and copy the "help"
> script to all of them, only to get a customized message.

Isn't that a criticism of the git-shell-commands facility in general?
If it is common to have a lot of users with distinct home directories
but all with git-shell as their login shell, then the
git-shell-commands should not go in their home directory to begin
with, no?

I think sharing a home directory is fine and the normal thing to do
with such a restricted account, fwiw, so I am not the one to guess
what people who do something different would find most useful.  Maybe
I am not the right person to have proposed this patch in the first
place --- I saw something that looked wrong and proposed what I
thought was a reasonable fix, but I am not actively depending on
git-shell myself, so...

*shrug*

Hope that helps,
Jonathan

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Isn't that a criticism of the git-shell-commands facility in general?
> If it is common to have a lot of users with distinct home directories
> but all with git-shell as their login shell, then the
> git-shell-commands should not go in their home directory to begin
> with, no?

You can give one set of commands to some users while restricting
others, no?

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Isn't that a criticism of the git-shell-commands facility in general?
>> If it is common to have a lot of users with distinct home directories
>> but all with git-shell as their login shell, then the
>> git-shell-commands should not go in their home directory to begin
>> with, no?
>
> You can give one set of commands to some users while restricting
> others, no?

Yes, I assume one goal of the current design was to let you set up
multiple configurations by making multiple home directories.

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> Isn't that a criticism of the git-shell-commands facility in general?
>>> If it is common to have a lot of users with distinct home directories
>>> but all with git-shell as their login shell, then the
>>> git-shell-commands should not go in their home directory to begin
>>> with, no?
>>
>> You can give one set of commands to some users while restricting
>> others, no?
>
> Yes, I assume one goal of the current design was to let you set up
> multiple configurations by making multiple home directories.

Even if the site configures its 47 git-shell users to share the same
home directory /home/gituser, I still think it is a bad design to
force the administrator to create a directory in it, only to add a
script called "help".

The purpose of the directory is to keep custom commands that are
allowed.  If the site administrator does not want any command, it
would be more natural to expect that the way to disable them would
be _not_ to have that directory which is a collection of allowed
commands.  Adding that directory and add a "help" that exits with
non-zero feels quite a roundabout and counter-intuitive way, no?

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Junio C Hamano wrote:

> The purpose of the directory is to keep custom commands that are
> allowed.  If the site administrator does not want any command, it
> would be more natural to expect that the way to disable them would
> be _not_ to have that directory which is a collection of allowed
> commands.  Adding that directory and add a "help" that exits with
> non-zero feels quite a roundabout and counter-intuitive way, no?

I think it comes down to the reason the site admin doesn't want to
allow interactive logins.  That reason seems to be mostly that
presenting a

	git>

prompt at which you can only ask for "help" or "exit" is a bit
confusing and pointless.  I have sympathy for that, which is why I
looked for a way for the admin to ask to avoid the prompt altogether
in that case.

I do not think the reason is "because I don't want a
git-shell-commands directory".  I think it's good to have basically
one kind of setup instead of significantly different ones with and
without that special directory --- and it means that starting from a
setup like this, one can easily drop in additional commands like
set-head or create-repo without changing anything basic.  It's making
the admin's later life easier.

Maybe a better test than "help exits with special exit code" is "there
are no other custom commands than help".  Would that be more sensible?

>From a "make it possible to emulate gitolite" point of view, that
doesn't permit disabling the interactive mode when there are other
commands available, so my hunch is that it wouldn't.

Jonathan

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> The purpose of the directory is to keep custom commands that are
>> allowed.  If the site administrator does not want any command, it
>> would be more natural to expect that the way to disable them would
>> be _not_ to have that directory which is a collection of allowed
>> commands.  Adding that directory and add a "help" that exits with
>> non-zero feels quite a roundabout and counter-intuitive way, no?
>
> I think it comes down to the reason the site admin doesn't want to
> allow interactive logins.  That reason seems to be mostly that
> presenting a
>
> 	git>
>
> prompt at which you can only ask for "help" or "exit" is a bit
> confusing and pointless.  I have sympathy for that, which is why I
> looked for a way for the admin to ask to avoid the prompt altogether
> in that case.

Yeah, the prompt does look pointless.

> I do not think the reason is "because I don't want a
> git-shell-commands directory".  I think it's good to have basically
> one kind of setup instead of significantly different ones with and
> without that special directory --- and it means that starting from a
> setup like this, one can easily drop in additional commands like
> set-head or create-repo without changing anything basic.  It's making
> the admin's later life easier.

I do not think I follow.  If the admin wants to eventually have
extra commands supported at the site, but not yet ready to do so,
isn't it more natural to start with a less elaborate configuration
(i.e. without the directory) now and then add the directory when the
site is ready for offering extra commands later?

> Maybe a better test than "help exits with special exit code" is "there
> are no other custom commands than help".  Would that be more sensible?
>
> From a "make it possible to emulate gitolite" point of view, that
> doesn't permit disabling the interactive mode when there are other
> commands available, so my hunch is that it wouldn't.

A paragraph I had in the message you are responding to before I sent
it out (but removed because it felt somewhat offtopic) said "if the
mechanism to disable weren't the magic 'help exited with failure'
but 'an interactive-disabled flag file exists there', I may find it
less strange to have the directory there", or something like that.

And that flag file could be a custom script that gives a custom
message.

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jeff King]

On Sun, Feb 10, 2013 at 11:17:24PM -0800, Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Isn't that a criticism of the git-shell-commands facility in general?
> > If it is common to have a lot of users with distinct home directories
> > but all with git-shell as their login shell, then the
> > git-shell-commands should not go in their home directory to begin
> > with, no?
> 
> You can give one set of commands to some users while restricting
> others, no?

But that seems to me to argue against /etc/git/shell-disabled or
similar, which would apply to every user. Or are you proposing that the
check be:

  if -d ~/git-shell-commands; then
          : ok, interactive
  elif -x /etc/git/shell-disabled; then
          exec /etc/git/shell-disabled
  else
          echo >&2 'go away'
          exit 1
  fi

That at least means you can apply _whether_ to disable the shell
selectively for each user (by providing or not a git-shell-commands
directory), but you cannot individually select the script that runs for
that user.  But it's probably still flexible enough; you can, after all, run
arbitrary code in the shell-disabled script, so it can select which
class of user it was called on and dispatch to a sub-script.

-Peff

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Feb 10, 2013 at 11:17:24PM -0800, Junio C Hamano wrote:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>> 
>> > Isn't that a criticism of the git-shell-commands facility in general?
>> > If it is common to have a lot of users with distinct home directories
>> > but all with git-shell as their login shell, then the
>> > git-shell-commands should not go in their home directory to begin
>> > with, no?
>> 
>> You can give one set of commands to some users while restricting
>> others, no?
>
> But that seems to me to argue against /etc/git/shell-disabled or
> similar, which would apply to every user. Or are you proposing that the
> check be:
>
>   if -d ~/git-shell-commands; then
>           : ok, interactive
>   elif -x /etc/git/shell-disabled; then
>           exec /etc/git/shell-disabled
>   else
>           echo >&2 'go away'
>           exit 1
>   fi

That "shell-disabled" thing was to allow customizing the existing
die() that triggers here:

	} else if (argc == 1) {
		/* Allow the user to run an interactive shell */
		cd_to_homedir();
		if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
			die("Interactive git shell is not enabled.\n"
			    "hint: ~/" COMMAND_DIR " should exist "
			    "and have read and execute access.");
		}
		run_shell();
		exit(0);

so it is more like

	if ! test -d $HOME/git-shell-commands
	then
		if test -x /etc/git/shell-disabled
                then
			exec /etc/git/shell-disabled
		else
			die Interactive is not enabled
		fi
	fi
        ... do whatever in run_shell() ...


> That at least means you can apply _whether_ to disable the shell
> selectively for each user (by providing or not a git-shell-commands
> directory), but you cannot individually select the script that runs for
> that user.  But it's probably still flexible enough;...

Such a flexibility is not a goal of /etc/git/shell-disabled.  The
sole goal is to make the life easier for those site owners that do
not want any interactive shell access to give more friendly and
customized error message.

Those who want further flexibility can exit with non-zero from the
"help" (which is still a misnomer for a hook to disable interactive
for the user).

My primary objection is that implementing only that "more flexible
but requires more configuration work" solution without giving
simpler solution (i.e. just one thing to configure) to the majory of
site owners who only have simpler problem to solve (i.e. just want
to customize "no interactive here"), and saying that the latter can
be done on top.  It is backwards mentality.

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jeff King]

On Mon, Feb 11, 2013 at 09:18:18AM -0800, Junio C Hamano wrote:

> That "shell-disabled" thing was to allow customizing the existing
> die() that triggers here:
> [...]
> so it is more like
> 
> 	if ! test -d $HOME/git-shell-commands
> 	then
> 		if test -x /etc/git/shell-disabled
>                 then
> 			exec /etc/git/shell-disabled
> 		else
> 			die Interactive is not enabled
> 		fi
> 	fi
>         ... do whatever in run_shell() ...

OK, that is equivalent to what I said (or at least what I was trying to
say :) ).

> > That at least means you can apply _whether_ to disable the shell
> > selectively for each user (by providing or not a git-shell-commands
> > directory), but you cannot individually select the script that runs for
> > that user.  But it's probably still flexible enough;...
> 
> Such a flexibility is not a goal of /etc/git/shell-disabled.  The
> sole goal is to make the life easier for those site owners that do
> not want any interactive shell access to give more friendly and
> customized error message.
> 
> Those who want further flexibility can exit with non-zero from the
> "help" (which is still a misnomer for a hook to disable interactive
> for the user).

Ah, I thought you were proposing shell-disabled _instead_ of Jonathan's
patch, not in addition to.

> My primary objection is that implementing only that "more flexible
> but requires more configuration work" solution without giving
> simpler solution (i.e. just one thing to configure) to the majory of
> site owners who only have simpler problem to solve (i.e. just want
> to customize "no interactive here"), and saying that the latter can
> be done on top.  It is backwards mentality.

Oh, absolutely. The easy case should be easy, and the hard case
possible. But another way of doing that (which would also make life
easier for admins who want to share config besides shell-disabled) would
be:

  1. Give Jonathan's magic meaning to ~/git-shell-commands/help's exit
     code.

  2. Make /etc/git/shell-commands a fallback if ~/git-shell-commands
     does not exist.

That turns your /etc/git/shell-disabled into /etc/git/shell-commands/help.
It is just as simple to do a site-wide change, still allows per-user
overrides, and additionally gives people who _do_ want the interactive
commands the ability to configure them site-wide instead of symlinking a
directory into everybody's homedir.

The only downside is that it has the confusing "create this directory to
turn on interactivity, then create a file in it to turn it back off"
feature.

I admit I don't care too much, though. I have never actually used
git-shell, as my systems are all either too small (i.e., users are
trusted and have shell access) or too big (grown well beyond a single
server that connects users straight to git-shell). In fact, there seems
to be a lot of guessing in this thread about what people would want, as
it seems none of us actually uses the feature. Maybe that is a sign it
is being over-engineered. :)

-Peff

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Ethan Reesor]

On Mon, Feb 11, 2013 at 2:12 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Isn't that a criticism of the git-shell-commands facility in general?
> If it is common to have a lot of users with distinct home directories
> but all with git-shell as their login shell, then the
> git-shell-commands should not go in their home directory to begin
> with, no?

I know nothing of the security issues, but why not have a
/etc/git-shell-commands?

-- 
Ethan Reesor (Gmail)

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Ethan Reesor]

On Mon, Feb 11, 2013 at 2:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> And for the remaining 20% of those who do not like the canned
> message but still do not need any custom command, I think it is way
> suboptimal to force them to create git-shell-commands directory for
> 47 users his host gives git-shell access to, and copy the "help"
> script to all of them, only to get a customized message.  It would
> help them quite a lot if you just called /etc/git/shell-disabled or
> some hook that generates a customized message; then there is no need
> to add any git-shell-commands directory and a "help" script every
> time he gets one new user, no?
>
> For those who _do_ want to give customized commands to their users,
> they can already have "help" script to give a friendly message.  It
> just felt silly to force sites to create the directory only to
> refuse an access to the "custom commands" feature, especially when
> the existence of that directory is a signal that the site may want
> to give its users an acess to that feature.

Again, would it not be more elegant and powerful to A) have the
shell-disabled message/hook/etc specified by git-config on some level,
be it /etc/gitconfig or ~/.gitconfig, and B) have Jonathan's patch
whereby ~/git-shell-commands/help returning non-zero closes the
connection? Have shell.c read for settings in the pattern:
        [shell "disabled"]
                message = "Hi, this is your server speaking. I've
replaced the usual message."
                command = "/path/to/some/command"
If shell.disabled.command is defined, don't bother with the message.
If it is not, but shell.disabled.message is, display that. If neither
of them are, display the default message, and make that one more
friendly.

Even if that was implemented, there is still an argument for
Jonathan's patch. For example, I'm building a server where
~/git-shell-commands/help does something interesting. But sometimes,
something fails. When that something fails, I want to close the
connection for whatever reason.

So, any reason not to have both (on top of making a better default message)?

--
Ethan Reesor (Gmail)

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Ethan Reesor <firelizzard@gmail.com> writes:

>> For those who _do_ want to give customized commands to their users,
>> they can already have "help" script to give a friendly message.  It
>> just felt silly to force sites to create the directory only to
>> refuse an access to the "custom commands" feature, especially when
>> the existence of that directory is a signal that the site may want
>> to give its users an acess to that feature.
>
> Again, would it not be more elegant and powerful to A) have the
> shell-disabled message/hook/etc specified by git-config on some level,
> be it /etc/gitconfig or ~/.gitconfig, and B) have Jonathan's patch
> whereby ~/git-shell-commands/help returning non-zero closes the
> connection?

Isn't that what I have essentially been saying?

For sites that do not want per-user customizable "other commands",
have a single site-wide hook instead of having to create otherwise
empty shell-commands directories for all users.  For users a site
wants to allow customized commands, have the directory and custom
"help" message.  I do not care too deeply if "help" exiting non-zero
caused the connection closed, but I care about not forcing a lot of
effort to customize messages to people who do *not* need
customizability.

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Ethan Reesor]

On Mon, Feb 11, 2013 at 2:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ethan Reesor <firelizzard@gmail.com> writes:
>> Again, would it not be more elegant and powerful to A) have the
>> shell-disabled message/hook/etc specified by git-config on some level,
>> be it /etc/gitconfig or ~/.gitconfig, and B) have Jonathan's patch
>> whereby ~/git-shell-commands/help returning non-zero closes the
>> connection?
>
> Isn't that what I have essentially been saying?

That is what you've been saying. I reiterated because I like the idea
of having it managed via git config.

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Junio C Hamano]

Ethan Reesor <firelizzard@gmail.com> writes:

> On Mon, Feb 11, 2013 at 2:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ethan Reesor <firelizzard@gmail.com> writes:
>>> Again, would it not be more elegant and powerful to A) have the
>>> shell-disabled message/hook/etc specified by git-config on some level,
>>> be it /etc/gitconfig or ~/.gitconfig, and B) have Jonathan's patch
>>> whereby ~/git-shell-commands/help returning non-zero closes the
>>> connection?
>>
>> Isn't that what I have essentially been saying?
>
> That is what you've been saying. I reiterated because I like the idea
> of having it managed via git config.

Yes, and I've been ignoring the "git config".  I do not think it
gives enough customizability Jonathan's example of listing user
owned repositories, for example.  Having a config variable in
/etc/gitconfig that points at a random script on the filesystem does
not buy us much over an approach to have a global hook at a known
place on the filesystem, no?

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jeff King]

On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote:

> > This of course now means that the ~/git-shell-commands should not be
> > empty, since that is where this default command also will be present.
> 
> How about this?

I like the general direction this is going, but:

> +When run interactively (with no arguments), 'git-shell' will
> +automatically run `~/git-shell-commands/help` on startup, provided it
> +exists.  If the 'help' command fails then the interactive shell is
> +aborted.

Doesn't that mean that people who currently do allow interactive access
and have a ~/git-shell-commands/help (that returns zero) will get
spammed by its as a motd each time they connect?

To be honest, I am not really clear on what interactive git-shell is
used for. AFAIK, it does nothing unless you have set up custom commands,
and I have never actually seen them in the wild. So maybe it is not a
big deal.

If I understand correctly, calling it "check-interactive", "greeting",
or something instead of "help" would be sufficient, and then you
wouldn't have to worry about backwards compatibility.

> diff --git a/shell.c b/shell.c
> index 84b237fe..3abc2b84 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -63,10 +63,16 @@ static void cd_to_homedir(void)
>  
>  static void run_shell(void)
>  {
> -	int done = 0;
> +	int done = 0, status;
>  	static const char *help_argv[] = { HELP_COMMAND, NULL };
>  	/* Print help if enabled */
> -	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +	status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +	if (!status)
> +		; /* success */
> +	else if (status == -1 && errno == ENOENT)
> +		; /* help disabled */
> +	else
> +		exit(status);

This kicks in only when there is no command given, right? So if I ran
"ssh example.com", it would give me the help message rather than (or in
addition) giving me interactive access.

What about "ssh example.com foo"? Do we want to allow a custom message
there, too (it might be different there; e.g., an allowed list of
commands might make more sense)?

-Peff

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Jeff King wrote:
> On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote:

>> +When run interactively (with no arguments), 'git-shell' will
>> +automatically run `~/git-shell-commands/help` on startup, provided it
>> +exists.  If the 'help' command fails then the interactive shell is
>> +aborted.
>
> Doesn't that mean that people who currently do allow interactive access
> and have a ~/git-shell-commands/help (that returns zero) will get
> spammed by its as a motd each time they connect?

Only interactive connections.  That's the existing behavior.

[...]
> What about "ssh example.com foo"? Do we want to allow a custom message
> there, too (it might be different there; e.g., an allowed list of
> commands might make more sense)?

I wouldn't mind, but it's definitely not my itch.

Hoping that clarifies,
Jonathan

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jeff King]

On Sun, Feb 10, 2013 at 08:14:04PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> > On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote:
> 
> >> +When run interactively (with no arguments), 'git-shell' will
> >> +automatically run `~/git-shell-commands/help` on startup, provided it
> >> +exists.  If the 'help' command fails then the interactive shell is
> >> +aborted.
> >
> > Doesn't that mean that people who currently do allow interactive access
> > and have a ~/git-shell-commands/help (that returns zero) will get
> > spammed by its as a motd each time they connect?
> 
> Only interactive connections.  That's the existing behavior.

Ah, sorry. I misread the patch. I see now that we already run help, and
this is just making the exit value significant. In that case, yeah, I
think it's fine.

-Peff

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Jeff King wrote:
> On Sun, Feb 10, 2013 at 08:14:04PM -0800, Jonathan Nieder wrote:

>> Only interactive connections.  That's the existing behavior.
>
> Ah, sorry. I misread the patch. I see now that we already run help, and
> this is just making the exit value significant. In that case, yeah, I
> think it's fine.

No problem --- the description was unclear.  Would retitling the patch
to "shell: pay attention to exit status from 'help' command" work?

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jeff King]

On Sun, Feb 10, 2013 at 08:26:09PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> > On Sun, Feb 10, 2013 at 08:14:04PM -0800, Jonathan Nieder wrote:
> 
> >> Only interactive connections.  That's the existing behavior.
> >
> > Ah, sorry. I misread the patch. I see now that we already run help, and
> > this is just making the exit value significant. In that case, yeah, I
> > think it's fine.
> 
> No problem --- the description was unclear.  Would retitling the patch
> to "shell: pay attention to exit status from 'help' command" work?

I think what threw me off was reading the documentation part of the
patch, which adds a note that we run "help" on startup, and then
elaborates on the exit value. I didn't realize that the first half was
documenting what already happened.

Tweaking the third paragraph of the commit message to:

  An appropriate greeting might even include more complex information,
  like a list of repositories the user has access to.  If the
  git-shell-commands directory exists and contains a "help" script, we
  already run it when the shell is run without any commands, giving the
  server a chance to provide a custom message. Unfortunately, the
  presence of the git-shell-commands directory means we also enter an
  interactive mode, prompting and accepting commands (of which there may
  be none) from the user, which many servers would not want. To solve
  this, we abort the interactive shell on a non-zero exit code from the
  "help" script. This lets the server say whatever it likes, and then
  hangup.

makes it more clear to me. But once you explained it, I realize that I
also could have just read the C code part of the patch more carefully. :)

So I'm fine with or without that change.

-Peff

[PATCH 0/2 v2] shell: allow 'help' command to disable interactive shell

[Jonathan Nieder]

Jeff King wrote:

> I think what threw me off was reading the documentation part of the
> patch, which adds a note that we run "help" on startup, and then
> elaborates on the exit value. I didn't realize that the first half was
> documenting what already happened.
>
> Tweaking the third paragraph of the commit message to:

Very nice.  How about this version?

Jonathan Nieder (2):
  shell doc: emphasize purpose and security guarantees
  shell: pay attention to exit status from 'help' command

 Documentation/git-shell.txt | 86 +++++++++++++++++++++++++++++++++++++--------
 shell.c                     | 10 ++++--
 2 files changed, 79 insertions(+), 17 deletions(-)

[PATCH 1/2] shell doc: emphasize purpose and security model

[Jonathan Nieder]

The original git-shell(1) manpage emphasized that the shell
supports only git transport commands, and as the shell gained
features that emphasis and focus in the manual has been lost.
Bring it back by splitting the manpage into a few short sections
and fleshing out each:

 - SYNOPSIS, describing how the shell gets used in practice
 - DESCRIPTION, which gives an overview of the purpose and
   guarantees provided by this restricted shell
 - COMMANDS, listing supported commands and restrictions on the
   arguments they accept
 - INTERACTIVE USE, describing the interactive mode

Also add a "see also" section with some relevant related reading.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
New text.  Split off from patch 2 --- this is just documenting
existing behavior.

 Documentation/git-shell.txt | 66 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 9b925060..4fe93203 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -9,25 +9,61 @@ git-shell - Restricted login shell for Git-only SSH access
 SYNOPSIS
 --------
 [verse]
-'git shell' [-c <command> <argument>]
+'chsh' -s $(which git-shell) git
+'git clone' `git@localhost:/path/to/repo.git`
+'ssh' `git@localhost`
 
 DESCRIPTION
 -----------
 
-A login shell for SSH accounts to provide restricted Git access. When
-'-c' is given, the program executes <command> non-interactively;
-<command> can be one of 'git receive-pack', 'git upload-pack', 'git
-upload-archive', 'cvs server', or a command in COMMAND_DIR. The shell
-is started in interactive mode when no arguments are given; in this
-case, COMMAND_DIR must exist, and any of the executables in it can be
-invoked.
-
-'cvs server' is a special command which executes git-cvsserver.
-
-COMMAND_DIR is the path "$HOME/git-shell-commands". The user must have
-read and execute permissions to the directory in order to execute the
-programs in it. The programs are executed with a cwd of $HOME, and
-<argument> is parsed as a command-line string.
+This is a login shell for SSH accounts to provide restricted Git access.
+It permits execution only of server-side Git commands implementing the
+pull/push functionality, plus custom commands present in a subdirectory
+named `git-shell-commands` in the user's home directory.
+
+COMMANDS
+--------
+
+'git shell' accepts the following commands after the '-c' option:
+
+'git receive-pack <argument>'::
+'git upload-pack <argument>'::
+'git upload-archive <argument>'::
+	Call the corresponding server-side command to support
+	the client's 'git push', 'git fetch', or 'git archive --remote'
+	request.
+'cvs server'::
+	Imitate a CVS server.  See linkgit:git-cvsserver[1].
+
+If a `~/git-shell-commands` directory is present, 'git shell' will
+also handle other, custom commands by running
+"`git-shell-commands/<command> <arguments>`" from the user's home
+directory.
+
+INTERACTIVE USE
+---------------
+
+By default, the commands above can be executed only with the '-c'
+option; the shell is not interactive.
+
+If a `~/git-shell-commands` directory is present, 'git shell'
+can also be run interactively (with no arguments).  If a `help`
+command is present in the `git-shell-commands` directory, it is
+run to provide the user with an overview of allowed actions.  Then a
+"`git> `" prompt is presented at which one can enter any of the
+commands from the `git-shell-commands` directory, or `exit` to close
+the connection.
+
+Generally this mode is used as an administrative interface to allow
+users to list repositories they have access to, create, delete, or
+rename repositories, or change repository descriptions and
+permissions.
+
+SEE ALSO
+--------
+ssh(1),
+linkgit:git-daemon[1],
+contrib/git-shell-commands/README
 
 GIT
 ---
-- 
1.8.1.3

Re: [PATCH 1/2] shell doc: emphasize purpose and security model

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
> index 9b925060..4fe93203 100644
> --- a/Documentation/git-shell.txt
> +++ b/Documentation/git-shell.txt
> @@ -9,25 +9,61 @@ git-shell - Restricted login shell for Git-only SSH access
>  SYNOPSIS
>  --------
>  [verse]
> -'git shell' [-c <command> <argument>]
> +'chsh' -s $(which git-shell) git

<review type="nitpick" mode="posix-police">
Please don't use "which" in scripts.  Perhaps "command -v" is more
suitable here.
</review>

Otherwise looks good to me.  Thanks.

> +'git clone' `git@localhost:/path/to/repo.git`
> +'ssh' `git@localhost`
>  
>  DESCRIPTION
>  -----------
>  
> +This is a login shell for SSH accounts to provide restricted Git access.
> +It permits execution only of server-side Git commands implementing the
> +pull/push functionality, plus custom commands present in a subdirectory
> +named `git-shell-commands` in the user's home directory.
> +
> +COMMANDS
> +--------
> +
> +'git shell' accepts the following commands after the '-c' option:
> +
> +'git receive-pack <argument>'::
> +'git upload-pack <argument>'::
> +'git upload-archive <argument>'::
> +	Call the corresponding server-side command to support
> +	the client's 'git push', 'git fetch', or 'git archive --remote'
> +	request.
> +'cvs server'::
> +	Imitate a CVS server.  See linkgit:git-cvsserver[1].
> +
> +If a `~/git-shell-commands` directory is present, 'git shell' will
> +also handle other, custom commands by running
> +"`git-shell-commands/<command> <arguments>`" from the user's home
> +directory.
> +
> +INTERACTIVE USE
> +---------------
> +
> +By default, the commands above can be executed only with the '-c'
> +option; the shell is not interactive.
> +
> +If a `~/git-shell-commands` directory is present, 'git shell'
> +can also be run interactively (with no arguments).  If a `help`
> +command is present in the `git-shell-commands` directory, it is
> +run to provide the user with an overview of allowed actions.  Then a
> +"`git> `" prompt is presented at which one can enter any of the
> +commands from the `git-shell-commands` directory, or `exit` to close
> +the connection.
> +
> +Generally this mode is used as an administrative interface to allow
> +users to list repositories they have access to, create, delete, or
> +rename repositories, or change repository descriptions and
> +permissions.
> +
> +SEE ALSO
> +--------
> +ssh(1),
> +linkgit:git-daemon[1],
> +contrib/git-shell-commands/README
>  
>  GIT
>  ---

Re: [PATCH 1/2] shell doc: emphasize purpose and security model

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> --- a/Documentation/git-shell.txt
>> +++ b/Documentation/git-shell.txt
>> @@ -9,25 +9,61 @@ git-shell - Restricted login shell for Git-only SSH access
>>  SYNOPSIS
>>  --------
>>  [verse]
>> -'git shell' [-c <command> <argument>]
>> +'chsh' -s $(which git-shell) git
[...]
>                                               "command -v"

Sounds good.

(chsh isn't in POSIX either, FWIW. ;-))

Jonathan

Re: [PATCH 1/2] shell doc: emphasize purpose and security model

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
> index 9b925060..4fe93203 100644
> --- a/Documentation/git-shell.txt
> +++ b/Documentation/git-shell.txt
> @@ -9,25 +9,61 @@ git-shell - Restricted login shell for Git-only SSH access
>  SYNOPSIS
>  --------
>  [verse]
> -'git shell' [-c <command> <argument>]
> +'chsh' -s $(which git-shell) git
> +'git clone' `git@localhost:/path/to/repo.git`
> +'ssh' `git@localhost`

I am wondering if we want to do the following instead of/in addition
to fixing the $(which git-shell).  It is not like we only allow a
single user and its name has to be 'git'.

diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 4fe9320..6829ea9 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -9,9 +9,9 @@ git-shell - Restricted login shell for Git-only SSH access
 SYNOPSIS
 --------
 [verse]
-'chsh' -s $(which git-shell) git
-'git clone' `git@localhost:/path/to/repo.git`
-'ssh' `git@localhost`
+'chsh' -s /path/to/git-shell <user>
+'git clone' `<user>@localhost:/path/to/repo.git`
+'ssh' `<user>@localhost`
 
 DESCRIPTION
 -----------

[PATCH 2/2] shell: pay attention to exit status from 'help' command

[Jonathan Nieder]

If I disable git-shell's interactive mode by removing the
~/git-shell-commands directory, then attempts to use 'ssh' with the
git account interactively produce an error message intended for the
administrator:

	$ ssh git@myserver
	fatal: Interactive git shell is not enabled.
	hint: ~/git-shell-commands should exist and have read and execute access.
	$

That is helpful for the new admin who is wondering "What? Why isn't
the git-shell I just set up working?", but once the site setup is
finished, it is better to give the user a friendly hint that she is on
the right track, like GitHub does:

	Hi <username>! You've successfully authenticated, but
	GitHub does not provide shell access.

An appropriate greeting might even include more complex information,
like a list of repositories the user has access to.  If the
git-shell-commands directory exists and contains a "help" script, we
already run it when the shell is run without any commands, giving the
server a chance to provide a custom message.  Unfortunately, the
presence of the git-shell-commands directory means we also enter an
interactive mode, prompting and accepting commands (of which there may
be none) from the user, which many servers would not want.  To solve
this, we abort the interactive shell on a non-zero exit code from the
"help" script.  This lets the server say whatever it likes, and then
hang up.

Downside: this will prevent interactive git-shell logins in existing
setups where the "help" script exits with nonzero status by mistake.
Hopefully those are rare enough to not cause much trouble in practice.

Reported-by: Ethan Reesor <firelizzard@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Jeff King <peff@peff.net>
---
 Documentation/git-shell.txt | 20 ++++++++++++++++++++
 shell.c                     | 10 ++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 4fe93203..60051e63 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -59,6 +59,26 @@ users to list repositories they have access to, create, delete, or
 rename repositories, or change repository descriptions and
 permissions.
 
+If the `help` command exists and exits with nonzero status, the
+interactive shell is aborted.
+
+EXAMPLE
+-------
+
+To disable interactive logins, displaying a greeting instead:
++
+----------------
+$ chsh -s /usr/bin/git-shell
+$ mkdir $HOME/git-shell-commands
+$ cat >$HOME/git-shell-commands/help <<\EOF
+#!/bin/sh
+printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"
+printf '%s\n' "provide interactive shell access."
+exit 128
+EOF
+$ chmod +x $HOME/git-shell-commands/help
+----------------
+
 SEE ALSO
 --------
 ssh(1),
diff --git a/shell.c b/shell.c
index 84b237fe..3abc2b84 100644
--- a/shell.c
+++ b/shell.c
@@ -63,10 +63,16 @@ static void cd_to_homedir(void)
 
 static void run_shell(void)
 {
-	int done = 0;
+	int done = 0, status;
 	static const char *help_argv[] = { HELP_COMMAND, NULL };
 	/* Print help if enabled */
-	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+	status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+	if (!status)
+		; /* success */
+	else if (status == -1 && errno == ENOENT)
+		; /* help disabled */
+	else
+		exit(status);
 
 	do {
 		struct strbuf line = STRBUF_INIT;
-- 
1.8.1.3

Re: [PATCH 2/2] shell: pay attention to exit status from 'help' command

[Ethan Reesor]

I feel like the suggestion I posted in response to Junio C Hamano
<gitster@pobox.com>'s complaint on the RFC for this patch provides a
more elegant solution to the problem of administrators wanting to
prevent interactive sessions for users with their login shell set to
git-prompt. The suggestion was as follows:

> How is this for an alternative? Have shell.c look for a
>        [shell]
>                missing_commands_directory = "Stuff is broke."
> setting. If the setting is missing, then it prints the default message
> (the current message). That way, there's a default setting, there can
> be a system-wide message, there can be a user specific message, and
> those messages can be set via `git-config`.

On Mon, Feb 11, 2013 at 12:58 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> If I disable git-shell's interactive mode by removing the
> ~/git-shell-commands directory, then attempts to use 'ssh' with the
> git account interactively produce an error message intended for the
> administrator:
>
>         $ ssh git@myserver
>         fatal: Interactive git shell is not enabled.
>         hint: ~/git-shell-commands should exist and have read and execute access.
>         $
>
> That is helpful for the new admin who is wondering "What? Why isn't
> the git-shell I just set up working?", but once the site setup is
> finished, it is better to give the user a friendly hint that she is on
> the right track, like GitHub does:
>
>         Hi <username>! You've successfully authenticated, but
>         GitHub does not provide shell access.
>
> An appropriate greeting might even include more complex information,
> like a list of repositories the user has access to.  If the
> git-shell-commands directory exists and contains a "help" script, we
> already run it when the shell is run without any commands, giving the
> server a chance to provide a custom message.  Unfortunately, the
> presence of the git-shell-commands directory means we also enter an
> interactive mode, prompting and accepting commands (of which there may
> be none) from the user, which many servers would not want.  To solve
> this, we abort the interactive shell on a non-zero exit code from the
> "help" script.  This lets the server say whatever it likes, and then
> hang up.
>
> Downside: this will prevent interactive git-shell logins in existing
> setups where the "help" script exits with nonzero status by mistake.
> Hopefully those are rare enough to not cause much trouble in practice.
>
> Reported-by: Ethan Reesor <firelizzard@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Improved-by: Jeff King <peff@peff.net>
> ---
>  Documentation/git-shell.txt | 20 ++++++++++++++++++++
>  shell.c                     | 10 ++++++++--
>  2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
> index 4fe93203..60051e63 100644
> --- a/Documentation/git-shell.txt
> +++ b/Documentation/git-shell.txt
> @@ -59,6 +59,26 @@ users to list repositories they have access to, create, delete, or
>  rename repositories, or change repository descriptions and
>  permissions.
>
> +If the `help` command exists and exits with nonzero status, the
> +interactive shell is aborted.
> +
> +EXAMPLE
> +-------
> +
> +To disable interactive logins, displaying a greeting instead:
> ++
> +----------------
> +$ chsh -s /usr/bin/git-shell
> +$ mkdir $HOME/git-shell-commands
> +$ cat >$HOME/git-shell-commands/help <<\EOF
> +#!/bin/sh
> +printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"
> +printf '%s\n' "provide interactive shell access."
> +exit 128
> +EOF
> +$ chmod +x $HOME/git-shell-commands/help
> +----------------
> +
>  SEE ALSO
>  --------
>  ssh(1),
> diff --git a/shell.c b/shell.c
> index 84b237fe..3abc2b84 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -63,10 +63,16 @@ static void cd_to_homedir(void)
>
>  static void run_shell(void)
>  {
> -       int done = 0;
> +       int done = 0, status;
>         static const char *help_argv[] = { HELP_COMMAND, NULL };
>         /* Print help if enabled */
> -       run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +       status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +       if (!status)
> +               ; /* success */
> +       else if (status == -1 && errno == ENOENT)
> +               ; /* help disabled */
> +       else
> +               exit(status);
>
>         do {
>                 struct strbuf line = STRBUF_INIT;
> --
> 1.8.1.3
>



--
Ethan Reesor (Gmail)

Re: [PATCH 2/2] shell: pay attention to exit status from 'help' command

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
> index 4fe93203..60051e63 100644
> --- a/Documentation/git-shell.txt
> +++ b/Documentation/git-shell.txt
> @@ -59,6 +59,26 @@ users to list repositories they have access to, create, delete, or
>  rename repositories, or change repository descriptions and
>  permissions.
>  
> +If the `help` command exists and exits with nonzero status, the
> +interactive shell is aborted.
> +
> +EXAMPLE
> +-------
> +
> +To disable interactive logins, displaying a greeting instead:
> ++
> +----------------
> +$ chsh -s /usr/bin/git-shell
> +$ mkdir $HOME/git-shell-commands
> +$ cat >$HOME/git-shell-commands/help <<\EOF
> +#!/bin/sh
> +printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"

Where in the sshd to git-shell exec chain is $USER variable set for
the user?  Just being curious if this is the simplest but one of the
more robust ways to get the user's name.

I still think forcing the site administrator create a directory for
each and every user only to house a single script that denies the
access is a wrong design, but the code seems to correctly implement
that design.

Re: [PATCH 2/2] shell: pay attention to exit status from 'help' command

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> +To disable interactive logins, displaying a greeting instead:
>> ++
>> +----------------
>> +$ chsh -s /usr/bin/git-shell
>> +$ mkdir $HOME/git-shell-commands
>> +$ cat >$HOME/git-shell-commands/help <<\EOF
>> +#!/bin/sh
>> +printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"
>
> Where in the sshd to git-shell exec chain is $USER variable set for
> the user?  Just being curious if this is the simplest but one of the
> more robust ways to get the user's name.

That's a good question.  environment= in an authorized_keys file is
obsolete, so USER generally represents the actual logged in user.

That means the main way to base behavior on private key (letting one
system user represent multiple people) is a gitolite-style command=
wrapper that checks SSH_ORIGINAL_COMMAND.  In that setup, there is no
reason to forward simple no-args "are you there?" requests to the
git-shell, so we can forget about it here.

So by the time we get to git-shell, most likely either

 A) this is a generic system user, with a username like "git", and the
    above example would insult the client with "Hi git!" or "Hi
    project-x-git!"

or

 B) each person has a separate account on the system, perhaps to help
    the admin to set filesystem permissions based on users and groups,
    and the above would address the user by her normal name.

Jonathan

Re: [PATCH 2/2] shell: pay attention to exit status from 'help' command

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> +To disable interactive logins, displaying a greeting instead:
>>> ++
>>> +----------------
>>> +$ chsh -s /usr/bin/git-shell
>>> +$ mkdir $HOME/git-shell-commands
>>> +$ cat >$HOME/git-shell-commands/help <<\EOF
>>> +#!/bin/sh
>>> +printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"
>>
>> Where in the sshd to git-shell exec chain is $USER variable set for
>> the user?  Just being curious if this is the simplest but one of the
>> more robust ways to get the user's name.
>
> That's a good question.  environment= in an authorized_keys file is
> obsolete, so USER generally represents the actual logged in user.
>
> That means the main way to base behavior on private key (letting one
> system user represent multiple people) is a gitolite-style command=
> wrapper that checks SSH_ORIGINAL_COMMAND.  In that setup, there is no
> reason to forward simple no-args "are you there?" requests to the
> git-shell, so we can forget about it here.
>
> So by the time we get to git-shell, most likely either
>
>  A) this is a generic system user, with a username like "git", and the
>     above example would insult the client with "Hi git!" or "Hi
>     project-x-git!"
>
> or
>
>  B) each person has a separate account on the system, perhaps to help
>     the admin to set filesystem permissions based on users and groups,
>     and the above would address the user by her normal name.

What return value getuid(2) would give us was not something I was
worried about.  Use of git-shell would be pointless if that does not
work to offer isolation between users.

I was wondering who would set the $USER variable based on the uid
assigned to the process during the remote login process and it is a
behaviour we can rely on across platforms.  It appears that when
coming over ssh, it is the ssh daemon that sets USER (and LOGNAME,
HOME, etc.) before running the login shell (session.c::do_child()
that is called from do_exec_pty() or do_exec_no_pty() in openssh).

Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jeff King]

On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote:

> diff --git a/shell.c b/shell.c
> index 84b237fe..3abc2b84 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -63,10 +63,16 @@ static void cd_to_homedir(void)
>  
>  static void run_shell(void)
>  {
> -	int done = 0;
> +	int done = 0, status;
>  	static const char *help_argv[] = { HELP_COMMAND, NULL };
>  	/* Print help if enabled */
> -	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +	status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +	if (!status)
> +		; /* success */
> +	else if (status == -1 && errno == ENOENT)
> +		; /* help disabled */
> +	else
> +		exit(status);

One final comment on this. I believe we convert an exit code of 127 from
the child into ENOENT. So something like:

  #!/bin/sh
  echo >&2 "Sorry, no interactive shells allowed."
  exti 1

would actually go into the "help disabled" code path and accidentally
run an interactive shell. I wondered if this is something that might
happen accidentally (since the old semantics of "help" were that exit
code did not matter), and if there might be security implications to
entering an interactive shell. But I think we are OK for two reasons:

  1. An old script would not be trying to exit with failure and
     expecting to abort the interactive session; that is a new feature
     you are adding. So even if we accidentally exit 127 (because the
     old script relied on a missing command), it is not changing the
     semantics.

  2. Even if we accidentally do enter the interactive prompt, it should
     not be a security issue. It is not like you can then run arbitrary
     commands; unless you have put something else into
     ~/git-shell-commands, the user can only run "help" over and over.

Maybe obvious, but I wanted to note it as part of the review. I think we
need to be extra careful with thinking through git-shell security
implications, since it is a major potential attack surface for many git
setups.

-Peff

[PATCH v3 0/2] shell: allow 'no-interactive-login' command to disable interactive shell

[Jonathan Nieder]

Hi again,

Here's a reroll along the lines described at
http://thread.gmane.org/gmane.comp.version-control.git/216229

As before, this series is meant to give users of basic 'git shell'
setups a chance to imitate some nice behaviors that GitHub and
gitolite offer in more complicated ways.  Thanks for your help on it
so far.

Jonathan Nieder (2):
  shell doc: emphasize purpose and security model
  shell: allow customization of "interactive login disabled" message

 Documentation/git-shell.txt | 86 +++++++++++++++++++++++++++++++++++++--------
 shell.c                     | 13 +++++++
 2 files changed, 84 insertions(+), 15 deletions(-)

[PATCH 1/2] shell doc: emphasize purpose and security model

[Jonathan Nieder]

The original git-shell(1) manpage emphasized that the shell supports
only git transport commands.  As the shell gained features, that
emphasis and focus in the manual has been lost.  Bring it back by
splitting the manpage into a few short sections and fleshing out each:

 - SYNOPSIS, describing how the shell gets used in practice
 - DESCRIPTION, which gives an overview of the purpose and guarantees
   provided by this restricted shell
 - COMMANDS, listing supported commands and restrictions on the
   arguments they accept
 - INTERACTIVE USE, describing the interactive mode

Also add a "see also" section with related reading.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Changes since v2:

 - use "command -v" instead of "which" in synopsis to subtly reinforce
   good habits
 - use <user> instead of hardcoding "git" username in synopsis
 - give up on typesetting "git> " in monospace, since the toolchain
   doesn't seem to like lonely backticks :/
 - clarify change description

The actual text is pretty much the same.

 Documentation/git-shell.txt | 66 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 9b925060..544b21aa 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -9,25 +9,61 @@ git-shell - Restricted login shell for Git-only SSH access
 SYNOPSIS
 --------
 [verse]
-'git shell' [-c <command> <argument>]
+'chsh' -s $(command -v git-shell) <user>
+'git clone' <user>`@localhost:/path/to/repo.git`
+'ssh' <user>`@localhost`
 
 DESCRIPTION
 -----------
 
-A login shell for SSH accounts to provide restricted Git access. When
-'-c' is given, the program executes <command> non-interactively;
-<command> can be one of 'git receive-pack', 'git upload-pack', 'git
-upload-archive', 'cvs server', or a command in COMMAND_DIR. The shell
-is started in interactive mode when no arguments are given; in this
-case, COMMAND_DIR must exist, and any of the executables in it can be
-invoked.
-
-'cvs server' is a special command which executes git-cvsserver.
-
-COMMAND_DIR is the path "$HOME/git-shell-commands". The user must have
-read and execute permissions to the directory in order to execute the
-programs in it. The programs are executed with a cwd of $HOME, and
-<argument> is parsed as a command-line string.
+This is a login shell for SSH accounts to provide restricted Git access.
+It permits execution only of server-side Git commands implementing the
+pull/push functionality, plus custom commands present in a subdirectory
+named `git-shell-commands` in the user's home directory.
+
+COMMANDS
+--------
+
+'git shell' accepts the following commands after the '-c' option:
+
+'git receive-pack <argument>'::
+'git upload-pack <argument>'::
+'git upload-archive <argument>'::
+	Call the corresponding server-side command to support
+	the client's 'git push', 'git fetch', or 'git archive --remote'
+	request.
+'cvs server'::
+	Imitate a CVS server.  See linkgit:git-cvsserver[1].
+
+If a `~/git-shell-commands` directory is present, 'git shell' will
+also handle other, custom commands by running
+"`git-shell-commands/<command> <arguments>`" from the user's home
+directory.
+
+INTERACTIVE USE
+---------------
+
+By default, the commands above can be executed only with the '-c'
+option; the shell is not interactive.
+
+If a `~/git-shell-commands` directory is present, 'git shell'
+can also be run interactively (with no arguments).  If a `help`
+command is present in the `git-shell-commands` directory, it is
+run to provide the user with an overview of allowed actions.  Then a
+"git> " prompt is presented at which one can enter any of the
+commands from the `git-shell-commands` directory, or `exit` to close
+the connection.
+
+Generally this mode is used as an administrative interface to allow
+users to list repositories they have access to, create, delete, or
+rename repositories, or change repository descriptions and
+permissions.
+
+SEE ALSO
+--------
+ssh(1),
+linkgit:git-daemon[1],
+contrib/git-shell-commands/README
 
 GIT
 ---
-- 
1.8.2.rc3

[PATCH 2/2] shell: new no-interactive-login command to print a custom message

[Jonathan Nieder]

If I disable git-shell's interactive mode by removing the
~/git-shell-commands directory, attempts to use 'ssh' in produce a
message intended for the administrator:

	$ ssh git@myserver
	fatal: Interactive git shell is not enabled.
	hint: ~/git-shell-commands should exist and have read and execute access.
	$

That is helpful for the new admin who is wondering "What? Why isn't
the git-shell I just set up working?", but once the site setup is
complete, it would be better to give the user a friendly hint that she
is on the right track, like GitHub does.

	Hi <username>! You've successfully authenticated, but
	GitHub does not provide shell access.

An appropriate greeting might even include more complex dynamic
information, like gitolite's list of repositories the user has access
to.  Add support for a ~/git-shell-commands/no-interactive-login
command that generates an arbitrary greeting.  When the user tries to
log in:

 * If the file ~/git-shell-commands/no-interactive-login exists,
   run no-interactive-login to let the server say what it likes,
   then hang up.

 * Otherwise, if ~/git-shell-commands/ is present, start an
   interactive read-eval-print loop.

 * Otherwise, print the usual configuration hint and hang up.

Reported-by: Ethan Reesor <firelizzard@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Jeff King <peff@peff.net>
---
v2 jammed this functionality into the "help" command, which was kind
of silly.  Hopefully this version's better.

This is not urgent at all.  If it looks like a good change, I'd be
happy to see it be a part of the 1.8.3 cycle.

Thoughts?
Jonathan

 Documentation/git-shell.txt | 20 ++++++++++++++++++++
 shell.c                     | 13 +++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 544b21aa..c35051ba 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -59,6 +59,26 @@ users to list repositories they have access to, create, delete, or
 rename repositories, or change repository descriptions and
 permissions.
 
+If a `no-interactive-login` command exists, then it is run and the
+interactive shell is aborted.
+
+EXAMPLE
+-------
+
+To disable interactive logins, displaying a greeting instead:
++
+----------------
+$ chsh -s /usr/bin/git-shell
+$ mkdir $HOME/git-shell-commands
+$ cat >$HOME/git-shell-commands/no-interactive-login <<\EOF
+#!/bin/sh
+printf '%s\n' "Hi $USER! You've successfully authenticated, but I do not"
+printf '%s\n' "provide interactive shell access."
+exit 128
+EOF
+$ chmod +x $HOME/git-shell-commands/no-interactive-login
+----------------
+
 SEE ALSO
 --------
 ssh(1),
diff --git a/shell.c b/shell.c
index 84b237fe..1429870a 100644
--- a/shell.c
+++ b/shell.c
@@ -6,6 +6,7 @@
 
 #define COMMAND_DIR "git-shell-commands"
 #define HELP_COMMAND COMMAND_DIR "/help"
+#define NOLOGIN_COMMAND COMMAND_DIR "/no-interactive-login"
 
 static int do_generic_cmd(const char *me, char *arg)
 {
@@ -65,6 +66,18 @@ static void run_shell(void)
 {
 	int done = 0;
 	static const char *help_argv[] = { HELP_COMMAND, NULL };
+
+	if (!access(NOLOGIN_COMMAND, F_OK)) {
+		/* Interactive login disabled. */
+		const char *argv[] = { NOLOGIN_COMMAND, NULL };
+		int status;
+
+		status = run_command_v_opt(argv, 0);
+		if (status < 0)
+			exit(127);
+		exit(status);
+	}
+
 	/* Print help if enabled */
 	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
 
-- 
1.8.2.rc3

Re: [PATCH 2/2] shell: new no-interactive-login command to print a custom message

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> If I disable git-shell's interactive mode by removing the
> ~/git-shell-commands directory, attempts to use 'ssh' in produce a
> message intended for the administrator:

Sorry, but -ECANTPARSE.  s/in produce/produces/ perhaps?  Or if you
meant "ssh in" as a verb, then "attempts to ssh in to the service
produces a message".  I dunno.

Patch text looks good, including the documentation.

Thanks.

Re: [PATCH 2/2] shell: new no-interactive-login command to print a custom message

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> If I disable git-shell's interactive mode by removing the
>> ~/git-shell-commands directory, attempts to use 'ssh' in produce a
>> message intended for the administrator:
>
> Sorry, but -ECANTPARSE.  s/in produce/produces/ perhaps?  Or if you
> meant "ssh in" as a verb, then "attempts to ssh in to the service
> produces a message".  I dunno.

Sloppy of me.  Yes, it should say something like this:

	If I disable git-shell's interactive mode by removing the
	~/git-shell-commands directory, attempts to ssh in to the service
	produce a message intended for the administrator:

Re: [PATCH 2/2] shell: new no-interactive-login command to print a custom message

[Ramkumar Ramachandra]

Jonathan Nieder wrote:
>  * If the file ~/git-shell-commands/no-interactive-login exists,
>    run no-interactive-login to let the server say what it likes,
>    then hang up.
>
>  * Otherwise, if ~/git-shell-commands/ is present, start an
>    interactive read-eval-print loop.
>
>  * Otherwise, print the usual configuration hint and hang up.

Excellent.  A way to suppress the ugly warning, and replace it with a
nice message in a non-interactive shell.  You've chosen
"no-interactive-login" as the name of this special file, which is
reasonable.  I'm not too fond of the name "git-shell-commands" in the
first place, but I suspect it's too late to do anything about it now.

> diff --git a/shell.c b/shell.c
> index 84b237fe..1429870a 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -6,6 +6,7 @@
>
>  #define COMMAND_DIR "git-shell-commands"
>  #define HELP_COMMAND COMMAND_DIR "/help"
> +#define NOLOGIN_COMMAND COMMAND_DIR "/no-interactive-login"
>
>  static int do_generic_cmd(const char *me, char *arg)
>  {
> @@ -65,6 +66,18 @@ static void run_shell(void)
>  {
>         int done = 0;
>         static const char *help_argv[] = { HELP_COMMAND, NULL };
> +
> +       if (!access(NOLOGIN_COMMAND, F_OK)) {
> +               /* Interactive login disabled. */

You're just checking for its existence here, not for execute permissions.

> +               const char *argv[] = { NOLOGIN_COMMAND, NULL };
> +               int status;
> +
> +               status = run_command_v_opt(argv, 0);

If "no-interactive-login" doesn't have execute permissions, we'll get
an error from here:

    fatal: cannot exec 'git-shell-commands/no-interactive-login':
Permission denied

Would you like to check that the file has execute permission in
advance to prevent some extra processing (in run_command_v_opt,
start_command and friends) before this message is printed?

Looks good otherwise.

Re: [PATCH 2/2] shell: new no-interactive-login command to print a custom message

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:

>>  * If the file ~/git-shell-commands/no-interactive-login exists,
>>    run no-interactive-login to let the server say what it likes,
>>    then hang up.
[...]
> If "no-interactive-login" doesn't have execute permissions, we'll get
> an error from here:
>
>     fatal: cannot exec 'git-shell-commands/no-interactive-login': Permission denied

Yep.  Intended.

Thanks for looking it over,
Jonathan

Re: [PATCH v3 0/2] shell: allow 'no-interactive-login' command to disable interactive shell

[Jeff King]

On Sat, Mar 09, 2013 at 01:52:37PM -0800, Jonathan Nieder wrote:

> Here's a reroll along the lines described at
> http://thread.gmane.org/gmane.comp.version-control.git/216229
> 
> As before, this series is meant to give users of basic 'git shell'
> setups a chance to imitate some nice behaviors that GitHub and
> gitolite offer in more complicated ways.  Thanks for your help on it
> so far.

Thanks, this version looks good to me.

-Peff