Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[W. Trevor King]

On Fri, Apr 12, 2013 at 01:05:50AM -0400, Jeff King wrote:
   Like many programs that switch user id, the daemon does not reset
   environment variables such a `$HOME` when it runs git programs like
   `upload-pack` and `receive-pack`. When using this option, you may also
   want to set and export `HOME` to point at the home directory of
   `user` before starting the daemon, and make sure the Git
   configuration file in that directory are readable by `user`.

How about and make sure any Git configuration files, since there
might not be any Git configuration files.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Evan Priestley]

Jonathan Nieder jrnie...@gmail.com wrote:

 I'm not sure whether to keep 96b9e0e (config: treat user and xdg
 config permission problem as errors) in the long run, BTW.
 
 Insights welcome.

For what it's worth, here's an anecdote about this:

I work on some open source software which includes a web-based repository 
browser for Git, somewhat similar to gitweb. We implement this partially by 
executing `git` commands from the webserver (usually Apache). For example, we 
run `git cat-file …` to retrieve file content to show to the user.

After this change, a number of users who manage installs of the software are 
hitting fatal: unable to access '/root/.config/git/config': Permission denied 
while browsing repositories, because their Apache runs as some unprivileged 
user (like apache or www-data) but with HOME=/root. We've seen about half a 
dozen reports of this now, so I believe this sort of setup is at least somewhat 
common and not just a bizarre one-off user with a broken environment. Users 
generally have difficulty resolving this error on their own, as it's not 
obvious that this boils down to an Apache environmental issue.

We'll likely resolve this by running `HOME=/ git ...` instead of `git ...` when 
we execute commands (or some more finessed version of that, but basically 
pointing HOME at some dummy readable directory). From cursory investigation, it 
appears we can't avoid this fatal with more surgical settings like GIT_CONFIG 
or XDG_CONFIG_HOME, since git still ends up looking in HOME and fataling 
anyway. This fix is a bit clunky, but not really a big deal.

I imagine our use case is fairly unusual, but I wanted to relate it in case 
it's helpful in balancing concerns here. If I've missed a more reasonable 
approach to solving this than redirecting HOME, please let me know, but it 
looks like that's more or less what the git-daemon patch is doing too.

Thanks,
Evan Priestley

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 07:26:36AM -0400, W. Trevor King wrote:

 On Fri, Apr 12, 2013 at 01:05:50AM -0400, Jeff King wrote:
Like many programs that switch user id, the daemon does not reset
environment variables such a `$HOME` when it runs git programs like
`upload-pack` and `receive-pack`. When using this option, you may also
want to set and export `HOME` to point at the home directory of
`user` before starting the daemon, and make sure the Git
configuration file in that directory are readable by `user`.
 
 How about and make sure any Git configuration files, since there
 might not be any Git configuration files.

Yeah, that is better. Thanks.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Junio C Hamano]

Jeff King p...@peff.net writes:

 How about and make sure any Git configuration files, since there
 might not be any Git configuration files.

 Yeah, that is better. Thanks.

OK, then...

-- 8 --
Subject: [PATCH] doc: clarify that git daemon --user=user option does not 
export HOME=~user

Signed-off-by: Jeff King p...@peff.net
Helped-by: W. Trevor King wk...@tremily.us
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 Documentation/git-daemon.txt | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 7e5098a..2ac07ba 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -147,6 +147,13 @@ OPTIONS
 Giving these options is an error when used with `--inetd`; use
 the facility of inet daemon to achieve the same before spawning
 'git daemon' if needed.
++
+Like many programs that switch user id, the daemon does not reset
+environment variables such as `$HOME` when it runs git programs,
+e.g. `upload-pack` and `receive-pack`. When using this option, you
+may also want to set and export `HOME` to point at the home
+directory of `user` before starting the daemon, and make sure any
+Git configuration files in that directory are readable by `user`.
 
 --enable=service::
 --disable=service::
-- 
1.8.2.1-472-g6c5785c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 09:08:31AM -0700, Junio C Hamano wrote:

 OK, then...

 -- 8 --
 Subject: [PATCH] doc: clarify that git daemon --user=user option does not 
 export HOME=~user

I'd add this motiviation to the body of the commit message:

  The fact that we don't set $HOME may confuse admins who
  expect $HOME/.gitconfig to be respected. And worse, since
  96b9e0e3, a git-daemon started by root is likely to fail
  to run at all, as the user we switch to generally cannot
  read ~root.

This still feels ugly, like we are documenting some gotcha
that is going to hit most admins, when we could be helping
them in the code.

One option we have not explored is an environment variable
to loosen git's requirement. I'm thinking something like
GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default
when git-daemon uses --user.

That would leave all existing setups working, but would
still enable the extra protections for people not running
git-daemon (and people who use git via sudo could choose to
set it, too, if they would prefer that to setting up HOME).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Mike Galbraith]

On Fri, 2013-04-12 at 09:08 -0700, Junio C Hamano wrote: 
 Jeff King p...@peff.net writes:
 
  How about and make sure any Git configuration files, since there
  might not be any Git configuration files.
 
  Yeah, that is better. Thanks.
 
 OK, then...
 
 -- 8 --
 Subject: [PATCH] doc: clarify that git daemon --user=user option does not 
 export HOME=~user
 
 Signed-off-by: Jeff King p...@peff.net
 Helped-by: W. Trevor King wk...@tremily.us
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---
  Documentation/git-daemon.txt | 7 +++
  1 file changed, 7 insertions(+)
 
 diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
 index 7e5098a..2ac07ba 100644
 --- a/Documentation/git-daemon.txt
 +++ b/Documentation/git-daemon.txt
 @@ -147,6 +147,13 @@ OPTIONS
  Giving these options is an error when used with `--inetd`; use
  the facility of inet daemon to achieve the same before spawning
  'git daemon' if needed.
 ++
 +Like many programs that switch user id, the daemon does not reset
 +environment variables such as `$HOME` when it runs git programs,
 +e.g. `upload-pack` and `receive-pack`. When using this option, you
 +may also want to set and export `HOME` to point at the home
 +directory of `user` before starting the daemon, and make sure any
 +Git configuration files in that directory are readable by `user`.

The you may want to.. is perhaps a little understated given it will
fail -EGOAWAY if git-daemon is started via init scripts if you don't.
(but otoh, that's enough of a hint to anyone setting the thing up, no
need to write paragraphs of legal-beagle boiler-plate for dinky bug;)

-Mike 

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 12:16:00PM -0400, Jeff King wrote:

 One option we have not explored is an environment variable
 to loosen git's requirement. I'm thinking something like
 GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default
 when git-daemon uses --user.
 
 That would leave all existing setups working, but would
 still enable the extra protections for people not running
 git-daemon (and people who use git via sudo could choose to
 set it, too, if they would prefer that to setting up HOME).

So here's what I came up with. I tried to make the exception as tight as
possible by checking that $HOME was actually the problem, as that is the
common problem (you switch users, but HOME is pointing to the old user).

It means that we would still die if ~root is readable, but
~root/.gitconfig is not (or ~root/.config is not). I think that is
probably a good thing, though, as it is an indication of something more
interesting going on that the user should investigate.

Given how tight the exception is, I almost wonder if we should drop the
environment variable completely, and just never complain about this case
(in other words, just make it a loosening of 96b9e0e3).

-Peff

---
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6a875f2..e004ee8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -806,6 +806,15 @@ for further details.
temporarily to avoid using a buggy `/etc/gitconfig` file while
waiting for someone with sufficient permissions to fix it.
 
+'GIT_INACCESSIBLE_HOME_OK'::
+   Normally git will complain and die if it cannot access
+   `$HOME/.gitconfig`. If this variable is set to 1, then
+   a `$HOME` directory which cannot be accessed will not be
+   considered an error. This can be useful if you are switching
+   user ids without resetting `$HOME`, and are willing to take the
+   risk that some configuration options might not be used (because
+   git could not read the config file that contained them).
+
 'GIT_FLUSH'::
If this environment variable is set to 1, then commands such
as 'git blame' (in incremental mode), 'git rev-list', 'git log',
diff --git a/cache.h b/cache.h
index e1e8ce8..01f300f 100644
--- a/cache.h
+++ b/cache.h
@@ -358,6 +358,7 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define GIT_NOTES_REWRITE_REF_ENVIRONMENT GIT_NOTES_REWRITE_REF
 #define GIT_NOTES_REWRITE_MODE_ENVIRONMENT GIT_NOTES_REWRITE_MODE
 #define GIT_LITERAL_PATHSPECS_ENVIRONMENT GIT_LITERAL_PATHSPECS
