{"thread":{"id":"55267","subject":"[PATCH] http: store credential when PKI auth is used","startedAt":"2021-03-06T22:56:28Z","lastAt":"2021-03-12T01:25:42Z","messageCount":4,"participants":["John Szakmeister","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"418412","messageId":"20210306225253.87130-1-john@szakmeister.net","threadId":"55267","inReplyTo":null,"subject":"[PATCH] http: store credential when PKI auth is used","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2021-03-06T22:52:53Z","receivedAt":"2021-03-06T22:56:28Z","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\nSigned-off-by: John Szakmeister <john@szakmeister.net>\n---\n\nI'm not sure if certificate passwords were not stored for some reason, but\nsearching the archives I didn't see a mention of it.  Hopefully this is\nacceptable.  I did try this in an environment where we have client SSL certs and\nthis made the user experience much better.\n\n http.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex f8ea28bb2e..440890695f 100644\n--- a/http.c\n+++ b/http.c\n@@ -1637,6 +1637,8 @@ 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\tif (cert_auth.password)\n+\t\t\tcredential_approve(&cert_auth);\n \t\treturn HTTP_OK;\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n-- \n2.30.1\n\n"},{"id":"418736","messageId":"YEkljZWg4+lTQRyS@coredump.intra.peff.net","threadId":"55267","inReplyTo":"20210306225253.87130-1-john@szakmeister.net","subject":"Re: [PATCH] http: store credential when PKI auth is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-10T20:01:17Z","receivedAt":"2021-03-10T20:01:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 06, 2021 at 05:52:53PM -0500, John Szakmeister wrote:\n\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> Signed-off-by: John Szakmeister <john@szakmeister.net>\n> ---\n> \n> I'm not sure if certificate passwords were not stored for some reason, but\n> searching the archives I didn't see a mention of it.  Hopefully this is\n> acceptable.  I did try this in an environment where we have client SSL certs and\n> this made the user experience much better.\n\nI think it was just something that nobody ever cared about before. The\ncert password request got converted to credential_fill() as part of\n148bb6a7b4 (http: use credential API to get passwords, 2011-12-10). That\ncommit added approve/reject for http, because that's what I really cared\nabout, but the intent was always to treat most password queries\nconsistently.\n\n> diff --git a/http.c b/http.c\n> index f8ea28bb2e..440890695f 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1637,6 +1637,8 @@ 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\tif (cert_auth.password)\n> +\t\t\tcredential_approve(&cert_auth);\n\nSo I think this is the right direction, but:\n\n  - you'd need a credential_reject() somewhere, too. Otherwise a bad\n    password will get stored and then reused over and over, with no\n    opportunity to tell the helper to forget about it.\n\n  - is this the best spot to check that our password was right? We only\n    care about unlocking the local cert, which in theory is independent\n    of what the server tells us. I suspect we can't really tell the\n    difference, though (we hand the cert path and password off to curl,\n    and then hopefully a request is successful). So this may be the best\n    we can do for approval. But for rejection, I doubt that a 401 would\n    be the right response. How does curl respond when the password is\n    bad? Likewise, what happens if the password is bad but the server is\n    willing to make the request anyway (does curl bail immediately, or\n    might we get an HTTP 200 even with a bad cert password)?\n\n  - I think proxy_cert_auth would probably want the same treatment.\n\n  - The \"if (cert_auth.password)\" is redundant. credential_approve()\n    will return silently if there is no password to approve. I know\n    you're copying the pattern from directly above, but I think that one\n    should be cleaned up, too.\n\n    (I also was mildly surprised that this worked at all, since\n    credential_approve() will bail if there is no username field. But\n    the cert code fills in an empty username).\n\nMost of those are \"nice to have\" improvements over what you have here,\nbut I think without a matching reject() this would be a regression.\n\n-Peff\n"},{"id":"418913","messageId":"CAEBDL5U=BxHzYWmG2Cpw+XcMJTF8_Qp0KXoKz6N+fHp1ZWdbRQ@mail.gmail.com","threadId":"55267","inReplyTo":"YEkljZWg4+lTQRyS@coredump.intra.peff.net","subject":"Re: [PATCH] http: store credential when PKI auth is used","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2021-03-12T01:01:53Z","receivedAt":"2021-03-12T01:02:40Z","isPatch":true,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Wed, Mar 10, 2021 at 3:01 PM Jeff King <peff@peff.net> wrote:\n[snip]\n> I think it was just something that nobody ever cared about before. The\n> cert password request got converted to credential_fill() as part of\n> 148bb6a7b4 (http: use credential API to get passwords, 2011-12-10). That\n> commit added approve/reject for http, because that's what I really cared\n> about, but the intent was always to treat most password queries\n> consistently.\n\nI figured it'd be something like that, but wanted to ask, just in case. :-)\n\n> > diff --git a/http.c b/http.c\n> > index f8ea28bb2e..440890695f 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -1637,6 +1637,8 @@ 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> > +             if (cert_auth.password)\n> > +                     credential_approve(&cert_auth);\n>\n> So I think this is the right direction, but:\n>\n>   - you'd need a credential_reject() somewhere, too. Otherwise a bad\n>     password will get stored and then reused over and over, with no\n>     opportunity to tell the helper to forget about it.\n\nRight, I totally forgot about that aspect of this.\n\n>   - is this the best spot to check that our password was right? We only\n>     care about unlocking the local cert, which in theory is independent\n>     of what the server tells us. I suspect we can't really tell the\n>     difference, though (we hand the cert path and password off to curl,\n>     and then hopefully a request is successful). So this may be the best\n>     we can do for approval. But for rejection, I doubt that a 401 would\n>     be the right response. How does curl respond when the password is\n>     bad? Likewise, what happens if the password is bad but the server is\n>     willing to make the request anyway (does curl bail immediately, or\n>     might we get an HTTP 200 even with a bad cert password)?\n\nIt turns out that it was the right spot.  Curl will report\nCURLE_SSL_CERTPROBLEM when provided a bad password, so\nI ended up checking for that and using it to reject the credential.  It's a\nbit loose in that other problems with the client certificate could cause\nthe same error, but there's not a good way to know whether the issue\nwas password-related or not.  So the new patch set will just reject it,\neven if the issue was ultimately some other problem with the cert.  It\nseemed like the best response given the information we have.\n\n>   - I think proxy_cert_auth would probably want the same treatment.\n\nOh, I think I misread this before making my fixes.  I think what you're\nsaying here is that proxy_cert_auth should be approved and rejected\nin the same spots as the client cert auth?  I missed that but am happy\nto add it, if that's what you meant.  The only trouble is that I don't have\na great way of checking that particular feature.\n\n>   - The \"if (cert_auth.password)\" is redundant. credential_approve()\n>     will return silently if there is no password to approve. I know\n>     you're copying the pattern from directly above, but I think that one\n>     should be cleaned up, too.\n\nDone.\n\n>     (I also was mildly surprised that this worked at all, since\n>     credential_approve() will bail if there is no username field. But\n>     the cert code fills in an empty username).\n>\n> Most of those are \"nice to have\" improvements over what you have here,\n> but I think without a matching reject() this would be a regression.\n\nNo worries.  If we can figure out the details, I'm happy to add it.\n\n-John\n"},{"id":"418914","messageId":"YErC1LIaxomLd3Gu@coredump.intra.peff.net","threadId":"55267","inReplyTo":"CAEBDL5U=BxHzYWmG2Cpw+XcMJTF8_Qp0KXoKz6N+fHp1ZWdbRQ@mail.gmail.com","subject":"Re: [PATCH] http: store credential when PKI auth is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-12T01:24:36Z","receivedAt":"2021-03-12T01:25:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 11, 2021 at 08:01:53PM -0500, John Szakmeister wrote:\n\n> >   - I think proxy_cert_auth would probably want the same treatment.\n> \n> Oh, I think I misread this before making my fixes.  I think what you're\n> saying here is that proxy_cert_auth should be approved and rejected\n> in the same spots as the client cert auth?  I missed that but am happy\n> to add it, if that's what you meant.  The only trouble is that I don't have\n> a great way of checking that particular feature.\n\nYep, that's what I meant. Looking at CURLE_SSL_* in curl.h, it looks\nlike there's no way to distinguish a proxy cert problem from a regular\ncert problem. So probably we'd need to reject both when we see\nCURLE_SSL_CERTPROBLEM. As long as somebody is not using both at once, it\nwould not matter at all. And even if they are, the worst case is having\nto put in their password again.\n\nThat said, given that nobody has asked for it and you have no easy way\nof testing it, I'm content to leave it be for now. Your patches\nshouldn't make anything worse there, and it shouldn't be too hard to\nfind this discussion in the list archive later.\n\n-Peff\n"}]}