Re: [PATCH v4] http: add http.sslVerifyStatus to check stapled OCSP responses

graysongordon-gl <[email protected]> writes:

> Verified: t5551 over plain http and t5559 over https both pass all 61
> tests, with the four new ones skipping on the former and running on the
> latter.
>  Documentation/config/http.adoc | 17 +++++++++++++++++
>  http.c                         | 10 ++++++++++
>  t/t5551-http-fetch-smart.sh    | 29 +++++++++++++++++++++++++++++
>  3 files changed, 56 insertions(+)

OK, instead of adding a new test script that weighs 72-line we are
testing the feature with 29-line addition, which sounds like a good
economy ;-).

The code changes and the documentation haven't changed since the
previous round, both looking good.

Will replace.  Thanks.

> diff --git a/Documentation/config/http.adoc b/Documentation/config/http.adoc
> index 792a71b413..40b849bf7f 100644
> --- a/Documentation/config/http.adoc
> +++ b/Documentation/config/http.adoc
> @@ -196,6 +196,23 @@ http.sslVerify::
>  	over HTTPS. Defaults to true. Can be overridden by the
>  	`GIT_SSL_NO_VERIFY` environment variable.
>  
> +http.sslVerifyStatus::
> +	Whether to check the revocation status of the server
> +	certificate using the stapled OCSP response supplied during
> +	the TLS handshake ("OCSP stapling"). Defaults to false.
> ++
> +This is fail-closed: if the server staples no response, verification
> +fails. Set it per remote, e.g.
> +`http.https://example.com/.sslVerifyStatus`, rather than globally.
> ++
> +What it changes depends on the TLS backend libcurl was built against.
> +An OpenSSL-linked build ignores a stapled response unless this is set.
> +A GnuTLS-linked build consults the staple during ordinary certificate
> +verification, so it already rejects a revoked certificate under
> +`http.sslVerify` alone, and setting this to `false` does not disable
> +that. Where a backend cannot check the staple at all, git fails with an
> +error rather than continuing unchecked.
> +
>  http.sslCert::
>  	File containing the SSL certificate when fetching or pushing
>  	over HTTPS. Can be overridden by the `GIT_SSL_CERT` environment
> diff --git a/http.c b/http.c
> index caccf2108e..94f8dd817a 100644
> --- a/http.c
> +++ b/http.c
> @@ -44,6 +44,7 @@ static CURL *curl_default;
>  char curl_errorstr[CURL_ERROR_SIZE];
>  
>  static int curl_ssl_verify = -1;
> +static int curl_ssl_verify_status;
>  static int curl_ssl_try;
>  static char *curl_http_version;
>  static char *ssl_cert;
> @@ -400,6 +401,10 @@ static int http_options(const char *var, const char *value,
>  		curl_ssl_verify = git_config_bool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp("http.sslverifystatus", var)) {
> +		curl_ssl_verify_status = git_config_bool(var, value);
> +		return 0;
> +	}
>  	if (!strcmp("http.sslcipherlist", var))
>  		return git_config_string(&ssl_cipherlist, var, value);
>  	if (!strcmp("http.sslversion", var))
> @@ -1133,6 +1138,11 @@ static CURL *get_curl_handle(void)
>  		curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2L);
>  	}
>  
> +	if (curl_ssl_verify_status &&
> +	    curl_easy_setopt(result, CURLOPT_SSL_VERIFYSTATUS, 1L) != CURLE_OK)
> +		die(_("http.sslVerifyStatus is set, but the TLS backend of "
> +		      "this libcurl cannot verify certificate status"));
> +
>      if (curl_http_version) {
>  		long opt;
>  		if (!get_curl_http_version_opt(curl_http_version, &opt)) {
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 805bec025c..c11e96c1ac 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -680,6 +680,35 @@ test_expect_success 'passing hostname resolution information works' '
>  	git -c "http.curloptResolve=$BOGUS_HOST:$LIB_HTTPD_PORT:127.0.0.1" ls-remote "$BOGUS_HTTPD_URL/smart/repo.git" >/dev/null
>  '
>  
> +test_lazy_prereq SSL_VERIFYSTATUS '
> +	test "$HTTPD_PROTO" = "https" &&
> +	test_might_fail git -c http.sslVerifyStatus=true \
> +		ls-remote "$HTTPD_URL/smart/repo.git" 2>err &&
> +	! grep "cannot verify certificate status" err
> +'
> +
> +test_expect_success SSL_VERIFYSTATUS 'http.sslVerifyStatus=true fails without a staple' '
> +	test_must_fail git -c http.sslVerifyStatus=true \
> +		ls-remote "$HTTPD_URL/smart/repo.git"
> +'
> +
> +test_expect_success SSL_VERIFYSTATUS 'http.sslVerifyStatus=false is a no-op' '
> +	git -c http.sslVerifyStatus=false \
> +		ls-remote "$HTTPD_URL/smart/repo.git" >actual &&
> +	test_line_count -gt 0 actual
> +'
> +
> +test_expect_success SSL_VERIFYSTATUS 'per-URL sslVerifyStatus applies to a matching URL' '
> +	test_must_fail git -c "http.$HTTPD_URL/.sslVerifyStatus=true" \
> +		ls-remote "$HTTPD_URL/smart/repo.git"
> +'
> +
> +test_expect_success SSL_VERIFYSTATUS 'per-URL sslVerifyStatus is not applied to other URLs' '
> +	git -c "http.https://example.com/.sslVerifyStatus=true" \
> +		ls-remote "$HTTPD_URL/smart/repo.git" >actual &&
> +	test_line_count -gt 0 actual
> +'
> +
>  # here user%40host is the URL-encoded version of user@host,
>  # which is our intentionally-odd username to catch parsing errors
>  url_user=$HTTPD_URL_USER/auth/smart/repo.git