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

[Serverless][Security Solution][Endpoint] Remove use of hooks to check access to.lists-* for endpoint exceptions access #171412

Merged
merged 15 commits into from
Nov 22, 2023
Merged

[Serverless][Security Solution][Endpoint] Remove use of hooks to check access to.lists-* for endpoint exceptions access #171412

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
6344e93
use useListPrivileges to check access to `.lists-*` index
ashokaditya Nov 16, 2023
9a079cf
use write/read index privilege checks instead
ashokaditya Nov 16, 2023
4f4b65b
Merge branch 'main' into fix/dw-endpoint-exceptions-index-permissions…
ashokaditya Nov 16, 2023
2c02033
update logic to not use negation
ashokaditya Nov 20, 2023
fe9ee12
Merge branch 'main' into fix/dw-endpoint-exceptions-index-permissions…
ashokaditya Nov 20, 2023
f89170b
Merge branch 'main' into fix/dw-endpoint-exceptions-index-permissions…
ashokaditya Nov 21, 2023
394b1f0
remove need to check `.lists-*` privileges
ashokaditya Nov 21, 2023
6075930
Merge branch 'main' into fix/dw-endpoint-exceptions-index-permissions…
kibanamachine Nov 21, 2023
652a39d
Merge branch 'main' into fix/dw-endpoint-exceptions-index-permissions…
ashokaditya Nov 21, 2023
21abb7a
Merge branch 'main' into fix/dw-endpoint-exceptions-index-permissions…
ashokaditya Nov 22, 2023
da91adb
Merge branch 'main' into fix/dw-endpoint-exceptions-index-permissions…
ashokaditya Nov 22, 2023
31a94f7
Merge branch 'main' into fix/dw-endpoint-exceptions-index-permissions…
ashokaditya Nov 22, 2023
1938e6b
Merge branch 'main' into fix/dw-endpoint-exceptions-index-permissions…
ashokaditya Nov 22, 2023
82e3236
test for number of calls to `api/lists/index`
ashokaditya Nov 22, 2023
c025f9e
update test
ashokaditya Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions ...ns/security_solution/public/exceptions/hooks/use_endpoint_exceptions_capability/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,24 @@
*/

import { useMemo } from 'react';
import { useListsConfig } from '../../../detections/containers/detection_engine/lists/use_lists_config';
import { useListsPrivileges } from '../../../detections/containers/detection_engine/lists/use_lists_privileges';
import { useHasSecurityCapability } from '../../../helper_hooks';
import { useKibana } from '../../../common/lib/kibana';

export const useEndpointExceptionsCapability = (
capability: 'showEndpointExceptions' | 'crudEndpointExceptions'
) => {
const { loading: listsConfigLoading, needsConfiguration: needsListsConfiguration } =
useListsConfig();
): boolean => {
const { lists } = useKibana().services;
const { canManageIndex, loading: privilegesLoading } = useListsPrivileges();
const enabled = lists != null;
const needsIndexConfiguration = canManageIndex === false;
const needsConfiguration = !enabled || needsIndexConfiguration;
const hasAccessToLists = !(privilegesLoading || needsConfiguration);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it was a partial copy/paste from useListsConfig, e.g. needsConfiguration had a more comprehensive definition originally, and as such isn't quite correct here. But that logic was also specific to creating the lists index, and I think you could eliminate it entirely here (it doesn't matter whether they could create the index, we're not trying to do that here):

Suggested change
const enabled = lists != null;
const needsIndexConfiguration = canManageIndex === false;
const needsConfiguration = !enabled || needsIndexConfiguration;
const hasAccessToLists = !(privilegesLoading || needsConfiguration);
const listsEnabled = lists != null;
const hasListsAccess = canManageIndex;
const canUseLists = !privilegesLoading && listsEnabled && hasListsAccess

However, I had a question about the definition of hasListsAccess, since the above shows more clearly that this is centered around the canManageIndex privilege: If this capability requires reading/writing lists data, then canReadIndex and canWriteIndex from useListsPrivileges are what you want, here: manage has no relation to that concept.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for looking into this @rylnd. I do think using canReadIndex and canWriteIndex makes more sense than the manage privilege. I'll make that change.

Copy link
Member Author

Choose a reason for hiding this comment

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

9a079cf

Copy link
Member Author

Choose a reason for hiding this comment

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

and 2c02033

Copy link
Member Author

Choose a reason for hiding this comment

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

@rylnd @kqualters-elastic @yctercero I'm now wondering if there's a way to verify index privileges on the server side for API requests. I'm testing with a role that has read access to .lists-* index and also has endpoint_exceptions_all kibana privilege, such as

{
    "metadata": {},
    "elasticsearch": {
        "cluster": [],
        "indices": [
            {
                "names": [
                    ".items-*",
                    ".alerts-security.alerts-*"
                ],
                "privileges": [
                    "view_index_metadata",
                    "read",
                    "write",
                    "manage"
                ],
                "field_security": {
                    "grant": [
                        "*"
                    ],
                    "except": []
                },
                "allow_restricted_indices": false
            },
            {
                "names": [
                    ".lists-*"
                ],
                "privileges": [
                    "read",
                    "manage",
                    "view_index_metadata"
                ],
                "field_security": {
                    "grant": [
                        "*"
                    ],
                    "except": []
                },
                "allow_restricted_indices": false
            }
        ],
        "run_as": []
    },
    "kibana": [
        {
            "base": [],
            "feature": {
                "siem": [
                    "minimal_all",
                    "endpoint_list_all",
                    "endpoint_exceptions_all"                
                 ]
            },
            "spaces": [
                "*"
            ]
        }
    ]
}

which does restrict the role from doing any CUD operations on the UX as expected.

However, on the API side the role is able to do CUD operations, since endpoints exceptions are added to saved objects and now I'm wondering if we need index access for it at all? Or is there a way to restrict saved objects access using index privileges?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ashokaditya I think there's a lot of context here that I'm missing. Is there a ticket describing the behavior that you're trying to achieve here? Is this a bugfix that's triggered a permissions discussion?

Broadly, though: we typically are not proactive with ES permissions checking on the server, and instead attempt to perform the requested action as the user requesting it. If an error is raised within ES, we will surface that as part of the response.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks again for looking into this @rylnd . Apologies for not providing context. The idea is to restrict endpoint exceptions for serverless users. Sepecifically when a user doesn't have any endpoint PLI (essentials/complete) they should not be able to access endpoint exceptions. Here is the first PR that introduced this check.

After talking to my team, looks like we don't need the index privilege check at all, much like we don't for other endpoint artefacts (trusted apps, event filters etc.). We drive this purely via kibana privileges. So in the end, we don't need to use the useListsPrivileges for checking permissions to .lists-* indices.

Copy link
Member Author

Choose a reason for hiding this comment

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

@paul-tavares @dasansol92 @semd ☝🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

@kqualters-elastic @yctercero looks like we might need to look into this a bit more. "Endpoint exceptions" is not similar to endpoint artefacts and so I'm not so sure if access to it should be similar to endpoint artefacts. I'm going to try and find a time so we can quickly discuss this offline.

Copy link
Member Author

Choose a reason for hiding this comment

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

I spoke with @vitaliidm offline and looks like endpoint exceptions do not need .lists-* permissions.


const hasEndpointExceptionCapability = useHasSecurityCapability(capability);

return useMemo(
() => !listsConfigLoading && !needsListsConfiguration && hasEndpointExceptionCapability,
[hasEndpointExceptionCapability, listsConfigLoading, needsListsConfiguration]
() => hasAccessToLists && hasEndpointExceptionCapability,
[hasEndpointExceptionCapability, hasAccessToLists]
);
};