From 7f995f3fbbb98c9f3e14fdd79005cfba15d51d3f Mon Sep 17 00:00:00 2001 From: Andrew Heard Date: Wed, 18 Oct 2023 12:20:23 -0400 Subject: [PATCH 1/3] Migrate FIRAppAttestKeyIDStorage from UserDefaults to Keychain --- .../AppAttestProvider/FIRAppAttestProvider.m | 4 +- .../Storage/FIRAppAttestKeyIDStorage.h | 10 ++- .../Storage/FIRAppAttestKeyIDStorage.m | 87 ++++++++++++------- .../Storage/FIRAppAttestKeyIDStorageTests.m | 39 +++++++-- 4 files changed, 103 insertions(+), 37 deletions(-) diff --git a/FirebaseAppCheck/Sources/AppAttestProvider/FIRAppAttestProvider.m b/FirebaseAppCheck/Sources/AppAttestProvider/FIRAppAttestProvider.m index b59e8101009..aba6285defb 100644 --- a/FirebaseAppCheck/Sources/AppAttestProvider/FIRAppAttestProvider.m +++ b/FirebaseAppCheck/Sources/AppAttestProvider/FIRAppAttestProvider.m @@ -140,7 +140,9 @@ - (nullable instancetype)initWithApp:(FIRApp *)app { sessionWithConfiguration:[NSURLSessionConfiguration ephemeralSessionConfiguration]]; FIRAppAttestKeyIDStorage *keyIDStorage = - [[FIRAppAttestKeyIDStorage alloc] initWithAppName:app.name appID:app.options.googleAppID]; + [[FIRAppAttestKeyIDStorage alloc] initWithAppName:app.name + appID:app.options.googleAppID + accessGroup:app.options.appGroupID]; FIRAppCheckAPIService *APIService = [[FIRAppCheckAPIService alloc] initWithURLSession:URLSession diff --git a/FirebaseAppCheck/Sources/AppAttestProvider/Storage/FIRAppAttestKeyIDStorage.h b/FirebaseAppCheck/Sources/AppAttestProvider/Storage/FIRAppAttestKeyIDStorage.h index c0ff9ebd7d1..1ec677c3f41 100644 --- a/FirebaseAppCheck/Sources/AppAttestProvider/Storage/FIRAppAttestKeyIDStorage.h +++ b/FirebaseAppCheck/Sources/AppAttestProvider/Storage/FIRAppAttestKeyIDStorage.h @@ -17,6 +17,7 @@ #import @class FBLPromise; +@class GULKeychainStorage; NS_ASSUME_NONNULL_BEGIN @@ -53,7 +54,14 @@ NS_ASSUME_NONNULL_BEGIN * @param appID A Firebase App identifier (`FirebaseOptions.googleAppID`). The app ID will be used * as a part of the key to store the token for the storage instance. */ -- (instancetype)initWithAppName:(NSString *)appName appID:(NSString *)appID; +- (instancetype)initWithAppName:(NSString *)appName + appID:(NSString *)appID + accessGroup:(nullable NSString *)accessGroup; + +- (instancetype)initWithAppName:(NSString *)appName + appID:(NSString *)appID + keychainStorage:(GULKeychainStorage *)keychainStorage + accessGroup:(nullable NSString *)accessGroup NS_DESIGNATED_INITIALIZER; @end diff --git a/FirebaseAppCheck/Sources/AppAttestProvider/Storage/FIRAppAttestKeyIDStorage.m b/FirebaseAppCheck/Sources/AppAttestProvider/Storage/FIRAppAttestKeyIDStorage.m index 5eb8ffd88f1..83628960ce0 100644 --- a/FirebaseAppCheck/Sources/AppAttestProvider/Storage/FIRAppAttestKeyIDStorage.m +++ b/FirebaseAppCheck/Sources/AppAttestProvider/Storage/FIRAppAttestKeyIDStorage.m @@ -22,64 +22,91 @@ #import "FBLPromises.h" #endif +#import + #import "FirebaseAppCheck/Sources/Core/Errors/FIRAppCheckErrorUtil.h" -/// The `NSUserDefaults` suite name for the storage location of the app attest key ID. -static NSString *const kKeyIDStorageDefaultsSuiteName = @"com.firebase.FIRAppAttestKeyIDStorage"; +static NSString *const kKeychainService = @"com.firebase.app_check.app_attest_key_id_storage"; @interface FIRAppAttestKeyIDStorage () @property(nonatomic, readonly) NSString *appName; @property(nonatomic, readonly) NSString *appID; - -/// The app attest key ID is stored using `NSUserDefaults` . -@property(nonatomic, readonly) NSUserDefaults *userDefaults; +@property(nonatomic, readonly) GULKeychainStorage *keychainStorage; +@property(nonatomic, readonly, nullable) NSString *accessGroup; @end @implementation FIRAppAttestKeyIDStorage -- (instancetype)initWithAppName:(NSString *)appName appID:(NSString *)appID { +- (instancetype)initWithAppName:(NSString *)appName + appID:(NSString *)appID + keychainStorage:(GULKeychainStorage *)keychainStorage + accessGroup:(nullable NSString *)accessGroup { self = [super init]; if (self) { _appName = [appName copy]; _appID = [appID copy]; - _userDefaults = [[NSUserDefaults alloc] initWithSuiteName:kKeyIDStorageDefaultsSuiteName]; + _keychainStorage = keychainStorage; + _accessGroup = [accessGroup copy]; } return self; } -- (nonnull FBLPromise *)setAppAttestKeyID:(nullable NSString *)keyID { - [self storeAppAttestKeyID:keyID]; - return [FBLPromise resolvedWith:keyID]; +- (instancetype)initWithAppName:(NSString *)appName + appID:(NSString *)appID + accessGroup:(nullable NSString *)accessGroup { + GULKeychainStorage *keychainStorage = + [[GULKeychainStorage alloc] initWithService:kKeychainService]; + return [self initWithAppName:appName + appID:appID + keychainStorage:keychainStorage + accessGroup:accessGroup]; } -- (nonnull FBLPromise *)getAppAttestKeyID { - NSString *appAttestKeyID = [self appAttestKeyIDFromStorage]; - if (appAttestKeyID) { - return [FBLPromise resolvedWith:appAttestKeyID]; +- (nonnull FBLPromise *)setAppAttestKeyID:(nullable NSString *)keyID { + if (keyID) { + return [self storeAppAttestKeyID:keyID].recover(^NSError *(NSError *error) { + return [FIRAppCheckErrorUtil keychainErrorWithError:error]; + }); } else { - NSError *error = [FIRAppCheckErrorUtil appAttestKeyIDNotFound]; - FBLPromise *rejectedPromise = [FBLPromise pendingPromise]; - [rejectedPromise reject:error]; - return rejectedPromise; + return [self.keychainStorage removeObjectForKey:[self keyIDStorageKey] + accessGroup:self.accessGroup] + .then(^id _Nullable(NSNull *_Nullable value) { + return nil; + }) + .recover(^NSError *(NSError *error) { + return [FIRAppCheckErrorUtil keychainErrorWithError:error]; + }); } } -#pragma mark - Helpers - -- (void)storeAppAttestKeyID:(nullable NSString *)keyID { - if (keyID) { - [self.userDefaults setObject:keyID forKey:[self keyIDStorageKey]]; - } else { - [self.userDefaults removeObjectForKey:[self keyIDStorageKey]]; - } +- (nonnull FBLPromise *)getAppAttestKeyID { + return [self.keychainStorage getObjectForKey:[self keyIDStorageKey] + objectClass:[NSString class] + accessGroup:self.accessGroup] + .then(^NSString *(id storedKeyID) { + NSString *keyID = (NSString *)storedKeyID; + if ([keyID isKindOfClass:[NSString class]]) { + return keyID; + } else { + return nil; + } + }) + .recover(^NSError *(NSError *error) { + return [FIRAppCheckErrorUtil appAttestKeyIDNotFound]; + }); } -- (nullable NSString *)appAttestKeyIDFromStorage { - NSString *appAttestKeyID = nil; - appAttestKeyID = [self.userDefaults objectForKey:[self keyIDStorageKey]]; - return appAttestKeyID; +#pragma mark - Helpers + +- (FBLPromise *)storeAppAttestKeyID:(nullable NSString *)keyID { + return [self.keychainStorage setObject:keyID + forKey:[self keyIDStorageKey] + accessGroup:self.accessGroup] + .then(^id _Nullable(NSNull *_Nullable value) { + return keyID; + }); } - (NSString *)keyIDStorageKey { diff --git a/FirebaseAppCheck/Tests/Unit/AppAttestProvider/Storage/FIRAppAttestKeyIDStorageTests.m b/FirebaseAppCheck/Tests/Unit/AppAttestProvider/Storage/FIRAppAttestKeyIDStorageTests.m index f2db101a5e8..a48dbd7d4ef 100644 --- a/FirebaseAppCheck/Tests/Unit/AppAttestProvider/Storage/FIRAppAttestKeyIDStorageTests.m +++ b/FirebaseAppCheck/Tests/Unit/AppAttestProvider/Storage/FIRAppAttestKeyIDStorageTests.m @@ -16,6 +16,10 @@ #import +#import + +#import + #import "FBLPromise+Testing.h" #import "FirebaseAppCheck/Sources/AppAttestProvider/Storage/FIRAppAttestKeyIDStorage.h" @@ -35,7 +39,9 @@ - (void)setUp { self.appName = @"FIRAppAttestKeyIDStorageTestsApp"; self.appID = @"app_id"; - self.storage = [[FIRAppAttestKeyIDStorage alloc] initWithAppName:self.appName appID:self.appID]; + self.storage = [[FIRAppAttestKeyIDStorage alloc] initWithAppName:self.appName + appID:self.appID + accessGroup:nil]; } - (void)tearDown { @@ -48,7 +54,9 @@ - (void)tearDown { } - (void)testInitWithApp { - XCTAssertNotNil([[FIRAppAttestKeyIDStorage alloc] initWithAppName:self.appName appID:self.appID]); + XCTAssertNotNil([[FIRAppAttestKeyIDStorage alloc] initWithAppName:self.appName + appID:self.appID + accessGroup:nil]); } - (void)testSetAndGetAppAttestKeyID { @@ -73,10 +81,29 @@ - (void)testRemoveAppAttestKeyID { } - (void)testGetAppAttestKeyID_WhenAppAttestKeyIDNotFoundError { - __auto_type getPromise = [self.storage getAppAttestKeyID]; + // 1. Set up storage mock. + id mockKeychainStorage = OCMClassMock([GULKeychainStorage class]); + FIRAppAttestKeyIDStorage *keyIDStorage = + [[FIRAppAttestKeyIDStorage alloc] initWithAppName:self.appName + appID:self.appID + keychainStorage:mockKeychainStorage + accessGroup:nil]; + + // 2. Create and expect keychain error. + NSError *gulsKeychainError = [NSError errorWithDomain:@"com.guls.keychain" code:-1 userInfo:nil]; + OCMExpect([mockKeychainStorage getObjectForKey:[OCMArg any] + objectClass:[OCMArg any] + accessGroup:[OCMArg any]]) + .andReturn([FBLPromise resolvedWith:gulsKeychainError]); + + // 3. Get key ID and verify results. + __auto_type getPromise = [keyIDStorage getAppAttestKeyID]; XCTAssert(FBLWaitForPromisesWithTimeout(0.5)); XCTAssertNotNil(getPromise.error); XCTAssertEqualObjects(getPromise.error, [FIRAppCheckErrorUtil appAttestKeyIDNotFound]); + + // 4. Verify storage mock. + OCMVerifyAll(mockKeychainStorage); } - (void)testSetGetAppAttestKeyIDPerApp { @@ -105,9 +132,11 @@ - (void)assertIndependentSetGetForStoragesWithAppName1:(NSString *)appName1 appID2:(NSString *)appID2 { // Create two storages. FIRAppAttestKeyIDStorage *storage1 = [[FIRAppAttestKeyIDStorage alloc] initWithAppName:appName1 - appID:appID1]; + appID:appID1 + accessGroup:nil]; FIRAppAttestKeyIDStorage *storage2 = [[FIRAppAttestKeyIDStorage alloc] initWithAppName:appName2 - appID:appID2]; + appID:appID2 + accessGroup:nil]; // 1. Independently set app attest key IDs for the two storages. NSString *appAttestKeyID1 = @"app_attest_key_ID1"; FBLPromise *setPromise1 = [storage1 setAppAttestKeyID:appAttestKeyID1]; From 0e14f36302c111263fc520f5b5a01ccdf7d5360d Mon Sep 17 00:00:00 2001 From: Andrew Heard Date: Wed, 18 Oct 2023 12:44:12 -0400 Subject: [PATCH 2/3] Skip Keychain tests on macOS and SPM --- .../Storage/FIRAppAttestKeyIDStorageTests.m | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/FirebaseAppCheck/Tests/Unit/AppAttestProvider/Storage/FIRAppAttestKeyIDStorageTests.m b/FirebaseAppCheck/Tests/Unit/AppAttestProvider/Storage/FIRAppAttestKeyIDStorageTests.m index a48dbd7d4ef..921d9a38b4b 100644 --- a/FirebaseAppCheck/Tests/Unit/AppAttestProvider/Storage/FIRAppAttestKeyIDStorageTests.m +++ b/FirebaseAppCheck/Tests/Unit/AppAttestProvider/Storage/FIRAppAttestKeyIDStorageTests.m @@ -14,6 +14,17 @@ * limitations under the License. */ +#import + +// Tests that use the Keychain require a host app and Swift Package Manager +// does not support adding a host app to test targets. +#if !SWIFT_PACKAGE + +// Skip keychain tests on Catalyst and macOS. Tests are skipped because they +// involve interactions with the keychain that require a provisioning profile. +// See go/firebase-macos-keychain-popups for more details. +#if !TARGET_OS_MACCATALYST && !TARGET_OS_OSX + #import #import @@ -172,3 +183,7 @@ - (void)assertIndependentSetGetForStoragesWithAppName1:(NSString *)appName1 } @end + +#endif // !TARGET_OS_MACCATALYST && !TARGET_OS_OSX + +#endif // !SWIFT_PACKAGE From afd2efda17fbda041e67af8271e3ff08554533c2 Mon Sep 17 00:00:00 2001 From: Andrew Heard Date: Thu, 19 Oct 2023 15:02:43 -0400 Subject: [PATCH 3/3] Add CHANGELOG entry --- FirebaseAppCheck/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/FirebaseAppCheck/CHANGELOG.md b/FirebaseAppCheck/CHANGELOG.md index 72cb7e51d76..1265f19dd32 100644 --- a/FirebaseAppCheck/CHANGELOG.md +++ b/FirebaseAppCheck/CHANGELOG.md @@ -1,6 +1,7 @@ # 10.17.0 - [fixed] Replaced semantic imports (`@import FirebaseAppCheckInterop`) with umbrella header imports - (`#import `) for ObjC++ compatibility (#11916). + (`#import `) for ObjC++ compatibility. (#11916) +- [fixed] Prevented App Attest key migration during setup or restoration of a device from a backup. (#11962) # 10.9.0 - [feature] Added `limitedUseToken(completion:)` for obtaining limited-use tokens for