-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security Solution] Endpoint RBAC integration with AppFeatures architecture #158646
Conversation
This reverts commit 1d24002.
It happens when plugins.forceEnableAllPlugins: true
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/fleet (Team:Fleet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some initial feedback and questions. Going to do some manual testing now and will report back
...curity_solution/public/common/components/user_privileges/endpoint/use_endpoint_privileges.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/endpoint/endpoint_app_context_services.ts
Show resolved
Hide resolved
/** | ||
* Enables Endpoint Response Actions like isolate host, trusted apps, blocklist, etc. | ||
*/ | ||
endpointResponseActions = 'endpoint_response_actions', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these values final? or just preliminary until we get around to creating them in accordance to the PLI spreadsheet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These appFeatures represent the PLI in the spreadsheet, they are final. However, I am not familiar enough with the endpoint logic to be 100% sure I implemented the correct scope, if you guys think we should include something else, or something less, or change the names of the flags, I'll be happy to adapt it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. We'll likely adjust it. I'm thinking we create an individual item for each response action rather than combine them all into one. that gives us the freedom to ensure that future Response actions can be used in different PLI's if any new ones come up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be only one AppFeatureKey
for each PLI in the spreadsheet, the granularity should be configured at the Kibana feature level here:
kibana/x-pack/plugins/security_solution/server/lib/app_features/security_kibana_features.ts
Lines 170 to 198 in 4d9f0c3
[AppFeatureSecurityKey.endpointResponseActions]: { | |
subFeatureIds: endpointResponseActionsSubFeatureIds, | |
subFeaturesPrivileges: [ | |
{ | |
id: 'host_isolation_all', | |
api: [`${APP_ID}-writeHostIsolation`], | |
ui: ['writeHostIsolation'], | |
}, | |
], | |
}, | |
[AppFeatureSecurityKey.endpointExceptions]: { | |
subFeatureIds: [SecuritySubFeatureId.trustedApplications, SecuritySubFeatureId.blocklist], | |
subFeaturesPrivileges: [ | |
{ | |
id: 'host_isolation_exceptions_all', | |
api: [ | |
`${APP_ID}-accessHostIsolationExceptions`, | |
`${APP_ID}-writeHostIsolationExceptions`, | |
], | |
ui: ['accessHostIsolationExceptions', 'writeHostIsolationExceptions'], | |
}, | |
{ | |
id: 'host_isolation_exceptions_read', | |
api: [`${APP_ID}-accessHostIsolationExceptions`], | |
ui: ['accessHostIsolationExceptions'], | |
}, | |
], | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fleet change LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sergi,
Thanks for all of the changes and suggestions. Since we continue to have some differences in implementation approach, I'm going to ask that someone else on my team take on this review.
I did some limited testing focusing only on non-serverless and here is what I found:
- The HIE page no longer shows "permission needed" UI when using the URL to access the page. It seems to just redirect to the Endpoint List page, which is also problematic since a user might not have access to the endpoint list.
- In a license downgrade scenario, the Lists api no longer returns 403 when doing a GET when no data exists. This is different from the prior release and inconsistent with other APIs when user does not have Authz.
Perhaps these changes are fine with others, but I figure I would raise it since they differ from current implementation. Perhaps Kevin/Caitlin can confirm.
I was not able to do migration testing to ensure that prior user created Roles (created in 8.8) continue to work as they did after upgrading to 8.9. My main goal in this testing is to ensure that the newly added kibana sub-features (writeHostIsolationRelease
, deleteHostIsolationExceptions
, accessHostIsolationExceptions
) do not impact any role that might have been created in 8.9 by users. I understand that you indicated there is no impact, but I think we should still confirm.
x-pack/plugins/security_solution/server/endpoint/endpoint_app_context_services.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick test based on some of the changes here, mostly isolation/release and HIE access. I'll test more and report back, but for now, here's what I observed.
ESS
So I tested this out on ESS side and I created a role with HIE read and logged in with a below platinum license and I don't see a delete action for the HIE entry. Also, upgrading the license to platinum/enterprise didn't change this either. I still don't see a delete action with HIE Read access when an HIE entry exists for a non-superuser.
Also, I don't like that when trying to access a restricted page/link it doesn't show me the privileges required callout but instead tries to redirect the endpoint page.
update:
The above is as expected. I do need to have "write" access to be able to delete entries without a proper license.
Serverless:
It does work as expected on the serverless side with license downgrades and I can see the delete action on HIE entries. However, I got a not found error on isolate/release actions on the console for an endpoint. On the Endpoint list isolate/release action flyout I get a error callout but no info on the error.
I have a question regarding PLIs. What should be the behavior that should be tested when product_tiers
are not the same value? For instance,
xpack.serverless.security.productTypes:
[
{ product_line: 'security', product_tier: 'essentials' },
{ product_line: 'endpoint', product_tier: 'complete' },
]
Is that something that needs to be considered/tested?
x-pack/plugins/security_solution/public/management/links.test.ts
Outdated
Show resolved
Hide resolved
Hi @paul-tavares @ashokaditya, Kibana subFeaturesThese changes do NOT introduce any new Kibana subFeature, it uses only existing Kibana subFeatures. The way the new kibana/x-pack/plugins/security_solution/server/lib/app_features/security_kibana_features.ts Lines 183 to 197 in 4d9f0c3
For others like "Trusted Applications" or "Blocklist" (which do not have combined payment/free privileges) the entire subFeature is registered when the "Endpoint Exceptions" PLI is enabled, and it is not configured otherwise. kibana/x-pack/plugins/security_solution/server/lib/app_features/security_kibana_features.ts Line 182 in 4d9f0c3
No MigrationNo migration is needed using this approach because only the subFeatures IDs ( It is worth mentioning that Roles will keep subFeature IDs stored at ES even if the subFeature is no longer registered in Kibana Features. Meaning that we can pre-configure roles with a set of subFeatures enabled and they will be granted only when PLIs are enabled. Or in a downgrade + upgrade scenario, the privileges will be granted again in a transparent way. Does this help? |
This is not a valid scenario for the MVP. However, Mike P. asked to not limit the config in case they want to introduce this possibility in the future, so my guess is we can ignore this scenario for now. We can worry about it later if they finally decide to allow it.
I am not very familiar with the Endpoint console. Do you know if the bug is caused by the |
Yes, the user will have authz for read HIE without license, it will be restricted only by role, it is consistent because we are removing the "has data" check from the authz in this PR. I explained this change in a previous comment: Consistency: This flag is used to grant read authorization without a license when there is data to read, it makes sense for the downgrade scenario, but that means we are "de facto" allowing reading without a license. The only scenario in which the API will return an error is when there is no data to read, which is a bit inconsistent. The only change I am introducing is the API will return an empty array instead of an error when there's no data. I think this way is more consistent and it does not represent any issue compared with what we have now. About this:
That's right, I changed that to stay consistent with other pages here: 6a8f13e However, the "NoPrivilege" page makes sense in the scenario the user is not authorized because of its role, but it does not make sense when the Role has the subFeature enabled and it is still unauthorized because of the license (or productType in serverless), we should show an upsell message in this scenario, like the one we have in Entity Analytics: This difference can be solved using the Upselling component registry we put together along with the new AppFeatures, it allows registering Serverless (based on PLI) and also ESS (based on license) upsell components, so we'll be able to cover all situations. However, in order to not overcomplicate this PR, this will be done in a follow-up PR. |
Got it.
I don't think it is cause of the authz, as the other response actions work fine. I believe it is because the isolate/unisolate APIs are redirected from |
Thanks @semd for your comments above. Specifically the one that describes the approach and the section around migration not being needed. The explanation around how user roles are stored and what they contain was helpful for me. |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked it out for both ESS and serverless, and it works as expected.
💛 Build succeeded, but was flaky
Failed CI Steps
Test Failures
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
This PR adapts the endpoint RBAC to the new Serverless PLI features architecture.
The changes are the following:
App Features
New appFeatures keys for endpoint
The
endpointExceptions
PLI has been added to the Endpoint Essentials product tier andendpointResponseActions
to the Endpoint Completekibana/x-pack/plugins/serverless_security/common/pli/pli_config.ts
Lines 20 to 23 in 686bc2e
Endpoint appFeatures capabilities config
The features configuration for each appFeature (PLI) has been added. They will be configured within the Security Kibana features only when the appFeature is enabled by the selected Security product type. (Note that all of them will be always added in regular ESS deployments, only in Serverless we'll have different product types)
kibana/x-pack/plugins/security_solution/server/lib/app_features/security_kibana_features.ts
Lines 170 to 198 in 4d9f0c3
These are the capabilities that seemed relevant to me for each PLI, but I don't have enough expertise in Endpoint operations to know for sure what Kibana sub-features and capabilities need to be included in each appFeature. The PLIs are in a private spreadsheet with the following descriptions.
endpointExceptions:
endpointResponseActions:
I'll need Endpoint team members to confirm there's no missing or wrong capability in each appFeature config.
Host isolation capabilities
It is important to mention that in the configuration above, to have some capabilities available we are adding some sub-features directly using the
subFeatureIds
entry, but for host_isolation capabilities, we are doing it in a slightly different way, using thesubFeaturesPrivileges
, this way the privileges are added to existing subFeatures.The reason is we need to have the write (isolate operation) only in payment product types, but the read and delete (release operation) capabilities should be always available, to allow releasing previously isolated hosts after a product downgrade.
To do this we always include the
host_isolation_all
andhost_isolation_exceptions_all
subFeatures in the base configuration, but they only contain read and delete capabilities by default, only when the product tier allows the proper appFeatures the write capability is added to the same subFeatures privileges.Endpoint Authz module
Remove "superuser" specific check
This specific check:
Has been removed, this has no behavioral impact, superuser has all capabilities enabled anyway.
Remove usage of
endpointRbacEnabled
andendpointRbacV1Enabled
experimental flagsThey are already enabled by default. superuser will still have the authorization to access all the features. The only change is the endpoint sub-features will always be visible in the Kibana Privilege section of the Role management page, they were hidden when these experimental flags were disabled.
Remove double write check for read authorizations:
We were doing unnecessary checks for the write capabilities in the read authorizations, like:
const canReadEndpointList = canWriteEndpointList || hasKibanaPrivilege(fleetAuthz, 'readEndpointList');
. Sub-features already add read and write capabilities on theall
privilege, so these double checks were unnecessary.Extract
hasHostIsolationExceptionsItems
flagThis flag was used to grant read and delete authorization for Host Isolation Exceptions (HIE) when there is data, basically turning them free features when there is data to perform the actions. This is needed to allow users to remove HIE after a license downgrade scenario, which is good.
However, we needed to do this API call from outside the auth module, in every place we needed to call
calculateEndpointAuthz
, and we were also adding the responsibility to do some auth-specific logic with licenses outside the auth module, which is not good.In addition, it is not very consistent to make authorization depend on the existence of data to perform an action. Authorization should be based only on the role capabilities and tiers/licenses, if some parts of the application want to show/hide stuff depending on the data, that's not the auth module's responsibility.
I checked all the places where we use the HIE read and delete authorizations, and the only place where we really need them to be denied (when there is no data) is in the links, we need to remove the HIE link from the app in this situation.
So, this PR moves the data check to the links.ts module, making the read and delete permissions always granted without a license (they will still be useless without data), the same way the
canUnIsolateHost
authorization works. And then doing the async data check to remove the HIE link in the management/links.ts module itself, only in the last case where we really need to know it:kibana/x-pack/plugins/security_solution/public/management/links.ts
Lines 257 to 262 in 4d9f0c3
This flag extraction is unrelated to the integration of the new architecture, I included it only to extract complexity from the authz module and simplify its usage, but this change can be rolled back if we consider it.
Testing
To start the application in ESS (non-serverless) mode, run it normally with
yarn start
. Everything should keep working as usual with all features available and capabilities should only be restricted by the user role.To start the application in Serverless mode run with
yarn serverless-security
. It sets a random root path, so access the main URL at "http://localhost:5601/" to be redirected.By default the "Endpoint Complete" product line is selected in the serverless.security.yml config, so everything should be available as in ESS with the default config.
kibana/config/serverless.security.yml
Lines 11 to 15 in 686bc2e
Once in Serverless mode, in order to see the difference between product types, we can change the Endpoint
product_tier
toessentials
, as per the pli_config, this change should remove all the capabilities included by theendpointResponseActions
appFeatures config.To check how the application behaves without the
endpointExceptions
PLI, we can remove the Endpointproduct_line
entirely from the product array, leaving the Securityproduct_line
alone.Next steps
Upselling page
The product upselling page has not been registered for endpoint pages in this PR, so when any of these pages are unauthorized because of the serverless product tier, and they are accessed directly by URL they still show the
Privileges required
screen.This is arguably not entirely correct. However, an upselling page can be registered to display a "Buy a higher tier" message when the privilege is denied because of the product type, if it is unauthorized because of the user role the "Privileges required" page will still show.
I did not include the endpoint upselling page in this PR to keep it simple, but the registry is already implemented in the main proposal, we can define and register them in a follow-up PR.
Superuser role in authz module
Almost all "superuser" role conditionals have been removed from the Endpoint authz module, but there is only one check left here:
kibana/x-pack/plugins/security_solution/common/endpoint/service/authz/authz.ts
Line 85 in 24330f2
This
canAccessEndpointManagement
flag looks deprecated, and it seems to be used incorrectly in the few places where it is checked. If we could fix the places that it is used, checking the proper authz flag, we could definitively remove theuserRoles
parameter from thecalculateEndpointAuthz
function, this will have an impact in the different places where this function is called since they will no longer need any async logic.