On 12/14, Brandon Williams wrote:
> On 12/14, Jeff King wrote:
> > 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.
>
> I'll fix the commit message to better discuss the reasoning behind the
> change.
>
> > > Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>
> > > ---
> > > 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.
>
> K I'll add this in as a test.
>
> > > @@ -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.
>
> Well, that's unfortunate! It does sound like a more full-proof solution
> to these dumb http alternates could be involved. I don't think your
> simple "lazy" solution may be enough to not just die by default.
>
> By default ftp/ftps will have a policy of "user only" which means they
> will be set by the call to get_curl_allowed_protocol(-1) but not set by
> get_curl_allowed_protocol(0). This would result in the call to
> check_alternates_protocol_restrictions failing all the time unless the
> user explicitly sets ftp/ftps to "always" or "never". If that is the
> desired behavior then your proposed solution would be fine, otherwise we
> may have to do the more involved approach.
Naively looking at the code (and your longer suggestion), is there a
reason why we couldn't simply have http-walker set CURLOPT_PROTOCOLS
with get_curl_allowed_protocols(0) in the fetch_alternates() function?
That way we just override the CURLOPT_PROTOCOLS value when alternates
are involved.
Like so:
diff --git a/http-walker.c b/http-walker.c
index c2f81cd..b284cec 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -339,6 +339,8 @@ static void fetch_alternates(struct walker *walker, const char *base)
curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
curl_easy_setopt(slot->curl, CURLOPT_URL, url.buf);
+ curl_easy_setopt(slot->curl, CURLOPT_PROTOCOLS,
+ get_curl_allowed_protocols(0));
alt_req.base = base;
alt_req.url = &url;
--
Brandon Williams