From: Mike Galbraith <bitbucket@online.de>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>,
"W. Trevor King" <wking@tremily.us>, git <git@vger.kernel.org>
Subject: Re: [PATCH v2] config: allow inaccessible configuration under $HOME
Date: Sat, 13 Apr 2013 06:28:16 +0200 [thread overview]
Message-ID: <1365827296.4643.9.camel@marge.simpson.net> (raw)
In-Reply-To: <20130412210318.GU27070@google.com>
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;
> }
next prev parent reply other threads:[~2013-04-13 4:28 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-10 5:33 regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon Mike Galbraith
2013-04-10 13:56 ` W. Trevor King
2013-04-11 3:39 ` Mike Galbraith
2013-04-11 5:42 ` Jeff King
2013-04-11 7:59 ` Mike Galbraith
2013-04-11 15:35 ` Junio C Hamano
2013-04-11 17:24 ` Jeff King
2013-04-11 18:11 ` Jonathan Nieder
2013-04-11 18:14 ` Jeff King
2013-04-11 18:25 ` Jonathan Nieder
2013-04-11 19:54 ` Junio C Hamano
2013-04-11 20:03 ` W. Trevor King
2013-04-11 22:20 ` Junio C Hamano
2013-04-11 22:23 ` Jeff King
2013-04-12 0:57 ` W. Trevor King
2013-04-12 4:11 ` Junio C Hamano
2013-04-12 4:35 ` Jeff King
2013-04-12 4:46 ` Junio C Hamano
2013-04-12 5:05 ` Jeff King
2013-04-12 5:46 ` Mike Galbraith
2013-04-12 11:26 ` W. Trevor King
2013-04-12 14:48 ` Jeff King
2013-04-12 16:08 ` Junio C Hamano
2013-04-12 16:16 ` Jeff King
2013-04-12 17:05 ` Jeff King
2013-04-12 18:23 ` Junio C Hamano
2013-04-12 19:01 ` Jeff King
2013-04-12 19:51 ` Junio C Hamano
2013-04-12 19:58 ` Jeff King
2013-04-12 20:45 ` Junio C Hamano
2013-04-12 19:14 ` [PATCH] config: allow inaccessible configuration under $HOME Jonathan Nieder
2013-04-12 19:37 ` Jeff King
2013-04-12 20:34 ` [PATCH] fixup! " Jonathan Nieder
2013-04-12 21:03 ` [PATCH v2] " Jonathan Nieder
2013-04-13 4:28 ` Mike Galbraith [this message]
2013-05-25 11:35 ` Jason A. Donenfeld
2013-04-12 17:31 ` regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon Junio C Hamano
2013-04-12 16:21 ` Mike Galbraith
2013-04-11 20:08 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1365827296.4643.9.camel@marge.simpson.net \
--to=bitbucket@online.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=peff@peff.net \
--cc=wking@tremily.us \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).