Re: [PATCH v2 2/2] config: documentation for HTTPS proxy client cert.

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2020, 18:58 UTC
Message-ID: <xmqqblpjg8mf.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <c40207a3928f9cbc490b9ef2e99e7cba7788e7c0.1582759438.git.gitgitgadget@gmail.com>

"Jorge Lopez Silva via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

> From: Jorge Lopez Silva <jalopezsilva@gmail.com>
>
> The commit adds 4 options, client cert, key, key password and CA info.
> The CA info can be used to specify a different CA path to validate the
> HTTPS proxy cert.
>
> Signed-off-by: Jorge Lopez Silva <jalopezsilva@gmail.com>
> ---

Thanks, this should be part of the previous patch, as it was that commit, not this one, that adds 4 options ;-)

Show 5 quoted lines

> +http.proxycert::
> +	File indicating a client certificate to use to authenticate with an HTTPS proxy.
> +
> +http.proxykey::
> +	File indicating a private key to use to authenticate with an HTTPS proxy.

I think these files not merely "indicate" but they themselves "hold", "contain" and/or "store" the certificate and key. Perhaps more like...

	The pathname of a file that stores a client certificate to ...

Also, it is customary to camelCase the configuration variable names. As I understand http.proxykey is roughly corresponds to existing http.sslKey (the former is for proxy, the latter is for the target host), I'd expect these two to be spelled http.proxySSLCert and http.proxySSLKey respectively (without omitting "SSL", as that is the underlying cURL option name if I am reading the code in 1/2 correctly).

> +http.proxykeypass::
> +	When communicating to the proxy using TLS (using an HTTPS proxy), use this
> +	option along `http.proxykey` to indicate a password for the key.

And this would be "http.proxyKeyPasswd" for the same two reasons.

There are a couple of curious things, though:

 * Is it a good idea to use a keyfile that is encrypted, but leave
   the encryption password on disk in the configuration file to
   begin with?

 * This teaches our system about PROXY_KEYPASSWD that protects
   PROXY_SSLKEY, but why isn't there a similar configuration
   variable for CURLOPT_KEYPASSWD that protects CURLOPT_SSLKEY?

It is possible that the answer to these questions are the same---an on-disk password is a bad idea, so we deliberately omit a config that gives value to CURLOPT_KEYPASSWD and instead use the credential subsystem (see http.c::has_cert_password() and its caller). If so, I think it would be prudent to follow the same pattern if possible?

Show 7 quoted lines

> +http.proxycainfo::
> +	File containing the certificates to verify the proxy with when using an HTTPS
> +	proxy.
> +
>  http.emptyAuth::
>  	Attempt authentication without seeking a username or password.  This
>  	can be used to attempt GSS-Negotiate authentication without specifying