git pull aborts in 50% of cases

git pull aborts in 50% of cases

[Alexey Dobriyan]

$ git pull
Already up-to-date.
$ git pull
Already up-to-date.
$ git pull
Already up-to-date.
$ git pull
fatal: unexpected EOF
Fetch failure: git://git.kernel.org/pub/scm/git/git.git
$ git pull
fatal: unexpected EOF
Fetch failure: git://git.kernel.org/pub/scm/git/git.git
$ git pull
Already up-to-date.
$ git pull
fatal: unexpected EOF
Fetch failure: git://git.kernel.org/pub/scm/git/git.git
$ git pull
fatal: unexpected EOF
Fetch failure: git://git.kernel.org/pub/scm/git/git.git

Ditto for "git fetch --tags".

Can somebody explain what's going on?

Re: git pull aborts in 50% of cases

[H. Peter Anvin]

Alexey Dobriyan wrote:
> $ git pull
> Already up-to-date.
> $ git pull
> Already up-to-date.
> $ git pull
> Already up-to-date.
> $ git pull
> fatal: unexpected EOF
> Fetch failure: git://git.kernel.org/pub/scm/git/git.git
> $ git pull
> fatal: unexpected EOF
> Fetch failure: git://git.kernel.org/pub/scm/git/git.git
> $ git pull
> Already up-to-date.
> $ git pull
> fatal: unexpected EOF
> Fetch failure: git://git.kernel.org/pub/scm/git/git.git
> $ git pull
> fatal: unexpected EOF
> Fetch failure: git://git.kernel.org/pub/scm/git/git.git
> 
> Ditto for "git fetch --tags".
> 
> Can somebody explain what's going on?
> 

Do you know which IP address is aborting?  There are two servers behind 
git.kernel.org.

	-hpa

Re: git pull aborts in 50% of cases

[Alexey Dobriyan]

On Fri, Dec 02, 2005 at 10:58:43AM -0800, H. Peter Anvin wrote:
> Alexey Dobriyan wrote:
> >$ git pull
> >Already up-to-date.
> >$ git pull
> >Already up-to-date.
> >$ git pull
> >Already up-to-date.
> >$ git pull
> >fatal: unexpected EOF
> >Fetch failure: git://git.kernel.org/pub/scm/git/git.git
> >$ git pull
> >fatal: unexpected EOF
> >Fetch failure: git://git.kernel.org/pub/scm/git/git.git
> >$ git pull
> >Already up-to-date.
> >$ git pull
> >fatal: unexpected EOF
> >Fetch failure: git://git.kernel.org/pub/scm/git/git.git
> >$ git pull
> >fatal: unexpected EOF
> >Fetch failure: git://git.kernel.org/pub/scm/git/git.git
> >
> >Ditto for "git fetch --tags".
> >
> >Can somebody explain what's going on?
> >
>
> Do you know which IP address is aborting?  There are two servers behind
> git.kernel.org.

Heisenbug :-\. I'll send IP next time.

~/linux/linux-linus $ while true; do git pull; done
Already up-to-date.
Already up-to-date.
Already up-to-date.
Already up-to-date.
Already up-to-date.
Already up-to-date.
Already up-to-date.
Already up-to-date.
Already up-to-date.
Already up-to-date.
Already up-to-date.

Re: git pull aborts in 50% of cases

[H. Peter Anvin]

Alexey Dobriyan wrote:
> 
> Heisenbug :-\. I'll send IP next time.
> 

Actually, it turns out the two servers were running different versions; 
one 0.99.9j and one 0.99.9k.  They're both running 0.99.9j now.

0.99.9k is clearly bad.

	-hpa

Re: git pull aborts in 50% of cases

[Junio C Hamano]

H. Peter Anvin <hpa <at> zytor.com> writes:

> Actually, it turns out the two servers were running different versions; 
> one 0.99.9j and one 0.99.9k.  They're both running 0.99.9j now.
> 
> 0.99.9k is clearly bad.

This is troublesome, since I do not think it is hitting the one that was
causing trouble for the snapshotting procedure, and I cannot seem to reproduce
it with random set of flags and parameters myself.

I know you run it with --inetd, but what other parameters do you run it with,
and in what kind of environment?  Specifically:

 - do you use whitelist, like "git-daemon --inetd --export-all /pub/scm"?
 - if so, are any symlinks involved in /pub area?
 - if so, does adding real path like /mnt/real/pub/scm for /pub/scm help?
 - do you run with --strict-paths?

Re: git pull aborts in 50% of cases

[Johannes Schindelin]

Hi,

On Fri, 2 Dec 2005, H. Peter Anvin wrote:

> Alexey Dobriyan wrote:
> > 
> > Heisenbug :-\. I'll send IP next time.
> > 
> 
> Actually, it turns out the two servers were running different versions; one
> 0.99.9j and one 0.99.9k.  They're both running 0.99.9j now.
> 
> 0.99.9k is clearly bad.

Huh? It could be slower, and it could therefore hit the maximum client 
count faster, but it should not be bad.

All changes to pull were done in a manner so as to be backward compatible. 
In both ways.

Hth,
Dscho

Re: git pull aborts in 50% of cases

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> 0.99.9k is clearly bad.
>
> Huh? It could be slower, and it could therefore hit the maximum client 
> count faster, but it should not be bad.
>
> All changes to pull were done in a manner so as to be backward compatible. 
> In both ways.

I do not think the fetch-pack common computation changes is
involved in this problem at all.

What is suspect is the repository validity check code,
specifically (quoting from diff between 0.99.9j and 0.99.9k
daemon.c::path_ok() function):

+               /* The validation is done on the paths after enter_repo
+                * canonicalization, so whitelist should be written in
+                * terms of real pathnames (i.e. after ~user is expanded
+                * and symlinks resolved).
+                */

I suspect (but have not heard back from HPA to confirm) that
kernel.org runs git-daemon with /pub/scm as the whitelist, but
there is a symbolic link (or bind mount?) involved, and the real
path checked based on getcwd() return value is somewhere else.

Re: git pull aborts in 50% of cases

[H. Peter Anvin]

Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> 
>>>0.99.9k is clearly bad.
>>
>>Huh? It could be slower, and it could therefore hit the maximum client 
>>count faster, but it should not be bad.
>>
>>All changes to pull were done in a manner so as to be backward compatible. 
>>In both ways.
> 
> 
> I do not think the fetch-pack common computation changes is
> involved in this problem at all.
> 
> What is suspect is the repository validity check code,
> specifically (quoting from diff between 0.99.9j and 0.99.9k
> daemon.c::path_ok() function):
> 
> +               /* The validation is done on the paths after enter_repo
> +                * canonicalization, so whitelist should be written in
> +                * terms of real pathnames (i.e. after ~user is expanded
> +                * and symlinks resolved).
> +                */
> 
> I suspect (but have not heard back from HPA to confirm) that
> kernel.org runs git-daemon with /pub/scm as the whitelist, but
> there is a symbolic link (or bind mount?) involved, and the real
> path checked based on getcwd() return value is somewhere else.

/pub is a symbolic link.  We shouldn't rely on getcwd() for this kind of 
stuff; it's bad for a whole bunch of reasons.

	-hpa

Re: git pull aborts in 50% of cases

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> /pub is a symbolic link.  We shouldn't rely on getcwd() for this kind of 
> stuff; it's bad for a whole bunch of reasons.

Well, if I recall correctly it was done this way because
DWIMmery needs to be done before the validation.

Anyway, here is a rewrite of tonight (I resurrected your "belts
and suspenders paranoia patch" for this).  Would appreciate it
if people can try this out (in the proposed updates branch).

The rules (in 0.99.9k and "master" so far) have been that if you
have symlinked public, whitelist should say what the canonical
names of them are (and the way canonical names are obtained were
getcwd()).  The rule of this patch is different: whitelist says
what the remote requestor can ask for.  So if your /pub is a
symlink to /mnt/mnt1/pub, you do not have to say /mnt/mnt1/pub
to export it.  Instead you whitelist /pub (or /pub/scm).  Also
if your ~bob is /home1/bob and ~alice is /home2/alice, you do
not say "/home1 /home2" -- instead, you say "~alice ~bob".

-- >8 --
Subject: [PATCH] daemon.c and path.enter_repo(): revamp path validation.

The whitelist of git-daemon is checked against return value from
enter_repo(), and enter_repo() used to return the value obtained
from getcwd() to avoid directory aliasing issues as discussed
earier (mid October 2005).

Unfortunately, it did not go well as we hoped.

For example, /pub on a kernel.org public machine is a symlink to
its real mountpoint, and it is understandable that the
administrator does not want to adjust the whitelist every time
/pub needs to point at a different partition for storage
allcation or whatever reasons.  Being able to keep using
/pub/scm as the whitelist is a desirable property.

So this version of enter_repo() reports what it used to chdir()
and validate, but does not use getcwd() to canonicalize the
directory name.  When it sees a user relative path ~user/path,
it internally resolves it to try chdir() there, but it still
reports ~user/path (possibly after appending .git if allowed to
do so, in which case it would report ~user/path.git).

What this means is that if a whitelist wants to allow a user
relative path, it needs to say "~" (for all users) or list user
home directories like "~alice" "~bob".  And no, you cannot say
/home if the advertised way to access user home directories are
~alice,~bob, etc.  The whole point of this is to avoid
unnecessary aliasing issues.

Anyway, because of this, daemon needs to do a bit more work to
guard itself.  Namely, it needs to make sure that the accessor
does not try to exploit its leading path match rule by inserting
/../ in the middle or hanging /.. at the end.  I resurrected the
belts and suspender paranoia code HPA did for this purpose.

This check cannot be done in the enter_repo() unconditionally,
because there are valid callers of enter_repo() that want to
honor /../; authorized users coming over ssh to run send-pack
and fetch-pack should be allowed to do so.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 daemon.c |   64 ++++++++++++++++++++++++--
 path.c   |  153 ++++++++++++++++++++++++++++++++++++++++----------------------
 2 files changed, 159 insertions(+), 58 deletions(-)

018c8d740d18b7fe7d9d5a24fc1d008e29d70bc4
diff --git a/daemon.c b/daemon.c
index 91b9656..539f6e8 100644
--- a/daemon.c
+++ b/daemon.c
@@ -82,9 +82,63 @@ static void loginfo(const char *err, ...
 	va_end(params);
 }
 
+static int avoid_alias(char *p)
+{
+	int sl, ndot;
+
+	/* 
+	 * This resurrects the belts and suspenders paranoia check by HPA
+	 * done in <435560F7.4080006@zytor.com> thread, now enter_repo()
+	 * does not do getcwd() based path canonicalizations.
+	 *
+	 * sl becomes true immediately after seeing '/' and continues to
+	 * be true as long as dots continue after that without intervening
+	 * non-dot character.
+	 */
+	if (!p || (*p != '/' && *p != '~'))
+		return -1;
+	sl = 1; ndot = 0;
+	p++;
+
+	while (1) {
+		char ch = *p++;
+		if (sl) {
+			if (ch == '.')
+				ndot++;
+			else if (ch == '/') {
+				if (ndot < 3)
+					/* reject //, /./ and /../ */
+					return -1;
+				ndot = 0;
+			}
+			else if (ch == 0) {
+				if (0 < ndot && ndot < 3)
+					/* reject /.$ and /..$ */
+					return -1;
+				return 0;
+			}
+			else
+				sl = ndot = 0;
+		}
+		else if (ch == 0)
+			return 0;
+		else if (ch == '/') {
+			sl = 1;
+			ndot = 0;
+		}
+	}
+}
+
 static char *path_ok(char *dir)
 {
-	char *path = enter_repo(dir, strict_paths);
+	char *path;
+
+	if (avoid_alias(dir)) {
+		logerror("'%s': aliased", dir);
+		return NULL;
+	}
+
+	path = enter_repo(dir, strict_paths);
 
 	if (!path) {
 		logerror("'%s': unable to chdir or not a git archive", dir);
@@ -96,9 +150,11 @@ static char *path_ok(char *dir)
 		int pathlen = strlen(path);
 
 		/* The validation is done on the paths after enter_repo
-		 * canonicalization, so whitelist should be written in
-		 * terms of real pathnames (i.e. after ~user is expanded
-		 * and symlinks resolved).
+		 * appends optional {.git,.git/.git} and friends, but 
+		 * it does not use getcwd().  So if your /pub is
+		 * a symlink to /mnt/pub, you can whitelist /pub and
+		 * do not have to say /mnt/pub.
+		 * Do not say /pub/.
 		 */
 		for ( pp = ok_paths ; *pp ; pp++ ) {
 			int len = strlen(*pp);
diff --git a/path.c b/path.c
index 2c077c0..334b2bd 100644
--- a/path.c
+++ b/path.c
@@ -131,76 +131,121 @@ int validate_symref(const char *path)
 	return -1;
 }
 
-static char *current_dir(void)
+static char *user_path(char *buf, char *path, int sz)
 {
-	return getcwd(pathname, sizeof(pathname));
-}
-
-static int user_chdir(char *path)
-{
-	char *dir = path;
+	struct passwd *pw;
+	char *slash;
+	int len, baselen;
 
-	if(*dir == '~') {		/* user-relative path */
-		struct passwd *pw;
-		char *slash = strchr(dir, '/');
-
-		dir++;
-		/* '~/' and '~' (no slash) means users own home-dir */
-		if(!*dir || *dir == '/')
-			pw = getpwuid(getuid());
-		else {
-			if (slash) {
-				*slash = '\0';
-				pw = getpwnam(dir);
-				*slash = '/';
-			}
-			else
-				pw = getpwnam(dir);
+	if (!path || path[0] != '~')
+		return NULL;
+	path++;
+	slash = strchr(path, '/');
+	if (path[0] == '/' || !path[0]) {
+		pw = getpwuid(getuid());
+	}
+	else {
+		if (slash) {
+			*slash = 0;
+			pw = getpwnam(path);
+			*slash = '/';
 		}
-
-		/* make sure we got something back that we can chdir() to */
-		if(!pw || chdir(pw->pw_dir) < 0)
-			return -1;
-
-		if(!slash || !slash[1]) /* no path following username */
-			return 0;
-
-		dir = slash + 1;
+		else
+			pw = getpwnam(path);
 	}
-
-	/* ~foo/path/to/repo is now path/to/repo and we're in foo's homedir */
-	if(chdir(dir) < 0)
-		return -1;
-
-	return 0;
+	if (!pw || !pw->pw_dir || sz <= strlen(pw->pw_dir))
+		return NULL;
+	baselen = strlen(pw->pw_dir);
+	memcpy(buf, pw->pw_dir, baselen);
+	while ((1 < baselen) && (buf[baselen-1] == '/')) {
+		buf[baselen-1] = 0;
+		baselen--;
+	}
+	if (slash && slash[1]) {
+		len = strlen(slash);
+		if (sz <= baselen + len)
+			return NULL;
+		memcpy(buf + baselen, slash, len + 1);
+	}
+	return buf;
 }
 
+/*
+ * First, one directory to try is determined by the following algorithm.
+ *
+ * (0) If "strict" is given, the path is used as given and no DWIM is
+ *     done. Otherwise:
+ * (1) "~/path" to mean path under the running user's home directory;
+ * (2) "~user/path" to mean path under named user's home directory;
+ * (3) "relative/path" to mean cwd relative directory; or
+ * (4) "/absolute/path" to mean absolute directory.
+ *
+ * Unless "strict" is given, we try access() for existence of "%s.git/.git",
+ * "%s/.git", "%s.git", "%s" in this order.  The first one that exists is
+ * what we try.
+ *
+ * Second, we try chdir() to that.  Upon failure, we return NULL.
+ *
+ * Then, we try if the current directory is a valid git repository.
+ * Upon failure, we return NULL.
+ *
+ * If all goes well, we return the directory we used to chdir() (but
+ * before ~user is expanded), avoiding getcwd() resolving symbolic
+ * links.  User relative paths are also returned as they are given,
+ * except DWIM suffixing.
+ */
 char *enter_repo(char *path, int strict)
 {
-	if(!path)
+	static char used_path[PATH_MAX];
+	static char validated_path[PATH_MAX];
+
+	if (!path)
 		return NULL;
 
-	if (strict) {
-		if (chdir(path) < 0)
+	if (!strict) {
+		static const char *suffix[] = {
+			".git/.git", "/.git", ".git", "", NULL,
+		};
+		int len = strlen(path);
+		int i;
+		while ((1 < len) && (path[len-1] == '/')) {
+			path[len-1] = 0;
+			len--;
+		}
+		if (PATH_MAX <= len)
 			return NULL;
-	}
-	else {
-		if (!*path)
-			; /* happy -- no chdir */
-		else if (!user_chdir(path))
-			; /* happy -- as given */
-		else if (!user_chdir(mkpath("%s.git", path)))
-			; /* happy -- uemacs --> uemacs.git */
-		else
+		if (path[0] == '~') {
+			if (!user_path(used_path, path, PATH_MAX))
+				return NULL;
+			strcpy(validated_path, path);
+			path = used_path;
+		}
+		else if (PATH_MAX - 10 < len)
 			return NULL;
-		(void)chdir(".git");
+		else {
+			path = strcpy(used_path, path);
+			strcpy(validated_path, path);
+		}
+		len = strlen(path);
+		for (i = 0; suffix[i]; i++) {
+			strcpy(path + len, suffix[i]);
+			if (!access(path, F_OK)) {
+				strcat(validated_path, suffix[i]);
+				break;
+			}
+		}
+		if (!suffix[i] || chdir(path))
+			return NULL;
+		path = validated_path;
 	}
+	else if (chdir(path))
+		return NULL;
 
-	if(access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
-	   validate_symref("HEAD") == 0) {
+	if (access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
+	    validate_symref("HEAD") == 0) {
 		putenv("GIT_DIR=.");
 		check_repository_format();
-		return current_dir();
+		return path;
 	}
 
 	return NULL;
-- 
0.99.9.GIT

Re: git pull aborts in 50% of cases

[H. Peter Anvin]

Junio C Hamano wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
> 
>>/pub is a symbolic link.  We shouldn't rely on getcwd() for this kind of 
>>stuff; it's bad for a whole bunch of reasons.
> 
> 
> Well, if I recall correctly it was done this way because
> DWIMmery needs to be done before the validation.
> 
> Anyway, here is a rewrite of tonight (I resurrected your "belts
> and suspenders paranoia patch" for this).  Would appreciate it
> if people can try this out (in the proposed updates branch).
> 
> The rules (in 0.99.9k and "master" so far) have been that if you
> have symlinked public, whitelist should say what the canonical
> names of them are (and the way canonical names are obtained were
> getcwd()).  The rule of this patch is different: whitelist says
> what the remote requestor can ask for.  So if your /pub is a
> symlink to /mnt/mnt1/pub, you do not have to say /mnt/mnt1/pub
> to export it.  Instead you whitelist /pub (or /pub/scm).  Also
> if your ~bob is /home1/bob and ~alice is /home2/alice, you do
> not say "/home1 /home2" -- instead, you say "~alice ~bob".
> 

Yup, this is the way to do it.  Forcing people to use canonical names is 
quite a nonstarter.

	-hpa

[RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)

[Junio C Hamano]

Having slept over the patch I am responding to, I tend to think
this is more trouble than its worth.  Validating the
directory enter_repo() chdir()'ed into and validated to be a
good git repository should be done on its canonical name as
getcwd() returns, not with a userland aliasing avoidance.

As an administrator, being able to say /pub/scm on the
whitelist, knowing /pub to be a symbolic link points at
somewhere today but maybe at different place tomorrow, and not
having to adjust the whitelist whenever that happens, is indeed
nice.  We do not allow the remote requestor to say /../ in the
path, so we trap him within the directories the whitelist
describes.

Not.

For example, I can by mistake create a symbolic link:

	ln -s /home /pub/scm/git/git.git/oops

now accesses /pub/scm/git/oops/hpa/secret.git/ is not
restricted.  We could hand-resolve the each level from the
request to see if no "funny" symbolic links are involved, but
what is the definition of "funny"?  When we see /pub pointing at
somewhere in /mnt/disk47/slice31, we should not complain.  When
we see "oops" under git in the above example, we would want to
complain.  These things are hard to get right.

I tend to say that the 0.99.9k (and the current master) rule to
make validation always work on what getcwd() gives back is
easier to understand (which generally means safer).  Can I talk
you into adjusting your whitelist on kernel.org machines?

Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)

[H. Peter Anvin]

Junio C Hamano wrote:
> 
> For example, I can by mistake create a symbolic link:
> 
> 	ln -s /home /pub/scm/git/git.git/oops
> 
> now accesses /pub/scm/git/oops/hpa/secret.git/ is not
> restricted.  We could hand-resolve the each level from the
> request to see if no "funny" symbolic links are involved, but
> what is the definition of "funny"?  When we see /pub pointing at
> somewhere in /mnt/disk47/slice31, we should not complain.  When
> we see "oops" under git in the above example, we would want to
> complain.  These things are hard to get right.
> 

Actually, it's a policy decision whether or not symlinks should be 
allowed to exit space like that; in Apache, for example, it's a 
configurable.

> I tend to say that the 0.99.9k (and the current master) rule to
> make validation always work on what getcwd() gives back is
> easier to understand (which generally means safer).  Can I talk
> you into adjusting your whitelist on kernel.org machines?

I'm not happy about it, but it's not a huge deal on kernel.org. 
However, I think it's the wrong thing, especially in the light of 
allowing user-relative paths.

At the very least, if you insist on using getcwd() names, you should 
pre-canonicalize the whitelist, too.

	-hpa

Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)

[Linus Torvalds]

On Sat, 3 Dec 2005, H. Peter Anvin wrote:
> 
> At the very least, if you insist on using getcwd() names, you should
> pre-canonicalize the whitelist, too.

That would probably solve the problem and sounds like the right 
user-friendly solution.

		Linus

Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sat, 3 Dec 2005, H. Peter Anvin wrote:
>> 
>> At the very least, if you insist on using getcwd() names, you should
>> pre-canonicalize the whitelist, too.
>
> That would probably solve the problem and sounds like the right 
> user-friendly solution.

I agree with you that limiting with names exposed to the
end-user is the right approach and we should avoid having
administrators to use getcwd() names.  So in that sense, I think
the patch I sent earlier is going in the right direction.

To cope with the "oops" symlink problem, we could introduce
"--strict-symlink" flag to daemon that does these things:

 - enter_repo() returns three things: "~alice/foo.git/.git",
   "/home/alice/foo.git/.git", and "/home/alice"; the first is
   what remote requested with DWIM, the second is where it
   chdir()ed, and the third is what it expanded the
   user-relative base to.  None of them uses getcwd().  When
   user-relative path is not involved, the first two are the
   same, and the last one is undefined (and not used).

 - daemon checks the whitelist with the first one, and if the
   path is not allowed, the processing stops there with failure.
   Without --strict-symlink, this is the only test done with
   whitelist.  This means the whitelist should have /pub/scm and
   ~alice, not /mnt/disk47/slice31/scm nor /home2/alice.

   With --strict-symlink, it uses the latter two with the
   whitelist entry it matched, to determine where to start
   further "strict symlink check".  The part that matched the
   whitelist is OK and the rest is checked [*1*]:

   - If "~alice" was whitelisted, it knows ~alice expanded to
     /home/alice, and starts checking from /home/alice/foo.git
     and then checks /home/alice/foo.git/.git.

   - If "~alice/foo.git" was whitelisted, it knows it expands to
     /home/alice/foo.git, and checks /home/alice/foo.git/.git.

   - If a whitelist entry "/pub/scm" was matched against a
     request "/pub/scm/git/git.git", it checks /pub/scm/git and
     /pub/scm/git.git

   The strict symlink check tries to readlink() each of what are
   to be checked by the above logic, and rejects if it was found
   to be a symlink that starts with a "/" (i.e. absolute
   pathname) or anything that has undesiable aliasing effect;
   "belts and suspender paranoia" in daemon.c::avoid_alias(),
   which is stricter than needed but is safe is a good starting
   point, but we may want to allow things that do not step
   outside the prefix we matched.

However, I am not ready to do all of the above, not just yet.

Since I wanted to do another maintenance update this weekend,
I'd throw in the last-night's patch in the master so that at
least 0.99.9l works with /pub whitelist, knowing the "oops"
symlink issue is still to be solved.  That way we can keep the
users' configuration the same when later we introduce all of the
above.

Thoughts?

[Footnote]

*1* If we later introduce "/pub/scm/**/*.git", we allow symlinks
in the first directories without glob patterns, i.e. "/pub/scm",
so this is somewhat future-proof.

Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

Trivial typo.

>    - If a whitelist entry "/pub/scm" was matched against a
>      request "/pub/scm/git/git.git", it checks /pub/scm/git and
>      /pub/scm/git.git

Obviously "/pub/scm/git" and "/pub/scm/git/git.git" are the ones
that are checked here.  Whitelist "/pub/scm" means "/pub/scm
and under, and the administrator knows /pub or /pub/scm may be
symlinks pointing at places he wants them to, so do not check
and complain where they point at".

Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> At the very least, if you insist on using getcwd() names, you should 
> pre-canonicalize the whitelist, too.

With the current "prefix" rule (and not allowing /ho to match
/home) that sounds possible and sensivle, but that is not nice
in the long run.  We may later want to say "/pub/git/**/*.git"
for example to mean "any subdirectory under /pub/git but the
base directory name must be something ending with '.git'".

Hmm...

Re: [RFC] daemon whitelist handling (Re: git pull aborts in 50% of cases)

[H. Peter Anvin]

Junio C Hamano wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>>At the very least, if you insist on using getcwd() names, you should 
>>pre-canonicalize the whitelist, too.
> 
> With the current "prefix" rule (and not allowing /ho to match
> /home) that sounds possible and sensivle, but that is not nice
> in the long run.  We may later want to say "/pub/git/**/*.git"
> for example to mean "any subdirectory under /pub/git but the
> base directory name must be something ending with '.git'".
> 
> Hmm...
> 

Yep, this stuff is hard.  For example, on kernel.org I'm not concerned 
about symbolic links; the likelihood of an accidental symbolic link that 
would violate security is very small.  Other applications might be 
different.

Arguably, the correct interface is to modularize it, and have both the 
user request, the post-DWIM output, and the

	-hpa