From patchwork Fri Nov 16 08:47:25 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10685717 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 462D31709 for ; Fri, 16 Nov 2018 08:47:30 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 34BFF28D53 for ; Fri, 16 Nov 2018 08:47:30 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 266C528FDE; Fri, 16 Nov 2018 08:47:30 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 99DCB28D53 for ; Fri, 16 Nov 2018 08:47:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727742AbeKPS6t (ORCPT ); Fri, 16 Nov 2018 13:58:49 -0500 Received: from cloud.peff.net ([104.130.231.41]:41812 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1727398AbeKPS6t (ORCPT ); Fri, 16 Nov 2018 13:58:49 -0500 Received: (qmail 29127 invoked by uid 109); 16 Nov 2018 08:47:27 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Fri, 16 Nov 2018 08:47:27 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 24914 invoked by uid 111); 16 Nov 2018 08:46:48 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Fri, 16 Nov 2018 03:46:48 -0500 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Fri, 16 Nov 2018 03:47:25 -0500 Date: Fri, 16 Nov 2018 03:47:25 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 1/3] remote-curl: refactor smart-http discovery Message-ID: <20181116084725.GA31603@sigill.intra.peff.net> References: <20181116084427.GA31493@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181116084427.GA31493@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP After making initial contact with an http server, we have to decide if the server supports smart-http, and if so, which version. Our rules are a bit inconsistent: 1. For v0, we require that the content-type indicates a smart-http response. We also require the response to look vaguely like a pkt-line starting with "#". If one of those does not match, we fall back to dumb-http. But according to our http protocol spec[1]: Dumb servers MUST NOT return a return type starting with `application/x-git-`. If we see the expected content-type, we should consider it smart-http. At that point we can parse the pkt-line for real, and complain if it is not syntactically valid. 2. For v2, we do not actually check the content-type. Our v2 protocol spec says[2]: When using the http:// or https:// transport a client makes a "smart" info/refs request as described in `http-protocol.txt`[...] and the http spec is clear that for a smart-http[3]: The Content-Type MUST be `application/x-$servicename-advertisement`. So it is required according to the spec. These inconsistencies were easy to miss because of the way the original code was written as an inline conditional. Let's pull it out into its own function for readability, and improve a few things: - we now predicate the smart/dumb decision entirely on the presence of the correct content-type - we do a real pkt-line parse before deciding how to proceed (and die if it isn't valid) - use skip_prefix() for comparing service strings, instead of constructing expected output in a strbuf; this avoids dealing with memory cleanup Note that this _is_ tightening what the client will allow. It's all according to the spec, but it's possible that other implementations might violate these. However, violating these particular rules seems like an odd choice for a server to make. [1] Documentation/technical/http-protocol.txt, l. 166-167 [2] Documentation/technical/protocol-v2.txt, l. 63-64 [3] Documentation/technical/http-protocol.txt, l. 247 Helped-by: Josh Steadmon Signed-off-by: Jeff King Reviewed-by: Josh Steadmon --- remote-curl.c | 93 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 34 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 762a55a75f..dd9bc41aa1 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -330,9 +330,65 @@ static int get_protocol_http_header(enum protocol_version version, return 0; } +static void check_smart_http(struct discovery *d, const char *service, + struct strbuf *type) +{ + char *src_buf; + size_t src_len; + char *line; + const char *p; + + /* + * If we don't see x-$service-advertisement, then it's not smart-http. + * But once we do, we commit to it and assume any other protocol + * violations are hard errors. + */ + if (!skip_prefix(type->buf, "application/x-", &p) || + !skip_prefix(p, service, &p) || + strcmp(p, "-advertisement")) + return; + + /* + * "Peek" at the first packet by using a separate buf/len pair; some + * cases below require us leaving the originals intact. + */ + src_buf = d->buf; + src_len = d->len; + line = packet_read_line_buf(&src_buf, &src_len, NULL); + if (!line) + die("invalid server response; expected service, got flush packet"); + + if (skip_prefix(line, "# service=", &p) && !strcmp(p, service)) { + /* + * The header can include additional metadata lines, up + * until a packet flush marker. Ignore these now, but + * in the future we might start to scan them. + */ + while (packet_read_line_buf(&src_buf, &src_len, NULL)) + ; + + /* + * v0 smart http; callers expect us to soak up the + * service and header packets + */ + d->buf = src_buf; + d->len = src_len; + d->proto_git = 1; + + } else if (starts_with(line, "version 2")) { + /* + * v2 smart http; do not consume version packet, which will + * be handled elsewhere. + */ + d->proto_git = 1; + + } else { + die("invalid server response; got '%s'", line); + } +} + static struct discovery *discover_refs(const char *service, int for_push) { - struct strbuf exp = STRBUF_INIT; struct strbuf type = STRBUF_INIT; struct strbuf charset = STRBUF_INIT; struct strbuf buffer = STRBUF_INIT; @@ -405,38 +461,8 @@ static struct discovery *discover_refs(const char *service, int for_push) last->buf_alloc = strbuf_detach(&buffer, &last->len); last->buf = last->buf_alloc; - strbuf_addf(&exp, "application/x-%s-advertisement", service); - if (maybe_smart && - (5 <= last->len && last->buf[4] == '#') && - !strbuf_cmp(&exp, &type)) { - char *line; - - /* - * smart HTTP response; validate that the service - * pkt-line matches our request. - */ - line = packet_read_line_buf(&last->buf, &last->len, NULL); - if (!line) - die("invalid server response; expected service, got flush packet"); - - strbuf_reset(&exp); - strbuf_addf(&exp, "# service=%s", service); - if (strcmp(line, exp.buf)) - die("invalid server response; got '%s'", line); - strbuf_release(&exp); - - /* The header can include additional metadata lines, up - * until a packet flush marker. Ignore these now, but - * in the future we might start to scan them. - */ - while (packet_read_line_buf(&last->buf, &last->len, NULL)) - ; - - last->proto_git = 1; - } else if (maybe_smart && - last->len > 5 && starts_with(last->buf + 4, "version 2")) { - last->proto_git = 1; - } + if (maybe_smart) + check_smart_http(last, service, &type); if (last->proto_git) last->refs = parse_git_refs(last, for_push); @@ -444,7 +470,6 @@ static struct discovery *discover_refs(const char *service, int for_push) last->refs = parse_info_refs(last); strbuf_release(&refs_url); - strbuf_release(&exp); strbuf_release(&type); strbuf_release(&charset); strbuf_release(&effective_url); From patchwork Fri Nov 16 08:48:38 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10685719 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id C9EC513BF for ; Fri, 16 Nov 2018 08:48:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B999B28C75 for ; Fri, 16 Nov 2018 08:48:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id AC20E2CB08; Fri, 16 Nov 2018 08:48:42 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5D56928C75 for ; Fri, 16 Nov 2018 08:48:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727568AbeKPTAC (ORCPT ); Fri, 16 Nov 2018 14:00:02 -0500 Received: from cloud.peff.net ([104.130.231.41]:41818 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1727398AbeKPTAC (ORCPT ); Fri, 16 Nov 2018 14:00:02 -0500 Received: (qmail 29201 invoked by uid 109); 16 Nov 2018 08:48:40 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Fri, 16 Nov 2018 08:48:40 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 24930 invoked by uid 111); 16 Nov 2018 08:48:01 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Fri, 16 Nov 2018 03:48:01 -0500 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Fri, 16 Nov 2018 03:48:38 -0500 Date: Fri, 16 Nov 2018 03:48:38 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 2/3] remote-curl: tighten "version 2" check for smart-http Message-ID: <20181116084838.GB31603@sigill.intra.peff.net> References: <20181116084427.GA31493@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181116084427.GA31493@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP In a v2 smart-http conversation, the server should reply to our initial request with a pkt-line saying "version 2" (this is the start of the "capabilities advertisement"). We check for the string using starts_with(), but that's overly permissive (it would match "version 20", for example). Let's tighten this check to use strcmp(). Note that we don't need to worry about a trailing newline here, because the ptk-line code will have chomped it for us already. Signed-off-by: Jeff King Reviewed-by: Josh Steadmon --- remote-curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index dd9bc41aa1..3c9c4a07c3 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -375,7 +375,7 @@ static void check_smart_http(struct discovery *d, const char *service, d->len = src_len; d->proto_git = 1; - } else if (starts_with(line, "version 2")) { + } else if (!strcmp(line, "version 2")) { /* * v2 smart http; do not consume version packet, which will * be handled elsewhere. From patchwork Fri Nov 16 08:49:51 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10685721 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 31F3E13BF for ; Fri, 16 Nov 2018 08:49:56 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 20A072D674 for ; Fri, 16 Nov 2018 08:49:56 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 14FB22D666; Fri, 16 Nov 2018 08:49:56 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id ED0D42D312 for ; Fri, 16 Nov 2018 08:49:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2389395AbeKPTBP (ORCPT ); Fri, 16 Nov 2018 14:01:15 -0500 Received: from cloud.peff.net ([104.130.231.41]:41824 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1727567AbeKPTBO (ORCPT ); Fri, 16 Nov 2018 14:01:14 -0500 Received: (qmail 29248 invoked by uid 109); 16 Nov 2018 08:49:52 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Fri, 16 Nov 2018 08:49:52 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 25222 invoked by uid 111); 16 Nov 2018 08:49:14 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Fri, 16 Nov 2018 03:49:14 -0500 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Fri, 16 Nov 2018 03:49:51 -0500 Date: Fri, 16 Nov 2018 03:49:51 -0500 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 3/3] remote-curl: die on server-side errors Message-ID: <20181116084951.GC31603@sigill.intra.peff.net> References: <20181116084427.GA31493@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181116084427.GA31493@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Josh Steadmon When a smart HTTP server sends an error message via pkt-line, remote-curl will fail to detect the error (which usually results in incorrectly falling back to dumb-HTTP mode). This patch adds a check in check_smart_http() for server-side error messages, as well as a test case for this issue. Signed-off-by: Josh Steadmon Signed-off-by: Jeff King --- remote-curl.c | 3 +++ t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 4 ++++ t/lib-httpd/error-smart-http.sh | 3 +++ t/t5551-http-fetch-smart.sh | 5 +++++ 5 files changed, 16 insertions(+) create mode 100644 t/lib-httpd/error-smart-http.sh diff --git a/remote-curl.c b/remote-curl.c index 3c9c4a07c3..b1309f2bdc 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -382,6 +382,9 @@ static void check_smart_http(struct discovery *d, const char *service, */ d->proto_git = 1; + } else if (skip_prefix(line, "ERR ", &p)) { + die(_("remote error: %s"), p); + } else { die("invalid server response; got '%s'", line); } diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index a8729f8232..4e946623bb 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -131,6 +131,7 @@ prepare_httpd() { mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH" cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH" install_script broken-smart-http.sh + install_script error-smart-http.sh install_script error.sh install_script apply-one-time-sed.sh diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 581c010d8f..6de2bc775c 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/ ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1 ScriptAlias /broken_smart/ broken-smart-http.sh/ +ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1 @@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1 Options ExecCGI + + Options ExecCGI + Options ExecCGI diff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh new file mode 100644 index 0000000000..e65d447fc4 --- /dev/null +++ b/t/lib-httpd/error-smart-http.sh @@ -0,0 +1,3 @@ +echo "Content-Type: application/x-git-upload-pack-advertisement" +echo +printf "%s" "0019ERR server-side error" diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 8630b0cc39..ba83e567e5 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' ' ! grep "=> Send data" err ' +test_expect_success 'server-side error detected' ' + test_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual && + grep "server-side error" actual +' + stop_httpd test_done