Re: [PATCH v2] config: allow inaccessible configuration under $HOME

2013-05-25 Thread Jason A. Donenfeld
Jonathan's patch would indeed be nice. In cgit, we are forced to do
ugly things like this:

/* Do not look in /etc/ for gitconfig and gitattributes. */
setenv("GIT_CONFIG_NOSYSTEM", "1", 1);
setenv("GIT_ATTR_NOSYSTEM", "1", 1);

/* We unset HOME and XDG_CONFIG_HOME before calling the git
setup function
 * so that we don't make unneccessary filesystem accesses. */
user_home = getenv("HOME");
xdg_home = getenv("XDG_CONFIG_HOME");
unsetenv("HOME");
unsetenv("XDG_CONFIG_HOME");

/* Setup the git directory and initialize the notes system.
Both of these
 * load local configuration from the git repository, so we do
them both while
 * the HOME variables are unset. */
setup_git_directory_gently(&nongit);
init_display_notes(NULL);

/* We restore the unset variables afterward. */
if (user_home)
setenv("HOME", user_home, 1);
if (xdg_home)
setenv("XDG_CONFIG_HOME", xdg_home, 1);

A big nasty guard around the git setup function, so that we don't error out.
--
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: [PATCH v2] config: allow inaccessible configuration under $HOME

2013-04-12 Thread Mike Galbraith
Tested, original setup works fine.

On Fri, 2013-04-12 at 14:03 -0700, Jonathan Nieder wrote: 
> The changes v1.7.12.1~2^2~4 (config: warn on inaccessible files,
> 2012-08-21) and v1.8.1.1~22^2~2 (config: treat user and xdg config
> permission problems as errors, 2012-10-13) were intended to prevent
> important configuration (think "[transfer] fsckobjects") from being
> ignored when the configuration is unintentionally unreadable (for
> example with EIO on a flaky filesystem, or with ENOMEM due to a DoS
> attack).  Usually ~/.gitconfig and ~/.config/git are readable by the
> current user, and if they aren't then it would be easy to fix those
> permissions, so the damage from adding this check should have been
> minimal.
> 
> Unfortunately the access() check often trips when git is being run as
> a server.  A daemon (such as inetd or git-daemon) starts as "root",
> creates a listening socket, and then drops privileges, meaning that
> when git commands are invoked they cannot access $HOME and die with
> 
>  fatal: unable to access '/root/.config/git/config': Permission denied
> 
> Any patch to fix this would have one of three problems:
> 
>   1. We annoy sysadmins who need to take an extra step to handle HOME
>  when dropping privileges (the current behavior, or any other
>  proposal that they have to opt into).
> 
>   2. We annoy sysadmins who want to set HOME when dropping privileges,
>  either by making what they want to do impossible, or making them
>  set an extra variable or option to accomplish what used to work
>  (e.g., a patch to git-daemon to set HOME when --user is passed).
> 
>   3. We loosen the check, so some cases which might be noteworthy are
>  not caught.
> 
> This patch is of type (3).
> 
> Treat user and xdg configuration that are inaccessible due to
> permissions (EACCES) as though no user configuration was provided at
> all.
> 
> An alternative method would be to check if $HOME is readable, but that
> would not help in cases where the user who dropped privileges had a
> globally readable HOME with only .config or .gitconfig being private.
> 
> This does not change the behavior when /etc/gitconfig or .git/config
> is unreadable (since those are more serious configuration errors),
> nor when ~/.gitconfig or ~/.config/git is unreadable due to problems
> other than permissions.
> 
> Signed-off-by: Jonathan Nieder 
> Improved-by: Jeff King 
> ---
> Jonathan Nieder wrote:
> 
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)
> > warning(_("unable to access '%s': %s"), path, strerror(errno));
> >  }
> >  
> > +static int access_error_is_ok(int err, unsigned flag)
> > +{
> > +   return errno == ENOENT || errno == ENOTDIR ||
> 
> Sigh, I can't spell "err".  Here's a v2 incorporating that change and
> with commit message incorporating the latest discussion.
> 
>  builtin/config.c  |  4 ++--
>  config.c  | 10 +-
>  dir.c |  4 ++--
>  git-compat-util.h |  5 +++--
>  wrapper.c | 14 ++
>  5 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index 33c9bf9..19ffcaf 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char 
> *prefix)
>*/
>   die("$HOME not set");
>  
> - if (access_or_warn(user_config, R_OK) &&
> - xdg_config && !access_or_warn(xdg_config, R_OK))
> + if (access_or_warn(user_config, R_OK, 0) &&
> + xdg_config && !access_or_warn(xdg_config, R_OK, 0))
>   given_config_file = xdg_config;
>   else
>   given_config_file = user_config;
> diff --git a/config.c b/config.c
> index aefd80b..830ee14 100644
> --- a/config.c
> +++ b/config.c
> @@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct 
> config_include_data *inc
>   path = buf.buf;
>   }
>  
> - if (!access_or_die(path, R_OK)) {
> + if (!access_or_die(path, R_OK, 0)) {
>   if (++inc->depth > MAX_INCLUDE_DEPTH)
>   die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
>   cf && cf->name ? cf->name : "the command line");
> @@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const 
> char *repo_config)
>  
>   home_config_paths(&user_config, &xdg_config, "config");
>  
> - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {
> + if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 
> 0)) {
>   ret += git_config_from_file(fn, git_etc_gitconfig(),
>   data);
>   found += 1;
>   }
>  
> - if (xdg_config && !access_or_die(xdg_config, R_OK)) {
> + if (xdg_config && !access_or_die(xdg_config, R_OK, ACC