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

Support CSRF Request Header #34555

Merged
merged 1 commit into from
Aug 21, 2023
Merged

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Jul 5, 2023

Fixes #34513.

This PR:

  • Allows passing CSRF tokens as headers
  • Updates the documentation
  • Adds tests

Also CC @stuartwdouglas

@sberyozkin
Copy link
Member Author

Hey Andy @ia3andy, if you can give this PR a try against the real HTMX setup then it would be very appreciated, thanks

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

🙈 The PR is closed and the preview is expired.

* Token header which will provide a CSRF token.
*/
@ConfigItem
public Optional<String> tokenHeaderName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why making this optional? How is it different from formFieldName?

I would be nice if we could avoid having to set a config to make this work?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ia3andy CSRF feature offers a double cookie submit pattern for form payloads by default, if HTML uses form tags. If this new property is non optional, then, what does it mean for such forms, that it is ok not to provide a form parameter token or that both header and form parameter have to be provided ? Neither of these options is good for the forms, and the filter does not know how a given form has been submitted

Copy link
Member Author

@sberyozkin sberyozkin Jul 5, 2023

Choose a reason for hiding this comment

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

Right now, if the header is configured, and if it is present and the header check passed, then the form parameter is not checked, and if it is not present then the form parameter must be present but it is not a default behavior. If we make it a default then we tell, whatever works for the client, form param or header, will do. But the feature must stay in control and decide, via config, how it wants the token be presented

Copy link
Member Author

Choose a reason for hiding this comment

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

Andy, can Renarde recorder set a headet property for this feature if you prefer it be a default?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sberyozkin would it be a problem to assume that one OR the other (form param or header) should be present and contain a valid value to make the request valid?

Copy link
Member Author

Choose a reason for hiding this comment

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

@FroMage

Yes, I assumed that a CSRF token would be validated either as a form element or a header, whichever one is present. It's fine to have config to disable one or the other, but by default, they should both be allowed, I think.

IMHO, the server should stay in control, as opposed to giving 2 options to the clients at the same time, it is getting ambiguos for HTML <form> case, what exactly is a problem with configuring the header, are we talking about some total zero-config setup ?

Copy link
Member

Choose a reason for hiding this comment

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

Most clients are going to mix regular forms and ajax calls, so by default, both should work the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @FroMage

Copy link
Member Author

@sberyozkin sberyozkin Jul 6, 2023

Choose a reason for hiding this comment

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

So, if it is impossible to add headers to HTML form submits, then there is no ambiguity, looks like it is either Ajax or form for a single request. I'd like to avoid the case where we get HTML form submit but instead of the hidden form field value making it into the form payload we get a header which introduces an ambiguity - but looks like it can't ever happen, so perhaps we can indeed make the header config non-optional, thanks @ia3andy @FroMage

Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to add the header with the token inside for html form submit through htmx.

FroMage
FroMage previously requested changes Jul 6, 2023
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#custom-request-headers says that any value for the header is acceptable, and we don't need the token.

I'm not sure what to make of this. I guess the rationale is that only same-origin scripts will be allowed to set any custom header, and so it doesn't matter if we have a token?

Also, https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#verifying-origin-with-standard-headers says we should validate origins in our filter. Why do we not do that?


* sent as `HTTPOnly` cookie to the client, and
`Double Submit Cookie` techninque requires that the CSRF token is never directly exposed to scripts executed on the client-side. CSRF token is:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what it means "never directly exposed to scripts executed on the client-side" because if it's in a form element, then scripts can access it. What matters is that it's not exposed to scripts with a different origin than the endpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

@FroMage Sure, that needs to be clarified, the CSRF cookie that is HttpOnly

docs/src/main/asciidoc/security-csrf-prevention.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/security-csrf-prevention.adoc Outdated Show resolved Hide resolved
@sberyozkin
Copy link
Member Author

sberyozkin commented Jul 6, 2023

@FroMage

https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#custom-request-headers says that any value for the header is acceptable, and we don't need the token.
I'm not sure what to make of this. I guess the rationale is that only same-origin scripts will be allowed to set any custom header, and so it doesn't matter if we have a token?

We don't do that pattern, we support a double submit pattern, cookie, and the token - in the hidden form or request header, user does not have to do anything extra.

Also, https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#verifying-origin-with-standard-headers says we should validate origins in our filter. Why do we not do that?

Sorry, typed the wrong text earlier...
CSRF filter can't duplicate what CORS filter does - CORS filter has to be used to control Origins/etc

@FroMage
Copy link
Member

FroMage commented Jul 6, 2023

We don't do that pattern, we support a double submit pattern, cookie, and the token - in the hidden form or request header, user does not have to do anything extra.

I'm not sure what you mean by user having to do something.

My point is that OWASP says any value as an extra header is sufficient against CSRF attacks, and the value of the header doesn't have to be a token, it can be 1 for example, or true. All is needed is proof that the request originated from a valid origin, which is guaranteed by the browser model in order to set http request headers.

@sberyozkin
Copy link
Member Author

@FroMage Hi Steph,

I'm not sure what you mean by user having to do something.
My point is that OWASP says any value as an extra header is sufficient against CSRF attacks, and the value of the header doesn't have to be a token, it can be 1 for example, or true. All is needed is proof that the request originated from a valid origin, which is guaranteed by the browser model in order to set http request headers.

