{"thread":{"id":"48871","subject":"[PATCH] RFC: submodule-config: introduce trust level","startedAt":"2018-07-12T19:05:08Z","lastAt":"2018-07-12T20:54:33Z","messageCount":2,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"352387","messageId":"20180712190456.29337-1-sbeller@google.com","threadId":"48871","inReplyTo":null,"subject":"[PATCH] RFC: submodule-config: introduce trust level","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-12T19:04:56Z","receivedAt":"2018-07-12T19:05:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The series merged at 614ea03a71e (Merge branch\n'bw/submodule-config-cleanup', 2017-08-26) went to great length to make it\nexplicit to the caller where a value came from, as that would help the\ncaller to be careful to decide which values to take from where, i.e. be\ncareful about security implications.\n\nIn practice we always want to stack the settings starting with the\n.gitmodules file as a base and then put the config on top of it.\nSo let's manage the security aspects impolitely in the submodule-config\nmachinery directly where we implement its parsing as there is a good\nplace to reason about the trust that we need to put into a parsed value.\n\nThis patch implements the trust level that is passed to the parsing,'\ncurrently we only pass in the 'in_repo' level of trust, which is the\n.gitmodules file.\n\nFollow up patches could add other sources that populate the submodule\nconfig again.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n This is on top of ao/config-from-gitmodules.\n\n submodule-config.c | 31 ++++++++++++++++++++++++++++---\n 1 file changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 77421a49719..09eab9f00e0 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -21,6 +21,11 @@ struct submodule_cache {\n \tunsigned gitmodules_read:1;\n };\n \n+enum submodule_config_trust_level {\n+\tin_repo = 1,\n+\tconfigured_by_user = 2\n+};\n+\n /*\n  * thin wrapper struct needed to insert 'struct submodule' entries to\n  * the hashmap\n@@ -387,12 +392,14 @@ struct parse_config_parameter {\n \tstruct submodule_cache *cache;\n \tconst struct object_id *treeish_name;\n \tconst struct object_id *gitmodules_oid;\n+\tenum submodule_config_trust_level source;\n \tint overwrite;\n };\n \n static int parse_config(const char *var, const char *value, void *data)\n {\n \tstruct parse_config_parameter *me = data;\n+\tenum submodule_config_trust_level source = me->source;\n \tstruct submodule *submodule;\n \tstruct strbuf name = STRBUF_INIT, item = STRBUF_INIT;\n \tint ret = 0;\n@@ -406,6 +413,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\t\t\t     name.buf);\n \n \tif (!strcmp(item.buf, \"path\")) {\n+\t\t/* all sources allowed */\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->path)\n@@ -419,6 +427,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n \t} else if (!strcmp(item.buf, \"fetchrecursesubmodules\")) {\n+\t\t/* all sources allowed */\n \t\t/* when parsing worktree configurations we can die early */\n \t\tint die_on_error = is_null_oid(me->gitmodules_oid);\n \t\tif (!me->overwrite &&\n@@ -430,6 +439,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\t\t\t\t\t\tvar, value,\n \t\t\t\t\t\t\t\tdie_on_error);\n \t} else if (!strcmp(item.buf, \"ignore\")) {\n+\t\t/* all sources allowed */\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite && submodule->ignore)\n@@ -446,6 +456,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n \t} else if (!strcmp(item.buf, \"url\")) {\n+\t\t/* all sources allowed */\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n \t\t} else if (!me->overwrite && submodule->url) {\n@@ -456,16 +467,27 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n \t} else if (!strcmp(item.buf, \"update\")) {\n+\t\tstruct submodule_update_strategy st;\n \t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n \t\telse if (!me->overwrite &&\n \t\t\t submodule->update_strategy.type != SM_UPDATE_UNSPECIFIED)\n \t\t\twarn_multiple_config(me->treeish_name, submodule->name,\n \t\t\t\t\t     \"update\");\n-\t\telse if (parse_submodule_update_strategy(value,\n-\t\t\t &submodule->update_strategy) < 0)\n-\t\t\t\tdie(_(\"invalid value for %s\"), var);\n+\t\telse if (parse_submodule_update_strategy(value, &st) < 0)\n+\t\t\tdie(_(\"invalid value for %s\"), var);\n+\t\telse if (source <= in_repo) {\n+\t\t\tif (st.type == SM_UPDATE_COMMAND) {\n+\t\t\t\tsubmodule->update_strategy.type = st.type;\n+\t\t\t\tsubmodule->update_strategy.command = \\\n+\t\t\t\t\"!echo Not trusting command in submodule.<name>.update\";\n+\t\t\t} else {\n+\t\t\t\tsubmodule->update_strategy.type = st.type;\n+\t\t\t\tsubmodule->update_strategy.command = st.command;\n+\t\t\t}\n+\t\t}\n \t} else if (!strcmp(item.buf, \"shallow\")) {\n+\t\t/* all sources allowed */\n \t\tif (!me->overwrite && submodule->recommend_shallow != -1)\n \t\t\twarn_multiple_config(me->treeish_name, submodule->name,\n \t\t\t\t\t     \"shallow\");\n@@ -473,6 +495,7 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tsubmodule->recommend_shallow =\n \t\t\t\tgit_config_bool(var, value);\n \t} else if (!strcmp(item.buf, \"branch\")) {\n+\t\t/* all sources allowed */\n \t\tif (!me->overwrite && submodule->branch)\n \t\t\twarn_multiple_config(me->treeish_name, submodule->name,\n \t\t\t\t\t     \"branch\");\n@@ -560,6 +583,7 @@ static const struct submodule *config_from(struct submodule_cache *cache,\n \tparameter.treeish_name = treeish_name;\n \tparameter.gitmodules_oid = &oid;\n \tparameter.overwrite = 0;\n+\tparameter.source = in_repo;\n \tgit_config_from_mem(parse_config, CONFIG_ORIGIN_SUBMODULE_BLOB, rev.buf,\n \t\t\tconfig, config_size, &parameter);\n \tstrbuf_release(&rev);\n@@ -617,6 +641,7 @@ static int gitmodules_cb(const char *var, const char *value, void *data)\n \tparameter.treeish_name = NULL;\n \tparameter.gitmodules_oid = &null_oid;\n \tparameter.overwrite = 1;\n+\tparameter.source = in_repo;\n \n \treturn parse_config(var, value, &parameter);\n }\n-- \n2.18.0.203.gfac676dfb9-goog\n\n"},{"id":"352436","messageId":"xmqqmuuwuqd7.fsf@gitster-ct.c.googlers.com","threadId":"48871","inReplyTo":"20180712190456.29337-1-sbeller@google.com","subject":"Re: [PATCH] RFC: submodule-config: introduce trust level","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T20:54:28Z","receivedAt":"2018-07-12T20:54:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> In practice we always want to stack the settings starting with the\n> .gitmodules file as a base and then put the config on top of it.\n\nCould you substantiate this claim with justification?  Thanks.\n"}]}