diff mbox series

[v2] http: document sslcert and sslkey types and extend to proxy

Message ID pull.1520.v2.git.1682014322604.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2] http: document sslcert and sslkey types and extend to proxy | expand

Commit Message

Ricky Davidson April 20, 2023, 6:12 p.m. UTC 
From: Ricky Davidson <Ricky.Davidson@hii-tsd.com>

0a01d41 added http.sslCertType and http.sslKeyType, but:

1. does not document the feature.
2. does not apply to SSL proxy equivalents.

Documents http.sslCertType and http.sslKeyType. Implements
http.proxySSLCertType. Same for http.sslKeyType and
http.proxySSLKeyType equivalents and related environment
variables.

Signed-off-by: Ricky Davidson <Ricky.Davidson@hii-tsd.com>
---
    [PATCH] http: document sslcert and sslkey types and extend to proxy
    
    0a01d41ee4ca7f8afb75219f46f4f1c573465075 wonderfully added
    http.sslCertType and http.sslKeyType, but has a couple problems:
    
     1. does not document the feature.
     2. does not apply to SSL proxy equivalents.
    
    Documents http.sslCertType and http.sslKeyType. Implements
    http.proxySSLCertType. Same for http.sslKeyType and http.proxySSLKeyType
    equivalents and related environment variables.
    
    Signed-off-by: Ricky Davidson Ricky.Davidson@hii-tsd.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1520%2FRicky-Davidson-hii-tsd%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1520/Ricky-Davidson-hii-tsd/master-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1520

Range-diff vs v1:

 1:  711d90215d0 ! 1:  78b96f60ec8 http: document sslcert and sslkey types and extend to proxy
     @@ Documentation/config/http.txt: http.proxySSLCert::
      +http.proxySSLKeyType::
      +	Format of the client private key used to authenticate with an HTTPS proxy.
      +	Supported formats are `PEM` and `ENG`. The format `ENG` enables loading from
     -+	a crypto engine. Can be overridden by the `GIT_PROXY_SSL_CERT_TYPE` environment
     ++	a crypto engine. Can be overridden by the `GIT_PROXY_SSL_KEY_TYPE` environment
      +	variable.
      +
       http.proxySSLCertPasswordProtected::
     @@ Documentation/config/http.txt: http.sslCert::
      +http.sslCertType::
      +	Format of the SSL certificate used to authenticate over HTTPS.
      +	Supported formats are `PEM` and `ENG`. The format `ENG` enables loading from
     -+	a crypto engine. Can be overridden by the `GIT_PROXY_SSL_CERT_TYPE` environment
     ++	a crypto engine. Can be overridden by the `GIT_SSL_CERT_TYPE` environment
      +	variable.
      +
       http.sslKey::
     @@ Documentation/config/http.txt: http.sslCert::
      +http.sslKeyType::
      +	Format of the SSL private key used to authenticate over HTTPS.
      +	Supported formats are `PEM` and `ENG`. The format `ENG` enables loading from
     -+	a crypto engine. Can be overridden by the `GIT_PROXY_SSL_CERT_TYPE` environment
     ++	a crypto engine. Can be overridden by the `GIT_SSL_CERT_TYPE` environment
      +	variable.
      +
       http.sslCertPasswordProtected::


 Documentation/config/http.txt | 24 ++++++++++++++++++++++++
 http.c                        | 12 ++++++++++++
 2 files changed, 36 insertions(+)


base-commit: 667fcf4e15379790f0b609d6a83d578e69f20301

Comments

Junio C Hamano April 20, 2023, 7:43 p.m. UTC | #1 
"Ricky Davidson via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ricky Davidson <Ricky.Davidson@hii-tsd.com>

I think it is the first time we see your patches around here.
Welcome to Git developer community.

> 0a01d41 added http.sslCertType and http.sslKeyType, but:
>
> 1. does not document the feature.
> 2. does not apply to SSL proxy equivalents.

The above description would read better to have "it" to serve as the
subject for the two sentences that point out rooms for improvement
of what the earlier commit did somewhere.  Perhaps between "but" and
the colon after it, e.g. "X did Y, but it: (1) did not do W, and (2)
did not do Z."  Alternatively, "X did Y, but: (1) it did not do
W. (2) it did not do Z." would also work.

The way we refer to an existing commit is:

	0a01d41e (http: add support for different sslcert and sslkey
	types., 2023-03-20) added ...

Running "git show --pretty=reference -s $commit" would give you a
properly formatted reference.

> Documents http.sslCertType and http.sslKeyType. Implements
> http.proxySSLCertType. Same for http.sslKeyType and
> http.proxySSLKeyType equivalents and related environment
> variables.

After explaining the status quo and talking about what we want to
improve, we write what we wanted the code to become with this patch
in imperative mood, as if we are giving an order to "become like
so", instead of third-person present tense.

I.e. something like "Document X and Y, and implement W and Z for
completeness.  Do the same for A and B."

Other than that, well-written in an understandable way.  Very nice.

>  Documentation/config/http.txt | 24 ++++++++++++++++++++++++
>  http.c                        | 12 ++++++++++++
>  2 files changed, 36 insertions(+)

I wonder if we can add some tests for the feature, though.

Thanks.
diff mbox series

Patch

diff --git a/Documentation/config/http.txt b/Documentation/config/http.txt
index afeeccfbfa7..53386b90185 100644
--- a/Documentation/config/http.txt
+++ b/Documentation/config/http.txt
@@ -34,11 +34,23 @@  http.proxySSLCert::
 	with an HTTPS proxy. Can be overridden by the `GIT_PROXY_SSL_CERT` environment
 	variable.
 
+http.proxySSLCertType::
+	Format of the client certificate used to authenticate with an HTTPS proxy.
+	Supported formats are `PEM` and `ENG`. The format `ENG` enables loading from
+	a crypto engine. Can be overridden by the `GIT_PROXY_SSL_CERT_TYPE` environment
+	variable.
+
 http.proxySSLKey::
 	The pathname of a file that stores a private key to use to authenticate with
 	an HTTPS proxy. Can be overridden by the `GIT_PROXY_SSL_KEY` environment
 	variable.
 
+http.proxySSLKeyType::
+	Format of the client private key used to authenticate with an HTTPS proxy.
+	Supported formats are `PEM` and `ENG`. The format `ENG` enables loading from
+	a crypto engine. Can be overridden by the `GIT_PROXY_SSL_KEY_TYPE` environment
+	variable.
+
 http.proxySSLCertPasswordProtected::
 	Enable Git's password prompt for the proxy SSL certificate.  Otherwise OpenSSL
 	will prompt the user, possibly many times, if the certificate or private key
@@ -161,11 +173,23 @@  http.sslCert::
 	over HTTPS. Can be overridden by the `GIT_SSL_CERT` environment
 	variable.
 
+http.sslCertType::
+	Format of the SSL certificate used to authenticate over HTTPS.
+	Supported formats are `PEM` and `ENG`. The format `ENG` enables loading from
+	a crypto engine. Can be overridden by the `GIT_SSL_CERT_TYPE` environment
+	variable.
+
 http.sslKey::
 	File containing the SSL private key when fetching or pushing
 	over HTTPS. Can be overridden by the `GIT_SSL_KEY` environment
 	variable.
 
+http.sslKeyType::
+	Format of the SSL private key used to authenticate over HTTPS.
+	Supported formats are `PEM` and `ENG`. The format `ENG` enables loading from
+	a crypto engine. Can be overridden by the `GIT_SSL_CERT_TYPE` environment
+	variable.
+
 http.sslCertPasswordProtected::
 	Enable Git's password prompt for the SSL certificate.  Otherwise
 	OpenSSL will prompt the user, possibly many times, if the
diff --git a/http.c b/http.c
index d5d82c5230f..bee4ea64115 100644
--- a/http.c
+++ b/http.c
@@ -74,7 +74,9 @@  static const char *curl_http_proxy;
 static const char *http_proxy_authmethod;
 
 static const char *http_proxy_ssl_cert;
+static const char *http_proxy_ssl_cert_type;
 static const char *http_proxy_ssl_key;
+static const char *http_proxy_ssl_key_type;
 static const char *http_proxy_ssl_ca_info;
 static struct credential proxy_cert_auth = CREDENTIAL_INIT;
 static int proxy_ssl_cert_password_required;
@@ -441,9 +443,13 @@  static int http_options(const char *var, const char *value, void *cb)
 
 	if (!strcmp("http.proxysslcert", var))
 		return git_config_string(&http_proxy_ssl_cert, var, value);
+	if (!strcmp("http.proxysslcerttype", var))
+		return git_config_string(&http_proxy_ssl_cert_type, var, value);
 
 	if (!strcmp("http.proxysslkey", var))
 		return git_config_string(&http_proxy_ssl_key, var, value);
+	if (!strcmp("http.proxysslkeytype", var))
+		return git_config_string(&http_proxy_ssl_key_type, var, value);
 
 	if (!strcmp("http.proxysslcainfo", var))
 		return git_config_string(&http_proxy_ssl_ca_info, var, value);
@@ -1146,9 +1152,13 @@  static CURL *get_curl_handle(void)
 
 			if (http_proxy_ssl_cert)
 				curl_easy_setopt(result, CURLOPT_PROXY_SSLCERT, http_proxy_ssl_cert);
+			if (http_proxy_ssl_cert_type)
+				curl_easy_setopt(result, CURLOPT_PROXY_SSLCERTTYPE, http_proxy_ssl_cert_type);
 
 			if (http_proxy_ssl_key)
 				curl_easy_setopt(result, CURLOPT_PROXY_SSLKEY, http_proxy_ssl_key);
+			if (http_proxy_ssl_key_type)
+				curl_easy_setopt(result, CURLOPT_PROXY_SSLKEYTYPE, http_proxy_ssl_key_type);
 
 			if (has_proxy_cert_password())
 				curl_easy_setopt(result, CURLOPT_PROXY_KEYPASSWD, proxy_cert_auth.password);
@@ -1285,7 +1295,9 @@  void http_init(struct remote *remote, const char *url, int proactive_auth)
 		max_requests = DEFAULT_MAX_REQUESTS;
 
 	set_from_env(&http_proxy_ssl_cert, "GIT_PROXY_SSL_CERT");
+	set_from_env(&http_proxy_ssl_cert_type, "GIT_PROXY_SSL_CERT_TYPE");
 	set_from_env(&http_proxy_ssl_key, "GIT_PROXY_SSL_KEY");
+	set_from_env(&http_proxy_ssl_key_type, "GIT_PROXY_SSL_KEY_TYPE");
 	set_from_env(&http_proxy_ssl_ca_info, "GIT_PROXY_SSL_CAINFO");
 
 	if (getenv("GIT_PROXY_SSL_CERT_PASSWORD_PROTECTED"))