{"thread":{"id":"55297","subject":"[PATCH v2 1/2] http: store credential when PKI auth is used","startedAt":"2021-03-12T00:49:43Z","lastAt":"2021-03-12T02:38:31Z","messageCount":9,"participants":["John Szakmeister","Junio C Hamano","Jeff King","brian m. carlson"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"418909","messageId":"20210312004842.30697-2-john@szakmeister.net","threadId":"55297","inReplyTo":"20210312004842.30697-1-john@szakmeister.net","subject":"[PATCH v2 1/2] http: store credential when PKI auth is used","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2021-03-12T00:48:41Z","receivedAt":"2021-03-12T00:49:43Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"We already looked for the PKI credentials in the credential store, but\nfailed to approve it on success.  Meaning, the PKI certificate password\nwas never stored and git would request it on every connection to the\nremote.  Let's complete the chain by storing the certificate password on\nsuccess.\n\nLikewise, we also need to reject the credential when there is a failure.\nCurl appears to report client-related certificate issues are reported\nwith the CURLE_SSL_CERTPROBLEM error.  This includes not only a bad\npassword, but potentially other client certificate related problems.\nSince we cannot get more information from curl, we'll go ahead and\nreject the credential upon receiving that error, just to be safe and\navoid caching or saving a bad password.\n\nSigned-off-by: John Szakmeister <john@szakmeister.net>\n---\n http.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex f8ea28bb2e..12a8aaba48 100644\n--- a/http.c\n+++ b/http.c\n@@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results)\n \t\tcredential_approve(&http_auth);\n \t\tif (proxy_auth.password)\n \t\t\tcredential_approve(&proxy_auth);\n+\t\tcredential_approve(&cert_auth);\n \t\treturn HTTP_OK;\n+\t} else if (results->curl_result == CURLE_SSL_CERTPROBLEM) {\n+\t\t/*\n+\t\t * We can't tell from here whether it's a bad path, bad\n+\t\t * certificate, bad password, or something else wrong\n+\t\t * with the certificate.  So we reject the credential to\n+\t\t * avoid caching or saving a bad password.\n+\t\t */\n+\t\tcredential_reject(&http_auth);\n+\t\treturn HTTP_NOAUTH;\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n \telse if (results->http_code == 401) {\n-- \n2.30.1\n\n"},{"id":"418910","messageId":"20210312004842.30697-1-john@szakmeister.net","threadId":"55297","inReplyTo":null,"subject":"[PATCH v2 0/2] http: store credential when PKI auth is used","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2021-03-12T00:48:40Z","receivedAt":"2021-03-12T00:49:44Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"Here's my second attempt at getting the certificate password into the credential\nstore.  I tested from a working PKI setup and found curl--at least reasonable\nrecent versions of it--return CURLE_SSL_CERTPROBLEM:\n\n       CURLE_SSL_CERTPROBLEM (58)\n              problem with the local client certificate.\n\nIt appears there could be another possible error from curl:\n\n       CURLE_SSL_CONNECT_ERROR (35)\n              A  problem  occurred  somewhere  in the SSL/TLS handshake. You\n              really want the error buffer and read the message there as it\n              pinpoints the problem slightly more. Could be  certificates  (file\n              formats, paths, permissions), passwords, and others.\n\nThis seems less likely to be a bad client password scenario, so I did not look\nfor this particular error to reject it.\n\nI also added one other small patch to remove the check of a non-empty password\nbefore calling credential_store() for proxy_auth, as credential_store() already\nchecks for a non-empty password and gracefully handles it when it doesn't.\n\n-John\n\nJohn Szakmeister (2):\n  http: store credential when PKI auth is used\n  http: drop the check for an empty proxy password before approving\n\n http.c | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\n-- \n2.30.1\n\n"},{"id":"418911","messageId":"20210312004842.30697-3-john@szakmeister.net","threadId":"55297","inReplyTo":"20210312004842.30697-1-john@szakmeister.net","subject":"[PATCH v2 2/2] http: drop the check for an empty proxy password before approving","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2021-03-12T00:48:42Z","receivedAt":"2021-03-12T00:49:44Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"credential_approve() already checks for a non-empty password before\nsaving, so there's no need to do the extra check here.\n\nSigned-off-by: John Szakmeister <john@szakmeister.net>\n---\n http.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 12a8aaba48..b0d3ce6c6b 100644\n--- a/http.c\n+++ b/http.c\n@@ -1635,8 +1635,7 @@ static int handle_curl_result(struct slot_results *results)\n \n \tif (results->curl_result == CURLE_OK) {\n \t\tcredential_approve(&http_auth);\n-\t\tif (proxy_auth.password)\n-\t\t\tcredential_approve(&proxy_auth);\n+\t\tcredential_approve(&proxy_auth);\n \t\tcredential_approve(&cert_auth);\n \t\treturn HTTP_OK;\n \t} else if (results->curl_result == CURLE_SSL_CERTPROBLEM) {\n-- \n2.30.1\n\n"},{"id":"418912","messageId":"xmqqeeglb32i.fsf@gitster.g","threadId":"55297","inReplyTo":"20210312004842.30697-2-john@szakmeister.net","subject":"Re: [PATCH v2 1/2] http: store credential when PKI auth is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-12T00:58:29Z","receivedAt":"2021-03-12T00:59:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Szakmeister <john@szakmeister.net> writes:\n\n> Likewise, we also need to reject the credential when there is a failure.\n> Curl appears to report client-related certificate issues are reported\n> with the CURLE_SSL_CERTPROBLEM error.  This includes not only a bad\n> password, but potentially other client certificate related problems.\n>\n> Since we cannot get more information from curl, we'll go ahead and\n> reject the credential upon receiving that error, just to be safe and\n> avoid caching or saving a bad password.\n\nI think this is sensible enough.  As long as a tentative network\nfailure to talk to the server, or an overloaded server that fails to\naccept new connection, won't trigger rejection of a password, it is\nOK, I would think.\n\n> Signed-off-by: John Szakmeister <john@szakmeister.net>\n> ---\n>  http.c | 10 ++++++++++\n>  1 file changed, 10 insertions(+)\n>\n> diff --git a/http.c b/http.c\n> index f8ea28bb2e..12a8aaba48 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results)\n>  \t\tcredential_approve(&http_auth);\n>  \t\tif (proxy_auth.password)\n>  \t\t\tcredential_approve(&proxy_auth);\n> +\t\tcredential_approve(&cert_auth);\n>  \t\treturn HTTP_OK;\n> +\t} else if (results->curl_result == CURLE_SSL_CERTPROBLEM) {\n> +\t\t/*\n> +\t\t * We can't tell from here whether it's a bad path, bad\n> +\t\t * certificate, bad password, or something else wrong\n> +\t\t * with the certificate.  So we reject the credential to\n> +\t\t * avoid caching or saving a bad password.\n> +\t\t */\n> +\t\tcredential_reject(&http_auth);\n> +\t\treturn HTTP_NOAUTH;\n>  \t} else if (missing_target(results))\n>  \t\treturn HTTP_MISSING_TARGET;\n>  \telse if (results->http_code == 401) {\n"},{"id":"418915","messageId":"YErEaJ25gY8dzErv@coredump.intra.peff.net","threadId":"55297","inReplyTo":"20210312004842.30697-1-john@szakmeister.net","subject":"Re: [PATCH v2 0/2] http: store credential when PKI auth is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-12T01:31:20Z","receivedAt":"2021-03-12T01:32:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 11, 2021 at 07:48:40PM -0500, John Szakmeister wrote:\n\n> Here's my second attempt at getting the certificate password into the credential\n> store.  I tested from a working PKI setup and found curl--at least reasonable\n> recent versions of it--return CURLE_SSL_CERTPROBLEM:\n> \n>        CURLE_SSL_CERTPROBLEM (58)\n>               problem with the local client certificate.\n> \n> It appears there could be another possible error from curl:\n> \n>        CURLE_SSL_CONNECT_ERROR (35)\n>               A  problem  occurred  somewhere  in the SSL/TLS handshake. You\n>               really want the error buffer and read the message there as it\n>               pinpoints the problem slightly more. Could be  certificates  (file\n>               formats, paths, permissions), passwords, and others.\n> \n> This seems less likely to be a bad client password scenario, so I did not look\n> for this particular error to reject it.\n> \n> I also added one other small patch to remove the check of a non-empty password\n> before calling credential_store() for proxy_auth, as credential_store() already\n> checks for a non-empty password and gracefully handles it when it doesn't.\n\nThanks. Both patches look good to me. I wondered briefly if we needed to\nworry about old versions of curl missing CURLE_SSL_CERTPROBLEM. But it\nseems to have shown up in ~2002, so I think we are fine to assume it's\nthere.\n\nIt would be nice if we had some tests here, but we currently do not\ncover any of the ssl-cert stuff in the test suite. I suspect adding them\nwould be a big pain to configure and maintain, so I'm OK to leave it off\nfor now. Hopefully you gave it some basic manual testing with your\nworking setup (good password is stored, bad password is rejected).\n\nLooking at how we generate the server-side cert for our http tests, we\ncould _probably_ do something similar for a client-side cert, and just\nconfigure the server to accept a self-signed certificate. But like I\nsaid, I'm OK to leave that for another series (though of course if you\nwant to work on it, that would be very much appreciated).\n\n-Peff\n"},{"id":"418919","messageId":"YErGymyECXjPXWcP@camp.crustytoothpaste.net","threadId":"55297","inReplyTo":"20210312004842.30697-2-john@szakmeister.net","subject":"Re: [PATCH v2 1/2] http: store credential when PKI auth is used","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-03-12T01:41:30Z","receivedAt":"2021-03-12T01:42:19Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-03-12 at 00:48:41, John Szakmeister wrote:\n> We already looked for the PKI credentials in the credential store, but\n> failed to approve it on success.  Meaning, the PKI certificate password\n> was never stored and git would request it on every connection to the\n> remote.  Let's complete the chain by storing the certificate password on\n> success.\n> \n> Likewise, we also need to reject the credential when there is a failure.\n> Curl appears to report client-related certificate issues are reported\n> with the CURLE_SSL_CERTPROBLEM error.  This includes not only a bad\n> password, but potentially other client certificate related problems.\n> Since we cannot get more information from curl, we'll go ahead and\n> reject the credential upon receiving that error, just to be safe and\n> avoid caching or saving a bad password.\n> \n> Signed-off-by: John Szakmeister <john@szakmeister.net>\n> ---\n>  http.c | 10 ++++++++++\n>  1 file changed, 10 insertions(+)\n> \n> diff --git a/http.c b/http.c\n> index f8ea28bb2e..12a8aaba48 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results)\n>  \t\tcredential_approve(&http_auth);\n>  \t\tif (proxy_auth.password)\n>  \t\t\tcredential_approve(&proxy_auth);\n> +\t\tcredential_approve(&cert_auth);\n>  \t\treturn HTTP_OK;\n> +\t} else if (results->curl_result == CURLE_SSL_CERTPROBLEM) {\n> +\t\t/*\n> +\t\t * We can't tell from here whether it's a bad path, bad\n> +\t\t * certificate, bad password, or something else wrong\n> +\t\t * with the certificate.  So we reject the credential to\n> +\t\t * avoid caching or saving a bad password.\n> +\t\t */\n> +\t\tcredential_reject(&http_auth);\n\nIs this supposed to be &cert_auth here?  I'm not sure how a bad HTTP\npassword would even have been tested in this case.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"418920","messageId":"YErHsQwIC2grgjwI@coredump.intra.peff.net","threadId":"55297","inReplyTo":"YErGymyECXjPXWcP@camp.crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] http: store credential when PKI auth is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-12T01:45:21Z","receivedAt":"2021-03-12T01:46:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 12, 2021 at 01:41:30AM +0000, brian m. carlson wrote:\n\n> > diff --git a/http.c b/http.c\n> > index f8ea28bb2e..12a8aaba48 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results)\n> >  \t\tcredential_approve(&http_auth);\n> >  \t\tif (proxy_auth.password)\n> >  \t\t\tcredential_approve(&proxy_auth);\n> > +\t\tcredential_approve(&cert_auth);\n> >  \t\treturn HTTP_OK;\n> > +\t} else if (results->curl_result == CURLE_SSL_CERTPROBLEM) {\n> > +\t\t/*\n> > +\t\t * We can't tell from here whether it's a bad path, bad\n> > +\t\t * certificate, bad password, or something else wrong\n> > +\t\t * with the certificate.  So we reject the credential to\n> > +\t\t * avoid caching or saving a bad password.\n> > +\t\t */\n> > +\t\tcredential_reject(&http_auth);\n> \n> Is this supposed to be &cert_auth here?  I'm not sure how a bad HTTP\n> password would even have been tested in this case.\n\nGood catch! When reviewing, I was so busy thinking about _where_ this\nline should go that I didn't even notice what it said. :)\n\n-Peff\n"},{"id":"418921","messageId":"CAEBDL5VeCe4UT7HFB0my0a1Cqmi-aXdYm86z9D6KSrMdi7fa+g@mail.gmail.com","threadId":"55297","inReplyTo":"YErHsQwIC2grgjwI@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] http: store credential when PKI auth is used","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2021-03-12T02:27:11Z","receivedAt":"2021-03-12T02:28:13Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Thu, Mar 11, 2021 at 8:45 PM Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Mar 12, 2021 at 01:41:30AM +0000, brian m. carlson wrote:\n>\n> > > diff --git a/http.c b/http.c\n> > > index f8ea28bb2e..12a8aaba48 100644\n> > > --- a/http.c\n> > > +++ b/http.c\n> > > @@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results)\n> > >             credential_approve(&http_auth);\n> > >             if (proxy_auth.password)\n> > >                     credential_approve(&proxy_auth);\n> > > +           credential_approve(&cert_auth);\n> > >             return HTTP_OK;\n> > > +   } else if (results->curl_result == CURLE_SSL_CERTPROBLEM) {\n> > > +           /*\n> > > +            * We can't tell from here whether it's a bad path, bad\n> > > +            * certificate, bad password, or something else wrong\n> > > +            * with the certificate.  So we reject the credential to\n> > > +            * avoid caching or saving a bad password.\n> > > +            */\n> > > +           credential_reject(&http_auth);\n> >\n> > Is this supposed to be &cert_auth here?  I'm not sure how a bad HTTP\n> > password would even have been tested in this case.\n>\n> Good catch! When reviewing, I was so busy thinking about _where_ this\n> line should go that I didn't even notice what it said. :)\n\nGood catch!  I don't even know how I did that. :-/  The system I\ncreated the patch on is inaccessible via the Internet and I can't\nreally get data off of it.  This is entirely an error in translation\non my part.  The diff I printed has the correct line.  My bad.  I'll\nsend an update soon.\n\nJohn\n"},{"id":"418922","messageId":"CAEBDL5VynABbrOTupEjx_mT9f8zgWaqmHQOFR-5CFYpe9xsk=Q@mail.gmail.com","threadId":"55297","inReplyTo":"YErEaJ25gY8dzErv@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/2] http: store credential when PKI auth is used","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2021-03-12T02:37:44Z","receivedAt":"2021-03-12T02:38:31Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Thu, Mar 11, 2021 at 8:31 PM Jeff King <peff@peff.net> wrote:\n[snip]\n> Thanks. Both patches look good to me. I wondered briefly if we needed to\n> worry about old versions of curl missing CURLE_SSL_CERTPROBLEM. But it\n> seems to have shown up in ~2002, so I think we are fine to assume it's\n> there.\n>\n> It would be nice if we had some tests here, but we currently do not\n> cover any of the ssl-cert stuff in the test suite. I suspect adding them\n> would be a big pain to configure and maintain, so I'm OK to leave it off\n> for now. Hopefully you gave it some basic manual testing with your\n> working setup (good password is stored, bad password is rejected).\n\nI did do some manual testing in an environment at work where they have\nthis set up.  Unfortunately, the way I went about this was not optimal.  I'll\nwork the issue differently in the future, so I don't have that kind of\ntranslation\nissue again.\n\n> Looking at how we generate the server-side cert for our http tests, we\n> could _probably_ do something similar for a client-side cert, and just\n> configure the server to accept a self-signed certificate. But like I\n> said, I'm OK to leave that for another series (though of course if you\n> want to work on it, that would be very much appreciated).\n\nI looked at things a little bit, but it was too much to take on right\nnow.  I could\nprobably get something together to help make it happen.  I've been down that\nroad before, so I know it can be involved, but it would be nice to have tests.\nI'm not signing up just yet for that, but when a rainy weekend hits, I'll see\nabout taking a stab at it.\n\n-John\n"}]}