[Valet 4.0] Remove .always accessibility specifier #197

dfed · 2019-12-24T03:26:34Z

This PR removes the .always and .alwaysThisDeviceOnly accessibility specifiers. These specifiers were deprecated in iOS 12, tvOS 12, macOS 10.14, and watchOS 5. They were deprecated because they do not provide any security protection for the user.

The downside of removing these specifiers is that we were using .alwaysThisDeviceOnly to reliably determine the Bundle Seed ID (aka. App ID or Team ID). In this PR, I've removed the code that would return INVALID_SHARED_ACCESS_GROUP_PREFIX when determining the Bundle Seed ID. Instead, we return nil, which makes the keychainQuery nullable. If we can't determine the Bundle Seed ID, this means we can't access the keychain. This propagates to Valet (and friend) instances as a failure to access the keychain, since these objects need to unwrap their keychainQuery before interacting with the keychain. If we don't yet have a keychainQuery, we attempt to determine one before interacting with the keychain.

Note that as part of this PR I removed the KeychainQueryConvertible protocol, since that protocol relied on a non-null keychainQuery. It also wasn't providing us much value, as we only supported migrating values from a Valet (not a SecureEnclaveValet or SinglePromptSecureEnclaveValet).

NickEntin · 2019-12-29T20:12:30Z

Sources/Valet/Internal/SecItem.swift

