firebase / firebase-admin-go

Firebase Admin Go SDK
Apache License 2.0
1.12k stars 239 forks source link

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

Closed bamnet closed 1 year ago

bamnet commented 2 years ago

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.

bamnet commented 2 years ago

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 commented 2 years ago

@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:

bamnet commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

lahirumaramba commented 2 years ago

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 commented 2 years ago

Done, PTAL / kick off workflows!

bamnet commented 2 years ago

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

lahirumaramba commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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

wesselvanderlinden commented 1 year ago

@lahirumaramba any progress? :)

bamnet commented 1 year ago

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

bamnet commented 1 year ago

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

lahirumaramba commented 1 year ago

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

bamnet commented 1 year ago

Thanks for the review @kevinthecheung!

@lahirumaramba back over to you?

lahirumaramba commented 1 year ago

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