Support dynamic CSP rules to mitigate clickjacking #5641

tianleh · 2023-12-23T11:13:19Z

Description

This PR is to mitigate the Clickjacking vulnerability as stated #5639.

Notes for the repo maintainers: please add backport label to the branch 2.x. We are targeting at 2.12.0 release for this feature.

We will target at 2.13.0. Please help add the backport label to the branch 2.x.

12/27/2023
Unit tests are being added.

12/23/2023
For now, I am sending the PR without any tests to collect early feedback. More tests will be added later.

Issues Resolved

fixes #5639

Screenshot

Testing the changes

Test Case 1

Step 0: There are no csp rules configured in OSD yml.

Step 1: Verify that there is no index .opensearch_dashboards_config by default.
Run command GET .opensearch_dashboards_config/_doc/csp.rules

Receive this response.

Step 2: Verify the response header has added frame-ancestors 'self' by default.

Step 3: Verify through a local test html file.

<html>
<head>
<title>Clickjack test page</title>
</head>
<body>
<p>Test clickjacking!</p>
<iframe src="http://localhost:5601/app/home#/" width="500" height="500"></iframe>
</body>
</html>

Open the test html in browser and expect to see the following page.

Step 4: Update the index .opensearch_dashboards_config to allow embedding inside local file.

PUT .opensearch_dashboards_config/_doc/csp.rules
{
  "value": "frame-ancestors 'self' file://* filesystem:"
}

Step 5: Verify that the test html can open now.

Test Case 2

Step 0: There are CSP rules configured in OSD YML.

csp.rules: ["frame-ancestors 'self' file://* filesystem:"]

Step 1:
Verify that by default there is no index .opensearch_dashboards_config.

GET .opensearch_dashboards_config/_doc/csp.rules

Returns index_not_found_exception

Step 2:

Verify that the values from the OSD YML are used.

Step 3:

Update the .opensearch_dashboards_config with different CSP rules from the YML.


PUT .opensearch_dashboards_config/_doc/csp.rules
{
  "value": "frame-ancestors 'self' http://example-test.com"
}

Step 4:

Verify that the values from the index are used.

Step 5:

Update .opensearch_dashboards_config with empty rules.

PUT .opensearch_dashboards_config/_doc/csp.rules
{
  "value": ""
}

Step 6:

Verify that the values from the YML are used.

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

codecov · 2023-12-23T11:27:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.12%. Comparing base (1c5ad6c) to head (fcc9875).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5641      +/-   ##
==========================================
+ Coverage   67.11%   67.12%   +0.01%     
==========================================
  Files        3322     3323       +1     
  Lines       64271    64291      +20     
  Branches    10333    10336       +3     
==========================================
+ Hits        43135    43155      +20     
  Misses      18614    18614              
  Partials     2522     2522

Flag	Coverage Δ
Linux_1	`31.66% <ø> (ø)`
Linux_2	`55.20% <ø> (ø)`
Linux_3	`44.72% <100.00%> (+0.03%)`	⬆️
Linux_4	`35.10% <ø> (ø)`
Windows_1	`31.69% <ø> (ø)`
Windows_2	`55.17% <ø> (ø)`
Windows_3	`44.73% <100.00%> (+0.03%)`	⬆️
Windows_4	`35.10% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tianleh · 2023-12-29T04:26:39Z

The one failed https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/7353558209/job/20019665599?pr=5641 to build on macOS ARM64 failed but was successful in previous run

https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/7353233916

Will monitor in next run.

Could anyone with permission rerun it for me or grant me the re-run workflow permission?

tianleh · 2023-12-29T04:31:23Z

Hi team @opensearch-project/opensearch-dashboards-core , could you please help review?

cc @seraphjiang @wbeckler @zengyan-amazon

src/plugins/csp_configuration_provider/server/plugin.ts

src/plugins/csp_configuration_provider/server/types.ts

src/plugins/csp_configuration_provider/server/provider.ts

tianleh · 2024-01-03T01:36:40Z

cc current oncall @AMoo-Miki for visibility. Please help review and add the backport labels 2.x.

seraphjiang · 2024-01-04T21:59:09Z

@Flyingliuhub @bandinib-amzn @ZilongX would you help to review

kavilla · 2024-01-05T20:09:31Z

pulling down

src/plugins/csp_configuration_provider/README.md

Signed-off-by: Tianle Huang <[email protected]>

* support dynamic csp rules to mitigate clickjacking Signed-off-by: Tianle Huang <[email protected]> * add unit tests for the provider class Signed-off-by: Tianle Huang <[email protected]> * move request handler to its own class Signed-off-by: Tianle Huang <[email protected]> * add license headers Signed-off-by: Tianle Huang <[email protected]> * fix failed unit tests Signed-off-by: Tianle Huang <[email protected]> * add unit tests for the handler Signed-off-by: Tianle Huang <[email protected]> * add content to read me Signed-off-by: Tianle Huang <[email protected]> * fix test error Signed-off-by: Tianle Huang <[email protected]> * update readme Signed-off-by: Tianle Huang <[email protected]> * update CHANGELOG.md Signed-off-by: Tianle Huang <[email protected]> * update snap tests Signed-off-by: Tianle Huang <[email protected]> * update snapshots Signed-off-by: Tianle Huang <[email protected]> * fix a wrong import Signed-off-by: Tianle Huang <[email protected]> * undo changes in listing snap Signed-off-by: Tianle Huang <[email protected]> * improve wording Signed-off-by: Tianle Huang <[email protected]> * set client after default client is created Signed-off-by: Tianle Huang <[email protected]> * update return value and add a unit test Signed-off-by: Tianle Huang <[email protected]> * remove unnecessary dependency Signed-off-by: Tianle Huang <[email protected]> * make the name of the index configurable Signed-off-by: Tianle Huang <[email protected]> * expose APIs and update file structures Signed-off-by: Tianle Huang <[email protected]> * add header Signed-off-by: Tianle Huang <[email protected]> * fix link error Signed-off-by: Tianle Huang <[email protected]> * fix link error Signed-off-by: Tianle Huang <[email protected]> * add more unit tests Signed-off-by: Tianle Huang <[email protected]> * add more unit tests Signed-off-by: Tianle Huang <[email protected]> * update api path Signed-off-by: Tianle Huang <[email protected]> * remove logging Signed-off-by: Tianle Huang <[email protected]> * update path Signed-off-by: Tianle Huang <[email protected]> * rename index name Signed-off-by: Tianle Huang <[email protected]> * update wording Signed-off-by: Tianle Huang <[email protected]> * make the new plugin disabled by default Signed-off-by: Tianle Huang <[email protected]> * do not update defaults to avoid breaking change Signed-off-by: Tianle Huang <[email protected]> * update readme to reflect new API path Signed-off-by: Tianle Huang <[email protected]> * update handler to append frame-ancestors conditionally Signed-off-by: Tianle Huang <[email protected]> * update readme Signed-off-by: Tianle Huang <[email protected]> * clean up code to prepare for application config Signed-off-by: Tianle Huang <[email protected]> * reset change log Signed-off-by: Tianle Huang <[email protected]> * reset change log again Signed-off-by: Tianle Huang <[email protected]> * update accordingly to new changes in applicationConfig Signed-off-by: Tianle Huang <[email protected]> * update changelog Signed-off-by: Tianle Huang <[email protected]> * rename to a new plugin name Signed-off-by: Tianle Huang <[email protected]> * rename Signed-off-by: Tianle Huang <[email protected]> * rename more Signed-off-by: Tianle Huang <[email protected]> * sync changelog from main Signed-off-by: Tianle Huang <[email protected]> * onboard to app config Signed-off-by: Tianle Huang <[email protected]> * fix comment Signed-off-by: Tianle Huang <[email protected]> * update yml Signed-off-by: Tianle Huang <[email protected]> * update readme Signed-off-by: Tianle Huang <[email protected]> * update change log Signed-off-by: Tianle Huang <[email protected]> * call out single quotes in readme Signed-off-by: Tianle Huang <[email protected]> * update yml Signed-off-by: Tianle Huang <[email protected]> * update default Signed-off-by: Tianle Huang <[email protected]> * add reference link Signed-off-by: Tianle Huang <[email protected]> * update js doc Signed-off-by: Tianle Huang <[email protected]> * rename Signed-off-by: Tianle Huang <[email protected]> * use new name Signed-off-by: Tianle Huang <[email protected]> * redo changelog update Signed-off-by: Tianle Huang <[email protected]> * remove link Signed-off-by: Tianle Huang <[email protected]> * better name Signed-off-by: Tianle Huang <[email protected]> --------- Signed-off-by: Tianle Huang <[email protected]> (cherry picked from commit 58fb588) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* support dynamic csp rules to mitigate clickjacking * add unit tests for the provider class * move request handler to its own class * add license headers * fix failed unit tests * add unit tests for the handler * add content to read me * fix test error * update readme * update CHANGELOG.md * update snap tests * update snapshots * fix a wrong import * undo changes in listing snap * improve wording * set client after default client is created * update return value and add a unit test * remove unnecessary dependency * make the name of the index configurable * expose APIs and update file structures * add header * fix link error * fix link error * add more unit tests * add more unit tests * update api path * remove logging * update path * rename index name * update wording * make the new plugin disabled by default * do not update defaults to avoid breaking change * update readme to reflect new API path * update handler to append frame-ancestors conditionally * update readme * clean up code to prepare for application config * reset change log * reset change log again * update accordingly to new changes in applicationConfig * update changelog * rename to a new plugin name * rename * rename more * sync changelog from main * onboard to app config * fix comment * update yml * update readme * update change log * call out single quotes in readme * update yml * update default * add reference link * update js doc * rename * use new name * redo changelog update * remove link * better name --------- (cherry picked from commit 58fb588) Signed-off-by: Tianle Huang <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

github-actions bot added the first-time-contributor label Dec 23, 2023

tianleh changed the title ~~support dynamic csp rules to mitigate clickjacking~~ Support dynamic CSP rules to mitigate clickjacking Dec 29, 2023

tianleh marked this pull request as ready for review December 29, 2023 04:26

tianleh requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, BSFishy, curq, bandinib-amzn and SuZhou-Joe as code owners December 29, 2023 04:26

ruanyl reviewed Dec 30, 2023

View reviewed changes

src/plugins/csp_configuration_provider/server/plugin.ts Outdated Show resolved Hide resolved

src/plugins/csp_configuration_provider/server/types.ts Outdated Show resolved Hide resolved

SuZhou-Joe reviewed Jan 2, 2024

View reviewed changes

src/plugins/csp_configuration_provider/server/provider.ts Outdated Show resolved Hide resolved

tianleh force-pushed the cj-mitigation branch from 8dbe9d8 to fa125ac Compare January 3, 2024 01:30

kavilla added the backport 2.x label Jan 3, 2024

kavilla reviewed Jan 5, 2024

View reviewed changes

src/plugins/csp_configuration_provider/README.md Outdated Show resolved Hide resolved

tianleh added 24 commits March 8, 2024 14:35

clean up code to prepare for application config

4dbcc21

Signed-off-by: Tianle Huang <[email protected]>

reset change log

22e5394

Signed-off-by: Tianle Huang <[email protected]>

reset change log again

27ce0b1

Signed-off-by: Tianle Huang <[email protected]>

update accordingly to new changes in applicationConfig

d55b459

Signed-off-by: Tianle Huang <[email protected]>

update changelog

9f371e7

Signed-off-by: Tianle Huang <[email protected]>

rename to a new plugin name

54a5db1

Signed-off-by: Tianle Huang <[email protected]>

rename

0e9e146

Signed-off-by: Tianle Huang <[email protected]>

rename more

b434169

Signed-off-by: Tianle Huang <[email protected]>

sync changelog from main

e67af3c

Signed-off-by: Tianle Huang <[email protected]>

onboard to app config

2d12575

Signed-off-by: Tianle Huang <[email protected]>

fix comment

ef3a55d

Signed-off-by: Tianle Huang <[email protected]>

update yml

f2ac166

Signed-off-by: Tianle Huang <[email protected]>

update readme

f8dc3b4

Signed-off-by: Tianle Huang <[email protected]>

update change log

0c6a1cc

Signed-off-by: Tianle Huang <[email protected]>

call out single quotes in readme

78004c9

Signed-off-by: Tianle Huang <[email protected]>

update yml

3dc2024

Signed-off-by: Tianle Huang <[email protected]>

update default

140375f

Signed-off-by: Tianle Huang <[email protected]>

add reference link

e31cc9b

Signed-off-by: Tianle Huang <[email protected]>

update js doc

688f65a

Signed-off-by: Tianle Huang <[email protected]>

rename

c99b02f

Signed-off-by: Tianle Huang <[email protected]>

use new name

68f3ea2

Signed-off-by: Tianle Huang <[email protected]>

redo changelog update

8c8eb0f

Signed-off-by: Tianle Huang <[email protected]>

remove link

78d8b32

Signed-off-by: Tianle Huang <[email protected]>

better name

fcc9875

Signed-off-by: Tianle Huang <[email protected]>

bandinib-amzn force-pushed the cj-mitigation branch from fa8bcd6 to fcc9875 Compare March 8, 2024 22:35

ZilongX approved these changes Mar 8, 2024

View reviewed changes

ZilongX merged commit 58fb588 into opensearch-project:main Mar 8, 2024
68 checks passed

opensearch-trigger-bot bot mentioned this pull request Mar 8, 2024

[Backport 2.x] Support dynamic CSP rules to mitigate clickjacking #6101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dynamic CSP rules to mitigate clickjacking #5641

Support dynamic CSP rules to mitigate clickjacking #5641

tianleh commented Dec 23, 2023 •

edited

Loading

codecov bot commented Dec 23, 2023 •

edited

Loading

tianleh commented Dec 29, 2023

tianleh commented Dec 29, 2023

tianleh commented Jan 3, 2024

seraphjiang commented Jan 4, 2024

kavilla commented Jan 5, 2024

Support dynamic CSP rules to mitigate clickjacking #5641

Support dynamic CSP rules to mitigate clickjacking #5641

Conversation

tianleh commented Dec 23, 2023 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Test Case 1

Test Case 2

Check List

codecov bot commented Dec 23, 2023 • edited Loading

Codecov Report

tianleh commented Dec 29, 2023

tianleh commented Dec 29, 2023

tianleh commented Jan 3, 2024

seraphjiang commented Jan 4, 2024

kavilla commented Jan 5, 2024

tianleh commented Dec 23, 2023 •

edited

Loading

codecov bot commented Dec 23, 2023 •

edited

Loading