From eb6e9cb1695b7edf3647cacbe55384e3c50a1afc Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Fri, 19 Jan 2024 17:48:45 -0800 Subject: [PATCH 01/14] Use actual kSec key names in KeychainAttribute.Attribute case names This change fixes a bug where the String representation of these keys were supplied to the keychain query as opposed to the actual key names. --- GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift | 9 +++++++-- GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift | 7 ------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift b/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift index bfc1d243..81a41f31 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift @@ -24,6 +24,7 @@ public final class KeychainAttribute: NSObject { /// Indicates whether to treat macOS keychain items like iOS keychain items. /// /// This attribute will set `kSecUseDataProtectionKeychain` as true in the Keychain query. + @available(macOS 10.15, *) case useDataProtectionKeychain /// The `String` name for the access group to use in the Keychain query. case accessGroup(String) @@ -32,9 +33,13 @@ public final class KeychainAttribute: NSObject { public var keyName: String { switch self { case .useDataProtectionKeychain: - return "kSecUseDataProtectionKeychain" + if #available(macOS 10.15, *) { + return kSecUseDataProtectionKeychain as String + } else { + fatalError("`KeychainAttribute.Attribute.useDataProtectionKeychain is only available on macOS 10.15 and greater") + } case .accessGroup: - return "kSecKeychainAccessGroup" + return kSecAttrAccessGroup as String } } } diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift index 1017745e..ff9080c9 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift @@ -34,11 +34,6 @@ public protocol KeychainHelper { final class KeychainWrapper: KeychainHelper { let accountName = "OAuth" let keychainAttributes: Set - @available(macOS 10.15, *) - private var isMaxMacOSVersionGreaterThanTenOneFive: Bool { - let tenOneFive = OperatingSystemVersion(majorVersion: 10, minorVersion: 15, patchVersion: 0) - return ProcessInfo().isOperatingSystemAtLeast(tenOneFive) - } init(keychainAttributes: Set = []) { self.keychainAttributes = keychainAttributes @@ -54,11 +49,9 @@ final class KeychainWrapper: KeychainHelper { keychainAttributes.forEach { configuration in switch configuration.attribute { case .useDataProtectionKeychain: -#if os(macOS) && isMaxMacOSVersionGreaterThanTenOneFive if #available(macOS 10.15, *) { query[configuration.attribute.keyName] = kCFBooleanTrue } -#endif case .accessGroup(let name): query[configuration.attribute.keyName] = name } From 10cac9e4770fa8cf50f73e6e6fb06e1f2a771e28 Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Fri, 19 Jan 2024 18:26:36 -0800 Subject: [PATCH 02/14] Use data protection and access group attributes in macOS example --- .../Example-macOS/Example-macOS.xcodeproj/project.pbxproj | 2 +- .../Example-macOS/Source/GTMAppAuthExampleViewController.m | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Examples/Example-macOS/Example-macOS.xcodeproj/project.pbxproj b/Examples/Example-macOS/Example-macOS.xcodeproj/project.pbxproj index 32b944ef..329ffe12 100644 --- a/Examples/Example-macOS/Example-macOS.xcodeproj/project.pbxproj +++ b/Examples/Example-macOS/Example-macOS.xcodeproj/project.pbxproj @@ -3,7 +3,7 @@ archiveVersion = 1; classes = { }; - objectVersion = 52; + objectVersion = 54; objects = { /* Begin PBXBuildFile section */ diff --git a/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m b/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m index cad65ca9..5f047c17 100644 --- a/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m +++ b/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m @@ -65,7 +65,12 @@ @implementation GTMAppAuthExampleViewController - (void)viewDidLoad { [super viewDidLoad]; - self.keychainStore = [[GTMKeychainStore alloc] initWithItemName:kExampleAuthorizerKey]; + GTMKeychainAttribute *dataProtection = [GTMKeychainAttribute useDataProtectionKeychain]; + GTMKeychainAttribute *accessGroup = [GTMKeychainAttribute keychainAccessGroupWithName:@"test"]; + NSSet *attributes = [NSSet setWithArray:@[dataProtection, accessGroup]]; + self.keychainStore = [[GTMKeychainStore alloc] initWithItemName:kExampleAuthorizerKey + keychainAttributes:attributes]; +// self.keychainStore = [[GTMKeychainStore alloc] initWithItemName:kExampleAuthorizerKey]; #if !defined(NS_BLOCK_ASSERTIONS) // NOTE: From b3f59fa1ed337930a723fa602a4170949538fb15 Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Wed, 24 Jan 2024 16:40:43 -0800 Subject: [PATCH 03/14] Update KeychainAttribute.swift to check availability for iOS, tvOS, and watchOS as well --- GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift b/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift index 81a41f31..b13b5f90 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift @@ -24,7 +24,7 @@ public final class KeychainAttribute: NSObject { /// Indicates whether to treat macOS keychain items like iOS keychain items. /// /// This attribute will set `kSecUseDataProtectionKeychain` as true in the Keychain query. - @available(macOS 10.15, *) + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) case useDataProtectionKeychain /// The `String` name for the access group to use in the Keychain query. case accessGroup(String) @@ -33,7 +33,7 @@ public final class KeychainAttribute: NSObject { public var keyName: String { switch self { case .useDataProtectionKeychain: - if #available(macOS 10.15, *) { + if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { return kSecUseDataProtectionKeychain as String } else { fatalError("`KeychainAttribute.Attribute.useDataProtectionKeychain is only available on macOS 10.15 and greater") From 38279a94c83f685efa14b56954f4a0ba1e4e4fdd Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Wed, 24 Jan 2024 16:53:49 -0800 Subject: [PATCH 04/14] Remove availability checkin KeychainHelper.swift This check isn't needed since the enum case already has restricted availability. --- GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift index ff9080c9..9f970a7a 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift @@ -49,9 +49,7 @@ final class KeychainWrapper: KeychainHelper { keychainAttributes.forEach { configuration in switch configuration.attribute { case .useDataProtectionKeychain: - if #available(macOS 10.15, *) { - query[configuration.attribute.keyName] = kCFBooleanTrue - } + query[configuration.attribute.keyName] = kCFBooleanTrue case .accessGroup(let name): query[configuration.attribute.keyName] = name } From f1c5892981fdea30d7ccc3958f31b9680335c2d7 Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Wed, 24 Jan 2024 17:49:05 -0800 Subject: [PATCH 05/14] Add keychain access group attribute and entitlement --- .../Example-macOS.xcodeproj/project.pbxproj | 12 ++++++++++++ .../Source/GTMAppAuthExampleViewController.m | 10 ++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/Examples/Example-macOS/Example-macOS.xcodeproj/project.pbxproj b/Examples/Example-macOS/Example-macOS.xcodeproj/project.pbxproj index 329ffe12..f7586115 100644 --- a/Examples/Example-macOS/Example-macOS.xcodeproj/project.pbxproj +++ b/Examples/Example-macOS/Example-macOS.xcodeproj/project.pbxproj @@ -21,6 +21,7 @@ /* Begin PBXFileReference section */ 7355833228AFFC0B00AC24DC /* GTMAppAuth */ = {isa = PBXFileReference; lastKnownFileType = wrapper; name = GTMAppAuth; path = ../..; sourceTree = ""; }; + 73B03D4C2B5EF66300EEA5DC /* Example-macOS.entitlements */ = {isa = PBXFileReference; lastKnownFileType = text.plist.entitlements; path = "Example-macOS.entitlements"; sourceTree = ""; }; C1AF3AE62818785B003BAEFF /* README.md */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = net.daringfireball.markdown; path = README.md; sourceTree = ""; }; F6016E701D2AC11F003497D7 /* Example-macOS.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = "Example-macOS.app"; sourceTree = BUILT_PRODUCTS_DIR; }; F6016E8B1D2BD988003497D7 /* AppDelegate.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = AppDelegate.m; path = Source/AppDelegate.m; sourceTree = ""; }; @@ -66,6 +67,7 @@ F6016E671D2AC11E003497D7 = { isa = PBXGroup; children = ( + 73B03D4C2B5EF66300EEA5DC /* Example-macOS.entitlements */, 7355833128AFFC0B00AC24DC /* Packages */, C1AF3AE62818785B003BAEFF /* README.md */, F6016E8A1D2BD973003497D7 /* Source */, @@ -292,7 +294,11 @@ isa = XCBuildConfiguration; buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; + CODE_SIGN_ENTITLEMENTS = "Example-macOS.entitlements"; + CODE_SIGN_IDENTITY = "Apple Development"; + CODE_SIGN_STYLE = Automatic; COMBINE_HIDPI_IMAGES = YES; + DEVELOPMENT_TEAM = ""; INFOPLIST_FILE = "$(SRCROOT)/Source/Info.plist"; LD_RUNPATH_SEARCH_PATHS = ( "$(inherited)", @@ -300,6 +306,7 @@ ); PRODUCT_BUNDLE_IDENTIFIER = "com.example.GTMAppAuth.Example-macOS"; PRODUCT_NAME = "Example-macOS"; + PROVISIONING_PROFILE_SPECIFIER = ""; }; name = Debug; }; @@ -307,7 +314,11 @@ isa = XCBuildConfiguration; buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; + CODE_SIGN_ENTITLEMENTS = "Example-macOS.entitlements"; + CODE_SIGN_IDENTITY = "Apple Development"; + CODE_SIGN_STYLE = Automatic; COMBINE_HIDPI_IMAGES = YES; + DEVELOPMENT_TEAM = ""; INFOPLIST_FILE = "$(SRCROOT)/Source/Info.plist"; LD_RUNPATH_SEARCH_PATHS = ( "$(inherited)", @@ -315,6 +326,7 @@ ); PRODUCT_BUNDLE_IDENTIFIER = "com.example.GTMAppAuth.Example-macOS"; PRODUCT_NAME = "Example-macOS"; + PROVISIONING_PROFILE_SPECIFIER = ""; }; name = Release; }; diff --git a/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m b/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m index 5f047c17..45f2172f 100644 --- a/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m +++ b/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m @@ -30,6 +30,8 @@ #import "AppDelegate.h" +static NSString *const kTeamIDPrefix = @"YOUR_TEAM_ID"; + /*! @brief The OIDC issuer from which the configuration will be discovered. */ static NSString *const kIssuer = @"https://accounts.google.com"; @@ -66,13 +68,17 @@ - (void)viewDidLoad { [super viewDidLoad]; GTMKeychainAttribute *dataProtection = [GTMKeychainAttribute useDataProtectionKeychain]; - GTMKeychainAttribute *accessGroup = [GTMKeychainAttribute keychainAccessGroupWithName:@"test"]; + NSString *bundleID = @"com.example.GTMAppAuth.Example-macOS.test-group"; + NSString *testGroup = [NSString stringWithFormat:@"%@.%@", kTeamIDPrefix, bundleID]; + GTMKeychainAttribute *accessGroup = [GTMKeychainAttribute keychainAccessGroupWithName:testGroup]; NSSet *attributes = [NSSet setWithArray:@[dataProtection, accessGroup]]; self.keychainStore = [[GTMKeychainStore alloc] initWithItemName:kExampleAuthorizerKey keychainAttributes:attributes]; -// self.keychainStore = [[GTMKeychainStore alloc] initWithItemName:kExampleAuthorizerKey]; #if !defined(NS_BLOCK_ASSERTIONS) + NSAssert(![kTeamIDPrefix isEqualToString:@"YOUR_TEAM_ID"], + @"Update kTeamIDPrefix with your own team ID."); + // NOTE: // // To run this sample, you need to register your own Google API client at From 3ca4379b13454a0247d5a5b314da6e3f8a4b2c0f Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Wed, 24 Jan 2024 17:51:04 -0800 Subject: [PATCH 06/14] Add macOS example entitlement file --- Examples/Example-macOS/Example-macOS.entitlements | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 Examples/Example-macOS/Example-macOS.entitlements diff --git a/Examples/Example-macOS/Example-macOS.entitlements b/Examples/Example-macOS/Example-macOS.entitlements new file mode 100644 index 00000000..92a2b454 --- /dev/null +++ b/Examples/Example-macOS/Example-macOS.entitlements @@ -0,0 +1,10 @@ + + + + + keychain-access-groups + + $(AppIdentifierPrefix)com.example.GTMAppAuth.Example-macOS.test-group + + + From 4f9f90d2972eb204cce7bdfe9292a648ddde0d0e Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Fri, 26 Jan 2024 14:21:18 -0800 Subject: [PATCH 07/14] Check availability static property for useDataProtectionKeychain --- GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift b/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift index b13b5f90..634aefbb 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainAttribute.swift @@ -58,7 +58,7 @@ public final class KeychainAttribute: NSObject { /// Creates an instance of `KeychainAttribute` whose attribute is set to /// `.useDataProtectionKeychain`. /// - Returns: An instance of `KeychainAttribute`. - @available(macOS 10.15, *) + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) @objc public static let useDataProtectionKeychain = KeychainAttribute( attribute: .useDataProtectionKeychain ) From 841d54aefa8f60a4ae899275f81fe3df6d24924b Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Fri, 26 Jan 2024 15:23:22 -0800 Subject: [PATCH 08/14] Add test verifying the correct keychain attributes are used --- .../KeychainStore/KeychainStoreTests.swift | 45 ++++++++++++++++--- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/GTMAppAuth/Tests/Unit/KeychainStore/KeychainStoreTests.swift b/GTMAppAuth/Tests/Unit/KeychainStore/KeychainStoreTests.swift index 4b621884..5043d053 100644 --- a/GTMAppAuth/Tests/Unit/KeychainStore/KeychainStoreTests.swift +++ b/GTMAppAuth/Tests/Unit/KeychainStore/KeychainStoreTests.swift @@ -48,8 +48,41 @@ class KeychainStoreTests: XCTestCase { keychainHelper.passwordStore.removeAll() keychainHelper.generatedKeychainQuery = nil } + + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) + func testKeychainAttributeKeysHaveCorrectNames() throws { + let expectedAccessGroup = "unit-test-group" + let attributes: Set = [ + KeychainAttribute.useDataProtectionKeychain, + KeychainAttribute.keychainAccessGroup(name: expectedAccessGroup) + ] + let keychainHelperFake = KeychainHelperFake(keychainAttributes: attributes) + let store = KeychainStore( + itemName: TestingConstants.testKeychainItemName, + keychainHelper: keychainHelperFake + ) + + try store.save(authSession: authSession) + guard let testQuery = keychainHelperFake.generatedKeychainQuery as? [String: AnyHashable] else { + XCTFail("`keychainHelperFake` missing keychain query attributes") + return + } + + let comparisonQuery = comparisonKeychainQuery( + withAttributes: attributes, + accountName: keychainHelperFake.accountName, + service: TestingConstants.testKeychainItemName + ) + XCTAssertEqual(testQuery, comparisonQuery) + XCTAssertNotNil(testQuery[kSecUseDataProtectionKeychain as String]) + guard let testAccessGroup = testQuery[kSecAttrAccessGroup as String] as? String else { + XCTFail("`testQuery` should have a keychain access group") + return + } + XCTAssertEqual(testAccessGroup, expectedAccessGroup) + } - @available(macOS 10.15, *) + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) func testKeychainQueryHasDataProtectionAttributeOnSave() throws { let useDataProtectionAttributeSet: Set = [.useDataProtectionKeychain] let fakeWithDataProtection = KeychainHelperFake( @@ -109,7 +142,7 @@ class KeychainStoreTests: XCTestCase { XCTAssertEqual(expectedGroupName, testGroupName) } - @available(macOS 10.15, *) + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) func testKeychainQueryHasDataProtectionAndAccessGroupAttributesOnSave() throws { let expectedGroupName = "testGroup" let accessGroupAttributeSet: Set = [ @@ -154,7 +187,7 @@ class KeychainStoreTests: XCTestCase { XCTAssertEqual(testAccessGroupName, expectedGroupName) } - @available(macOS 10.15, *) + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) func testKeychainQueryHasDataProtectionAttributeOnRead() throws { let useDataProtectionAttributeSet: Set = [.useDataProtectionKeychain] let fakeWithDataProtection = KeychainHelperFake( @@ -216,7 +249,7 @@ class KeychainStoreTests: XCTestCase { XCTAssertEqual(expectedGroupName, testGroupName) } - @available(macOS 10.15, *) + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) func testKeychainQueryHasDataProtectionAndAccessGroupAttributesOnRead() throws { let expectedGroupName = "testGroup" let accessGroupAttributeSet: Set = [ @@ -262,7 +295,7 @@ class KeychainStoreTests: XCTestCase { XCTAssertEqual(testAccessGroupName, expectedGroupName) } - @available(macOS 10.15, *) + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) func testKeychainQueryHasDataProtectionAttributeOnRemove() throws { let useDataProtectionAttributeSet: Set = [.useDataProtectionKeychain] let fakeWithDataProtection = KeychainHelperFake( @@ -324,7 +357,7 @@ class KeychainStoreTests: XCTestCase { XCTAssertEqual(expectedGroupName, testGroupName) } - @available(macOS 10.15, *) + @available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) func testKeychainQueryHasDataProtectionAndAccessGroupAttributesOnRemove() throws { let expectedGroupName = "testGroup" let accessGroupAttributeSet: Set = [ From e6630b430bec10925cbd0d1b327e2d04bca92911 Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Mon, 29 Jan 2024 14:53:30 -0800 Subject: [PATCH 09/14] Add doc comments and constants to build access group --- .../Source/GTMAppAuthExampleViewController.m | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m b/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m index 45f2172f..acee3ead 100644 --- a/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m +++ b/Examples/Example-macOS/Source/GTMAppAuthExampleViewController.m @@ -30,6 +30,16 @@ #import "AppDelegate.h" +/*! @brief The bundle ID will use in constructing the app group string for keychain queries. + @discussion The string here is a combination of this example app's bundle ID and the keychain + access group name added in the app's entitlements file. + */ +static NSString *kBundleIDAccessGroup = @"com.example.GTMAppAuth.Example-macOS.test-group"; + +/*! @brief The team ID you will use in constructing the app group string for keychain queries. + @discussion The team ID you will use can be found in your developer team profile page on + developer.apple.com. + */ static NSString *const kTeamIDPrefix = @"YOUR_TEAM_ID"; /*! @brief The OIDC issuer from which the configuration will be discovered. @@ -68,8 +78,7 @@ - (void)viewDidLoad { [super viewDidLoad]; GTMKeychainAttribute *dataProtection = [GTMKeychainAttribute useDataProtectionKeychain]; - NSString *bundleID = @"com.example.GTMAppAuth.Example-macOS.test-group"; - NSString *testGroup = [NSString stringWithFormat:@"%@.%@", kTeamIDPrefix, bundleID]; + NSString *testGroup = [NSString stringWithFormat:@"%@.%@", kTeamIDPrefix, kBundleIDAccessGroup]; GTMKeychainAttribute *accessGroup = [GTMKeychainAttribute keychainAccessGroupWithName:testGroup]; NSSet *attributes = [NSSet setWithArray:@[dataProtection, accessGroup]]; self.keychainStore = [[GTMKeychainStore alloc] initWithItemName:kExampleAuthorizerKey From ca3a39290c2bd705f4d20b0a9f72a403f8414c87 Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Tue, 30 Jan 2024 16:10:50 -0800 Subject: [PATCH 10/14] Update KeychainHelper protocol to take optional CFString Also update use of kSecAttrAccessible on macOS to only occur if also using kSecUseDataProtectionKeychain per Apple docs: https://developer.apple.com/documentation/security/ksecattraccessible\?language\=objc --- .../KeychainStore/KeychainHelper.swift | 4 +-- .../Sources/KeychainStore/KeychainStore.swift | 36 +++++++++++++++++-- .../Tests/Helpers/KeychainHelperFake.swift | 2 +- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift index 9f970a7a..16b15d8a 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift @@ -26,7 +26,7 @@ public protocol KeychainHelper { func password(forService service: String) throws -> String func passwordData(forService service: String) throws -> Data func removePassword(forService service: String) throws - func setPassword(_ password: String, forService service: String, accessibility: CFTypeRef) throws + func setPassword(_ password: String, forService service: String, accessibility: CFTypeRef?) throws func setPassword(data: Data, forService service: String, accessibility: CFTypeRef?) throws } @@ -104,7 +104,7 @@ final class KeychainWrapper: KeychainHelper { func setPassword( _ password: String, forService service: String, - accessibility: CFTypeRef + accessibility: CFTypeRef? ) throws { let passwordData = Data(password.utf8) try setPassword(data: passwordData, forService: service, accessibility: accessibility) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift b/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift index 63af95af..bc654466 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift @@ -101,10 +101,20 @@ public final class KeychainStore: NSObject, AuthSessionStore { @objc(saveAuthSession:error:) public func save(authSession: AuthSession) throws { let authSessionData: Data = try authSessionData(fromAuthSession: authSession) + + var maybeAccessibility: CFString? = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly + if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { + #if os(macOS) + if !keychainAttributes.contains(.useDataProtectionKeychain) { + maybeAccessibility = nil + } + #endif + } + try keychainHelper.setPassword( data: authSessionData, forService: itemName, - accessibility: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly + accessibility: maybeAccessibility ) } @@ -118,10 +128,20 @@ public final class KeychainStore: NSObject, AuthSessionStore { @objc(saveAuthSession:withItemName:error:) public func save(authSession: AuthSession, withItemName itemName: String) throws { let authSessionData = try authSessionData(fromAuthSession: authSession) + + var maybeAccessibility: CFString? = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly + if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { + #if os(macOS) + if !keychainAttributes.contains(.useDataProtectionKeychain) { + maybeAccessibility = nil + } + #endif + } + try keychainHelper.setPassword( data: authSessionData, forService: itemName, - accessibility: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly + accessibility: maybeAccessibility ) } @@ -268,10 +288,20 @@ public final class KeychainStore: NSObject, AuthSessionStore { .persistenceResponseString(forAuthSession: authSession) else { throw KeychainStore.Error.failedToCreateResponseStringFromAuthSession(authSession) } + + var maybeAccessibility: CFString? = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly + if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { + #if os(macOS) + if !keychainAttributes.contains(.useDataProtectionKeychain) { + maybeAccessibility = nil + } + #endif + } + try keychainHelper.setPassword( persistence, forService: itemName, - accessibility: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly) + accessibility: maybeAccessibility) } } diff --git a/GTMAppAuth/Tests/Helpers/KeychainHelperFake.swift b/GTMAppAuth/Tests/Helpers/KeychainHelperFake.swift index 6a7cc87a..2160204b 100644 --- a/GTMAppAuth/Tests/Helpers/KeychainHelperFake.swift +++ b/GTMAppAuth/Tests/Helpers/KeychainHelperFake.swift @@ -91,7 +91,7 @@ public class KeychainHelperFake: NSObject, KeychainHelper { @objc public func setPassword( _ password: String, forService service: String, - accessibility: CFTypeRef + accessibility: CFTypeRef? ) throws { do { try removePassword(forService: service) From 25a950cd3c22c5f1a71af40423fe2b406d6688ac Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Wed, 31 Jan 2024 14:39:21 -0800 Subject: [PATCH 11/14] Add method to KeychainHelper protocol taking no accessibility The other existing methods either take an accessibility param or an optional param. --- .../KeychainStore/KeychainHelper.swift | 10 ++- .../Sources/KeychainStore/KeychainStore.swift | 15 +++-- .../Helpers/GTMAppAuthTestingHelpers.swift | 1 + .../Tests/Helpers/KeychainHelperFake.swift | 19 +++++- .../GTMOAuth2CompatibilityTests.swift | 63 +++++++++++++++++++ 5 files changed, 98 insertions(+), 10 deletions(-) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift index 16b15d8a..6b9425c8 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainHelper.swift @@ -26,7 +26,8 @@ public protocol KeychainHelper { func password(forService service: String) throws -> String func passwordData(forService service: String) throws -> Data func removePassword(forService service: String) throws - func setPassword(_ password: String, forService service: String, accessibility: CFTypeRef?) throws + func setPassword(_ password: String, forService service: String) throws + func setPassword(_ password: String, forService service: String, accessibility: CFTypeRef) throws func setPassword(data: Data, forService service: String, accessibility: CFTypeRef?) throws } @@ -100,11 +101,16 @@ final class KeychainWrapper: KeychainHelper { throw KeychainStore.Error.failedToDeletePassword(forItemName: service) } } + + func setPassword(_ password: String, forService service: String) throws { + let passwordData = Data(password.utf8) + try setPassword(data: passwordData, forService: service, accessibility: nil) + } func setPassword( _ password: String, forService service: String, - accessibility: CFTypeRef? + accessibility: CFTypeRef ) throws { let passwordData = Data(password.utf8) try setPassword(data: passwordData, forService: service, accessibility: accessibility) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift b/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift index bc654466..309ab47a 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift @@ -62,10 +62,11 @@ public final class KeychainStore: NSObject, AuthSessionStore { /// - Parameters: /// - itemName: The `String` name for the credential to store in the keychain. /// - keychainHelper: An instance conforming to `KeychainHelper`. + /// - Note: The `KeychainHelper`'s `keychainAttributes` are used if present. @objc public convenience init(itemName: String, keychainHelper: KeychainHelper) { self.init( itemName: itemName, - keychainAttributes: [], + keychainAttributes: keychainHelper.keychainAttributes, keychainHelper: keychainHelper ) } @@ -289,19 +290,21 @@ public final class KeychainStore: NSObject, AuthSessionStore { throw KeychainStore.Error.failedToCreateResponseStringFromAuthSession(authSession) } - var maybeAccessibility: CFString? = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { - #if os(macOS) + // We must use `kSecUseDataProtectionKeychain` if we use a `kSecAttrAccessible` attribute + // (https://developer.apple.com/documentation/security/ksecattraccessible?language=objc) +#if os(macOS) if !keychainAttributes.contains(.useDataProtectionKeychain) { - maybeAccessibility = nil + try keychainHelper.setPassword(persistence, forService: itemName) + return } - #endif +#endif } try keychainHelper.setPassword( persistence, forService: itemName, - accessibility: maybeAccessibility) + accessibility: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly) } } diff --git a/GTMAppAuth/Tests/Helpers/GTMAppAuthTestingHelpers.swift b/GTMAppAuth/Tests/Helpers/GTMAppAuthTestingHelpers.swift index 71421f73..1008cc57 100644 --- a/GTMAppAuth/Tests/Helpers/GTMAppAuthTestingHelpers.swift +++ b/GTMAppAuth/Tests/Helpers/GTMAppAuthTestingHelpers.swift @@ -26,6 +26,7 @@ public protocol Testing { @objc(GTMTestingConstants) public class TestingConstants: NSObject { + @objc public static let testAccessGroup = "testAccessGroup" @objc public static let testAccessToken = "access_token" @objc public static let accessTokenExpiresIn = 3600 @objc public static let testRefreshToken = "refresh_token" diff --git a/GTMAppAuth/Tests/Helpers/KeychainHelperFake.swift b/GTMAppAuth/Tests/Helpers/KeychainHelperFake.swift index 2160204b..7a17da94 100644 --- a/GTMAppAuth/Tests/Helpers/KeychainHelperFake.swift +++ b/GTMAppAuth/Tests/Helpers/KeychainHelperFake.swift @@ -88,10 +88,25 @@ public class KeychainHelperFake: NSObject, KeychainHelper { } } + @objc public func setPassword(_ password: String, forService service: String) throws { + do { + try removePassword(forService: service) + } catch KeychainStore.Error.failedToDeletePasswordBecauseItemNotFound { + // No need to throw this error since we are setting a new password + } catch { + throw error + } + + guard let passwordData = password.data(using: .utf8) else { + throw KeychainStore.Error.unexpectedPasswordData(forItemName: service) + } + try setPassword(data: passwordData, forService: service, accessibility: nil) + } + @objc public func setPassword( _ password: String, forService service: String, - accessibility: CFTypeRef? + accessibility: CFTypeRef ) throws { do { try removePassword(forService: service) @@ -104,7 +119,7 @@ public class KeychainHelperFake: NSObject, KeychainHelper { guard let passwordData = password.data(using: .utf8) else { throw KeychainStore.Error.unexpectedPasswordData(forItemName: service) } - try setPassword(data: passwordData, forService: service, accessibility: nil) + try setPassword(data: passwordData, forService: service, accessibility: accessibility) } @objc public func setPassword( diff --git a/GTMAppAuth/Tests/Unit/KeychainStore/GTMOAuth2CompatibilityTests.swift b/GTMAppAuth/Tests/Unit/KeychainStore/GTMOAuth2CompatibilityTests.swift index 49dfb521..26b55629 100644 --- a/GTMAppAuth/Tests/Unit/KeychainStore/GTMOAuth2CompatibilityTests.swift +++ b/GTMAppAuth/Tests/Unit/KeychainStore/GTMOAuth2CompatibilityTests.swift @@ -29,6 +29,16 @@ class GTMOAuth2CompatibilityTests: XCTestCase { private lazy var testPersistenceString: String = { return "access_token=\(TestingConstants.testAccessToken)&refresh_token=\(TestingConstants.testRefreshToken)&scope=\(TestingConstants.testScope2)&serviceProvider=\(TestingConstants.testServiceProvider)&userEmail=foo%40foo.com&userEmailIsVerified=y&userID=\(TestingConstants.testUserID)" }() + private let keychainHelperWithAttributes = KeychainHelperFake( + keychainAttributes: [.useDataProtectionKeychain, + .keychainAccessGroup(name: TestingConstants.testAccessGroup)] + ) + private lazy var keychainStoreWithAttributes: KeychainStore = { + return KeychainStore( + itemName: TestingConstants.testKeychainItemName, + keychainHelper: keychainHelperWithAttributes + ) + }() private let keychainHelper = KeychainHelperFake(keychainAttributes: []) private lazy var keychainStore: KeychainStore = { return KeychainStore( @@ -63,6 +73,9 @@ class GTMOAuth2CompatibilityTests: XCTestCase { func testSaveOAuth2AuthSession() throws { try keychainStore.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession) + // Save with keychain attributes to simulate macOS environment with + // `kSecUseDataProtectionKeychain` + try keychainStoreWithAttributes.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession) } func testSaveGTMOAuth2AuthSessionThrowsError() { @@ -80,7 +93,34 @@ class GTMOAuth2CompatibilityTests: XCTestCase { func testRemoveOAuth2AuthSession() throws { try keychainStore.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession) + let _ = try keychainStore.retrieveAuthSessionInGTMOAuth2Format( + tokenURL: TestingConstants.testTokenURL, + redirectURI: TestingConstants.testRedirectURI, + clientID: TestingConstants.testClientID, + clientSecret: TestingConstants.testClientSecret + ) try keychainStore.removeAuthSession() + XCTAssertThrowsError(try keychainStore.retrieveAuthSession()) { thrownError in + XCTAssertEqual( + thrownError as? KeychainStore.Error, + KeychainStore.Error.passwordNotFound(forItemName: TestingConstants.testKeychainItemName) + ) + } + + try keychainStoreWithAttributes.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession) + let _ = try keychainStoreWithAttributes.retrieveAuthSessionInGTMOAuth2Format( + tokenURL: TestingConstants.testTokenURL, + redirectURI: TestingConstants.testRedirectURI, + clientID: TestingConstants.testClientID, + clientSecret: TestingConstants.testClientSecret + ) + try keychainStoreWithAttributes.removeAuthSession() + XCTAssertThrowsError(try keychainStoreWithAttributes.retrieveAuthSession()) { thrownError in + XCTAssertEqual( + thrownError as? KeychainStore.Error, + KeychainStore.Error.passwordNotFound(forItemName: TestingConstants.testKeychainItemName) + ) + } } func testRemoveOAuth2AuthSessionhrowsError() { @@ -116,6 +156,29 @@ class GTMOAuth2CompatibilityTests: XCTestCase { XCTAssertEqual(authSession.userEmailIsVerified, expectedAuthSession.userEmailIsVerified) XCTAssertEqual(authSession.canAuthorize, expectedAuthSession.canAuthorize) } + + func testAuthSessionFromKeychainWithAttributesForName() throws { + try keychainStoreWithAttributes.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession) + let authSession = try keychainStoreWithAttributes.retrieveAuthSessionInGTMOAuth2Format( + tokenURL: TestingConstants.testTokenURL, + redirectURI: TestingConstants.testRedirectURI, + clientID: TestingConstants.testClientID, + clientSecret: TestingConstants.testClientID + ) + + XCTAssertEqual(authSession.authState.scope, expectedAuthSession.authState.scope) + XCTAssertEqual( + authSession.authState.lastTokenResponse?.accessToken, + expectedAuthSession.authState.lastTokenResponse?.accessToken + ) + XCTAssertEqual(authSession.authState.refreshToken, expectedAuthSession.authState.refreshToken) + XCTAssertEqual(authSession.authState.isAuthorized, expectedAuthSession.authState.isAuthorized) + XCTAssertEqual(authSession.serviceProvider, expectedAuthSession.serviceProvider) + XCTAssertEqual(authSession.userID, expectedAuthSession.userID) + XCTAssertEqual(authSession.userEmail, expectedAuthSession.userEmail) + XCTAssertEqual(authSession.userEmailIsVerified, expectedAuthSession.userEmailIsVerified) + XCTAssertEqual(authSession.canAuthorize, expectedAuthSession.canAuthorize) + } func testAuthSessionFromKeychainForNameThrowsError() throws { try keychainStore.saveWithGTMOAuth2Format(forAuthSession: expectedAuthSession) From c30b646974c507601c3b1f978a77f69df846d349 Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Wed, 31 Jan 2024 14:54:16 -0800 Subject: [PATCH 12/14] Add availability check for useDataProtection in test so all platforms pass --- .../KeychainStore/GTMOAuth2CompatibilityTests.swift | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/GTMAppAuth/Tests/Unit/KeychainStore/GTMOAuth2CompatibilityTests.swift b/GTMAppAuth/Tests/Unit/KeychainStore/GTMOAuth2CompatibilityTests.swift index 26b55629..5245d9f5 100644 --- a/GTMAppAuth/Tests/Unit/KeychainStore/GTMOAuth2CompatibilityTests.swift +++ b/GTMAppAuth/Tests/Unit/KeychainStore/GTMOAuth2CompatibilityTests.swift @@ -29,11 +29,15 @@ class GTMOAuth2CompatibilityTests: XCTestCase { private lazy var testPersistenceString: String = { return "access_token=\(TestingConstants.testAccessToken)&refresh_token=\(TestingConstants.testRefreshToken)&scope=\(TestingConstants.testScope2)&serviceProvider=\(TestingConstants.testServiceProvider)&userEmail=foo%40foo.com&userEmailIsVerified=y&userID=\(TestingConstants.testUserID)" }() - private let keychainHelperWithAttributes = KeychainHelperFake( - keychainAttributes: [.useDataProtectionKeychain, - .keychainAccessGroup(name: TestingConstants.testAccessGroup)] - ) private lazy var keychainStoreWithAttributes: KeychainStore = { + let attributes: Set + if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { + attributes = [.useDataProtectionKeychain, + .keychainAccessGroup(name: TestingConstants.testAccessGroup)] + } else { + attributes = [.keychainAccessGroup(name: TestingConstants.testAccessGroup)] + } + let keychainHelperWithAttributes = KeychainHelperFake(keychainAttributes: attributes) return KeychainStore( itemName: TestingConstants.testKeychainItemName, keychainHelper: keychainHelperWithAttributes From 8610b19165759bfd6467564adedb5cb70fc622c4 Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Wed, 31 Jan 2024 16:10:20 -0800 Subject: [PATCH 13/14] Update indentation for #if os checks --- GTMAppAuth/Sources/KeychainStore/KeychainStore.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift b/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift index 309ab47a..82623bda 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift @@ -105,11 +105,11 @@ public final class KeychainStore: NSObject, AuthSessionStore { var maybeAccessibility: CFString? = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { - #if os(macOS) +#if os(macOS) if !keychainAttributes.contains(.useDataProtectionKeychain) { maybeAccessibility = nil } - #endif +#endif } try keychainHelper.setPassword( @@ -132,11 +132,11 @@ public final class KeychainStore: NSObject, AuthSessionStore { var maybeAccessibility: CFString? = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { - #if os(macOS) +#if os(macOS) if !keychainAttributes.contains(.useDataProtectionKeychain) { maybeAccessibility = nil } - #endif +#endif } try keychainHelper.setPassword( From 33feab398ba11f15495767e32fd74238e86fcfd7 Mon Sep 17 00:00:00 2001 From: Matthew Mathias Date: Thu, 1 Feb 2024 10:38:12 -0800 Subject: [PATCH 14/14] Streamline calls to save the AuthSession --- .../Sources/KeychainStore/KeychainStore.swift | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift b/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift index 82623bda..885fdadf 100644 --- a/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift +++ b/GTMAppAuth/Sources/KeychainStore/KeychainStore.swift @@ -101,22 +101,7 @@ public final class KeychainStore: NSObject, AuthSessionStore { @objc(saveAuthSession:error:) public func save(authSession: AuthSession) throws { - let authSessionData: Data = try authSessionData(fromAuthSession: authSession) - - var maybeAccessibility: CFString? = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly - if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { -#if os(macOS) - if !keychainAttributes.contains(.useDataProtectionKeychain) { - maybeAccessibility = nil - } -#endif - } - - try keychainHelper.setPassword( - data: authSessionData, - forService: itemName, - accessibility: maybeAccessibility - ) + try save(authSession: authSession, withItemName: itemName) } /// Saves the provided `AuthSession` using the provided item name. @@ -132,6 +117,8 @@ public final class KeychainStore: NSObject, AuthSessionStore { var maybeAccessibility: CFString? = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { + // On macOS, we must use `kSecUseDataProtectionKeychain` if using `kSecAttrAccessible` + // (https://developer.apple.com/documentation/security/ksecattraccessible?language=objc) #if os(macOS) if !keychainAttributes.contains(.useDataProtectionKeychain) { maybeAccessibility = nil @@ -291,7 +278,7 @@ public final class KeychainStore: NSObject, AuthSessionStore { } if #available(macOS 10.15, iOS 13.0, tvOS 13.0, watchOS 6.0, *) { - // We must use `kSecUseDataProtectionKeychain` if we use a `kSecAttrAccessible` attribute + // On macOS, we must use `kSecUseDataProtectionKeychain` if using `kSecAttrAccessible` // (https://developer.apple.com/documentation/security/ksecattraccessible?language=objc) #if os(macOS) if !keychainAttributes.contains(.useDataProtectionKeychain) {