http: add support for different sslcert and sslkey types. #1474
Conversation
Welcome to GitGitGadgetHi @smalishevskiy, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
/allow |
|
User smalishevskiy is now allowed to use GitGitGadget. WARNING: smalishevskiy has no public email address set on GitHub; |
|
/submit |
|
Error: Could not determine full name of smalishevskiy |
|
/submit |
|
Submitted as pull.1474.git.git.1679233875803.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
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. Signed-off-by: Stanislav Malishevskiy <stanislav.malishevskiy@gmail.com>
|
/submit |
|
Error: a4b2284 was already submitted |
smalishevskiy
force-pushed
the
ssl_types_support
branch
from
a4b2284
to
899a076
Compare
smalishevskiy
changed the title
|
/submit |
|
Submitted as pull.1474.v2.git.git.1679327330032.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Jeff King wrote (reply to this): 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 |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Stanislav Malishevskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> 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.
>
> Signed-off-by: Stanislav Malishevskiy <stanislav.malishevskiy@gmail.com>
> ---
> http.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
Thanks.
Is this something that can be protected from future breakage with a
few new tests somewhere in t/t5559 (which may also involve touching
t/lib-httpd.sh as well)? |
|
On the Git mailing list, Stanislav M wrote (reply to this): > > @@ -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 |
|
On the Git mailing list, Stanislav M wrote (reply to this): > Is this something that can be protected from future breakage with a
> few new tests somewhere in t/t5559 (which may also involve touching
> t/lib-httpd.sh as well)?
I am not sure about that. That required only for local access to the
cert and key from curl
пн, 20 мар. 2023 г. в 20:23, Junio C Hamano <gitster@pobox.com>:
>
> "Stanislav Malishevskiy via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > 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.
> >
> > Signed-off-by: Stanislav Malishevskiy <stanislav.malishevskiy@gmail.com>
> > ---
>
> > http.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
>
> Thanks.
>
> Is this something that can be protected from future breakage with a
> few new tests somewhere in t/t5559 (which may also involve touching
> t/lib-httpd.sh as well)? |
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Mar 20, 2023 at 10:23:41AM -0700, Junio C Hamano wrote:
> Is this something that can be protected from future breakage with a
> few new tests somewhere in t/t5559 (which may also involve touching
> t/lib-httpd.sh as well)?
I don't think we'd want to put it there. While it is the only test that
requires SSL currently, it's also specific to HTTP/2, so it may not get
run everywhere.
The original design of the SSL support in the test suite was that you'd
do a run of the whole suite with LIB_HTTPD_SSL set, and then it would
run all of the usual tests with ssl. But experience has shown that
nobody does that. So I think there are two paths forward here:
1. Add a mode to CI that runs with LIB_HTTPD_SSL set. We'd need to fix
the bit-rotted tests that fail (usually due to things like
expecting "http" in the output instead of "https", etc). I linked
to earlier discussion there elsewhere in the thread.
2. Add a specific "test with https" script that covers some basic
tests (possibly even just including t5551, in the same way t5559
does). If the platform apache doesn't support ssl, then it should
fail gracefully. And then we could add more SSL specific tests
to that script.
-Peff |
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Mar 20, 2023 at 09:21:44PM +0300, Stanislav M wrote:
> > > @@ -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
That sounds great. Thanks for confirming!
-Peff |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <peff@peff.net> writes:
> 2. Add a specific "test with https" script that covers some basic
> tests (possibly even just including t5551, in the same way t5559
> does). If the platform apache doesn't support ssl, then it should
> fail gracefully. And then we could add more SSL specific tests
> to that script.
Both choices sound sensible. This second one may be simpler to
arrange.
Thanks. |
|
On the Git mailing list, Stanislav M wrote (reply to this): I want to do some clarification there. Those changes are not related
to http or https directly.
That only configures curl to know how to access the certificate and
key locally on the client box, so if the way is wrong there is a
simple error: Certificate or key not found.
вт, 21 мар. 2023 г. в 20:43, Junio C Hamano <gitster@pobox.com>:
>
> Jeff King <peff@peff.net> writes:
>
> > 2. Add a specific "test with https" script that covers some basic
> > tests (possibly even just including t5551, in the same way t5559
> > does). If the platform apache doesn't support ssl, then it should
> > fail gracefully. And then we could add more SSL specific tests
> > to that script.
>
> Both choices sound sensible. This second one may be simpler to
> arrange.
>
> Thanks. |
|
On the Git mailing list, Jeff King wrote (reply to this): On Thu, Mar 23, 2023 at 12:33:59PM +0300, Stanislav M wrote:
> I want to do some clarification there. Those changes are not related
> to http or https directly.
> That only configures curl to know how to access the certificate and
> key locally on the client box, so if the way is wrong there is a
> simple error: Certificate or key not found.
Yes, but I'm not sure if there is a way for Git to trigger curl to look
at the certificate that does not involve feeding it an https URL (and we
want a valid one, because we want to see that it correctly speaks to the
server).
-Peff |
|
On the Git mailing list, Stanislav M wrote (reply to this): In my opinion they need the same set of tests which is used as usual
for https. But use the right certificate and key.
But I don't have any idea how to do that with hardware usb eToken in my case.
чт, 23 мар. 2023 г. в 21:02, Jeff King <peff@peff.net>:
>
> On Thu, Mar 23, 2023 at 12:33:59PM +0300, Stanislav M wrote:
>
> > I want to do some clarification there. Those changes are not related
> > to http or https directly.
> > That only configures curl to know how to access the certificate and
> > key locally on the client box, so if the way is wrong there is a
> > simple error: Certificate or key not found.
>
> Yes, but I'm not sure if there is a way for Git to trigger curl to look
> at the certificate that does not involve feeding it an https URL (and we
> want a valid one, because we want to see that it correctly speaks to the
> server).
>
> -Peff |
|
This branch is now known as |
|
This patch series was integrated into seen via f8c3b81. |
|
This patch series was integrated into seen via a612e5e. |
|
This patch series was integrated into seen via cb21295. |
|
This patch series was integrated into seen via c2693cb. |
|
This patch series was integrated into seen via fb5677b. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Stanislav M <stanislav.malishevskiy@gmail.com> writes:
[administrivia: do not top-post]
>> Yes, but I'm not sure if there is a way for Git to trigger curl to look
>> at the certificate that does not involve feeding it an https URL (and we
>> want a valid one, because we want to see that it correctly speaks to the
>> server).
> ...
> In my opinion they need the same set of tests which is used as usual
> for https. But use the right certificate and key.
> But I don't have any idea how to do that with hardware usb eToken in my case.
OK, so where does this put us, with respect to the change? We have
some behaviour change that we do not know how to test? How would we
know when we break it in the future? It is not like the new feature
is not useful enough that nobody would care if it gets broken by
accident or anything like that, so...?
At least perhaps we can throw bogus strings in the environment and
make sure cURL library gives complaints, or something?
Thanks. |
|
On the Git mailing list, Stanislav M wrote (reply to this): Yes. If you set bogus strings in the environment cURL should return
an error the same as if you set the wrong file for certificate or key.
So you can set
GIT_SSL_CERT=some_real_pem_file - That should work (PEM type used by default)
GIT_SSL_CERT=some_real_pem_file GIT_SSL_CERT_TYPE=PEM - That should work too
GIT_SSL_CERT=some_real_pem_file GIT_SSL_CERT_TYPE=Bogus - That shouldn't work
GIT_SSL_CERT=some_real_der_file GIT_SSL_CERT_TYPE=DER - I am not sure
about that, because as I far remember there issue with DER in openssl
I think that more detailed information there:
https://curl.se/libcurl/c/CURLOPT_SSLKEYTYPE.html
Basically that only a format of cert and key file or engine in case of
pkcs11 url instead of file in others cases.
So if you set it into right values, respect your ssl cert and ssl key
- https should work. But if not, error from curl should returned
ср, 29 мар. 2023 г. в 21:53, Junio C Hamano <gitster@pobox.com>:
>
> Stanislav M <stanislav.malishevskiy@gmail.com> writes:
>
> [administrivia: do not top-post]
>
> >> Yes, but I'm not sure if there is a way for Git to trigger curl to look
> >> at the certificate that does not involve feeding it an https URL (and we
> >> want a valid one, because we want to see that it correctly speaks to the
> >> server).
> > ...
> > In my opinion they need the same set of tests which is used as usual
> > for https. But use the right certificate and key.
> > But I don't have any idea how to do that with hardware usb eToken in my case.
>
> OK, so where does this put us, with respect to the change? We have
> some behaviour change that we do not know how to test? How would we
> know when we break it in the future? It is not like the new feature
> is not useful enough that nobody would care if it gets broken by
> accident or anything like that, so...?
>
> At least perhaps we can throw bogus strings in the environment and
> make sure cURL library gives complaints, or something?
>
> Thanks. |
|
This patch series was integrated into seen via 87f0757. |
|
There was a status update in the "New Topics" section about the branch Add a few configuration variables to tell the cURL library that different types of ssl-cert and ssl-key are in use. Will merge to 'next'. source: <pull.1474.v2.git.git.1679327330032.gitgitgadget@gmail.com> |
|
On the Git mailing list, Jeff King wrote (reply to this): On Wed, Mar 29, 2023 at 11:53:11AM -0700, Junio C Hamano wrote:
> > In my opinion they need the same set of tests which is used as usual
> > for https. But use the right certificate and key.
> > But I don't have any idea how to do that with hardware usb eToken in my case.
>
> OK, so where does this put us, with respect to the change? We have
> some behaviour change that we do not know how to test? How would we
> know when we break it in the future? It is not like the new feature
> is not useful enough that nobody would care if it gets broken by
> accident or anything like that, so...?
>
> At least perhaps we can throw bogus strings in the environment and
> make sure cURL library gives complaints, or something?
I would be OK taking the patches without any further tests. It is not
really making anything worse in the sense that we already do not test
any of the client-cert stuff.
If we can cheaply add some tests that at least exercise the code and
hand off to curl, that is better than nothing, I guess.
I think the ideal would be a new t5565 that sets LIB_HTTPD_SSL
unconditionally and actually tests various client-cert formats and
requests using a made-on-the-fly cert. But I don't want to hold up a
patch we otherwise think is OK on the basis of long-term technical debt
we already have.
-Peff |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <peff@peff.net> writes:
> On Wed, Mar 29, 2023 at 11:53:11AM -0700, Junio C Hamano wrote:
>
> ...
>> > But I don't have any idea how to do that with hardware usb eToken in my case.
>>
>> OK, so where does this put us, with respect to the change? ...
> I would be OK taking the patches without any further tests. It is not
> really making anything worse in the sense that we already do not test
> any of the client-cert stuff.
Alright. I think the patch has already been queued for some time,
and it is OK to merge it down to 'next'.
> If we can cheaply add some tests that at least exercise the code and
> hand off to curl, that is better than nothing, I guess.
>
> I think the ideal would be a new t5565 that sets LIB_HTTPD_SSL
> unconditionally and actually tests various client-cert formats and
> requests using a made-on-the-fly cert.
Thanks.
|
|
This patch series was integrated into seen via b4f2403. |
|
This patch series was integrated into seen via 7cbb895. |
|
This patch series was integrated into next via 773716f. |
|
This patch series was integrated into seen via b2a00ca. |
|
This patch series was integrated into seen via 82a1058. |
|
This patch series was integrated into seen via d668c2e. |
|
This patch series was integrated into seen via b803db4. |
|
This patch series was integrated into seen via 65d252f. |
|
There was a status update in the "Cooking" section about the branch Add a few configuration variables to tell the cURL library that different types of ssl-cert and ssl-key are in use. Will merge to 'master'. source: <pull.1474.v2.git.git.1679327330032.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via a180248. |
|
This patch series was integrated into seen via 0a8c337. |
|
This patch series was integrated into master via 0a8c337. |
|
This patch series was integrated into next via 0a8c337. |
|
Closed via 0a8c337. |
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.
Signed-off-by: Stanislav Malishevskiy stanislav.malishevskiy@gmail.com
Changes since v1:
cc: Jeff King peff@peff.net