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

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

Merged
merged 6 commits into from
Apr 12, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Require that App ID Prefix be explicitly passed into Shared Access Gr…
…oup Valets
  • Loading branch information
dfed committed Feb 25, 2020
commit b15410c7ebf7221616243121156685afa2dab959
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,14 @@ In addition to allowing the storage of strings, Valet allows the storage of `Dat
### Sharing Secrets Among Multiple Applications

```swift
let mySharedValet = Valet.sharedAccessGroupValet(with: Identifier(nonEmpty: "Druidia")!, accessibility: .whenUnlocked)
let mySharedValet = Valet.sharedAccessGroupValet(with: Identifier(nonEmpty: "AppID12345.Druidia")!, accessibility: .whenUnlocked)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be cleaner to create a distinct SharedAccessGroupIdentifier that takes two separate strings (the shared access group id and the Valet id), rather than using Identifier here. That way it is more obvious what this identifier is, and consumers don't have to know that it needs to be formatted as two strings concatenated with a period.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot. Made code changes in 5078ffc. Changes to documentation incoming.

```

```objc
VALValet *const mySharedValet = [VALValet valetWithSharedAccessGroupIdentifier:@"Druidia" accessibility:VALAccessibilityWhenUnlocked];
VALValet *const mySharedValet = [VALValet valetWithSharedAccessGroupIdentifier:@"AppID12345.Druidia" accessibility:VALAccessibilityWhenUnlocked];
```

This instance can be used to store and retrieve data securely across any app written by the same developer with the value `Druidia` under the `keychain-access-groups` key in the app’s `Entitlements` file, when the device is unlocked. Note that `myValet` and `mySharedValet` can not read or modify one another’s values because the two Valets were created with different initializers. All Valet types can share secrets across applications written by the same developer by using the `sharedAccessGroupValet` initializer.
This instance can be used to store and retrieve data securely across any app written by the same developer that has `AppID12345.Druidia` (or `$(AppIdentifierPrefix)Druidia`) set as a value for the `keychain-access-groups` key in the app’s `Entitlements`, where `AppID12345` is the application’s [App ID prefix](https://developer.apple.com/documentation/security/keychain_services/keychain_items/sharing_access_to_keychain_items_among_a_collection_of_apps#2974920). This Valet is accessible when the device is unlocked. Note that `myValet` and `mySharedValet` can not read or modify one another’s values because the two Valets were created with different initializers. All Valet types can share secrets across applications written by the same developer by using the `sharedAccessGroupValet` initializer.

### Sharing Secrets Across Devices with iCloud

Expand Down
45 changes: 0 additions & 45 deletions Sources/Valet/Internal/SecItem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,52 +31,7 @@ internal func execute<ReturnType>(in lock: NSLock, block: () throws -> ReturnTyp


internal final class SecItem {

// MARK: Internal Class Properties

/// Programatically grab the required prefix for the shared access group (i.e. Bundle Seed ID). The value for the kSecAttrAccessGroup key in queries for data that is shared between apps must be of the format bundleSeedID.sharedAccessGroup. For more information on the Bundle Seed ID, see https://developer.apple.com/library/ios/qa/qa1713/_index.html
internal static var sharedAccessGroupPrefix: String? {
var query: [CFString : Any] = [
kSecClass : kSecClassGenericPassword,
kSecAttrAccount : "SharedAccessGroupPrefixPlaceholder",
kSecReturnAttributes : true,
kSecAttrAccessible : Accessibility.afterFirstUnlockThisDeviceOnly.secAccessibilityAttribute
]

if #available(iOS 13.0, tvOS 13.0, watchOS 6.0, macOS 10.15, *) {
// Add kSecUseDataProtectionKeychain to the query to ensure we can retrieve the shared access group prefix.
query[kSecUseDataProtectionKeychain] = true
}

secItemLock.lock()
defer {
secItemLock.unlock()
}

var result: AnyObject? = nil
var status = SecItemCopyMatching(query as CFDictionary, &result)

if status == errSecItemNotFound {
status = SecItemAdd(query as CFDictionary, &result)
}

guard status == errSecSuccess, let queryResult = result as? [CFString : AnyHashable], let accessGroup = queryResult[kSecAttrAccessGroup] as? String else {
// We may not be able to access the shared access group prefix because the accessibility of the above keychain data is set to `afterFirstUnlock`.
// Consumers should always check `canAccessKeychain()` after creating a Valet and before using it. Doing so will catch this error.
return nil
}

let components = accessGroup.components(separatedBy: ".")
if let bundleSeedIdentifier = components.first, !bundleSeedIdentifier.isEmpty {
return bundleSeedIdentifier

} else {
// We may not be able to access the shared access group prefix because the accessibility of the above keychain data is set to `afterFirstUnlock`.
// Consumers should always check `canAccessKeychain()` after creating a Valet and before using it. Doing so will catch this error.
return nil
}
}

// MARK: Internal Class Methods

internal static func copy<DesiredType>(matching query: [String : AnyHashable]) throws -> DesiredType {
Expand Down
22 changes: 3 additions & 19 deletions Sources/Valet/Internal/Service.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ internal enum Service: CustomStringConvertible, Equatable {

// MARK: Internal Methods

internal func generateBaseQuery() throws -> [String : AnyHashable] {
internal func generateBaseQuery() -> [String : AnyHashable] {
var baseQuery: [String : AnyHashable] = [
kSecClass as String : kSecClassGenericPassword as String,
kSecAttrService as String : secService,
Expand All @@ -70,31 +70,15 @@ internal enum Service: CustomStringConvertible, Equatable {
configuration = desiredConfiguration

case let .sharedAccessGroup(identifier, desiredConfiguration):
guard let sharedAccessGroupPrefix = SecItem.sharedAccessGroupPrefix else {
throw KeychainError.couldNotAccessKeychain
}
if identifier.description.hasPrefix("\(sharedAccessGroupPrefix).") {
// The Bundle Seed ID was passed in as a prefix to the identifier.
baseQuery[kSecAttrAccessGroup as String] = identifier.description
} else {
baseQuery[kSecAttrAccessGroup as String] = "\(sharedAccessGroupPrefix).\(identifier.description)"
}
baseQuery[kSecAttrAccessGroup as String] = identifier.description
configuration = desiredConfiguration

#if os(macOS)
case let .standardOverride(_, desiredConfiguration):
configuration = desiredConfiguration

case let .sharedAccessGroupOverride(identifier, desiredConfiguration):
guard let sharedAccessGroupPrefix = SecItem.sharedAccessGroupPrefix else {
throw KeychainError.couldNotAccessKeychain
}
if identifier.description.hasPrefix("\(sharedAccessGroupPrefix).") {
// The Bundle Seed ID was passed in as a prefix to the identifier.
baseQuery[kSecAttrAccessGroup as String] = identifier.description
} else {
baseQuery[kSecAttrAccessGroup as String] = "\(sharedAccessGroupPrefix).\(identifier.description)"
}
baseQuery[kSecAttrAccessGroup as String] = identifier.description
configuration = desiredConfiguration
#endif
}
Expand Down
33 changes: 11 additions & 22 deletions Sources/Valet/SecureEnclaveValet.swift
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public final class SecureEnclaveValet: NSObject {
self.identifier = identifier
self.service = service
self.accessControl = accessControl
_keychainQuery = try? service.generateBaseQuery()
baseKeychainQuery = service.generateBaseQuery()
}

// MARK: Hashable
Expand Down Expand Up @@ -126,7 +126,7 @@ public final class SecureEnclaveValet: NSObject {
@objc
public func setObject(_ object: Data, forKey key: String) throws {
try execute(in: lock) {
try SecureEnclave.setObject(object, forKey: key, options: try keychainQuery())
try SecureEnclave.setObject(object, forKey: key, options: baseKeychainQuery)
}
}

Expand All @@ -138,7 +138,7 @@ public final class SecureEnclaveValet: NSObject {
@objc
public func object(forKey key: String, withPrompt userPrompt: String) throws -> Data {
try execute(in: lock) {
try SecureEnclave.object(forKey: key, withPrompt: userPrompt, options: try keychainQuery())
try SecureEnclave.object(forKey: key, withPrompt: userPrompt, options: baseKeychainQuery)
}
}

Expand All @@ -148,7 +148,7 @@ public final class SecureEnclaveValet: NSObject {
/// - Note: Will never prompt the user for Face ID, Touch ID, or password.
public func containsObject(forKey key: String) throws -> Bool {
try execute(in: lock) {
try SecureEnclave.containsObject(forKey: key, options: try keychainQuery())
try SecureEnclave.containsObject(forKey: key, options: baseKeychainQuery)
}
}

Expand All @@ -159,7 +159,7 @@ public final class SecureEnclaveValet: NSObject {
@objc
public func setString(_ string: String, forKey key: String) throws {
try execute(in: lock) {
try SecureEnclave.setString(string, forKey: key, options: try keychainQuery())
try SecureEnclave.setString(string, forKey: key, options: baseKeychainQuery)
}
}

Expand All @@ -171,7 +171,7 @@ public final class SecureEnclaveValet: NSObject {
@objc
public func string(forKey key: String, withPrompt userPrompt: String) throws -> String {
try execute(in: lock) {
try SecureEnclave.string(forKey: key, withPrompt: userPrompt, options: try keychainQuery())
try SecureEnclave.string(forKey: key, withPrompt: userPrompt, options: baseKeychainQuery)
}
}

Expand All @@ -181,7 +181,7 @@ public final class SecureEnclaveValet: NSObject {
@objc
public func removeObject(forKey key: String) throws {
try execute(in: lock) {
try Keychain.removeObject(forKey: key, options: try keychainQuery())
try Keychain.removeObject(forKey: key, options: baseKeychainQuery)
}
}

Expand All @@ -190,7 +190,7 @@ public final class SecureEnclaveValet: NSObject {
@objc
public func removeAllObjects() throws {
try execute(in: lock) {
try Keychain.removeAllObjects(matching: try keychainQuery())
try Keychain.removeAllObjects(matching: baseKeychainQuery)
}
}

Expand All @@ -203,7 +203,7 @@ public final class SecureEnclaveValet: NSObject {
@objc
public func migrateObjects(matching query: [String : AnyHashable], removeOnCompletion: Bool) throws {
try execute(in: lock) {
try Keychain.migrateObjects(matching: query, into: try keychainQuery(), removeOnCompletion: removeOnCompletion)
try Keychain.migrateObjects(matching: query, into: baseKeychainQuery, removeOnCompletion: removeOnCompletion)
}
}

Expand All @@ -215,7 +215,7 @@ public final class SecureEnclaveValet: NSObject {
/// - Note: The keychain is not modified if an error is thrown.
@objc
public func migrateObjects(from valet: Valet, removeOnCompletion: Bool) throws {
try migrateObjects(matching: try valet.keychainQuery(), removeOnCompletion: removeOnCompletion)
try migrateObjects(matching: valet.baseKeychainQuery, removeOnCompletion: removeOnCompletion)
}

// MARK: Internal Properties
Expand All @@ -225,19 +225,8 @@ public final class SecureEnclaveValet: NSObject {
// MARK: Private Properties

private let lock = NSLock()
private var _keychainQuery: [String : AnyHashable]?
private let baseKeychainQuery: [String : AnyHashable]

// MARK: Private Methods

private func keychainQuery() throws -> [String : AnyHashable] {
if let keychainQuery = _keychainQuery {
return keychainQuery
} else {
let keychainQuery = try service.generateBaseQuery()
_keychainQuery = keychainQuery
return keychainQuery
}
}
}


Expand Down
34 changes: 11 additions & 23 deletions Sources/Valet/SinglePromptSecureEnclaveValet.swift
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
self.identifier = identifier
self.service = service
self.accessControl = accessControl
_baseKeychainQuery = try? service.generateBaseQuery()
baseKeychainQuery = service.generateBaseQuery()
}

// MARK: Hashable
Expand Down Expand Up @@ -130,7 +130,7 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
@objc
public func setObject(_ object: Data, forKey key: String) throws {
try execute(in: lock) {
try SecureEnclave.setObject(object, forKey: key, options: try baseKeychainQuery())
try SecureEnclave.setObject(object, forKey: key, options: baseKeychainQuery)
}
}

Expand All @@ -152,7 +152,7 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
/// - Note: Will never prompt the user for Face ID, Touch ID, or password.
public func containsObject(forKey key: String) throws -> Bool {
try execute(in: lock) {
try SecureEnclave.containsObject(forKey: key, options: try self.baseKeychainQuery())
try SecureEnclave.containsObject(forKey: key, options: baseKeychainQuery)
}
}

Expand All @@ -163,7 +163,7 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
@objc
public func setString(_ string: String, forKey key: String) throws {
try execute(in: lock) {
try SecureEnclave.setString(string, forKey: key, options: try baseKeychainQuery())
try SecureEnclave.setString(string, forKey: key, options: baseKeychainQuery)
}
}

Expand Down Expand Up @@ -209,7 +209,7 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
@objc
public func removeObject(forKey key: String) throws {
try execute(in: lock) {
try Keychain.removeObject(forKey: key, options: try baseKeychainQuery())
try Keychain.removeObject(forKey: key, options: baseKeychainQuery)
}
}

Expand All @@ -218,7 +218,7 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
@objc
public func removeAllObjects() throws {
try execute(in: lock) {
try Keychain.removeAllObjects(matching: try baseKeychainQuery())
try Keychain.removeAllObjects(matching: baseKeychainQuery)
}
}

Expand All @@ -231,7 +231,7 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
@objc
public func migrateObjects(matching query: [String : AnyHashable], removeOnCompletion: Bool) throws {
try execute(in: lock) {
try Keychain.migrateObjects(matching: query, into: try baseKeychainQuery(), removeOnCompletion: removeOnCompletion)
try Keychain.migrateObjects(matching: query, into: baseKeychainQuery, removeOnCompletion: removeOnCompletion)
}
}

Expand All @@ -243,7 +243,7 @@ public final class SinglePromptSecureEnclaveValet: NSObject {
/// - Note: The keychain is not modified if an error is thrown.
@objc
public func migrateObjects(from valet: Valet, removeOnCompletion: Bool) throws {
try migrateObjects(matching: try valet.keychainQuery(), removeOnCompletion: removeOnCompletion)
try migrateObjects(matching: valet.baseKeychainQuery, removeOnCompletion: removeOnCompletion)
}

// MARK: Internal Properties
Expand All @@ -254,26 +254,14 @@ public final class SinglePromptSecureEnclaveValet: NSObject {

private let lock = NSLock()
private var localAuthenticationContext = LAContext()
private var _baseKeychainQuery: [String : AnyHashable]?

// MARK: Private Methods

private func baseKeychainQuery() throws -> [String : AnyHashable] {
if let baseKeychainQuery = _baseKeychainQuery {
return baseKeychainQuery
} else {
let baseKeychainQuery = try service.generateBaseQuery()
_baseKeychainQuery = baseKeychainQuery
return baseKeychainQuery
}
}

private let baseKeychainQuery: [String : AnyHashable]

/// A keychain query dictionary that allows for continued read access to the Secure Enclave after the a single unlock event.
/// This query should be used when retrieving keychain data, but should not be used for keychain writes or `containsObject` checks.
/// Using this query in a `containsObject` check can cause a false positive in the case where an element has been removed from
/// the keychain by the operating system due to a face, fingerprint, or password change.
private func continuedAuthenticationKeychainQuery() throws -> [String : AnyHashable] {
var keychainQuery = try baseKeychainQuery()
var keychainQuery = baseKeychainQuery
keychainQuery[kSecUseAuthenticationContext as String] = localAuthenticationContext
return keychainQuery
}
Expand Down
Loading