[WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support #442

eozmen410 · 2020-08-11T08:46:04Z

Hello Wicket devs!

This PR adds Cross-Origin Opener Policy and Cross-Origin Embedder Policy support for Wicket.

COOP is a security mitigation that lets developers isolate their resources against side-channel attacks and information leaks. COEP prevents a document from loading any non-same-origin resources which don't explicitly grant the document permission to be loaded. COOP and COEP are independent mechanisms and they can be enabled, tested and deployed separately. Using COEP and COOP together allows developers to safely use powerful features such as SharedArrayBuffer, performance.measureMemory(), and the JS Self-Profiling API. Both COOP and COEP require adding headers to the response. COOP and COEP are now supported by all major browsers. See https://web.dev/why-coop-coep/ for reference.

Here's a summary of all the changes:

We have added 2 new request cycle listeners, the CoopRequestCycleListener and CoepRequestCycleListener to handle adding the headers for the respective security mitigations.
The listeners can be configured using the CoopConfiguration and CoepConfiguration classes that use the builder pattern for ease of use by Wicket users.
Using CoopConfiguration Wicket users will be able to specify the policy they want for COOP (same-origin, same-origin-allow-popups or unsafe-none) and add exempted paths to specify the endpoints for which COOP will not be enabled.
Similarly using CoepConfiguration Wicket users will be able to add exempted paths for which COEP will be disabled and specify if they want COEP to be enforcing (header set as Cross-Origin-Embedder-Policy) or reporting (header set as Cross-Origin-Embedder-Policy-Report-Only)
We have added 2 new methods to the WebApplication class to make it convenient for users to enableCoop and enableCoep.

Here are sample uses of enabling these mechnisms, in the init() method of the WebApplication:

//enabling Cross-Origin Opener Policy
enableCoop(new CoopConfiguration.Builder()
			.withMode(CoopMode.SAME_ORIGIN).withExemptions("<exemptions>").build());
//enabling Cross-Origin Embedder Policy
enableCoep(new CoepConfiguration.Builder()
			.withMode(CoepMode.ENFORCING).withExemptions("<exemptions>").build());

* Initial coop implementation * Fixed typo +reformatting code * Update wicket-core/src/main/java/org/apache/wicket/coop/CoopConfiguration.java Co-authored-by: Sal <[email protected]> * Update wicket-core/src/main/java/org/apache/wicket/coop/CoopConfiguration.java Co-authored-by: Sal <[email protected]> * Updates based on comments on the PR * Initial COEP implementation that doesn't handle report-to and setting up a reporting endpoint * Added javadocs and reformatted code * Fixed typo in javadoc * Updated valid values for COOP, same-origin-allow-popups instead of same-site * Made builder methods public so they can be called from init() in a sample app, added default values for builder fields to avoid null pointer exceptions * making exempted paths a HashSet for faster lookup * Using Set instead of HashSet in the declaration of exemptedPaths + reformatting code * Reformatting code to match Wicket's style * Indentation fix for CoepMode enum * Added tests for each COOP value, inlined url argument for checkHeaders in tests, formatted log statement to include path variable for exempted paths Co-authored-by: Sal <[email protected]>

svenmeier · 2020-08-11T17:47:15Z

wicket-core/src/main/java/org/apache/wicket/coop/CoopConfiguration.java

+		this.mode = mode;
+	}
+
+	public static class Builder


I know the Builder pattern is considered best-practice nowadays :)
But since we don't use it in Wicket, we could live without it here for this simple type of configuration, to be more inline with the rest of the code.

svenmeier · 2020-08-11T17:49:26Z

wicket-core/src/main/java/org/apache/wicket/protocol/http/WebApplication.java

@@ -1111,4 +1115,14 @@ public ContentSecurityPolicySettings getCspSettings()
 		}
 		return cspSettings;
 	}
+
+	public void enableCoop(CoopConfiguration coopConfig)


I don't think we should add these helpers, WebApplication has enough methods already and adding a listener quite easy.

How about this idea:

add the CoopConfiguration and CoepConfiguration as fields in SecuritySettings.

add third mode DISABLED to the existing ones (ENFORCING and REPORTING)

in Application#initApplication() add some logic to auto-add the Coop/Coep listener(s) when they are enabled

This way the developer will have to configure the security settings and don't bother how they are applied.

svenmeier · 2020-08-11T17:53:06Z

