feat(appcheck): Add App Check token verification support with replay protection - #1233
feat(appcheck): Add App Check token verification support with replay protection#1233yvonnep165 wants to merge 10 commits into
Conversation
…eckTokenOptions classes
…n and one time token verification
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Firebase App Check service, including token verification capabilities, option and response models, custom exceptions, and comprehensive unit tests. The review feedback highlights two critical issues in AppCheckTokenVerifier.java: a potential NullPointerException when checking the audience claim if it is missing from the JWT, and a resource leak due to the HttpResponse not being closed after executing the backend verification request.
…quest execute in a try-finally block
weixifan
left a comment
weixifan
left a comment
There was a problem hiding this comment.
Thank you for this PR! Some minor comments.
| private final JsonFactory jsonFactory; | ||
| private volatile DefaultJWTProcessor<SecurityContext> jwtProcessor; | ||
|
|
||
| public AppCheckTokenVerifier(FirebaseApp app) { |
There was a problem hiding this comment.
In the Admin SDK, do we use Guice or Dagger to manage the dependency injection of parameters? If so, a singleton-scoped JWK parameter can be injected and managed by Guice or Dagger instead.
| } | ||
|
|
||
| protected JWKSource<SecurityContext> createKeySource() throws MalformedURLException { | ||
| return JWKSourceBuilder.create(URI.create(JWKS_URL).toURL()).retrying(true).build(); |
There was a problem hiding this comment.
Can we please cache this for 6 hours, using .cache(...)? The refresh timeout can be left at default (15 seconds).
|
|
||
| private DefaultJWTProcessor<SecurityContext> getJwtProcessor() { | ||
| DefaultJWTProcessor<SecurityContext> processor = this.jwtProcessor; | ||
| if (processor == null) { |
There was a problem hiding this comment.
Is there a reason this initialization logic can't be put into the constructor? Doing so avoids the subtle and error-prone synchronization logic. For example, the necessity of the double null-check is not immediately obvious.
| ErrorCode.INVALID_ARGUMENT, "App Check token has no 'iss' (issuer) claim."); | ||
| } | ||
|
|
||
| if (!issuer.startsWith(APP_CHECK_ISSUER)) { |
There was a problem hiding this comment.
I think in Admin SDK, we don't have access to the developer's project number. This is actually a deficiency, as https://google.aip.dev/cloud/2510 states that the project number is the canonical identifier, and there are issues in working with project IDs in general.
In the future, if we ever address this issue and we finally get access to the project number, it's actually better to check for strict equality to https://firebaseappcheck.googleapis.com/<project_number> here -- for example, https://firebaseappcheck.googleapis.com/12345678.
Perhaps a comment explaining this would be appropriate here.
| } catch (BadJOSEException e) { | ||
| throw new FirebaseAppCheckException( | ||
| ErrorCode.INVALID_ARGUMENT, | ||
| "Check your project: " + projectId + ". Firebase App Check token is invalid: " |
There was a problem hiding this comment.
I think this exception type is the one corresponding to an invalid signature, while the JOSEException is thrown when an internal error happens.
| */ | ||
| public DecodedAppCheckToken(Map<String, Object> claims) { | ||
| checkNotNull(claims, "Claims map must not be null"); | ||
| checkArgument(claims.containsKey("sub"), "Claims map must contain sub"); |
There was a problem hiding this comment.
We should probably also check the other required claims iss, aud, exp, and iat.
| /** | ||
| * Returns the expiration time in seconds since the Unix epoch. | ||
| */ | ||
| public long getExpirationTime() { |
There was a problem hiding this comment.
I think java.time.Instant is a better candidate to hold instants in time, since we have access to Java 8.
| /** | ||
| * Returns the issued-at time in seconds since the Unix epoch. | ||
| */ | ||
| public long getIssuedAt() { |
| /** | ||
| * Represents a verified Firebase App Check token. | ||
| */ | ||
| public class DecodedAppCheckToken { |
There was a problem hiding this comment.
If this class is part of the public API, please also consider exposing provider and the optional claim jti.
| * @return This builder. | ||
| */ | ||
| public Builder setConsume(Optional<Boolean> consume) { | ||
| this.consume = consume != null ? consume : Optional.<Boolean>empty(); |
There was a problem hiding this comment.
In my opinion, allowing nulls into an optional parameter re-exposes the exact same problem that Optional<> was intended to solve. I think it's better for us to check for nullness and throw in this case.
2 participants
This PR adds support for App Check standard token verification and one-time token verification for replay protection. This enables backend servers to verify standard App Check JWTs locally and optionally verify/consume limited-use (replay-protected) tokens via backend RPC.
FirebaseAppCheckservice entry point with synchronous and asynchronousverifyTokenmethods.DecodedAppCheckToken,VerifyAppCheckTokenResponse,VerifyAppCheckTokenOptions,FirebaseAppCheckException) for token verification results and options.