Sure, that pattern (which I'm not sure is used by anyone, DJango, Angular links shared by Andy, yourself, work with the token) is mentioned in that OWASP cheatsheet as an alternative to a double submit cookie pattern (cookie plus a token in a form field or request header) - but Quarkus implements double submit cookie pattern, supporting a request header in this PR works as part of this pattern.

As I said earlier, if someone wants to use that header with some random value pattern, without cookies, tokens, fine, but they don't need Quarkus support for it :-), just check the header is present in the JAX-RS code with @NotNull or something like that...

@FroMage
Copy link
Member

FroMage commented Jul 6, 2023

As I said earlier, if someone wants to use that header with some random value pattern, without cookies, tokens, fine, but they don't need Quarkus support for it :-), just check the header is present in the JAX-RS code with @NotNull or something like that...

Well that's not a very helpful thing to tell users, who will have to either implement their own filter, or duplicate this logic everywhere. In general, if it's useful for most users, we can implement it, we don't tell users to do it themselves.

The version where the header is required but doesn't need a token makes it super easy for users, because they don't have to obtain the token, and we don't have to make the cookie non-HttpOnly.

What I'm unsure of, is how it compares to the version with a token value. If they provide exactly equal security, I don't see the point in requiring (or checking) a token header value.

@sberyozkin
Copy link
Member Author

@FroMage

Well that's not a very helpful thing to tell users, who will have to either implement their own filter, or duplicate this logic everywhere. In general, if it's useful for most users, we can implement it, we don't tell users to do it themselves.

It is a single boolean check. Nothing else.

The version where the header is required but doesn't need a token makes it super easy for users, because they don't have to obtain the token, and we don't have to make the cookie non-HttpOnly.

I'm not sure what exactly you are proposing, scrap the current implementation which implements the double cookie submit pattern ?

What I'm unsure of, is how it compares to the version with a token value. If they provide exactly equal security,

What you can be certain about is that the double cookie submit pattern is not less secure.

I don't see the point in requiring (or checking) a token header value.

As I said CSRF feature is about implementing a double cookie submit pattern - cookie must have a token, possibly signed, and the raw token has to be posted as either a form field or request header.

If you'd like to have a filter for only checking some header's presence - then it should be a new extension.

@sberyozkin
Copy link
Member Author

sberyozkin commented Jul 6, 2023

I'm not sure why we are talking about this header-only pattern, given that both yourself and Andy linked to popular frameworks which use the token in the header.

@sberyozkin
Copy link
Member Author

@FroMage

Or, we can introduce, an enum like Pattern, DOUBLE_COOKIE_SUBMIT - default, and if HEADER_ONLY is set then it is only a boolean check for the request header, but IMHO it can't happen in this PR, double submit cookie pattern works on its own and this PR is about ensuring this pattern works not only with the form field but also with the header.

Next, once we have a very clear understanding of

If they provide exactly equal security,

then we can follow up and make it an option for the users.

IMHO, it would be a reasonable way forward, does it work for you ?

@sberyozkin sberyozkin force-pushed the csrf_check_header branch from c743f4c to c2a83d2 Compare July 6, 2023 17:45
@sberyozkin
Copy link
Member Author

sberyozkin commented Jul 6, 2023

@ia3andy @FroMage Have a look please at the latest update.

Steph, as I said, if you are really keen to support a header only, I can do it, the simpler/lighter the better, you are right, but IMHO we should not try to combine multiple patterns in one PR here. I can add a header only option as a pattern enum value after we analyze deeper what it really means.

We also have CORS support , there is a lot of content about CORS vs CSRF, my understanding with the proper CORS support, Quarkus will be protected against someone reusing a session cookie from the 3rd party script.

@sberyozkin sberyozkin marked this pull request as ready for review July 12, 2023 11:13
@sberyozkin
Copy link
Member Author

sberyozkin commented Jul 12, 2023

@ia3andy @FroMage I propose this PR go ahead - IMHO it is important not to dilute what CSRF feature does - it implements the double submit cookie pattern - form field or header having to matth the cookie - are 2 variations of this pattern.

Lighter version, header only pattern can be considered later though I'm not sure it will be necessary - it would likely be much cheaper for users do a simple header check themselves than having to bring a CSRF feature dependency, with it double submit cookie code support, for it to check the presence of the header only

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

Thanks @sberyozkin

@sberyozkin sberyozkin dismissed FroMage’s stale review August 16, 2023 10:13

Steph, I think your key comments have been addressed, we can explore the idea of header-only solution, without enforcing a double-cookie submit pattern, later for sure, please investigate further as well, and we can add something related

@sberyozkin
Copy link
Member Author

@FroMage Steph, I'm going to keep the PR open for another couple of days in case you'd like to add something, thanks

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 16, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@sberyozkin sberyozkin merged commit 2eb2076 into quarkusio:main Aug 21, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Aug 21, 2023
@sberyozkin sberyozkin deleted the csrf_check_header branch August 21, 2023 10:51
@quarkus-bot quarkus-bot bot added this to the 3.4 - main milestone Aug 21, 2023
* Token header which will provide a CSRF token.
*/
@ConfigItem
public String tokenHeaderName;
Copy link
Member

Choose a reason for hiding this comment

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

This should have a default value. Merely importing the CSRF extension now makes my build fail :(

@FroMage
Copy link
Member

FroMage commented Aug 24, 2023

Sorry for the late review, but this is what made the Renarde extension break on master.

@FroMage
Copy link
Member

FroMage commented Aug 24, 2023

@FroMage
Copy link
Member

FroMage commented Aug 24, 2023

I'll open a bug.

@sberyozkin
Copy link
Member Author

Sorry @FroMage, fixing now

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

Successfully merging this pull request may close these issues.

CSRF Reactive detect CSRF token from header "X-CSRF-Token" (currently only form param)
3 participants