Are you sure two separate listeners are the best solution? COEP is a single header only and to quote [https://web.dev/coop-coep/]: "Use a combination of COOP and COEP HTTP headers to opt a web page into a special cross-origin isolated state."
I wonder whether a single listener would be better from the usage perspective.

eozmen410 · 2020-08-12T08:29:39Z

Hi @svenmeier!

Thanks for your review. :)

Are you sure two separate listeners are the best solution? COEP is a single header only and to quote [https://web.dev/coop-coep/]: "Use a combination of COOP and COEP HTTP headers to opt a web page into a special cross-origin isolated state."
I wonder whether a single listener would be better from the usage perspective.

Here's our reasoning behind having 2 separate listeners:

We chose to have two separate listeners for COOP and COEP because they are independent mechanisms that can be deployed separately. Setting COEP headers ensures that a document can only load resources from the same origin, or resources explicitly marked as loadable from another origin. This means that, when COEP is enabled, all the resources loaded from cross-origin need to support CORS and will be blocked if they don't. COOP on the other hand does not require CORS support. It's sufficient to set the COOP response header to process-isolate your document so potential attackers can't access your global object if they were opening a pop-up (preventing XS-leaks). If the application doesn't depend on cross-origin interactions between top-level windows, there should be no observable effect of enabling COOP, while enabling COEP might require some refactoring on the Wicket user's part if they are using cross-origin resources.

You're right in pointing out that enabling COOP and COEP together is the best practice for cross-origin isolation. We thought we would give Wicket users more flexibility if they wanted to enable them separately for any reason.

Having a single listener for both COOP and COEP would also achieve the same results, if we provide flexible ways to configure the listener. In that case, while we can provide a single configuration, it might be somewhat confusing to have CoepMode and CoopMode especially since the acronyms COEP and COOP are already fairly confusing! Having separate configs also has less risk of introducing non-backward compatible configs on that object.

If you think having a single listener is better from a usability perspective, we can change our implementation to merge COOP and COEP into a single listener and provide two separate config objects and handy constructors for the listener that can receive either CooepConfiguration or CoopConfiguration or both. WDYT?

martin-g · 2020-08-12T08:38:41Z

especially since the acronyms COEP and COOP are already fairly confusing!

I'm glad it is not just me finding those acronyms too cryptic.
I, personally, don't mind to use long name like CrossOriginOpenerPolicyConfiguration. The IDEs help to type it quickly and it is much clearer to (re-)read the code.

martin-g · 2020-08-12T08:41:57Z

wicket-core/src/main/java/org/apache/wicket/coop/CoopConfiguration.java

+		return exemptions.contains(path);
+	}
+
+	public void addCoopHeader(WebResponse resp)


Any reason this method is here ?
The config classes (often called XyzSettings in Wicket) usually are POJOs with getters and setters. IMO such action method should be in the listener class.

eozmen410 · 2020-08-14T08:48:37Z

Hi @martin-g , @svenmeier !

How about this idea:
add the CoopConfiguration and CoepConfiguration as fields in SecuritySettings.
add third mode DISABLED to the existing ones (ENFORCING and REPORTING)
in Application#initApplication() add some logic to auto-add the Coop/Coep listener(s) when they are enabled
This way the developer will have to configure the security settings and don't bother how they are applied.

We made the changes suggested by @martin-g in the comment above. Now the config objects live in SecuritySettings and users can use the setter methods to configure COOP and COEP. I've also renamed the configs to have longer names to avoid any confusion between the acronyms! If the configs are not DISABLED the listeners are added automatically in Application#initApplication(). Users can use the following lines in the init() method to enable COOP or COEP.

getSecuritySettings().setCrossOriginOpenerPolicyConfiguration(CoopMode.SAME_ORIGIN, "exemptions");
getSecuritySettings().setCrossOriginEmbedderPolicyConfiguration(CoepMode.ENFORCING, "exemptions");

martin-g

Sorry for the late review - vacation times!

wicket-core/src/main/java/org/apache/wicket/Application.java

martin-g · 2020-08-24T12:56:21Z

wicket-core/src/main/java/org/apache/wicket/coep/CoepConfiguration.java

@@ -0,0 +1,108 @@
+/*


It'd have been better to put coep package under org.apache.wicket.security parent package but I see that the csp package is already in the root (o.a.w) so it would be inconsistent to do this now for coep and coop ... :-/

martin-g · 2020-08-24T13:01:36Z

wicket-core/src/main/java/org/apache/wicket/coop/CoopRequestCycleListener.java

+	@Override
+	public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+	{
+		HttpServletRequest request = (HttpServletRequest)cycle.getRequest().getContainerRequest();


Better check that the container request is HttpServletRequest before casting it.
Otherwise tests using MockWebRequest will have do manually disable these listeners.

We ran into this problem when running the tests in wicket-core and we disabled the listeners in the DummyApplication for RequestCycleListenerTest by overriding the init method here since it was only this test case that was causing the problem. We can also revert these changes and add a check for HttpServletRequest WDYT?

…op and coep, removed CoopConfiguration and CoepConfiguration files

eozmen410 · 2020-08-24T14:50:14Z

Thanks for the review @martin-g ! I've renamed the request cycle listeners :) Is there anything else we can do to get this merged?

svenmeier

IMHO the additions to Application would be better located in WebApplication#internalInit().
CSP is enforced there too.

eozmen410 · 2020-08-24T15:26:56Z

IMHO the additions to Application would be better located in WebApplication#internalInit().
CSP is enforced there too.

AFAICT the internalInit() method is called before init(), since we expect the users to configure their policies for COOP and COEP in the init() method, and we will add the listeners with the configs if they are not DISABLED, if we move the secuirtyInit() to WebApplication#internalInit() the configuration decisions made by the user in their WebApplication#init() will not have an effect on listener behavior. WDYT?

svenmeier · 2020-08-24T16:04:48Z

The listeners should always query the configuration anyway - what if the app changes the configuration later on?

eozmen410 · 2020-08-25T07:25:08Z

The listeners should always query the configuration anyway - what if the app changes the configuration later on?

By our design right now, changes made to the configuration after the init() method will not have an effect on the listener. We didn't think users would want to change their policies at runtime. Also since securityInit() is called once during initialization, if a listener is not added in the secuirtyInit() (configuration was DISABLED in init()) and later on changes the configuration (enable at runtime) the listener would not be added.

Are there use cases for changing the policies at runtime?

svenmeier · 2020-08-25T07:33:36Z

It's a matter of consistency, please see how CSPRequestCycleListener does it.

We have to take care of naming too: securityInit() is not where 'an application's security is initialized'.

salcho · 2020-08-25T10:50:52Z

It's a matter of consistency, please see how CSPRequestCycleListener does it.

We have to take care of naming too: securityInit() is not where 'an application's security is initialized'.

Hi @svenmeier!

We have updated the name of the method now, thanks for the suggestion! We were just wondering if you could elaborate a bit on what the use case is for changing security configurations at runtime. Do you have any cases where a developer would like to change the set of scripts that are allowed to run or the set of endpoints intended to be used cross-origin at runtime?

While I agree that consistency is important, I also think it makes sense to programmatically add listeners that depend on user config. IMHO, the current approach has two downsides:

Listeners get added to the stack regardless of user config, adding latency to the processing of each request even if they don't act on the request in any way (in CSP for instance, the mustProtect method is called on every request even if ContentSecurityPolicySettings is empty).
Security configurations are not immutable. This is often the source of vulnerabilities because an attacker can usually force a code path that disables/corrupts security mitigations. Typical examples of this are reflecting origins in CORS headers at runtime, bypassing CSP when it's built at runtime with user input or creating arbitrary redirects on hosts based on user input.

Perhaps Wicket has some good reasons why CSP works the way it does currently! If that isn't the case, do you think this could be a good opportunity to improve performance/security? I think having a config-dependent init that runs after the user has a chance to configure their app would achieve this!

We could keep consistency by moving the adding of the CSP listener to this proposed new init too, so that the listener is never added if no CSP has been configured. This would set a nice trend for future listeners.

These are principles we've used in other contributions (see Struts' implementation of COOP, CSP and Fetch Metadata) and would really like to know if Wicket has use cases where these principles don't apply, so we'd definitely appreciate some feedback!

martin-g · 2020-08-25T11:05:44Z

Listeners get added to the stack regardless of user config, adding latency to the processing of each request even if they don't act on the request in any way (in CSP for instance, the mustProtect method is called on every request even if ContentSecurityPolicySettings is empty)

This is a good point! I don't remember it being discussed. And I think it was not considered.
IMO the CSP listener should be reworked to behave like the new Coop/Coep ones.
If an application needs to reconfigure something after the application init phase it can always go through the long path:

iterate over the request cycle listeners
remove any of them
change configuration
(re)add any of them

salcho · 2020-08-26T12:03:24Z

Thanks Martin! If you all agree, we could rename the current coopCoepInit method to something else (we had securityInit before but configurationDependentInit or some other name you think suits better works for us) and have this PR merged as is, so that the CSP listener can be moved in a follow-up PR.

svenmeier · 2020-08-26T15:39:18Z

Please move your code into a new WebApplication#validateInit() - don't forget to call super.validateInit().
I'll do the same for the CSP enforcement.

We can decide later which configurations we want to be frozen after the application was initialized.

eozmen410 · 2020-08-26T20:14:07Z

Thanks @svenmeier, @martin-g , @salcho ! I've moved the lines that add the Coop/Coep listeners to WebApplication#validateInit() like you suggested.

martin-g · 2020-08-27T08:54:05Z

I've made some minor improvements to the PR.

salcho · 2020-08-27T11:16:23Z

Thank you all! We'll be submitting a PR updating Wicket documentation soon! We're super excited to have contributed these features :)

martin-g · 2020-08-27T12:00:19Z

Thank you, @eozmen410 & @salcho !

eozmen410 and others added 2 commits August 11, 2020 11:18

Fixed typo in COEP debug statement

e347fd7

svenmeier reviewed Aug 11, 2020

View reviewed changes

martin-g reviewed Aug 12, 2020

View reviewed changes

Refactoring configs into SecuritySettings for COOP and COEP

a7a1d97

eozmen410 requested a review from martin-g August 14, 2020 08:57

martin-g approved these changes Aug 24, 2020

View reviewed changes

Renamed the request cycle listeners and tests with longer names of co…

a157556

…op and coep, removed CoopConfiguration and CoepConfiguration files

svenmeier suggested changes Aug 24, 2020

View reviewed changes

Renamed securityInit() method to coopCoepInit()

977dd3f

Moved adding coop/coep listeners to validetInit in WebApplication

e262326

WICKET-6805 Formatting, cleanup and minor improvements

a3db74c

svenmeier approved these changes Aug 27, 2020

View reviewed changes

martin-g merged commit 0f42d33 into apache:master Aug 27, 2020

eozmen410 mentioned this pull request Aug 28, 2020

Documentation for Fetch Metadata and Coop/Coep Suppport #447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support #442

[WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support #442

eozmen410 commented Aug 11, 2020

svenmeier Aug 11, 2020

svenmeier Aug 11, 2020

martin-g Aug 12, 2020 •

edited

Loading

svenmeier commented Aug 11, 2020

eozmen410 commented Aug 12, 2020

martin-g commented Aug 12, 2020

martin-g Aug 12, 2020

eozmen410 commented Aug 14, 2020

martin-g left a comment

martin-g Aug 24, 2020

martin-g Aug 24, 2020

eozmen410 Aug 24, 2020

eozmen410 commented Aug 24, 2020

svenmeier left a comment

eozmen410 commented Aug 24, 2020 •

edited

Loading

svenmeier commented Aug 24, 2020

eozmen410 commented Aug 25, 2020

svenmeier commented Aug 25, 2020

salcho commented Aug 25, 2020

martin-g commented Aug 25, 2020

salcho commented Aug 26, 2020

svenmeier commented Aug 26, 2020

eozmen410 commented Aug 26, 2020

martin-g commented Aug 27, 2020

salcho commented Aug 27, 2020

martin-g commented Aug 27, 2020

[WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support #442

[WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support #442

Conversation

eozmen410 commented Aug 11, 2020

svenmeier Aug 11, 2020

Choose a reason for hiding this comment

svenmeier Aug 11, 2020

Choose a reason for hiding this comment

martin-g Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

svenmeier commented Aug 11, 2020

eozmen410 commented Aug 12, 2020

martin-g commented Aug 12, 2020

martin-g Aug 12, 2020

Choose a reason for hiding this comment

eozmen410 commented Aug 14, 2020

martin-g left a comment

Choose a reason for hiding this comment

martin-g Aug 24, 2020

Choose a reason for hiding this comment

martin-g Aug 24, 2020

Choose a reason for hiding this comment

eozmen410 Aug 24, 2020

Choose a reason for hiding this comment

eozmen410 commented Aug 24, 2020

svenmeier left a comment

Choose a reason for hiding this comment

eozmen410 commented Aug 24, 2020 • edited Loading

svenmeier commented Aug 24, 2020

eozmen410 commented Aug 25, 2020

svenmeier commented Aug 25, 2020

salcho commented Aug 25, 2020

martin-g commented Aug 25, 2020

salcho commented Aug 26, 2020

svenmeier commented Aug 26, 2020

eozmen410 commented Aug 26, 2020

martin-g commented Aug 27, 2020

salcho commented Aug 27, 2020

martin-g commented Aug 27, 2020

martin-g Aug 12, 2020 •

edited

Loading

eozmen410 commented Aug 24, 2020 •

edited

Loading