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
base: master
from

Conversation

Projects
None yet
7 participants
@schweikert
Contributor

schweikert commented Nov 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Nov 4, 2016

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

mention-bot commented Nov 4, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@schweikert

schweikert Nov 4, 2016

Contributor

This is not working correctly yet...

Contributor

schweikert commented Nov 4, 2016

This is not working correctly yet...

@schweikert schweikert closed this Nov 4, 2016

@schweikert schweikert reopened this Nov 4, 2016

Show outdated Hide outdated lib/vtls/darwinssl.c
query_dict = CFDictionaryCreate(NULL, (const void **)keys,
(const void **)values, 4L,
&kCFCopyStringDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks);
CFRelease(values[3]);

This comment has been minimized.

@jay

jay Nov 4, 2016

Member

Why did you get rid of this CFRelease

@jay

jay Nov 4, 2016

Member

Why did you get rid of this CFRelease

This comment has been minimized.

@schweikert

schweikert Nov 6, 2016

Contributor

This is a mistake. I will readd it. Thanks for noticing.

@schweikert

schweikert Nov 6, 2016

Contributor

This is a mistake. I will readd it. Thanks for noticing.

Show outdated Hide outdated lib/vtls/darwinssl.c
keys[3] = kSecMatchPolicy;
/* match the name of the certificate (this doesn't seem to work :( ) */

This comment has been minimized.

@jay

jay Nov 4, 2016

Member

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.

@jay

jay Nov 4, 2016

Member

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.

This comment has been minimized.

@schweikert

schweikert Nov 6, 2016

Contributor

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.

@schweikert

schweikert Nov 6, 2016

Contributor

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.

Show outdated Hide outdated lib/vtls/darwinssl.c
@@ -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 */

This comment has been minimized.

@jay

jay Nov 4, 2016

Member

if an unlimited number of results may be returned then the comment "one is enough" is wrong isn't it?

@jay

jay Nov 4, 2016

Member

if an unlimited number of results may be returned then the comment "one is enough" is wrong isn't it?

This comment has been minimized.

@schweikert

schweikert Nov 6, 2016

Contributor

Indeed...

@schweikert

schweikert Nov 6, 2016

Contributor

Indeed...

@bagder bagder added the SSL/TLS label Nov 5, 2016

schweikert added some commits Nov 6, 2016

@nickzman

This comment has been minimized.

Show comment
Hide comment
@nickzman

nickzman Nov 8, 2016

Collaborator

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

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@schweikert

schweikert Nov 8, 2016

Contributor

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

Contributor

schweikert commented Nov 8, 2016

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

@nickzman

This comment has been minimized.

Show comment
Hide comment
@nickzman

nickzman Nov 9, 2016

Collaborator

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?

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@schweikert

schweikert Nov 9, 2016

Contributor

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

Contributor

schweikert commented Nov 9, 2016

@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 from fix SSL client certificate not found on MacOS Sierra to fix SSL client certificate not found on macOS Sierra Nov 9, 2016

Show outdated Hide outdated lib/vtls/darwinssl.c
&kCFCopyStringDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks);
CFRelease(values[3]);

This comment has been minimized.

@jay

jay Nov 9, 2016

Member

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

@jay

jay Nov 9, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@nickzman

nickzman Nov 11, 2016

Collaborator

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.

Collaborator

nickzman commented Nov 11, 2016

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

This comment has been minimized.

Show comment
Hide comment
@schweikert

schweikert Nov 11, 2016

Contributor

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

Contributor

schweikert commented Nov 11, 2016

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

@nickzman

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 15, 2016

Member

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

Member

bagder commented Nov 15, 2016

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

@mschel

This comment has been minimized.

Show comment
Hide comment
@mschel

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

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

This comment has been minimized.

Show comment
Hide comment
@schweikert

schweikert Dec 6, 2016

Contributor

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

Contributor

schweikert commented Dec 6, 2016

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

@mschel

This comment has been minimized.

Show comment
Hide comment
@mschel

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

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

This comment has been minimized.

Show comment
Hide comment
@schweikert

schweikert Dec 6, 2016

Contributor

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

Contributor

schweikert commented Dec 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mschel

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

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

This comment has been minimized.

Show comment
Hide comment
@chris-araman

chris-araman Dec 22, 2016

Contributor

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

Contributor

chris-araman commented Dec 22, 2016

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

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 22, 2016

Member

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

Member

bagder commented Dec 22, 2016

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

@chris-araman

This comment has been minimized.

Show comment
Hide comment
@chris-araman

chris-araman Dec 22, 2016

Contributor

Will do.

Contributor

chris-araman commented Dec 22, 2016

Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment