> 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 <gitster@xxxxxxxxx> wrote: > > "Jorge Lopez Silva via GitGitGadget" <gitgitgadget@xxxxxxxxx> > writes: > > > From: Jorge Lopez Silva <jalopezsilva@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > --- > > 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
