policy-engine/src/main: Set basic CORS headers #196

wking · 2020-01-22T21:54:51Z

Allow simple requests by including the:

Access-Control-Allow-Origin: *

header, so folks can write cross-origin web applications that query our Cincinnati server. We're going to be building the response anyway, so we don't even get denial-of-service protection by blocking cross-origin requests for Cincy data.

Also set Access-Control-Allow-Methods for good measure, although we do not need to support preflight requests.

Implementation is based on these docs.

wking · 2020-01-23T00:11:57Z

cargo-test:

 41 | use actix_web::middleware::cors::Cors;
   |                            ^^^^ could not find `cors` in `middleware`

because CORS support moved out of actix-web into its own crate in actix-web 1.0.2. Pushed v906508b -> 8c8067f to pivot to actix-cors ^0.2.0.

wking · 2020-01-23T05:48:03Z

cargo-test:

 error[E0277]: the trait bound `actix_cors::CorsFactory: actix_service::transform::IntoTransform<_, actix_web::app_service::AppRouting>` is not satisfied
  --> policy-engine/src/main.rs:85:17
   |
85 | /                 Cors::new()
86 | |                     .allowed_origin("*")
87 | |                     .allowed_methods(vec!["HEAD", "GET"])
88 | |                     .finish(),
   | |_____________________________^ the trait `actix_service::transform::IntoTransform<_, actix_web::app_service::AppRouting>` is not implemented for `actix_cors::CorsFactory`

That also turns up here, but I'm not clear enough on the Rust to understand what they're saying there yet. My example looks a lot like the actix-cors docs to me.

steveej

The used versions of actix-cors and actix-web are incompatible. Please see my suggestions which at least make it compile and run. In addition to that we should also have a test to verify these changes.

policy-engine/Cargo.toml

policy-engine/src/main.rs

wking · 2020-01-30T19:42:27Z

Pushed a8733ab -> c471c49 with @steveej's suggested changes.

wking · 2020-01-30T19:52:23Z

In addition to that we should also have a test to verify these changes.

Not sure how to set this up. Would I plug into here somehow? Looks like at the moment all that has access to is the body of the response, not the full HTTP response with headers and a body.

steveej · 2020-01-30T20:46:15Z

I think you forgot to push the changes to the Cargo.lock after my recent suggestions.

I played with the tests locally and would like to add a commit on top of your changes if that's fine with you. The tests will still be broken but we can continue to work on it together based on my commit.

wking · 2020-01-31T17:36:49Z

I think you forgot to push the changes to the Cargo.lock...

openshift/release#6935 is in flight to make this easier to do, and impossible to forget (because the required images CI job would fail).

LalatenduMohanty

Why do not we have any tests with this PR?

steveej · 2020-02-03T17:47:15Z

I've been working on getting tests in for this as well as figuring out if this is working correctly in this branch: https://github.com/steveeJ-forks/openshift-cincinnati/tree/forks/wking/cors

However, I can't get the policy-engine to respond to or return any CORS related headers. I suggest we wait for #195 to land which bumps all actix crates to their latest versions. The latest versions have better documentation and we might have more luck with that.

On a side note, I don't fully understand the issue any client would be seeing without this PR. Cincinnati doesn't block any requests for me, no matter what Origin header I provide with the request.

wking · 2020-02-03T21:43:46Z

On a side note, I don't fully understand the issue any client would be seeing without this PR.

CORS is a client-side mechanism. More details in the MDN page linked from my initial PR post, but basically this is Cincy telling browsers "it's ok if a page outside of api.openshift.com to make Cincy requests. That's neither a XSS attack nor malicious data exfiltration.".

LalatenduMohanty · 2020-02-04T07:45:19Z

CORS is a client-side mechanism. More details in the MDN page linked from my initial PR post, but basically this is Cincy telling browsers "it's ok if a page outside of api.openshift.com to make Cincy requests. That's neither a XSS attack nor malicious data exfiltration.".

+1 , basically with this PR we are open to CORS requests from clients which would fail otherwise. AFAIK the typical example is our web browsers which does not allow CORS requests by default.

wking · 2020-02-04T15:59:59Z

Setting this header will avoid the:

CORS header ‘Access-Control-Allow-Origin’ missing

error mentioned in 066d1dc, when Cincinnati is fetched from a page outside of api.openshift.com.

steveej · 2020-02-04T17:02:18Z

And should Access-Control-Allow-Origin be included in the headers in every GET or HEAD request made to any endpoint?

wking · 2020-02-11T18:02:11Z

And should Access-Control-Allow-Origin be included in the headers in every GET or HEAD request made to any endpoint?

s/request/response/, but sure, why not. Seems academic until we have other endpoints to talk about? I don't care one way or the other about having the header on metrics responses, and don't know if the global setup I have here will affect them or not, since they're on a different port.

openshift-bot · 2021-02-23T06:58:47Z

/retest