{"thread":{"id":"59415","subject":"[PATCH] That change for support different sslcert and sslkey types.","startedAt":"2023-03-19T13:51:21Z","lastAt":"2023-03-30T00:20:59Z","messageCount":16,"participants":["Stanislav Malishevskiy via GitGitGadget","Jeff King","Junio C Hamano","Stanislav M"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"473729","messageId":"pull.1474.git.git.1679233875803.gitgitgadget@gmail.com","threadId":"59415","inReplyTo":null,"subject":"[PATCH] That change for support different sslcert and sslkey types.","fromName":"Stanislav Malishevskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-19T13:51:15Z","receivedAt":"2023-03-19T13:51:21Z","isPatch":true,"sender":{"key":"name:Stanislav Malishevskiy","avatar":null},"body":"From: Stanislav Malishevskiy \n\nBasically git work with default curl ssl type - PEM. But for support\neTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type\nand as sslkey type. So there added additional options for http for make\nthat possible.\n\nSigned-off-by: Stanislav Malishevskiy stanislav.malishevskiy@gmail.com\n---\n That change for support different sslcert and sslkey types.\n \n Basically git work with default curl ssl type - PEM. But for support\n eTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type\n and as sslkey type. So there added additional options for http for make\n that possible.\n \n Signed-off-by: Stanislav Malishevskiy stanislav.malishevskiy@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1474%2Fsmalishevskiy%2Fssl_types_support-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1474/smalishevskiy/ssl_types_support-v1\nPull-Request: https://github.com/git/git/pull/1474\n\n http.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex dbe4d29ef7a..d5d82c5230f 100644\n--- a/http.c\n+++ b/http.c\n@@ -40,6 +40,7 @@ static int curl_ssl_verify = -1;\n static int curl_ssl_try;\n static const char *curl_http_version = NULL;\n static const char *ssl_cert;\n+static const char *ssl_cert_type;\n static const char *ssl_cipherlist;\n static const char *ssl_version;\n static struct {\n@@ -59,6 +60,7 @@ static struct {\n #endif\n };\n static const char *ssl_key;\n+static const char *ssl_key_type;\n static const char *ssl_capath;\n static const char *curl_no_proxy;\n #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY\n@@ -374,8 +376,12 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&ssl_version, var, value);\n \tif (!strcmp(\"http.sslcert\", var))\n \t\treturn git_config_pathname(&ssl_cert, var, value);\n+\tif (!strcmp(\"http.sslcerttype\", var))\n+\t\treturn git_config_string(&ssl_cert_type, var, value);\n \tif (!strcmp(\"http.sslkey\", var))\n \t\treturn git_config_pathname(&ssl_key, var, value);\n+\tif (!strcmp(\"http.sslkeytype\", var))\n+\t\treturn git_config_string(&ssl_key_type, var, value);\n \tif (!strcmp(\"http.sslcapath\", var))\n \t\treturn git_config_pathname(&ssl_capath, var, value);\n \tif (!strcmp(\"http.sslcainfo\", var))\n@@ -1014,10 +1020,14 @@ static CURL *get_curl_handle(void)\n \n \tif (ssl_cert)\n \t\tcurl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n+\tif (ssl_cert_type)\n+\t\tcurl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type);\n \tif (has_cert_password())\n \t\tcurl_easy_setopt(result, CURLOPT_KEYPASSWD, cert_auth.password);\n \tif (ssl_key)\n \t\tcurl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);\n+\tif (ssl_key_type)\n+\t\tcurl_easy_setopt(result, CURLOPT_SSLKEYTYPE, ssl_key_type);\n \tif (ssl_capath)\n \t\tcurl_easy_setopt(result, CURLOPT_CAPATH, ssl_capath);\n #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY\n@@ -1252,7 +1262,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \t\tcurl_ssl_verify = 0;\n \n \tset_from_env(&ssl_cert, \"GIT_SSL_CERT\");\n+\tset_from_env(&ssl_cert_type, \"GIT_SSL_CERT_TYPE\");\n \tset_from_env(&ssl_key, \"GIT_SSL_KEY\");\n+\tset_from_env(&ssl_key_type, \"GIT_SSL_KEY_TYPE\");\n \tset_from_env(&ssl_capath, \"GIT_SSL_CAPATH\");\n \tset_from_env(&ssl_cainfo, \"GIT_SSL_CAINFO\");\n \n\nbase-commit: 950264636c68591989456e3ba0a5442f93152c1a\n-- \ngitgitgadget\n"},{"id":"473774","messageId":"pull.1474.v2.git.git.1679327330032.gitgitgadget@gmail.com","threadId":"59415","inReplyTo":"pull.1474.git.git.1679233875803.gitgitgadget@gmail.com","subject":"[PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Stanislav Malishevskiy via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-20T15:48:49Z","receivedAt":"2023-03-20T15:57:57Z","isPatch":true,"sender":{"key":"name:Stanislav Malishevskiy","avatar":null},"body":"From: Stanislav Malishevskiy \n\nBasically git work with default curl ssl type - PEM. But for support\neTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type\nand as sslkey type. So there added additional options for http to make\nthat possible.\n\nSigned-off-by: Stanislav Malishevskiy \n---\n http: add support for different sslcert and sslkey types.\n \n Basically git work with default curl ssl type - PEM. But for support\n eTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type\n and as sslkey type. So there added additional options for http to make\n that possible.\n \n Signed-off-by: Stanislav Malishevskiy stanislav.malishevskiy@gmail.com\n \n Changes since v1:\n \n * Fixed the commit message (found by Khalid Masum)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1474%2Fsmalishevskiy%2Fssl_types_support-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1474/smalishevskiy/ssl_types_support-v2\nPull-Request: https://github.com/git/git/pull/1474\n\nRange-diff vs v1:\n\n 1: a4b2284dd36 ! 1: 899a0761d99 That change for support different sslcert and sslkey types.\n @@ Metadata\n Author: Stanislav Malishevskiy \n \n ## Commit message ##\n - That change for support different sslcert and sslkey types.\n + http: add support for different sslcert and sslkey types.\n \n Basically git work with default curl ssl type - PEM. But for support\n eTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type\n - and as sslkey type. So there added additional options for http for make\n + and as sslkey type. So there added additional options for http to make\n that possible.\n \n - Signed-off-by: Stanislav Malishevskiy stanislav.malishevskiy@gmail.com\n + Signed-off-by: Stanislav Malishevskiy \n \n ## http.c ##\n @@ http.c: static int curl_ssl_verify = -1;\n\n\n http.c | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex dbe4d29ef7a..d5d82c5230f 100644\n--- a/http.c\n+++ b/http.c\n@@ -40,6 +40,7 @@ static int curl_ssl_verify = -1;\n static int curl_ssl_try;\n static const char *curl_http_version = NULL;\n static const char *ssl_cert;\n+static const char *ssl_cert_type;\n static const char *ssl_cipherlist;\n static const char *ssl_version;\n static struct {\n@@ -59,6 +60,7 @@ static struct {\n #endif\n };\n static const char *ssl_key;\n+static const char *ssl_key_type;\n static const char *ssl_capath;\n static const char *curl_no_proxy;\n #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY\n@@ -374,8 +376,12 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&ssl_version, var, value);\n \tif (!strcmp(\"http.sslcert\", var))\n \t\treturn git_config_pathname(&ssl_cert, var, value);\n+\tif (!strcmp(\"http.sslcerttype\", var))\n+\t\treturn git_config_string(&ssl_cert_type, var, value);\n \tif (!strcmp(\"http.sslkey\", var))\n \t\treturn git_config_pathname(&ssl_key, var, value);\n+\tif (!strcmp(\"http.sslkeytype\", var))\n+\t\treturn git_config_string(&ssl_key_type, var, value);\n \tif (!strcmp(\"http.sslcapath\", var))\n \t\treturn git_config_pathname(&ssl_capath, var, value);\n \tif (!strcmp(\"http.sslcainfo\", var))\n@@ -1014,10 +1020,14 @@ static CURL *get_curl_handle(void)\n \n \tif (ssl_cert)\n \t\tcurl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n+\tif (ssl_cert_type)\n+\t\tcurl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type);\n \tif (has_cert_password())\n \t\tcurl_easy_setopt(result, CURLOPT_KEYPASSWD, cert_auth.password);\n \tif (ssl_key)\n \t\tcurl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);\n+\tif (ssl_key_type)\n+\t\tcurl_easy_setopt(result, CURLOPT_SSLKEYTYPE, ssl_key_type);\n \tif (ssl_capath)\n \t\tcurl_easy_setopt(result, CURLOPT_CAPATH, ssl_capath);\n #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY\n@@ -1252,7 +1262,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \t\tcurl_ssl_verify = 0;\n \n \tset_from_env(&ssl_cert, \"GIT_SSL_CERT\");\n+\tset_from_env(&ssl_cert_type, \"GIT_SSL_CERT_TYPE\");\n \tset_from_env(&ssl_key, \"GIT_SSL_KEY\");\n+\tset_from_env(&ssl_key_type, \"GIT_SSL_KEY_TYPE\");\n \tset_from_env(&ssl_capath, \"GIT_SSL_CAPATH\");\n \tset_from_env(&ssl_cainfo, \"GIT_SSL_CAINFO\");\n \n\nbase-commit: 950264636c68591989456e3ba0a5442f93152c1a\n-- \ngitgitgadget\n"},{"id":"473786","messageId":"20230320171051.GA2615782@coredump.intra.peff.net","threadId":"59415","inReplyTo":"pull.1474.v2.git.git.1679327330032.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-20T17:10:51Z","receivedAt":"2023-03-20T17:16:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2023 at 03:48:49PM +0000, Stanislav Malishevskiy via GitGitGadget wrote:\n\n> From: Stanislav Malishevskiy \n> \n> Basically git work with default curl ssl type - PEM. But for support\n> eTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type\n> and as sslkey type. So there added additional options for http to make\n> that possible.\n\nSeems like a reasonable thing to want, and the patch looks pretty clean.\nTwo small points:\n\n> http.c | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n\nThere are no tests here. I think it might be possible to add them, but\nour https test support is currently optional, and has bit-rotted a bit\nover the years. There's some discussion here:\n\n https://lore.kernel.org/git/Y9s7vyHKXP+TQPRm@pobox.com/\n\nSo I don't think it makes sense to block this patch over the lack of\ntests, but it's something we might keep in mind to add if the test\nsituation improves.\n\n> @@ -1014,10 +1020,14 @@ static CURL *get_curl_handle(void)\n> \n> \tif (ssl_cert)\n> \t\tcurl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n> +\tif (ssl_cert_type)\n> +\t\tcurl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type);\n\nWe're just feeding curl whatever string the user gave us (which is good,\nsince we don't know which ones are valid). But what happens with:\n\n GIT_SSL_CERT_TYPE=bogus git fetch ...\n\nShould we check for an error here, or will the actual request later\ncomplain properly?\n\n-Peff\n"},{"id":"473789","messageId":"xmqqlejrmj4y.fsf@gitster.g","threadId":"59415","inReplyTo":"pull.1474.v2.git.git.1679327330032.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-20T17:23:41Z","receivedAt":"2023-03-20T17:28:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stanislav Malishevskiy via GitGitGadget\" \nwrites:\n\n> From: Stanislav Malishevskiy \n>\n> Basically git work with default curl ssl type - PEM. But for support\n> eTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type\n> and as sslkey type. So there added additional options for http to make\n> that possible.\n>\n> Signed-off-by: Stanislav Malishevskiy \n> ---\n\n> http.c | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n\nThanks.\n\nIs this something that can be protected from future breakage with a\nfew new tests somewhere in t/t5559 (which may also involve touching\nt/lib-httpd.sh as well)?\n"},{"id":"473796","messageId":"CAEpdKf=THU=sc3S3W4azBmYp7+J945GxFqhh_i-5kV0kunMdNw@mail.gmail.com","threadId":"59415","inReplyTo":"20230320171051.GA2615782@coredump.intra.peff.net","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Stanislav M","fromEmail":"stanislav.malishevskiy@gmail.com","sentAt":"2023-03-20T18:21:44Z","receivedAt":"2023-03-20T18:30:31Z","isPatch":true,"sender":{"key":"stanislav.malishevskiy@gmail.com","avatar":null},"body":"> > @@ -1014,10 +1020,14 @@ static CURL *get_curl_handle(void)\n> >\n> > if (ssl_cert)\n> > curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n> > + if (ssl_cert_type)\n> > + curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type);\n>\n> We're just feeding curl whatever string the user gave us (which is good,\n> since we don't know which ones are valid). But what happens with:\n>\n> GIT_SSL_CERT_TYPE=bogus git fetch ...\n>\n> Should we check for an error here, or will the actual request later\n> complain properly?\n\n\nCurl itself validates that string. And if we pass the wrong type or\nnot pass 'ENG' in case of pkcs11: curl will return an error. In that\ncase git do the same if GIT_SSL_CERT passed wrong ss 'ENG' in case of\npkcs11: curl will return an error. In that case git do the same if\nGIT_SSL_CERT passed wrong\n\n\nпн, 20 мар. 2023 г. в 20:10, Jeff King :\n>\n> On Mon, Mar 20, 2023 at 03:48:49PM +0000, Stanislav Malishevskiy via GitGitGadget wrote:\n>\n> > From: Stanislav Malishevskiy \n> >\n> > Basically git work with default curl ssl type - PEM. But for support\n> > eTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type\n> > and as sslkey type. So there added additional options for http to make\n> > that possible.\n>\n> Seems like a reasonable thing to want, and the patch looks pretty clean.\n> Two small points:\n>\n> > http.c | 12 ++++++++++++\n> > 1 file changed, 12 insertions(+)\n>\n> There are no tests here. I think it might be possible to add them, but\n> our https test support is currently optional, and has bit-rotted a bit\n> over the years. There's some discussion here:\n>\n> https://lore.kernel.org/git/Y9s7vyHKXP+TQPRm@pobox.com/\n>\n> So I don't think it makes sense to block this patch over the lack of\n> tests, but it's something we might keep in mind to add if the test\n> situation improves.\n>\n> > @@ -1014,10 +1020,14 @@ static CURL *get_curl_handle(void)\n> >\n> > if (ssl_cert)\n> > curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n> > + if (ssl_cert_type)\n> > + curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type);\n>\n> We're just feeding curl whatever string the user gave us (which is good,\n> since we don't know which ones are valid). But what happens with:\n>\n> GIT_SSL_CERT_TYPE=bogus git fetch ...\n>\n> Should we check for an error here, or will the actual request later\n> complain properly?\n>\n> -Peff\n"},{"id":"473797","messageId":"CAEpdKfnWHtbZDFGhYhz81DRMiG8tbuw1CVWrd7ppSj++pODdYQ@mail.gmail.com","threadId":"59415","inReplyTo":"xmqqlejrmj4y.fsf@gitster.g","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Stanislav M","fromEmail":"stanislav.malishevskiy@gmail.com","sentAt":"2023-03-20T18:24:00Z","receivedAt":"2023-03-20T18:32:12Z","isPatch":true,"sender":{"key":"stanislav.malishevskiy@gmail.com","avatar":null},"body":"> Is this something that can be protected from future breakage with a\n> few new tests somewhere in t/t5559 (which may also involve touching\n> t/lib-httpd.sh as well)?\n\n\nI am not sure about that. That required only for local access to the\ncert and key from curl\n\n\nпн, 20 мар. 2023 г. в 20:23, Junio C Hamano :\n>\n> \"Stanislav Malishevskiy via GitGitGadget\" \n> writes:\n>\n> > From: Stanislav Malishevskiy \n> >\n> > Basically git work with default curl ssl type - PEM. But for support\n> > eTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type\n> > and as sslkey type. So there added additional options for http to make\n> > that possible.\n> >\n> > Signed-off-by: Stanislav Malishevskiy \n> > ---\n>\n> > http.c | 12 ++++++++++++\n> > 1 file changed, 12 insertions(+)\n>\n> Thanks.\n>\n> Is this something that can be protected from future breakage with a\n> few new tests somewhere in t/t5559 (which may also involve touching\n> t/lib-httpd.sh as well)?\n"},{"id":"473852","messageId":"20230321172223.GA3119834@coredump.intra.peff.net","threadId":"59415","inReplyTo":"xmqqlejrmj4y.fsf@gitster.g","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-21T17:22:23Z","receivedAt":"2023-03-21T17:22:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2023 at 10:23:41AM -0700, Junio C Hamano wrote:\n\n> Is this something that can be protected from future breakage with a\n> few new tests somewhere in t/t5559 (which may also involve touching\n> t/lib-httpd.sh as well)?\n\nI don't think we'd want to put it there. While it is the only test that\nrequires SSL currently, it's also specific to HTTP/2, so it may not get\nrun everywhere.\n\nThe original design of the SSL support in the test suite was that you'd\ndo a run of the whole suite with LIB_HTTPD_SSL set, and then it would\nrun all of the usual tests with ssl. But experience has shown that\nnobody does that. So I think there are two paths forward here:\n\n 1. Add a mode to CI that runs with LIB_HTTPD_SSL set. We'd need to fix\n the bit-rotted tests that fail (usually due to things like\n expecting \"http\" in the output instead of \"https\", etc). I linked\n to earlier discussion there elsewhere in the thread.\n\n 2. Add a specific \"test with https\" script that covers some basic\n tests (possibly even just including t5551, in the same way t5559\n does). If the platform apache doesn't support ssl, then it should\n fail gracefully. And then we could add more SSL specific tests\n to that script.\n\n-Peff\n"},{"id":"473853","messageId":"20230321172245.GB3119834@coredump.intra.peff.net","threadId":"59415","inReplyTo":"CAEpdKf=THU=sc3S3W4azBmYp7+J945GxFqhh_i-5kV0kunMdNw@mail.gmail.com","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-21T17:22:45Z","receivedAt":"2023-03-21T17:22:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2023 at 09:21:44PM +0300, Stanislav M wrote:\n\n> > > @@ -1014,10 +1020,14 @@ static CURL *get_curl_handle(void)\n> > >\n> > > if (ssl_cert)\n> > > curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n> > > + if (ssl_cert_type)\n> > > + curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type);\n> >\n> > We're just feeding curl whatever string the user gave us (which is good,\n> > since we don't know which ones are valid). But what happens with:\n> >\n> > GIT_SSL_CERT_TYPE=bogus git fetch ...\n> >\n> > Should we check for an error here, or will the actual request later\n> > complain properly?\n> \n> Curl itself validates that string. And if we pass the wrong type or\n> not pass 'ENG' in case of pkcs11: curl will return an error. In that\n> case git do the same if GIT_SSL_CERT passed wrong ss 'ENG' in case of\n> pkcs11: curl will return an error. In that case git do the same if\n> GIT_SSL_CERT passed wrong\n\nThat sounds great. Thanks for confirming!\n\n-Peff\n"},{"id":"473859","messageId":"xmqqjzzahufj.fsf@gitster.g","threadId":"59415","inReplyTo":"20230321172223.GA3119834@coredump.intra.peff.net","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-21T17:43:12Z","receivedAt":"2023-03-21T17:43:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 2. Add a specific \"test with https\" script that covers some basic\n> tests (possibly even just including t5551, in the same way t5559\n> does). If the platform apache doesn't support ssl, then it should\n> fail gracefully. And then we could add more SSL specific tests\n> to that script.\n\nBoth choices sound sensible. This second one may be simpler to\narrange.\n\nThanks.\n"},{"id":"473981","messageId":"CAEpdKf=pMcMn6q0wGGcrFN1yi1duguSNLDkqhDmvMBeKQ--yuA@mail.gmail.com","threadId":"59415","inReplyTo":"xmqqjzzahufj.fsf@gitster.g","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Stanislav M","fromEmail":"stanislav.malishevskiy@gmail.com","sentAt":"2023-03-23T09:33:59Z","receivedAt":"2023-03-23T09:36:39Z","isPatch":true,"sender":{"key":"stanislav.malishevskiy@gmail.com","avatar":null},"body":"I want to do some clarification there. Those changes are not related\nto http or https directly.\nThat only configures curl to know how to access the certificate and\nkey locally on the client box, so if the way is wrong there is a\nsimple error: Certificate or key not found.\n\nвт, 21 мар. 2023 г. в 20:43, Junio C Hamano :\n>\n> Jeff King writes:\n>\n> > 2. Add a specific \"test with https\" script that covers some basic\n> > tests (possibly even just including t5551, in the same way t5559\n> > does). If the platform apache doesn't support ssl, then it should\n> > fail gracefully. And then we could add more SSL specific tests\n> > to that script.\n>\n> Both choices sound sensible. This second one may be simpler to\n> arrange.\n>\n> Thanks.\n"},{"id":"474022","messageId":"20230323180159.GA1015351@coredump.intra.peff.net","threadId":"59415","inReplyTo":"CAEpdKf=pMcMn6q0wGGcrFN1yi1duguSNLDkqhDmvMBeKQ--yuA@mail.gmail.com","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-23T18:01:59Z","receivedAt":"2023-03-23T18:02:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 23, 2023 at 12:33:59PM +0300, Stanislav M wrote:\n\n> I want to do some clarification there. Those changes are not related\n> to http or https directly.\n> That only configures curl to know how to access the certificate and\n> key locally on the client box, so if the way is wrong there is a\n> simple error: Certificate or key not found.\n\nYes, but I'm not sure if there is a way for Git to trigger curl to look\nat the certificate that does not involve feeding it an https URL (and we\nwant a valid one, because we want to see that it correctly speaks to the\nserver).\n\n-Peff\n"},{"id":"474023","messageId":"CAEpdKfm0S4tkLz8DMnkJmhMmJk8Cj1tUm7NbMe7kpATq0bkxCw@mail.gmail.com","threadId":"59415","inReplyTo":"20230323180159.GA1015351@coredump.intra.peff.net","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Stanislav M","fromEmail":"stanislav.malishevskiy@gmail.com","sentAt":"2023-03-23T18:16:50Z","receivedAt":"2023-03-23T18:17:08Z","isPatch":true,"sender":{"key":"stanislav.malishevskiy@gmail.com","avatar":null},"body":"In my opinion they need the same set of tests which is used as usual\nfor https. But use the right certificate and key.\nBut I don't have any idea how to do that with hardware usb eToken in my case.\n\nчт, 23 мар. 2023 г. в 21:02, Jeff King :\n>\n> On Thu, Mar 23, 2023 at 12:33:59PM +0300, Stanislav M wrote:\n>\n> > I want to do some clarification there. Those changes are not related\n> > to http or https directly.\n> > That only configures curl to know how to access the certificate and\n> > key locally on the client box, so if the way is wrong there is a\n> > simple error: Certificate or key not found.\n>\n> Yes, but I'm not sure if there is a way for Git to trigger curl to look\n> at the certificate that does not involve feeding it an https URL (and we\n> want a valid one, because we want to see that it correctly speaks to the\n> server).\n>\n> -Peff\n"},{"id":"474418","messageId":"xmqqfs9n9yp4.fsf@gitster.g","threadId":"59415","inReplyTo":"CAEpdKfm0S4tkLz8DMnkJmhMmJk8Cj1tUm7NbMe7kpATq0bkxCw@mail.gmail.com","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-29T18:53:11Z","receivedAt":"2023-03-29T18:53:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stanislav M writes:\n\n[administrivia: do not top-post]\n\n>> Yes, but I'm not sure if there is a way for Git to trigger curl to look\n>> at the certificate that does not involve feeding it an https URL (and we\n>> want a valid one, because we want to see that it correctly speaks to the\n>> server).\n> ...\n> In my opinion they need the same set of tests which is used as usual\n> for https. But use the right certificate and key.\n> But I don't have any idea how to do that with hardware usb eToken in my case.\n\nOK, so where does this put us, with respect to the change? We have\nsome behaviour change that we do not know how to test? How would we\nknow when we break it in the future? It is not like the new feature\nis not useful enough that nobody would care if it gets broken by\naccident or anything like that, so...?\n\nAt least perhaps we can throw bogus strings in the environment and\nmake sure cURL library gives complaints, or something?\n\nThanks.\n"},{"id":"474421","messageId":"CAEpdKfnewLsUA37V1-mAW9T+LB1g-PkgjCHY1pxjsff+otZa-A@mail.gmail.com","threadId":"59415","inReplyTo":"xmqqfs9n9yp4.fsf@gitster.g","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Stanislav M","fromEmail":"stanislav.malishevskiy@gmail.com","sentAt":"2023-03-29T19:22:22Z","receivedAt":"2023-03-29T19:22:49Z","isPatch":true,"sender":{"key":"stanislav.malishevskiy@gmail.com","avatar":null},"body":"Yes. If you set bogus strings in the environment cURL should return\nan error the same as if you set the wrong file for certificate or key.\n\nSo you can set\n\nGIT_SSL_CERT=some_real_pem_file - That should work (PEM type used by default)\n\nGIT_SSL_CERT=some_real_pem_file GIT_SSL_CERT_TYPE=PEM - That should work too\n\nGIT_SSL_CERT=some_real_pem_file GIT_SSL_CERT_TYPE=Bogus - That shouldn't work\n\nGIT_SSL_CERT=some_real_der_file GIT_SSL_CERT_TYPE=DER - I am not sure\nabout that, because as I far remember there issue with DER in openssl\n\nI think that more detailed information there:\nhttps://curl.se/libcurl/c/CURLOPT_SSLKEYTYPE.html\n\nBasically that only a format of cert and key file or engine in case of\npkcs11 url instead of file in others cases.\n\nSo if you set it into right values, respect your ssl cert and ssl key\n- https should work. But if not, error from curl should returned\n\nср, 29 мар. 2023 г. в 21:53, Junio C Hamano :\n>\n> Stanislav M writes:\n>\n> [administrivia: do not top-post]\n>\n> >> Yes, but I'm not sure if there is a way for Git to trigger curl to look\n> >> at the certificate that does not involve feeding it an https URL (and we\n> >> want a valid one, because we want to see that it correctly speaks to the\n> >> server).\n> > ...\n> > In my opinion they need the same set of tests which is used as usual\n> > for https. But use the right certificate and key.\n> > But I don't have any idea how to do that with hardware usb eToken in my case.\n>\n> OK, so where does this put us, with respect to the change? We have\n> some behaviour change that we do not know how to test? How would we\n> know when we break it in the future? It is not like the new feature\n> is not useful enough that nobody would care if it gets broken by\n> accident or anything like that, so...?\n>\n> At least perhaps we can throw bogus strings in the environment and\n> make sure cURL library gives complaints, or something?\n>\n> Thanks.\n"},{"id":"474435","messageId":"20230329232347.GB2314218@coredump.intra.peff.net","threadId":"59415","inReplyTo":"xmqqfs9n9yp4.fsf@gitster.g","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-29T23:23:47Z","receivedAt":"2023-03-29T23:24:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 29, 2023 at 11:53:11AM -0700, Junio C Hamano wrote:\n\n> > In my opinion they need the same set of tests which is used as usual\n> > for https. But use the right certificate and key.\n> > But I don't have any idea how to do that with hardware usb eToken in my case.\n> \n> OK, so where does this put us, with respect to the change? We have\n> some behaviour change that we do not know how to test? How would we\n> know when we break it in the future? It is not like the new feature\n> is not useful enough that nobody would care if it gets broken by\n> accident or anything like that, so...?\n> \n> At least perhaps we can throw bogus strings in the environment and\n> make sure cURL library gives complaints, or something?\n\nI would be OK taking the patches without any further tests. It is not\nreally making anything worse in the sense that we already do not test\nany of the client-cert stuff.\n\nIf we can cheaply add some tests that at least exercise the code and\nhand off to curl, that is better than nothing, I guess.\n\nI think the ideal would be a new t5565 that sets LIB_HTTPD_SSL\nunconditionally and actually tests various client-cert formats and\nrequests using a made-on-the-fly cert. But I don't want to hold up a\npatch we otherwise think is OK on the basis of long-term technical debt\nwe already have.\n\n-Peff\n"},{"id":"474439","messageId":"xmqqmt3v5btk.fsf@gitster.g","threadId":"59415","inReplyTo":"20230329232347.GB2314218@coredump.intra.peff.net","subject":"Re: [PATCH v2] http: add support for different sslcert and sslkey types.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T00:20:55Z","receivedAt":"2023-03-30T00:20:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 29, 2023 at 11:53:11AM -0700, Junio C Hamano wrote:\n>\n> ...\n>> > But I don't have any idea how to do that with hardware usb eToken in my case.\n>> \n>> OK, so where does this put us, with respect to the change? ...\n> I would be OK taking the patches without any further tests. It is not\n> really making anything worse in the sense that we already do not test\n> any of the client-cert stuff.\n\nAlright. I think the patch has already been queued for some time,\nand it is OK to merge it down to 'next'.\n\n> If we can cheaply add some tests that at least exercise the code and\n> hand off to curl, that is better than nothing, I guess.\n>\n> I think the ideal would be a new t5565 that sets LIB_HTTPD_SSL\n> unconditionally and actually tests various client-cert formats and\n> requests using a made-on-the-fly cert.\n\nThanks.\n\n"}]}