[PATCH] RFC: submodule-config: introduce trust level

[PATCH] RFC: submodule-config: introduce trust level

[Stefan Beller]

The series merged at 614ea03a71e (Merge branch
'bw/submodule-config-cleanup', 2017-08-26) went to great length to make it
explicit to the caller where a value came from, as that would help the
caller to be careful to decide which values to take from where, i.e. be
careful about security implications.

In practice we always want to stack the settings starting with the
.gitmodules file as a base and then put the config on top of it.
So let's manage the security aspects impolitely in the submodule-config
machinery directly where we implement its parsing as there is a good
place to reason about the trust that we need to put into a parsed value.

This patch implements the trust level that is passed to the parsing,'
currently we only pass in the 'in_repo' level of trust, which is the
.gitmodules file.

Follow up patches could add other sources that populate the submodule
config again.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 This is on top of ao/config-from-gitmodules.

 submodule-config.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 77421a49719..09eab9f00e0 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -21,6 +21,11 @@ struct submodule_cache {
 	unsigned gitmodules_read:1;
 };
 
+enum submodule_config_trust_level {
+	in_repo = 1,
+	configured_by_user = 2
+};
+
 /*
  * thin wrapper struct needed to insert 'struct submodule' entries to
  * the hashmap
@@ -387,12 +392,14 @@ struct parse_config_parameter {
 	struct submodule_cache *cache;
 	const struct object_id *treeish_name;
 	const struct object_id *gitmodules_oid;
+	enum submodule_config_trust_level source;
 	int overwrite;
 };
 
 static int parse_config(const char *var, const char *value, void *data)
 {
 	struct parse_config_parameter *me = data;
+	enum submodule_config_trust_level source = me->source;
 	struct submodule *submodule;
 	struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
 	int ret = 0;
@@ -406,6 +413,7 @@ static int parse_config(const char *var, const char *value, void *data)
 					     name.buf);
 
 	if (!strcmp(item.buf, "path")) {
+		/* all sources allowed */
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->path)
@@ -419,6 +427,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			cache_put_path(me->cache, submodule);
 		}
 	} else if (!strcmp(item.buf, "fetchrecursesubmodules")) {
+		/* all sources allowed */
 		/* when parsing worktree configurations we can die early */
 		int die_on_error = is_null_oid(me->gitmodules_oid);
 		if (!me->overwrite &&
@@ -430,6 +439,7 @@ static int parse_config(const char *var, const char *value, void *data)
 								var, value,
 								die_on_error);
 	} else if (!strcmp(item.buf, "ignore")) {
+		/* all sources allowed */
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite && submodule->ignore)
@@ -446,6 +456,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			submodule->ignore = xstrdup(value);
 		}
 	} else if (!strcmp(item.buf, "url")) {
+		/* all sources allowed */
 		if (!value) {
 			ret = config_error_nonbool(var);
 		} else if (!me->overwrite && submodule->url) {
@@ -456,16 +467,27 @@ static int parse_config(const char *var, const char *value, void *data)
 			submodule->url = xstrdup(value);
 		}
 	} else if (!strcmp(item.buf, "update")) {
+		struct submodule_update_strategy st;
 		if (!value)
 			ret = config_error_nonbool(var);
 		else if (!me->overwrite &&
 			 submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)
 			warn_multiple_config(me->treeish_name, submodule->name,
 					     "update");
-		else if (parse_submodule_update_strategy(value,
-			 &submodule->update_strategy) < 0)
-				die(_("invalid value for %s"), var);
+		else if (parse_submodule_update_strategy(value, &st) < 0)
+			die(_("invalid value for %s"), var);
+		else if (source <= in_repo) {
+			if (st.type == SM_UPDATE_COMMAND) {
+				submodule->update_strategy.type = st.type;
+				submodule->update_strategy.command = \
+				"!echo Not trusting command in submodule.<name>.update";
+			} else {
+				submodule->update_strategy.type = st.type;
+				submodule->update_strategy.command = st.command;
+			}
+		}
 	} else if (!strcmp(item.buf, "shallow")) {
+		/* all sources allowed */
 		if (!me->overwrite && submodule->recommend_shallow != -1)
 			warn_multiple_config(me->treeish_name, submodule->name,
 					     "shallow");
@@ -473,6 +495,7 @@ static int parse_config(const char *var, const char *value, void *data)
 			submodule->recommend_shallow =
 				git_config_bool(var, value);
 	} else if (!strcmp(item.buf, "branch")) {
+		/* all sources allowed */
 		if (!me->overwrite && submodule->branch)
 			warn_multiple_config(me->treeish_name, submodule->name,
 					     "branch");
@@ -560,6 +583,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,
 	parameter.treeish_name = treeish_name;
 	parameter.gitmodules_oid = &oid;
 	parameter.overwrite = 0;
+	parameter.source = in_repo;
 	git_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,
 			config, config_size, &parameter);
 	strbuf_release(&rev);
@@ -617,6 +641,7 @@ static int gitmodules_cb(const char *var, const char *value, void *data)
 	parameter.treeish_name = NULL;
 	parameter.gitmodules_oid = &null_oid;
 	parameter.overwrite = 1;
+	parameter.source = in_repo;
 
 	return parse_config(var, value, &parameter);
 }
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH] RFC: submodule-config: introduce trust level

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> In practice we always want to stack the settings starting with the
> .gitmodules file as a base and then put the config on top of it.

Could you substantiate this claim with justification?  Thanks.