| Message ID | 20210312004842.30697-2-john@szakmeister.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | http: store credential when PKI auth is used | expand |
John Szakmeister <john@szakmeister.net> writes: > Likewise, we also need to reject the credential when there is a failure. > Curl appears to report client-related certificate issues are reported > with the CURLE_SSL_CERTPROBLEM error. This includes not only a bad > password, but potentially other client certificate related problems. > > Since we cannot get more information from curl, we'll go ahead and > reject the credential upon receiving that error, just to be safe and > avoid caching or saving a bad password. I think this is sensible enough. As long as a tentative network failure to talk to the server, or an overloaded server that fails to accept new connection, won't trigger rejection of a password, it is OK, I would think. > Signed-off-by: John Szakmeister <john@szakmeister.net> > --- > http.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/http.c b/http.c > index f8ea28bb2e..12a8aaba48 100644 > --- a/http.c > +++ b/http.c > @@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results) > credential_approve(&http_auth); > if (proxy_auth.password) > credential_approve(&proxy_auth); > + credential_approve(&cert_auth); > return HTTP_OK; > + } else if (results->curl_result == CURLE_SSL_CERTPROBLEM) { > + /* > + * We can't tell from here whether it's a bad path, bad > + * certificate, bad password, or something else wrong > + * with the certificate. So we reject the credential to > + * avoid caching or saving a bad password. > + */ > + credential_reject(&http_auth); > + return HTTP_NOAUTH; > } else if (missing_target(results)) > return HTTP_MISSING_TARGET; > else if (results->http_code == 401) {
On 2021-03-12 at 00:48:41, John Szakmeister wrote: > We already looked for the PKI credentials in the credential store, but > failed to approve it on success. Meaning, the PKI certificate password > was never stored and git would request it on every connection to the > remote. Let's complete the chain by storing the certificate password on > success. > > Likewise, we also need to reject the credential when there is a failure. > Curl appears to report client-related certificate issues are reported > with the CURLE_SSL_CERTPROBLEM error. This includes not only a bad > password, but potentially other client certificate related problems. > Since we cannot get more information from curl, we'll go ahead and > reject the credential upon receiving that error, just to be safe and > avoid caching or saving a bad password. > > Signed-off-by: John Szakmeister <john@szakmeister.net> > --- > http.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/http.c b/http.c > index f8ea28bb2e..12a8aaba48 100644 > --- a/http.c > +++ b/http.c > @@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results) > credential_approve(&http_auth); > if (proxy_auth.password) > credential_approve(&proxy_auth); > + credential_approve(&cert_auth); > return HTTP_OK; > + } else if (results->curl_result == CURLE_SSL_CERTPROBLEM) { > + /* > + * We can't tell from here whether it's a bad path, bad > + * certificate, bad password, or something else wrong > + * with the certificate. So we reject the credential to > + * avoid caching or saving a bad password. > + */ > + credential_reject(&http_auth); Is this supposed to be &cert_auth here? I'm not sure how a bad HTTP password would even have been tested in this case.
On Fri, Mar 12, 2021 at 01:41:30AM +0000, brian m. carlson wrote: > > diff --git a/http.c b/http.c > > index f8ea28bb2e..12a8aaba48 100644 > > --- a/http.c > > +++ b/http.c > > @@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results) > > credential_approve(&http_auth); > > if (proxy_auth.password) > > credential_approve(&proxy_auth); > > + credential_approve(&cert_auth); > > return HTTP_OK; > > + } else if (results->curl_result == CURLE_SSL_CERTPROBLEM) { > > + /* > > + * We can't tell from here whether it's a bad path, bad > > + * certificate, bad password, or something else wrong > > + * with the certificate. So we reject the credential to > > + * avoid caching or saving a bad password. > > + */ > > + credential_reject(&http_auth); > > Is this supposed to be &cert_auth here? I'm not sure how a bad HTTP > password would even have been tested in this case. Good catch! When reviewing, I was so busy thinking about _where_ this line should go that I didn't even notice what it said. :) -Peff
On Thu, Mar 11, 2021 at 8:45 PM Jeff King <peff@peff.net> wrote: > > On Fri, Mar 12, 2021 at 01:41:30AM +0000, brian m. carlson wrote: > > > > diff --git a/http.c b/http.c > > > index f8ea28bb2e..12a8aaba48 100644 > > > --- a/http.c > > > +++ b/http.c > > > @@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results) > > > credential_approve(&http_auth); > > > if (proxy_auth.password) > > > credential_approve(&proxy_auth); > > > + credential_approve(&cert_auth); > > > return HTTP_OK; > > > + } else if (results->curl_result == CURLE_SSL_CERTPROBLEM) { > > > + /* > > > + * We can't tell from here whether it's a bad path, bad > > > + * certificate, bad password, or something else wrong > > > + * with the certificate. So we reject the credential to > > > + * avoid caching or saving a bad password. > > > + */ > > > + credential_reject(&http_auth); > > > > Is this supposed to be &cert_auth here? I'm not sure how a bad HTTP > > password would even have been tested in this case. > > Good catch! When reviewing, I was so busy thinking about _where_ this > line should go that I didn't even notice what it said. :) Good catch! I don't even know how I did that. :-/ The system I created the patch on is inaccessible via the Internet and I can't really get data off of it. This is entirely an error in translation on my part. The diff I printed has the correct line. My bad. I'll send an update soon. John
diff --git a/http.c b/http.c index f8ea28bb2e..12a8aaba48 100644 --- a/http.c +++ b/http.c @@ -1637,7 +1637,17 @@ static int handle_curl_result(struct slot_results *results) credential_approve(&http_auth); if (proxy_auth.password) credential_approve(&proxy_auth); + credential_approve(&cert_auth); return HTTP_OK; + } else if (results->curl_result == CURLE_SSL_CERTPROBLEM) { + /* + * We can't tell from here whether it's a bad path, bad + * certificate, bad password, or something else wrong + * with the certificate. So we reject the credential to + * avoid caching or saving a bad password. + */ + credential_reject(&http_auth); + return HTTP_NOAUTH; } else if (missing_target(results)) return HTTP_MISSING_TARGET; else if (results->http_code == 401) {
We already looked for the PKI credentials in the credential store, but failed to approve it on success. Meaning, the PKI certificate password was never stored and git would request it on every connection to the remote. Let's complete the chain by storing the certificate password on success. Likewise, we also need to reject the credential when there is a failure. Curl appears to report client-related certificate issues are reported with the CURLE_SSL_CERTPROBLEM error. This includes not only a bad password, but potentially other client certificate related problems. Since we cannot get more information from curl, we'll go ahead and reject the credential upon receiving that error, just to be safe and avoid caching or saving a bad password. Signed-off-by: John Szakmeister <john@szakmeister.net> --- http.c | 10 ++++++++++ 1 file changed, 10 insertions(+)