{"thread":{"id":"20176","subject":"[PATCH] Preserve the protection mode for the Git config files","startedAt":"2009-07-21T15:24:36Z","lastAt":"2009-07-27T18:18:53Z","messageCount":5,"participants":["Catalin Marinas","Junio C Hamano","Nanako Shiraishi","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"118375","messageId":"20090721152435.16642.47207.stgit@pc1117.cambridge.arm.com","threadId":"20176","inReplyTo":null,"subject":"[PATCH] Preserve the protection mode for the Git config files","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@arm.com","sentAt":"2009-07-21T15:24:36Z","receivedAt":"2009-07-21T15:24:36Z","isPatch":true,"sender":{"key":"catalin.marinas@arm.com","avatar":null},"body":"Every time an option is set, the config file protection mode is changed\nto 0666 & ~umask even if it was different before. This patch is useful\nif people store passwords (SMTP server in the StGit case) and do not\nwant others to read the .gitconfig file.\n\nSigned-off-by: Catalin Marinas <catalin.marinas@arm.com>\n---\n lockfile.c |    6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex eb931ed..87ee233 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -124,8 +124,12 @@ static char *resolve_symlink(char *p, size_t s)\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n+\tstruct stat st;\n+\n \tif (strlen(path) >= sizeof(lk->filename))\n \t\treturn -1;\n+\tif (stat(path, &st) < 0)\n+\t\tst.st_mode = 0666;\n \tstrcpy(lk->filename, path);\n \t/*\n \t * subtract 5 from size to make sure there's room for adding\n@@ -134,7 +138,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tif (!(flags & LOCK_NODEREF))\n \t\tresolve_symlink(lk->filename, sizeof(lk->filename)-5);\n \tstrcat(lk->filename, \".lock\");\n-\tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n+\tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, st.st_mode);\n \tif (0 <= lk->fd) {\n \t\tif (!lock_file_list) {\n \t\t\tsigchain_push_common(remove_lock_file_on_signal);\n"},{"id":"118474","messageId":"7vab2wlh4y.fsf@alter.siamese.dyndns.org","threadId":"20176","inReplyTo":"20090721152435.16642.47207.stgit@pc1117.cambridge.arm.com","subject":"Re: [PATCH] Preserve the protection mode for the Git config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-22T18:14:21Z","receivedAt":"2009-07-22T18:14:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Catalin Marinas <catalin.marinas@arm.com> writes:\n\n> Every time an option is set, the config file protection mode is changed\n> to 0666 & ~umask even if it was different before. This patch is useful\n> if people store passwords (SMTP server in the StGit case) and do not\n> want others to read the .gitconfig file.\n>\n> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>\n> ---\n>  lockfile.c |    6 +++++-\n>  1 files changed, 5 insertions(+), 1 deletions(-)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index eb931ed..87ee233 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -134,7 +138,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n>  \tif (!(flags & LOCK_NODEREF))\n>  \t\tresolve_symlink(lk->filename, sizeof(lk->filename)-5);\n>  \tstrcat(lk->filename, \".lock\");\n> -\tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n> +\tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, st.st_mode);\n>  \tif (0 <= lk->fd) {\n>  \t\tif (!lock_file_list) {\n>  \t\t\tsigchain_push_common(remove_lock_file_on_signal);\n\nYour log message talks about .git/config and nothing else, but I think\nthis codepath affects everything that is created under the lock, such as\nthe index and refs.\n\nLater in the function we call adjust_shared_perm(), and I had to wonder if\nthis change have an adverse effect in a shared repository setting.\n\nFor example, in a repository shared (with core.sharedrepository = true)\nbetween users whose umask is 002, if somebody makes a ref unreadable by\nothers by mistake, the current code fixes the mistake in a later update,\nbut with your patch, the ref will kept unreadable.\n\nThis change in behaviour is justifiable only because the only thing the\nuser who said \"core.sharedrepository = true\" cares about is that refs are\nreadable by the group members (otherwise s/he would have used a more\nexplicit setting like \"core.sharedrepository = 0660\", and the\nadjust_shared_perm() code will do the right thing, with or without your\npatch).\n\nThe patch description must defend itself a bit better, perhaps by saying\nsomething like this at the end.\n\n\tThis patch touches the codepath that affects not just .git/config\n\tbut other files like the index and the loose refs, so they also\n\tinherit the original protection bits.  In a private repository,\n\tthis is not an issue exactly because the repository is private,\n\n\tIn a shared repository, a later call made in this function to\n\tadjust_shared_perm() widens the permission bits as configured.\n\tBecause adjust_shared_perm() is designed to do so from any mode\n\tlimited by user's umask, even though this patch changes the\n\tbehaviour in the strict sense, it should not affect the outcome in\n\ta negative way and what is explicitly marked as allowed in the\n\tconfiguration will still be allowed.\n"},{"id":"118507","messageId":"20090723070815.6117@nanako3.lavabit.com","threadId":"20176","inReplyTo":"7vab2wlh4y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Preserve the protection mode for the Git config files","fromName":"Nanako Shiraishi","fromEmail":"nanako3@lavabit.com","sentAt":"2009-07-22T22:08:15Z","receivedAt":"2009-07-22T22:08:15Z","isPatch":true,"sender":{"key":"nanako3@lavabit.com","avatar":"https://gravatar.com/avatar/3777b9e201c5883a62b1a6fdf7c53f2d712d1d80989146063ea861e33aad72a8?d=mp&s=160"},"body":"Quoting Junio C Hamano <gitster@pobox.com>\n\n> This change in behaviour is justifiable only because the only thing the\n> user who said \"core.sharedrepository = true\" cares about is that refs are\n> readable by the group members (otherwise s/he would have used a more\n> explicit setting like \"core.sharedrepository = 0660\", and the\n> adjust_shared_perm() code will do the right thing, with or without your\n> patch).\n>\n> The patch description must defend itself a bit better, perhaps by saying\n> something like this at the end.\n>\n> \tThis patch touches the codepath that affects not just .git/config\n> \tbut other files like the index and the loose refs, so they also\n> \tinherit the original protection bits.  In a private repository,\n> \tthis is not an issue exactly because the repository is private,\n>\n> \tIn a shared repository, a later call made in this function to\n> \tadjust_shared_perm() widens the permission bits as configured.\n> \tBecause adjust_shared_perm() is designed to do so from any mode\n> \tlimited by user's umask, even though this patch changes the\n> \tbehaviour in the strict sense, it should not affect the outcome in\n> \ta negative way and what is explicitly marked as allowed in the\n> \tconfiguration will still be allowed.\n\nI have two questions.\n\n1. 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?\n\n2. 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?\n\n-- \nNanako Shiraishi\nhttp://ivory.ap.teacup.com/nanako3/\n"},{"id":"118518","messageId":"alpine.DEB.1.00.0907230031450.3155@pacific.mpi-cbg.de","threadId":"20176","inReplyTo":"20090723070815.6117@nanako3.lavabit.com","subject":"Re: [PATCH] Preserve the protection mode for the Git config files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-22T22:37:11Z","receivedAt":"2009-07-22T22:37:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 23 Jul 2009, Nanako Shiraishi wrote:\n\n> 1. Why would you keep sensitive information in the config file in the \n>    first place? Wouldn't it be better to introduce a level of \n>    indirection, making a variable in the config file to point to a \n>    private file only you can read and store secrets in the latter?\n\nI agree that secret information should probably go to another file, \nalthough care has to be taken not to write that other file with \"git \nconfig -f\", as that would display the very same issue.\n\n> 2. Why is your config file more secret than your history?\n\nThat one's easy.  If you store passwords in the config file, it _is_ more \nsecret than the history.  You might be very willing to show people what \nyou did, but still be unwilling to allow people to push commits with your \ncredentials.\n\n> Wouldn't it solve your problem without any patch if you set \n> core.sharedrepository to 0600?\n\nI doubt it, as that config setting does not change anything in the working \ndirectory retro-actively.\n\nYou _could_ chmod 0700 .git.  But that is probably not what Catalin \nwanted.\n\nCiao,\nDscho\n"},{"id":"118889","messageId":"1248718733.12375.65.camel@pc1117.cambridge.arm.com","threadId":"20176","inReplyTo":"7vab2wlh4y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Preserve the protection mode for the Git config files","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@arm.com","sentAt":"2009-07-27T18:18:53Z","receivedAt":"2009-07-27T18:18:53Z","isPatch":true,"sender":{"key":"catalin.marinas@arm.com","avatar":null},"body":"Hi Junio,\n\nOn Wed, 2009-07-22 at 11:14 -0700, Junio C Hamano wrote:\n> Catalin Marinas <catalin.marinas@arm.com> writes:\n> > Every time an option is set, the config file protection mode is changed\n> > to 0666 & ~umask even if it was different before. This patch is useful\n> > if people store passwords (SMTP server in the StGit case) and do not\n> > want others to read the .gitconfig file.\n[...]\n> Your log message talks about .git/config and nothing else, but I think\n> this codepath affects everything that is created under the lock, such as\n> the index and refs.\n\nI haven't checked all the places where this function is called. For my\nuse-case, I store the SMTP password in the .git/config file (or\n~/.gitconfig) and every time I update this file (with git or via stgit),\nthe permission gets changed.\n\n> The patch description must defend itself a bit better, perhaps by saying\n> something like this at the end.\n> \n> \tThis patch touches the codepath that affects not just .git/config\n> \tbut other files like the index and the loose refs, so they also\n> \tinherit the original protection bits.  In a private repository,\n> \tthis is not an issue exactly because the repository is private,\n> \n> \tIn a shared repository, a later call made in this function to\n> \tadjust_shared_perm() widens the permission bits as configured.\n> \tBecause adjust_shared_perm() is designed to do so from any mode\n> \tlimited by user's umask, even though this patch changes the\n> \tbehaviour in the strict sense, it should not affect the outcome in\n> \ta negative way and what is explicitly marked as allowed in the\n> \tconfiguration will still be allowed.\n\nThanks for the explanation. Would you like me to repost with your\ndescription?\n\nThanks.\n\n-- \nCatalin\n"}]}