Re: [PATCH] "git shell" won't work, need "git-shell"

Tommi Virtanen [off-list ref] writes:

On Sun, Aug 24, 2008 at 01:36:37PM -0700, Junio C Hamano wrote:

quoted

Of course it would.  Does using /usr/libexec/git-core/git-shell work?

It would, but do you really want people using that?

I do not have particular preference either way.  What people wanted was to
have smaller number of git-foo on $PATH, and especially as "git-shell" is
not something people would be typing from their command line, so I dunno.

On Sun, Aug 24, 2008 at 02:20:20PM -0700, Junio C Hamano wrote:

I do not have particular preference either way.  What people wanted was to
have smaller number of git-foo on $PATH, and especially as "git-shell" is
not something people would be typing from their command line, so I dunno.

That's true, but I kinda think libexec is something only used
*internally*, and you can't claim /etc/passwd to be internal to git..

At the minimum, git-shell(1) should explain that one needs to use the
libexec path.

-- 
:(){ :|:&};:

Hi,

On Mon, 25 Aug 2008, Tommi Virtanen wrote:

On Sun, Aug 24, 2008 at 02:20:20PM -0700, Junio C Hamano wrote:

quoted

I do not have particular preference either way.  What people wanted was to
have smaller number of git-foo on $PATH, and especially as "git-shell" is
not something people would be typing from their command line, so I dunno.

That's true, but I kinda think libexec is something only used
*internally*, and you can't claim /etc/passwd to be internal to git..

At the minimum, git-shell(1) should explain that one needs to use the
libexec path.

Or maybe the real issue can be fixed?  Namely that your configuration does 
not work?  That would involve you actually finding out what's happening, 
though.

Ciao,
Dscho

quoted

At the minimum, git-shell(1) should explain that one needs to use the
libexec path.

Or maybe the real issue can be fixed?  Namely that your configuration does 
not work?  That would involve you actually finding out what's happening, 
though.

He said so:

The shell field in /etc/passwd is *exec*ed, not interpreted via sh -c
or some such. For example, source of Debian's shadow, containing
/bin/login:

libmisc/shell.c:80:	execle (file, arg, (char *) 0, envp);

I also tested this for real, and having a

test:x:1001:1001:,,,:/home/test:/usr/bin/git-shell

line works, and

test:x:1001:1001:,,,:/home/test:/usr/bin/git shell

just makes ssh loop asking for a password, logging

"User test not allowed because shell /usr/bin/git shell does not exist"

So, as far as I understand, as it currently is, "git shell" is utterly
useless for what it was meant to do. Restoring "git-shell" will fix
it.

Rephrasing your question, do you (Dscho) actually know *anyone* who has
a working setup with "/usr/bin/git shell" in /etc/passwd?

Paolo

On Sun, Aug 24, 2008 at 02:20:20PM -0700, Junio C Hamano wrote:

Tommi Virtanen [off-list ref] writes:

quoted

On Sun, Aug 24, 2008 at 01:36:37PM -0700, Junio C Hamano wrote:

quoted

Of course it would.  Does using /usr/libexec/git-core/git-shell work?

It would, but do you really want people using that?

I do not have particular preference either way.  What people wanted was to
have smaller number of git-foo on $PATH, and especially as "git-shell" is
not something people would be typing from their command line, so I dunno.

Can we agree that direct calls of libexec stuff should never be part of
the "official" interface (i.e. not workarounds for deprecated usage)?
Considering that calling the git-shell executable directly is the _only_
sensible way of using this interface, it should follow that it has to be
in /usr/bin, no matter if users type this command or not.

(I'm actually a little confused that you bring up the "typing from their
command line" aspect at all, since that never seemed to be relevant
criterium. People type the commit command all the time, yet we do not
install git-commit. Typing the three git-* commands we do install -
git-receive-pack, git-upload-pack and git-upload-archive - should be on
the other hand pretty rare occasion. About gitk, well, 'git k' would
just look silly, I guess. ;-)

-- 
				Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC.  -- Bill Gates

On Mon, Aug 25, 2008 at 9:08 PM, Petr Baudis [off-list ref] wrote:

Can we agree that direct calls of libexec stuff should never be part of
the "official" interface (i.e. not workarounds for deprecated usage)?

Agreed. It looks somewhat strange to type the libexec path in /etc/passwd.

Considering that calling the git-shell executable directly is the _only_
sensible way of using this interface, it should follow that it has to be
in /usr/bin, no matter if users type this command or not.

Perhaps, /usr/sbin would be a better place, as it is intended only for
system administration binaries.

Dmitry

