Skip to content
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

Closed
wants to merge 11 commits into from

Conversation

schweikert
Copy link
Contributor

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

@mention-bot
Copy link

@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.

@schweikert
Copy link
Contributor Author

This is not working correctly yet...

@schweikert schweikert closed this Nov 4, 2016
@schweikert schweikert reopened this Nov 4, 2016
query_dict = CFDictionaryCreate(NULL, (const void **)keys,
(const void **)values, 4L,
&kCFCopyStringDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks);
CFRelease(values[3]);
Copy link
Member

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

Copy link
Contributor Author

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 :( ) */
Copy link
Member

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.

Copy link
Contributor Author

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 */
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed...

@bagder bagder added the TLS label Nov 5, 2016
@nickzman
Copy link
Member

nickzman commented Nov 8, 2016

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".

@schweikert
Copy link
Contributor Author

@nickzman: I have fixed the code style (and also a minor issue with the matching that I found)

@nickzman
Copy link
Member

nickzman commented Nov 9, 2016

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?

@schweikert
Copy link
Contributor Author

@nickzman: it looks that way, but the call to CopyIdentityWithLabelOldSchool for older macOS versions is still there, at line 955

@schweikert schweikert changed the title fix SSL client certificate not found on MacOS Sierra fix SSL client certificate not found on macOS Sierra Nov 9, 2016
&kCFCopyStringDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks);
CFRelease(values[3]);

Copy link
Member

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

@nickzman
Copy link
Member

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.

@schweikert
Copy link
Contributor Author

@nickzman: Thanks for the explanation. I have reverted that change.

Copy link
Member

@nickzman nickzman left a 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.

@bagder bagder closed this in 7c9b9ad Nov 15, 2016
@bagder
Copy link
Member

bagder commented Nov 15, 2016

Thanks a lot. Both of you. I squashed and pushed just now!

@mschel
Copy link

mschel commented Dec 6, 2016

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.

@schweikert
Copy link
Contributor Author

I just tested it again with the latest nightly snapshot, and it works for me. Make sure that you configure with '--with-darwinssl'.

@mschel
Copy link

mschel commented Dec 6, 2016

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'm using git from homebrew with no option installed (particular not '--with-brewed-curl') so it should fall back on my systems curl.

@schweikert
Copy link
Contributor Author

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...

@mschel
Copy link

mschel commented Dec 6, 2016

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 ;-)
Thanks!

Update: with 7.51.0 the curl command works aswell...

@chris-araman
Copy link
Contributor

This change appears to have broken iOS builds, as SecCertificateCopyCommonName does not exist there.

@bagder
Copy link
Member

bagder commented Dec 22, 2016

Then please file a proper bug/issue, a comment here will just vanish...

@chris-araman
Copy link
Contributor

Will do.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

7 participants