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.

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

Conversation

ashokaditya
Copy link
Member

@ashokaditya ashokaditya commented Nov 16, 2023

Summary

Instead of using useListsConfig this PR uses useListPrivileges to verify access to .lists-* index pattern.

follow up of /pull/165613
related /pull/170671 (closed in favour of this)
fixes /issues/169268

Checklist

@ashokaditya ashokaditya force-pushed the fix/dw-endpoint-exceptions-index-permissions-check-169268 branch from 3981464 to 6344e93 Compare November 16, 2023 15:39
@ashokaditya ashokaditya marked this pull request as ready for review November 16, 2023 15:40
@ashokaditya ashokaditya requested a review from a team as a code owner November 16, 2023 15:40
Copy link
Contributor

@e40pud e40pud left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 18 to 21
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.

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.

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.

@ashokaditya ashokaditya requested a review from rylnd November 16, 2023 22:05
@ashokaditya
Copy link
Member Author

@elasticmachine merge upstream

@ashokaditya ashokaditya enabled auto-merge (squash) November 22, 2023 14:29
@ashokaditya ashokaditya disabled auto-merge November 22, 2023 14:32
@ashokaditya ashokaditya requested a review from a team as a code owner November 22, 2023 14:35
@ashokaditya ashokaditya enabled auto-merge (squash) November 22, 2023 14:38
@kibana-ci
Copy link
Collaborator

kibana-ci commented Nov 22, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-test-helpers-test-utils 9 - -9
@kbn/securitysolution-ecs 337 - -337
expressionHeatmap 108 - -108
total -454

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id before after diff
@kbn/securitysolution-ecs 1 - -1

Async chunks

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

id before after diff
securitySolution 12.8MB 12.8MB +6.0KB
securitySolutionServerless 330.6KB 336.9KB +6.3KB
total +12.3KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/securitysolution-ecs 32 - -32
expressionHeatmap 2 - -2
total -34

Page load bundle

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

id before after diff
securitySolutionServerless 41.7KB 41.7KB -1.0B
Unknown metric groups

API count

id before after diff
@kbn/core-test-helpers-test-utils 9 - -9
@kbn/securitysolution-ecs 341 - -341
expressionHeatmap 112 - -112
total -462

ESLint disabled line counts

id before after diff
@kbn/securitysolution-ecs 1 - -1
expressionHeatmap 1 - -1
total -2

References to deprecated APIs

id before after diff
expressionHeatmap 3 - -3

Total ESLint disabled count

id before after diff
@kbn/securitysolution-ecs 1 - -1
expressionHeatmap 1 - -1
total -2

History

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

cc @ashokaditya

@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 22, 2023
…k to check access to`.lists-*` (elastic#171412)

## Summary

Instead of using `useListsConfig` this PR uses `useListPrivileges` to
verify access to `.lists-*` index pattern.

follow up of elastic/pull/165613
related elastic/pull/170671 (closed in favour of this)
fixes elastic/issues/169268

### Checklist

- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 8c3322e)
@ashokaditya ashokaditya deleted the fix/dw-endpoint-exceptions-index-permissions-check-169268 branch November 22, 2023 19:31
@ashokaditya ashokaditya changed the title [Serverless][Security Solution][Endpoint] use useListPrivileges hook to check access to.lists-* [Serverless][Security Solution][Endpoint] remove use of hooks to check access to.lists-* for endpoint exceptions access Nov 22, 2023
@ashokaditya ashokaditya changed the title [Serverless][Security Solution][Endpoint] remove use of hooks to check access to.lists-* for endpoint exceptions access [Serverless][Security Solution][Endpoint] Remove use of hooks to check access to.lists-* for endpoint exceptions access Nov 22, 2023
janmonschke pushed a commit to janmonschke/kibana that referenced this pull request Nov 23, 2023
…to check access to`.lists-*` for endpoint exceptions access (elastic#171412) (elastic#171794)

# Backport

This will backport the following commits from `main` to `8.11`:
- [[Serverless][Security Solution][Endpoint] Remove use of hooks to
check access to`.lists-*` for endpoint exceptions access
(elastic#171412)](elastic#171412)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT
[{"author":{"name":"Ash","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-11-22T19:10:50Z","message":"[Serverless][Security
Solution][Endpoint] Remove use of hooks to check access to`.lists-*` for
endpoint exceptions access (elastic#171412)\n\n## Summary\r\n\r\nInstead of
using `useListsConfig` this PR uses `useListPrivileges` to\r\nverify
access to `.lists-*` index pattern.\r\n\r\nfollow up of
elastic/pull/165613\r\nrelated elastic/pull/170671 (closed
in favour of this)\r\nfixes elastic/issues/169268\r\n\r\n###
Checklist\r\n\r\n- [ ] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<[email protected]>","sha":"8c3322ed44ccfbc4e91e0e9ef31f77b79c549cb8","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","OLM
Sprint","v8.11.0","v8.12.0","v8.11.1"],"number":171412,"url":"https://github.com/elastic/kibana/pull/171412","mergeCommit":{"message":"[Serverless][Security
Solution][Endpoint] Remove use of hooks to check access to`.lists-*` for
endpoint exceptions access (elastic#171412)\n\n## Summary\r\n\r\nInstead of
using `useListsConfig` this PR uses `useListPrivileges` to\r\nverify
access to `.lists-*` index pattern.\r\n\r\nfollow up of
elastic/pull/165613\r\nrelated elastic/pull/170671 (closed
in favour of this)\r\nfixes elastic/issues/169268\r\n\r\n###
Checklist\r\n\r\n- [ ] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<[email protected]>","sha":"8c3322ed44ccfbc4e91e0e9ef31f77b79c549cb8"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/171412","number":171412,"mergeCommit":{"message":"[Serverless][Security
Solution][Endpoint] Remove use of hooks to check access to`.lists-*` for
endpoint exceptions access (elastic#171412)\n\n## Summary\r\n\r\nInstead of
using `useListsConfig` this PR uses `useListPrivileges` to\r\nverify
access to `.lists-*` index pattern.\r\n\r\nfollow up of
elastic/pull/165613\r\nrelated elastic/pull/170671 (closed
in favour of this)\r\nfixes elastic/issues/169268\r\n\r\n###
Checklist\r\n\r\n- [ ] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<[email protected]>","sha":"8c3322ed44ccfbc4e91e0e9ef31f77b79c549cb8"}}]}]
BACKPORT-->

Co-authored-by: Ash <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] useListsConfig performs many repeated /api/lists/index calls
7 participants