[workspace]feat: Add ACL auditor #8557

SuZhou-Joe · 2024-10-11T07:01:38Z

Description

This PR is mainly to add an auditor per request, if somehow a call to the saved objects client bypass the ACL, the auditor is supposed to audit a bypass error.

Issues Resolved

Screenshot

Testing the changes

Enabled permission control: savedObjects.permission.enabled: true
Enabled workspace: workspace.enabled: true
Claim a user as dashboard admin so that others will be regarded as normal user: opensearchDashboards.dashboardAdmin.users: ["admin"]
Clicking around, there should not be a message with pattern: [ACLCounterCheckoutFailed] counter state: xxx

Changelog

feat: Add ACL auditor

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

Signed-off-by: SuZhou-Joe <[email protected]>

codecov · 2024-10-11T07:31:16Z

Codecov Report

Attention: Patch coverage is 83.33333% with 21 lines in your changes missing coverage. Please review.

Project coverage is 60.96%. Comparing base (98df4dd) to head (2c29dce).
Report is 87 commits behind head on main.

Files with missing lines	Patch %	Lines
..._objects/workspace_saved_objects_client_wrapper.ts	80.00%	7 Missing and 4 partials ⚠️
src/core/server/utils/acl_auditor.ts	81.48%	2 Missing and 3 partials ⚠️
...rkspace/server/saved_objects/repository_wrapper.ts	75.00%	0 Missing and 3 partials ⚠️
src/core/server/utils/client_call_auditor.ts	87.50%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8557      +/-   ##
==========================================
+ Coverage   60.93%   60.96%   +0.03%     
==========================================
  Files        3769     3780      +11     
  Lines       89506    89973     +467     
  Branches    14012    14099      +87     
==========================================
+ Hits        54539    54853     +314     
- Misses      31557    31676     +119     
- Partials     3410     3444      +34

Flag	Coverage Δ
Linux_1	`29.24% <81.74%> (+0.24%)`	⬆️
Linux_2	`56.32% <84.09%> (+0.07%)`	⬆️
Linux_3	`37.80% <27.27%> (+0.04%)`	⬆️
Linux_4	`29.95% <27.27%> (+0.03%)`	⬆️
Windows_1	`29.26% <81.74%> (+0.24%)`	⬆️
Windows_2	`56.27% <84.09%> (+0.07%)`	⬆️
Windows_3	`37.80% <27.27%> (+0.04%)`	⬆️
Windows_4	`29.95% <27.27%> (+0.03%)`	⬆️

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.

Signed-off-by: SuZhou-Joe <[email protected]>

wanglam · 2024-10-11T08:59:45Z

src/plugins/workspace/server/permission_control/client.ts

@@ -66,6 +71,8 @@ export class SavedObjectsPermissionControl {
      savedObjectsToGet.length > 0
        ? (await this.getScopedClient?.(request)?.bulkGet(savedObjectsToGet))?.saved_objects || []
        : [];
+    // System request, -1 * savedObjectsToGet.length for compensation.
+    ACLAuditor?.increment(ACLAuditorStateKey.DATABASE_OPERATION, -1 * savedObjectsToGet.length);


How about add a decrement method?

I would like to keep the interface simple, increment with a negative value can give the same functionality here actually.

src/plugins/workspace/server/plugin.ts

src/plugins/workspace/server/saved_objects/repository_wrapper.ts

wanglam · 2024-10-11T09:31:42Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

@@ -485,10 +519,14 @@ export class WorkspaceSavedObjectsClientWrapper {
            false
          ))
        ) {
+          ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_FAILURE, 1);


Since we will throw error if not permitted, the sum of VALIDATE_FAILURE + VALIDATE_SUCCESS will always less than DATABASE_OPERATION? For example, we have 10 objects and increment 10 data source operation. We don't have permission to the last saved objects. We will only increment 1 VALIDATE_FAILURE and throw error if not permitted. Other permitted saved objects won't be recorded.