@@ -105,7 +104,7 @@ internal final class SecItem {
            // We should always be able to access the shared access group prefix because the accessibility of the above keychain data is set to `always`.


Need to update this comment

NickEntin · 2019-12-29T20:13:07Z

Sources/Valet/Internal/SecItem.swift

@@ -90,11 +90,10 @@ internal final class SecItem {
        }

        guard status == errSecSuccess, let queryResult = result as? [CFString : AnyHashable], let accessGroup = queryResult[kSecAttrAccessGroup] as? String else {
-            ErrorHandler.assertionFailure("Could not find shared access group prefix.")
-            // We should always be able to access the shared access group prefix because the accessibility of the above keychain data is set to `always`.
+            // We may not be able to access the shared access group prefix because the accessibility of the above keychain data is set to `afterFirstUnlock`.
            // In other words, we should never hit this code. This code is here as a failsafe to prevent a crash in a scenario where the keychain is entirely hosed.


This isn't entirely true any more

NickEntin · 2019-12-29T20:19:10Z

Sources/Valet/SecureEnclaveValet.swift

@@ -125,6 +135,9 @@ public final class SecureEnclaveValet: NSObject {
    /// - returns: The data currently stored in the keychain for the provided key. Returns `.itemNotFound` if no object exists in the keychain for the specified key, or if the keychain is inaccessible. Returns `.userCancelled` if the user cancels the user-presence prompt.
    public func object(forKey key: String, withPrompt userPrompt: String) -> SecureEnclave.Result<Data> {
        return execute(in: lock) {
+            guard let keychainQuery = keychainQuery else {
+                return .itemNotFound


Should we add a new result case for .keychainNotAccessible? (applies throughout diff)

Per the comment above:

Returns .itemNotFound if no object exists in the keychain for the specified key, or if the keychain is inaccessible.

So we're doing the right thing per the comment. Adding a new case here would be a breaking change. The Valet 4.0 code will throw the right error (couldNotAccessKeychain ) in this case assuming #198 lands

NickEntin · 2019-12-29T20:21:07Z

Sources/Valet/SecureEnclaveValet.swift

@@ -135,6 +148,9 @@ public final class SecureEnclaveValet: NSObject {
    @objc(containsObjectForKey:)
    public func containsObject(forKey key: String) -> Bool {


We should note in the header doc that it will return false if the keychain isn't accessible.

NickEntin · 2019-12-29T20:27:49Z

Sources/Valet/SinglePromptSecureEnclaveValet.swift

@@ -138,6 +151,9 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
    @objc(containsObjectForKey:)
    public func containsObject(forKey key: String) -> Bool {


Might be worth noting in the header doc that it will return false if the keychain is not accessible.

NickEntin · 2019-12-29T20:29:05Z

Sources/Valet/SinglePromptSecureEnclaveValet.swift

@@ -176,6 +198,9 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
    @objc(allKeysWithUserPrompt:)
    public func allKeys(userPrompt: String) -> Set<String> {


Might be worth noting the that an empty set will be returned when the keychain is not accessible.

NickEntin · 2019-12-29T20:32:42Z

Sources/Valet/Valet.swift

@@ -168,6 +179,9 @@ public final class Valet: NSObject, KeychainQueryConvertible {
    @objc(containsObjectForKey:)


Might be worth noting in the header doc that it returns false when keychain is not accessible.

NickEntin · 2019-12-29T20:33:14Z

Sources/Valet/Valet.swift

@@ -196,6 +216,9 @@ public final class Valet: NSObject, KeychainQueryConvertible {
    @objc
    public func allKeys() -> Set<String> {


Same thing about header doc

NickEntin · 2019-12-29T20:50:08Z

Looks good overall, most comments are about documenting behavior.

One thing to consider here is how consumers that were previously using .always/.alwaysThisDeviceOnly can migrate over to a more secure specifier. We expose the ability to migrate from a raw query or from a Valet, but if the Valet previously used one of those specifiers you won't be able to migrate for it, and consumers shouldn't need to know what the base query from that Valet looked like.

dfed · 2019-12-29T21:07:35Z

One thing to consider here is how consumers that were previously using .always/.alwaysThisDeviceOnly can migrate over to a more secure specifier. We expose the ability to migrate from a raw query or from a Valet, but if the Valet previously used one of those specifiers you won't be able to migrate for it, and consumers shouldn't need to know what the base query from that Valet looked like.

This is a good point. I've added helper methods in ce5c0e5.

dfed · 2019-12-30T03:32:26Z

Merging so I can deal with conflicts. Happy to take post-merge feedback on commits made after review.

NickEntin · 2019-12-30T07:07:13Z

Tests/ValetIntegrationTests/ValetIntegrationTests 2.swift

@@ -0,0 +1,738 @@
+//


Did you accidentally duplicate this file? Not sure how this compiled since there's now 2 ValetIntegrationTests classes...

Indeed 🤦‍♂️. I’ve since removed the duplicate from the develop branch.

dfed requested review from gfontenot, NickEntin and EricMuller22 December 24, 2019 03:26

dfed mentioned this pull request Dec 24, 2019

[Public Beta] Valet 4 wishlist #179

Closed

10 tasks

dfed changed the base branch from dfed--simplify-ci-v4 to develop--4.0 December 24, 2019 03:30

dfed force-pushed the dfed--remove-always-accessibility-specifier branch from bf6808c to a9bc846 Compare December 24, 2019 03:30

dfed added 2 commits December 24, 2019 14:06

Remove Always accessibility specifier

149c8f4

@testable import Valet

a72acd2

dfed force-pushed the dfed--remove-always-accessibility-specifier branch from a9bc846 to a72acd2 Compare December 24, 2019 22:07

dfed and others added 3 commits December 24, 2019 14:07

Fix test

46d4b20

Formatting fixes

8d84d2b

Ensure test environment is signed before testing shared access keychain

2d5b829

dfed mentioned this pull request Dec 25, 2019

[Valet 4.0] Convert APIs to throw rather than return booleans or enums #198

Merged

NickEntin approved these changes Dec 29, 2019

View reviewed changes

dfed added 3 commits December 29, 2019 12:51

Update comments

06ae396

Better comment

9e6ed00

Add migration helper methods

ce5c0e5

dfed merged commit 40c014d into develop--4.0 Dec 30, 2019

dfed deleted the dfed--remove-always-accessibility-specifier branch December 30, 2019 03:34

NickEntin reviewed Dec 30, 2019

View reviewed changes

dfed mentioned this pull request Feb 25, 2020

[Valet 4.0] Require that App ID Prefix be explicitly passed into Shared Access Group Valets #218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Valet 4.0] Remove .always accessibility specifier #197

[Valet 4.0] Remove .always accessibility specifier #197

dfed commented Dec 24, 2019 •

edited

Loading

NickEntin Dec 29, 2019

NickEntin Dec 29, 2019

NickEntin Dec 29, 2019

dfed Dec 29, 2019

NickEntin Dec 29, 2019

NickEntin Dec 29, 2019

NickEntin Dec 29, 2019

NickEntin Dec 29, 2019

NickEntin Dec 29, 2019

NickEntin commented Dec 29, 2019

dfed commented Dec 29, 2019

dfed commented Dec 30, 2019

NickEntin Dec 30, 2019

dfed Dec 30, 2019 •

edited

Loading

		@@ -105,7 +104,7 @@ internal final class SecItem {
		// We should always be able to access the shared access group prefix because the accessibility of the above keychain data is set to `always`.

		@@ -135,6 +148,9 @@ public final class SecureEnclaveValet: NSObject {
		@objc(containsObjectForKey:)
		public func containsObject(forKey key: String) -> Bool {

		@@ -138,6 +151,9 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
		@objc(containsObjectForKey:)
		public func containsObject(forKey key: String) -> Bool {

		@@ -176,6 +198,9 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
		@objc(allKeysWithUserPrompt:)
		public func allKeys(userPrompt: String) -> Set<String> {

		@@ -168,6 +179,9 @@ public final class Valet: NSObject, KeychainQueryConvertible {
		@objc(containsObjectForKey:)

		@@ -196,6 +216,9 @@ public final class Valet: NSObject, KeychainQueryConvertible {
		@objc
		public func allKeys() -> Set<String> {

[Valet 4.0] Remove .always accessibility specifier #197

[Valet 4.0] Remove .always accessibility specifier #197

Conversation

dfed commented Dec 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickEntin commented Dec 29, 2019

dfed commented Dec 29, 2019

dfed commented Dec 30, 2019

Choose a reason for hiding this comment

dfed Dec 30, 2019 • edited Loading

Choose a reason for hiding this comment

dfed commented Dec 24, 2019 •

edited

Loading

dfed Dec 30, 2019 •

edited

Loading