On Mon, Aug 25, 2008 at 09:20:38PM +0400, Dmitry Potapov wrote:

Perhaps, /usr/sbin would be a better place, as it is intended only for
system administration binaries.

I'd argue that git-shell isn't *exclusively* for root, which is the
criteria for sbin. It's pretty easy to imagine a user setting up their
own ~/.ssh/authorized_keys with a special passphraseless key that
can only do git operations.

-- 
:(){ :|:&};:

On Mon, Aug 25, 2008 at 9:26 PM, Tommi Virtanen [off-list ref] wrote:

On Mon, Aug 25, 2008 at 09:20:38PM +0400, Dmitry Potapov wrote:

quoted

Perhaps, /usr/sbin would be a better place, as it is intended only for
system administration binaries.

I'd argue that git-shell isn't *exclusively* for root, which is the
criteria for sbin.

I don't think that your criteria is correct. mysqld is also not exclusively
for root as you can run it on a non-privilege port, yet, it is placed in
/usr/sbin. Placing in /usr/sbin does not mean that users do not have access
to it, but that those binaries are not run by users from their command line.

Dmitry

Hi,

On Mon, 25 Aug 2008, Paolo Bonzini wrote:

quoted

That would involve you actually finding out what's happening, though.

He said so:

quoted

test:x:1001:1001:,,,:/home/test:/usr/bin/git shell

just makes ssh loop asking for a password, logging

"User test not allowed because shell /usr/bin/git shell does not exist"

Okay, so this means that you cannot pass arguments to the login shell.  
Makes me wonder... I had the impression that bash was called with --login.

Ciao,
Dscho

Hi,

On Mon, 25 Aug 2008, Dmitry Potapov wrote:

On Mon, Aug 25, 2008 at 9:08 PM, Petr Baudis [off-list ref] wrote:

quoted

Can we agree that direct calls of libexec stuff should never be part 
of the "official" interface (i.e. not workarounds for deprecated 
usage)?

Agreed. It looks somewhat strange to type the libexec path in 
/etc/passwd.

FWIW I do not agree.  IMNHO libexec/ is just a way to organize executable 
parts of any software package that are usually not called from the command 
line.  And a login shell qualifies for that.

quoted

Considering that calling the git-shell executable directly is the 
_only_ sensible way of using this interface, it should follow that it 
has to be in /usr/bin, no matter if users type this command or not.

Perhaps, /usr/sbin would be a better place, as it is intended only for 
system administration binaries.

Does it not strike you as odd, then, that "sh" -- by far the most common 
login shell -- does not live in /usr/sbin/?

Ciao,
Dscho

2008/8/25 Johannes Schindelin [off-list ref]:

Hi,

On Mon, 25 Aug 2008, Paolo Bonzini wrote:

quoted

quoted

That would involve you actually finding out what's happening, though.

He said so:

quoted

test:x:1001:1001:,,,:/home/test:/usr/bin/git shell

just makes ssh loop asking for a password, logging

"User test not allowed because shell /usr/bin/git shell does not exist"

Okay, so this means that you cannot pass arguments to the login shell.
Makes me wonder... I had the impression that bash was called with --login.

When you login, a '-' is prepended in argv[0], ie, bash checks if it's called
"-bash". This is documented in man bash, but I couldn't find it in man login
or man agetty, not sure where else it might be written down.

-- 
Mikael Magnusson

Hi,

On Mon, 25 Aug 2008, Mikael Magnusson wrote:

2008/8/25 Johannes Schindelin [off-list ref]:

quoted

On Mon, 25 Aug 2008, Paolo Bonzini wrote:

quoted

quoted

That would involve you actually finding out what's happening, 
though.

He said so:

quoted

test:x:1001:1001:,,,:/home/test:/usr/bin/git shell

just makes ssh loop asking for a password, logging

"User test not allowed because shell /usr/bin/git shell does not 
exist"

Okay, so this means that you cannot pass arguments to the login shell. 
Makes me wonder... I had the impression that bash was called with 
--login.

When you login, a '-' is prepended in argv[0], ie, bash checks if it's 
called "-bash". This is documented in man bash, but I couldn't find it 
in man login or man agetty, not sure where else it might be written 
down.

Thanks!

So does this mean that we could check in git.c if there is a leading "-" 
in argv[0]?  If so, then the builtin git-shell should be called by 
default.

At least I do not expect many instances of git being called with argv[0] 
starting with a "-"... :-)

Ciao,
Dscho

Hi,

On Mon, 25 Aug 2008, Johannes Schindelin wrote:

On Mon, 25 Aug 2008, Mikael Magnusson wrote:

quoted

