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

[App Check] Migrate AppAttestKeyIDStorage from UserDefaults to Keychain #11962

Closed
wants to merge 3 commits into from

Conversation

andrewheard
Copy link
Contributor

@andrewheard andrewheard commented Oct 18, 2023

Updated FIRAppAttestKeyIDStorage to use the Keychain as its underlying storage instead of NSUserDefaults. Since we use kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly in GULKeychainUtils, this means that the key IDs will not be migrated to new devices when restoring from backup.

Context: "The keys that you generate remain valid through regular app updates, but don’t survive app reinstallation, device migration, or restoration of a device from a backup." -- Start over on reinstallation

#no-changelog

@andrewheard andrewheard marked this pull request as ready for review October 19, 2023 19:09
@andrewheard andrewheard marked this pull request as draft October 19, 2023 21:27
@paulb777
Copy link
Member

After discussion, we decided to put on hold while investigating a more targeted approach based on a specific error message.

@umairalitail
Copy link

@andrewheard, is this PR related to this issue? #11926

@jostster
Copy link
Contributor

@andrewheard, is this PR related to this issue? #11926

Yes it is related and could solve it. However, I think the ideal approach would be to leave in the UserDefaults and like @paulb777 mentioned is target a specific error message to reset the key like my work around does.

@andrewheard
Copy link
Contributor Author

@andrewheard, is this PR related to this issue? #11926

Yes it is related and could solve it. However, I think the ideal approach would be to leave in the UserDefaults and like @paulb777 mentioned is target a specific error message to reset the key like my work around does.

@umairalitail As @jostster said, this is related and could solve the issue in some scenarios (e.g., device migration) but it's unclear, at least to me, that it would resolve the issue in all scenarios (e.g., app reinstallation on same device). I'm holding off on this PR in favour of handling the specific DCError codes, e.g., DCErrorInvalidInput (code 2) and DCErrorInvalidKey (code 3), that we've been seeing in error reports.

@jostster
Copy link
Contributor

@andrewheard, is this PR related to this issue? #11926

Yes it is related and could solve it. However, I think the ideal approach would be to leave in the UserDefaults and like @paulb777 mentioned is target a specific error message to reset the key like my work around does.

@umairalitail As @jostster said, this is related and could solve the issue in some scenarios (e.g., device migration) but it's unclear, at least to me, that it would resolve the issue in all scenarios (e.g., app reinstallation on same device). I'm holding off on this PR in favour of handling the specific DCError codes, e.g., DCErrorInvalidInput (code 2) and DCErrorInvalidKey (code 3), that we've been seeing in error reports.

Would it make sense to also make the resetAttestation public so developers could reset in other rare use case scenarios?

@firebase firebase locked and limited conversation to collaborators Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants