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] Reset App Attest key state if attestKey fails #11986

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

andrewheard
Copy link
Contributor

@andrewheard andrewheard commented Oct 23, 2023

Resets the stored key ID if attestKey:clientDataHash:completionHandler: fails due to DCErrorInvalidKey. This should resolve App Check / App Attest failures after app reinstallation, device migration, or restoration of a device from a backup (see docs).

@andrewheard andrewheard marked this pull request as ready for review October 24, 2023 17:23
@andrewheard andrewheard requested a review from paulb777 October 24, 2023 17:23
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Thanks!

@andrewheard andrewheard merged commit 1317f08 into master Oct 24, 2023
50 checks passed
@andrewheard andrewheard deleted the ah/appattest-invalidkey branch October 24, 2023 17:30
@jostster
Copy link
Contributor

jostster commented Nov 1, 2023

Should the block attempt to retry the query after a reset instead of continuing with the error?

@paulb777
Copy link
Member

paulb777 commented Nov 1, 2023

@jostster My understanding is that the error is the Promises mechanic to trigger a reset.

@jostster
Copy link
Contributor

jostster commented Nov 1, 2023

@paulb777 Gotcha, so clients would still need to look for the specific error and display something to the user to try the action again? Or would there be a way in the sdk to have it auto retry the previous action when this trigger is hit and the key is reset?

@paulb777
Copy link
Member

paulb777 commented Nov 1, 2023

@andrewheard
Copy link
Contributor Author

My understanding is that the error is the Promises mechanic to trigger a reset.
-- @paulb777

This is accurate. The FIRAppAttestRejectionError isn't propagated to the user. Line 251 will return YES, resulting in the retry block (line 254) being invoked:

return [FBLPromise onQueue:self.queue
attempts:1
delay:0
condition:^BOOL(NSInteger attemptCount, NSError *_Nonnull error) {
// Reset keyID before retrying.
keyIDForAttempt = nil;
return [error isKindOfClass:[FIRAppAttestRejectionError class]];
}
retry:^FBLPromise<NSArray * /*[keyID, attestArtifact]*/> *_Nullable {
return [self attestKeyGenerateIfNeededWithID:keyIDForAttempt];
}]

The Promises documentation on retry might help clarify the mechanics.

@jostster There shouldn't be any need to trigger a reset yourself (handled by the SDK).

@jostster
Copy link
Contributor

jostster commented Nov 2, 2023

Thanks so much, we will undo our temp fix and implement this update. While it will be hard to replicate as we have already reset our key we will keep an eye on it for our end users and file a ticket should there be an edge case to this resolution. Thanks for the quick fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants