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.

Sign up for GitHub

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

[Security Solution] split endpoint rbac feature flags #143991

Merged
merged 1 commit into from
Oct 28, 2022
Merged

[Security Solution] split endpoint rbac feature flags #143991

merged 1 commit into from
Oct 28, 2022

Conversation

joeypoon
Copy link
Member

Summary

Add endpointRbacV1Enabled feature flag that only enables RBAC for response actions.

Checklist

For maintainers

@joeypoon joeypoon added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.6.0 ci:cloud-deploy Create or update a Cloud deployment labels Oct 25, 2022
@joeypoon joeypoon marked this pull request as ready for review October 26, 2022 00:19
@joeypoon joeypoon requested review from a team as code owners October 26, 2022 00:19
@joeypoon joeypoon requested review from pzl and parkiino October 26, 2022 00:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

dasansol92
dasansol92 reviewed Oct 26, 2022
View reviewed changes
Copy link
Contributor

@dasansol92 dasansol92 left a comment

Choose a reason for hiding this comment

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

Looking great! Just left two questions, let me know what you think! 🙂

x-pack/plugins/security_solution/common/endpoint/service/authz/authz.test.ts
@@ -115,6 +115,10 @@ describe('Endpoint Authz service', () => {
});

describe('and endpoint rbac is enabled', () => {
beforeEach(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

the outer describe has superuser so we're ensuring this test block doesn't.

...curity_solution/public/common/components/user_privileges/endpoint/use_endpoint_privileges.ts
@@ -57,7 +58,7 @@ export const useEndpointPrivileges = (): Immutable<EndpointPrivileges> => {
licenseService,
fleetAuthz,
userRoles,
isEndpointRbacEnabled,
isEndpointRbacEnabled || isEndpointRbacV1Enabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add another param here instead of using an OR in the existing one. Doing that, we can check if one or the other is enabled in authz and then use one or the other depending on the feature. For example, I don't want to use privileges for Trusted Apps if only the isEndpointRbacV1Enabled is enabled. Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

is there a use case for this? I purposely didn't want to over engineer it since we have pretty known use cases. since we added FF for the sake of privileges, it feels a bit odd to account for using the FF without privileges. this approach (in conjunction with the changes in authz.ts) controls privileges through the availability of subfeatures. since isEndpointRbacV1Enabled doesn't contain trusted apps subfeature, for example, trusted apps privilege would be false if only isEndpointRbacV1Enabled is enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think from the change here it will be evaluated if one of both is enabled. So for a user having isEndpointRbacV1Enabled set to true, we will evaluate privileges for Trusted Apps, Event Filters or others. And we should return false as it is not supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Spoke offline, the control is handled by the change made in features.ts. So we are ok with this! Thanks for the clarification @joeypoon

x-pack/plugins/security_solution/common/endpoint/service/authz/authz.ts
}

// not superuser and FF not enabled, no access
if (!isEndpointRbacEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As I wrote below, I think we should check here the specific FF depending on the feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

see #143991 (comment)

@joeypoon joeypoon force-pushed the feature/rbac-split-flags branch 2 times, most recently from 83d8660 to 00cbd84 Compare October 26, 2022 20:38
@kevinlog
Copy link
Contributor

Checked it out and tried it - works great!

I can see the only the v1 set of RBAC features with the new feature flag set:
image

The response console shows the permitted actions only:
image

This version of the role has Host Isolation disabled:
image

The Endpoint management view are still blocked by superuser access as expected:
image

LGTM!

kevinlog
kevinlog approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@kevinlog kevinlog left a comment

Choose a reason for hiding this comment

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

Just fix the merge conflict and 🚢

@joeypoon joeypoon force-pushed the feature/rbac-split-flags branch from 00cbd84 to 22f73fe Compare October 28, 2022 14:36
@joeypoon joeypoon enabled auto-merge (squash) October 28, 2022 14:37
@kibana-ci
Copy link
Collaborator

kibana-ci commented Oct 28, 2022 •
edited
Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #4 / Timeline search and filters Update kqlMode for timeline "before all" hook for "should be able to update timeline kqlMode with filter"

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 9.6MB 9.6MB +83.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 50.6KB 50.6KB +25.0B

History

  • 💛 Build #82982 was flaky 00cbd844bb8dc214dcd8d15384fb980927cc0e44
  • 💚 Build #82682 succeeded 8a208143758889d1d433784046f3008a92697396

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@joeypoon joeypoon merged commit c5b1afd into elastic:main Oct 28, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 28, 2022
@joeypoon joeypoon deleted the feature/rbac-split-flags branch October 28, 2022 15:49
This was referenced Nov 14, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinlog kevinlog kevinlog approved these changes

@dasansol92 dasansol92 dasansol92 approved these changes

@pzl pzl Awaiting requested review from pzl pzl was automatically assigned from elastic/security-onboarding-and-lifecycle-mgt

@parkiino parkiino Awaiting requested review from parkiino parkiino was automatically assigned from elastic/security-onboarding-and-lifecycle-mgt

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joeypoon @elasticmachine @kevinlog @kibana-ci @dasansol92 @kibanamachine