[Osquery] RBAC #106669

patrykkopycinski · 2021-07-26T06:55:22Z

Summary

Adds RBAC for Osquery plugin

The idea is to allow the non-superusers to access Osquery independently from Fleet and also to allow for finer grain control over specific Osquery features.

Use case:
SOC Analyst 1 should be able to run only Saved queries (he should not be able to type the query directly as in some cases it may have an impact on the performance)

SOC Manager should be able to run any queries and also be able to setup Scheduled query groups (it's going to be renamed to packs, that's why the permission name is using that name)

elasticmachine · 2021-07-26T06:55:24Z

Pinging @elastic/security-asset-management (Team:Asset Management)

patrykkopycinski · 2021-07-26T13:44:57Z

@snide it's just Kibana management, there is nothing to review for @elastic/security-design

snide · 2021-07-26T15:52:08Z

@patrykkopycinski The @elastic/security-design team is likely the most knowledgable of OSQuery and the need for the management controls. The @elastic/kibana-design likely would have familiarity with the management controls themselves. Either way from your screens it looks like some design feedback might help.

Here's some quick feedback after checking it locally myself.

I'm guessing the UI controls were built out in another PR and this was just turning that UI on?

snide · 2021-07-26T16:05:30Z

Also. I think the UX in here can be a bit confusing. When setting up these controls they are essentially custom roles, rather than targeted ones. For example, I could set these controls in both "All" and "Read" but they would essentially be the same. Shouldn't it maybe read as "Custom"?

jportner · 2021-07-26T16:27:36Z

I'm guessing the UI controls were built out in another PR and this was just turning that UI on?

@snide These UI controls are generated programmatically by the Security plugin, based on the features and privileges that were registered by the consumer (in this case the Osquery plugin).

Thanks for the feedback, happy to address any and all UI concerns for the Privileges flyout but all of that (besides the sentence case comment) should probably be split out into a separate issue with the Team:Security label so it drops onto our project board.

Also. I think the UX in here can be a bit confusing. When setting up these controls they are essentially custom roles, rather than targeted ones. For example, I could set these controls in both "All" and "Read" but they would essentially be the same. Shouldn't it maybe read as "Custom"?

You're looking at the feature privilege. "Custom" is selected from the base privileges at the top.

snide · 2021-07-26T16:36:08Z

Valid that this is on the @elastic/kibana-security team. I'll write up an issue. The "sub-privileges" distinction vs. the predefined ones though seems like a pretty big UX hole especially when everything is collapsed.

jportner

It is not possible to comprehensively review this in a vacuum. I don't have any context around the Osquery plugin or what it is intended to do. It would be helpful to understand how Osquery is intended to be used (is it part of Fleet?)

Ultimately I think it's a bad idea to attempt to add feature privileges on their own, you should also be adding the corresponding controls to your plugin (lock down HTTP routes with api tags, dynamically change React components based on UI capabilities, and ideally add some amount of end-to-end integration tests and/or functional tests that exercise the different combinations of privileges that can be used).

A couple other things to note:

I just wanted to mention that sub-feature privileges are only available with a Gold license level by default. Just in case you are not aware; if you leave this as-is, you should ensure that Osquery on its own is usable with a Basic license because the sub-feature privileges will not be configurable.
If the Osquery feature itself is only intended to be available in a Gold+ license level, then you should specify that via the minimumLicense field.
At the moment, the Osquery plugin requires the Security plugin. That means if the Security plugin is disabled, the Osquery plugin will not be available. Is this intended?

Looking forward to chatting more about this and helping out any way I can. If it makes more sense to take this offline and have a conversation via Slack or something else, that's fine too, let me know!

I'm not my team's expert on Kibana Feature Privileges, that would be @legrego -- I'd like for him to take another pass on this and make sure I didn't miss anything.

jportner · 2021-07-28T20:05:34Z

x-pack/plugins/osquery/server/plugin.ts

