Skip to content
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

Jump to bottom

mgr/dashboard: allow cross origin when the url is set #49060

Merged
merged 1 commit into from Nov 30, 2022
Merged

mgr/dashboard: allow cross origin when the url is set #49060

merged 1 commit into from Nov 30, 2022

Conversation

nizamial09
Copy link
Member

@nizamial09 nizamial09 commented Nov 25, 2022 •
edited

Allow CORS when the cross_origin_url is set in the config opt.

you have to update the cross_origin_url setting with the url of the
requesting entity.

The request needs to have the header Access-Control-Allow-Origin
with the origin URL

The url can be set using this command
ceph config set mgr mgr/dashboard/cross_origin_url http://localhost:4200

multiple urls can be set as
ceph config set mgr mgr/dashboard/cross_origin_url http://localhost:4200,http://localhost:4201

If multiple url is provided in the configuration option, then whatever
url is there in the Access-Control-Allow-Origin request header will be
allowed for CORS

Once the URL is set you have to restart the dashboard module to restart
the cherrypy server with the new CORS policies

Fixes: https://tracker.ceph.com/issues/58086
Signed-off-by: Nizamudeen A nia@redhat.com

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@nizamial09 nizamial09 requested a review from a team as a code owner November 25, 2022 15:07
@nizamial09 nizamial09 changed the title mgr/dashboard: allow cross origin when the url is set mgr/dashboard: allow cross origin when the url is set Nov 25, 2022
@nizamial09
Copy link
Member Author

jenkins test dashboard

@nizamial09 nizamial09 force-pushed the accept-cors-dashboard branch 2 times, most recently from 2cbf0f9 to a36914c Compare November 28, 2022 05:50
pereman2
pereman2 requested changes Nov 28, 2022
View reviewed changes
src/pybind/mgr/dashboard/module.py Show resolved Hide resolved
src/pybind/mgr/dashboard/module.py Outdated Show resolved Hide resolved
@nizamial09
mgr/dashboard: allow cross origin when the url is set
1cf017b 
Allow CORS when the cross_origin_url is set in the config opt.

you have to update the cross_origin_url setting with the url of the
requesting entity.

The request needs to have the header `Access-Control-Allow-Origin`
with the origin URL

The url can be set using this command
`ceph config set mgr mgr/dashboard/cross_origin_url
http://localhost:4200`

multiple urls can be set as
`ceph config set mgr mgr/dashboard/cross_origin_url
http://localhost:4200,http://localhost:4201`

If multiple url is provided in the configuration option, then whatever
url is there in the Access-Control-Allow-Origin request header will be
allowed for CORS

Once the URL is set you have to restart the dashboard module to restart
the cherrypy server with the new CORS policies

Fixes: https://tracker.ceph.com/issues/58086
Signed-off-by: Nizamudeen A <nia@redhat.com>
@nizamial09
Copy link
Member Author

jenkins test make check

@nizamial09
Copy link
Member Author

jenkins test dashboard

avanthakkar
avanthakkar approved these changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@avanthakkar avanthakkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

pereman2
pereman2 approved these changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@pereman2 pereman2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nizamial09 nizamial09 merged commit e4e9627 into ceph:main Nov 30, 2022
10 of 14 checks passed
@nizamial09 nizamial09 deleted the accept-cors-dashboard branch November 30, 2022 04:26
This was referenced Nov 30, 2022
Merged
Merged
epuertat
epuertat reviewed Jan 4, 2023
View reviewed changes
Copy link
Member

@epuertat epuertat left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Besides the request handler code reduction, another thing to consider would be to extend the code in the main module following some pattern, like the supported 'plugin' pattern. Currently the module.py is 600 lines, and the plugin approach allows to decouple code from the main module and just use 'hooks' to extend current functionality.

If you guys need any guidance with that, plz let me know and I'll tell you more about the Dashboard plugin framework.

src/pybind/mgr/dashboard/module.py
Comment on lines +249 to +251
req_header_origin_url = req_head.get('Access-Control-Allow-Origin')
cross_origin_urls = mgr.get_localized_module_option('cross_origin_url', '')
cross_origin_url_list = [url.strip() for url in cross_origin_urls.split(',')]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every request handler will be executed once per request (and requests are one of the scaling factors of webservers). Therefore it's essential to keep them simple and remove any processing that it's strictly not mandatory. The fact that every HTTP request requires to interact with the ceph-mgr API might result in a DoS vulnerability (unauthenticated users might send thousands of dummy HTTP requests and cause a performance degradation in the ceph-mgr). I remember that get_module_option calls are cached and don't end up hitting the ceph-mon, but even though it's healthy not to risk that.

My recommendation here would be to remove the get_localized_module_option() (BTW, get_module_option() would be enough, since there's no reason to configure different CORS setting per active manager) and pass that fixed param to the cors_tool(self, cross_origin_list) and then configured as 'tools.cors_tool.cross_origin_list': '...'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epuertat epuertat epuertat left review comments

@avanthakkar avanthakkar avanthakkar approved these changes

@pereman2 pereman2 pereman2 approved these changes

Assignees
No one assigned
Labels
dashboard pybind
Projects
Ceph-Dashboard
Archived in project
Milestone
No milestone
4 participants
@nizamial09 @avanthakkar @pereman2 @epuertat