awinogrodzki / next-firebase-auth-edge

Next.js Firebase Authentication for Edge and Node.js runtimes. Compatible with latest Next.js features.
https://next-firebase-auth-edge-docs.vercel.app/
MIT License
498 stars 43 forks source link

Best practice for Google public key rotation #231

Closed steve-marmalade closed 1 month ago

steve-marmalade commented 2 months ago

Hello @awinogrodzki !

Apologies for bringing up a topic that you have covered several times before (#203 , #151), but I would benefit from an even clearer recommendation from you.

Current situation

My middleware.ts is implemented as follows:

export const middleware = async (request: NextRequest) => {
  return authMiddleware(request, {
    loginPath: "/api/login",
    logoutPath: "/api/logout",
    ...authConfig,
    checkRevoked: false,
    handleValidToken: async (token, headers) =>
      handleValidToken(request, token, headers),
    handleInvalidToken: async () => handleInvalidToken(request),
    handleError: async (error) => {
      console.error("Unhandled authentication error", { error });
      return await handleInvalidToken(request);
    },
  });
};

I am seeing the following error, which means that the handleError is getting called:

Unhandled authentication error {
error: [Error: Invalid argument: idToken has "kid" claim which does not correspond to a known public key. Most likely the token is expired, so get a fresh token from your client app and try again.] {
code: 'INVALID_ARGUMENT'
}
}

Based on the above, the way I'm treating an error is to essentially treat the request as a logged-out request and proceed that way. This is fine, except I continue to see these error logs over and over. I assume they will persist until the user chooses to log back in.

The core of my question: is there something I should do in handleError to clear the user's cookies so that they are in a completely logged out state? You gave me some advice on logging out a user from within middleware here, but you mentioned we have to return a redirect and in this case I'd want to instead send the user to the page they requested as normal (just with their auth cookies removed).

Thank you!

awinogrodzki commented 1 month ago

Hey @steve-marmalade!

Great question!

I've added a paragraph about this in the article I published not so long ago

*handleError can be called with INVALID_ARGUMENT error after Google public keys are updated.

This is a form of key rotation and is expected. [See this Github issue for more info](https://github.com/awinogrodzki/next firebase-auth-edge/issues/151?ref=hackernoon.com#issuecomment-1952928802)

Back to your message:

I am seeing the following error, which means that the handleError is getting called:

Basically, handleError is called with INVALID_ARGUMENT that results from NO_MATCHING_KID JWT error, which happens when:

The reason I re-throw NO_MATCHING_KID as INVALID_ARGUMENT is simple: that is the behaviour I observe in official firebase-admin library.

I suppose I could move this error to handleInvalidToken and add specific reason with description and call stack.

That would allow you to use handleError without any type of filtering, and still enable to debug less common issues related to missing kid from inside handleInvalidToken, when needed

How does that sound?

awinogrodzki commented 1 month ago

The core of my question: is there something I should do in handleError to clear the user's cookies so that they are in a completely logged out state?

@steve-marmalade, I didn't cover the "clear user cookies" part in my previous answer:

There is a function you can use to clear user cookies, it's called removeCookies and can be imported from next-firebase-auth-edge/lib/next/cookies

Anyways, if we move this error to handleInvalidToken, I don't think you would need to remove the cookies anymore.

I will add removeCookies to the docs anyways, it can be useful in certain cases

steve-marmalade commented 1 month ago

Hey @awinogrodzki , thanks for the thoughtful response as always. From my perspective, moving this error to handleInvalidToken would be the most convenient!

awinogrodzki commented 1 month ago

Hey @steve-marmalade!

The new way of handling this error has been introduced in v1.7.0-canary.8

I've introduced new InvalidTokenReason.INVALID_KID, which indicates that the kid has changed likely due to Google certificate rotation.

from now on handleError should receive only unexpected issues

I've also updated the docs with removeCookies method, in case you still need to logout user explicitly in some cases

I will release new docs shortly.

tm-bookshop commented 1 month ago

@awinogrodzki I've had users impacted by this invalid kid issue caused by Google cert rotation and I was hoping you could answer a follow up question to the rest of this issue:

Inside the handleInvalidToken function, what are our options for handling the InvalidTokenReason.INVALID_KID error code? Is it possible to to call refreshCredentials at that point in time, or is the only option to sign our users out whenever the Google certificate is rotated (this doesn't seem correct, I don't regularly get logged out of any sites I frequent)?

awinogrodzki commented 1 month ago

Hey @tm-bookshop!

Inside the handleInvalidToken function, what are our options for handling the InvalidTokenReason.INVALID_KID error code?

The expected action should be to redirect user to login page to allow them to re-authenticate

Is it possible to to call refreshCredentials at that point in time, or is the only option to sign our users out whenever the Google certificate is rotated

It's not possible, at least not at the moment. Some time in the past I created experimental flag to allow "eternal" session by refreshing user token on expired KID, but got no feedback, so I decided to drop it for the time being

(this doesn't seem correct, I don't regularly get logged out of any sites I frequent)

The reason it's working this way is simple – it's the same behaviour as in official firebase-admin library. In other words, if you were to implement authentication using firebase-admin, you would experience the same behaviour

I can create an experimental flag, eg. experimental_enableTokenRefreshOnExpiredKid or something similar, that will allow you to have endless session, at least in theory.

I am not sure how token refresh will behave after kids are expired though, so I would need your help to test it and provide feedback. Are you willing to help with this?

awinogrodzki commented 1 month ago

Hello again @tm-bookshop,

In v1.7.0-canary.15 I've reintroduced experimental flag to refresh token after kid header has expired.

You should pass experimental_enableTokenRefreshOnExpiredKidHeader: true to authMiddleware and getTokens.

I tested this option extensively and it seems it works as expected.

In short: if you set experimental_enableTokenRefreshOnExpiredKidHeader to true in authMiddleware and getTokens, users should no longer face with INVALID_KID error and the session will go until cookie expires.

Please let me know if you face any issues

tm-bookshop commented 1 month ago

@awinogrodzki Thanks so much for the quick response, we are going to try this out as soon as possible!

perepechin commented 1 week ago

You should pass experimental_enableTokenRefreshOnExpiredKidHeader: true to authMiddleware and getTokens.

It works as expected for me as well. Thank you!

tm-bookshop commented 1 week ago

We also finally got confirmation of this refresh working as expected, thanks again!

awinogrodzki commented 1 week ago

That's great news @perepechin @tm-bookshop. Thank you for testing and getting back on the topic!

I will soon introduce this option without experimental_ prefix :-)