Skip to content

Implement dynamic logic for CORS request filtering #2230

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

Closed
otsakir opened this issue Jun 13, 2017 · 11 comments
Closed

Implement dynamic logic for CORS request filtering #2230

otsakir opened this issue Jun 13, 2017 · 11 comments
Assignees
Labels

Comments

@otsakir
Copy link
Contributor

otsakir commented Jun 13, 2017

Which CORS requests should get responses with Allow-* headers ? We can do one of the following:

Use a configuration list defined by an administrator.

For example define a new section in restcom.xml like the following:

<corsWhitelist>
	<origin>http://foo.restcomm.com</origin>
</corsWhitelist>

and add the necessary configuration scripts to populate this configuration from restcomm.conf/advanced.conf

Set up a convention

Set up a convention based on the destination name of the request. For example if the incoming request targeted domainA.restcomm.com return an Access-Control-Allow-Origin header like Access-Control-Allow-Origin=rvd.domainA.restcomm.com.

@deruelle , @leftyb thoughts ?

Or of course we could support both ways.

@otsakir otsakir added this to the Orestis Sprint 5 milestone Jun 13, 2017
@otsakir otsakir self-assigned this Jun 13, 2017
@deruelle
Copy link
Member

deruelle commented Jun 13, 2017

@otsakir the second option makes more sense to me so far so it stays dynamic and as new domains are added no additional configuration is required.

@otsakir
Copy link
Contributor Author

otsakir commented Jun 13, 2017

ok @deruelle , but what about the convention itself ? does it feel right ?

@lefty ?

@otsakir
Copy link
Contributor Author

otsakir commented Jun 13, 2017

Also, some more thinking...

The issue relates to this:
#2212

The <rcml-server/> configuration section was initially created for the notifications sent to RVD when an account was removed. I'm wondering whether it makes sense to use it for bootstrapping the CORS filter.

Btw, @deruelle , it's not only about cloud. It makes sense to be able to use an arbitrary RVD address (i.e. a configurable one, not a convention-based one) for more flexibility.

@deruelle
Copy link
Member

Good point, we should have a simple way to configure the mapping in that case corresponding to your logic below

For example if the incoming request targeted domainA.restcomm.com return an Access-Control-Allow-Origin header like Access-Control-Allow-Origin=rvd.domainA.restcomm.com.

@otsakir
Copy link
Contributor Author

otsakir commented Jun 15, 2017

@deruelle , after discussion with @leftyb , we agreed to take advantage of existing rcmlsesrver-api configuration section in restcomm.xml which points to the RCML server/rvd. It was initially created for sending notifications when an account was removed.

Here is a sample:

	<rcmlserver-api>
		<base-url>/restcomm-rvd/services</base-url>
		<notifications>true</notifications>
		<timeout>5000</timeout>
		<timeout-per-notification>500</timeout-per-notification>
	</rcmlserver-api>

<base-url/> holds a relative url. The idea is to have CorsFilter check _base-url and if it's absolute, apply the CORS allow logic and return headers appropriately.

The section above should probably be refactored to the following to better reflect current state of codebase:

	<rcmlserver>
                <base-url>http://rvdserver.restcomm.com:8081</base-url>
		<api-path>/restcomm-rvd/services</path>
		<notifications>true</notifications>
		<timeout>5000</timeout>
		<timeout-per-notification>500</timeout-per-notification>
	</rcmlserver>

As far as i remember there are no major dependencies of this section other than RcmlserverApi class.

Another issue should be created to handle setting restcomm.xml/rcmlserver appropriately

@otsakir
Copy link
Contributor Author

otsakir commented Jun 15, 2017

Also @deruelle , regarding your comment

@otsakir the second option makes more sense to me so far so it stays dynamic and as new domains are added no additional configuration is required.

If you what you mean is to have a restcomm instance hooked up to multiple RVD instances in different domains each, i see no easy way to do this.

  • Think a little what happens with the 'Visual Designer' link that appears in Dashboard. Where will it point to ? Probably it should get removed altogether.
  • What about metadata information retrieved from RVD for a set of applications in the 'Apps' ? We need to check the application URL and contact the respective app server. Will this scale ?
  • What about notifications to RVD when an account is removed ?
  • What happens with the absolute links to the RCML controller that are stored in the Application. Where will they point to ?

We need to be very careful when drafting this out and always have in mind what happens with the storage layer behind the servers. Will it be shared or not ?

@deruelle
Copy link
Member

@otsakir what I mean is multi-tenancy ie that a given Restcomm Connect instance can manage many organizations (domains) and so should RVD. At any given point in time though a given Restcomm Connnect can send a request only for the domain it is currently processing the request for ie a request for cloud.restcomm.com will only go to rvd.cloud.restcomm.com never to rvd.anotherorg.restcomm.com by example

@otsakir
Copy link
Contributor Author

otsakir commented Jun 16, 2017

ok @deruelle, i see. So there should be some kind of logic both in the client and the server. In the client (Dashboard) it will be able to guess rvd's location from restcomm's location (i.e. where index.html was retrieved from) and in the server it will check for Origin header in the inverse way. So, yes, we're definitely talking about a convention here.

This seems part of another round of tasks. I think we can proceed with using configuration based allowwed origins for now (already mostly implemented) and review this as part of the organization work.

wdyt ?

otsakir added a commit that referenced this issue Jun 16, 2017
… and RcmlserverApi class

- renamed rcmlserver-api to rcmlserver
- added api-path property

Refers #2230
otsakir added a commit that referenced this issue Jun 16, 2017
This relates to 'rcmlserver' configuration section since it used it

Refers #2230
otsakir added a commit that referenced this issue Jun 16, 2017
@deruelle
Copy link
Member

@otsakir works for me but please create a new issue to track it down and put the solution in there for when the time comes we know what has to be done and see if you can include it in the Organizations Epic as well. Thanks !

@otsakir
Copy link
Contributor Author

otsakir commented Jun 20, 2017

ok @deruelle :

#2254
#2255

@otsakir
Copy link
Contributor Author

otsakir commented Jul 13, 2017

Closing this since it has been reviewed and merged.

@otsakir otsakir closed this as completed Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants