[PATCH] Make core.sharedRepository more generic (version 2)

[PATCH] Make core.sharedRepository more generic (version 2)

[Heikki Orsila]

git init --shared=0xxx, where '0xxx' is an octal number, will create
a repository with file modes set to '0xxx'. User's with a safe umask
value (0077) can use this option to force file modes. For example,
'0640' is a group-readable but not group-writable regardless of
user's umask value.

"git config core.sharedRepository 0xxx" is also handled.

Version 2 handles the directory x flags better than version 1.

Signed-off-by: Heikki Orsila <heikki.orsila@iki.fi>
---
 Documentation/config.txt   |    7 +++++-
 Documentation/git-init.txt |    8 ++++++-
 builtin-init-db.c          |    4 +-
 cache.h                    |   16 ++++++++++++--
 path.c                     |   32 ++++++++++++++----------------
 setup.c                    |   45 ++++++++++++++++++++++++++++++++++++++++---
 6 files changed, 84 insertions(+), 28 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fe43b12..7a24f6e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -261,7 +261,12 @@ core.sharedRepository::
 	group-writable). When 'all' (or 'world' or 'everybody'), the
 	repository will be readable by all users, additionally to being
 	group-shareable. When 'umask' (or 'false'), git will use permissions
-	reported by umask(2). See linkgit:git-init[1]. False by default.
+	reported by umask(2). When '0xxx', where '0xxx' is an octal number,
+	files in the repository will have this mode value. '0xxx' will override
+	user's umask value, and thus, users with a safe umask (0077) can use
+	this option. Examples: '0660' is equivalent to 'group'. '0640' is a
+	repository that is group-readable but not group-writable.
+	See linkgit:git-init[1]. False by default.
 
 core.warnAmbiguousRefs::
 	If true, git will warn you if the ref name you passed it is ambiguous
diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 62914da..b17ae84 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -31,7 +31,7 @@ structure, some suggested "exclude patterns", and copies of non-executing
 "hook" files.  The suggested patterns and hook files are all modifiable and
 extensible.
 
---shared[={false|true|umask|group|all|world|everybody}]::
+--shared[={false|true|umask|group|all|world|everybody|0xxx}]::
 
 Specify that the git repository is to be shared amongst several users.  This
 allows users belonging to the same group to push into that
@@ -52,6 +52,12 @@ is given:
  - 'all' (or 'world' or 'everybody'): Same as 'group', but make the repository
    readable by all users.
 
+ - '0xxx': '0xxx' is an octal number and each file will have mode '0xxx'
+   Any option except 'umask' can be set using this option. '0xxx' will
+   override users umask(2) value, and thus, users with a safe umask (0077)
+   can use this option. '0640' will create a repository which is group-readable
+   but not writable. '0660' is equivalent to 'group'.
+
 By default, the configuration flag receive.denyNonFastForwards is enabled
 in shared repositories, so that you cannot force a non fast-forwarding push
 into it.
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 2854868..8c63295 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -400,9 +400,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 		char buf[10];
 		/* We do not spell "group" and such, so that
 		 * the configuration can be read by older version
-		 * of git.
+		 * of git. Note, we use octal numbers.
 		 */
-		sprintf(buf, "%d", shared_repository);
+		sprintf(buf, "0%o", shared_repository);
 		git_config_set("core.sharedrepository", buf);
 		git_config_set("receive.denyNonFastforwards", "true");
 	}
diff --git a/cache.h b/cache.h
index 2a1e7ec..f9c8d2b 100644
--- a/cache.h
+++ b/cache.h
@@ -474,10 +474,20 @@ static inline void hashclr(unsigned char *hash)
 
 int git_mkstemp(char *path, size_t n, const char *template);
 
+/*
+ * NOTE NOTE NOTE!!
+ *
+ * PERM_UMASK, OLD_PERM_GROUP and OLD_PERM_EVERYBODY enumerations must
+ * not be changed. Old repositories have core.sharedrepository written in
+ * numeric format, and therefore these values are preserved for compatibility
+ * reasons.
+ */
 enum sharedrepo {
-	PERM_UMASK = 0,
-	PERM_GROUP,
-	PERM_EVERYBODY
+	PERM_UMASK          = 0,
+	OLD_PERM_GROUP      = 1,
+	OLD_PERM_EVERYBODY  = 2,
+	PERM_GROUP          = 0660,
+	PERM_EVERYBODY      = 0664,
 };
 int git_config_perm(const char *var, const char *value);
 int adjust_shared_perm(const char *path);
diff --git a/path.c b/path.c
index f4ed979..cfa0529 100644
--- a/path.c
+++ b/path.c
@@ -266,24 +266,22 @@ int adjust_shared_perm(const char *path)
 	if (lstat(path, &st) < 0)
 		return -1;
 	mode = st.st_mode;
-	if (mode & S_IRUSR)
-		mode |= (shared_repository == PERM_GROUP
-			 ? S_IRGRP
-			 : (shared_repository == PERM_EVERYBODY
-			    ? (S_IRGRP|S_IROTH)
-			    : 0));
-
-	if (mode & S_IWUSR)
-		mode |= S_IWGRP;
-
-	if (mode & S_IXUSR)
-		mode |= (shared_repository == PERM_GROUP
-			 ? S_IXGRP
-			 : (shared_repository == PERM_EVERYBODY
-			    ? (S_IXGRP|S_IXOTH)
-			    : 0));
-	if (S_ISDIR(mode))
+
+	if (shared_repository) {
+		mode = (mode & ~0777) | shared_repository;
+	} else {
+		/* Preserve old PERM_UMASK behaviour */
+		if (mode & S_IWUSR)
+			mode |= S_IWGRP;
+	}
+
+	if (S_ISDIR(mode)) {
 		mode |= FORCE_DIR_SET_GID;
+
+		/* Copy read bits to execute bits */
+		mode |= (shared_repository & 0444) >> 2;
+	}
+
 	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
 		return -2;
 	return 0;
diff --git a/setup.c b/setup.c
index 3d2d958..0905509 100644
--- a/setup.c
+++ b/setup.c
@@ -430,6 +430,8 @@ int git_config_perm(const char *var, const char *value)
 {
 	if (value) {
 		int i;
+		char *endptr;
+
 		if (!strcmp(value, "umask"))
 			return PERM_UMASK;
 		if (!strcmp(value, "group"))
@@ -438,11 +440,46 @@ int git_config_perm(const char *var, const char *value)
 		    !strcmp(value, "world") ||
 		    !strcmp(value, "everybody"))
 			return PERM_EVERYBODY;
-		i = atoi(value);
-		if (i > 1)
-			return i;
+
+		/* Parse octal numbers */
+		i = strtol(value, &endptr, 8);
+		if (*endptr != 0) {
+			/* Not an octal number. Maybe true/false? */
+			if (git_config_bool(var, value))
+				return PERM_GROUP;
+			else
+				return PERM_UMASK;
+		}
+
+		/* Handle compatibility cases */
+		switch (i) {
+		case PERM_UMASK:               /* 0 */
+			return PERM_UMASK;
+		case OLD_PERM_GROUP:           /* 1 */
+			return PERM_GROUP;
+		case OLD_PERM_EVERYBODY:       /* 2 */
+			return PERM_EVERYBODY;
+		}
+
+		/* A filemode value was given: 0xxx */
+
+		if ((i & 0600) != 0600)
+			die("Problem with core.sharedRepository filemode value"
+			    " (0%.3o).\nThe owner of files must always have "
+			    "read and write permissions.", i);
+
+		if (i & 0002)
+			warning("core.sharedRepository filemode (0%.3o) is a "
+				"security threat.\nMasking off write "
+				"permission for others\n", i);
+
+		/*
+                 * Mask filemode value. Others can not get write permission.
+		 * x flags for directories are handled separately.
+                 */
+		return i & 0664;
 	}
-	return git_config_bool(var, value);
+	return PERM_GROUP;
 }
 
 int check_repository_format_version(const char *var, const char *value)
-- 
1.5.4.4

Re: [PATCH] Make core.sharedRepository more generic (version 2)

[Junio C Hamano]

Heikki Orsila <heikki.orsila@iki.fi> writes:

> diff --git a/builtin-init-db.c b/builtin-init-db.c
> index 2854868..8c63295 100644
> --- a/builtin-init-db.c
> +++ b/builtin-init-db.c
> @@ -400,9 +400,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  		char buf[10];
>  		/* We do not spell "group" and such, so that
>  		 * the configuration can be read by older version
> -		 * of git.
> +		 * of git. Note, we use octal numbers.
>  		 */
> -		sprintf(buf, "%d", shared_repository);
> +		sprintf(buf, "0%o", shared_repository);

Unconditionally doing this makes the resulting repository unusable by git
1.5.5 and older, even when the user wanted to use the bog standard "git
init --shared".  You can limit the extent of damage if you continue
writing PERM_GROUP and PERM_EVERYBODY out as 1 and 2, and use the new
octal notation only when the user used the settings allowed only with new
git.

> @@ -438,11 +440,46 @@ int git_config_perm(const char *var, const char *value)
>  		    !strcmp(value, "world") ||
>  		    !strcmp(value, "everybody"))
>  			return PERM_EVERYBODY;
> +
> +		/* Parse octal numbers */
> +		i = strtol(value, &endptr, 8);
> +		if (*endptr != 0) {
> +			/* Not an octal number. Maybe true/false? */
> +			if (git_config_bool(var, value))
> +				return PERM_GROUP;
> +			else
> +				return PERM_UMASK;
> +		}
> +
> +		/* Handle compatibility cases */
> +		switch (i) {
> +		case PERM_UMASK:               /* 0 */
> +			return PERM_UMASK;
> +		case OLD_PERM_GROUP:           /* 1 */
> +			return PERM_GROUP;
> +		case OLD_PERM_EVERYBODY:       /* 2 */
> +			return PERM_EVERYBODY;
> +		}

This is valid only because forcing "chmod 0", "chmod 1", nor "chmod 2"
would not make any sense.  We might want to explain that in comment.

> +		/* A filemode value was given: 0xxx */
> +
> +		if ((i & 0600) != 0600)
> +			die("Problem with core.sharedRepository filemode value"
> +			    " (0%.3o).\nThe owner of files must always have "
> +			    "read and write permissions.", i);
> +
> +		if (i & 0002)
> +			warning("core.sharedRepository filemode (0%.3o) is a "
> +				"security threat.\nMasking off write "
> +				"permission for others\n", i);

I am not sure about this.

If the user explicitly asked for world-writable, I think we should allow
it.  "is a" is too strong a statement to make without knowing how the
access to the repository is arranged.  In a setting where there is nothing
but a restricted access over ssh, and "update-paranoid" hook in place, it
may not be a threat at all, and you are forbidding hosting services to use
such an access control model.

> +		/*
> +                 * Mask filemode value. Others can not get write permission.
> +		 * x flags for directories are handled separately.
> +                 */
Whitespace breakages.

> +		return i & 0664;

>  	}
> -	return git_config_bool(var, value);
> +	return PERM_GROUP;

At this point we know value is NULL so always returning PERM_GROUP
probably makes sense.  But if you did

	if (!value)
	        return PERM_GROUP

upfront, you can lose one level of indentation from the major parts
of this function (this is 70% style and 30% readability comment).

Re: [PATCH] Make core.sharedRepository more generic (version 2)

[Heikki Orsila]

On Mon, Apr 14, 2008 at 05:08:03PM -0700, Junio C Hamano wrote:
> > +		 * of git. Note, we use octal numbers.
> >  		 */
> > -		sprintf(buf, "%d", shared_repository);
> > +		sprintf(buf, "0%o", shared_repository);
> 
> Unconditionally doing this makes the resulting repository unusable by git
> 1.5.5 and older, even when the user wanted to use the bog standard "git
> init --shared".  You can limit the extent of damage if you continue
> writing PERM_GROUP and PERM_EVERYBODY out as 1 and 2, and use the new
> octal notation only when the user used the settings allowed only with new
> git.

I submitted a new patch that should address all the points.

Heikki

Re: [PATCH] Make core.sharedRepository more generic (version 2)

[Heikki Orsila]

On Tue, Apr 15, 2008 at 03:43:10AM +0300, Heikki Orsila wrote:
> On Mon, Apr 14, 2008 at 05:08:03PM -0700, Junio C Hamano wrote:
> > > +		 * of git. Note, we use octal numbers.
> > >  		 */
> > > -		sprintf(buf, "%d", shared_repository);
> > > +		sprintf(buf, "0%o", shared_repository);
> > 
> > Unconditionally doing this makes the resulting repository unusable by git
> > 1.5.5 and older, even when the user wanted to use the bog standard "git
> > init --shared".  You can limit the extent of damage if you continue
> > writing PERM_GROUP and PERM_EVERYBODY out as 1 and 2, and use the new
> > octal notation only when the user used the settings allowed only with new
> > git.
> 
> I submitted a new patch that should address all the points.

Submitted another version that really fixes the o+w case. Forgot 
the 0664 mask there. Now it's 0666.

-- 
Heikki Orsila
heikki.orsila@iki.fi
http://www.iki.fi/shd