From: Junio C Hamano <hidden> Date: 2016-06-15 22:47:05
Catalin Marinas [off-list ref] writes:
quoted hunk
Every time an option is set, the config file protection mode is changed
to 0666 & ~umask even if it was different before. This patch is useful
if people store passwords (SMTP server in the StGit case) and do not
want others to read the .gitconfig file.
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
lockfile.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
@@ -134,7 +138,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)if(!(flags&LOCK_NODEREF))resolve_symlink(lk->filename,sizeof(lk->filename)-5);strcat(lk->filename,".lock");-lk->fd=open(lk->filename,O_RDWR|O_CREAT|O_EXCL,0666);+lk->fd=open(lk->filename,O_RDWR|O_CREAT|O_EXCL,st.st_mode);if(0<=lk->fd){if(!lock_file_list){sigchain_push_common(remove_lock_file_on_signal);
Your log message talks about .git/config and nothing else, but I think
this codepath affects everything that is created under the lock, such as
the index and refs.
Later in the function we call adjust_shared_perm(), and I had to wonder if
this change have an adverse effect in a shared repository setting.
For example, in a repository shared (with core.sharedrepository = true)
between users whose umask is 002, if somebody makes a ref unreadable by
others by mistake, the current code fixes the mistake in a later update,
but with your patch, the ref will kept unreadable.
This change in behaviour is justifiable only because the only thing the
user who said "core.sharedrepository = true" cares about is that refs are
readable by the group members (otherwise s/he would have used a more
explicit setting like "core.sharedrepository = 0660", and the
adjust_shared_perm() code will do the right thing, with or without your
patch).
The patch description must defend itself a bit better, perhaps by saying
something like this at the end.
This patch touches the codepath that affects not just .git/config
but other files like the index and the loose refs, so they also
inherit the original protection bits. In a private repository,
this is not an issue exactly because the repository is private,
In a shared repository, a later call made in this function to
adjust_shared_perm() widens the permission bits as configured.
Because adjust_shared_perm() is designed to do so from any mode
limited by user's umask, even though this patch changes the
behaviour in the strict sense, it should not affect the outcome in
a negative way and what is explicitly marked as allowed in the
configuration will still be allowed.
This change in behaviour is justifiable only because the only thing the
user who said "core.sharedrepository = true" cares about is that refs are
readable by the group members (otherwise s/he would have used a more
explicit setting like "core.sharedrepository = 0660", and the
adjust_shared_perm() code will do the right thing, with or without your
patch).
The patch description must defend itself a bit better, perhaps by saying
something like this at the end.
This patch touches the codepath that affects not just .git/config
but other files like the index and the loose refs, so they also
inherit the original protection bits. In a private repository,
this is not an issue exactly because the repository is private,
In a shared repository, a later call made in this function to
adjust_shared_perm() widens the permission bits as configured.
Because adjust_shared_perm() is designed to do so from any mode
limited by user's umask, even though this patch changes the
behaviour in the strict sense, it should not affect the outcome in
a negative way and what is explicitly marked as allowed in the
configuration will still be allowed.
I have two questions.
1. Why would you keep sensitive information in the config file in the first place? Wouldn't it be better to introduce a level of indirection, making a variable in the config file to point to a private file only you can read and store secrets in the latter?
2. Why is your config file more secret than your history? Wouldn't it solve your problem without any patch if you set core.sharedrepository to 0600?
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
From: Johannes Schindelin <hidden> Date: 2016-06-15 22:47:05
Hi,
On Thu, 23 Jul 2009, Nanako Shiraishi wrote:
1. Why would you keep sensitive information in the config file in the
first place? Wouldn't it be better to introduce a level of
indirection, making a variable in the config file to point to a
private file only you can read and store secrets in the latter?
I agree that secret information should probably go to another file,
although care has to be taken not to write that other file with "git
config -f", as that would display the very same issue.
2. Why is your config file more secret than your history?
That one's easy. If you store passwords in the config file, it _is_ more
secret than the history. You might be very willing to show people what
you did, but still be unwilling to allow people to push commits with your
credentials.
Wouldn't it solve your problem without any patch if you set
core.sharedrepository to 0600?
I doubt it, as that config setting does not change anything in the working
directory retro-actively.
You _could_ chmod 0700 .git. But that is probably not what Catalin
wanted.
Ciao,
Dscho
Hi Junio,
On Wed, 2009-07-22 at 11:14 -0700, Junio C Hamano wrote:
Catalin Marinas [off-list ref] writes:
quoted
Every time an option is set, the config file protection mode is changed
to 0666 & ~umask even if it was different before. This patch is useful
if people store passwords (SMTP server in the StGit case) and do not
want others to read the .gitconfig file.
[...]
Your log message talks about .git/config and nothing else, but I think
this codepath affects everything that is created under the lock, such as
the index and refs.
I haven't checked all the places where this function is called. For my
use-case, I store the SMTP password in the .git/config file (or
~/.gitconfig) and every time I update this file (with git or via stgit),
the permission gets changed.
The patch description must defend itself a bit better, perhaps by saying
something like this at the end.
This patch touches the codepath that affects not just .git/config
but other files like the index and the loose refs, so they also
inherit the original protection bits. In a private repository,
this is not an issue exactly because the repository is private,
In a shared repository, a later call made in this function to
adjust_shared_perm() widens the permission bits as configured.
Because adjust_shared_perm() is designed to do so from any mode
limited by user's umask, even though this patch changes the
behaviour in the strict sense, it should not affect the outcome in
a negative way and what is explicitly marked as allowed in the
configuration will still be allowed.
Thanks for the explanation. Would you like me to repost with your
description?
Thanks.
--
Catalin