Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed

[Jeff King <peff@peff.net>]

On Tue, Dec 13, 2016 at 05:40:37PM -0800, Brandon Williams wrote:

> Add the from_user parameter to the 'is_transport_allowed' function.
> This allows callers to query if a transport protocol is allowed, given
> that the caller knows that the protocol is coming from the user (1) or
> not from the user (0) such as redirects in libcurl.  If unknown a -1
> should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
> to determine if the protocol came from the user.

I think your commit message is upside-down with respect to the purpose
of the patch. The end goal we want is for http to distinguish between
protocol restrictions for redirects versus initial requests. The rest is
an implementation detail. It's definitely still worth discussing that
implementation detail (though I think your in-code comments may be
sufficient), but I don't see the rationale discussed here at all.

> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  http.c      | 14 +++++++-------
>  transport.c |  8 +++++---
>  transport.h | 13 ++++++++++---
>  3 files changed, 22 insertions(+), 13 deletions(-)

I'm trying to think of a way to test this. I guess the case we are
covering here is when a server redirects, but the protocol is only
allowed from the user. So:

diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 044cc152f..d911afd24 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -30,5 +30,12 @@ test_expect_success 'curl limits redirects' '
 	test_must_fail git clone "$HTTPD_URL/loop-redir/smart/repo.git"
 '
 
+test_expect_success 'http can be limited to from-user' '
+	git -c protocol.http.allow=user \
+		clone "$HTTPD_URL/smart/repo.git" plain.git &&
+	test_must_fail git -c protocol.http.allow=user \
+		clone "$HTTPD_URL/smart-redir-perm/repo.git" redir.git
+'
+
 stop_httpd
 test_done

It's an oddball configuration, and you'd probably just set
http.followRedirects=false in practice, but it does correctly check this
case.

> @@ -588,9 +588,9 @@ static CURL *get_curl_handle(void)
>  #endif
>  #if LIBCURL_VERSION_NUM >= 0x071304
>  	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> -			 get_curl_allowed_protocols());
> +			 get_curl_allowed_protocols(0));
>  	curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> -			 get_curl_allowed_protocols());
> +			 get_curl_allowed_protocols(-1));

This covers internal redirects done by libcurl, but not the dumb-walker
http-alternates nonsense. We have to feed the URL from http-alternates
back to curl ourselves, so it uses CURLOPT_PROTOCOLS even though it
should count as "not from the user".

To fix that, I think we'd need something like:

  - get_curl_handle() stops setting these options, as it is done only
    once when the curl handle is initialized. Instead, the protocol
    restrictions should go into get_active_slot(), which is called for
    each request.  The values set would remain the same, and be the
    baseline.

  - the http-walker.c code would need to know when it's requesting from
    the base URL, and when it's an alternate. I think this would depend
    on the position of the "alt" in in the linked list it keeps.

  - when requesting from an alternate, http-walker would set
    CURLOPT_PROTOCOLS with get_curl_allowed_protocols(0)

I have to admit that it sounds like a fair bit of work for a pretty
obscure case. You'd have to:

  1. Turn http.allowRedirects to "true", to allow redirects even for
     non-initial contact.

  2. Turn one of protocol.{http,https,ftp,ftps}.allow to "user" to
     restrict it from being used in a redirect.

I'm tempted to punt on it and just do:

  if (http_follow_config == HTTP_FOLLOW_ALWAYS &&
      get_curl_allowed_protocols(0) != get_curl_allowed_protocols(-1))
	die("user-only protocol restrictions not implemented for http-alternates");

which errs on the safe side. We could even shove that down into the case
where we actually see some alternates, like:

diff --git a/http-walker.c b/http-walker.c
index c2f81cd6a..5bcc850b1 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -160,6 +160,12 @@ static void prefetch(struct walker *walker, unsigned char *sha1)
 #endif
 }
 
+static void check_alternates_protocol_restrictions(void)
+{
+	if (get_curl_allowed_protocols(0) != get_curl_allowed_protocol(-1))
+		die("user-only protocol restrictions not implemented for http alternates");
+}
+
 static void process_alternates_response(void *callback_data)
 {
 	struct alternates_request *alt_req =
@@ -272,6 +278,7 @@ static void process_alternates_response(void *callback_data)
 			/* skip "objects\n" at end */
 			if (okay) {
 				struct strbuf target = STRBUF_INIT;
+				check_alternates_protocol_restrictions();
 				strbuf_add(&target, base, serverlen);
 				strbuf_add(&target, data + i, posn - i - 7);
 				warning("adding alternate object store: %s",

I find it unlikely that anybody would ever care, but at least we'd do
the safe thing. I dunno. Maybe I am just being lazy.

-Peff