Add CrossSite Request Forgery prevention filter

[sberyozkin]

Fixes #8399.

This PR is based on the ideas shown by @kekbur in the code shared at #8399 but with some modifications:

• It is a RESTEasy Reactive implementation of a preMatching (implicitly non-blocking) server request filter
• RoutingContext's way of checking the form fields is used and the InputStream is recreated
• Token verification in the filter is optional - documentation shows how to easily compare the values in the application code when dealing with the large payloads
• the filter is configurable (various cookie properties, form field name, etc).

However I've not managed to resolve 2 problems yet which might be related:

• I could not make CsrfReactiveConfig runtime init - the test is failing if it is runtime init saying that CsrfReactiveConfig has not been initialized at the time of CsrfRequestResponseReactiveFilter's construction ( CsrfReactiveConfig is injected into the CsrfRequestResponseReactiveFilter constructor). I tried initializing the filter it at the runtime init time - by injecting an empty recorder which did not really help...
• I could not create an internal non-static SecureRandom field in CsrfRequestResponseReactiveFilter - the native test buid is failing in this case - looks like RESTEasy Reactive filters are kept in a static field in RESTEasy Reactive? So a new SecureRandom is created directly in the request filter method implementation right now - not a big problem as the token creation should not happen often but I wonder if it can be optimized

@geoand @mkouba Can you please review and advise on resolving these 2 issues ?

[mkouba]

I could not make CsrfReactiveConfig runtime init - the test is failing if it is runtime init saying that CsrfReactiveConfig has not been initialized at the time of CsrfRequestResponseReactiveFilter's construction ( CsrfReactiveConfig is injected into the CsrfRequestResponseReactiveFilter constructor). I tried initializing the filter it at the runtime init time - by injecting an empty recorder which did not really help...

@sberyozkin @geoand Hm, when exactly is the CsrfRequestResponseReactiveFilter created?

[sberyozkin]

@mkouba I'm adding it here

[mkouba]

@mkouba I'm adding it here

Yes, but that's not enough information - this merely adds the bean but does not say when the filter is created.

[sberyozkin]

@mkouba I see, sure, Georgios, can you clarify please when RESTEasy Reactive does it ?

[sberyozkin]

Yeah, I've just realized the same is the case with another reactive filter config: https://github.com/quarkusio/quarkus/blob/main/extensions/oidc-client-reactive-filter/runtime/src/main/java/io/quarkus/oidc/client/reactive/filter/runtime/OidcClientReactiveFilterConfig.java

[geoand]

@sberyozkin have you tried using field injection?

[sberyozkin]

@geoand I haven't as injecting all the properties is not great (8 properties are there right now but can grow)

[geoand]

I mean injecting the config object (like you are doing now), but instead of using constructor injection, using field injection

[sberyozkin]

Oops :-), sure, I'll try shortly

[sberyozkin]

@geoand, @mkouba, Injecting it as a field makes no difference, here is a stack trace:

Caused by: java.lang.RuntimeException: Error injecting io.quarkus.csrf.reactive.runtime.CsrfReactiveConfig io.quarkus.csrf.reactive.runtime.CsrfRequestResponseReactiveFilter.config
	at io.quarkus.csrf.reactive.runtime.CsrfRequestResponseReactiveFilter_Bean.create(Unknown Source)
	at io.quarkus.csrf.reactive.runtime.CsrfRequestResponseReactiveFilter_Bean.create(Unknown Source)
	at io.quarkus.arc.impl.AbstractSharedContext.createInstanceHandle(AbstractSharedContext.java:111)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:35)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:32)
	at io.quarkus.arc.impl.LazyValue.get(LazyValue.java:26)
	at io.quarkus.arc.impl.ComputingCache.computeIfAbsent(ComputingCache.java:69)
	at io.quarkus.arc.impl.AbstractSharedContext.get(AbstractSharedContext.java:32)
	at io.quarkus.csrf.reactive.runtime.CsrfRequestResponseReactiveFilter_Bean.get(Unknown Source)
	at io.quarkus.csrf.reactive.runtime.CsrfRequestResponseReactiveFilter_Bean.get(Unknown Source)
	at io.quarkus.csrf.reactive.runtime.CsrfRequestResponseReactiveFilter$GeneratedServerResponseFilter$filter_Bean.create(Unknown Source)
	at io.quarkus.csrf.reactive.runtime.CsrfRequestResponseReactiveFilter$GeneratedServerResponseFilter$filter_Bean.create(Unknown Source)
	at io.quarkus.arc.impl.AbstractSharedContext.createInstanceHandle(AbstractSharedContext.java:111)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:35)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:32)
	at io.quarkus.arc.impl.LazyValue.get(LazyValue.java:26)
	at io.quarkus.arc.impl.ComputingCache.computeIfAbsent(ComputingCache.java:69)
	at io.quarkus.arc.impl.AbstractSharedContext.get(AbstractSharedContext.java:32)
	at io.quarkus.csrf.reactive.runtime.CsrfRequestResponseReactiveFilter$GeneratedServerResponseFilter$filter_Bean.get(Unknown Source)
	at io.quarkus.csrf.reactive.runtime.CsrfRequestResponseReactiveFilter$GeneratedServerResponseFilter$filter_Bean.get(Unknown Source)
	at io.quarkus.arc.impl.ArcContainerImpl.beanInstanceHandle(ArcContainerImpl.java:468)
	at io.quarkus.arc.impl.ArcContainerImpl.beanInstanceHandle(ArcContainerImpl.java:481)
	at io.quarkus.arc.impl.ArcContainerImpl$1.get(ArcContainerImpl.java:285)
	at io.quarkus.arc.impl.ArcContainerImpl$1.get(ArcContainerImpl.java:282)
	at io.quarkus.arc.runtime.BeanContainerImpl$1.create(BeanContainerImpl.java:36)
	at io.quarkus.resteasy.reactive.common.runtime.ArcBeanFactory.createInstance(ArcBeanFactory.java:27)
	at org.jboss.resteasy.reactive.server.core.startup.RuntimeInterceptorDeployment.createInterceptorInstances(RuntimeInterceptorDeployment.java:153)
	at org.jboss.resteasy.reactive.server.core.startup.RuntimeInterceptorDeployment.<init>(RuntimeInterceptorDeployment.java:65)
	at org.jboss.resteasy.reactive.server.core.startup.RuntimeDeploymentManager.deploy(RuntimeDeploymentManager.java:95)
	at io.quarkus.resteasy.reactive.server.runtime.ResteasyReactiveRecorder.createDeployment(ResteasyReactiveRecorder.java:174)
	at io.quarkus.deployment.steps.ResteasyReactiveProcessor$setupDeployment1557341086.deploy_0(Unknown Source)
	at io.quarkus.deployment.steps.ResteasyReactiveProcessor$setupDeployment1557341086.deploy(Unknown Source)
	... 50 more
Caused by: javax.enterprise.inject.CreationException: Config root [io.quarkus.csrf.reactive.runtime.CsrfReactiveConfig] with config phase [RUN_TIME] not initialized yet.
	at io.quarkus.csrf.reactive.runtime.CsrfReactiveConfig_ce03c2a4e7c8cf63d1ce84a2009a32d0498e3425_Synthetic_Bean.create(Unknown Source)
	at io.quarkus.csrf.reactive.runtime.CsrfReactiveConfig_ce03c2a4e7c8cf63d1ce84a2009a32d0498e3425_Synthetic_Bean.get(Unknown Source)
	at io.quarkus.csrf.reactive.runtime.CsrfReactiveConfig_ce03c2a4e7c8cf63d1ce84a2009a32d0498e3425_Synthetic_Bean.get(Unknown Source)
	at io.quarkus.arc.impl.CurrentInjectionPointProvider.get(CurrentInjectionPointProvider.java:60)
	... 82 more

It looks like the reactive filters can not have a Runtime Init config at this stage which is not a PR blocker, and it should be looked at in a follow enhancement request, do you agree ?

What about the 2nd problem (re SecureRandom, see the description) ?

[geoand]

SecureRandom should indeed not be a static field, this causes problems in native (which can be overcome however if necessary)

[sberyozkin]

@geoand So if you look at the description of the 2nd problem - it does not work if I set as an instance variable.

I think I'll go ahead and open 2 issues as I believe there is nothing that can be done at this PR level to address either of the issues.

Can you please review the reactive code etc ?

[geoand]

Can you please review the reactive code etc ?

Sorry, I have too many bugs to look into currently, so this will have to wait.

[geoand]

To be honest, I don't think the implementation of CsrfRequestResponseReactiveFilter will work properly. What you likely need in this case is to update the handler chain with a custom ServerRestHandler.

[sberyozkin]

@geoand I can ask someone else to review if you are busy, np.

To be honest, I don't think the implementation of CsrfRequestResponseReactiveFilter will work properly. What you likely need in this case is to update the handler chain with a custom ServerRestHandler.

I have the tests passing and I do not understand your comment - the implementation seems sound to me. I'm OK with reworking it as ServerRestHandler - but is it really necessary, should I do if the users spot some actual issues later ?

[geoand]

Nevermind my comment about ServerRestHandler, what you are doing can work, however I strongly suggest the filters be rewritten to return Uni<Void> so you can avoid pausing the resuming the execution of the handler chain manually.

[sberyozkin]

@geoand I've added Uni<Void> and it really looks nice and neat, thanks, and I think we should doc more how to do it - I can give it a try a bit later as supporting asynchronous filters is a great feature.
I've also progressed with supporting RUN_TIME config - right now it is not cool, the way it is done (I need to keep a ConfigGroup so that quarkus.* namespace is recognized) and duplicate the same in the filter constructor - I can live with it for sure as the users won't notice, the important thing the RUN_TIME config is supported. So I'm happy enough.

But I've also just seen @mkouba's suggestion - so I can give it a quick try

thanks

[sberyozkin]

I've squashed the last commit so that if Martin's suggestion works I can keep it as a separate commit to review

[sberyozkin]

Now I'm really happy with the way I can handle the runtime config in the filter :-), @geoand does it look OK to you ?

[geoand]

I'll have a look tomorrow

[geoand]

I think it would be best to squash the commits

[sberyozkin]

Sure

[sberyozkin]

Though I'd like to add one more config option, for the CSRF cookie optionally not to be created per every new GET request - there is an assumption there an HTML form will be returned for every one but in real applications it won't be the case, will do all tomorrow, thanks

[sberyozkin]

I've just noticed a perf improvement suggestion from @mkouba as well, the config should be accessed via a local var, will take care of it as well

[geoand]

LGTM

[sberyozkin]

I updated the code to use a local variable to access the config instance, and added 2 more config properties, one allowing to avoid creating the cookies for all the requests (this one is also checked in the test), and one allowing to pass through non-url-form-encoded POSTs as a complex application may have all sort of interactions with the outside world, and POST JSONs should not be really blocked if a resource method exists, in the latter case the CORS filter + XHTMLRequest may be handling the CSRF risks separately...

I think it is probably all for now, it can be tricky to implement such a feature covering all the variations from the very start, but we'll have a good enough base...

[quarkus-bot]

Failing Jobs - Building 618b8d5

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
⌛	JVM Tests - JDK 11 Windows	Build	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 17
✔️	JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/smallrye-graphql/deployment 
! Skipped: extensions/smallrye-graphql-client/deployment integration-tests/hibernate-orm-graphql-panache integration-tests/smallrye-graphql and 1 more

📦 extensions/smallrye-graphql/deployment

✖ io.quarkus.smallrye.graphql.deployment.RequestLeakDetectionTest.testWithConcurrentCalls line 63 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.smallrye.graphql.deployment.RequestLeakDetectionTest that uses java.util.List, java.util.Listint was not fulfilled within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

[sberyozkin]

Looks like a window build has just died, have been waiting for it for all day to finish :-)

[sberyozkin]

I'll just go ahead and merge to give it a couple of days to settle, as I'm off tomorrow as well :-)

[FroMage]

Wait a minute: this should also support multipart forms! #28379