Actually if there is an error, the decorator will reset the auditor and won't checkout the record for this client call.

https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8557/files#diff-6f4daf9988f5262d2403821efd458faf7157c7e7473ec65a16ae2724a92d6556R693

Signed-off-by: SuZhou-Joe <[email protected]>

src/plugins/workspace/common/constants.ts

Signed-off-by: SuZhou-Joe <[email protected]>

src/core/server/utils/acl_auditor.ts

src/core/server/utils/client_call_auditor.ts

Hailong-am · 2024-10-12T04:00:52Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

+    const ACLAuditor = getACLAuditor(wrapperOptions.request);
+    const clientCallAuditor = getClientCallAuditor(wrapperOptions.request);


do we want to provide a flag to turn off aduit behavior?, could enabled by default.

I was thinking that but there are too many feature flags now and I'd prefer to use the permission control feature flag. The auditor behavior won't add any impact to the UI or the server.

Signed-off-by: SuZhou-Joe <[email protected]>

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

wanglam · 2024-10-12T04:40:45Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

+             * The catch here is required because unhandled promise will make server crashed,
+             * and we will reset the auditor state when catch an error.
+             */
+            () => {


Do we need to return Promise.reject here? Or the promise will be resolved after return.

No, we already return the result, the result will give the rejected error if the promise fails.

Signed-off-by: SuZhou-Joe <[email protected]>

* feat: enable acl auditor Signed-off-by: SuZhou-Joe <[email protected]> * Changeset file for PR #8557 created/updated * feat: optmize code Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize code Signed-off-by: SuZhou-Joe <[email protected]> * fix: update workspace metadata is giving error log Signed-off-by: SuZhou-Joe <[email protected]> * feat: update Signed-off-by: SuZhou-Joe <[email protected]> * feat: refactor clientCallAuditor Signed-off-by: SuZhou-Joe <[email protected]> * feat: add unit test Signed-off-by: SuZhou-Joe <[email protected]> * feat: update Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize code and wording Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize code and wording Signed-off-by: SuZhou-Joe <[email protected]> * feat: add comments Signed-off-by: SuZhou-Joe <[email protected]> * feat: update Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize comment Signed-off-by: SuZhou-Joe <[email protected]> * fix: type error in workspace_saved_objects_client_wrapper.test.ts Signed-off-by: SuZhou-Joe <[email protected]> * feat: update Signed-off-by: SuZhou-Joe <[email protected]> * feat: add unit test Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit c8f386e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* feat: enable acl auditor * Changeset file for PR #8557 created/updated * feat: optmize code * feat: optimize code * fix: update workspace metadata is giving error log * feat: update * feat: refactor clientCallAuditor * feat: add unit test * feat: update * feat: optimize code and wording * feat: optimize code and wording * feat: add comments * feat: update * feat: optimize comment * fix: type error in workspace_saved_objects_client_wrapper.test.ts * feat: update * feat: add unit test --------- (cherry picked from commit c8f386e) Signed-off-by: SuZhou-Joe <[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> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

* feat: enable acl auditor Signed-off-by: SuZhou-Joe <[email protected]> * Changeset file for PR opensearch-project#8557 created/updated * feat: optmize code Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize code Signed-off-by: SuZhou-Joe <[email protected]> * fix: update workspace metadata is giving error log Signed-off-by: SuZhou-Joe <[email protected]> * feat: update Signed-off-by: SuZhou-Joe <[email protected]> * feat: refactor clientCallAuditor Signed-off-by: SuZhou-Joe <[email protected]> * feat: add unit test Signed-off-by: SuZhou-Joe <[email protected]> * feat: update Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize code and wording Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize code and wording Signed-off-by: SuZhou-Joe <[email protected]> * feat: add comments Signed-off-by: SuZhou-Joe <[email protected]> * feat: update Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize comment Signed-off-by: SuZhou-Joe <[email protected]> * fix: type error in workspace_saved_objects_client_wrapper.test.ts Signed-off-by: SuZhou-Joe <[email protected]> * feat: update Signed-off-by: SuZhou-Joe <[email protected]> * feat: add unit test Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

feat: enable acl auditor

1020e3a

Signed-off-by: SuZhou-Joe <[email protected]>

github-actions bot added the distinguished-contributor label Oct 11, 2024

SuZhou-Joe added backport 2.x workspace distinguished-contributor and removed distinguished-contributor labels Oct 11, 2024

Changeset file for PR opensearch-project#8557 created/updated

c1811e6

SuZhou-Joe added 3 commits October 11, 2024 15:03

feat: optmize code

37dc2f5

Signed-off-by: SuZhou-Joe <[email protected]>

feat: optimize code

7a50119

Signed-off-by: SuZhou-Joe <[email protected]>

fix: update workspace metadata is giving error log

4db3c82

Signed-off-by: SuZhou-Joe <[email protected]>

feat: update

a45d361

Signed-off-by: SuZhou-Joe <[email protected]>

wanglam reviewed Oct 11, 2024

View reviewed changes

SuZhou-Joe added 2 commits October 11, 2024 18:10

feat: refactor clientCallAuditor

ab939ba

Signed-off-by: SuZhou-Joe <[email protected]>

feat: add unit test

57c023d

Signed-off-by: SuZhou-Joe <[email protected]>

Hailong-am reviewed Oct 11, 2024

View reviewed changes

src/plugins/workspace/common/constants.ts Outdated Show resolved Hide resolved

feat: update

86f165f

Signed-off-by: SuZhou-Joe <[email protected]>

Hailong-am reviewed Oct 12, 2024

View reviewed changes

SuZhou-Joe added 3 commits October 12, 2024 12:25

feat: optimize code and wording

4fb9841

Signed-off-by: SuZhou-Joe <[email protected]>

feat: optimize code and wording

e044a7e

Signed-off-by: SuZhou-Joe <[email protected]>

feat: add comments

7aa1d02

Signed-off-by: SuZhou-Joe <[email protected]>

wanglam reviewed Oct 12, 2024

View reviewed changes

SuZhou-Joe added 5 commits October 12, 2024 13:17

feat: update

29672e0

Signed-off-by: SuZhou-Joe <[email protected]>

feat: optimize comment

ae0c809

Signed-off-by: SuZhou-Joe <[email protected]>

fix: type error in workspace_saved_objects_client_wrapper.test.ts

db5afdd

Signed-off-by: SuZhou-Joe <[email protected]>

feat: update

fa9cdcb

Signed-off-by: SuZhou-Joe <[email protected]>

feat: add unit test

2c29dce

Signed-off-by: SuZhou-Joe <[email protected]>

Hailong-am approved these changes Oct 12, 2024

View reviewed changes

ruanyl approved these changes Oct 14, 2024

View reviewed changes

SuZhou-Joe merged commit c8f386e into opensearch-project:main Oct 14, 2024
67 of 68 checks passed

opensearch-trigger-bot bot mentioned this pull request Oct 14, 2024

[Backport 2.x] [workspace]feat: Add ACL auditor #8586

Merged

SuZhou-Joe added the v2.18.0 label Oct 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[workspace]feat: Add ACL auditor #8557

[workspace]feat: Add ACL auditor #8557

SuZhou-Joe commented Oct 11, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

wanglam Oct 11, 2024

SuZhou-Joe Oct 12, 2024

wanglam Oct 11, 2024

SuZhou-Joe Oct 12, 2024

Hailong-am Oct 12, 2024

SuZhou-Joe Oct 12, 2024

wanglam Oct 12, 2024

SuZhou-Joe Oct 12, 2024

		const ACLAuditor = getACLAuditor(wrapperOptions.request);
		const clientCallAuditor = getClientCallAuditor(wrapperOptions.request);

[workspace]feat: Add ACL auditor #8557

[workspace]feat: Add ACL auditor #8557

Conversation

SuZhou-Joe commented Oct 11, 2024

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2024 •

edited

Loading