Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Revert private keychain use in the Security Update Checker when run a…
…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.
- Loading branch information
