Skip to content
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.

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

feat(appcheck): Add initial classes for App Check #672

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lahirumaramba
Copy link
Member

  • Add verifyToken() API

} else if (!claimsSet.getIssuer().startsWith(APP_CHECK_ISSUER)) {
errorMessage = "invalid iss";
} else if (claimsSet.getSubject().isEmpty()) {
errorMessage = "invalid sub";
Copy link
Member Author

Choose a reason for hiding this comment

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

Please ignore the error messages for now, these need to be improved to add more detail.

+ "environment variable.");
AppCheckTokenVerifier appCheckTokenVerifier = AppCheckTokenVerifier.builder()
.setProjectId(projectId)
.build();
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can use a token factory to clean this up a bit more later. That way we can also configure the JWKS url, cache duration, etc.

INTERNAL,

/**
* User is not authenticated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm: this error is used for token minting using ExchangeCustomToken, right? Maybe we can be specific here and say "The service account credential is invalid."

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. We don't need this for verify token API. Let's remove it for now and add when we implement the createToken API later.

final class AppCheckTokenVerifier {

private final URL jwksUrl;
private final String projectId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor point: Is it possible to base this on project number (a long)? This will comply with https://google.aip.dev/cloud/2510 for third-parties only using project numbers.

Copy link
Member Author

Choose a reason for hiding this comment

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

So this is interesting. The Admin SDK uses the project ID (string) and does not have any context of the project number. We addressed this by converting aud to an array and adding both id and number in the same claim.

DecodedAppCheckToken verifyToken(String token) throws FirebaseAppCheckException {
SignedJWT signedJWT;
JWTClaimsSet claimsSet;
String scopedProjectId = String.format("projects/%s", projectId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: instead of scopedProjectId, we should use the term projectName (in the spirit of https://google.aip.dev/122).

errorMessage = String.format("The provided App Check token has incorrect 'aud' (audience) "
+ "claim. Expected %s but got %s. %s",
scopedProjectId, claimsSet.getAudience().toString(), projectIdMatchMessage);
} else if (!claimsSet.getIssuer().startsWith(APP_CHECK_ISSUER)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer strict .equals() here instead of .startsWith().

Copy link
Member Author

Choose a reason for hiding this comment

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

The issuer contains the App ID (which is not known to the Admin SDK). That's why we decided to do a startsWith here. Also see the equivalent of Node.js https://github.com/firebase/firebase-admin-node/blob/a32195daa9848b261fe892d9f606152a40ff2915/src/app-check/token-verifier.ts#L121

* A decoded and verified Firebase App Check token. See {@link FirebaseAppCheck#verifyToken(String)}
* for details on how to obtain an instance of this class.
*/
public final class DecodedAppCheckToken {
Copy link
Contributor

Choose a reason for hiding this comment

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

I almost wonder whether we should be exposing all the contents of the token or offer our own opinionated data layer. For example, we can offer just one method getAppId() for now like our Node.js (I think).

Copy link
Member Author

Choose a reason for hiding this comment

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

In go/fac-admin-verify-token we decided to wrap the full token in a response object with helper methods like getAppId(). So it will be something similar to:

public final class VerifyAppCheckTokenResponse {

  public String getAppId()

  public DecodedAppCheckToken getToken()
}

This is similar to the Node.js API: https://github.com/firebase/firebase-admin-node/blob/a32195daa9848b261fe892d9f606152a40ff2915/src/app-check/app-check-api.ts#L95

@bhackstock
Copy link

Is there an estimate for when this logic will be available in a release ❓🛳️

We are using a java-only backend and would like to verify app check tokens for our backend API calls!
If there is anything I can help with or contribute something myself, I'd be glad to 😊

(don't know if this the right place to post/ask about this, please advise if that's not the case 👀)

@lahirumaramba
Copy link
Member Author

Hey @bhackstock you at the right place! I can't promise you a timeline, but this is a feature that we currently have on our roadmap. Stay tuned! :)

}

/** Returns the Expiration Time for this token. */
public String getExpirationTime() {

Choose a reason for hiding this comment

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

IIRC we don't yet require Java 8+ for our SDKs, so we can't use the java.time API. But we could at least return an integer (and document it as seconds since the Unix epoch) instead of a string.

}

/** Returns the Issued At for this token. */
public String getIssuedAt() {
Copy link

@kaibolay kaibolay Oct 20, 2022

Choose a reason for hiding this comment

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

ditto. Ideally that'd return java.time.Instant, but barring that this should return an integer like getExpirationTime()

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. Change to Long here and above. Thanks!

@bard-kristian
Copy link

Is this being worked on?

@lahirumaramba lahirumaramba marked this pull request as ready for review February 15, 2023 18:40
@lahirumaramba lahirumaramba changed the title Add initial classes for App Check feat(appcheck): Add initial classes for App Check Feb 15, 2023
@tjarvstrand
Copy link

Any progress on this? It's kind of strange that AppCheck is not part of the SDK API after such a long time...

@alaegin
Copy link

alaegin commented Feb 27, 2024

Hi, is there any chance that it will be implemented?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants