[PATCH] transport: do not allow to push over git:// protocol

This protocol has never been designed for pushing. Attempts to push
over git:// usually result in

  fatal: The remote end hung up unexpectedly

That message does not really point out the reason. With this patch, we get

  error: this protocol does not support pushing
  error: failed to push some refs to 'git://some-host.com/my/repo'

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 I wanted to advise using remote.*.pushurl too, more friendly. But then I
 had to detect if url comes from command line or config, and I gave up.

 transport.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/transport.c b/transport.c
index fa279d5..b109145 100644
--- a/transport.c
+++ b/transport.c

@@ -933,7 +933,8 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->set_option = NULL;
 		ret->get_refs_list = get_refs_via_connect;
 		ret->fetch = fetch_refs_via_pack;
-		ret->push_refs = git_transport_push;
+		if (prefixcmp(url, "git://"))
+			ret->push_refs = git_transport_push;
 		ret->connect = connect_git;
 		ret->disconnect = disconnect_git;
 		ret->smart_options = &(data->options);

@@ -1075,6 +1076,8 @@ int transport_push(struct transport *transport,
 
 		return ret;
 	}
+	else
+		return error("this protocol does not support pushing");
 	return 1;
 }
 

-- 
1.7.3.1.256.g2539c.dirty

On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyễn Thái Ngọc Duy wrote:

This protocol has never been designed for pushing. Attempts to push
over git:// usually result in

  fatal: The remote end hung up unexpectedly

That message does not really point out the reason. With this patch, we get

  error: this protocol does not support pushing
  error: failed to push some refs to 'git://some-host.com/my/repo'

What about sticking code to return an error to git daemon instead of this?

Here's what happens if I try to push to one of repos on this computer
over git://:

$ git push git://localhost/foobar
fatal: remote error: W access for foobar DENIED to anonymous

So send-pack can deal with ERR packet (and yes, that error message
is really from Gitolite).

Aside: git archive seemingly can't deal with ERR packets. And worse
yet, it doesn't even print what it received, resulting this:

$ git archive --remote=git://localhost/foobar HEAD
fatal: git archive: protocol error


-Ilari 

2011/10/1 Ilari Liusvaara [off-list ref]:

What about sticking code to return an error to git daemon instead of this?

Here's what happens if I try to push to one of repos on this computer
over git://:

$ git push git://localhost/foobar
fatal: remote error: W access for foobar DENIED to anonymous

So send-pack can deal with ERR packet (and yes, that error message
is really from Gitolite).

I'm dealing with git.gnome.org and not sure what's the server behind.
I had a look at git-daemon and it does allow push, but disabled by
default. So yes, maybe updating git-daemon is better.

Aside: git archive seemingly can't deal with ERR packets. And worse
yet, it doesn't even print what it received, resulting this:

$ git archive --remote=git://localhost/foobar HEAD
fatal: git archive: protocol error

Yes, builtin/archive.c seems only recognize either ACK or NACK.
pack-protocol.txt does not mention about ERR either, which seems to be
introduced in a807328 (connect.c: add a way for git-daemon to pass an
error back to client).
-- 
Duy

Ilari Liusvaara wrote:

What about sticking code to return an error to git daemon instead of this?

The code has even been written:
http://thread.gmane.org/gmane.comp.version-control.git/145456/focus=145573

Testing and other improvements would be very welcome.

On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:

This protocol has never been designed for pushing. Attempts to push
over git:// usually result in

  fatal: The remote end hung up unexpectedly

That message does not really point out the reason. With this patch, we get

  error: this protocol does not support pushing
  error: failed to push some refs to 'git://some-host.com/my/repo'

I thought pushing over git:// _is_ supported. It's just that most
servers don't have it turned on, for the obvious lack-of-authentication
reasons.

See 4b3b1e1 (git-push through git protocol, 2007-01-21), and the
discussion here:

  http://thread.gmane.org/gmane.comp.version-control.git/37325

Your patch shuts it off at the client level, so even with it turned on
for the server, the client can never get to it.

I still think push-over-git:// is a bit insane, and especially now with
smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
seeing it deprecated. But just shutting it off without a deprecation
period seems unnecessarily harsh.

The real problem here seems to be that instead of communicating "no, we
don't support that", git-daemon just hangs up. It would be a much nicer
fix if we could change that. I'm not sure it's possible, though. There's
not much room in the beginning of the room to make that communication in
a way that's backwards compatible.

-Peff

Am 10/3/2011 9:42, schrieb Jeff King:

I still think push-over-git:// is a bit insane, and especially now with
smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
seeing it deprecated.

You must be kidding ;) It is so much easier to type

  git daemon --export-all --enable=receive-pack

for a one-shot, temporary git connection compared to setting up a
smart-http, ssh, or even a rsh server.

-- Hannes

2011/10/1 Jonathan Nieder [off-list ref]:

Ilari Liusvaara wrote:

quoted

What about sticking code to return an error to git daemon instead of this?

The code has even been written:
http://thread.gmane.org/gmane.comp.version-control.git/145456/focus=145573

Testing and other improvements would be very welcome.

Tests aside, are there any problems with the patch? I don't see any
followup discussions. Personally I don't see much value in adding the
description though.
-- 
Duy

On Mon, Oct 03, 2011 at 10:44:23AM +0200, Johannes Sixt wrote:

Am 10/3/2011 9:42, schrieb Jeff King:

quoted

I still think push-over-git:// is a bit insane, and especially now with
smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
seeing it deprecated.

You must be kidding ;) It is so much easier to type

  git daemon --export-all --enable=receive-pack

for a one-shot, temporary git connection compared to setting up a
smart-http, ssh, or even a rsh server.

Ah, yeah, I didn't think about one-shot invocations like that (I think
the original motivation was somebody actually running it all the time).

So yeah, that makes it even worse for the client to start refusing this
without even contacting the server. I forgot that we added the "ERR"
response way back in a807328 (connect.c: add a way for git-daemon to
pass an error back to client, 2008-11-01).

GitHub uses it to make nice messages:

  $ git push origin
  fatal: remote error:
    You can't push to git://github.com/gitster/git.git
    Use git@github.com:gitster/git.git

We should maybe do something like the patch below:

diff --git a/daemon.c b/daemon.c
index 4c8346d..c1fa55f 100644
--- a/daemon.c
+++ b/daemon.c

@@ -255,6 +255,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	loginfo("Request %s for '%s'", service->name, dir);
 
 	if (!enabled && !service->overridable) {
+		packet_write(1, "ERR %s: service not enabled", service->name);
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
 		return -1;

@@ -288,6 +289,8 @@ static int run_service(char *dir, struct daemon_service *service)
 			enabled = service_enabled;
 	}
 	if (!enabled) {
+		packet_write(1, "ERR %s: service not enabled for '%s'",
+		       service->name, path);
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;

but:

  1. There is some information leakage there. In particular, one can
     tell the difference now between "repo does not exist" and
     "receive-pack is not turned on". Personally, I think the tradeoff
     to have actual error messages is worth it. HTTP has had real error
     codes for decades, and I don't think anybody is too up-in-arms that
     I can probe which pages are 404, and which are 401.

  2. It probably makes sense to have a more human-friendly error
     message.

  3. It may be worth adding error messages for lots of other conditions
     (e.g., no such repo). Assuming we accept the information leakage
     for (1).

-Peff

2011/10/3 Jeff King [off-list ref]:

So yeah, that makes it even worse for the client to start refusing this
without even contacting the server. I forgot that we added the "ERR"
response way back in a807328 (connect.c: add a way for git-daemon to
pass an error back to client, 2008-11-01).

GitHub uses it to make nice messages:

 $ git push origin
 fatal: remote error:
   You can't push to git://github.com/gitster/git.git
   Use git@github.com:gitster/git.git

We should maybe do something like the patch below:

Jonathan also mentions another patch

http://article.gmane.org/gmane.comp.version-control.git/182536

but:

 1. There is some information leakage there. In particular, one can
    tell the difference now between "repo does not exist" and
    "receive-pack is not turned on". Personally, I think the tradeoff
    to have actual error messages is worth it. HTTP has had real error
    codes for decades, and I don't think anybody is too up-in-arms that
    I can probe which pages are 404, and which are 401.

To me, just "<service>: access denied" is enough. Not particularly
friendly but should be a good enough clue.
-- 
Duy

On Mon, Oct 03, 2011 at 08:44:22PM +1100, Nguyen Thai Ngoc Duy wrote:

quoted

GitHub uses it to make nice messages:

 $ git push origin
 fatal: remote error:
   You can't push to git://github.com/gitster/git.git
   Use git@github.com:gitster/git.git

We should maybe do something like the patch below:

Jonathan also mentions another patch

http://article.gmane.org/gmane.comp.version-control.git/182536

Yeah, I was just reading that. Sorry, I should have read the rest of the
thread more carefully. :)

quoted

 1. There is some information leakage there. In particular, one can
    tell the difference now between "repo does not exist" and
    "receive-pack is not turned on". Personally, I think the tradeoff
    to have actual error messages is worth it. HTTP has had real error
    codes for decades, and I don't think anybody is too up-in-arms that
    I can probe which pages are 404, and which are 401.

To me, just "<service>: access denied" is enough. Not particularly
friendly but should be a good enough clue.

Yeah, maybe. Certainly it's better than "the remote end hung up
unexpectedly".

However, the leakage is still there. You would get "the remote hung up"
for no-such-repo, and "access denied" for this. Or were you just
proposing that _all_ errors give "access denied". Certainly it's better
than just hanging up, too, and there is no leakage there.

It might be nice to default to that, and let sites easily enable
friendlier messages, though.

-Peff

Johannes Sixt [off-list ref] writes:

Am 10/3/2011 9:42, schrieb Jeff King:

quoted

I still think push-over-git:// is a bit insane, and especially now with
smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind
seeing it deprecated.

You must be kidding ;) It is so much easier to type

  git daemon --export-all --enable=receive-pack

for a one-shot, temporary git connection compared to setting up a
smart-http, ssh, or even a rsh server.

I wonder if that is the case... but 48% responders of "Git User's
Survey 2011" (3424 out of 7100 responders who answered queston 
"23) How do you publish/propagate your changes?") answered that they
use push via git protocol.

See https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL

-- 
Jakub Narębski

On Mon, Oct 3, 2011 at 8:47 PM, Jeff King [off-list ref] wrote:

quoted

To me, just "<service>: access denied" is enough. Not particularly
friendly but should be a good enough clue.

Yeah, maybe. Certainly it's better than "the remote end hung up
unexpectedly".

However, the leakage is still there. You would get "the remote hung up"
for no-such-repo, and "access denied" for this. Or were you just
proposing that _all_ errors give "access denied". Certainly it's better
than just hanging up, too, and there is no leakage there.

All of them. At least it's good to know my request has reached (and
rejected by) the server, not dropped on the floor by some random
firewall along the line.

It might be nice to default to that, and let sites easily enable
friendlier messages, though.

I'm thinking of passing "verbose" option back to server to get more
helpful messages, the option would be turned off by default. It's up
to admin to decide (would be actually helpful during deployment test,
for example). Or is it possible already?
-- 
Duy

On Mon, Oct 03, 2011 at 02:49:15AM -0700, Jakub Narebski wrote:

I wonder if that is the case... but 48% responders of "Git User's
Survey 2011" (3424 out of 7100 responders who answered queston 
"23) How do you publish/propagate your changes?") answered that they
use push via git protocol.

See https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL

I refuse to believe that 48% of people are using git:// to push. Surely
they are interpreting that response to overlap with "git over ssh" and
"git over http".

-Peff

On Mon, Oct 03, 2011 at 03:42:51AM -0400, Jeff King wrote:

On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:

The real problem here seems to be that instead of communicating "no, we
don't support that", git-daemon just hangs up. It would be a much nicer
fix if we could change that. I'm not sure it's possible, though. There's
not much room in the beginning of the room to make that communication in
a way that's backwards compatible.

Oh, sure it is possible (except for remote snapshot):

$ /usr/bin/git fetch git://localhost/foobar
fatal: remote error: R access for foobar DENIED to anonymous
$ /usr/bin/git push git://localhost/foobar
fatal: remote error: W access for foobar DENIED to anonymous
$ /usr/bin/git archive --remote=git://localhost/foobar HEAD
fatal: git archive: protocol error
$ /usr/bin/git --version
git version 1.7.6.3

Supported for fetch and push since 1.6.1-rc1 (And 1.6.1 was over
2.5 years ago). Oh, and even before that, but with slightly more
ugly error message.

Oh, and adding interpretation of ERR packets to git archive is easy
(and I even happen to have git:// server that can send those to
test against):

$ git archive --remote=git://localhost/foobar HEAD
fatal: remote error: R access for foobar DENIED to anonymous

(I also tested that remote snapshotting of repository that should be
readable succeeds, it does).

--- >8 ----

From ce3a402e4fa72cf603f92801d6f021ff89d3ac35 Mon Sep 17 00:00:00 2001
From: Ilari Liusvaara <redacted>
Date: Mon, 3 Oct 2011 13:55:37 +0300
Subject: [PATCH] Support ERR in remote archive like in fetch/push

Make ERR as first packet of remote snapshot reply work like it does in
fetch/push. Lets servers decline remote snapshot with message the same
way as declining fetch/push with a message.

Signed-off-by: Ilari Liusvaara <redacted>
---
 builtin/archive.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index 883c009..931956d 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c

@@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,
 	if (strcmp(buf, "ACK")) {
 		if (len > 5 && !prefixcmp(buf, "NACK "))
 			die(_("git archive: NACK %s"), buf + 5);
+		if (len > 4 && !prefixcmp(buf, "ERR "))
+			die(_("remote error: %s"), buf + 4);
 		die(_("git archive: protocol error"));
 	}
 

-- 
1.7.7.3.g2791de.dirty


-Ilari

Nguyen Thai Ngoc Duy wrote:

To me, just "<service>: access denied" is enough. Not particularly
friendly but should be a good enough clue.

Yes, I think you're right.  It also has the benefit of being easily
parsable, so some day the client might learn to give a friendly
message in the operator's chosen language.

Ilari Liusvaara wrote:

Oh, and adding interpretation of ERR packets to git archive is easy
(and I even happen to have git:// server that can send those to
test against):

$ git archive --remote=git://localhost/foobar HEAD
fatal: remote error: R access for foobar DENIED to anonymous

(I also tested that remote snapshotting of repository that should be
readable succeeds, it does).

Sounds like a good idea to me.  Let's see what René thinks; also
changing the subject line to attract other reviewers.

Am 03.10.2011 13:26, schrieb Jonathan Nieder:

Ilari Liusvaara wrote:

quoted

Oh, and adding interpretation of ERR packets to git archive is easy
(and I even happen to have git:// server that can send those to
test against):

$ git archive --remote=git://localhost/foobar HEAD
fatal: remote error: R access for foobar DENIED to anonymous

(I also tested that remote snapshotting of repository that should be
readable succeeds, it does).

Sounds like a good idea to me.  Let's see what René thinks; also
changing the subject line to attract other reviewers.

Looks good to me, but I'm not too familiar with the remote protocol.

quoted

--- >8 ----

From: Ilari Liusvaara <redacted>
Date: Mon, 3 Oct 2011 13:55:37 +0300
Subject: [PATCH] Support ERR in remote archive like in fetch/push

Make ERR as first packet of remote snapshot reply work like it does in
fetch/push. Lets servers decline remote snapshot with message the same
way as declining fetch/push with a message.

Signed-off-by: Ilari Liusvaara <redacted>
---
 builtin/archive.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index 883c009..931956d 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c

@@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,
 	if (strcmp(buf, "ACK")) {
 		if (len > 5 && !prefixcmp(buf, "NACK "))
 			die(_("git archive: NACK %s"), buf + 5);
+		if (len > 4 && !prefixcmp(buf, "ERR "))
+			die(_("remote error: %s"), buf + 4);
 		die(_("git archive: protocol error"));
 	}
 

-- 
1.7.7.3.g2791de.dirty

On Mon, Oct 03, 2011 at 02:01:59PM +0300, Ilari Liusvaara wrote:

Yeah, maybe with this patch also?

-- 8< --
Subject: [PATCH] pack-protocol: document "ERR" line

Since a807328 (connect.c: add a way for git-daemon to pass an error
back to client), git client recognizes "ERR" line and prints a
friendly message to user if an error happens at server side.

Document this.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Documentation/technical/pack-protocol.txt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index a7004c6..546980c 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt

@@ -60,6 +60,13 @@ process on the server side over the Git protocol is this:
      "0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
      nc -v example.com 9418
 
+If the server refuses the request for some reasons, it could abort
+gracefully with an error message.
+
+----
+  error-line     =  PKT-LINE("ERR" SP explanation-text)
+----
+
 
 SSH Transport
 -------------

-- 
-- 8< --

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Note that if a service fails, then "access denied" is printed too.
 Not sure if it's a good thing, the service in question may have
 responded to user already. On the other hand, this catches faults
 from start_command() in run_service_command().

 daemon.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4c8346d..6552ca7 100644
--- a/daemon.c
+++ b/daemon.c

@@ -562,7 +562,10 @@ static int execute(void)
 			 * Note: The directory here is probably context sensitive,
 			 * and might depend on the actual service being performed.
 			 */
-			return run_service(line + namelen + 5, s);
+			if (!run_service(line + namelen + 5, s))
+				return 0;
+			packet_write(1, "ERR %s: access denied", line + namelen + 5);
+			return -1;
 		}
 	}
 

-- 
1.7.3.1.256.g2539c.dirty

Nguyễn Thái Ngọc Duy wrote:

At first I liked the simplification relative to the patch I sent.
This means the error message is shown when

 1. the service is not enabled at all
 2. path not allowed (for example because it doesn't exist, because
    of permission problems, or because it is blacklisted)
 3. the repository is not exported
 4. the service is not enabled for $path
 5. the service command exited with nonzero status

Unfortunately I think that last case (#5) would be confusing and would
break protocol, especially when the command dies at an inconvenient
moment.  Better for the service command to send an appropriate error
indicator and to just hang up when it fails to do so.