From: Stanislav M <stanislav.malishevskiy@gmail.com>
Date: Mon, 20 Mar 2023 18:21:44 GMT
Subject: Re: [PATCH v2] http: add support for different sslcert and sslkey types.
Message-ID: <CAEpdKf=THU=sc3S3W4azBmYp7+J945GxFqhh_i-5kV0kunMdNw@mail.gmail.com>
In-Reply-To: <20230320171051.GA2615782@coredump.intra.peff.net>

> > @@ -1014,10 +1020,14 @@ static CURL *get_curl_handle(void)
> >
> >       if (ssl_cert)
> >               curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
> > +     if (ssl_cert_type)
> > +             curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type);
>
> We're just feeding curl whatever string the user gave us (which is good,
> since we don't know which ones are valid). But what happens with:
>
>   GIT_SSL_CERT_TYPE=bogus git fetch ...
>
> Should we check for an error here, or will the actual request later
> complain properly?


Curl itself validates that string. And if we pass the wrong type or
not pass 'ENG' in case of pkcs11: curl will return an error. In that
case git do the same if GIT_SSL_CERT passed wrong ss 'ENG' in case of
pkcs11: curl will return an error. In that case git do the same if
GIT_SSL_CERT passed wrong


пн, 20 мар. 2023 г. в 20:10, Jeff King <peff@peff.net>:
>
> On Mon, Mar 20, 2023 at 03:48:49PM +0000, Stanislav Malishevskiy via GitGitGadget wrote:
>
> > From: Stanislav Malishevskiy <s.malishevskiy@auriga.com>
> >
> > Basically git work with default curl ssl type - PEM. But for support
> > eTokens like SafeNet tokens via pksc11 need setup 'ENG' as sslcert type
> > and as sslkey type. So there added additional options for http to make
> > that possible.
>
> Seems like a reasonable thing to want, and the patch looks pretty clean.
> Two small points:
>
> >  http.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
>
> There are no tests here. I think it might be possible to add them, but
> our https test support is currently optional, and has bit-rotted a bit
> over the years. There's some discussion here:
>
>   https://lore.kernel.org/git/Y9s7vyHKXP+TQPRm@pobox.com/
>
> So I don't think it makes sense to block this patch over the lack of
> tests, but it's something we might keep in mind to add if the test
> situation improves.
>
> > @@ -1014,10 +1020,14 @@ static CURL *get_curl_handle(void)
> >
> >       if (ssl_cert)
> >               curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
> > +     if (ssl_cert_type)
> > +             curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type);
>
> We're just feeding curl whatever string the user gave us (which is good,
> since we don't know which ones are valid). But what happens with:
>
>   GIT_SSL_CERT_TYPE=bogus git fetch ...
>
> Should we check for an error here, or will the actual request later
> complain properly?
>
> -Peff