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

> 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.