2008/8/25 Johannes Schindelin [off-list ref]:

quoted

On Mon, 25 Aug 2008, Paolo Bonzini wrote:

quoted

quoted

That would involve you actually finding out what's happening, 
though.

He said so:

quoted

test:x:1001:1001:,,,:/home/test:/usr/bin/git shell

just makes ssh loop asking for a password, logging

"User test not allowed because shell /usr/bin/git shell does not 
exist"

Okay, so this means that you cannot pass arguments to the login shell. 
Makes me wonder... I had the impression that bash was called with 
--login.

When you login, a '-' is prepended in argv[0], ie, bash checks if it's 
called "-bash". This is documented in man bash, but I couldn't find it 
in man login or man agetty, not sure where else it might be written 
down.

Thanks!

So does this mean that we could check in git.c if there is a leading "-" 
in argv[0]?  If so, then the builtin git-shell should be called by 
default.

At least I do not expect many instances of git being called with argv[0] 
starting with a "-"... :-)

Oh, well.  I just tested again, and slapped my head when it did not work, 
remembering that we do _not_ call Git as a login shell.  Instead, we call 
ssh with the "-c" option, which just passes it to the shell.  
Consequently, argv[0] does not get a "-" prepended.

I seem to remember that Hannes had some code to support "-c" as an 
indicator that Git should execute git-shell, but I just might have dreamt 
that, too.

Ciao,
Dscho

On Mon, Aug 25, 2008 at 08:17:12PM +0200, Johannes Schindelin wrote:

Does it not strike you as odd, then, that "sh" -- by far the most common 
login shell -- does not live in /usr/sbin/?

Not at all. "sh" is *often* run directly from the command line. Without
it being in PATH, system(3) and many other things would not work, but
no one tuns git-shell as the real shell to do some job.

Dmitry

On Mon, Aug 25, 2008 at 08:17:12PM +0200, Johannes Schindelin wrote:

Hi,

On Mon, 25 Aug 2008, Dmitry Potapov wrote:

quoted

On Mon, Aug 25, 2008 at 9:08 PM, Petr Baudis [off-list ref] wrote:

quoted

Can we agree that direct calls of libexec stuff should never be part 
of the "official" interface (i.e. not workarounds for deprecated 
usage)?

Agreed. It looks somewhat strange to type the libexec path in 
/etc/passwd.

FWIW I do not agree.  IMNHO libexec/ is just a way to organize executable 
parts of any software package that are usually not called from the command 
line.  And a login shell qualifies for that.

quoted

quoted

Considering that calling the git-shell executable directly is the 
_only_ sensible way of using this interface, it should follow that it 
has to be in /usr/bin, no matter if users type this command or not.

Perhaps, /usr/sbin would be a better place, as it is intended only for 
system administration binaries.

Does it not strike you as odd, then, that "sh" -- by far the most common 
login shell -- does not live in /usr/sbin/?

But nologin is in /usr/sbin.

Mike

It is not a good practice to prefer performance over readability in
something as performance uncritical as finding the trailing slash
of argv[0].

So avoid head-scratching by making the loop user-readable, and not
hyper-performance-optimized.

Signed-off-by: Johannes Schindelin <redacted>
---

	The original version threatened to cause eye-cancer with this
	developer, so that I had to run to the physician real quick.
	That was quite hard, as no ophtalmologist seemed to be in
	office at 10:30pm.  But I found a real good witch, and she
	diagnozed me as healthy.

 git.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/git.c b/git.c
index a647741..2fd74c4 100644
--- a/git.c
+++ b/git.c

@@ -427,9 +427,8 @@ int main(int argc, const char **argv)
 	 * name, and the dirname as the default exec_path
 	 * if we don't have anything better.
 	 */
