New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix SSL client certificate not found on macOS Sierra #1105
Conversation
@schweikert, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickzman, @ldx and @bagder to be potential reviewers. |
This is not working correctly yet... |
query_dict = CFDictionaryCreate(NULL, (const void **)keys, | ||
(const void **)values, 4L, | ||
&kCFCopyStringDictionaryKeyCallBacks, | ||
&kCFTypeDictionaryValueCallBacks); | ||
CFRelease(values[3]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you get rid of this CFRelease
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a mistake. I will readd it. Thanks for noticing.
keys[3] = kSecMatchPolicy; | ||
/* match the name of the certificate (this doesn't seem to work :( ) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this doesn't work why are you proposing it? I'm not familiar with this code so I'm not doing anything more than a cursory look, but I don't understand this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the impression that it is a bug in Macos Sierra that it isn't working this way. Still, it might work correctly with later Macos versions and seems to be the right thing to use the API to do what is being attempted here.
@@ -900,21 +903,43 @@ static OSStatus CopyIdentityWithLabel(char *label, | |||
keys[0] = kSecClass; | |||
values[1] = kCFBooleanTrue; /* we want a reference */ | |||
keys[1] = kSecReturnRef; | |||
values[2] = kSecMatchLimitOne; /* one is enough, thanks */ | |||
values[2] = kSecMatchLimitAll; /* one is enough, thanks */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if an unlimited number of results may be returned then the comment "one is enough" is wrong isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed...
Have you built your change with --enable-debug turned on at configure time? I tried building your branch, but it's failing due to a number of coding style warnings. Please fix those. Also, sorry to be Captain Pedantic here, but it's "macOS," not "Macos". |
@nickzman: I have fixed the code style (and also a minor issue with the matching that I found) |
Okay, you fixed the problem, but you blew away support for Leopard and Snow Leopard users, since SecItemCopyMatching() didn't show up until Snow Leopard, and kSecClassIdentity didn't show up until Lion. We go as far back as supporting Leopard, so can you please restore the API check previously located at line 891, and the call to CopyIdentityWithLabelOldSchool() previously at lines 918-923 if the check fails? |
@nickzman: it looks that way, but the call to CopyIdentityWithLabelOldSchool for older macOS versions is still there, at line 955 |
&kCFCopyStringDictionaryKeyCallBacks, | ||
&kCFTypeDictionaryValueCallBacks); | ||
CFRelease(values[3]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do column alignment so that continuation lines are inward from the parentheses, like
func(a,
b)
query_dict = CFDictionaryCreate(NULL, (const void **)keys,
(const void **)values, 5L,
&kCFCopyStringDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks);
also I think CFRelease(values[3]);
should now be moved to the others, query_dict etc
It's still there, but if the app was built on Lion and later but built with a deployment target of Snow Leopard, then the kSecClassIdentity constant will be weak-linked. That is why the API check was present. |
@nickzman: Thanks for the explanation. I have reverted that change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. It's unfortunate we have to work around Apple's bug, but the change does fix the problem for me.
Thanks a lot. Both of you. I squashed and pushed just now! |
Just wanted to let you guys know - I've built curl from the source (7.52.0-DEV) and it did not do the trick with accessing the keychain again. Got the same error that was reported on top. |
I just tested it again with the latest nightly snapshot, and it works for me. Make sure that you configure with '--with-darwinssl'. |
I've just downloaded the latest HEAD from GitHub and did a configuration with '--with-darwinssl' option enabled. 'make' and 'sudo make install' afterwards - the 'curl --version' command tells me it is the latest version. The keychain is showing the certificate producing the errors. |
I am not going to comment further on this, but here is my last tip: test with 'curl -E "Your Name"' directly to see if curl works, and then move on to git and libcurl... |
Ok sorry - I'm not that much into curl, but the tip you gave me works perfect (I don't know tho if the old version would deny working). I thought libcurl would be part of curl. Well nevermind - seems like the fix works - just not for my setup ;-) Update: with 7.51.0 the curl command works aswell... |
This change appears to have broken iOS builds, as |
Then please file a proper bug/issue, a comment here will just vanish... |
Will do. |
curl can't retrieve a client certificate on MacOS Sierra. The reason seems to be that the search for the identity is not done correctly. See also this, which a symptom for the same problem:
git/git-scm.com#850