feat(appcheck): Add App Check token verification #484

bamnet · 2022-02-06T05:27:04Z

Add an appcheck module which provides the ability to verify tokens, closing #483.

Returns a token if valid and error otherwise, similar to the implementation in Node SDK. PTAL and let me know what you think!

Sample usage:

client, err := app.AppCheck(context.Background())
if err != nil {
	log.Fatalf("error getting AppCheck client: %v\n", err)
}

token := "..."
_, err := client.VerifyToken(token)
if err != nil {
	log.Fatalf("error verifying token: %v\n", err)
	return
}

RELEASE NOTE: Added a new app_check.verify_token() API to verify App Check tokens.

Passes very basic tests in a sample app, but needs more work.

The auth token doesn't represent the app check token, particularly the aud[].

All keys are borrowed from the Firebase Admin Node SDK.

Coverage is now ~87% which is pretty good I think. The remaining code is difficult to test because it overlaps with functionality built in to the JWT package.

bamnet · 2022-02-09T02:04:24Z

Thanks for kicking off those workflows! I fixed up the lint and go1.16 errors.

The go1.12 error is tricky, it looks like I'll need to rewrite the JWKS library, the general purpose one requires crypto/ed25519 which wasn't introduced until 1.13 [ref].

Alternatively, we could bump go.mod from 1.11 => 1.13, but I'm not sure how critical go1.11/1.12 is to this library (perhaps an artifact of supporting all GAE standard platforms).

MicahParks · 2022-02-09T15:06:31Z

@bamnet I'd be happy to make a Go 1.12 compatible branch or fork of github.com/MicahParks/keyfunc, if you'd like.

However, there would be at least a couple of drawbacks:

The github.com/golang-jwt/jwt project could not be used. Neither the /v3 nor /v4 module support Go 1.12.
- github.com/form3tech-oss/jwt-go could be used, but that's archived.
The exported error types wouldn't be available for unwrapping. (Since that was introduced in 1.13.)

bamnet · 2022-02-09T15:54:02Z

Thanks for the offer @MicahParks! I am slowly resigning to the fact that the best solution here is likely to roll our own mini-JWT/JWKS implementation here. After locally patching keyfunc I realized the bigger golang-jwt problem dependency problem and, if we're doing out own JWTs, it's not that much work to add some very specific JWKS support.

There's some code I can likely extract from the existing auth package provided here, which already rolls it's own very specific JWT code.

lahirumaramba · 2022-02-09T18:23:48Z

Hi @bamnet Thank you so much for your contribution! This is on my radar and I plan to review the code this week. Thank you for fixing the lint errors.

Re: Go 1.12 compatibility: We prefer to stay aligned with the Google Cloud Go clients and it seems like the minimum requirement is still Go 1.11. Let me discuss this with the team and get back to you.

In the meantime, I found this auth0 go module that seems to support jwks. I didn't check to see if it works on 1.11 though.

Writing our own key-fetcher also sounds like a reasonable approach... please refer to the current implementation in auth/token_verifier.go to see if we can reuse/refactor components from there.

We need to get the public API approved (an internal process) before merging this change. I will get that process started.

Thank you for your patience!

lahirumaramba · 2022-03-04T22:34:40Z

@bamnet Thank you for your patience! We have decided to update the minimum Go version to 1.15. See #485.
Once that is merged, we should be able to continue with this PR.

bamnet · 2022-03-05T05:27:27Z

Woohoo, thanks for the update! I'll stay tuned for the go version bump.

lahirumaramba · 2022-03-07T20:21:26Z

Woohoo, thanks for the update! I'll stay tuned for the go version bump.

@bamnet changes are now in the dev branch. I have updated the CI workflows as part of the version bump.
Please rebase this PR with the dev branch and that will trigger the CI tests again. Thank you!

bamnet · 2022-03-09T05:20:14Z

Done, PTAL / kick off workflows!

This got lost in the merge.

bamnet · 2022-04-11T03:29:21Z

FYI - I've re-based with the latest dev branch to pull in the latest releases.

lahirumaramba · 2022-04-11T16:57:17Z

Thank you @bamnet! The next step is to complete the internal review process for the new public API. I will start the process later this week. Please note that this might take a few weeks to complete. Once the API is approved, we can merge this PR. I will track the progress here. Thanks again for your patience.

bamnet · 2022-06-14T02:52:05Z

Hi @lahirumaramba, I just wanted to check in and see if there's anything I can do to move the API process forward? Happy to help fill out any documentation, etc.

wesselvanderlinden · 2022-08-04T12:39:12Z

What is the status of this PR? Can this be merged? We really need this, can I help in any way?

wesselvanderlinden · 2022-10-06T15:01:05Z

@lahirumaramba any progress? :)

bamnet · 2022-10-21T03:59:17Z

FYI - I've rebased with the latest dev branch to pull in all the work from the past 6 months. PTAL.

lahirumaramba

Hey @bamnet Thank you for your contribution and thank you so much for your patience! We just completed the internal review process for this API.

The changes looks good just need a few things to address before we can merge this. I have left a few comments. Let me know if you have any questions. Thanks!

appcheck/appcheck.go

lahirumaramba · 2022-10-21T18:23:46Z

appcheck/appcheck.go

+// NewClient creates a new App Check client.
+func NewClient(ctx context.Context, conf *internal.AppCheckConfig) (*Client, error) {
+	// TODO: Add support for overriding the HTTP client using the App one.
+	jwks, err := keyfunc.Get(conf.JWKSUrl, keyfunc.Options{


Do you know if keyfunc supports overriding the http client?

Yes, keyfunc.Options accepts an http.Client. How should we expose that option to developers?

appcheck/appcheck.go

firebase.go

appcheck/appcheck.go

bamnet · 2022-10-21T20:47:29Z

Thanks for all the feedback. Incorporated ~all of it, LMK what I missed.

appcheck/appcheck.go

lahirumaramba · 2022-10-21T21:14:58Z

appcheck/appcheck.go

+
+// DecodedAppCheckToken represents a verified App Check token.
+//
+// DecodedAppCheckToken provides typed accessors to the common JWT fields such as Audience (aud) // and ExpiresAt (exp).


Looks like we are missing a new line after ... fields such as Audience (aud)

Done.

I also changed DecodedAppCheckToken to use ExpiresAt which matches this documentation and mirrors the IssuedAt instead of the Expires we had earlier. LMK if I should revert back to Expires and fix the docstring.

lahirumaramba · 2022-10-21T21:18:50Z

Hi @kevinthecheung could we get a review on the reference docs, please? Thank you!

appcheck/appcheck.go

Co-authored-by: Kevin Cheung <kevinthecheung@users.noreply.github.com>

bamnet · 2022-10-26T18:07:01Z

Thanks for the review @kevinthecheung!

@lahirumaramba back over to you?

lahirumaramba · 2022-10-26T19:12:00Z

Thank you, @bamnet !
We will include this feature in the next release of Admin Go SDK.

bamnet added 5 commits February 3, 2022 23:10

Initial appcheck.verify implementation.

350ea01

Passes very basic tests in a sample app, but needs more work.

Return an actual token.

ce26936

The auth token doesn't represent the app check token, particularly the aud[].

Cleanup errors, comments, and more!

1ec1d36

Initial set of app check unit tests.

95152c9

All keys are borrowed from the Firebase Admin Node SDK.

Add and reorganize a few more tests.

2d8b282

Coverage is now ~87% which is pretty good I think. The remaining code is difficult to test because it overlaps with functionality built in to the JWT package.

bamnet changed the title ~~feat: Add App Check token verification (#483)~~ feat: Add App Check token verification Feb 6, 2022

lahirumaramba self-requested a review February 8, 2022 19:32

lahirumaramba self-assigned this Feb 8, 2022

bamnet added 2 commits February 8, 2022 20:08

Fix lint errors by adding more comments.

556e159

Fix missing go-cmp module.

a7f816a

Fix updated variable name.

1e355b7

Merge remote-tracking branch 'upstream/dev' into dev

ff376a1

bamnet added 3 commits March 24, 2022 20:31

Merge remote-tracking branch 'upstream/dev' into dev

d07a8fd

Fix go-cmp dependency.

0562523

This got lost in the merge.

Merge remote-tracking branch 'upstream/dev' into dev

2b44507

Merge remote-tracking branch 'upstream/dev' into dev

99d7f81

lahirumaramba requested changes Oct 21, 2022

View reviewed changes

lahirumaramba assigned bamnet and unassigned lahirumaramba Oct 21, 2022

bamnet added 5 commits October 21, 2022 15:40

Doc and formatting feedback from @ lahirumaramba.

607af6d

ioutil is deprecated as of Go 1.16.

05f10c6

Cache keys for 6 hours.

690adc7

Switch to DecodedAppCheckToken.

1ac4a71

Remove JWKSUrl config option.

4e87e6e

lahirumaramba reviewed Oct 21, 2022

View reviewed changes

appcheck/appcheck.go Show resolved Hide resolved

lahirumaramba reviewed Oct 21, 2022

View reviewed changes

appcheck/appcheck.go Outdated Show resolved Hide resolved

lahirumaramba approved these changes Oct 21, 2022

View reviewed changes

lahirumaramba requested a review from kevinthecheung October 21, 2022 21:19

lahirumaramba assigned kevinthecheung Oct 21, 2022

Small doc & naming tweaks.

6f0f304

kevinthecheung approved these changes Oct 26, 2022

View reviewed changes

appcheck/appcheck.go Outdated Show resolved Hide resolved

Tweak capitalization of verifies

82bdca3

Co-authored-by: Kevin Cheung <kevinthecheung@users.noreply.github.com>

lahirumaramba merged commit 606008d into firebase:dev Oct 26, 2022

lahirumaramba added the release-note label Oct 26, 2022

lahirumaramba changed the title ~~feat: Add App Check token verification~~ feat(appcheck): Add App Check token verification Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appcheck): Add App Check token verification #484

feat(appcheck): Add App Check token verification #484

feat(appcheck): Add App Check token verification #484

feat(appcheck): Add App Check token verification #484

Conversation

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