-	do
-		--slash;
-	while (cmd <= slash && !is_dir_sep(*slash));
+	while (cmd <= slash && !is_dir_sep(*slash))
+		slash--;
 	if (slash < cmd) {
 		cmd = lookup_program_in_path(cmd);
 		for (slash = (char *)cmd + strlen(cmd) - 1;

-- 
1.6.0.211.ga840e.dirty

When a program is called via ssh's '-c' option, the login shell is called 
on the remote computer, with the given arguments.  In the case that Git 
was specified as login shell in /etc/passwd, Git used to complain that it 
does not know the '-c' option and die.

This commit assumes that '-c' indicates that Git was specified as
a login shell, and hands off to git-shell.

Noticed by Tommi Virtanen.

Signed-off-by: Johannes Schindelin <redacted>
---

	This should be pretty uncontroversial, as it turns a former
	error into something useful.

	However, I would not know where in the documentation (in addition
	to git-shell.txt, I guess), this change should be described.

 git.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/git.c b/git.c
index 2fd74c4..b16736a 100644
--- a/git.c
+++ b/git.c

@@ -419,14 +419,23 @@ static void execv_dashed_external(const char **argv)
 int main(int argc, const char **argv)
 {
 	const char *cmd = argv[0] && *argv[0] ? argv[0] : "git-help";
-	char *slash = (char *)cmd + strlen(cmd);
+	char *slash;
 	int done_alias = 0;
 
 	/*
+	 * When Git is called with "-c", it is either an error, or Git
+	 * was specified as a login shell in /etc/passwd.  Assuming the
+	 * latter, we continue with git-shell.
+	 */
+	if (argc > 2 && !strcmp(argv[1], "-c"))
+		cmd = "git-shell";
+
+	/*
 	 * Take the basename of argv[0] as the command
 	 * name, and the dirname as the default exec_path
 	 * if we don't have anything better.
 	 */
+	slash = (char *)cmd + strlen(cmd);
 	while (cmd <= slash && !is_dir_sep(*slash))
 		slash--;
 	if (slash < cmd) {

-- 
1.6.0.211.ga840e.dirty

On Mon, Aug 25, 2008 at 10:52:46PM +0200, Johannes Schindelin wrote:

When a program is called via ssh's '-c' option, the login shell is called 
on the remote computer, with the given arguments.  In the case that Git 
was specified as login shell in /etc/passwd, Git used to complain that it 
does not know the '-c' option and die.

This commit assumes that '-c' indicates that Git was specified as
a login shell, and hands off to git-shell.

Noticed by Tommi Virtanen.

My imagination is insufficient in coming up with an uglier kludge, and
I sincerely hope my name isn't associated with this in any way.

-- 
:(){ :|:&};:

Hi,

On Tue, 26 Aug 2008, Tommi Virtanen wrote:

On Mon, Aug 25, 2008 at 10:52:46PM +0200, Johannes Schindelin wrote:

quoted

When a program is called via ssh's '-c' option, the login shell is called 
on the remote computer, with the given arguments.  In the case that Git 
was specified as login shell in /etc/passwd, Git used to complain that it 
does not know the '-c' option and die.

This commit assumes that '-c' indicates that Git was specified as
a login shell, and hands off to git-shell.

Noticed by Tommi Virtanen.

My imagination is insufficient in coming up with an uglier kludge, and
I sincerely hope my name isn't associated with this in any way.

I have no problems deleting your name from the commit message.  None at 
all.  Even if you actually did notice the issue.

I do disagree with you that it is a kludge though.  I think it makes 
complete sense to add this to Documentation/git.txt in addition to other 
documentation that is lacking from my patch, though:

-- snip --
-c <command>::
	execute <command> in git-shell.
-- snap --

Ciao,
Dscho

On Tue, Aug 26, 2008 at 12:05:21AM +0200, Johannes Schindelin wrote:

I do disagree with you that it is a kludge though.  I think it makes 
complete sense to add this to Documentation/git.txt in addition to other 
documentation that is lacking from my patch, though:

-- snip --
-c <command>::
	execute <command> in git-shell.
-- snap --

All I'm going to say is that that's not the way you build trustable
software. You take a minimal interface and restrict untrusted users to
that, you don't add a feature to the widest possible interface..

-- 
:(){ :|:&};:

Hi,

On Tue, 26 Aug 2008, Tommi Virtanen wrote:

On Tue, Aug 26, 2008 at 12:05:21AM +0200, Johannes Schindelin wrote:

quoted

I do disagree with you that it is a kludge though.  I think it makes 
complete sense to add this to Documentation/git.txt in addition to 
other documentation that is lacking from my patch, though:

-- snip --
-c <command>::
	execute <command> in git-shell.
-- snap --

All I'm going to say is that that's not the way you build trustable 
software. You take a minimal interface and restrict untrusted users to 
that, you don't add a feature to the widest possible interface..

I do not get your point.

But then, I think I start not to care anymore, as I think your reasoning 
is bogus.  "widest possible interface", "trustable software", etc.  Sounds 
pretty buzzy-wordy to me.

Yeah, you would need to audit it.  Maybe you would even have to check for 
"*argv[0] == '-'" and set argv[0] to "git-shell" if so.  But buzz-wording 
just makes me go away and not listen anymore.

Whatever,
Dscho

2008/8/25 Johannes Schindelin [off-list ref]:

Isn't it more likely that this form was chosen to indicate that the
loop body was expected to execute at least once, rather than zero or
more times?

Mike