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

Apache/macOS Private Keychain Breaks Time Capsule Backups #1922

Closed
jriesen opened this issue Feb 12, 2017 · 8 comments
Closed

Apache/macOS Private Keychain Breaks Time Capsule Backups #1922

jriesen opened this issue Feb 12, 2017 · 8 comments
Milestone

Comments

@jriesen
Copy link

jriesen commented Feb 12, 2017

Issue report

Question 1: What is the problem?

A few weeks ago, my Mac stopped backing up to the external USB HDD I have attacked to my Apple Time Capsule. When alerted to the problem, I noticed that I could view the drive normally via Finder, but that Time Machine had 'forgotten' the login to the device. When I tried to re-add the device as my Time Machine backup, I typed in the device password for the Time Capsule and received the following error:

Keychain error -25307 occurred while creating a System Keychain entry for the username "Joseph Riesen" and URL "afp://Joseph%20Riesen@Joseph's%20Airport%20Extreme._afpovertcp._tcp.local./Super%20Backup%20Disk".

screen shot 2017-02-12 at 11 58 18 am

After some internet research, I discovered that this appears to be a problem with making a modification to /Library/Preferences/com.apple.security.plist, as the latest version of Passenger now does to make a private keychain (as described at https://blog.phusion.nl/2017/01/24/passenger-5-1-2/). Apparently this exact same problem was caused back in 2013 by a program called iStat Server when they updated com.apple.security.plist to add a custom keychain -- you can see a discussion thread at https://arstechnica.com/civis/viewtopic.php?t=1225417

According to that thread, the iStat Server team apparently fixed this problem with version 2.21. (Mind you, this was back in 2013.) I'm not sure what change they made in their software to resolve the problem.

If I remove the DLDBSearchList and DefaultKeychain sections entirely (leaving SecItemSynchronizable as-is), the problem is resolved. If either the DLDBSearchList or DefaultKeychain sections are present, the problem reoccurs. (And yes, I'm deleting both the key and value array elements.) As such, if I revert my file back to its original form, everything works correctly again:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
	<key>SecItemSynchronizable</key>
	<true/>
</dict>
</plist>

Naturally, I expect this will break the custom keychain functionality in Passenger, so I consider this a temporary workaround. Also, the lines are automatically re-added to the file whenever I reboot Apache, causing my next scheduled Time Machine backup to fail.

Question 2: Passenger version and integration mode:

Phusion Passenger 5.1.2 via Homebrew, installed in Apache/2.4.23 (the current built-in version from Apple)

Question 3: OS or Linux distro, platform (including version):

macOS Sierra 10.12.3

Question 4: Passenger installation method:

OS X Homebrew

Question 5: Your app's programming language (including any version managers) and framework (including versions):

I run a Rails development website on this system. Looks like it's Rails 4.2.7.1 on Ruby 2.3.1p112, but I don't believe this is relevant to the problem.

Question 6: Are you using a PaaS and/or containerization? If so which one?

No.

Question 7: Anything else about your setup that we should know?

N/A

Thank you for your help!

@CamJN
Copy link
Contributor

CamJN commented Feb 13, 2017

Thanks for reporting this and providing a very helpful writeup. I've reproduced the issue locally and am digging into this.

@ravenchorus
Copy link

Came here to post about this very issue but couldn't state it any better than this.

I'm running the same versions of Mac OS, with Ruby 2.3.3 installed via Homebrew. I'm seeing the same problem with my Time Machine backups (only in my case backing up to OS X Server via SMB) and a possibly related VPN connection issue.

@CamJN
Copy link
Contributor

CamJN commented Feb 15, 2017

Yes, until I can fix this in Passenger here's a workaround:

sudo apachectl stop
sudo defaults delete /Library/Preferences/com.apple.security.plist DLDBSearchList
sudo defaults delete /Library/Preferences/com.apple.security.plist DefaultKeychain

Then edit your apache config to include PassengerDisableSecurityUpdateCheck on
Then you can start Apache again and not experience this issue. I'm working on fixing this properly, but so far don't have anything reliable to share.

@ravenchorus
Copy link

Perfect, thank you!

@vfragosop
Copy link

I don't know if this information is helpful, but I accidentally deleted my passenger keychain during a clean up. After that, my whole keychain stopped working and couldn't store any new password. The error message I was getting was: Keychain "passenger" cannot be found to store "xxxxxxxxx".

Creating a passenger keychain didn't solve the issue. Resetting all my keychains didn't help either and the only solution was to delete those com.apple.security.plist entries as @CamJN replied above.

In my opinion, passenger shouldn't be messing with DLDBSearchList and DefaultKeychain.

CamJN added a commit that referenced this issue Mar 8, 2017
…s root on macOS, in order to avoid changing the default System Keychain. Closes GH-1922. Remove Cert and Key from keychain separately, to avoid errors when clearing the client certificate.

libcurl Keychain Problem, Solution, and Code

Overview of the general Problem:

The original problem is that on macOS libcurl doesn’t load the client certificate in such a way that it has permission to use it without asking the user first. This causes a popup which we want to prevent.

Overview of the general Solution:

I work around the problem by loading the client certificate into the keychain with the correct access permissions before we call curl, and then removing it afterwards.

History of approaches taken:
Initially I loaded the cert and removed the certificate each using a single call to the keychain services api.
Then I found that when passenger was run as root like when run under the system apache, that we got errors when I tried to remove the certificate/key pair that led me to believe there was a permissions issue.
In response to that I created a private keychain and added the certificate to it when I detected that the system keychain was the default keychain for the user Passenger is running as. I then set the private keychain as the default keychain for the user (curl only uses the default keychain, it doesn't allow you to set a specific keychain to use). I then reset the keychain to the old default on passenger shutdown.
However it turns out that even if you reset the default keychain back to what it was, macOS does not fully revert the changes to /Library/Preferences/com.apple.security.plist which in turn breaks things like time machine and automatically joining wifi networks.
So I ditched the private keychain approach and went back and researched the error I got when I removed the certificate/key pair from the keychain, and found that if I keep a copy of the random label given to the private key when it is added to the keychain, that I can remove the key and certificate separately and not get the same error that I was getting.

Code:
The code for this resides entirely in SecurityUpdateChecker.h, Crypto.h, and Crypto.cpp

The relevant code in SecurityUpdateChecker.h is the call to `crypto->preAuthKey(clientCertPath.c_str(), CLIENT_CERT_PWD, CLIENT_CERT_LABEL)` in `prepareCurlPOST()` and the call to `crypto->killKey(CLIENT_CERT_LABEL);` in `checkAndLogSecurityUpdate()`. These functions are responsible for adding the certificate/key pair to the keychain and then removing it again after lib curl is done.

While I was using the private keychain approach, there was a bit more code to create the private keychain, and keep track of the old default keychain and whether or not the private keychain was in use, but it has been removed now that this approach was been abandoned.

The Crypto.h header code is just there to make things compile.

Crypto.cpp is where all the interesting code is:

The constructor initializes the variable that holds the private key's random label: id to NULL.

`preAuthKey` checks if the certificate/key pair is already in the keychain using `lookupKeychainItem`, and if so issues an error and returns an unsuccessful status because we prefer writing a warning and skipping the update check to causing a keychain popup. If the cert/key pair is not found then we disable popups (disabling popups and doing nothing more causes libcurl to crash, so it's not a valid approach to the the general problem), load the certificate by calling `copyIdentityFromPKCS12File`, then re-enable popups (because according to the documentation this is a system wide flag and we don't want to break other software, especially since  we know anything that uses libcurl is liable to blow up). After each call to a keychain services api, we check for errors and log them as needed.

`lookupKeychainItem` calls `createQueryDict` to receive a query to find our certificate/key pair if it is in the keychain. Then calls `SecItemCopyMatching` which is the keychain services API for searching the keychain. It returns whether or not the certificate was found and if so sets the second argument pointer to point to it.

`createQueryDict` simply builds a dictionary that contains the key value pairs to uniquely identify our cert/key pair and specify that we want a reference to it, if found.

`copyIdentityFromPKCS12File` is where the certificate is actually loaded. We begin by reading in the p12 file's data, and creating variables of the correct types for use with the keychain services API. Then if the file is read in correctly we call `createAccess` which returns an access struct that allows us to use the certificate after it's loaded (this is what's missing from libcurl) then after packing the access struct and password into a dictionary we pass that and the p12 file's data to `SecPKCS12Import` which does the actual loading of the certificate/key pair. Then if that succeeds we record the random label that the keychain assigned to the private key in the `id` variable and cleanup.

`createAccess` simply wraps the call to `SecAccessCreate` to move some type wrangling out of an already huge function, `SecAccessCreate ` creates the actual access struct.

`killKey` checks if the cert/key pair is found using `lookupKeychainItem`, and if so builds a dictionary to target the certificate then calls the SecItemDelete api to delete the cert, and then if a key label is present, does the same for the key too and clears the `id` variable.
@kcrawford
Copy link

This also affects other system level functionality including connections to Active Directory and 802.1X networks. We deploy passenger with puppet so now after each puppet run we have to fix the system's com.apple.security.plist.

@mhw
Copy link

mhw commented Mar 21, 2017

I ran into this with Passenger running under Nginx, both installed from Homebrew and with Nginx running as a system service (i.e. started as root on system startup). The workaround is the same as in #1922 (comment), except you need to add passenger_disable_security_update_check on; to /usr/local/etc/nginx/nginx.conf.

Was really confused about what had caused TimeMachine to stop working until I saw the blog post about the Passenger issue.

For me the keychain error was slightly different too: "Keychain error -25308 occurred while creating a System Keychain entry..." I'm not convinced the -25308 is meaningful though now.

@timraasveld
Copy link

timraasveld commented Apr 30, 2017

Copy-paste-to-your-terminal fix for Nginx:

sudo defaults delete /Library/Preferences/com.apple.security.plist DLDBSearchList
sudo defaults delete /Library/Preferences/com.apple.security.plist DefaultKeychain
sed -i '' $'s/passenger_root/passenger_disable_security_update_check on;\\\n    passenger_root/' /usr/local/etc/nginx/nginx.conf

based on @CamJN and @mhw s info

@OnixGH OnixGH added this to the 5.1.3 milestone Jul 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants