Add experimental CORS server support #519

bostko · 2017-01-13T19:14:03Z

Include org.apache.cxf.rs.security.cors.CrossOriginResourceSharing implementation

m4rkmckenna · 2017-01-13T19:17:45Z

@bostko how is the filter configured?

geomacy

Some initial comments; haven't finished looking through it but will continue tomorrow.

geomacy · 2017-01-17T17:04:59Z

core/src/main/java/org/apache/brooklyn/core/BrooklynFeatureEnablement.java

@@ -61,6 +61,37 @@
    /** whether feeds are automatically registered when set on entities, so that they are persisted */
    public static final String FEATURE_FEED_REGISTRATION_PROPERTY = FEATURE_PROPERTY_PREFIX+".feedRegistration";

+    /**


I didn't say that CORS was always insecure; used judiciously, it can be a perfectly acceptable mechanism for many use-cases. Used badly or inappropriately, it can open you up to increased security risks, but the same can be said about lots of things. I would remove the warnings from this comment, and simply explain what the feature is that is being enabled. It is up to users to determine whether CORS is applicable in their case and to enable it if so.

geomacy · 2017-01-17T17:06:49Z

core/src/main/java/org/apache/brooklyn/core/BrooklynFeatureEnablement.java

+     * In short, the proposed architecture and use of CORS is more complex, less secure,
+     * and more difficult to manage than the alternative of web client ---> Apache Brooklyn Server ----> fan out to backend servers + Apache Brooklyn API + etc.
+     *
+     * Notes by Geoff Macartney.


Definitely don't blame me! :-)

geomacy · 2017-01-17T17:08:21Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/filter/CorsImplSupplierFilter.java

+import java.util.Collections;
+import java.util.List;
+
+/**


Why repeat this? In any case, as above, just get rid of these warnings.

geomacy · 2017-01-17T17:10:37Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/filter/CorsImplSupplierFilter.java

+    private static final boolean brooklynFeatureEnabled = BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_CORS_CXF_PROPERTY);
+    static {
+        if (brooklynFeatureEnabled) {
+            LOGGER.warn("CORS brooklyn feature enabled.");


info not warn

geomacy · 2017-01-17T17:12:18Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/filter/CorsImplSupplierFilter.java

+    public CorsImplSupplierFilter(@Nullable ManagementContext managementContext) {
+        setFindResourceMethod(false);
+        if (managementContext != null) {
+            setAllowOrigins(JavaGroovyEquivalents.<List<String>>elvis(managementContext.getConfig().getConfig(ALLOWED_ORIGINS), Collections.<String>emptyList()));


why use elvis here? Also prefer ImmutableList.of() to emptyList.

It is possible for ALLOWED_ORIGINS to be null or empty, if so then I find elvis use full to setting it to an empty list.
Collections.emptyList() is used in CrossOriginResourceSharingFilter default constructor so I use the same class which is also immutable.

geomacy · 2017-01-17T17:41:07Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/filter/CorsImplSupplierFilter.java

+ */
+@Provider
+public class CorsImplSupplierFilter extends CrossOriginResourceSharingFilter {
+    public static final ConfigKey<List<String>> ALLOWED_ORIGINS = ConfigKeys.newConfigKey(new TypeToken<List<String>>() {}, BrooklynFeatureEnablement.FEATURE_CORS_CXF_PROPERTY + ".allowedOrigins");


We should use Karaf's Configuration Admin service to do the configuration in the Karaf case.

See for example how this is done in the karaf blueprint and used to inject configuration into the launcher.

@geomacy I see osgilauncher.cfg as configuration done by the distribution creator and brooklyn.cfg configured by the user. That's why I think it makes sense to have this as configurable in brooklyn.cfg since the origin is something that the user will decide on.

@neykov Disagree the cors configuration should be in a separate PID

As the CORS configuration has the potential to be hundreds of lines long

Personal preference to stick to the standard configuration approach to Brooklyn and not split it in smaller files.
Hundreds of lines long sounds like something has gone horribly wrong. I'd worry more about sending it on the network.

I don't think the config will be hundreds of lines as it is only the domain of the requesting site that needs to be added to the header, not separate entries for each page that may do the request. There will only be a handful of domains in most use-cases, I expect.

@neykov for the configuration, I agree up to a point; we probably need to think a bit about recommendations for what sort of configs are most appropriate in brooklyn.cfg, and what are better done in the conventional Karaf way of having a PID for a feature, e.g. for CORS you would have a cfg file such as org.apache.brooklyn.rest.filter.cors.cfg with the appropriate config in it. However, as there is only one property to be configured, I think that is probably overkill for this scenario.

neykov

Include the filter in /brooklyn-rest-server/src/main/webapp/WEB-INF/web.xml as well. LGTM otherwise.

neykov · 2017-01-18T10:17:04Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/filter/CorsImplSupplierFilter.java

+
+    public CorsImplSupplierFilter(@Nullable ManagementContext managementContext) {
+        setFindResourceMethod(false);
+        if (managementContext != null) {


Why would this be null in normal operation. Better throw instead of silently ignoring the problem.

tbouron

Minor changes to apply to the unit tests + comments from others.

Otherwise, LGTM

tbouron · 2017-01-18T10:29:31Z

rest/rest-server/src/test/java/org/apache/brooklyn/rest/CorsFilterLauncherTest.java

+
+        response = HttpTool.execAndConsume(client, httpOptionsRequest("script/groovy", shouldAllowOrigin, "POST"));
+        assertNull(response.getHeaderLists().get(HEADER_AC_ALLOW_ORIGIN), "Access Control Header should not be available.");
+        assertOkayResponse(response, "");


You don't actually test that the request is rejected if it comes from another domain

I don't think it's meant to be rejected, if I understand it right, it is the browser that will discard the response as it won't have the header it needs to bypass the same origin policy?

@geomacy that's right.

tbouron · 2017-01-18T10:30:52Z

rest/rest-server/src/test/java/org/apache/brooklyn/rest/CorsFilterLauncherTest.java

+public class CorsFilterLauncherTest extends BrooklynRestApiLauncherTestFixture {
+    @Test
+    public void testEnabledCorsSendsBasicAllowResponse() throws IOException {
+        BrooklynFeatureEnablement.enable(BrooklynFeatureEnablement.FEATURE_CORS_CXF_PROPERTY);


I would break down all tests into single, isolated unit test. Will be simpler to see and check what has been tested or not

You will save only BrooklynRestApiLauncher apiLauncher instantiation.
I do not think making a big test method will be simpler to read.
However I welcome suggestions for shortening assertions.

I meant, the current test function contains 4 separate tests. I would break those 4 down into their own test function to be more readable.

tbouron · 2017-01-18T10:31:03Z

rest/rest-server/src/test/java/org/apache/brooklyn/rest/CorsFilterLauncherTest.java

+    @Test
+    public void testCorsIsDisabled() throws IOException {
+        BrooklynFeatureEnablement.disable(BrooklynFeatureEnablement.FEATURE_CORS_CXF_PROPERTY);
+        final String shouldAllowOrigin = "http://foo.bar.com";


Same as above

geomacy · 2017-01-18T09:27:00Z

core/src/main/java/org/apache/brooklyn/core/BrooklynFeatureEnablement.java

@@ -61,6 +61,36 @@
    /** whether feeds are automatically registered when set on entities, so that they are persisted */
    public static final String FEATURE_FEED_REGISTRATION_PROPERTY = FEATURE_PROPERTY_PREFIX+".feedRegistration";

+    /**
+     * <p>
+     * Enabling CORS is strongly discouraged.


Valentin, as I mentioned before, I think you should remove this comment. It's not possible for us to know all the scenarios users may have, some of them may simply require CORS, and if they use it judiciously then it can be fine.

I would replace the entire comment with simply

/** Enables support for Cross Origin Resource Sharing (CORS) filtering on requests in BrooklynWebServer. If enabled, the allowed origins for the CORS headers should be configured using the "brooklyn.experimental.feature.corsCxfFeature. allowedOrigins" property. If this is not is not supplied then allowedOrigins will be a wildcard on all domains; this is strongly discouraged. Currently there is no support for varying these headers on a per-API-resource basis, that is, the same configured headers are applied to all requests. */

I think it is valuable to have more information.
What others think?

The point is that you can't tell at this level whether CORS is appropriate or not; that's more an architectural question. Sometimes CORS is a proper solution to a given need to integrate multiple server backends onto a page in a web application (e.g. with a plugin based web application). Sometimes it is a hack to glue in some REST call that you should really be handling server side. We can't simply say 'use of CORS is discouraged'.

CORS per se is a good thing. When combined with some other features like basic auth could be a terrible idea because the credentials need to be shared with all clients. Much easier for malicious users to extract the credentials from the javascript env and use them any way they like.
Basic auth would be the reason why I'd stick to proxy until we get something smarter like web tokens.

I like @geomacy's suggestion above, but perhaps with the proxy recommendation added to it.

geomacy · 2017-01-18T09:28:38Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/filter/CorsImplSupplierFilter.java

+import java.util.List;
+
+/**
+ * Using CORS is strongly discouraged.


as above, just delete this line and leave the one referring them to the other comment

geomacy · 2017-01-18T09:37:36Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java

@@ -72,7 +72,7 @@ public Response toResponse(Throwable throwable1) {
                Exceptions.collapse(throwable1));
        } else {
            LOG.debug("REST request running as {} threw: {}", Entitlements.getEntitlementContext(), 


This will lose the stack trace information; I suggest replacing this one line with one at info and one at debug, something like

LOG.info("CorsImplSupplierFilter running in {} caught: {}", Entitlements.getEntitlementContext(), throwable1.getMessage()); LOG.debug("CorsImplSupplierFilter exception details", throwable1);

similarly in other places where the same thing is being done

geomacy · 2017-01-18T10:37:09Z

rest/rest-server/src/test/java/org/apache/brooklyn/rest/CorsFilterLauncherTest.java

+import static org.testng.Assert.assertNull;
+
+/**
+ * If brooklyn.experimental.feature.corsCxfFeature.allowedOrigins is not supplied then allowedOrigins will be all domains.


Would it be worth adding a test for that? Just a suggestion, I don't think it's indispensable.

geomacy · 2017-01-18T11:00:56Z

I think that's all the comments I want to make except that it would be good to have an actual web page test case that demonstrates it working; maybe @m4rkmckenna could help out with that?

Apart from the particular comments above I'm happy to say this looks good to me.

m4rkmckenna · 2017-01-18T15:40:11Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/filter/CorsImplSupplierFilter.java

+        setFindResourceMethod(false);
+        Preconditions.checkNotNull(mgmt,"ManagementContext should be suppplied to CORS filter.");
+        setAllowOrigins(JavaGroovyEquivalents.<List<String>>elvis(
+                mgmt.getConfig().getConfig(ALLOWED_ORIGINS), Collections.<String>emptyList()));


@bostko Please expose all configuration the filter provides as it is all needed

See https://cxf.apache.org/javadoc/latest/org/apache/cxf/rs/security/cors/CrossOriginResourceSharingFilter.html

If we start supporting all config options of the filter, then I could see this being its own PID.

@neykov could you give guidance how this can be done?

m4rkmckenna

Im happy once other tests have been addressed

Ive pen tested to the best of my ability :)

bostko · 2017-01-20T22:37:56Z

PR is updated.
To test it in the karaf distribution you need to have etc/org.apache.brooklyn.rest.filter.cors.cfg

Include org.apache.cxf.rs.security.cors.CrossOriginResourceSharing implementation

m4rkmckenna

Tested 51e568e with apache/brooklyn-dist#81

Once other comments addressed good to merge

bostko · 2017-01-23T12:31:44Z

@m4rkmckenna are there comments which I missed to address?

m4rkmckenna · 2017-01-23T12:36:26Z

@bostko One from @geomacy RE javadoc (DRY) ... I think the rest have been addressed

geomacy

My comments have been addressed.

geomacy · 2017-01-23T13:11:17Z

Looks good, and other reviewers approved. Will merge now.

Add experimental CORS server support Include org.apache.cxf.rs.security.cors.CrossOriginResourceSharing implementation

bostko force-pushed the cxf_cors_filter branch 7 times, most recently from 61fa6a5 to f63593c Compare January 17, 2017 15:52

bostko changed the title ~~WIP Add CXF CrossOriginResourceSharing filter~~ Add CXF CrossOriginResourceSharing filter Jan 17, 2017

bostko force-pushed the cxf_cors_filter branch 2 times, most recently from 7462f7e to 9e5dff5 Compare January 17, 2017 16:33

bostko changed the title ~~Add CXF CrossOriginResourceSharing filter~~ Add experimental CORS server support Jan 17, 2017

geomacy reviewed Jan 17, 2017

View reviewed changes

bostko force-pushed the cxf_cors_filter branch 3 times, most recently from eb9f5b6 to 0c3fcf2 Compare January 18, 2017 09:15

neykov reviewed Jan 18, 2017

View reviewed changes

tbouron requested changes Jan 18, 2017

View reviewed changes

geomacy reviewed Jan 18, 2017

View reviewed changes

bostko force-pushed the cxf_cors_filter branch 2 times, most recently from f592ecb to 828475b Compare January 18, 2017 10:47

bostko force-pushed the cxf_cors_filter branch 6 times, most recently from 57cccbb to 6a60db1 Compare January 18, 2017 15:36

m4rkmckenna requested changes Jan 18, 2017

View reviewed changes

bostko force-pushed the cxf_cors_filter branch from 6a60db1 to d32db15 Compare January 18, 2017 16:30

m4rkmckenna reviewed Jan 19, 2017

View reviewed changes

bostko force-pushed the cxf_cors_filter branch 3 times, most recently from 4fb1f47 to c5e4afd Compare January 20, 2017 22:35

bostko force-pushed the cxf_cors_filter branch from c5e4afd to e92bfc0 Compare January 20, 2017 23:30

bostko mentioned this pull request Jan 20, 2017

Adds REST API CORS configuration file in karaf distribution apache/brooklyn-dist#81

Closed

bostko and others added 3 commits January 22, 2017 06:45

Add experimental CORS server support

31598a0

Include org.apache.cxf.rs.security.cors.CrossOriginResourceSharing implementation

WIP Make CORS filter configurable

8085db5

CORS bean - fix setting default values

51e568e

bostko force-pushed the cxf_cors_filter branch from e92bfc0 to 51e568e Compare January 22, 2017 05:26

m4rkmckenna approved these changes Jan 22, 2017

View reviewed changes

tbouron approved these changes Jan 23, 2017

View reviewed changes

geomacy approved these changes Jan 23, 2017

View reviewed changes

asfgit merged commit 51e568e into apache:master Jan 23, 2017

asfgit pushed a commit that referenced this pull request Jan 23, 2017

Closes #519

a9047f6

Add experimental CORS server support Include org.apache.cxf.rs.security.cors.CrossOriginResourceSharing implementation

Add experimental CORS server support #519

Add experimental CORS server support #519

Conversation

bostko commented Jan 13, 2017 • edited

m4rkmckenna commented Jan 13, 2017

geomacy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbouron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbouron Jan 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geomacy Jan 18, 2017 • edited

Choose a reason for hiding this comment

geomacy commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4rkmckenna left a comment

Choose a reason for hiding this comment

bostko commented Jan 20, 2017

m4rkmckenna left a comment

Choose a reason for hiding this comment

bostko commented Jan 23, 2017 • edited

m4rkmckenna commented Jan 23, 2017

geomacy left a comment

Choose a reason for hiding this comment

geomacy commented Jan 23, 2017

bostko commented Jan 13, 2017 •

edited

tbouron Jan 18, 2017 •

edited

geomacy Jan 18, 2017 •

edited

bostko commented Jan 23, 2017 •

edited