Secret rotation does not work without overwrite
Closed this issue · 14 comments
I noticed that all requests get rejected after the secret is rotated, as the hash no longer matches. The only way around this is to set the overwrite-flag in generateToken. This however breaks our app when used with multiple taps.
Does it make sense to throw an error during generateToken() if secret rotation is enabled? I would have expected a new token to be issued if the hash no longer matched, as this does not necessarily indicate that the token has been tempered.
Currently the only way to get around is to either set the overwrite flag, or have all users clear their cookies.
I'm using csrf-csrf 3.0.1 with express in the backend and angular as frontend.
Disclaimer: it's 1am for me right now.
This is a good point, other libraries typically handle this by allowing an array of secrets and trying to validate the request with each one. You could in theory implement this kind of approach yourself.
Associate a secret with some kind of id and also keep that id in the request session, or even as a separate httpOnly cookie, so that you can identify which secret was used for the incoming request.
In the array case, the identifier could just be an index, and you could eventually remove it after some period of time or some number of requests, as you start hashing with the more recent secret.
You would also need to track which is the main one, and your getSecret logic would have to accommodate getting the secret based on this identifier.
Perhaps a little tedious, but will need to put more thought on how to better accommodate this internally.
This suggestion may be problematic distinguishing the getSecret call for incoming/outgoing, and distinguishing whether to get a secret based on the req identifier, or the latest secret for the outgoing case.
Disclaimer taken ;-)
I get the logic around keeping a history of secrets and I think this should work.
But I believe this could be solved in a simpler way:
When the user explicitly calls generateToken (i.e. by calling the /csrf-endpoint) and we detect that the hash has changed, we could use the overwrite behavior: don't issue a new token every time, but only if the hash has changed.
Of course that's not what we want when validating the token - this is where the history-approach you mentioned would come in handy.
Right now we get 403-errors during every call to the backend (even during GET-requests) after the secret has been rotated, as the express-middleware detects the preexisting cookie, but fails to validate it. This seems unnecessary restrictive: I see no harm in just refreshing the cookie for non-mutating requests.
When the user explicitly calls generateToken (i.e. by calling the /csrf-endpoint) and we detect that the hash has changed, we could use the overwrite behavior: don't issue a new token every time, but only if the hash has changed.
Of course that's not what we want when validating the token - this is where the history-approach you mentioned would come in handy.
Right now we get 403-errors during every call to the backend (even during GET-requests) after the secret has been rotated, as the express-middleware detects the preexisting cookie, but fails to validate it. This seems unnecessary restrictive: I see no harm in just refreshing the cookie for non-mutating requests.
So you'd like the logic in generate token to be updated to have an implicit overwrite based on a differing hash of an already existing token? Could you not create a wrapper function around generateToken?
I think I'd be more in favor of having this logic in the validate really, for example: when the provided token, and the original token in the cookie match, but the hash doesn't, assign a new token and still respond with some kind of error. This would still result in some form of erroneous response where the frontend would have to make the request again after receiving the updated token. This is actually how JIra handles it if your XSRF token times out and it doesn't work, they prompt you with a popup to re-try the action as the failed request would have assigned a new token.
But ideally I'd like to try and avoid adding additional configurable functionality to the package to minimise bloat for those who don't need it, and to try and minimise the configuration. My original goal was to try and keep the library as minimal and as flexible as possible, but make it so that user implementation specific stuff could still work with it, i.e. wrapper functions / custom middleware.
Hi guys,
I'm thinking about use this middleware to implement in my application, but then I saw this issue, can you contribute in that logic you are talking for more o less having an idea on how implement it. I've been working with my NodeJS backend server and a front-end in React, maybe can you explain with more details on how implement the logic you talk about for try and test in the backend this implementation. In fact, you talk about the secrets rotation, but in what way can we know what secret was used, or maybe we need to call getSecret method before perform the hash ?. As I told before, I still not work with the middleware, and a I have a question, the hash is used for hashing the csrf token in the cookie, or in what way the hash was used ?.
I prefer to ask you before test the middleware and use in my applicaitons. Thanks in advance !
Hey @dcorralf if you wish to reply further, please raise your own issue or discussion - do not further respond to this thread regarding this topic, we should avoid hijacking threads. Currently this thread is discussing what possible implementation options are available to hopefully reach an ideal solution, if you just need help with using the middleware - please post separately or reach out on the Discord, thanks!
maybe can you explain with more details on how implement the logic you talk about for try and test in the backend this implementation.
The implementation is entirely up to you and is dependent on your code base and your particular use case, I can give you a generic example based on what I described, but this example might not work for you, because it may vary and depend on the rest of your setup.
getSecret: (req) => {
return secrets[req.session.secretIdentifier || currentCsrfIdentifier];
}
So in this above case, your secrets is some global backend state - or whatever else - it can be whatever you're tracking your secrets in.
When you generate a token you do something like this:
// When overwriting a token
const token = generateToken(req,, true);
req.session.secretIdentifier = currentSecretIdentifier
// When setting an initial token
const initialToken = typeof req.session.csrfToken === 'undefined';
const token = generateToken(req);
req.session.secretIdentifier = currentSecretIdentifier
And in this above example, currentSecretIdentifier is entirely up to your rotation implementation and how you're tracking your secret changes. The idea is when you rotate in a new secret, you create some identifier for it so you can map your incoming requests to the secret that was used.
Alternatively you could also just store the secret used in the backend session state:
req.session.csrfSecret = currentCsrfSecret // this assumes backend session state only - do NOT expose session state to frontend
// Then your getSecret would be like this:
getSecret: (req) => req.session.csrfSecret || currentCsrfSecret
In fact, you talk about the secrets rotation, but in what way can we know what secret was used,
As per my example described in my previous post, and a high-level example provided in the answer above in this comment. It's entirely up to you how you keep track of what secret was used, the example I give above is just one option.
or maybe we need to call getSecret method before perform the hash ?
Not sure why you think this, you shouldn't have to call getSecret yourself.
As I told before, I still not work with the middleware, and a I have a question, the hash is used for hashing the csrf token in the cookie, or in what way the hash was used ?.
No, the hash is not used to hash the token, this doesn't make sense. If you need the hash to get the hash, how do you get the hash in the first place? You should refer to the OWASP links in the README to read up and learn how CSRF protection works.
- A token is generated
- The secret is used to create a hash of the generated token
- The token and hash is stored in a httpOnly cookie
- The token is also sent to the frontend as part of a response payload
- The frontend makes a request with the token from step 4 included in such a way that the configured
getTokenFromRequestcan retrieve it. - The backend gets the token out of the request, and it gets the value out of the cookie. The cookie value is split into the original token and token hash values. The token sent in the request is compared to the token in the cookie. Then the token in the request is again hashed using the secret returned from
getSecretand this hash is compared to the hash from the cookie.
Again, please do not reply to this post on this thread - if you have further questions, please ask them on the discord, or in a different issue / discussion.
I get this error : ForbiddenError: invalid csrf token , when I am trying to use the initially genrated token when login as below , export const sendTokenResponse = async (res, req, user, message) => { const accessToken = generateJWTToken(user); // Generate a CSRF token const token = generateToken(req, res);
res.status(200).json({ data: { user, access_token: accessToken, token: token }, message, }); }; I am sotring this token value in frontend localstorage and trying to use that value in postman request header x-csrf-token , I was able to console.log the token value inside csrfErrorHandler middleware , just applied after doubleCsrfProtection middleware. but not inside this export const { invalidCsrfTokenError, generateToken, doubleCsrfProtection } = doubleCsrf({ getSecret: () => process.env.CSRF_SECRET, cookieName: process.env.CSRF_COOKIE_NAME, cookieOptions: { sameSite: "lax", secure: false, signed: true, httpOnly: true, maxAge: 3600, }, getTokenFromRequest: (req) => { console.log(req); return req.headers["x-csrf-token"]; }, });
please can someone help me??, (note: I am a beginner)
Can you open a new issue, please?
Hi @psibean! Looks good, thanks for the quick fix.
This does not quite solve my problem (my secrets are rotated in Kubernetes, which does not provide a history). But from the perspective of the library, this is the way to go.
I still need to figure out why the middleware is blocking other get-requests. As far as I understand this should only happen for mutating requests or calls to the csrf-endpoint. This might be an issue in my express-app, so let me rule that out first and then get back to you.
Hi @psibean! Looks good, thanks for the quick fix. This does not quite solve my problem (my secrets are rotated in Kubernetes, which does not provide a history). But from the perspective of the library, this is the way to go.
Right, but with the linked changes, we're no longer going to be throwing an error in the generateToken if you provide the validateOnGeneration flag as false. This means generateToken won't throw, and will instead just return a new token.
Does that not cover your needs?
This is in addition to the array option.
I still need to figure out why the middleware is blocking other get-requests. As far as I understand this should only happen for mutating requests or calls to the csrf-endpoint. This might be an issue in my express-app, so let me rule that out first and then get back to you.
Yeah, csrf-csrf has GET as an ignored method by default, so that shouldn't be happening
Right, but with the linked changes, we're no longer going to be throwing an error in the generateToken if you provide the validateOnGeneration flag as false. This means generateToken won't throw, and will instead just return a new token.
Nice - I missed that flag while scanning the code (shouldn't review before first coffee). This would be perfect for our use case.
Sorry for the late response! I took some time to test and indeed if the token changes, then all requests are rejected with 403 (even GET-Requests). This is especially problematic as the problem cannot be fixed by just reloading the page or fetching a new token - the user has to delete the cookie by hand to start over.
This happens due to this line, which is before the check for ignored methods:
https://github.com/Psifi-Solutions/csrf-csrf/blob/main/src/index.ts#L183
The good news is that your proposed changes should also fix this.
Good to know! I'll look at getting this in on a patch release some time soon. Hav been incredibly busy lately!
Hey @gtudan I've published 3.0.2 which includes the proposed solution / changes in a backwards compatible manner.
In the case where another major release is needed, we can consolidate some of the options as a single object parameter going forward.
Please advise if you encounter any issues with the getSecret: (req) => string | Array<string> setup and the validateOnReuse option now exposed via generateToken as a fourth parameter. I know it's a pain to do generateToken(req, res, false, false) or req.csrfToken(false, false) just to get that last param. But for now it's better than forcing a breaking change 😓
Hi @psibea, awesome, thanks for the fix!
Everything checked out locally, even with rotating secrets. I'll test it some more next week.