+
+const registerFeatures = (features: SetupPlugins['features']) => {
+  features.registerKibanaFeature({
+    id: 'osquery',


Nit: not 100% necessary but I'd suggest replacing instances of 'osquery' and the like with constants.

jportner · 2021-07-28T20:07:38Z

x-pack/plugins/osquery/server/plugin.ts

+    name: i18n.translate('xpack.features.osqueryFeatureName', {
+      defaultMessage: 'Osquery',
+    }),
+    order: 1300,


Just FYI, the Dev Tools feature already has order: 1300. ~~If two features have the same order, they are sorted alphabetically in the Kibana Privilege UI.~~ This is probably fine for something in stack management, but it begs the question, where do you want this to go in the UI exactly? Depending on your answer, you may not need to change this 😄

Edit: I checked the code and actually, if two features have the same order, it appears there is no second-level sorting. So this may appear before or after Dev Tools. You should probably pick a different order but I will defer to @legrego since we don't have any validation on this field.

We want this feature to appear consistently within Kibana's primary navigation, and the feature listing on the spaces/role management screens. If we intend for Osquery to appear before Dev Tools, then this order should be less than that of Dev Tools. Similarly, if we want Osquery to appear after Dev Toos, then this order should be greater than that of Dev Tools. We don't want the order of any feature to collide with the order of another feature.

You're right that we don't have validation around this today, but that would be good for us to add to the features plugin.

x-pack/plugins/osquery/server/plugin.ts

jportner · 2021-07-28T21:12:44Z

x-pack/plugins/osquery/server/plugin.ts

+                  all: [savedQuerySavedObjectType],
+                  read: [savedQuerySavedObjectType],


You don't need to grant both all and read SO privileges for a given type:

Suggested change

all: [savedQuerySavedObjectType],

read: [savedQuerySavedObjectType],

all: [savedQuerySavedObjectType],

read: [],

jportner · 2021-07-28T21:12:51Z

x-pack/plugins/osquery/server/plugin.ts

+                  all: [packSavedObjectType],
+                  read: [packSavedObjectType],


You don't need to grant both all and read SO privileges for a given type:

Suggested change

all: [packSavedObjectType],

read: [packSavedObjectType],

all: [packSavedObjectType],

read: [],

x-pack/plugins/osquery/server/plugin.ts

legrego

I did a quick pass through -- I agree with all of @jportner's comments, and have a couple additional thoughts below. Happy to chat about this if that would be helpful!

x-pack/plugins/osquery/server/config.ts

legrego · 2021-07-29T11:53:36Z

x-pack/plugins/osquery/server/plugin.ts

+  features.registerKibanaFeature({
+    id: 'osquery',
+    name: i18n.translate('xpack.features.osqueryFeatureName', {
+      defaultMessage: 'Osquery',


I don't know much about osquery, but their website suggests that this is all lowercase (osquery rather than Osquery).

I assume, perhaps incorrectly, that osquery is short for operating system query. If that's true, then using sentence case for an initialism doesn't seem right to me, but I'll ultimately defer to @elastic/kibana-docs: Should this be osquery, Osquery, OSquery, or something else entirely?

On the Security side for the Osquery Manager docs, and the approach is to use osquery when referring to the open source tool but to use Osquery Manager when referring to our integration.

x-pack/plugins/osquery/server/plugin.ts

…rbac

…ery-rbac' of github.com:patrykkopycinski/kibana into feat/osquery-rbac

joshdover · 2021-08-09T15:29:07Z

src/core/server/saved_objects/migrationsv2/integration_tests/type_registrations.test.ts

@@ -71,7 +71,7 @@ const previouslyRegisteredTypes = [
  'ml-telemetry',
  'monitoring-telemetry',
  'osquery-saved-query',
-  'osquery-usage-metric',
+  'osquery-manager-usage-metric',


@patrykkopycinski if we released a Kibana version with this SO type name then we need to add the old name to the array here to delete the old objects when the user upgrades: https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/migrations/core/elastic_index.ts#L34

changed, Thank you @joshdover!

@patrykkopycinski You know what I forgot to mention that I think the old type name actually needs to be in both arrays or a test is going to fail that ensures that all items in REMOVED_TYPES is also contained in this previouslyRegisteredTypes array.

joshdover

Core changes LGTM

jportner

A few comments but otherwise it's looking good!

Also, I can't stress enough:

ideally add some amount of end-to-end integration tests and/or functional tests that exercise the different combinations of privileges that can be used

If you absolutely don't have time to do this now, it would be good to open an issue in your team's backlog so you can pay down this tech debt in the future.

jportner · 2021-08-09T17:34:53Z

x-pack/plugins/osquery/server/routes/action/create_action_route.ts

@@ -47,6 +52,8 @@ export const createActionRoute = (router: IRouter, osqueryContext: OsqueryAppCon
        return response.badRequest({ body: new Error('No agents found for selection') });
      }

+      // TODO: Add check for `runSavedQueries` only


Can you elaborate on this?

Right now there is no way for the user with runSavedQueries-only privileges to run custom query from UI, but in the future when we will make the public API we need to make sure that we validate the query the user tries to run is coming from the Saved Queries library

jportner · 2021-08-09T18:49:29Z

x-pack/plugins/osquery/server/routes/action/create_action_route.ts

-      const esClient = context.core.elasticsearch.client.asCurrentUser;
-      const soClient = context.core.savedObjects.client;
+      const esClient = context.core.elasticsearch.client.asInternalUser;
+      const soClient = await getInternalSavedObjectsClient(osqueryContext.getStartServices);


This change means that the internal user (Kibana system user) is used for accessing saved objects. This effectively bypasses any authZ checks and skips Kibana audit logs.

If I understand correctly, it looks like:

parseAgentSelection below should continue to use the current user for accessing saved objects

incrementCounter below should use the internal user for accessing saved objects

If so, this needs to be changed to something like:

const savedObjectsClient = context.core.savedObjects.client; const internalSavedObjectsClient = await getInternalSavedObjectsClient(osqueryContext.getStartServices); ... const selectedAgents = await parseAgentSelection( esClient, savedObjectsClient, osqueryContext, agentSelection ); incrementCount(internalSavedObjectsClient, 'live_query'); if (!selectedAgents.length) { incrementCount(internalSavedObjectsClient, 'live_query', 'errors'); ...

parseAgentSelection is accessing .fleet-agents index, so for that we also want to use .asInternalUser

so I believe there are no changes needed

parseAgentSelection() also uses the saved objects client to find "ingest-package-policies" objects via packagePolicyService.list(), just wanted to be sure that is not overlooked 👍 as currently written, access to those saved objects will not be authorized or audited. If that is intended, then this is fine as-is!

you're right 🤦 Fixed it, Thank you a ton @jportner!

jportner · 2021-08-09T19:05:45Z

x-pack/plugins/osquery/server/plugin.ts

+import { PLUGIN_ID } from '../common';
+
+const registerFeatures = (features: SetupPlugins['features']) => {
+  features.registerKibanaFeature({


I thought you were going to use order: 2300 here, did it get removed on accident or on purpose?
Just wanted to double check 😄

…rbac

patrykkopycinski · 2021-08-09T22:41:46Z

A few comments but otherwise it's looking good!

Also, I can't stress enough:

ideally add some amount of end-to-end integration tests and/or functional tests that exercise the different combinations of privileges that can be used

If you absolutely don't have time to do this now, it would be good to open an issue in your team's backlog so you can pay down this tech debt in the future.

I totally agree, @lykkin is working on setup Cypress tests with Elastic agent, so we can test privileges e2e :)

kibanamachine · 2021-08-10T02:01:08Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`osquery`	241	245	+4

Async chunks

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

id	before	after	diff
`osquery`	912.4KB	922.3KB	+9.9KB

Page load bundle

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

id	before	after	diff
`osquery`	14.1KB	13.0KB	-1.1KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id	before	after	diff
`osquery-manager-usage-metric`	-	3	+3
`osquery-usage-metric`	3	-	-3
total			-0

Unknown metric groups

API count

id	before	after	diff
`fleet`	1149	1153	+4

API count missing comments

id	before	after	diff
`fleet`	1049	1052	+3

History

💔 Build #143908 failed ac77f1b
💚 Build #143901 succeeded 0bb8bc2
💚 Build #143816 succeeded 07cca8e
💔 Build #143780 failed 4fcb527
💚 Build #143524 succeeded dec423e

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

cc @patrykkopycinski

jportner

LGTM!

patrykkopycinski · 2021-08-10T14:36:15Z

@jportner Thank you for all the effort and support we have got from you while working on this PR 🙏

kibanamachine · 2021-08-10T14:39:47Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

gchaps · 2021-08-10T19:41:38Z

x-pack/plugins/osquery/public/common/hooks/use_osquery_integration.tsx

+    onError: (error: Error) =>
+      setErrorToast(error, {
+        title: i18n.translate('xpack.osquery.osquery_integration.fetchError', {
+          defaultMessage: 'Error while fetching osquery integration',


Suggest:

Error fetching osquery integration

gchaps · 2021-08-10T19:43:21Z

x-pack/plugins/osquery/public/fleet_integration/disabled_callout.tsx

+      <EuiFlexItem>
+        <EuiCallOut
+          title={i18n.translate('xpack.osquery.fleetIntegration.saveIntegrationCalloutTitle', {
+            defaultMessage: 'Save the integration to access the options below',


Add an ending period.

Save the integration to access the options below.

gchaps · 2021-08-10T19:49:55Z

x-pack/plugins/osquery/public/routes/components/missing_privileges.tsx

+          <h2>
+            <FormattedMessage
+              id="xpack.osquery.permissionDeniedErrorTitle"
+              defaultMessage="Permission denied"


Instead of "Permission denied" what about giving the user information on how to get access to this feature?

Title: Request access to xxxx

Description: Ask your administrator to add xxxx.

where xxx is the name of the feature.

I like the idea of including info about how to resolve the issue, but I also want to let the user know what the problem is (i.e. that permission is denied). What about something like this @gchaps?

Title: Permission denied

Description: You are not authorized to access this page. Ask your administrator to give you osquery Kibana privileges.

Here is what @melissaburpo and I brainstormed:

Title: Permission denied
Description: To access this page, ask your administrator for osquery Kibana privileges.

gchaps · 2021-08-10T19:50:40Z

x-pack/plugins/osquery/public/routes/components/missing_privileges.tsx

+          <p>
+            <FormattedMessage
+              id="xpack.osquery.permissionDeniedErrorMessage"
+              defaultMessage="You are not authorized to access this page."


See comment above.

gchaps · 2021-08-10T19:55:53Z

x-pack/plugins/osquery/public/saved_queries/form/index.tsx

-              id="xpack.osquery.savedQueries.form.scheduledQueryGroupConfigSection.title"
-              defaultMessage="Scheduled query group configuration"
+              id="xpack.osquery.savedQueries.form.scheduledQueryGroupConfigSection.description"
+              defaultMessage="The options listed below are optional and are only applied when the query is assigned to a scheduled query group."


Suggestion:

These options are applied when the query is assigned to a scheduled query group.

Interval(s) > Intervals

gchaps · 2021-08-10T20:04:15Z

x-pack/plugins/osquery/public/action_results/use_action_privileges.tsx

+      onError: (error: Error) =>
+        setErrorToast(error, {
+          title: i18n.translate('xpack.osquery.action_results_privileges.fetchError', {
+            defaultMessage: 'Error while fetching action results privileges',


Suggest:

Error fetching action results privileges

gchaps · 2021-08-10T20:06:47Z

x-pack/plugins/osquery/server/plugin.ts

+  features.registerKibanaFeature({
+    id: 'osquery',
+    name: i18n.translate('xpack.features.osqueryFeatureName', {
+      defaultMessage: 'Osquery',


On the Security side for the Osquery Manager docs, and the approach is to use osquery when referring to the open source tool but to use Osquery Manager when referring to our integration.

gchaps · 2021-08-10T20:07:13Z

x-pack/plugins/osquery/server/plugin.ts

+                api: [`${PLUGIN_ID}-runSavedQueries`],
+                id: 'run_saved_queries',
+                name: i18n.translate('xpack.osquery.features.runSavedQueriesPrivilegeName', {
+                  defaultMessage: 'Run Saved queries',


Run saved queries (lowercase S)

kibanamachine · 2021-08-12T14:37:09Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

Co-authored-by: Patryk Kopyciński <[email protected]>

patrykkopycinski added 2 commits July 26, 2021 08:47

[Osquery] RBAC

38de975

add config

ccbf3d3

patrykkopycinski requested review from jportner and a team July 26, 2021 06:55

patrykkopycinski self-assigned this Jul 26, 2021

patrykkopycinski requested a review from a team as a code owner July 26, 2021 06:55

cleanup comments

258b2fd

snide requested a review from a team July 26, 2021 12:35

snide mentioned this pull request Jul 26, 2021

Sub-privileges RBAC UX / UI Improvements #106758

Open

jportner reviewed Jul 28, 2021

View reviewed changes

legrego reviewed Jul 29, 2021

View reviewed changes

patrykkopycinski added 5 commits July 29, 2021 18:56

Merge branch 'master' of github.com:elastic/kibana into feat/osquery-…

f2c7514

…rbac

Merge branch 'master' of github.com:elastic/kibana into feat/osquery-…

6492f4d

…rbac

Merge branch 'master' of github.com:elastic/kibana into feat/osquery-…

91bd5cf

…rbac

Merge branch 'master' of github.com:elastic/kibana into feat/osquery-…

1148ada

…rbac

enforce RBAC

20ed82d

patrykkopycinski requested a review from a team as a code owner August 1, 2021 09:13

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Aug 1, 2021

patrykkopycinski added 3 commits August 8, 2021 10:37

Merge branch 'master' of github.com:elastic/kibana; branch 'feat/osqu…

05e3d08

…ery-rbac' of github.com:patrykkopycinski/kibana into feat/osquery-rbac

cleanup

e61dabf

cleanup

dec423e

joshdover suggested changes Aug 9, 2021

View reviewed changes

PR comments

4fcb527

joshdover approved these changes Aug 9, 2021

View reviewed changes

Update type_registrations.test.ts

07cca8e

jportner reviewed Aug 9, 2021

View reviewed changes

patrykkopycinski added 2 commits August 10, 2021 00:39

add order

6d2f1f6

Merge branch 'master' of github.com:elastic/kibana into feat/osquery-…

0bb8bc2

…rbac

patrykkopycinski added 2 commits August 10, 2021 01:20

Update create_action_route.ts

ac77f1b

Update create_action_route.ts

0976ba3

jportner self-requested a review August 10, 2021 14:32

jportner approved these changes Aug 10, 2021

View reviewed changes

patrykkopycinski merged commit 9edcf9e into elastic:master Aug 10, 2021

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 10, 2021

[Osquery] RBAC (elastic#106669)

831c7f8

kibanamachine mentioned this pull request Aug 10, 2021

[7.x] [Osquery] RBAC (#106669) #108051

Merged

patrykkopycinski deleted the feat/osquery-rbac branch August 10, 2021 14:44

gchaps reviewed Aug 10, 2021

View reviewed changes

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 12, 2021

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 12, 2021

kibanamachine added a commit that referenced this pull request Aug 12, 2021

[Osquery] RBAC (#106669) (#108051)

ae211c3

Co-authored-by: Patryk Kopyciński <[email protected]>

legrego mentioned this pull request Apr 19, 2022

[Osquery] Make Osquery All with All base privillege #130523

Merged

juliaElastic mentioned this pull request Apr 29, 2022

[Fleet] Expose Fleet services used by Osquery with authorization check #131240

Open

		all: [savedQuerySavedObjectType],
		read: [savedQuerySavedObjectType],

[Osquery] RBAC #106669

[Osquery] RBAC #106669

Conversation

patrykkopycinski commented Jul 26, 2021

Summary

elasticmachine commented Jul 26, 2021

patrykkopycinski commented Jul 26, 2021

snide commented Jul 26, 2021 • edited Loading

snide commented Jul 26, 2021

jportner commented Jul 26, 2021

snide commented Jul 26, 2021

jportner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Jul 28, 2021 • edited Loading

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

legrego left a comment

Choose a reason for hiding this comment

legrego Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

gchaps Aug 10, 2021 • edited Loading

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

joshdover left a comment

Choose a reason for hiding this comment

jportner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Aug 9, 2021 • edited Loading

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

patrykkopycinski commented Aug 9, 2021

kibanamachine commented Aug 10, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

Saved Objects .kibana field count

API count

API count missing comments

History

jportner left a comment

Choose a reason for hiding this comment

patrykkopycinski commented Aug 10, 2021

kibanamachine commented Aug 10, 2021

💚 Backport successful

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

gchaps Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Aug 12, 2021

snide commented Jul 26, 2021 •

edited

Loading

jportner Jul 28, 2021 •

edited

Loading

legrego Jul 29, 2021 •

edited

Loading

gchaps Aug 10, 2021 •

edited

Loading

jportner Aug 9, 2021 •

edited

Loading

gchaps Aug 10, 2021 •

edited

Loading