FIX pkcs11 uri with openssl backend

[Stormshield-robinc]

This is a fix for #5647

With configured our git with the following :

[http]
	sslBackend = openssl
	sslCert = "pkcs11:...."
	sslCertType = ENG
	sslKey = "pkcs11:..."
	sslKeyType = ENG
	sslCertPasswordProtected = 1

On linux this worked fine, but on windows it did not, despite having installed opensc drivers and configured openssl to use it (it seems that git does ignore the openssl config file).

I ended up trying the same uri with curl which, to my surprised, worked fine. This was even more surprising as both git and curl relied on the same libcurl.

Digging in the differences between curl and git, it seem that git does not set (neither exposes) the CURLOPT_SSLENGINE option that need to be set to "pkcs11" for this to work.

Curl side, when it sees a "pkcs11:" in the certificate it automatically assume a ENG type and set the SSLENGINE accordingly.

I made the fix with mostly the same idea

[dscho]

Thank you for contributing!

The patch in question is not specific to Windows, though. In light of that, I would like you to contribute this to the Git project (either "by hand", sending a plaintext-only email with the patch "non-whitespace-corrupted" to git@vger.kernel.org, or via GitGitGadget).

If the Git project accepts the patch, I am happy to fast-track it into Git for Windows.

[dscho]

On the Git mailing list, you'd most likely receive the feedback to use istarts_with() instead.

[dscho]

The is_pkcs11_uri() function does one thing in addition to looking for a specific prefix, and that's to test whether the argument is non-NULL. But that was done here already, so I don't know whether it's really worth having that inline function.

[dscho]

If I read this correctly, then ssl_cert_type is overridden here. I am not familiar enough with PKCS11 to know whether that's appropriate or not.

[Stormshield-robinc]

From a technical perspective, the curl_easy_setopt says it ok to set an option several times.
On the functional level, this does silently override a property which might not be acceptable for git mainteners

[dscho]

I guess that a check whether ssl_cert_type is either unset, or already contains ENG, would be needed, and if neither is the case a warning should be issued.

[dscho]

I took inspiration from curl code that force the key/cert type and sslengine values when it detect a pkcs11 uri in either sslkey of sslcert

Are you talking about libcurl code? If so, why do we have to repeat it in Git's code?

[dscho]

I would like you to contribute this to the Git project

@Stormshield-robinc I should also say that the Git project is very particular about their commit messages.
You will want to follow the guidance in https://github.blog/2022-06-30-write-better-commits-build-better-projects/ to improve the one-liner (and add your sign-off), in particular with a strong focus on this part:

	What you’re doing	Why you’re doing it
High-level (strategic)	Intent (what does this accomplish?)	Context (why does the code do what it does now?)
Low-level (tactical)	Implementation (what did you do to accomplish your goal?)	Justification (why is this change being made?)

[Stormshield-robinc]

Hi, thanks a lot for your returns, moreover since I am very new to git contributions.
I will fix the issues and submit it upstream

[dscho]

Hi, thanks a lot for your returns, moreover since I am very new to git contributions. I will fix the issues and submit it upstream

Thank you for understanding!

[Stormshield-robinc]

I took inspiration from curl code that force the key/cert type and sslengine values when it detect a pkcs11 uri in either sslkey of sslcert

Are you talking about libcurl code? If so, why do we have to repeat it in Git's code?

Nope, I am talking about the curl code. In my tests I add a curl working while git would not, despite both using the same libcurl.
I made a more in depth explanation in the issue : #5647

[dscho]

I made a more in depth explanation in the issue : #5647

Excellent. Please integrate this into the commit message, this information will be very welcomed by the reviewers.

[Stormshield-robinc]

Made the fixes, sorry for the delay.

I will see to forward the patch to the main git repository

[Stormshield-robinc]

The issue has been fixed upstream