Re: [PATCH] transport: add core.allowProtocol config option

[Jeff King <peff@peff.net>]

On Thu, Nov 03, 2016 at 10:25:15AM -0700, Brandon Williams wrote:

> > So I think this probably needs to be a separate parallel system where
> > each protocol can be white- or black-listed in a context-specific way.
> > Like:
> > 
> >   protocol.X.allow = always | user | never
> 
> It sounds like there is interest for this sort of behavior, it would
> definitely require a larger change than what I initially proposed.  One
> problem I see though is that with this we have support for both a
> blacklist and a whitelist.  Which wins?  Or do we simply generate a
> whitelist of allowed protocols which includes all protocols with allow
> set to 'always' and if it is set to 'never' then it just isn't included
> in the whitelist?

I think trying to combine the two or generate the whitelist from the
more flexible format is a recipe for madness.

I'd design the new system from scratch, and have it kick in _only_ when
GIT_ALLOW_PROTOCOL is not set. That lets existing callers continue to
have the safe behavior until they are ready to move to the new format.

Something like the patch below (which is just for illustration, and not
tested beyond compilation).

> I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> because I view a whitelist or blacklist as well black and white and
> having this user state adds in a gray area.

The lack of understanding the "user" context is what makes the current
system so painful. The only way a caller can influence the system is to
hand over the policy directly: this is allowed, this is not. But systems
like "go get" should not be setting policy. They should be giving us a
hint about the context, and letting git implement the policy.

-- >8 --
diff --git a/transport.c b/transport.c
index d57e8dec2..0d5b382db 100644
--- a/transport.c
+++ b/transport.c
@@ -664,10 +664,69 @@ static const struct string_list *protocol_whitelist(void)
 	return enabled ? &allowed : NULL;
 }
 
+enum protocol_allow_config {
+	PROTOCOL_ALLOW_NEVER,
+	PROTOCOL_ALLOW_USER_ONLY,
+	PROTOCOL_ALLOW_ALWAYS
+};
+
+static enum protocol_allow_config parse_protocol_config(const char *key,
+							const char *value)
+{
+	if (!strcasecmp(value, "always"))
+		return PROTOCOL_ALLOW_ALWAYS;
+	else if (!strcasecmp(value, "never"))
+		return PROTOCOL_ALLOW_NEVER;
+	else if (!strcasecmp(value, "user"))
+		return PROTOCOL_ALLOW_USER_ONLY;
+
+	/* XXX maybe also interpret git_config_bool() here? */
+	die("unknown value for config '%s': %s", key, value);
+}
+
+static enum protocol_allow_config get_protocol_config(const char *type)
+{
+	char *key = xstrfmt("protocol.%s.allow", type);
+	char *value;
+
+	if (!git_config_get_string(key, &value)) {
+		enum protocol_allow_config ret =
+			parse_protocol_config(key, value);
+		free(key);
+		free(value);
+		return ret;
+	}
+	free(key);
+
+	/* known safe */
+	if (!strcmp(type, "http") ||
+	    !strcmp(type, "https") ||
+	    !strcmp(type, "git") ||
+	    !strcmp(type, "ssh"))
+		return PROTOCOL_ALLOW_ALWAYS;
+
+	/* known scary; err on the side of caution */
+	if (!strcmp(type, "ext"))
+		return PROTOCOL_ALLOW_NEVER;
+
+	/* unknown; let them be used only directly by the user */
+	return PROTOCOL_ALLOW_USER_ONLY;
+}
+
 int is_transport_allowed(const char *type)
 {
-	const struct string_list *allowed = protocol_whitelist();
-	return !allowed || string_list_has_string(allowed, type);
+	const struct string_list *whitelist = protocol_whitelist();
+	if (whitelist)
+		return string_list_has_string(whitelist, type);
+
+	switch (get_protocol_config(type)) {
+	case PROTOCOL_ALLOW_ALWAYS:
+		return 1;
+	case PROTOCOL_ALLOW_NEVER:
+		return 0;
+	case PROTOCOL_ALLOW_USER_ONLY:
+		return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+	}
 }
 
 void transport_check_allowed(const char *type)