Florian Mickler [off-list ref] writes:
Is there a specific reason, that patch wasn't merged? It would allow
for non-pem ssl certificates to be loaded also (without pkcs11 at all).
I realize, that the underlying systems could and should set up
everything automagically as soon as i point them to the certificate that
i want to use. But not opening up these CURL Settings from git seems
kind of silly given that today's systems still seem kinda borked and do
not do that. What harm comes from these two tuning knobs being exposed?
Best regards,
Flo
[1] https://marc.info/?l=git&m=136675822032549&w=2
Almost always, when some patch aims to achieve a worthy goal, and in
the initial discussion on the thread more experienced project
members agree it is a worthwhile thing to do, the only reason why
the feature proposed does not materialize in later versions of Git
is because the developer with the original itch did not follow it
through after getting review comments and saying something that
makes reviewers to expect an updated version of the patch.
I didn't follow your marc.info URL, but I am reasonably sure, if I
were involved in the discussion, that would be the likely reason.
Hi Junio & Florian,
On Fri, 17 Dec 2021, Junio C Hamano wrote:
Florian Mickler [off-list ref] writes:
quoted
Is there a specific reason, that patch wasn't merged? It would allow
for non-pem ssl certificates to be loaded also (without pkcs11 at all).
I realize, that the underlying systems could and should set up
everything automagically as soon as i point them to the certificate that
i want to use. But not opening up these CURL Settings from git seems
kind of silly given that today's systems still seem kinda borked and do
not do that. What harm comes from these two tuning knobs being exposed?
Best regards,
Flo
[1] https://marc.info/?l=git&m=136675822032549&w=2
This corresponds to
https://lore.kernel.org/git/1366758207-7724-1-git-send-email-jqassar@gmail.com/
(for those who prefer lore.kernel.org over marc.info, and for those who
want to look for the Message-ID directly).
My summary of that thread:
- The patch implements something Git wants to support.
- A couple of improvements need to be made, such as:
* Error-checking needed to be improved
* Adding a hint to the documentation of `http.sslKeyType` being set to
`ENG` causing `http.sslKey` being interpreted differently.
* Adding regression tests, if possible
* Maybe a more complete commit message?
- Testing the smart card support was considered hard, especially given
that the contributor still wanted to contribute patches to cURL without
which it wouldn't work.
The patch seems to have been contributed via
https://curl.se/mail/lib-2013-04/0340.html, been reviewed and changes
were requested, but there was no other patch submission that I could
find.
However, over five years later, what looks like an equivalent fix to me
was applied:
https://github.com/curl/curl/commit/4939f3652473c1519d2b604068efb87ef7531874
- The contributor, Jerry Qassar, gave all the signs of working on a
next iteration ("reroll", as Junio likes to call it). But that never
materialized, either:
https://lore.kernel.org/git/?q=f%3Ajqassar
Based on this, the lack of a cURL contribution, and a quick web search
for the name "Jerry Qassar" I somehow doubt that Cc:ing them might
raise their attention.
Almost always, when some patch aims to achieve a worthy goal, and in
the initial discussion on the thread more experienced project
members agree it is a worthwhile thing to do, the only reason why
the feature proposed does not materialize in later versions of Git
is because the developer with the original itch did not follow it
through after getting review comments and saying something that
makes reviewers to expect an updated version of the patch.
I didn't follow your marc.info URL, but I am reasonably sure, if I
were involved in the discussion, that would be the likely reason.
Yes, you were involved in the discussion, and indeed, there was no
follow-up.
After more than 8 years, I do believe that the patch is fair game to be
picked up by any other interested contributor who might want to contribute
v2 (hint, hint, Florian... all it would take is to study the mail thread
from way back when and adapt the patch accordingly, of course after
rebasing it to a recent Git revision).
Ciao,
Dscho
Thank you for the summary!
I am currently looking into it. Got into a conversation with dwmw2 also about fixing libcurl or even openssl after i found some issues he opened about this weird interface. Which really sounds like it would be the real fix. (Although probably slower to trickle to end users and more complex)
For this patch i guess I will skip the engine variable, because it is not needed for my use case and my lib version. (Although I am still experimenting with the whole pkcs11 stuff)
That makes the error handling easier (none needed, imo).
The git libcurl min version is also raised above the introduction of those curlopts, so no libcurl-version-bracing needed.
I was looking into the testing framework, it might be possible to configure the https to require client auth on a different port and then testing that with pem and der file.
Adding pkcs11 infrastructure to the test harness might be a bit over the top though :-)
Best regards,
Flo
Am 20. Dezember 2021 23:21:28 MEZ schrieb Johannes Schindelin [off-list ref]:
Hi Junio & Florian,
On Fri, 17 Dec 2021, Junio C Hamano wrote:
quoted
Florian Mickler [off-list ref] writes:
quoted
Is there a specific reason, that patch wasn't merged? It would
allow
quoted
quoted
for non-pem ssl certificates to be loaded also (without pkcs11 at
all).
quoted
quoted
I realize, that the underlying systems could and should set up
everything automagically as soon as i point them to the certificate
that
quoted
quoted
i want to use. But not opening up these CURL Settings from git
seems
quoted
quoted
kind of silly given that today's systems still seem kinda borked
and do
quoted
quoted
not do that. What harm comes from these two tuning knobs being
exposed?
quoted
quoted
Best regards,
Flo
[1] https://marc.info/?l=git&m=136675822032549&w=2
This corresponds to
https://lore.kernel.org/git/1366758207-7724-1-git-send-email-jqassar@gmail.com/
(for those who prefer lore.kernel.org over marc.info, and for those who
want to look for the Message-ID directly).
My summary of that thread:
- The patch implements something Git wants to support.
- A couple of improvements need to be made, such as:
* Error-checking needed to be improved
* Adding a hint to the documentation of `http.sslKeyType` being set to
`ENG` causing `http.sslKey` being interpreted differently.
* Adding regression tests, if possible
* Maybe a more complete commit message?
- Testing the smart card support was considered hard, especially given
that the contributor still wanted to contribute patches to cURL without
which it wouldn't work.
The patch seems to have been contributed via
https://curl.se/mail/lib-2013-04/0340.html, been reviewed and changes
were requested, but there was no other patch submission that I could
find.
However, over five years later, what looks like an equivalent fix to me
was applied:
https://github.com/curl/curl/commit/4939f3652473c1519d2b604068efb87ef7531874
- The contributor, Jerry Qassar, gave all the signs of working on a
next iteration ("reroll", as Junio likes to call it). But that never
materialized, either:
https://lore.kernel.org/git/?q=f%3Ajqassar
Based on this, the lack of a cURL contribution, and a quick web search
for the name "Jerry Qassar" I somehow doubt that Cc:ing them might
raise their attention.
quoted
Almost always, when some patch aims to achieve a worthy goal, and in
the initial discussion on the thread more experienced project
members agree it is a worthwhile thing to do, the only reason why
the feature proposed does not materialize in later versions of Git
is because the developer with the original itch did not follow it
through after getting review comments and saying something that
makes reviewers to expect an updated version of the patch.
I didn't follow your marc.info URL, but I am reasonably sure, if I
were involved in the discussion, that would be the likely reason.
Yes, you were involved in the discussion, and indeed, there was no
follow-up.
After more than 8 years, I do believe that the patch is fair game to be
picked up by any other interested contributor who might want to
contribute
v2 (hint, hint, Florian... all it would take is to study the mail
thread
from way back when and adapt the patch accordingly, of course after
rebasing it to a recent Git revision).
Ciao,
Dscho
---
Signature
On Tue, Dec 21 2021, Florian Mickler wrote:
Thank you for the summary!
I am currently looking into it. Got into a conversation with dwmw2
also about fixing libcurl or even openssl after i found some issues he
opened about this weird interface. Which really sounds like it would
be the real fix. (Although probably slower to trickle to end users and
more complex)
For this patch i guess I will skip the engine variable, because it is not needed for my use case and my lib version. (Although I am still experimenting with the whole pkcs11 stuff)
That makes the error handling easier (none needed, imo).
The git libcurl min version is also raised above the introduction of those curlopts, so no libcurl-version-bracing needed.
I was looking into the testing framework, it might be possible to configure the https to require client auth on a different port and then testing that with pem and der file.
Adding pkcs11 infrastructure to the test harness might be a bit over the top though :-)
It would be really nice to have https tests in the test suite, but we
don't have any currently.
There was some discussion about setting them up around some other recent
libcurl related changes, but it hasn't been done.
So if you want to work on that it would be fantastic, but don't consider
it a blocker to just get CURLOPT_SSLCERTTYPE into git.git.
We basically assume that we don't need to re-test libcurl itself, and
since these settings amount to just setting various flags in the library
the need for them isn't all that great.
Am 20. Dezember 2021 23:21:28 MEZ schrieb Johannes Schindelin [off-list ref]:
quoted
Hi Junio & Florian,
On Fri, 17 Dec 2021, Junio C Hamano wrote:
quoted
Florian Mickler [off-list ref] writes:
quoted
Is there a specific reason, that patch wasn't merged? It would
allow
quoted
quoted
for non-pem ssl certificates to be loaded also (without pkcs11 at
all).
quoted
quoted
I realize, that the underlying systems could and should set up
everything automagically as soon as i point them to the certificate
that
quoted
quoted
i want to use. But not opening up these CURL Settings from git
seems
quoted
quoted
kind of silly given that today's systems still seem kinda borked
and do
quoted
quoted
not do that. What harm comes from these two tuning knobs being
exposed?
quoted
quoted
Best regards,
Flo
[1] https://marc.info/?l=git&m=136675822032549&w=2
This corresponds to
https://lore.kernel.org/git/1366758207-7724-1-git-send-email-jqassar@gmail.com/
(for those who prefer lore.kernel.org over marc.info, and for those who
want to look for the Message-ID directly).
My summary of that thread:
- The patch implements something Git wants to support.
- A couple of improvements need to be made, such as:
* Error-checking needed to be improved
* Adding a hint to the documentation of `http.sslKeyType` being set to
`ENG` causing `http.sslKey` being interpreted differently.
* Adding regression tests, if possible
* Maybe a more complete commit message?
- Testing the smart card support was considered hard, especially given
that the contributor still wanted to contribute patches to cURL without
which it wouldn't work.
The patch seems to have been contributed via
https://curl.se/mail/lib-2013-04/0340.html, been reviewed and changes
were requested, but there was no other patch submission that I could
find.
However, over five years later, what looks like an equivalent fix to me
was applied:
https://github.com/curl/curl/commit/4939f3652473c1519d2b604068efb87ef7531874
- The contributor, Jerry Qassar, gave all the signs of working on a
next iteration ("reroll", as Junio likes to call it). But that never
materialized, either:
https://lore.kernel.org/git/?q=f%3Ajqassar
Based on this, the lack of a cURL contribution, and a quick web search
for the name "Jerry Qassar" I somehow doubt that Cc:ing them might
raise their attention.
quoted
Almost always, when some patch aims to achieve a worthy goal, and in
the initial discussion on the thread more experienced project
members agree it is a worthwhile thing to do, the only reason why
the feature proposed does not materialize in later versions of Git
is because the developer with the original itch did not follow it
through after getting review comments and saying something that
makes reviewers to expect an updated version of the patch.
I didn't follow your marc.info URL, but I am reasonably sure, if I
were involved in the discussion, that would be the likely reason.
Yes, you were involved in the discussion, and indeed, there was no
follow-up.
After more than 8 years, I do believe that the patch is fair game to be
picked up by any other interested contributor who might want to
contribute
v2 (hint, hint, Florian... all it would take is to study the mail
thread
from way back when and adapt the patch accordingly, of course after
rebasing it to a recent Git revision).
Ciao,
Dscho
---
Signature