App Check: App Attest provider #8133

maksymmalyhin · 2021-05-21T16:50:12Z

support of Apple's App Attest attestation provider.

* 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 FIRAppAttestKeyIDStorage * Add FIRAppAttestKeyIDStorageTests * Review [Draft] * Style * Docs updates * Docs updates 2 * Review [Draft] 2 * Improve tests * Improve test readability * Improve test readability 2

* 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

* Update comments * FIRAppAttestArtifactStorage implementation and tests * Fix init * API docs * Clean up storage in tests * Comments * Disable Keychain dependent tests for SPM

* 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

* 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

* calculatre sha256 of random challenge for attestation * Test app adjustments * cleanup * use trailing closures in the test app

…main

* Implement assertion exchange * Tweak existing tests * Add tests * Rename JSON to better match gRPC message * Add HTTPBody helper * Review * Review 2 * Review 3

…main

…e-ios-sdk into appcheck-appattest-main

* App Attest assertion workflow draft * send request * assertion flow tests * style

…main

* Update artifact storage API and tests * Artifact storage implementation update * Save artifact for a key ID * Style * typos

* 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

…main

ncooke3

LGTM I've tagged any TODO's that were commented in case we'd like to resolve them/set up tasks to track them. No further action needed to merge this in– just want to make sure we don't forget anything!

ncooke3 · 2021-05-21T18:41:15Z

FirebaseAppCheck/Sources/AppAttestProvider/API/FIRAppAttestAPIService.m

+// TODO: Verify the following request fields.
+static NSString *const kRequestFieldArtifact = @"artifact";
+static NSString *const kRequestFieldAssertion = @"assertion";
+static NSString *const kRequestFieldAttestation = @"attestation_statement";
+static NSString *const kRequestFieldChallenge = @"challenge";
+static NSString *const kRequestFieldKeyID = @"key_id";


Just touching base, is this still a valid TODO?

Actually no, it looks like the backend accepts both variants.

ncooke3 · 2021-05-21T18:49:44Z

FirebaseAppCheck/Sources/AppAttestProvider/API/FIRAppAttestAPIService.h

+- (FBLPromise<FIRAppAttestAttestationResponse *> *)attestKeyWithAttestation:(NSData *)attestation
+                                                                      keyID:(NSString *)keyID
+                                                                  challenge:(NSData *)challenge;
+
+/// Exchanges attestation data (artifact & assertion) and a challenge for a FAC token.
+- (FBLPromise<FIRAppCheckToken *> *)getAppCheckTokenWithArtifact:(NSData *)artifact


Following this conversation we had, I had changed appCheckTokenWith to getAppCheckTokenWith. Reevaluating now, are you content with the style of the APIs here? I wasn't sure if its preferred to do nothing, prefix attestKeyWithAttestatation with get, or revert getAppCheckTokenWith to appCheckTokenWith

This is obviously pretty minor, but I had remembered it and wanted to bring up in case you now feel more strongly about one way or another.

ncooke3 · 2021-05-21T18:52:11Z

FirebaseAppCheck/Sources/AppAttestProvider/API/FIRAppAttestAPIService.m

+- (NSString *)base64StringWithData:(NSData *)data {
+  // TODO: Need to encode in base64URL?
+  return [data base64EncodedStringWithOptions:0];
+}


Do we have an answer now?

Addressed in #8170.

ncooke3 · 2021-05-21T18:54:25Z

FirebaseAppCheck/Sources/AppAttestProvider/FIRAppAttestProvider.m

+
+                return [self attestKey:keyID challenge:challenge];
+              })
+      // TODO: Handle a possible key rejection - generate another key.


Should a task be created to track this?

Addressed in #8170.

ncooke3 · 2021-05-21T18:57:32Z

FirebaseAppCheck/Tests/Unit/Core/FIRAppCheckTests.m

-                    // internal one.
-                    XCTAssertEqualObjects(result.error, providerError);
-                  }];
+                               // TODO: Expect a public domain error to be returned - not the


Is this TODO still relevant, should a task be added to track this?

It is still relevant. Currently we don't have public API for this method. The TODO is expected to be addressed when we introduce the public API.

maksymmalyhin · 2021-05-21T19:06:27Z

@ncooke3 Thank you for the review and spotting the unattended TODOs. I'll add another PR into appcheck-appattest-main with the cleanup.

paulb777

LGTM after umbrella header is updated.

paulb777 · 2021-05-21T19:18:48Z

FirebaseAppCheck/Apps/FIRAppCheckTestApp/Podfile

@@ -11,7 +11,5 @@ target 'FIRAppCheckTestApp' do
  pod 'FirebaseAppCheck', :path => '../../../'

  pod 'FirebaseCore', :path => '../../../'


Not for this PR, but there should be a build test in CI to keep this unstale.

paulb777 · 2021-05-21T19:21:45Z

FirebaseAppCheck/Sources/Public/FirebaseAppCheck/FIRAppAttestProvider.h

@@ -0,0 +1,36 @@
+/*


Please add to the umbrella header.

* 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

ncooke3

LGTM on green

maksymmalyhin and others added 21 commits April 28, 2021 16:54

Implement app attest key ID storage (#8014)

869f87c

* Implement FIRAppAttestKeyIDStorage * Add FIRAppAttestKeyIDStorageTests * Review [Draft] * Style * Docs updates * Docs updates 2 * Review [Draft] 2 * Improve tests * Improve test readability * Improve test readability 2

Merge branch 'master' into appcheck-appattest-main

f2b77f0

FIRAppAttestArtifactStorage implementation and tests (#8041)

ccac49c

* Update comments * FIRAppAttestArtifactStorage implementation and tests * Fix init * API docs * Clean up storage in tests * Comments * Disable Keychain dependent tests for SPM

Merge branch 'master' into appcheck-appattest-main

e7f1ff8

Define Exchange AppAttest Assertion for FAC token API (#8058)

ed07e4b

Merge branch 'master' into appcheck-appattest-main

67b5c3a

Merge branch 'master' into appcheck-appattest-main

6742e33

App Check App Attest initial handshake adjustments (#8067)

a35d7bd

* calculatre sha256 of random challenge for attestation * Test app adjustments * cleanup * use trailing closures in the test app

Merge remote-tracking branch 'origin/master' into appcheck-appattest-…

2b16a5f

…main

Implement API for ExchangeAppAttestAssertionRequest endpoint (#8065)

c2bb06b

* Implement assertion exchange * Tweak existing tests * Add tests * Rename JSON to better match gRPC message * Add HTTPBody helper * Review * Review 2 * Review 3

Merge remote-tracking branch 'origin/master' into appcheck-appattest-…

cd53ea8

…main

Merge branch 'appcheck-appattest-main' of github.com:firebase/firebas…

e04a789

…e-ios-sdk into appcheck-appattest-main

App Check App Attest assertion flow (#8083)

ac17a23

* App Attest assertion workflow draft * send request * assertion flow tests * style

Merge remote-tracking branch 'origin/master' into appcheck-appattest-…

b9d638e

…main

App Check: store App Attest artifact per key ID (#8097)

cea2f68

* Update artifact storage API and tests * Artifact storage implementation update * Save artifact for a key ID * Style * typos

Merge remote-tracking branch 'origin/master' into appcheck-appattest-…

f134778

…main

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

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

Changelog

2102187

maksymmalyhin requested review from ncooke3 and paulb777 May 21, 2021 18:30

ncooke3 approved these changes May 21, 2021

View reviewed changes

paulb777 approved these changes May 21, 2021

View reviewed changes

Merge branch 'master' into appcheck-appattest-main

c0650aa

maksymmalyhin requested review from ryanwilson and ncooke3 June 1, 2021 13:30

Add FIRAppAttestProvider.h the umbrella header

76486af

google-oss-bot added the public-api-change label Jun 1, 2021

ncooke3 approved these changes Jun 1, 2021

View reviewed changes

maksymmalyhin merged commit 302f91e into master Jun 1, 2021

maksymmalyhin deleted the appcheck-appattest-main branch June 1, 2021 14:47

paulb777 mentioned this pull request Jun 3, 2021

Nightly Testing Report #8175

Closed

ncooke3 mentioned this pull request Jun 11, 2021

App Check: optimize auto-refresh #8232

Merged

firebase locked and limited conversation to collaborators Jul 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App Check: App Attest provider #8133

App Check: App Attest provider #8133

maksymmalyhin commented May 21, 2021

ncooke3 left a comment

ncooke3 May 21, 2021

maksymmalyhin May 21, 2021

ncooke3 May 21, 2021

ncooke3 May 21, 2021

ncooke3 May 21, 2021

maksymmalyhin Jun 1, 2021

ncooke3 May 21, 2021

maksymmalyhin Jun 1, 2021

ncooke3 May 21, 2021

maksymmalyhin Jun 1, 2021

maksymmalyhin commented May 21, 2021

paulb777 left a comment

paulb777 May 21, 2021

paulb777 May 21, 2021

ncooke3 left a comment

		@@ -11,7 +11,5 @@ target 'FIRAppCheckTestApp' do
		pod 'FirebaseAppCheck', :path => '../../../'

		pod 'FirebaseCore', :path => '../../../'

App Check: App Attest provider #8133

App Check: App Attest provider #8133

Conversation

maksymmalyhin commented May 21, 2021

ncooke3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksymmalyhin commented May 21, 2021

paulb777 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncooke3 left a comment

Choose a reason for hiding this comment