App Check App Attest: handle attestation rejection #8170

maksymmalyhin · 2021-05-31T16:34:14Z

reset stored attestation data on attestation rejection from Firebase backend
outdated TODOs removed
kFIRLoggerAppCheckMessageCodeUnknown replaced by specific unique codes in log messages

google-oss-bot · 2021-05-31T16:35:06Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

ryanwilson

A couple small questions but otherwise LGTM, thanks!

ryanwilson · 2021-05-31T16:56:17Z

FirebaseAppCheck/Sources/AppAttestProvider/Errors/FIRAppAttestRejectionError.h

+
+@interface FIRAppAttestRejectionError : NSError
+
+- (instancetype)init;


Any reason why this is using a different error strategy than Core? https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseAppCheck/Sources/Core/Errors/FIRAppCheckErrorUtil.h

The Core strategy seems to make more sense to me (a static method to generate an NSError object instead of a subclass of NSError)

The App Attest provider is already pretty big (close to be bigger than the rest of App Check SDK), so it seems it will make sense to separate it in the future. This error is specific for App Attest, so having it independent from Core will make separation easier in the future. Also FIRAppCheckErrorUtil was intended to be used mostly for errors without specific classes.

SGTM, thanks

ryanwilson · 2021-05-31T17:01:40Z

FirebaseAppCheck/Sources/Core/Errors/FIRAppCheckHTTPError.m

+                      userInfo:userInfo];
+  if (self) {
+    _HTTPResponse = HTTPResponse;
+    _data = data;


Should we be using the userInfo property of the error and providing a key for data instead of having a subclass here? Or is there something I'm missing for why this should be its own class?

Working with specific NSError subclasses is so much easier in the client code because we need to check only the class to identify the type of the error and the data associated with it. In contrast we would need to check the error domain, the keys and the value types in the user info dict if we use NSError. Does it look reasonable to you?

I think that sounds reasonable, thanks!

ryanwilson · 2021-05-31T17:06:35Z

FirebaseAppCheck/Tests/Unit/AppAttestProvider/FIRAppAttestProviderTests.m

@@ -797,6 +923,48 @@ - (void)assertGetToken_WhenKeyRegistered_Success {
  [self verifyAllMocks];
 }

+- (void)expectAppAttestAvailabilityToBeCheckedAndStoredKeyRequested {


It wasn't immediately obvious to me that this would be setting up a rejection as part of the expectation - is there a way we can encode that in the (already long) name?

ryanwilson · 2021-05-31T19:26:45Z

FirebaseAppCheck/Sources/Core/FIRAppCheckLogger.m

+void FIRAppCheckDebugLog(NSString *message, ...) {
+  va_list args_ptr;
+  va_start(args_ptr, message);
+  FIRLogBasic(FIRLoggerLevelDebug, kFIRLoggerAppCheck, kFIRLoggerAppCheckMessageCodeUnknown,


Nit: If this is landing any time soon, I'd prefer that we pass in the code number inside the function and then pass in Unknown everywhere so it's easier to refactor at a later date. Just want to make sure we don't accidentally ship multiple logs with the same Unknown error code.

Actually, I intentionally left the default message code here. I don't remember when I actually used the code last time, so it looks like it has very limited usefulness. I would prefer to keep track of the codes only for logs with higher level and use the default one for debug. WDYT?

The codes are more for external usage - if a developer runs into a log statement they can then just search for that code to either find the code itself or other posts referencing that error message, for example:

https://www.google.com/search?q=%22I-COR000003%22

That way if we need to wordsmith the error message at all we can do so and not lose any historical context.

Ah, I didn't think of this particular use case. I'll add codes. Though it still looks much more relevant for non-debug logs, it may be useful for debug as well.

updated codes here and in other log messages.

ryanwilson

Thanks for updating all the logs!

* App Attest provider: attestation sequence (#7971) * App Attest provider: attestation sequence (#761) * App Attest draft WIP * FIRAppAttestProvider initializers * ./scripts/style.sh * FIRAppAttestProvider implementation draft * Basic FIRAppAttestProviderTests and fixes * style * testGetTokenWhenAppAttestIsNotSupported * More FIRAppAttestProviderTests * Cleanup * Remove unused file * Availability annotations on DCAppAttestService category. * Guard FIRAppAttestProvider with #if TARGET_OS_IOS * Formatting * Fix SPM * app_check.yaml: Add diagnostics SPM builds * fix yaml * Fix Firebase-Package scheme bad merge * Fix typo * FIRAppAttestProvider: hide default init * FIRAppAttestKeyIDStorage: methods placeholders * Comments * Fix updated block definition * Implement app attest key ID storage (#8014) * Implement FIRAppAttestKeyIDStorage * Add FIRAppAttestKeyIDStorageTests * Review [Draft] * Style * Docs updates * Docs updates 2 * Review [Draft] 2 * Improve tests * Improve test readability * Improve test readability 2 * App Check App Attest workflow updates: initial handshake (#8032) * Handshake adjustments (WIP) * Introduce FIRAppAttestProviderState * WIP: calculate attestation state * WIP: calculate attestation state 2 * formatting * Comments and moving code around * Fix init in tests * Fix state calculation flow * Cleanup state calculation and fix tests. * Cleanup and fixes. * Comments * formatting * Fix import * Typo fixes and additional comments * FIRAppAttestInitialHandshakeResponse API * Cleanup state calculation using FBLPromiseAwait * Cleanup * style * FIRAppAttestArtifactStorage implementation and tests (#8041) * Update comments * FIRAppAttestArtifactStorage implementation and tests * Fix init * API docs * Clean up storage in tests * Comments * Disable Keychain dependent tests for SPM * Implement App Attest `getRandomChallenge` (#8033) * Initial implementation * Parse response body for challenge and stub test cases * Review [Draft] * Avoid encoding challenge again * Add tests * Revert "Avoid encoding challenge again" and add TODO This reverts commit 69eb00d. * Document tests; Add test * Tests: Add URL validation check * Review * Define Exchange AppAttest Assertion for FAC token API (#8058) * App Check App Attest: attestation request (#8059) * App Attest provider API integration WIP * update tests * Draft attestation response parsing * Attestation request draft * style * AppAttest Attestation API tests draft * Error cases tests * style * Cleanup and API docs * Merge fix * Fix OCMock imports * Fix nullability modifier * Formatting * comments * App Check App Attest initial handshake adjustments (#8067) * calculatre sha256 of random challenge for attestation * Test app adjustments * cleanup * use trailing closures in the test app * Implement API for ExchangeAppAttestAssertionRequest endpoint (#8065) * Implement assertion exchange * Tweak existing tests * Add tests * Rename JSON to better match gRPC message * Add HTTPBody helper * Review * Review 2 * Review 3 * App Check App Attest assertion flow (#8083) * App Attest assertion workflow draft * send request * assertion flow tests * style * App Check: store App Attest artifact per key ID (#8097) * Update artifact storage API and tests * Artifact storage implementation update * Save artifact for a key ID * Style * typos * App Check: prevent concurrent token requests (#8117) * App Attest multiple get token method invocation tests * Ensure a single App Attest handshake sequence at the time * FIRAppCheckTests: get token request merging tests * FIRAppCheck: Ensure a single get token operation at the time * formatting * Test new request after merged requests * Release finished operation promise * Style * Typos * typo * Request merging tests for error cases * formatting * Changelog * App Check App Attest: handle attestation rejection (#8170) * Remove/update outdated TODOs * [WIP] Attestation rejection handling draft * style * retry tests draft * reset key ID before retry * Reset attestation * test error and fixes * style * More details in the name * Some debug logging * style * Use specific codes for log messages * style * Add FIRAppAttestProvider.h the umbrella header Co-authored-by: Nick Cooke <[email protected]>

* App Attest provider: attestation sequence (#7971) * App Attest provider: attestation sequence (#761) * App Attest draft WIP * FIRAppAttestProvider initializers * ./scripts/style.sh * FIRAppAttestProvider implementation draft * Basic FIRAppAttestProviderTests and fixes * style * testGetTokenWhenAppAttestIsNotSupported * More FIRAppAttestProviderTests * Cleanup * Remove unused file * Availability annotations on DCAppAttestService category. * Guard FIRAppAttestProvider with #if TARGET_OS_IOS * Formatting * Fix SPM * app_check.yaml: Add diagnostics SPM builds * fix yaml * Fix Firebase-Package scheme bad merge * Fix typo * FIRAppAttestProvider: hide default init * FIRAppAttestKeyIDStorage: methods placeholders * Comments * Fix updated block definition * Implement app attest key ID storage (#8014) * Implement FIRAppAttestKeyIDStorage * Add FIRAppAttestKeyIDStorageTests * Review [Draft] * Style * Docs updates * Docs updates 2 * Review [Draft] 2 * Improve tests * Improve test readability * Improve test readability 2 * App Check App Attest workflow updates: initial handshake (#8032) * Handshake adjustments (WIP) * Introduce FIRAppAttestProviderState * WIP: calculate attestation state * WIP: calculate attestation state 2 * formatting * Comments and moving code around * Fix init in tests * Fix state calculation flow * Cleanup state calculation and fix tests. * Cleanup and fixes. * Comments * formatting * Fix import * Typo fixes and additional comments * FIRAppAttestInitialHandshakeResponse API * Cleanup state calculation using FBLPromiseAwait * Cleanup * style * FIRAppAttestArtifactStorage implementation and tests (#8041) * Update comments * FIRAppAttestArtifactStorage implementation and tests * Fix init * API docs * Clean up storage in tests * Comments * Disable Keychain dependent tests for SPM * Implement App Attest `getRandomChallenge` (#8033) * Initial implementation * Parse response body for challenge and stub test cases * Review [Draft] * Avoid encoding challenge again * Add tests * Revert "Avoid encoding challenge again" and add TODO This reverts commit 69eb00d. * Document tests; Add test * Tests: Add URL validation check * Review * Define Exchange AppAttest Assertion for FAC token API (#8058) * App Check App Attest: attestation request (#8059) * App Attest provider API integration WIP * update tests * Draft attestation response parsing * Attestation request draft * style * AppAttest Attestation API tests draft * Error cases tests * style * Cleanup and API docs * Merge fix * Fix OCMock imports * Fix nullability modifier * Formatting * comments * App Check App Attest initial handshake adjustments (#8067) * calculatre sha256 of random challenge for attestation * Test app adjustments * cleanup * use trailing closures in the test app * Implement API for ExchangeAppAttestAssertionRequest endpoint (#8065) * Implement assertion exchange * Tweak existing tests * Add tests * Rename JSON to better match gRPC message * Add HTTPBody helper * Review * Review 2 * Review 3 * App Check App Attest assertion flow (#8083) * App Attest assertion workflow draft * send request * assertion flow tests * style * App Check: store App Attest artifact per key ID (#8097) * Update artifact storage API and tests * Artifact storage implementation update * Save artifact for a key ID * Style * typos * App Check: prevent concurrent token requests (#8117) * App Attest multiple get token method invocation tests * Ensure a single App Attest handshake sequence at the time * FIRAppCheckTests: get token request merging tests * FIRAppCheck: Ensure a single get token operation at the time * formatting * Test new request after merged requests * Release finished operation promise * Style * Typos * typo * Request merging tests for error cases * formatting * Changelog * App Check App Attest: handle attestation rejection (#8170) * Remove/update outdated TODOs * [WIP] Attestation rejection handling draft * style * retry tests draft * reset key ID before retry * Reset attestation * test error and fixes * style * More details in the name * Some debug logging * style * Use specific codes for log messages * style * Add FIRAppAttestProvider.h the umbrella header * Add receivedAtDate property to the FAC token * Update tests to check receivedAtDate field where important * [WIP] Use FIRAppCheckTokenRefreshResult instead in the refresher API * [WIP] Fix refgresher usage * Update tests with new API, keep old logic * Update tests with new logic * WIP * WIP * Initial refresh tests and fixes. * Add #import <TargetConditionals.h> where needed. * Formatting * Cleanup and test updates * style * Comments * Cleanup and comments * Fix catalyst * Changelog * Changelog version fix * Typo * Cleanup * Typo * Imports order Co-authored-by: Nick Cooke <[email protected]>

maksymmalyhin added 8 commits May 25, 2021 10:36

Remove/update outdated TODOs

c1b27ba

[WIP] Attestation rejection handling draft

2a14d38

style

df7e630

retry tests draft

dc2870c

reset key ID before retry

3066420

Reset attestation

7bde35c

test error and fixes

ede1a34

style

fafcaf4

maksymmalyhin requested a review from ryanwilson May 31, 2021 16:34

google-cla bot added the cla: yes label May 31, 2021

google-oss-bot added the api: appcheck label May 31, 2021

ryanwilson reviewed May 31, 2021

View reviewed changes

More details in the name

5bf5f06

ryanwilson approved these changes May 31, 2021

View reviewed changes

maksymmalyhin added 2 commits May 31, 2021 15:16

Some debug logging

f92a0ec

style

11edcac

maksymmalyhin marked this pull request as ready for review May 31, 2021 19:22

maksymmalyhin requested a review from ryanwilson May 31, 2021 19:23

ryanwilson reviewed May 31, 2021

View reviewed changes

ryanwilson approved these changes May 31, 2021

View reviewed changes

maksymmalyhin added 2 commits May 31, 2021 17:19

Use specific codes for log messages

5d808b7

style

3e46328

maksymmalyhin requested a review from ryanwilson May 31, 2021 21:22

ryanwilson approved these changes Jun 1, 2021

View reviewed changes

maksymmalyhin merged commit b8f80f3 into appcheck-appattest-main Jun 1, 2021

maksymmalyhin deleted the mm/appatest-cleanup branch June 1, 2021 13:27

maksymmalyhin mentioned this pull request Jun 1, 2021

App Check: App Attest provider #8133

Merged

firebase locked and limited conversation to collaborators Jul 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

App Check App Attest: handle attestation rejection #8170

App Check App Attest: handle attestation rejection #8170

Uh oh!

maksymmalyhin commented May 31, 2021 •

edited

Loading

Uh oh!

google-oss-bot commented May 31, 2021

Uh oh!

ryanwilson left a comment

Uh oh!

ryanwilson May 31, 2021

Uh oh!

maksymmalyhin May 31, 2021

Uh oh!

ryanwilson May 31, 2021

Uh oh!

ryanwilson May 31, 2021

Uh oh!

maksymmalyhin May 31, 2021

Uh oh!

ryanwilson May 31, 2021

Uh oh!

ryanwilson May 31, 2021

Uh oh!

ryanwilson May 31, 2021

Uh oh!

maksymmalyhin May 31, 2021 •

edited

Loading

Uh oh!

ryanwilson May 31, 2021

Uh oh!

maksymmalyhin May 31, 2021

Uh oh!

maksymmalyhin May 31, 2021

Uh oh!

ryanwilson left a comment

Uh oh!

Uh oh!


		@interface FIRAppAttestRejectionError : NSError

		- (instancetype)init;

App Check App Attest: handle attestation rejection #8170

App Check App Attest: handle attestation rejection #8170

Uh oh!

Conversation

maksymmalyhin commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented May 31, 2021

Uh oh!

ryanwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maksymmalyhin May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maksymmalyhin commented May 31, 2021 •

edited

Loading

maksymmalyhin May 31, 2021 •

edited

Loading