Skip to content

Update CSP handler to only query and modify frame ancestors instead of all CSP directives#6398

Merged
ZilongX merged 18 commits intoopensearch-project:mainfrom
tianleh:only-frame-ancestors
Apr 15, 2024
Merged

Update CSP handler to only query and modify frame ancestors instead of all CSP directives#6398
ZilongX merged 18 commits intoopensearch-project:mainfrom
tianleh:only-frame-ancestors

Conversation

@tianleh
Copy link
Member

@tianleh tianleh commented Apr 10, 2024

Description

We get some feedback from security engineers that allowing customers to modify all CSP directives could accidentally expose OSD to attacks. They suggest us to only allow modifying frame-ancestors and do not modify other CSP directives unless reviewed case by case.

Issues Resolved

Screenshot

Testing the changes

call get API

curl -X GET 'http://localhost:5601/api/appconfig/csp.rules.frame_ancestors'

{"statusCode":404,"error":"Not Found","message":"index_not_found_exception: [index_not_found_exception] Reason: no such index [.opensearch_dashboards_config]"}%

Confirm that frame ancestors have a default value 'self'.

Screenshot 2024-04-10 at 22 45 54

Confirm a local html file which embeds using iframe doesn't open OSD.

Screenshot 2024-04-10 at 22 48 59

call update API


curl 'http://localhost:5601/api/appconfig/csp.rules.frame_ancestors' -X POST -H 'Accept: application/json' -H 'Content-Type: application/json' -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' --data-raw '{"newValue":"file://* filesystem: https://example-site.org"}'

{"newValue":"file://* filesystem: https://example-site.org"}%

We can see that new CSP is taking effect.

Screenshot 2024-04-10 at 22 51 22

Confirm the html file can now open OSD.

Screenshot 2024-04-10 at 22 52 07

call get API again

curl -X GET 'http://localhost:5601/api/appconfig/csp.rules.frame_ancestors'

{"value":"file://* filesystem: https://example-site.org"}%

call the get API for all configs

curl -X GET 'http://localhost:5601/api/appconfig'
{"value":{"csp.rules.frame_ancestors":"file://* filesystem: https://example-site.org"}}%

call delete API

curl 'http://localhost:5601/api/appconfig/csp.rules.frame_ancestors' -X DELETE -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty'

{"deletedEntity":"csp.rules.frame_ancestors"}%

See that CSP is back to default.
Screenshot 2024-04-10 at 22 54 54

The local html file cannot open OSD.

Screenshot 2024-04-10 at 22 55 18

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link
Contributor

@yujin-emma yujin-emma left a comment

Choose a reason for hiding this comment

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

overall LGTM, left one small comment but not a blocker

@tianleh tianleh force-pushed the only-frame-ancestors branch from 6834c11 to 89e1790 Compare April 11, 2024 17:47
@seraphjiang
Copy link
Member

seraphjiang commented Apr 11, 2024

might be i missed, i didn't see update logic, but PR said only allow updating frame ancestors, doesn't seem match with the code change.

another general comment, it looks we rely on manually test every time make change to these files, could we automate the test.

@tianleh
Copy link
Member Author

tianleh commented Apr 11, 2024

might be i missed, i didn't see update logic, but PR said only allow updating frame ancestors, doesn't seem match with the code change.

another general comment, it looks we really on manually test every time make change to these files, could we automate the test.

The key change is at this line https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6398/files#diff-5cedee9d03c8765c4adb43d8196e75dfe8daec51ced75b9fe9b5df1ded5c8a57R55

We use the new configuration csp.rules.frame_ancestors instead of the previous csp.rules. Then the CSP handler logic is changed accordingly to consume it and set to frame_ancestors in CSP.

I will follow up on the test automation separately as it involves both API testing and UI testing.

@seraphjiang

@tianleh tianleh force-pushed the only-frame-ancestors branch 2 times, most recently from 301bed3 to cbab476 Compare April 15, 2024 18:28
tianleh added 18 commits April 15, 2024 19:36
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
Signed-off-by: Tianle Huang <tianleh@amazon.com>
@tianleh tianleh force-pushed the only-frame-ancestors branch from cbab476 to bf1c643 Compare April 15, 2024 19:36
Copy link
Member

@zhongnansu zhongnansu left a comment

Choose a reason for hiding this comment

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

lgtm

@ZilongX ZilongX merged commit 36c25ae into opensearch-project:main Apr 15, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 15, 2024
…f all CSP directives (#6398)

* only allow updating frame ancestors

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* refactor

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add test

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix link

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update key

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* rename

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update unit tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add change log

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* undo yml

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update readme and variable

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix link

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* make code generic

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* revert yml

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add more tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update key name

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* reword change title

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix typo

Signed-off-by: Tianle Huang <tianleh@amazon.com>

---------

Signed-off-by: Tianle Huang <tianleh@amazon.com>
(cherry picked from commit 36c25ae)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 15, 2024
…f all CSP directives (#6398)

* only allow updating frame ancestors

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* refactor

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add test

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix link

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update key

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* rename

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update unit tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add change log

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* undo yml

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update readme and variable

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix link

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* make code generic

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* revert yml

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add more tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update key name

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* reword change title

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix typo

Signed-off-by: Tianle Huang <tianleh@amazon.com>

---------

Signed-off-by: Tianle Huang <tianleh@amazon.com>
(cherry picked from commit 36c25ae)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
ZilongX pushed a commit that referenced this pull request Apr 16, 2024
…f all CSP directives (#6398) (#6463)

* only allow updating frame ancestors

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* refactor

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add test

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix link

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update key

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* rename

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update unit tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add change log

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* undo yml

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update readme and variable

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix link

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* make code generic

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* revert yml

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add more tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update key name

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* reword change title

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix typo

Signed-off-by: Tianle Huang <tianleh@amazon.com>

---------

Signed-off-by: Tianle Huang <tianleh@amazon.com>
(cherry picked from commit 36c25ae)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
zhongnansu pushed a commit that referenced this pull request Apr 16, 2024
…f all CSP directives (#6398) (#6464)

* only allow updating frame ancestors

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* refactor

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add test

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix link

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update key

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* rename

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update unit tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add change log

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* undo yml

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update readme and variable

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix link

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* make code generic

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* revert yml

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* add more tests

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* update key name

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* reword change title

Signed-off-by: Tianle Huang <tianleh@amazon.com>

* fix typo

Signed-off-by: Tianle Huang <tianleh@amazon.com>

---------

Signed-off-by: Tianle Huang <tianleh@amazon.com>
(cherry picked from commit 36c25ae)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants