{"thread":{"id":"57113","subject":"passing CURLOPT_CERTTYPE to libcurl","startedAt":"2021-12-18T00:49:52Z","lastAt":"2021-12-21T16:24:04Z","messageCount":5,"participants":["Florian Mickler","Junio C Hamano","Johannes Schindelin","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"444409","messageId":"20211218010621.2fdc2b3c@monster.mickler.org","threadId":"57113","inReplyTo":null,"subject":"passing CURLOPT_CERTTYPE to libcurl","fromName":"Florian Mickler","fromEmail":"florian@mickler.org","sentAt":"2021-12-18T00:06:21Z","receivedAt":"2021-12-18T00:49:52Z","isPatch":false,"sender":{"key":"florian@mickler.org","avatar":null},"body":"Hi,\n\nI recently needed to use a tls client certificate from my companies pki\ncard (smartcard) in order to get access to a git repository via https.\nThe client system I used was set up such that there was a pkcs11 openssl\nmodule that would asks for a pin whenever the certificate was needed. \n\nWhile I was able to connect to the git repo via curl with \n\n\tcurl -E 'pkcs\\:[REDACTED_PART_OF_PKCS_URL]' --key\\\n\t'pkcs:[REDACTED_PART_OF_PKCS_URL]' --cert-type ENG\\\n\t--key-type ENG $URL\n\nI was not able to connect to the host with current git. I could pass\nthe pkcs urls for key and cert, but openssl expected them to be in \"PEM\"\nencoding. While the certificates on the card where apparently of form\n\"ENG\". \n\nAfter a bit of searching, I found a patch[1] for git to pass\nthe cert-type to libcurl (CURLOPT_SSLKEYTYPE and CURLOPT_SSLCERTTYPE)\nfrom 2013. And sure enough, forward-porting it to current HEAD meant\nthat I could successfully connect to that host and clone the repo. \n\nOnly the CURLOPT_SSLKEYTYPE and CURLOPT_SSLCERTTYPE are needed in my\ncase, because the openssl config sets up the pkcs11 module. My\nunderstanding is that the pkcs11 module get's triggered by the\npkcs11:urls for key and cert. It might be this openssl module: \nhttps://github.com/OpenSC/libp11 \n\nIs there a specific reason, that patch wasn't merged? It would allow\nfor non-pem ssl certificates to be loaded also (without pkcs11 at all). \n\nI realize, that the underlying systems could and should set up\neverything automagically as soon as i point them to the certificate that\ni want to use. But not opening up these CURL Settings from git seems\nkind of silly given that today's systems still seem kinda borked and do\nnot do that. What harm comes from these two tuning knobs being exposed?\n\nBest regards,\nFlo\n\n\n[1] https://marc.info/?l=git&m=136675822032549&w=2\n\n\n"},{"id":"444413","messageId":"xmqq7dc2zplg.fsf@gitster.g","threadId":"57113","inReplyTo":"20211218010621.2fdc2b3c@monster.mickler.org","subject":"Re: passing CURLOPT_CERTTYPE to libcurl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-18T01:08:59Z","receivedAt":"2021-12-18T01:09:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Florian Mickler writes:\n\n> Is there a specific reason, that patch wasn't merged? It would allow\n> for non-pem ssl certificates to be loaded also (without pkcs11 at all). \n>\n> I realize, that the underlying systems could and should set up\n> everything automagically as soon as i point them to the certificate that\n> i want to use. But not opening up these CURL Settings from git seems\n> kind of silly given that today's systems still seem kinda borked and do\n> not do that. What harm comes from these two tuning knobs being exposed?\n>\n> Best regards,\n> Flo\n>\n>\n> [1] https://marc.info/?l=git&m=136675822032549&w=2\n\nAlmost always, when some patch aims to achieve a worthy goal, and in\nthe initial discussion on the thread more experienced project\nmembers agree it is a worthwhile thing to do, the only reason why\nthe feature proposed does not materialize in later versions of Git\nis because the developer with the original itch did not follow it\nthrough after getting review comments and saying something that\nmakes reviewers to expect an updated version of the patch.\n\nI didn't follow your marc.info URL, but I am reasonably sure, if I\nwere involved in the discussion, that would be the likely reason.\n\n"},{"id":"444551","messageId":"nycvar.QRO.7.76.6.2112202223300.347@tvgsbejvaqbjf.bet","threadId":"57113","inReplyTo":"xmqq7dc2zplg.fsf@gitster.g","subject":"Re: passing CURLOPT_CERTTYPE to libcurl","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-12-20T22:21:28Z","receivedAt":"2021-12-20T22:21:42Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & Florian,\n\nOn Fri, 17 Dec 2021, Junio C Hamano wrote:\n\n> Florian Mickler writes:\n>\n> > Is there a specific reason, that patch wasn't merged? It would allow\n> > for non-pem ssl certificates to be loaded also (without pkcs11 at all).\n> >\n> > I realize, that the underlying systems could and should set up\n> > everything automagically as soon as i point them to the certificate that\n> > i want to use. But not opening up these CURL Settings from git seems\n> > kind of silly given that today's systems still seem kinda borked and do\n> > not do that. What harm comes from these two tuning knobs being exposed?\n> >\n> > Best regards,\n> > Flo\n> >\n> >\n> > [1] https://marc.info/?l=git&m=136675822032549&w=2\n\nThis corresponds to\nhttps://lore.kernel.org/git/1366758207-7724-1-git-send-email-jqassar@gmail.com/\n(for those who prefer lore.kernel.org over marc.info, and for those who\nwant to look for the Message-ID directly).\n\nMy summary of that thread:\n\n- The patch implements something Git wants to support.\n\n- A couple of improvements need to be made, such as:\n\n * Error-checking needed to be improved\n\n * Adding a hint to the documentation of `http.sslKeyType` being set to\n `ENG` causing `http.sslKey` being interpreted differently.\n\n * Adding regression tests, if possible\n\n * Maybe a more complete commit message?\n\n- Testing the smart card support was considered hard, especially given\n that the contributor still wanted to contribute patches to cURL without\n which it wouldn't work.\n\n The patch seems to have been contributed via\n https://curl.se/mail/lib-2013-04/0340.html, been reviewed and changes\n were requested, but there was no other patch submission that I could\n find.\n\n However, over five years later, what looks like an equivalent fix to me\n was applied:\n https://github.com/curl/curl/commit/4939f3652473c1519d2b604068efb87ef7531874\n\n- The contributor, Jerry Qassar, gave all the signs of working on a\n next iteration (\"reroll\", as Junio likes to call it). But that never\n materialized, either:\n\n https://lore.kernel.org/git/?q=f%3Ajqassar\n\n Based on this, the lack of a cURL contribution, and a quick web search\n for the name \"Jerry Qassar\" I somehow doubt that Cc:ing them might\n raise their attention.\n\n> Almost always, when some patch aims to achieve a worthy goal, and in\n> the initial discussion on the thread more experienced project\n> members agree it is a worthwhile thing to do, the only reason why\n> the feature proposed does not materialize in later versions of Git\n> is because the developer with the original itch did not follow it\n> through after getting review comments and saying something that\n> makes reviewers to expect an updated version of the patch.\n>\n> I didn't follow your marc.info URL, but I am reasonably sure, if I\n> were involved in the discussion, that would be the likely reason.\n\nYes, you were involved in the discussion, and indeed, there was no\nfollow-up.\n\nAfter more than 8 years, I do believe that the patch is fair game to be\npicked up by any other interested contributor who might want to contribute\nv2 (hint, hint, Florian... all it would take is to study the mail thread\nfrom way back when and adapt the patch accordingly, of course after\nrebasing it to a recent Git revision).\n\nCiao,\nDscho\n"},{"id":"444594","messageId":"4B51B2CA-3BED-4046-8544-219DA737E7CD@mickler.org","threadId":"57113","inReplyTo":"nycvar.QRO.7.76.6.2112202223300.347@tvgsbejvaqbjf.bet","subject":"Re: passing CURLOPT_CERTTYPE to libcurl","fromName":"Florian Mickler","fromEmail":"florian@mickler.org","sentAt":"2021-12-21T09:45:23Z","receivedAt":"2021-12-21T09:45:32Z","isPatch":false,"sender":{"key":"florian@mickler.org","avatar":null},"body":"Thank you for the summary!\nI am currently looking into it. Got into a conversation with dwmw2 also about fixing libcurl or even openssl after i found some issues he opened about this weird interface. Which really sounds like it would be the real fix. (Although probably slower to trickle to end users and more complex)\n\n\nFor this patch i guess I will skip the engine variable, because it is not needed for my use case and my lib version. (Although I am still experimenting with the whole pkcs11 stuff)\n\nThat makes the error handling easier (none needed, imo).\nThe git libcurl min version is also raised above the introduction of those curlopts, so no libcurl-version-bracing needed.\n\nI was looking into the testing framework, it might be possible to configure the https to require client auth on a different port and then testing that with pem and der file.\n\nAdding pkcs11 infrastructure to the test harness might be a bit over the top though :-)\n\nBest regards,\nFlo\n\n\n\nAm 20. Dezember 2021 23:21:28 MEZ schrieb Johannes Schindelin :\n>Hi Junio & Florian,\n>\n>On Fri, 17 Dec 2021, Junio C Hamano wrote:\n>\n>> Florian Mickler writes:\n>>\n>> > Is there a specific reason, that patch wasn't merged? It would\n>allow\n>> > for non-pem ssl certificates to be loaded also (without pkcs11 at\n>all).\n>> >\n>> > I realize, that the underlying systems could and should set up\n>> > everything automagically as soon as i point them to the certificate\n>that\n>> > i want to use. But not opening up these CURL Settings from git\n>seems\n>> > kind of silly given that today's systems still seem kinda borked\n>and do\n>> > not do that. What harm comes from these two tuning knobs being\n>exposed?\n>> >\n>> > Best regards,\n>> > Flo\n>> >\n>> >\n>> > [1] https://marc.info/?l=git&m=136675822032549&w=2\n>\n>This corresponds to\n>https://lore.kernel.org/git/1366758207-7724-1-git-send-email-jqassar@gmail.com/\n>(for those who prefer lore.kernel.org over marc.info, and for those who\n>want to look for the Message-ID directly).\n>\n>My summary of that thread:\n>\n>- The patch implements something Git wants to support.\n>\n>- A couple of improvements need to be made, such as:\n>\n> * Error-checking needed to be improved\n>\n> * Adding a hint to the documentation of `http.sslKeyType` being set to\n> `ENG` causing `http.sslKey` being interpreted differently.\n>\n> * Adding regression tests, if possible\n>\n> * Maybe a more complete commit message?\n>\n>- Testing the smart card support was considered hard, especially given\n>that the contributor still wanted to contribute patches to cURL without\n> which it wouldn't work.\n>\n> The patch seems to have been contributed via\n> https://curl.se/mail/lib-2013-04/0340.html, been reviewed and changes\n> were requested, but there was no other patch submission that I could\n> find.\n>\n>However, over five years later, what looks like an equivalent fix to me\n> was applied:\n>https://github.com/curl/curl/commit/4939f3652473c1519d2b604068efb87ef7531874\n>\n>- The contributor, Jerry Qassar, gave all the signs of working on a\n> next iteration (\"reroll\", as Junio likes to call it). But that never\n> materialized, either:\n>\n> https://lore.kernel.org/git/?q=f%3Ajqassar\n>\n> Based on this, the lack of a cURL contribution, and a quick web search\n> for the name \"Jerry Qassar\" I somehow doubt that Cc:ing them might\n> raise their attention.\n>\n>> Almost always, when some patch aims to achieve a worthy goal, and in\n>> the initial discussion on the thread more experienced project\n>> members agree it is a worthwhile thing to do, the only reason why\n>> the feature proposed does not materialize in later versions of Git\n>> is because the developer with the original itch did not follow it\n>> through after getting review comments and saying something that\n>> makes reviewers to expect an updated version of the patch.\n>>\n>> I didn't follow your marc.info URL, but I am reasonably sure, if I\n>> were involved in the discussion, that would be the likely reason.\n>\n>Yes, you were involved in the discussion, and indeed, there was no\n>follow-up.\n>\n>After more than 8 years, I do believe that the patch is fair game to be\n>picked up by any other interested contributor who might want to\n>contribute\n>v2 (hint, hint, Florian... all it would take is to study the mail\n>thread\n>from way back when and adapt the patch accordingly, of course after\n>rebasing it to a recent Git revision).\n>\n>Ciao,\n>Dscho\n\n---\nSignature\n"},{"id":"444627","messageId":"211221.86h7b2q63j.gmgdl@evledraar.gmail.com","threadId":"57113","inReplyTo":"4B51B2CA-3BED-4046-8544-219DA737E7CD@mickler.org","subject":"Re: passing CURLOPT_CERTTYPE to libcurl","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-21T16:20:55Z","receivedAt":"2021-12-21T16:24:04Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 21 2021, Florian Mickler wrote:\n\n> Thank you for the summary!\n> I am currently looking into it. Got into a conversation with dwmw2\n> also about fixing libcurl or even openssl after i found some issues he\n> opened about this weird interface. Which really sounds like it would\n> be the real fix. (Although probably slower to trickle to end users and\n> more complex)\n>\n>\n> For this patch i guess I will skip the engine variable, because it is not needed for my use case and my lib version. (Although I am still experimenting with the whole pkcs11 stuff)\n>\n> That makes the error handling easier (none needed, imo).\n> The git libcurl min version is also raised above the introduction of those curlopts, so no libcurl-version-bracing needed.\n>\n> I was looking into the testing framework, it might be possible to configure the https to require client auth on a different port and then testing that with pem and der file.\n>\n> Adding pkcs11 infrastructure to the test harness might be a bit over the top though :-)\n\nIt would be really nice to have https tests in the test suite, but we\ndon't have any currently.\n\nThere was some discussion about setting them up around some other recent\nlibcurl related changes, but it hasn't been done.\n\nSo if you want to work on that it would be fantastic, but don't consider\nit a blocker to just get CURLOPT_SSLCERTTYPE into git.git.\n\nWe basically assume that we don't need to re-test libcurl itself, and\nsince these settings amount to just setting various flags in the library\nthe need for them isn't all that great.\n\n> Am 20. Dezember 2021 23:21:28 MEZ schrieb Johannes Schindelin :\n>>Hi Junio & Florian,\n>>\n>>On Fri, 17 Dec 2021, Junio C Hamano wrote:\n>>\n>>> Florian Mickler writes:\n>>>\n>>> > Is there a specific reason, that patch wasn't merged? It would\n>>allow\n>>> > for non-pem ssl certificates to be loaded also (without pkcs11 at\n>>all).\n>>> >\n>>> > I realize, that the underlying systems could and should set up\n>>> > everything automagically as soon as i point them to the certificate\n>>that\n>>> > i want to use. But not opening up these CURL Settings from git\n>>seems\n>>> > kind of silly given that today's systems still seem kinda borked\n>>and do\n>>> > not do that. What harm comes from these two tuning knobs being\n>>exposed?\n>>> >\n>>> > Best regards,\n>>> > Flo\n>>> >\n>>> >\n>>> > [1] https://marc.info/?l=git&m=136675822032549&w=2\n>>\n>>This corresponds to\n>>https://lore.kernel.org/git/1366758207-7724-1-git-send-email-jqassar@gmail.com/\n>>(for those who prefer lore.kernel.org over marc.info, and for those who\n>>want to look for the Message-ID directly).\n>>\n>>My summary of that thread:\n>>\n>>- The patch implements something Git wants to support.\n>>\n>>- A couple of improvements need to be made, such as:\n>>\n>> * Error-checking needed to be improved\n>>\n>> * Adding a hint to the documentation of `http.sslKeyType` being set to\n>> `ENG` causing `http.sslKey` being interpreted differently.\n>>\n>> * Adding regression tests, if possible\n>>\n>> * Maybe a more complete commit message?\n>>\n>>- Testing the smart card support was considered hard, especially given\n>>that the contributor still wanted to contribute patches to cURL without\n>> which it wouldn't work.\n>>\n>> The patch seems to have been contributed via\n>> https://curl.se/mail/lib-2013-04/0340.html, been reviewed and changes\n>> were requested, but there was no other patch submission that I could\n>> find.\n>>\n>>However, over five years later, what looks like an equivalent fix to me\n>> was applied:\n>>https://github.com/curl/curl/commit/4939f3652473c1519d2b604068efb87ef7531874\n>>\n>>- The contributor, Jerry Qassar, gave all the signs of working on a\n>> next iteration (\"reroll\", as Junio likes to call it). But that never\n>> materialized, either:\n>>\n>> https://lore.kernel.org/git/?q=f%3Ajqassar\n>>\n>> Based on this, the lack of a cURL contribution, and a quick web search\n>> for the name \"Jerry Qassar\" I somehow doubt that Cc:ing them might\n>> raise their attention.\n>>\n>>> Almost always, when some patch aims to achieve a worthy goal, and in\n>>> the initial discussion on the thread more experienced project\n>>> members agree it is a worthwhile thing to do, the only reason why\n>>> the feature proposed does not materialize in later versions of Git\n>>> is because the developer with the original itch did not follow it\n>>> through after getting review comments and saying something that\n>>> makes reviewers to expect an updated version of the patch.\n>>>\n>>> I didn't follow your marc.info URL, but I am reasonably sure, if I\n>>> were involved in the discussion, that would be the likely reason.\n>>\n>>Yes, you were involved in the discussion, and indeed, there was no\n>>follow-up.\n>>\n>>After more than 8 years, I do believe that the patch is fair game to be\n>>picked up by any other interested contributor who might want to\n>>contribute\n>>v2 (hint, hint, Florian... all it would take is to study the mail\n>>thread\n>>from way back when and adapt the patch accordingly, of course after\n>>rebasing it to a recent Git revision).\n>>\n>>Ciao,\n>>Dscho\n>\n> ---\n> Signature\n\n"}]}