[Telemetry] Check permissions when requesting telemetry by afharo · Pull Request #126238 · elastic/kibana

afharo · 2022-02-23T12:04:32Z

Summary

POST /api/telemetry/v2/clusters/_stats now checks that the user has the permissions decryptedTelemetry added in #96571 to only allow admins to retrieve telemetry. It returns 403 otherwise.

Adding security as a dependency to telemetry created a circular dependency because home relies on telemetry to set decide on whether to show the telemetry notice. I moved that logic to the telemetry plugin and registered the "renderer" via a new API exposed by the home plugin.

Resolves #96538.

Side wins

As we no longer need to scope the requests, I removed the kibanaRequest from the fetch context. Simplifying a lot the types in the usageCollection plugin.

Resolves #84183.

How to test

Locally

Create a user with a role that has read-only access to all spaces. Then, log in with that user and head to Advanced Settings > Usage Data (at the bottom), and attempt to retrieve the telemetry sample. You'll see it gets a 403.

Cloud

Using the auto-deployed Kibana 🧡

Since Cloud hides the section Advanced Settings > Usage Data, we need to curl the deployment to test this feature:

"admin" requesting unencrypted telemetry should get 200

$ curl -s -o /dev/null -w "%{http_code}" --request POST 'https://kibana-pr-126238.kb.us-west2.gcp.elastic-cloud.com:9243/api/telemetry/v2/clusters/_stats' \
--header 'Content-Type: application/json' \
--header 'kbn-xsrf: test' \
--header 'Authorization: Basic xxxxxx' \
--data-raw '{"unencrypted": true}'

200%

read-only user (access to all spaces but in read-only mode), requesting unencrypted telemetry should get 403

$ curl -s -o /dev/null -w "%{http_code}" --request POST 'https://kibana-pr-126238.kb.us-west2.gcp.elastic-cloud.com:9243/api/telemetry/v2/clusters/_stats' \
--header 'Content-Type: application/json' \
--header 'kbn-xsrf: test' \
--header 'Authorization: Basic xxxxxx' \
--data-raw '{"unencrypted": true}'

403%

any of the users above, requesting encrypted telemetry should get 200

$ curl -s -o /dev/null -w "%{http_code}" --request POST 'https://kibana-pr-126238.kb.us-west2.gcp.elastic-cloud.com:9243/api/telemetry/v2/clusters/_stats' \
--header 'Content-Type: application/json' \
--header 'kbn-xsrf: test' \
--header 'Authorization: Basic xxxxxx' \
--data-raw '{"unencrypted": false}'

200%

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…ion)

src/plugins/home/kibana.json

afharo · 2022-02-23T12:08:32Z

src/plugins/telemetry_collection_manager/server/plugin.ts

    const esClient = this.getElasticsearchClient(config);
    const soClient = this.getSavedObjectsClient(config);
    // Provide the kibanaRequest so opted-in plugins can scope their custom clients only if the request is not encrypted
-    const kibanaRequest = config.unencrypted ? config.request : void 0;


This is the main reason for allowing admin only to retrieve the sampled data. There's no longer a need to scope the requests. It may also help us with #84183 because kibanaRequest won't be needed in the context anymore.

afharo · 2022-02-23T12:09:08Z

src/plugins/telemetry_collection_manager/server/plugin.ts

-    return config.unencrypted
-      ? this.elasticsearchClient?.asScoped(config.request).asCurrentUser
-      : this.elasticsearchClient?.asInternalUser;
+    return this.elasticsearchClient?.asInternalUser;


We can now rely on the internal user only 🎉

afharo · 2022-02-23T12:48:06Z

src/plugins/usage_collection/server/collector/usage_collector.ts