+#define GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT GIT_INACCESSIBLE_HOME_OK
 
 /*
  * This environment variable is expected to contain a boolean indicating
diff --git a/daemon.c b/daemon.c
index 131b049..6c56cc0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)
if (cred  (initgroups(cred-pass-pw_name, cred-gid) ||
setgid (cred-gid) || setuid(cred-pass-pw_uid)))
die(cannot drop privileges);
-   setenv(GIT_CONFIG_INACCESSIBLE_HOME_OK, 1, 0);
+   setenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 1, 0);
 }
 
 static struct credentials *prepare_credentials(const char *user_name,
diff --git a/wrapper.c b/wrapper.c
index bac59d2..644f867 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -416,11 +416,52 @@ int access_or_die(const char *path, int mode)
return ret;
 }
 
+/*
+ * Returns true iff the path is under $HOME, that directory is inaccessible,
+ * and the user has told us through the environment that an inaccessible HOME
+ * is OK. The results are cached so that repeated calls will not make multiple
+ * syscalls.
+ */
+static int inaccessible_home_ok(const char *path)
+{
+   static int gentle = -1;
+   static int home_inaccessible = -1;
+   const char *home;
+
+   if (gentle  0)
+   gentle = git_env_bool(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 0);
+   if (!gentle)
+   return 0;
+
+   home = getenv(HOME);
+   if (!home)
+   return 0;
+   /*
+* We do not bother with normalizing PATHs to avoid symlinks
+* here, since the point is to catch paths that are
+* constructed as $HOME/
+*/
+   if (prefixcmp(path, home)  path[strlen(home)] == '/')
+   return 0;
+
+   if (home_inaccessible  0)
+   home_inaccessible = access(home, R_OK|X_OK)  0  errno == 
EACCES;
+   return home_inaccessible;
+}
+
 int access_or_die(const char *path, int mode)
 {
int ret = access(path, mode);
-   if (ret  errno != ENOENT  errno != ENOTDIR)
+   if (ret  errno != ENOENT  errno != ENOTDIR) {
+   /*
+* We do not have to worry about clobbering errno
+* in inaccessible_home_ok, because we get either EACCES
+* again, or we die.
+*/
+   if (errno == EACCES  inaccessible_home_ok(path))
+   

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Fri, Apr 12, 2013 at 09:08:31AM -0700, Junio C Hamano wrote:

 OK, then...

 -- 8 --
 Subject: [PATCH] doc: clarify that git daemon --user=user option does 
 not export HOME=~user

 I'd add this motiviation to the body of the commit message:

   The fact that we don't set $HOME may confuse admins who
   expect $HOME/.gitconfig to be respected. And worse, since
   96b9e0e3, a git-daemon started by root is likely to fail
   to run at all, as the user we switch to generally cannot
   read ~root.

 This still feels ugly, like we are documenting some gotcha
 that is going to hit most admins, when we could be helping
 them in the code.

I agree that it feels a bit wrong to sound as if we are blaming the
messanger (the one that notices a possible misconfiguration), but
you are correct that we should make a note on why we think it is a
good idea to add this piece of extra documentation in the history.

Will add the above before queuing.

 One option we have not explored is an environment variable
 to loosen git's requirement. I'm thinking something like
 GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default
 when git-daemon uses --user.

 That would leave all existing setups working, but would
 still enable the extra protections for people not running
 git-daemon (and people who use git via sudo could choose to
 set it, too, if they would prefer that to setting up HOME).

Perhaps.

Right now, the only case people noticed was that we complain when
the effective user cannot even tell if config file(s) exists or not.
Labelling this option as Treat unreable as missing is fine, but
an inaccessible homedir is OK is vastly different.  Imagine a new
version where we start _requiring_ something to exist (and we read
from it) and imagine further that the expected place of that thing
is somewhere inside $HOME. We cannot keep the promise to those who
set an inaccessible homedir is OK option when that happens, as we
may need that piece of information we wanted to read from there in
order to properly operate.

In any case, I think the loosening is an independent issue.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Junio C Hamano]

Jeff King p...@peff.net writes:

 So here's what I came up with. I tried to make the exception as tight as
 possible by checking that $HOME was actually the problem, as that is the
 common problem (you switch users, but HOME is pointing to the old user).
 ...
 diff --git a/daemon.c b/daemon.c
 index 131b049..6c56cc0 100644
 --- a/daemon.c
 +++ b/daemon.c
 @@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)
   if (cred  (initgroups(cred-pass-pw_name, cred-gid) ||
   setgid (cred-gid) || setuid(cred-pass-pw_uid)))
   die(cannot drop privileges);
 - setenv(GIT_CONFIG_INACCESSIBLE_HOME_OK, 1, 0);
 + setenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 1, 0);
  }

