Skip to content

App Check App Attest: attestation request #8059

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
May 12, 2021
Merged

App Check App Attest: attestation request #8059

merged 16 commits into from
May 12, 2021

Conversation

@maksymmalyhin
Copy link
Contributor

@maksymmalyhin maksymmalyhin commented May 11, 2021 •
edited
Loading

  • App Attest attestation request (initial handshake) implementation and tests
  • handshake sequence and tests adjusted

b/187424671

#no-changelog

@google-cla google-cla bot added the cla: yes label May 11, 2021
ncooke3
ncooke3 approved these changes May 11, 2021
View reviewed changes
Copy link
Member

@ncooke3 ncooke3 left a comment

Choose a reason for hiding this comment

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

LGTM, nice!

FirebaseAppCheck/Sources/AppAttestProvider/API/FIRAppAttestAPIService.h
/// @param challenge The challenge used to generate the attestation.
/// @return A promise that is fulfilled with a response object with an encrypted attestation
/// artifact and an Firebase App Check token or rejected with an error.
- (FBLPromise<FIRAppAttestAttestationResponse *> *)attestKeyWithAttestation:(NSData *)attestation
Copy link
Member

@ncooke3 ncooke3 May 11, 2021

Choose a reason for hiding this comment

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

WDYT about attestArtifact... instead of attestKey... for the API name?

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin May 12, 2021

Choose a reason for hiding this comment

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

It looks like the attestKey... name a bit better represent the purpose of the request (which is attesting the App Attest key generated on the device). It looks aligned with the method getRandomChallenge where we also use a verb to explain what the method does. From this perspective the method appCheckTokenWithArtifact could be renamed to something like getAppCheckTokenWith... or sendAppCheckExchangeRequestWith.... I'm not too strong about it and happy to discuss options. WDYT?

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin May 12, 2021

Choose a reason for hiding this comment

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

I'll probably merge the PR for now and update the method name in the following PRs if needed.

Copy link
Member

@ncooke3 ncooke3 May 12, 2021

Choose a reason for hiding this comment

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

That's a good point. 👍 I think getAppCheckTokenWith might hint more to the fact that there is a network request involved rather than just retrieving the FAC quickly from some local storage.

FirebaseAppCheck/Sources/AppAttestProvider/API/FIRAppAttestAPIService.h Outdated
Comment on lines 55 to 56
/// @param APIService An instance implementing `FIRAppCheckAPIServiceProtocol` to be used to send
/// network for Firebase App Check backend requests.
Copy link
Member

@ncooke3 ncooke3 May 11, 2021

Choose a reason for hiding this comment

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

Consider rewording to something like: ... to be used to send network requests to Firebase App Check backend.

@maksymmalyhin maksymmalyhin merged commit 786f613 into appcheck-appattest-main May 12, 2021
@maksymmalyhin maksymmalyhin deleted the mm/appcheck-initial-handshake3 branch May 12, 2021 13:34
@ncooke3 ncooke3 mentioned this pull request May 21, 2021
Merged
maksymmalyhin added a commit that referenced this pull request Jun 1, 2021
@maksymmalyhin @ncooke3
App Check: App Attest provider (#8133)
302f91e 
* 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 <36927374+ncooke3@users.noreply.github.com>
maksymmalyhin added a commit that referenced this pull request Jun 11, 2021
@maksymmalyhin @ncooke3
App Check: optimize auto-refresh (#8232)
2ec4dbd 
* 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 <36927374+ncooke3@users.noreply.github.com>
@firebase firebase locked and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ncooke3 ncooke3 ncooke3 approved these changes

@paulb777 paulb777 paulb777 approved these changes

@ryanwilson ryanwilson Awaiting requested review from ryanwilson

Assignees

No one assigned

Labels

api: appcheck cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maksymmalyhin @paulb777 @ncooke3 @google-oss-bot