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

[Mike Galbraith <bitbucket@online.de>]

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 <jrnieder@gmail.com>
> Improved-by: Jeff King <peff@peff.net>
> ---
> 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, ACCESS_EACCES_OK)) {
>  		ret += git_config_from_file(fn, xdg_config, data);
>  		found += 1;
>  	}
>  
> -	if (user_config && !access_or_die(user_config, R_OK)) {
> +	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {
>  		ret += git_config_from_file(fn, user_config, data);
>  		found += 1;
>  	}
>  
> -	if (repo_config && !access_or_die(repo_config, R_OK)) {
> +	if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
>  		ret += git_config_from_file(fn, repo_config, data);
>  		found += 1;
>  	}
> diff --git a/dir.c b/dir.c
> index 91cfd99..9cb2f3d 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1637,9 +1637,9 @@ void setup_standard_excludes(struct dir_struct *dir)
>  		home_config_paths(NULL, &xdg_path, "ignore");
>  		excludes_file = xdg_path;
>  	}
> -	if (!access_or_warn(path, R_OK))
> +	if (!access_or_warn(path, R_OK, 0))
>  		add_excludes_from_file(dir, path);
> -	if (excludes_file && !access_or_warn(excludes_file, R_OK))
> +	if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
>  		add_excludes_from_file(dir, excludes_file);
>  }
>  
> diff --git a/git-compat-util.h b/git-compat-util.h
> index cde442f..51a29b8 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -691,8 +691,9 @@ int remove_or_warn(unsigned int mode, const char *path);
>   * Call access(2), but warn for any error except "missing file"
>   * (ENOENT or ENOTDIR).
>   */
> -int access_or_warn(const char *path, int mode);
> -int access_or_die(const char *path, int mode);
> +#define ACCESS_EACCES_OK (1U << 0)
> +int access_or_warn(const char *path, int mode, unsigned flag);
> +int access_or_die(const char *path, int mode, unsigned flag);
>  
>  /* Warn on an inaccessible file that ought to be accessible */
>  void warn_on_inaccessible(const char *path);
> diff --git a/wrapper.c b/wrapper.c
> index bac59d2..dd7ecbb 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -408,18 +408,24 @@ void warn_on_inaccessible(const char *path)
>  	warning(_("unable to access '%s': %s"), path, strerror(errno));
>  }
>  
> -int access_or_warn(const char *path, int mode)
> +static int access_error_is_ok(int err, unsigned flag)
> +{
> +	return err == ENOENT || err == ENOTDIR ||
> +		((flag & ACCESS_EACCES_OK) && err == EACCES);
> +}
> +
> +int access_or_warn(const char *path, int mode, unsigned flag)
>  {
>  	int ret = access(path, mode);
> -	if (ret && errno != ENOENT && errno != ENOTDIR)
> +	if (ret && !access_error_is_ok(errno, flag))
>  		warn_on_inaccessible(path);
>  	return ret;
>  }
>  
> -int access_or_die(const char *path, int mode)
> +int access_or_die(const char *path, int mode, unsigned flag)
>  {
>  	int ret = access(path, mode);
> -	if (ret && errno != ENOENT && errno != ENOTDIR)
> +	if (ret && !access_error_is_ok(errno, flag))
>  		die_errno(_("unable to access '%s'"), path);
>  	return ret;
>  }