Compared against an unpublished diffbase???

  
  static struct credentials *prepare_credentials(const char *user_name,
 diff --git a/wrapper.c b/wrapper.c
 index bac59d2..644f867 100644
 --- a/wrapper.c
 +++ b/wrapper.c
 @@ -416,11 +416,52 @@ int access_or_die(const char *path, int mode)
   return ret;
  }
  
 +/*
 + * Returns true iff the path is under $HOME, that directory is inaccessible,
 + * and the user has told us through the environment that an inaccessible HOME
 + * is OK. The results are cached so that repeated calls will not make 
 multiple
 + * syscalls.
 + */
 +static int inaccessible_home_ok(const char *path)
 +{
 + static int gentle = -1;
 + static int home_inaccessible = -1;
 + const char *home;
 +
 + if (gentle  0)
 + gentle = git_env_bool(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 0);
 + if (!gentle)
 + return 0;
 +
 + home = getenv(HOME);
 + if (!home)
 + return 0;
 + /*
 +  * We do not bother with normalizing PATHs to avoid symlinks
 +  * here, since the point is to catch paths that are
 +  * constructed as $HOME/
 +  */
 + if (prefixcmp(path, home)  path[strlen(home)] == '/')
 + return 0;
 +
 + if (home_inaccessible  0)
 + home_inaccessible = access(home, R_OK|X_OK)  0  errno == 
 EACCES;
 + return home_inaccessible;
 +}
 +
  int access_or_die(const char *path, int mode)
  {
   int ret = access(path, mode);
 - if (ret  errno != ENOENT  errno != ENOTDIR)
 + if (ret  errno != ENOENT  errno != ENOTDIR) {
 + /*
 +  * We do not have to worry about clobbering errno
 +  * in inaccessible_home_ok, because we get either EACCES
 +  * again, or we die.
 +  */
 + if (errno == EACCES  inaccessible_home_ok(path))
 + return ret;

OK, so the idea is

 - The environment can tell us to ignore permission errors for paths
   under $HOME if (and only if) $HOME itself is not readable;

 - We got a permission error here.  inaccessible_home_ok() will tell
   us if the path is under $HOME and the above condition holds (in
   which case it will say ok, ignore that error).

which sounds good, but it relies on the caller of this function not
to try actually reading from the path.

If the access() failed due to ENOENT, the caller will get a negative
return from this function and will treat it as ok, it does not
exist, with the original or the updated code.  This new case is
treated the same way by the existing callers, i.e. pretending as if
there is _no_ file in that unreadable $HOME directory.

That semantics sounds sane and safe to me.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 11:23:46AM -0700, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  So here's what I came up with. I tried to make the exception as tight as
  possible by checking that $HOME was actually the problem, as that is the
  common problem (you switch users, but HOME is pointing to the old user).
  ...
  diff --git a/daemon.c b/daemon.c
  index 131b049..6c56cc0 100644
  --- a/daemon.c
  +++ b/daemon.c
  @@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)
  if (cred  (initgroups(cred-pass-pw_name, cred-gid) ||
  setgid (cred-gid) || setuid(cred-pass-pw_uid)))
  die(cannot drop privileges);
  -   setenv(GIT_CONFIG_INACCESSIBLE_HOME_OK, 1, 0);
  +   setenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 1, 0);
   }
 
 Compared against an unpublished diffbase???

Oops. Forgot I had made a WIP commit before running the diff.

 OK, so the idea is
 
  - The environment can tell us to ignore permission errors for paths
under $HOME if (and only if) $HOME itself is not readable;
 
  - We got a permission error here.  inaccessible_home_ok() will tell
us if the path is under $HOME and the above condition holds (in
which case it will say ok, ignore that error).

Exactly.

 which sounds good, but it relies on the caller of this function not
 to try actually reading from the path.

Yes, but that is the only sane thing for the caller to do, since it gets
the same exit code from ENOENT and ENOTDIR already. Probably a comment
describing the return value is in order.

 If the access() failed due to ENOENT, the caller will get a negative
 return from this function and will treat it as ok, it does not
 exist, with the original or the updated code.  This new case is
 treated the same way by the existing callers, i.e. pretending as if
 there is _no_ file in that unreadable $HOME directory.

Exactly.

 That semantics sounds sane and safe to me.

Thanks. I'll re-roll with a proper commit message and the fixups I
mentioned above. I think we should still do the documentation for
git-daemon. But it is no longer about oops, we broke git-daemon, but
you may want know that we do not set HOME in case you are doing
something tricky with config. I'll submit that with the re-roll, too.

Do you have an opinion on just dropping the environment variable
completely and behaving this way all the time? It would just fix the
cases people running into using su/sudo, too.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 12:51:19PM -0700, Junio C Hamano wrote:

  If the access() failed due to ENOENT, the caller will get a negative
  return from this function and will treat it as ok, it does not
  exist, with the original or the updated code.  This new case is
  treated the same way by the existing callers, i.e. pretending as if
  there is _no_ file in that unreadable $HOME directory.
 
  Exactly.
 
 The explanation you are replying to was meant to illustrate how this
 is not inaccessible is OK, but is treat inaccessible as missing,
 by the way.

Ah, I see the distinction you were making. Yes, that is what I was
thinking (and what the patch does); I just used the word OK instead.

 Well, at least to me, the documentation update was never about
 oops, we broke it, but was about be careful where the HOME you
 are using actually is from the beginning of the suggestion.  I was
 actually planning to apply it to maint-1.8.1 that predates the xdg
 stuff, and that is why the text only suggests to set HOME for the
 config.

Yes; I think the only change needed would be to the commit message I
proposed (if you even picked that up; I didn't look).

  Do you have an opinion on just dropping the environment variable
  completely and behaving this way all the time? It would just fix the
  cases people running into using su/sudo, too.
 
 With the tightening, people who used --user=daemon, expecting that
 they can later tweak the behaviour by touching ~daemon/.gitconfig,
 got an early warning that they need to set HOME themselves, but with
 any variant of the patch under discussion, as long as loosening is
 on by default, will no longer get that benefit.
 
 I am not yet convinced if that is a real fix/cure.
 
 So, no, I have not even reached the point where I can form an
 opinion if this behaviour should be the default.

OK. I'll hold off for now while we stew on it. Jonathan's patch looks OK
to me, but it has the same issue. But I think every path has to be one
of:

  1. We annoy sysadmins who need to take an extra step to handle the
 HOME situation with --user (the current behavior, or any other
 proposal that they have to opt into).

  2. We annoy sysadmins who want to set HOME with --user, either by
 making what they want to do impossible, or making them set an extra
 variable or option to accomplish what used to work (my patch to set
 HOME with --user).

  3. We loosen the check, so some cases which might be noteworthy are
 not caught (my patch, Jonathan's patch, etc).

I think any solution will have to fall into one of those slots. So we
need to pick the least evil one, and then hammer out its least evil
form.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Junio C Hamano]

Jeff King p...@peff.net writes:

 OK. I'll hold off for now while we stew on it. Jonathan's patch looks OK
 to me, but it has the same issue. But I think every path has to be one
 of:

   1. We annoy sysadmins who need to take an extra step to handle the
  HOME situation with --user (the current behavior, or any other
  proposal that they have to opt into).

   2. We annoy sysadmins who want to set HOME with --user, either by
  making what they want to do impossible, or making them set an extra
  variable or option to accomplish what used to work (my patch to set
  HOME with --user).

   3. We loosen the check, so some cases which might be noteworthy are
  not caught (my patch, Jonathan's patch, etc).

 I think any solution will have to fall into one of those slots. So we
 need to pick the least evil one, and then hammer out its least evil
 form.

That summarizes our options nicely, I think.  Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Thu, Apr 11, 2013 at 05:39:43AM +0200, Mike Galbraith wrote:

ALLOWED_ENV=PATH HOME
HOME=/
 
 I can work around it by changing the init script to use su - git -c bla
 bla to launch the thing, instead of using --user=git --group=daemon,
 but that's just a bandaid for the busted environment setup those
 switches were supposed to make happen, no?

 Yeah, I think the bug here is that git-daemon should be setting $HOME
 when it switches privileges with --user. Does this patch fix it for you?

 diff --git a/daemon.c b/daemon.c
 index 6aeddcb..a4451fd 100644
 --- a/daemon.c
 +++ b/daemon.c
 @@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)
   if (cred  (initgroups(cred-pass-pw_name, cred-gid) ||
   setgid (cred-gid) || setuid(cred-pass-pw_uid)))
   die(cannot drop privileges);
 + setenv(HOME, cred-pass-pw_dir, 1);
  }
  
  static struct credentials *prepare_credentials(const char *user_name,

Yeah, that sounds like the obvious fix to me.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 08:35:46AM -0700, Junio C Hamano wrote:

  Yeah, I think the bug here is that git-daemon should be setting $HOME
  when it switches privileges with --user. Does this patch fix it for you?
 [...]
 Yeah, that sounds like the obvious fix to me.

Here it is with a commit message.

-- 8 --
Subject: [PATCH] daemon: set HOME when we switch to --user

If git-daemon is invoked with the --user foo option, we
setuid and setgid to the foo user. However, we do not
currently touch $HOME or any other environment variables.

This means that a git-daemon (and its git subprocesses)
invoked as root will look at ~root/.gitconfig,
~root/.config/git, etc. This is probably not what the admin
expected; it would make more sense to load user-wide config
from ~foo.

Traditionally this wasn't that big a deal, as most sites do
not put config in either homedir (they would use the
system-wide /etc/gitconfig if they wanted global config).
However, since 96b9e0e (config: treat user and xdg config
permission problems as errors, 2012-10-13), it is now an
error to try to read from an inaccessible config file (which
a file in ~root is very likely to be), meaning that
git-daemon will not run at all in such a case.

We can fix this by setting HOME appropriately when we switch
users. Note that this is a regression for any site that uses
--user but depends on putting config in the $HOME of the
user invoking git-daemon. Since the original behavior was
never documented, and the new behavior is much more
sensible, we can consider this a bugfix.

Reported-by: Mike Galbraith bitbuc...@online.de
Signed-off-by: Jeff King p...@peff.net
---
I don't have any problem calling this a bugfix and claiming that anyone
who was depending on the original behavior is stupid and wrong. But it
should probably get a prominent slot in the ReleaseNotes.

 daemon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/daemon.c b/daemon.c
index 6aeddcb..a4451fd 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1091,6 +1091,7 @@ static void drop_privileges(struct credentials *cred)
if (cred  (initgroups(cred-pass-pw_name, cred-gid) ||
setgid (cred-gid) || setuid(cred-pass-pw_uid)))
die(cannot drop privileges);
+   setenv(HOME, cred-pass-pw_dir, 1);
 }
 
 static struct credentials *prepare_credentials(const char *user_name,
-- 
1.8.2.rc0.33.gd915649

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jonathan Nieder]

Jeff King wrote:

 Here it is with a commit message.

 -- 8 --
 Subject: [PATCH] daemon: set HOME when we switch to --user

Thanks for taking care of it.  For what it's worth,

Acked-by: Jonathan Nieder jrnie...@gmail.com

I'm not sure whether to keep 96b9e0e (config: treat user and xdg
config permission problem as errors) in the long run, BTW.  There have
been multiple reports about dropping privileges and not being able to
access the old HOME, and I'm not convinced any more that the
predictability is worth the breakage for such people.  Though checking
if $HOME is inaccessible and treating that case specially would be
even worse...

Insights welcome.

Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 11:11:03AM -0700, Jonathan Nieder wrote:

  -- 8 --
  Subject: [PATCH] daemon: set HOME when we switch to --user
 
 Thanks for taking care of it.  For what it's worth,
 
 Acked-by: Jonathan Nieder jrnie...@gmail.com
 
 I'm not sure whether to keep 96b9e0e (config: treat user and xdg
 config permission problem as errors) in the long run, BTW.  There have
 been multiple reports about dropping privileges and not being able to
 access the old HOME, and I'm not convinced any more that the
 predictability is worth the breakage for such people.  Though checking
 if $HOME is inaccessible and treating that case specially would be
 even worse...
 
 Insights welcome.

I could go either way. I think 96b9e0e is the right thing to do
conceptually, but I kind of doubt it was affecting all that many people.
And though it's _possible_ for it to be a security problem, I find it
much more likely that the site admin tries to set some config, gets
annoyed when it doesn't work, and debugs it. So from a practical
perspective, 96b9e0e may be doing more harm than good, even though it's
the right thing.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jonathan Nieder]

Jeff King wrote:

 I could go either way. I think 96b9e0e is the right thing to do
 conceptually, but I kind of doubt it was affecting all that many people.
 And though it's _possible_ for it to be a security problem, I find it
 much more likely that the site admin tries to set some config, gets
 annoyed when it doesn't work, and debugs it. So from a practical
 perspective, 96b9e0e may be doing more harm than good, even though it's
 the right thing.

Ok.  By the way, another commit to blame is v1.7.12.1~2^2~4 (config:
warn on inaccessible files, 2012-08-21), which made it into a warnable
offense instead of just a strange but accepted configuration. ;-)

I'm still leaning toward keeping v1.7.12.1~2^2~4 and 96b9e0e as being
worth it, though I could be easily swayed in the other direction (for
example via a patch by an interested user with documentation that
explains how to debug and makes it unlikely for the behavior to keep
flipping in the future).  Thanks for spelling out the trade-offs.

Sincerely,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Thu, Apr 11, 2013 at 11:11:03AM -0700, Jonathan Nieder wrote:

  -- 8 --
  Subject: [PATCH] daemon: set HOME when we switch to --user
 
 Thanks for taking care of it.  For what it's worth,
 
 Acked-by: Jonathan Nieder jrnie...@gmail.com
 
 I'm not sure whether to keep 96b9e0e (config: treat user and xdg
 config permission problem as errors) in the long run, BTW.  There have
 been multiple reports about dropping privileges and not being able to
 access the old HOME, and I'm not convinced any more that the
 predictability is worth the breakage for such people.  Though checking
 if $HOME is inaccessible and treating that case specially would be
 even worse...
 
 Insights welcome.

 I could go either way. I think 96b9e0e is the right thing to do
 conceptually, but I kind of doubt it was affecting all that many people.
 And though it's _possible_ for it to be a security problem, I find it
 much more likely that the site admin tries to set some config, gets
 annoyed when it doesn't work, and debugs it. So from a practical
 perspective, 96b9e0e may be doing more harm than good, even though it's
 the right thing.

Recent reports in this thread make us think so, I guess.

But reverting 96b9e0e alone would not help these people very much
though.  They will have reams of warning messages in their server
logs, and the way to fix it would be the same as the way to work
around the access_or_die(), namely, to set $HOME to point at a more
appropriate place before running git daemon.

I also have a suspicion that your patch makes things worse for
people who are more adept at these issues around running daemons
than the people who introduced this problem in the first place (eh,
that's us).  It is plausible that they may run multiple instances
of initially root but setuid() to an unprivileged user daemons,
giving each of them a separate play area by setting $HOME to
different values, just for management's ease not necessarily for
security (hence sharing the same unprivileged user), which will be
broken by the patch that unconditionally overrides $HOME.

A trade off to make things slightly easier for one sysadmin by
making another thing impossible to do for another sysadmin does not
sound like a good one.




--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 12:54:34PM -0700, Junio C Hamano wrote:

  I could go either way. I think 96b9e0e is the right thing to do
  conceptually, but I kind of doubt it was affecting all that many people.
  And though it's _possible_ for it to be a security problem, I find it
  much more likely that the site admin tries to set some config, gets
  annoyed when it doesn't work, and debugs it. So from a practical
  perspective, 96b9e0e may be doing more harm than good, even though it's
  the right thing.
 
 Recent reports in this thread make us think so, I guess.
 
 But reverting 96b9e0e alone would not help these people very much
 though.  They will have reams of warning messages in their server
 logs, and the way to fix it would be the same as the way to work
 around the access_or_die(), namely, to set $HOME to point at a more
 appropriate place before running git daemon.

Yeah, if we revert 96b9e0e, it would only make sense to revert the
warnings, too. Going halfway does not help anyone.

 I also have a suspicion that your patch makes things worse for
 people who are more adept at these issues around running daemons
 than the people who introduced this problem in the first place (eh,
 that's us).  It is plausible that they may run multiple instances
 of initially root but setuid() to an unprivileged user daemons,
 giving each of them a separate play area by setting $HOME to
 different values, just for management's ease not necessarily for
 security (hence sharing the same unprivileged user), which will be
 broken by the patch that unconditionally overrides $HOME.

Yes, we would definitely be breaking them with this patch. I don't know
how common that is. As you noted, it is a bad idea security-wise (if
everything runs as nobody, then the services are not insulated from
each other), but I can perhaps see a case where all git repos are owned
by the git user, but they may be accessed by different config
profiles, which are managed by $HOME.

You could still accomplish the same thing with git by setting
XDG_CONFIG_HOME, though that of course requires effort from the admin.
Sub-programs may not necessarily respect $XDG_CONFIG_HOME, though (e.g.,
anything run from a post-receive hook). On the other hand, people do not
generally push through git-daemon. But that feels like a weak argument.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[W. Trevor King]

On Thu, Apr 11, 2013 at 12:54:34PM -0700, Junio C Hamano wrote:
 I also have a suspicion that your patch makes things worse for
 people who are more adept at these issues around running daemons
 than the people who introduced this problem in the first place (eh,
 that's us)…
 
 A trade off to make things slightly easier for one sysadmin by
 making another thing impossible to do for another sysadmin does not
 sound like a good one.

As one of the less experienced folks tripped up by this issue, I think
that setting HOME explicitly before invoking the daemon is simple
enough (which is why I just fixed my invocation and didn't post to the
list).  The difficulty was figuring out why the daemon was dying in
the first place (which involved bisection for me as well).  Maybe
there could be an additional note about HOME to flesh out:

  fatal: unable to access '/root/.config/git/config': Permission denied

when there's an EACCES error for the per-user config?  On the other
hand, the code to handle this might be more trouble than it's worth.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Junio C Hamano]

W. Trevor King wk...@tremily.us writes:

 As one of the less experienced folks tripped up by this issue, I think
 that setting HOME explicitly before invoking the daemon is simple
 enough (which is why I just fixed my invocation and didn't post to the
 list).

Sounds like we need a documentation update somewhere.

 The difficulty was figuring out why the daemon was dying in
 the first place (which involved bisection for me as well).  Maybe
 there could be an additional note about HOME to flesh out:

   fatal: unable to access '/root/.config/git/config': Permission denied

 when there's an EACCES error for the per-user config?

Doesn't access_or_die() say

die_errno(_(unable to access '%s'), path);

already?  I am puzzled...



--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 03:20:46PM -0700, Junio C Hamano wrote:

 W. Trevor King wk...@tremily.us writes:
 
  As one of the less experienced folks tripped up by this issue, I think
  that setting HOME explicitly before invoking the daemon is simple
  enough (which is why I just fixed my invocation and didn't post to the
  list).
 
 Sounds like we need a documentation update somewhere.

Yeah, I'd be happy to drop my patch if somebody wants to write a
documentation update instead.

  The difficulty was figuring out why the daemon was dying in
  the first place (which involved bisection for me as well).  Maybe
  there could be an additional note about HOME to flesh out:
 
fatal: unable to access '/root/.config/git/config': Permission denied
 
  when there's an EACCES error for the per-user config?
 
 Doesn't access_or_die() say
 
 die_errno(_(unable to access '%s'), path);
 
 already?  I am puzzled...

I think the point is that it could add

  ...and I was looking in /root, because that is where your HOME points.
  Shouldn't you be able to read your own HOME directory?

which should make it painfully obvious to the user what is going on.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Junio C Hamano]

W. Trevor King wk...@tremily.us writes:

 On Thu, Apr 11, 2013 at 06:23:01PM -0400, Jeff King wrote:
 ...
 I think the point is that it could add
 
   ...and I was looking in /root, because that is where your HOME points.
   Shouldn't you be able to read your own HOME directory?
 
 which should make it painfully obvious to the user what is going on.

 That's more or less what I had in mind.
 ... However, this is a lot of hand holding to be
 printed along side the error message… Since git-daemon (or gitweb) is
 the most likely place for this problem to crop up, maybe a note in its
 (their) man pages would be a good idea?

The --user option to git-daemon would be a good place to do that, I
think.  Depending on what other setuid to less privileged before
running programs do (I do not know offhand), we can say something
like this perhaps?

--user::
... current description ...
+
(Like|Unlike) many programs that let you run programs as
specified user, the daemon does not reset environment variables
such as $HOME when it runs git programs like upload-pack and
receive-pack. Set and export HOME to point at the home directory
of the user you specify with this option before you start the
daemon, and make sure the Git configuration files in that
directory is readable by that user.

If we have to say Unlike above, then we probably should bite the
bullet and use Peff's patch, perhaps with an addition to the manual
page, perhaps like this.

--user::
... current description ...
+
Like many programs that let you run programs as specified user,
the daemon resets $HOME environment variable to that of the user
you specify with this option when it runs git programs like
upload-pack and receive-pack.  Make sure that the Git
configuration files in that directory is readable by that user.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Jeff King]

On Thu, Apr 11, 2013 at 09:11:20PM -0700, Junio C Hamano wrote:

 The --user option to git-daemon would be a good place to do that, I
 think.  Depending on what other setuid to less privileged before
 running programs do (I do not know offhand), we can say something
 like this perhaps?

That's a good question. I looked at (just sampling a few off the top of
my head):

  xinetd
  openbsd-inetd
  inetutils-inetd
  postfix
  dovecot
  courier

