FIX pkcs11 uri with openssl backend #5648
Conversation
There was a problem hiding this comment.
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 [email protected], or via GitGitGadget).
If the Git project accepts the patch, I am happy to fast-track it into Git for Windows.
http.c
Outdated
| @@ -1024,6 +1024,11 @@ static int get_curl_http_version_opt(const char *version_string, long *opt) | |||
| return -1; /* not found */ | |||
| } | |||
|
|
|||
| static bool is_pkcs11_uri(const char *string) | |||
| { | |||
| return string && strncasecmp(string, "pkcs11:", 7) == 0; | |||
There was a problem hiding this comment.
On the Git mailing list, you'd most likely receive the feedback to use istarts_with() instead.
http.c
Outdated
| if (ssl_cert_type) | ||
| curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, ssl_cert_type); | ||
| if (ssl_cert) { | ||
| curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert); | ||
| if (is_pkcs11_uri(ssl_cert)) { |
There was a problem hiding this comment.
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.
| if (ssl_cert) { | ||
| curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert); | ||
| if (is_pkcs11_uri(ssl_cert)) { | ||
| curl_easy_setopt(result, CURLOPT_SSLCERTTYPE, "ENG"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Are you talking about libcurl code? If so, why do we have to repeat it in Git's code? |
@Stormshield-robinc I should also say that the Git project is very particular about their commit messages.
|
|
Hi, thanks a lot for your returns, moreover since I am very new to git contributions. |
Thank you for understanding! |
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. |
Excellent. Please integrate this into the commit message, this information will be very welcomed by the reviewers. |
On windows, sslcert and sslkey containing pkcs11 uri does not work using the openssl backend. Fixed by forcing the correct libcurl option when detecting a pkcs11 uri, much like what the curl binary is doing. Signed-off-by: Robin Courgeon <[email protected]>
|
Made the fixes, sorry for the delay. I will see to forward the patch to the main git repository |
This is a fix for #5647
With configured our git with the following :
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