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

graysongordon-gl <[email protected]> writes:

> CURLOPT_SSL_VERIFYSTATUS has been available since libcurl 7.41.0, well
> below the 7.61.0 floor documented in INSTALL, so no version guard is
> needed.

Good to see that the author paid extra attention to compatibility.

> +	/*
> +	 * Ask the TLS backend to check the certificate's revocation
> +	 * status via the stapled OCSP response. libcurl defaults this
> +	 * off, and no backend except GnuTLS consults the staple on its
> +	 * own, so without this git will happily accept a certificate
> +	 * whose own staple says it has been revoked.
> +	 *
> +	 * Off by default because it is fail-closed: a server that
> +	 * staples nothing fails verification outright, so enabling it
> +	 * globally would break every remote that does not staple.
> +	 */

The comment may not be telling any lies per se, but it is dubious
that this belongs here as an in-code comment.  Developers hunting a
bug they suspect this setting might have caused will need access to
this information, and they can access it by running 'git blame' to
locate the commit that introduced the code.  As long as a solid
commit log message explains how you arrived at various design
decisions (such as 'off by default because'), they can use that as a
starting point.  For other developers hunting different bugs or
trying to add their own enhancements, the comment is a mere
distraction.

> +	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"));
> +


Thanks.