-
Notifications
You must be signed in to change notification settings - Fork 393
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
Wicket 6786: Add Fetch Metadata support #439
Conversation
...t-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
Outdated
Show resolved
Hide resolved
...t-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
Outdated
Show resolved
Hide resolved
...t-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
Outdated
Show resolved
Hide resolved
It would be nice to clean up the commits before merging! |
I'm on vacation right now, I'll be able to have a look at this pr at the end of July. |
… with CsrfPreventionRequestCycleListener
…st cycle listener. Sort static imports.
Thanks for the quick review Martin! I've sorted out the commits now and updated your comments. We'll hope Edmond will enjoy his holiday and wait for his review :) |
…bclasses. This is required by the WebSocketAwareCsrfPreventionRequestCycleListener
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good to me. The only problem I see is the API change, but that's easy to fix. Just make sure you do not change method signatures for public or protected methods.
...t-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks ok to me
That's great! Is there anything else we can do to have this PR merged? :) |
It would be great if another Wicket dev could have a look as well. Perhaps @martin-g or @svenmeier ? |
I'll take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it would be better to split CsrfPreventionRequestCycleListener into two (a list of?) 'RequestIsolationPolicies' (resource has a special meaning in Wicket, so I'd rather not have them named ResourceIsolationPolicies - this isn't a web standard terminology, is it?) :
- one for fetch-metadata
- one for the old referrer
So instead of the two requestListeners originally proposed in WICKET-6786, we have one listener with two policies instead.
This would prevent a lot of code duplication, checking the whitelist and handling conflicts would be done in CsrfPreventionRequestCycleListener once only.
Furthermore the whitelist is not checked against the "sec-fetch-site" header currently.
For this the policy would somehow have to provide the sourceUrl to the CsrfPreventionRequestCycleListener - maybe with an additional method.
I hope I was able to convey the solution I have in mind.
Hi Sven, Thanks for your review! While this moves away from the original proposal (see the conversation on WICKET-6786), I agree that having multiple Resource Isolation Policies is a better pattern. We'll push a commit soon! On the point of the whitelist not being checked against sec-fetch-site: that is by design, because Fetch Metadata header values are enums rather than origins, so the site in sec-fetch-site doesn't actually carry an origin but rather whether the request is same-origin, same-site or navigational. This is a little confusing, because Wicket implements a whitelist of origins and it's not always possible to get those origins, so this is a best-effort method. |
Thanks for clarifying the whitelist handling. I was confused because checkRequestFetchMetadata() checks the origin too. |
Hi @svenmeier! @eozmen410 has pointed out something that I missed before that brings us back to the reason why this is implemented the way it is and why we rejected the alternative you propose in our design:
Because of this, running both checks serially would produce contradictions. The new check would approve and the legacy would reject. That is: whenever the legacy check makes a decision correctly, both legacy and new will agree on the final decision, but in cases where the legacy check produces a false positive, both legacy and new will contradict each other! In the model you're proposing, if at least one check rejects then the request is fully rejected and so we would be back in the world of false positives that we're trying to get out of. In summary: while having multiple Resource Isolation Policies is a good idea for the future (for implementing specific policies for iframes, for instance), this PR should be seen as improving only one of those policies and not as adding an additional one. I hope that makes sense, but we would be happy to jump on a call or chat if you'd like to :) What do you think? (tagging @papegaaij for visibility and correctness, since he's very familiar with the legacy check) |
Yes, I agree. The old approach is only a fallback scenario. When a browser sends Fetch Metadata, that is much more reliable and the old strategy should not be used. It is likely that we will encounter some corner cases with this piece of code, but IMHO the current code is easy enough to adapt by our users if that were to happen. I'm happy with this PR. @svenmeier shall I merge? |
I'm all for this new feature, and the new implementation works fine as it seems. But the integration into the old CsrfPreventionRequestCycleListener is just hacky. The whole class is centered around the request origin, and it shows in the new method checkRequestFetchMetadata()
If this is supposed to be a hack to get this new protection into production ASAP, go ahead - probably no users will see this code anyway. But if we want to get this integrated nicely, I would suggest we create a new listener and deprecated the old one. |
…cy CsrfPreventionRequestCycleListener be a ResourceIsolationPolicy that can be used in combination with the DefaultResourceIsolationPolicy to add support for legacy browsers that don't send Sec-Fetch headers yet.
Hi @svenmeier , @papegaaij and @eozmen410 I think Sven is right in saying that the legacy CSRF protection makes many decisions based on the Origin that don't belong in Fetch Metadata. Since we all agree that the legacy protection is still useful to add support for legacy browsers, I've tried to find the right balance between the new security mechanism we want to provide and the legacy one in Wicket, without introducing breaking changes. I've pushed a new commit, here's a summary of the changes:
One possible improvement would be to extract the Origin-based protection entirely into a brand new OriginBasedResourceIsolationPolicy and run that by default too. That could potentially provide the best of both worlds. I hope this finds the right balance/compromises to keep Wicket users safe while introducing no breaking changes! WDYT? |
...t-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
Outdated
Show resolved
Hide resolved
...et-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
Outdated
Show resolved
Hide resolved
...et-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
Outdated
Show resolved
Hide resolved
...et-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
Outdated
Show resolved
Hide resolved
...et-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
Outdated
Show resolved
Hide resolved
...et-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
Outdated
Show resolved
Hide resolved
...et-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
Outdated
Show resolved
Hide resolved
...t-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
Outdated
Show resolved
Hide resolved
…acy browsers that don't send Sec-Fetch-* headers and add it as a default Resource Isolation Policy to the Fetch Metadata listener.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just few minor comments on the logging.
Otherwise it looks very good to me now!
Thank you, @salcho !
Thanks so much for your review @martin-g! @papegaaij I believe we're ready for merge now that the changes have been approved :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new RequestCycleListener
has many limitations, which would make it impossible for our applications to use it. Also, in this setup it brings very little benefit, because it still has the same false positives as the old implementation.
I've noticed that the code formatting is way off from Wicket standards. The code will need to be reformatted before it can be merged.
* This request listener uses the {@link DefaultResourceIsolationPolicy} by default and can be | ||
* customized with additional Resource Isolation Policies. | ||
* | ||
* This listener can be configured to add exempted URL paths that are intended to be used cross-site. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setup only allows specific paths to be used cross-site. The old implementation had an isChecked
method that would allow whitelisting pages or even specific requests. In our application, we use the latter. Specific components are annotated to be allowed cross-site. This cannot be done with just a path. Why not use the same isChecked
as the old implementation?
|
||
for (ResourceIsolationPolicy resourceIsolationPolicy : resourceIsolationPolicies) { | ||
if (!resourceIsolationPolicy.isRequestAllowed(containerRequest, targetedPage)) { | ||
log.debug("Isolation policy {} has rejected a request to {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop brings us back to the original problem: the old implementation has false positives and now both have to return true. The idea was to use the old as a fallback in case the fetch metadata headers were missing.
The old implementation had 3 possible outcomes: allowed, disallowed and unknown (because no header was found). As far as I can see, the new implementation has the same 3 possible outcomes. The old implementation should only be consulted when the new implementation is unknown.
This also brings us to the next problem: what if both don't know? In the old implementation, you could configure the behavior for that. In fact, you could configure 3 different CsrfAction
s for both 'unknown' and 'disallowed'. This implementation only supports one: ABORT
. Both SUPPRESS
and ALLOW
are missing.
this.resourceIsolationPolicies.addAll( | ||
asList( | ||
new DefaultResourceIsolationPolicy(), | ||
new OriginBasedResourceIsolationPolicy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way the OriginBasedResourceIsolationPolicy
is internal to this class and there is no way to configure its whitelist. Also, the whitelist has no effect, because DefaultResourceIsolationPolicy
will block the requests anyway.
We can do that after merging. This shouldn't be a stopper for contributors! |
Agreed |
Hi @papegaaij , @svenmeier, @martin-g & @eozmen410 I notice that different reviewers have conflicting designs in mind. This conflict comes from the fact that @papegaaij's applications use the current At this point, we can make changes to use Fetch Metadata as both a CSRF protection and an ACL component, but I believe we would be making Wicket a disservice by creating a security module that is: trivially bypassable (send a CORS-safelisted request to avoid setting the Origin header, which makes the check undecidable) and very hard to maintain, since the policies would be order-dependent and inter-dependent. We are very happy to contribute to Wicket, but unfortunately we only have a limited number of resources to do so. Having said that, I have a proposal that I think can reconciliate both of these approaches:
Our main goal is to provide easy to maintain security mitigations that are effective at keeping users safe in the web platform (here's an example of this in another Apache project) and I believe this proposal achieves just that. Would Wicket developers be interested in seeing something like this? We would be happy to push another set of changes if so. Thank you all for your patience and I'm looking forward to reading your thoughts! :) |
@salcho The current situation is a bit unfortunate indeed. My application does not use the whitelisting, but others might. Looing at the usage of these whitelists, I think the reason these whitelists exist is not for ACL, but again to prevent false positives. False positives are common with the old implementation and hard to solve.
I don't think this is needed. CSRF protection is meant to do just that, it's not an ACL. However, it must be possible to exclude some requests from this protection. The old implementation allowed this via an I'll take a look at the PR, and see what I can do to the improve protection, keep the flexibility and prevent the false positives. |
I think we're overdoing it ;) The actual implementation by @salcho was fine, for me the API wasn't just correct any more with all the old references to 'origin'. This is the simplest solution I can think of, e.g. renaming some methods and going through a change of responsibility in #checkRequest() is enough: I'd deprecate the old listener, and give the new one a fancy new name. |
This is what I came up with: The biggest change is that a resource isolation policy now has 3 possible outcomes: ALLOWED, DISALLOWED and UNKNOWN. They are checked in order, and the first not UNKNOWN will determine the final outcome. Other changes are the reintroduction of the configuration options from the old implementation and the 2 default policies are only added if you did not specify any. |
Btw, I've also reformatted the code to adhere to the Wicket standards. @salcho and @svenmeier if you two agree with my changes, I can merge it all. |
Your reference commit looks pretty good to me @papegaaij! +1 on merging with them! Thank you! |
I've merged the PR. Thanks for the excellent work @salcho and my apologies for the bumpy process. I think the whole discussion has led to a better implementation! |
Hello Wicket devs,
This PR builds Fetch Metadata support on top of Wicket's existing CSRF protection, namely:
Sec-Fetch-*
headers (i.e. comes from a modern browser), Fetch Metadata will be preferred. Otherwise, we will fall back to using the existing cross-request checks.Origin
orReferer
headers are present, Fetch Metadata will apply the same exemptions as the existing Origin-based protection, i.e. allowing cross-origin requests from exempted origins.Vary
header has been added to responses throughonEndRequest
to ensure that any cached responses include Fetch Metadata headers in their key. This is an added layer of security against cache poisoning.