Remove KeychainAccess package dependency

[mgcm]

WIP for #49

[scoates]

Genuinely, thank you for working on this.

Honest question: is it worth using a library like Valet here?

The reason I ask is: the native SecItem/kSec API in Foundation is ugly, and I admit I might not be doing it in the best way, but I have a "KeychainHelper" class in a client project that's less than 100 lines long… seems our needs here are also pretty light.

Also admittedly, I have an aversion to unnecessary dependencies, so there's some bias on my side.

Additionally, using upToNextMajorVersion (while much better than pointing at HEAD of a branch, and I know is considered common and good practice) is a much bigger opening to a supply chain attack than pinning a specific commit hash—it's easy for an account compromise to tag a malicious release version, and much harder to fake a specific hash. (Pinning a specific hash requires ongoing maintenance to get updates from upstream, though.) Here's an example of this kind of exploit from a different but similarly-managed (through version numbers vs. hash pinning) system—we'd at least have a chance to catch an exploit before rolling a version in Mactrix, a realtime exploit with GH Actions.

I write this with no illusion of authority here. Just want to have these conversations. (Sincerely hope that's okay; happy to move the conversation elsewhere, too.)

[mgcm]

@scoates fully agree on everything you said - I had this PR as a draft and used Valet just to confirm that using the Data Protection variant of the SecItem API got rid of those nasty keychain password dialogs.

As discussed in the corresponding ticket (#49) with @viktorstrate , going this route (with or without Valet) has implications on distribution and we're still figuring out the best way to move forward on that.

@viktorstrate not sure why you removed this from draft but I would refrain from merging this for now.

[viktorstrate]

As I understand it the entitlement is only required if we need keychain sharing (which we don't). The new API should be useable without this entitlement or any special developer certificate.

From the Apple docs:

Keychain items can be shared only between apps from the same developer. To share keychain items, third-party apps use access groups with a prefix allocated to them through the Apple Developer Program in their application groups. The prefix requirement and application group uniqueness are enforced through code signing, provisioning profiles, and the Apple Developer Program.

I agree that we should not depend on an external library for this.

[scoates]

Great! Thank you both. I'm sorry I missed the discussion. Glad we're on the same page.

[mgcm]

I've updated this MR with a minimal Keychain Wrapper that removes the third party dependencies.

To enable the Data Protection keychain mode, the kSecUseDataProtectionKeychain attribute must be set to true.

This PR keeps it false but if you toggle it, running the app without the keychain-access-groups entitlement will throw an error with the code -34018 -> Internal error when a required entitlement isn't present. see here.

As soon as the entitlement is added and the signing configured, it works.

[viktorstrate]

I have tried to replicate what you wrote @mgcm and I get the same errors. Also, I'm not even able to make it work when adding the entitlement because I don't have a paid develper account so I can not register a profile for the bundle identifier.

I suppose this makes sense (with or without the entitlement) since the modern API uses the app signature and identifier to grant access to the keychain. Without a proper developer account, there is no way to claim the bundle identifier which means that anyone could make an app that spoofed the ID to get the secrets without asking the user for permission.

Ideally we would need a paid developer account we can use for releases. Eventually I'd like to publish the app on AppStore and for that I need the paid account anyway. But for now without a paid account, I think the legacy keychain is the only/best option (right?).

My proposal is to merge this PR but with the kSecUseDataProtectionKeychain key set to false (as it is currently). That way we remove the KeychainAccess dependency, and we can easily switch to the modern keychain when I eventually get a paid developer account and we're ready to publish to the AppStore.

What do you think? If you have a better idea I'd love to hear it!