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

Commit Message

Linus Arver via GitGitGadget Feb. 26, 2020, 11:23 p.m. UTC

From: Jorge Lopez Silva <[e-mail]>

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 <[e-mail]>
---
 Documentation/config/http.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Junio C Hamano Feb. 27, 2020, 6:58 p.m. UTC | #1

"Jorge Lopez Silva via GitGitGadget" <[e-mail]>
writes:

> From: Jorge Lopez Silva <[e-mail]>
>
> 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 <[e-mail]>
> ---

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

> +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?

> +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

Jorge A López Silva March 3, 2020, 1:47 a.m. UTC | #2

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

Haha, yeah, you're right. I'll collapse the commits into a single one.

>  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).

Good point. Better descriptions and names will be added.

> 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?


Excellent point. Will adjust to re-use the same pattern.


On Thu, Feb 27, 2020 at 10:58 AM Junio C Hamano <[e-mail]> wrote:
>
> "Jorge Lopez Silva via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Jorge Lopez Silva <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
>
> Thanks, this should be part of the previous patch, as it was that
> commit, not this one, that adds 4 options ;-)
>
> > +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?
>
> > +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

Patch

diff --git a/Documentation/config/http.txt b/Documentation/config/http.txt
index e806033aab8..7e704687e87 100644
--- a/Documentation/config/http.txt
+++ b/Documentation/config/http.txt
@@ -29,6 +29,20 @@  http.proxyAuthMethod::
 * `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
 --
 
+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.
+
+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.
+
+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