and none of them sets HOME when dropping privileges. Admittedly some of
them do not drop privileges immediately in the same way (e.g., the imap
servers need to remain root so that they can switch to the right user to
read mail). Postfix does set HOME, but only when actually becoming the
user to do deliveries, not at startup.

I could also be wrong on one or more of those, as that is from some
quick grepping, but I think it's clear that the norm is not to set HOME
alongside setuid (of all of them, I would say git-daemon behaves most
like the inetd utils, as it does not ever become users at all).

 --user::
   ... current description ...
 +
 (Like|Unlike) many programs that let you run programs as
 specified user, the daemon does not reset environment variables
 such as $HOME when it runs git programs like upload-pack and
 receive-pack. Set and export HOME to point at the home directory
 of the user you specify with this option before you start the
 daemon, and make sure the Git configuration files in that
 directory is readable by that user.

So choosing Like here, I think this makes sense.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Junio C Hamano]

Jeff King p...@peff.net writes:

 On Thu, Apr 11, 2013 at 09:11:20PM -0700, Junio C Hamano wrote:

 The --user option to git-daemon would be a good place to do that, I
 think.  Depending on what other setuid to less privileged before
 running programs do (I do not know offhand), we can say something
 like this perhaps?

 That's a good question. I looked at (just sampling a few off the top of
 my head):

   xinetd
   openbsd-inetd
   inetutils-inetd
   postfix
   dovecot
   courier

 and none of them sets HOME when dropping privileges. Admittedly some of
 them do not drop privileges immediately in the same way (e.g., the imap
 servers need to remain root so that they can switch to the right user to
 read mail). Postfix does set HOME, but only when actually becoming the
 user to do deliveries, not at startup.

Thanks for checking.

For $HOME, it is sufficient to do an unsetenv-equivalent, and I
suspect some of them do just that, though.  Vanilla openbsd-inetd
doesn't, but Debian seems to have a patch to sanitize the
environment, for example.

But still...

 I could also be wrong on one or more of those, as that is from some
 quick grepping, but I think it's clear that the norm is not to set HOME
 alongside setuid (of all of them, I would say git-daemon behaves most
 like the inetd utils, as it does not ever become users at all).

 --user::
  ... current description ...
 +
 (Like|Unlike) many programs that let you run programs as
 specified user, the daemon does not reset environment variables
 such as $HOME when it runs git programs like upload-pack and
 receive-pack. Set and export HOME to point at the home directory
 of the user you specify with this option before you start the
 daemon, and make sure the Git configuration files in that
 directory is readable by that user.

 So choosing Like here, I think this makes sense.

I would prefer the simplicity ;-)

Set and export HOME to point at the home directory of the user you
specify with this option screams that it wants to be rephrased at
me, though.  It somehow sounds as if this option is a way to set and
export the environment variable unless re-read carefully X-.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: 96b9e0e3 config: treat user and xdg config permission problems as errors busted git-daemon

[Mike Galbraith]

On Fri, 2013-04-12 at 01:05 -0400, Jeff King wrote: 
 On Thu, Apr 11, 2013 at 09:46:35PM -0700, Junio C Hamano wrote:
 
   --user::
... current description ...
   +
   (Like|Unlike) many programs that let you run programs as
   specified user, the daemon does not reset environment variables
   such as $HOME when it runs git programs like upload-pack and
   receive-pack. Set and export HOME to point at the home directory
   of the user you specify with this option before you start the
   daemon, and make sure the Git configuration files in that
   directory is readable by that user.
  
   So choosing Like here, I think this makes sense.
  
  I would prefer the simplicity ;-)
  
  Set and export HOME to point at the home directory of the user you
  specify with this option screams that it wants to be rephrased at
  me, though.  It somehow sounds as if this option is a way to set and
  export the environment variable unless re-read carefully X-.
 
 Perhaps:
 
   Like many programs that switch user id, the daemon does not reset
   environment variables such a `$HOME` when it runs git programs like
   `upload-pack` and `receive-pack`. When using this option, you may also
   want to set and export `HOME` to point at the home directory of
   `user` before starting the daemon, and make sure the Git
   configuration file in that directory are readable by `user`.
 
 I tried to address your concern above (which I agree with), smooth over
 a few clunky wordings, and use user, which is defined in the heading
 of the option:
 
   --user=user, --group=group

I just updated my local rpm to set HOME, so the surprise of no workee
after upgrade here is forever gone.  Looks like suses (at least)
packagers will need to do the same, else canned setup ain't gonna work.

-Mike

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html