-  constructor(
-    log: Logger,
-    // Needed because it doesn't affect on anything here but being explicit creates a lot of pain down the line
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any


Removing anys 🥳

afharo · 2022-02-23T12:49:18Z

x-pack/plugins/task_manager/server/usage/task_manager_usage_collector.test.ts

 import { loggingSystemMock } from 'src/core/server/mocks';
 import {
  Collector,
-  createCollectorFetchContextWithKibanaMock,


I looked at the collector and they don't require the KibanaRequest mock.

…ding-on-security

afharo · 2022-02-24T10:23:52Z

src/plugins/dashboard/tsconfig.json

    { "path": "../navigation/tsconfig.json" },
    { "path": "../saved_objects_tagging_oss/tsconfig.json" },
    { "path": "../saved_objects/tsconfig.json" },
+    { "path": "../screenshot_mode/tsconfig.json" },


Not sure why it failed in this PR. But dashboard is importing types from screenshot_mode and it was missing this line.

@elastic/kibana-operations probably a bug in the type_check script that didn't catch it earlier in main?

We're not totally sure how TypeScript validates that projects have the references they need, it seems to validate it most of the time but there are edge cases where you can import from another project even when it's not listed in the projects deps. We are on the cusp of moving away from relative plugin imports to imported based on global module IDs for packages which will make this a non-problem.

This is a strange situation, but I'm glad we'll be moving away from it in the future. Thanks for adding the project to our tsconfig though!

afharo · 2022-02-24T10:27:14Z

src/plugins/home/public/application/components/welcome.tsx

  };

-  private redirecToAddData() {
+  private redirectToAddData() {


Just fixing a typo here 😇

jportner · 2022-02-24T14:45:29Z

src/plugins/telemetry/server/routes/telemetry_usage_stats.ts

+        const { hasAllRequested } = await security.authz
+          .checkPrivilegesWithRequest(req)
+          .globally({ kibana: 'decryptedTelemetry' });


You need to check the serialized privilege string here. I'd suggest adding a comment to clarify this design decision for future maintainers, too 😄

Suggested change

const { hasAllRequested } = await security.authz

.checkPrivilegesWithRequest(req)

.globally({ kibana: 'decryptedTelemetry' });

// Normally we would use `options: { tags: ['access:decryptedTelemetry'] }` in the route definition to check authorization for an

// API action, however, we want to check this conditionally based on the `unencrypted` parameter. In this case we need to use the

// security API directly to check privileges for this action. Note that the 'decryptedTelemetry' API privilege string is only

// granted to users that have "Global All" or "Global Read" privileges in Kibana.

const { checkPrivilegesWithRequest, actions } = security.authz;

const privileges = { kibana: actions.api.get('decryptedTelemetry') };

const { hasAllRequested } = await checkPrivilegesWithRequest(req).globally(privileges);

It works! Thank you! Is there any documentation I could have gone to earlier so I wouldn't bother you with this wrong usage?

You're welcome!

Here's where we have an example of securing an HTTP route with an API privilege string via an access: tag: https://www.elastic.co/guide/en/kibana/current/development-security.html#example-2-dev-tools

Unfortunately you couldn't use an access: tag here.
AFAIK we don't really have any dev docs or TS docs that explain how to use the Security plugin's API to check privileges directly. The only TS docs I could find is this snippet:

kibana/x-pack/plugins/security/server/authorization/actions/actions.ts

Lines 16 to 19 in 2c4e795

/** Actions are used to create the "actions" that are associated with Elasticsearch's

* application privileges, and are used to perform the authorization checks implemented

* by the various `checkPrivilegesWithRequest` derivatives.

*/

That's not too helpful 😅

I added an issue for us to add docs for the authorization service: #126355

afharo · 2022-02-27T18:18:52Z

@elasticmachine merge upstream

gmmorris

TM changes LGTM :)

ThomThomson

Presentation team changes LGTM

ThomThomson · 2022-03-01T17:36:48Z

src/plugins/dashboard/tsconfig.json

    { "path": "../navigation/tsconfig.json" },
    { "path": "../saved_objects_tagging_oss/tsconfig.json" },
    { "path": "../saved_objects/tsconfig.json" },
+    { "path": "../screenshot_mode/tsconfig.json" },


This is a strange situation, but I'm glad we'll be moving away from it in the future. Thanks for adding the project to our tsconfig though!

pgayvallet

Some nits and comments, but overall LGTM

src/plugins/home/public/application/components/welcome.test.tsx

src/plugins/home/public/services/welcome/welcome_service.test.ts

src/plugins/home/public/services/welcome/welcome_service.ts

src/plugins/telemetry/kibana.json

src/plugins/telemetry/server/routes/telemetry_usage_stats.test.ts

src/plugins/telemetry/server/routes/telemetry_usage_stats.ts

…ding-on-security

afharo · 2022-03-07T10:05:26Z

@elasticmachine merge upstream

flash1293

VisEditors changes LGTM, code review only

kibana-ci · 2022-03-07T11:33:44Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`home`	62	64	+2
`telemetry`	30	31	+1
total			+3

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
`home`	97	102	+5
`telemetryCollectionManager`	33	32	-1
total			+4

Async chunks

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

id	before	after	diff
`home`	77.4KB	75.8KB	-1.6KB

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
`usageCollection`	2	1	-1

Page load bundle

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

id	before	after	diff
`home`	9.8KB	10.6KB	+830.0B
`telemetry`	25.2KB	26.9KB	+1.7KB
total			+2.5KB

Unknown metric groups

API count

id	before	after	diff
`home`	133	140	+7
`telemetryCollectionManager`	33	32	-1
total			+6

ESLint disabled line counts

id	before	after	diff
`home`	26	28	+2
`usageCollection`	4	2	-2
total			-0

Total ESLint disabled count

id	before	after	diff
`home`	27	29	+2
`usageCollection`	5	3	-2
total			-0

History

💚 Build #27792 succeeded 51b02f0
💔 Build #27673 failed 8802cd8
💚 Build #26729 succeeded effa7f4
💚 Build #26423 succeeded 738cf14
💚 Build #26219 succeeded 5b48f6e
💚 Build #26149 succeeded 6b200bf

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

miltonhultgren

Infra changes LGTM 👍🏼

…ed-unexpectedly-error * 'main' of github.com:elastic/kibana: (46 commits) Fix copy and pasted renderer user_name test (elastic#126663) [Gauge] Vis editors gauge legacy percent mode. (elastic#126318) Remove all cases related code from timelines (elastic#127003) Hide Enterprise search panel when no nodes are present (elastic#127100) [Lens] Fixed flakiness on runtime fields' appearance on the list (elastic#126945) [Security Solution][Lists] - Add missing privileges callout to exception lists page (elastic#126874) [Security Solution][Lists] - Updates exception flyout edit error messages (elastic#126875) [Security Solution][Rules] - Remove rule selection for read only users (elastic#126827) Fix session cleanup test (elastic#126966) [ftr] implement support for accessing ES through CCS (elastic#126547) [type-summarizer] always use normalized paths, fix windows compat (elastic#127055) Revert "[ci] Configure hourly pipeline for a small spot instance trial (elastic#126824)" Revert "[CI] Expand spot instance trial a bit (elastic#126928)" [Alerting] Adding functional tests for alerting and actions telemetry (elastic#126528) [Telemetry] Check permissions when requesting telemetry (elastic#126238) Don't submit empty seed_urls or sitemap_urls when making a partial crawl request (elastic#126972) Remove License Requirement for Enterprise Search App Search Meta Engines (elastic#127046) [ML] Adding data recognizer module config cache (elastic#126338) skip flaky suite (elastic#126027) [Reporting] Improve error logging for rescheduled jobs (elastic#126737) ... # Conflicts: # x-pack/plugins/reporting/server/core.ts # x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

…re-browser-errors * 'main' of github.com:elastic/kibana: (46 commits) Fix copy and pasted renderer user_name test (elastic#126663) [Gauge] Vis editors gauge legacy percent mode. (elastic#126318) Remove all cases related code from timelines (elastic#127003) Hide Enterprise search panel when no nodes are present (elastic#127100) [Lens] Fixed flakiness on runtime fields' appearance on the list (elastic#126945) [Security Solution][Lists] - Add missing privileges callout to exception lists page (elastic#126874) [Security Solution][Lists] - Updates exception flyout edit error messages (elastic#126875) [Security Solution][Rules] - Remove rule selection for read only users (elastic#126827) Fix session cleanup test (elastic#126966) [ftr] implement support for accessing ES through CCS (elastic#126547) [type-summarizer] always use normalized paths, fix windows compat (elastic#127055) Revert "[ci] Configure hourly pipeline for a small spot instance trial (elastic#126824)" Revert "[CI] Expand spot instance trial a bit (elastic#126928)" [Alerting] Adding functional tests for alerting and actions telemetry (elastic#126528) [Telemetry] Check permissions when requesting telemetry (elastic#126238) Don't submit empty seed_urls or sitemap_urls when making a partial crawl request (elastic#126972) Remove License Requirement for Enterprise Search App Search Meta Engines (elastic#127046) [ML] Adding data recognizer module config cache (elastic#126338) skip flaky suite (elastic#126027) [Reporting] Improve error logging for rescheduled jobs (elastic#126737) ... # Conflicts: # x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

[Telemetry] Include security dependency

ca98c07

afharo added the ci:deploy-cloud label Feb 23, 2022

Clean usage collection types (removal of KibanaRequest context extens…

da5c756

…ion)

afharo commented Feb 23, 2022

View reviewed changes

afharo added 2 commits February 23, 2022 15:57

Fix premissions usage

de282e2

Remove home dependency on telemetry

7d4aa84

afharo added backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.2.0 labels Feb 23, 2022

afharo added 4 commits February 23, 2022 23:33

Fix types

64f6fd3

Merge branch 'main' of github.com:elastic/kibana into telemetry-depen…

0342826

…ding-on-security

Fix more types and tests

eea162b

Fix TS refs (no idea why it failed in this PR)

93aaf41

afharo changed the title ~~[Telemetry] Include security dependency~~ [Telemetry] Check permissions when requesting telemetry Feb 23, 2022

afharo added 4 commits February 24, 2022 10:30

Fix broken tests

543b712

Fix translation files

260aa2a

Remove unused variable

35dd344

Fix home public API missing export

2a5497b

afharo commented Feb 24, 2022

View reviewed changes

Only validate the permissions on unencrypted requests

6b200bf

jportner reviewed Feb 24, 2022

View reviewed changes

@jportner is our saviour

5b48f6e

afharo marked this pull request as ready for review February 24, 2022 14:59

afharo requested review from a team as code owners February 24, 2022 14:59

jportner mentioned this pull request Feb 24, 2022

[Docs] Add developer documentation for Authorization Service #126355

Closed

Merge branch 'main' into telemetry-depending-on-security

738cf14

Merge branch 'main' into telemetry-depending-on-security

effa7f4

gmmorris approved these changes Mar 1, 2022

View reviewed changes

ThomThomson approved these changes Mar 1, 2022

View reviewed changes

pgayvallet approved these changes Mar 1, 2022

View reviewed changes

afharo mentioned this pull request Mar 2, 2022

Self-managed telemetry Privacy Statement notice shown in Cloud environment #110638

Closed

afharo added 10 commits March 4, 2022 11:18

Merge branch 'main' of github.com:elastic/kibana into telemetry-depen…

7f76422

…ding-on-security

nit: test 2 different onRendered handlers

6788d62

nit: Use file.test.mocks.ts + jest.doMock

65d7835

Only allow one telemetry notice renderer

6f7e648

Make home dependency optional

40578d1

Use security mock instead of creating our own

8639532

Add functional tests

b50da6b

try/catch externally registered handlers

8802cd8

Merge branch 'main' of github.com:elastic/kibana into telemetry-depen…

90e7bee

…ding-on-security

Fix type check

51b02f0

afharo requested a review from a team as a code owner March 4, 2022 19:01

Merge branch 'main' into telemetry-depending-on-security

d15a542

flash1293 approved these changes Mar 7, 2022

View reviewed changes

pgayvallet approved these changes Mar 7, 2022

View reviewed changes

miltonhultgren approved these changes Mar 7, 2022

View reviewed changes

afharo merged commit cb70888 into elastic:main Mar 7, 2022

afharo deleted the telemetry-depending-on-security branch March 7, 2022 20:29

afharo mentioned this pull request Mar 8, 2022

[Telemetry] Move privacy statement URL to doc links #126597

Merged

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

-        const { hasAllRequested } = await security.authz
-          .checkPrivilegesWithRequest(req)
-          .globally({ kibana: 'decryptedTelemetry' });
+        // Normally we would use `options: { tags: ['access:decryptedTelemetry'] }` in the route definition to check authorization for an
+        // API action, however, we want to check this conditionally based on the `unencrypted` parameter. In this case we need to use the
+        // security API directly to check privileges for this action. Note that the 'decryptedTelemetry' API privilege string is only
+        // granted to users that have "Global All" or "Global Read" privileges in Kibana.
+        const { checkPrivilegesWithRequest, actions } = security.authz;
+        const privileges = { kibana: actions.api.get('decryptedTelemetry') };
+        const { hasAllRequested } = await checkPrivilegesWithRequest(req).globally(privileges);

	/** Actions are used to create the "actions" that are associated with Elasticsearch's
	* application privileges, and are used to perform the authorization checks implemented
	* by the various `checkPrivilegesWithRequest` derivatives.
	*/

Conversation

afharo commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Side wins

How to test

Locally

Cloud

Checklist

For maintainers

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afharo Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afharo commented Feb 27, 2022

Uh oh!

gmmorris left a comment

Choose a reason for hiding this comment

Uh oh!

ThomThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afharo commented Mar 7, 2022

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Mar 7, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

miltonhultgren left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

afharo commented Feb 23, 2022 •

edited

Loading

afharo Feb 24, 2022 •

edited

Loading