Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HTTPS proxy SSL options (cert, key, cainfo) #559

Closed

Conversation

jalopezsilva
Copy link

@jalopezsilva jalopezsilva commented Feb 20, 2020

Git currently supports connecting to proxies through HTTPS. However it does not allow you to configure SSL options when connecting (i.e. client cert, key, cainfo). These set of commits add the necessary options and documentation needed to support them.

Libcurl already has support for this so changes are somewhat minimal.

I ran the CI tests and verified manually with an HTTPS proxy that changes are working as expected. I didn't see integration tests under /t or tests that verified libcurl integration.

./bin-wrappers/git -c http.proxy=https://<PROXY-HOSTNAME> \
-c http.proxycert=<CERT> -c http.proxykey=<KEY> \
clone https://github.com/jalopezsilva/dotfiles.git  

Changes since v2:

  • Merged the two initial commits as the second one was adding documentation for the first.
  • Removed the SSL Cert password from configuration. I'm using a similar function to has_cert_password to retrieve it if needed.
  • Better names and descriptions were given to the options.
  • Introduced another commit adding environment variable overrides for the new options.

@jalopezsilva jalopezsilva changed the title Https proxy ssl options Add HTTPS proxy SSL options (cert, key, cainfo) Feb 20, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Feb 20, 2020

Welcome to GitGitGadget

Hi @jalopezsilva, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

@harry-hov
Copy link

/allow

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 21, 2020

User jalopezsilva is now allowed to use GitGitGadget.

WARNING: jalopezsilva has no public email address set on GitHub

@jalopezsilva
Copy link
Author

/preview

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 21, 2020

Preview email sent as pull.559.git.1582248035.gitgitgadget@gmail.com

@jalopezsilva
Copy link
Author

/preview

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 21, 2020

Preview email sent as pull.559.git.1582320738.gitgitgadget@gmail.com

@jalopezsilva
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 21, 2020

@@ -86,6 +86,14 @@ static long curl_low_speed_time = -1;
static int curl_ftp_no_epsv;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Fri, Feb 21, 2020 at 4:37 PM Jorge Lopez Silva via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Git currently supports performing connections to HTTPS proxies but we
> don't support doing mutual authentication with them (through TLS). This
> commit adds the necessary options to be able to send a client
> certificate to the HTTPS proxy.
> [...]
> Signed-off-by: Jorge Lopez Silva <jalopezsilva@gmail.com>
> ---
> diff --git a/http.c b/http.c
> @@ -1018,9 +1046,23 @@ static CURL *get_curl_handle(void)
>  #if LIBCURL_VERSION_NUM >= 0x073400
> -               else if (starts_with(curl_http_proxy, "https"))
> +               else if (starts_with(curl_http_proxy, "https")) {
>                         curl_easy_setopt(result,
>                                 CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
> +
> +                       if (http_proxy_ssl_cert != NULL) {
> +                               curl_easy_setopt(result,
> +                                       CURLOPT_PROXY_SSLCERT, http_proxy_ssl_cert);
> +                               }
> +                       if (http_proxy_ssl_key != NULL) {
> +                               curl_easy_setopt(result,
> +                                       CURLOPT_PROXY_SSLKEY, http_proxy_ssl_key);
> +                               }
> +                       if (http_proxy_ssl_key_passwd != NULL) {
> +                               curl_easy_setopt(result,
> +                                       CURLOPT_PROXY_KEYPASSWD, http_proxy_ssl_key_passwd);
> +                               }
> +                       }
>  #endif

All the closing braces in this hunk seem to be over-indented. Also,
all of the braces for the one-liner 'if' bodies can be dropped, thus
making it less noisy.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jorge A López Silva wrote (reply to this):

Thanks Eric for the feedback. I'm addressing your comments and sending a v2.


On Fri, Feb 21, 2020 at 2:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Feb 21, 2020 at 4:37 PM Jorge Lopez Silva via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Git currently supports performing connections to HTTPS proxies but we
> > don't support doing mutual authentication with them (through TLS). This
> > commit adds the necessary options to be able to send a client
> > certificate to the HTTPS proxy.
> > [...]
> > Signed-off-by: Jorge Lopez Silva <jalopezsilva@gmail.com>
> > ---
> > diff --git a/http.c b/http.c
> > @@ -1018,9 +1046,23 @@ static CURL *get_curl_handle(void)
> >  #if LIBCURL_VERSION_NUM >= 0x073400
> > -               else if (starts_with(curl_http_proxy, "https"))
> > +               else if (starts_with(curl_http_proxy, "https")) {
> >                         curl_easy_setopt(result,
> >                                 CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
> > +
> > +                       if (http_proxy_ssl_cert != NULL) {
> > +                               curl_easy_setopt(result,
> > +                                       CURLOPT_PROXY_SSLCERT, http_proxy_ssl_cert);
> > +                               }
> > +                       if (http_proxy_ssl_key != NULL) {
> > +                               curl_easy_setopt(result,
> > +                                       CURLOPT_PROXY_SSLKEY, http_proxy_ssl_key);
> > +                               }
> > +                       if (http_proxy_ssl_key_passwd != NULL) {
> > +                               curl_easy_setopt(result,
> > +                                       CURLOPT_PROXY_KEYPASSWD, http_proxy_ssl_key_passwd);
> > +                               }
> > +                       }
> >  #endif
>
> All the closing braces in this hunk seem to be over-indented. Also,
> all of the braces for the one-liner 'if' bodies can be dropped, thus
> making it less noisy.

@jalopezsilva
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 26, 2020

@@ -86,6 +86,14 @@ static long curl_low_speed_time = -1;
static int curl_ftp_no_epsv;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> +#if LIBCURL_VERSION_NUM >= 0x073400
> +static const char *http_proxy_ssl_cert;
> +static const char *http_proxy_ssl_key;
> +static const char *http_proxy_ssl_keypasswd;
> +#endif
> +static const char *http_proxy_ssl_ca_info;
> +
>  static struct {
>  	const char *name;
>  	long curlauth_param;
> @@ -365,6 +373,20 @@ static int http_options(const char *var, const char *value, void *cb)
>  	if (!strcmp("http.proxyauthmethod", var))
>  		return git_config_string(&http_proxy_authmethod, var, value);
>  
> +#if LIBCURL_VERSION_NUM >= 0x073400
> +	if (!strcmp("http.proxycert", var))
> +		return git_config_string(&http_proxy_ssl_cert, var, value);
> +
> +	if (!strcmp("http.proxykey", var))
> +		return git_config_string(&http_proxy_ssl_key, var, value);
> +
> +	if (!strcmp("http.proxykeypass", var))
> +		return git_config_string(&http_proxy_ssl_keypasswd, var, value);
> +
> +	if (!strcmp("http.proxycainfo", var))
> +		return git_config_string(&http_proxy_ssl_ca_info, var, value);
> +#endif

You may copy around your ~/.gitconfig to multiple hosts, some may
have newer and others may have older versions of libcurl, so it
would be OK for a version of Git built with older libcurl to at
least see and parse configurations meant for newer one, if only
to ignore and discard.

The only two effects these #if/#endif have are (1) they save a tiny
bit of memory, code and runtime cycle on an older platform and (2)
they make the resuting code ugly and harder to read.  I do not think
that the tradeoff is worth it.

>  	if (!strcmp("http.cookiefile", var))
>  		return git_config_pathname(&curl_cookie_file, var, value);
>  	if (!strcmp("http.savecookies", var)) {
> @@ -924,8 +946,14 @@ static CURL *get_curl_handle(void)
>  #if LIBCURL_VERSION_NUM >= 0x073400
>  		curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL);
>  #endif
> -	} else if (ssl_cainfo != NULL)
> -		curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
> +	} else if (ssl_cainfo != NULL || http_proxy_ssl_ca_info != NULL) {
> +		if (ssl_cainfo != NULL)
> +			curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
> +#if LIBCURL_VERSION_NUM >= 0x073400
> +		if (http_proxy_ssl_ca_info != NULL)
> +			curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, http_proxy_ssl_ca_info);
> +#endif
> +	}

On this codepath, unlike the config and variable definitions,
#if/#endif is absolutely necessary.

In any case, the code around here is messy, but it is mostly due to
the fact that the existing #if/#endif with if/elseif/... cascade was
messy.  The general idea is

 * We want to honor ssl_cainfo and http_proxy_ssl_ca_info, and use
   CAINFO when set, but

 * When http_schannel_use_ssl_cainfo is not in effect and
   http_ssl_backend is schannel, ssl_cainfo/http_proxy_ssl_ca_info
   business is completely skipped, and these two CAINFO are cleared
   instead.

I do not know if the above is the best code structure to express
that, but at least the way this patch adds code is the least noisy,
I guess.

> @@ -1018,9 +1046,19 @@ static CURL *get_curl_handle(void)
>  				CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
>  #endif
>  #if LIBCURL_VERSION_NUM >= 0x073400
> -		else if (starts_with(curl_http_proxy, "https"))
> -			curl_easy_setopt(result,
> -				CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
> +		else if (starts_with(curl_http_proxy, "https")) {
> +			curl_easy_setopt(result, CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
> +
> +			if (http_proxy_ssl_cert != NULL)
> +				curl_easy_setopt(result, CURLOPT_PROXY_SSLCERT, http_proxy_ssl_cert);
> +
> +			if (http_proxy_ssl_key != NULL)
> +				curl_easy_setopt(result, CURLOPT_PROXY_SSLKEY, http_proxy_ssl_key);
> +
> +			if (http_proxy_ssl_keypasswd != NULL)
> +				curl_easy_setopt(result, CURLOPT_PROXY_KEYPASSWD, http_proxy_ssl_keypasswd);

This part is more or less straight-forward.

This is a minor tangent, but I see many "var != NULL" instances used
as the condition to if statements, which we tend to frown upon
(instead just say "if (var) ...").  I know there are already many in
the existing code in this file, but this patch is making it even
worse.

> +		}
>  #endif
>  		if (strstr(curl_http_proxy, "://"))
>  			credential_from_url(&proxy_auth, curl_http_proxy);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jorge A López Silva wrote (reply to this):

> You may copy around your ~/.gitconfig to multiple hosts, some may
> have newer and others may have older versions of libcurl, so it
> would be OK for a version of Git built with older libcurl to at
> least see and parse configurations meant for newer one, if only
> to ignore and discard.
> The only two effects these #if/#endif have are (1) they save a tiny
> bit of memory, code and runtime cycle on an older platform and (2)
> they make the resuting code ugly and harder to read.  I do not think
> that the tradeoff is worth it.

I agree, thanks for the input. I'll remove the #if/#endif from the variables.

>  This part is more or less straight-forward.
> This is a minor tangent, but I see many "var != NULL" instances used
> as the condition to if statements, which we tend to frown upon
> (instead just say "if (var) ...").  I know there are already many in
> the existing code in this file, but this patch is making it even
> worse.

Understood, will fix!


On Thu, Feb 27, 2020 at 10:31 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Jorge Lopez Silva via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +#if LIBCURL_VERSION_NUM >= 0x073400
> > +static const char *http_proxy_ssl_cert;
> > +static const char *http_proxy_ssl_key;
> > +static const char *http_proxy_ssl_keypasswd;
> > +#endif
> > +static const char *http_proxy_ssl_ca_info;
> > +
> >  static struct {
> >       const char *name;
> >       long curlauth_param;
> > @@ -365,6 +373,20 @@ static int http_options(const char *var, const char *value, void *cb)
> >       if (!strcmp("http.proxyauthmethod", var))
> >               return git_config_string(&http_proxy_authmethod, var, value);
> >
> > +#if LIBCURL_VERSION_NUM >= 0x073400
> > +     if (!strcmp("http.proxycert", var))
> > +             return git_config_string(&http_proxy_ssl_cert, var, value);
> > +
> > +     if (!strcmp("http.proxykey", var))
> > +             return git_config_string(&http_proxy_ssl_key, var, value);
> > +
> > +     if (!strcmp("http.proxykeypass", var))
> > +             return git_config_string(&http_proxy_ssl_keypasswd, var, value);
> > +
> > +     if (!strcmp("http.proxycainfo", var))
> > +             return git_config_string(&http_proxy_ssl_ca_info, var, value);
> > +#endif
>
> You may copy around your ~/.gitconfig to multiple hosts, some may
> have newer and others may have older versions of libcurl, so it
> would be OK for a version of Git built with older libcurl to at
> least see and parse configurations meant for newer one, if only
> to ignore and discard.
>
> The only two effects these #if/#endif have are (1) they save a tiny
> bit of memory, code and runtime cycle on an older platform and (2)
> they make the resuting code ugly and harder to read.  I do not think
> that the tradeoff is worth it.
>
> >       if (!strcmp("http.cookiefile", var))
> >               return git_config_pathname(&curl_cookie_file, var, value);
> >       if (!strcmp("http.savecookies", var)) {
> > @@ -924,8 +946,14 @@ static CURL *get_curl_handle(void)
> >  #if LIBCURL_VERSION_NUM >= 0x073400
> >               curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL);
> >  #endif
> > -     } else if (ssl_cainfo != NULL)
> > -             curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
> > +     } else if (ssl_cainfo != NULL || http_proxy_ssl_ca_info != NULL) {
> > +             if (ssl_cainfo != NULL)
> > +                     curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
> > +#if LIBCURL_VERSION_NUM >= 0x073400
> > +             if (http_proxy_ssl_ca_info != NULL)
> > +                     curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, http_proxy_ssl_ca_info);
> > +#endif
> > +     }
>
> On this codepath, unlike the config and variable definitions,
> #if/#endif is absolutely necessary.
>
> In any case, the code around here is messy, but it is mostly due to
> the fact that the existing #if/#endif with if/elseif/... cascade was
> messy.  The general idea is
>
>  * We want to honor ssl_cainfo and http_proxy_ssl_ca_info, and use
>    CAINFO when set, but
>
>  * When http_schannel_use_ssl_cainfo is not in effect and
>    http_ssl_backend is schannel, ssl_cainfo/http_proxy_ssl_ca_info
>    business is completely skipped, and these two CAINFO are cleared
>    instead.
>
> I do not know if the above is the best code structure to express
> that, but at least the way this patch adds code is the least noisy,
> I guess.
>
> > @@ -1018,9 +1046,19 @@ static CURL *get_curl_handle(void)
> >                               CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
> >  #endif
> >  #if LIBCURL_VERSION_NUM >= 0x073400
> > -             else if (starts_with(curl_http_proxy, "https"))
> > -                     curl_easy_setopt(result,
> > -                             CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
> > +             else if (starts_with(curl_http_proxy, "https")) {
> > +                     curl_easy_setopt(result, CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
> > +
> > +                     if (http_proxy_ssl_cert != NULL)
> > +                             curl_easy_setopt(result, CURLOPT_PROXY_SSLCERT, http_proxy_ssl_cert);
> > +
> > +                     if (http_proxy_ssl_key != NULL)
> > +                             curl_easy_setopt(result, CURLOPT_PROXY_SSLKEY, http_proxy_ssl_key);
> > +
> > +                     if (http_proxy_ssl_keypasswd != NULL)
> > +                             curl_easy_setopt(result, CURLOPT_PROXY_KEYPASSWD, http_proxy_ssl_keypasswd);
>
> This part is more or less straight-forward.
>
> This is a minor tangent, but I see many "var != NULL" instances used
> as the condition to if statements, which we tend to frown upon
> (instead just say "if (var) ...").  I know there are already many in
> the existing code in this file, but this patch is making it even
> worse.
>
> > +             }
> >  #endif
> >               if (strstr(curl_http_proxy, "://"))
> >                       credential_from_url(&proxy_auth, curl_http_proxy);

@@ -29,6 +29,20 @@ http.proxyAuthMethod::
* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

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

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jorge A López Silva wrote (reply to this):

> 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@pobox.com> wrote:
>
> "Jorge Lopez Silva via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > 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 ;-)
>
> > +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

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 27, 2020

This patch series was integrated into pu via git@96b76b3.

@gitgitgadget gitgitgadget bot added the pu label Feb 27, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Feb 28, 2020

This branch is now known as js/https-proxy-config.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 28, 2020

This patch series was integrated into pu via git@9bb1026.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 28, 2020

This patch series was integrated into pu via git@f65a706.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 2, 2020

This patch series was integrated into pu via git@e9c90f8.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 3, 2020

This patch series was integrated into pu via git@186cdc7.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 3, 2020

This patch series was integrated into pu via git@652847d.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 4, 2020

There is an issue in commit 5fd52a8f9351fba6c1cb698877c0e6feeba75ea8:
Commit not signed off

@jalopezsilva
Copy link
Author

/preview

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 4, 2020

Preview email sent as pull.559.v3.git.1583346941.gitgitgadget@gmail.com

@jalopezsilva
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 4, 2020

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 5, 2020

This patch series was integrated into pu via git@b887870.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 6, 2020

This patch series was integrated into pu via git@8f389f3.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 9, 2020

This patch series was integrated into pu via git@3ae2e55.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 9, 2020

This patch series was integrated into next via git@8a06f85.

@gitgitgadget gitgitgadget bot added the next label Mar 9, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Mar 10, 2020

This patch series was integrated into pu via git@c7a38b1.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 11, 2020

This patch series was integrated into pu via git@03edde9.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 12, 2020

This patch series was integrated into pu via git@efa9197.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 15, 2020

This patch series was integrated into pu via git@c1a037b.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 15, 2020

This patch series was integrated into pu via git@1fe3d05.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 16, 2020

This patch series was integrated into pu via git@dbe6391.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 18, 2020

This patch series was integrated into pu via git@e788364.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 18, 2020

This patch series was integrated into pu via git@312f09c.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 20, 2020

This patch series was integrated into pu via git@5bc7d3f.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 21, 2020

This patch series was integrated into pu via git@40befed.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 21, 2020

This patch series was integrated into pu via git@bfcf969.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 23, 2020

This patch series was integrated into pu via git@9669c1a.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into pu via git@efbc080.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into pu via git@7414833.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into pu via git@aaa6255.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into next via git@aaa6255.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

This patch series was integrated into master via git@aaa6255.

@gitgitgadget gitgitgadget bot added the master label Mar 25, 2020
@gitgitgadget gitgitgadget bot closed this Mar 25, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented Mar 25, 2020

Closed via aaa6255.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants