v2, 2 partshttp: store credential when PKI auth is used

> 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);
> +		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(+)
> 
> 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);

> > 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. :)

> Here's my second attempt at getting the certificate password into the credential
> store.  I tested from a working PKI setup and found curl--at least reasonable
> recent versions of it--return CURLE_SSL_CERTPROBLEM:
> 
>        CURLE_SSL_CERTPROBLEM (58)
>               problem with the local client certificate.
> 
> It appears there could be another possible error from curl:
> 
>        CURLE_SSL_CONNECT_ERROR (35)
>               A  problem  occurred  somewhere  in the SSL/TLS handshake. You
>               really want the error buffer and read the message there as it
>               pinpoints the problem slightly more. Could be  certificates  (file
>               formats, paths, permissions), passwords, and others.
> 
> This seems less likely to be a bad client password scenario, so I did not look
> for this particular error to reject it.
> 
> I also added one other small patch to remove the check of a non-empty password
> before calling credential_store() for proxy_auth, as credential_store() already
> checks for a non-empty password and gracefully handles it when it doesn't.

> Thanks. Both patches look good to me. I wondered briefly if we needed to
> worry about old versions of curl missing CURLE_SSL_CERTPROBLEM. But it
> seems to have shown up in ~2002, so I think we are fine to assume it's
> there.
>
> It would be nice if we had some tests here, but we currently do not
> cover any of the ssl-cert stuff in the test suite. I suspect adding them
> would be a big pain to configure and maintain, so I'm OK to leave it off
> for now. Hopefully you gave it some basic manual testing with your
> working setup (good password is stored, bad password is rejected).

> Looking at how we generate the server-side cert for our http tests, we
> could _probably_ do something similar for a client-side cert, and just
> configure the server to accept a self-signed certificate. But like I
> said, I'm OK to leave that for another series (though of course if you
> want to work on it, that would be very much appreciated).