[Telemetry] Settings Collector: redact sensitive reported values #88675

Bamieh · 2021-01-19T13:45:41Z

The Usage Collector stack_management reports user changed settings.

This PR adds a new config to registered UI settings sensitive: boolean. The value of any UI setting marked as sensitive will be reported as a keyword [REDACTED] instead of the actual value. This hides the actual sensitive information while giving us some intelligence over which fields the users are interactive with the most.

Changes

Core Features:

A new sensitive: true config to the method core.uiSettings.register.

uiSettings.register({
  [NEWS_FEED_URL_SETTING]: {
    name: i18n.translate('xpack.securitySolution.uiSettings.newsFeedUrl', {
      defaultMessage: 'News feed URL',
    }),
    value: NEWS_FEED_URL_SETTING_DEFAULT,

    description: i18n.translate('xpack.securitySolution.uiSettings.newsFeedUrlDescription', {
      defaultMessage: '<p>News feed content will be retrieved from this URL</p>',
    }),
    category: [APP_ID],
    requiresPageReload: true,
    sensitive: true,
    schema: schema.string(),
  },
}),

Add isSensitive method to uiSettings plugin contract.
Values of UI Settings marked as sensitive will be reported reported as [REDACTED]

Schema updates:

UI Settings collector schema is not up to date with all the ui settings we have and it is evergreen!
Updated schema Typescript type MakeSchemaFrom to fail when the Usage TS type does not match the schema type property.
```
```
interface UsageStats {
  someProp: number;
}

const schema: MakeSchemaFrom<UsageStats> = {
  someProp': { type: 'long' },
}
```	  
```
Changing long to keyword for example will fail the type check
(in IDE as well) and not just the telemetry_check.ts.

Telemetry checker for UsageStats types now support the syntax Array<T> instead of only supporting T[].

```
interface UsageStats {
  someProp: number[];
  // this is now supported too!
  someProp2: Array<number>;
}
```

Tests:

Added tests for the ui settings collector function.
Add functional tests to make sure all UI Settings are mapped in the schema.
Add functional tests to make sure all sensitive UI settings are mapped as keywords. This will help us make sure we dont accidentally report sensitive fields to telemetry since this will trigger a code review from the telemetry team.

Current Sensitive UI Settings:

{
  'timelion:quandl.key'
  'securitySolution:defaultIndex'
  'securitySolution:newsFeedUrl'
  'xpackReporting:customPdfLogo'
  'notifications:banner'
  'timelion:graphite.url'
  'xpackDashboardMode:roles'
}

Got pinged for a codereview?

It is most likely a change adding sensitive: true to one of the registered ui settings your team owns. Please filter by codeowners and check for the changes.

Closes #85811

elasticmachine · 2021-01-19T13:47:24Z

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

afharo

Thank you for the type fixes ❤️

I think this list is a great starting point to unblock the works in the banners. However, I think we discussed an alternative approach during our grooming sessions. Are you planning on a follow-up PR to implement it?

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

...ugins/kibana_usage_collection/server/collectors/management/telemetry_management_collector.ts

…settings_enhancement

afharo

I love the new implementation! Thank you!

Although, I have some concerns about how the tests were implemented (needing to change an existing public API only for test purposes). Can we talk about other options before approving this PR?

x-pack/test/api_integration/apis/telemetry/management_collection.ts

afharo · 2021-01-26T11:30:04Z

src/core/server/ui_settings/routes/get.ts

+      path: '/api/kibana/settings',
+      validate: {
+        query: schema.object({
+          _allRegistered: schema.maybe(schema.boolean()),


We are exposing this only for test purposes. What do you think about following a similar approach to the Application Usage's functional tests?

A test plugin uses core's public APIs, and calls the existing method uiSettingsClient.getAll() and saves it in the window object.

Then the mocha tests retrieve that property and validate their content.

I agree exposing an API just for testing might not be ideal. I'm curious if anyone else from the team ( @elastic/kibana-core ) has different ideas.

Using the browser might be ok, even an api in the test plugin that exposes those settings might be even better too. Since api_integration tests dont open the browser and using an API is more reliable.

test/api_integration/apis/telemetry/index.js

flash1293

LGTM for Kibana app changes, didn't run the code. Makes timelion quandl key sensitive

afharo

LGTM! Thank you for this humongous effort!

Just added a couple of Super-NITs 😇

src/plugins/kibana_usage_collection/server/collectors/management/README.md

src/plugins/kibana_usage_collection/server/collectors/management/schema.ts

x-pack/plugins/security_solution/server/ui_settings.ts

rylnd

I believe that we need one additional setting to be marked as sensitive in SecSol.

rylnd · 2021-01-26T21:09:41Z

@Bamieh in discussing this with the team a potential feature enhancement was mentioned that I wanted to relay here: the ability to only redact sensitive values if they are not the default values.

This could maybe be inferred from other UI settings telemetry (e.g. "setting changed"), but in the case of multiple change events that would be ambiguous, so this would mainly answer the question "What proportion of users have modified values?"

pgayvallet

LGTM. Only comment I'd like to see addressed before merging is #88675 (comment)

pgayvallet · 2021-01-27T05:09:45Z

packages/kbn-telemetry-tools/src/tools/serializer.ts

+    if (symbolName === 'Array') {
+      if (node.typeArguments?.length !== 1) {
+        throw Error('Array type only supports 1 type parameter Array<T>');
+      }


UltraNIT: better safe than sorry I guess, but I still wonder if this assertion is really useful.

It is not needed but to stay consistent with other symbols we check for.

pgayvallet · 2021-01-27T05:16:52Z

src/core/server/ui_settings/ui_settings_client.ts

+    const definition = this.defaults[key];
+    return !!definition?.sensitive;


NIT (and personal opinion): Now that we have the ?? operator, I think definition?.sensitive ?? false would be slightly more explicit.

I might change my opinion about this in the future but the I think using ?? with false is redundan. I find !! more explicit and succent

pgayvallet · 2021-01-27T05:28:39Z

.../kibana_usage_collection/server/collectors/management/telemetry_management_collector.test.ts

+      const uiSettingsClient = uiSettingsServiceMock.createClient();
+      const getUiSettingsClient = jest.fn(() => uiSettingsClient);
+      uiSettingsClient.getUserProvided.mockResolvedValue(mockUserSettings);
+      uiSettingsClient.isSensitive.mockImplementation(mockIsSensitive);


NIT: these 4 lines are duplicated in all the tests of this describe section. Could probably be extracted to beforeEach or a function.

x-pack/test/usage_collection/test_suites/stack_management_usage/index.ts

…settings_enhancement

rylnd

SecSol changes LGTM. Thank you for adding that additional field!

kibanamachine · 2021-01-27T15:51:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c9d1314

Metrics [docs]

✅ unchanged

History

💔 Build #101415 failed 141b634
💚 Build #101151 succeeded 4dde310
💚 Build #101090 succeeded 210d7b0
💔 Build #101069 failed ec3bc7d
💔 Build #101057 failed 7b61956

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

…y-tests * 'master' of github.com:elastic/kibana: (276 commits) [Telemetry] Settings Collector: redact sensitive reported values (elastic#88675) [CI] Combines Jest test jobs (elastic#85850) [Upgrade Assistant] Migrate server to new es-js client (elastic#89207) Migrate maps_legacy, maps_oss, region_map, and tile_map plugions to TS projects (elastic#89351) [Vega Docs] Add experimental flag on the vega maps title (elastic#89402) Increase the time needed to locate the save viz toast (elastic#89301) [Enterprise Search] Add links to doc links service (elastic#89260) Fixed regex bug in Safari (elastic#89399) [Lens] Fix indexpattern checks for missing references (elastic#88840) [Lens] Clean up usage collector (elastic#89109) update apm index pattern (elastic#89395) [APM] Upgrade ES client (elastic#86594) Enable v2 so migrations, disable in FTR tests (elastic#89297) [Search Sessions] Make search session indicator UI opt-in, refactor per-app capabilities (elastic#88699) Cleanup OSS code from visualizations wizard (elastic#89092) [APM] Optimize API test order (elastic#88654) Rename conversion function, extract to module scope and add tests. (elastic#89018) [core.logging] Add ops logs to the KP logging system (elastic#88070) chore(NA): improve ts build refs performance on kbn bootstrap (elastic#89333) skip flaky suite (elastic#89379) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/timeline/timeline.tsx # x-pack/test/accessibility/config.ts

…ana into task-manager/shift-on-trend * 'task-manager/shift-on-trend' of github.com:gmmorris/kibana: (74 commits) [Metrics UI] Fix Host Overview boxes in Host Detail page (elastic#89299) [Telemetry] Settings Collector: redact sensitive reported values (elastic#88675) [CI] Combines Jest test jobs (elastic#85850) [Upgrade Assistant] Migrate server to new es-js client (elastic#89207) Migrate maps_legacy, maps_oss, region_map, and tile_map plugions to TS projects (elastic#89351) [Vega Docs] Add experimental flag on the vega maps title (elastic#89402) Increase the time needed to locate the save viz toast (elastic#89301) [Enterprise Search] Add links to doc links service (elastic#89260) Fixed regex bug in Safari (elastic#89399) [Lens] Fix indexpattern checks for missing references (elastic#88840) [Lens] Clean up usage collector (elastic#89109) update apm index pattern (elastic#89395) [APM] Upgrade ES client (elastic#86594) Enable v2 so migrations, disable in FTR tests (elastic#89297) [Search Sessions] Make search session indicator UI opt-in, refactor per-app capabilities (elastic#88699) Cleanup OSS code from visualizations wizard (elastic#89092) [APM] Optimize API test order (elastic#88654) Rename conversion function, extract to module scope and add tests. (elastic#89018) [core.logging] Add ops logs to the KP logging system (elastic#88070) chore(NA): improve ts build refs performance on kbn bootstrap (elastic#89333) ...

#88675) (#89434) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

elasticmachine · 2021-10-01T21:28:24Z

Pinging @elastic/kibana-core (Team:Core)

Bamieh added 2 commits January 19, 2021 15:09

add a whitelist for the collector and a blacklist for the schema

bc32777

add tests

e81d8bf

Bamieh added release_note:skip Skip the PR/issue when compiling release notes Team:KibanaTelemetry v7.12.0 labels Jan 19, 2021

Bamieh requested a review from afharo January 19, 2021 13:45

Bamieh requested a review from a team as a code owner January 19, 2021 13:45

afharo requested changes Jan 19, 2021

View reviewed changes

src/plugins/usage_collection/server/collector/collector.ts Show resolved Hide resolved

...ugins/kibana_usage_collection/server/collectors/management/telemetry_management_collector.ts Outdated Show resolved Hide resolved

wylieconlon added v8.0.0 and removed 8.0.0 labels Jan 20, 2021

Bamieh added 5 commits January 25, 2021 10:46

merge license changes from master

a4bb8a0

sensitive field + fully test ui_settings collector

1bf819a

update docs for sensitive config

5b6d98b

update ui_settings schema

192963f

Merge branch 'master' of github.com:elastic/kibana into telemetry/ui_…

a9a1068

…settings_enhancement

Bamieh requested a review from a team as a code owner January 26, 2021 10:42

Bamieh requested a review from a team January 26, 2021 10:42

Bamieh requested review from a team as code owners January 26, 2021 10:42

add documentation to management collector

e8fb492

afharo requested changes Jan 26, 2021

View reviewed changes

update docs

7b61956

flash1293 approved these changes Jan 26, 2021

View reviewed changes

Bamieh added 5 commits January 26, 2021 14:59

update functional tests

ec3bc7d

update telemetry tools

21b3059

revert tsconfig changes to tests

18e9e89

update telemetry mappings

210d7b0

[REDACTED] instead of true for sensitive ui settings

840d689

Bamieh requested a review from afharo January 26, 2021 16:05

afharo approved these changes Jan 26, 2021

View reviewed changes

src/plugins/kibana_usage_collection/server/collectors/management/README.md Outdated Show resolved Hide resolved

src/plugins/kibana_usage_collection/server/collectors/management/schema.ts Outdated Show resolved Hide resolved

afharo changed the title ~~[Telemetry] Settings Collector: use a whitelist for reported values~~ [Telemetry] Settings Collector: redact sensitive reported values Jan 26, 2021

address the nits

4dde310

rylnd reviewed Jan 26, 2021

View reviewed changes

x-pack/plugins/security_solution/server/ui_settings.ts Show resolved Hide resolved

rylnd reviewed Jan 26, 2021

View reviewed changes

pgayvallet approved these changes Jan 27, 2021

View reviewed changes

Bamieh added 2 commits January 27, 2021 11:31

Merge branch 'master' of github.com:elastic/kibana into telemetry/ui_…

0dd8b14

…settings_enhancement

another sensitive field + improve docs and test error message

141b634

Bamieh requested a review from rylnd January 27, 2021 12:37

remove unused import

c9d1314

rylnd approved these changes Jan 27, 2021

View reviewed changes

Bamieh merged commit 82902e8 into elastic:master Jan 27, 2021

Bamieh deleted the telemetry/ui_settings_enhancement branch January 27, 2021 15:57

Bamieh mentioned this pull request Jan 27, 2021

[7.x] [Telemetry] Settings Collector: redact sensitive reported values (#88675) #89434

Merged

Bamieh added a commit that referenced this pull request Jan 29, 2021

[7.x] [Telemetry] Settings Collector: redact sensitive reported values (

0de43fa

#88675) (#89434) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

lukeelmers added Feature:Telemetry Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Oct 1, 2021

lukeelmers removed the Team:KibanaTelemetry label Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Telemetry] Settings Collector: redact sensitive reported values #88675

[Telemetry] Settings Collector: redact sensitive reported values #88675

Bamieh commented Jan 19, 2021 •

edited

Loading

elasticmachine commented Jan 19, 2021

afharo left a comment

afharo left a comment

afharo Jan 26, 2021 •

edited

Loading

Bamieh Jan 26, 2021

flash1293 left a comment

afharo left a comment

rylnd left a comment

rylnd commented Jan 26, 2021

pgayvallet left a comment •

edited

Loading

pgayvallet Jan 27, 2021

Bamieh Jan 27, 2021

pgayvallet Jan 27, 2021

Bamieh Jan 27, 2021

pgayvallet Jan 27, 2021

rylnd left a comment

kibanamachine commented Jan 27, 2021

elasticmachine commented Oct 1, 2021

		const definition = this.defaults[key];
		return !!definition?.sensitive;

[Telemetry] Settings Collector: redact sensitive reported values #88675

[Telemetry] Settings Collector: redact sensitive reported values #88675

Conversation

Bamieh commented Jan 19, 2021 • edited Loading

Changes

Current Sensitive UI Settings:

Got pinged for a codereview?

elasticmachine commented Jan 19, 2021

afharo left a comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

afharo Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

Bamieh Jan 26, 2021

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

rylnd commented Jan 26, 2021

pgayvallet left a comment • edited Loading

Choose a reason for hiding this comment

pgayvallet Jan 27, 2021

Choose a reason for hiding this comment

Bamieh Jan 27, 2021

Choose a reason for hiding this comment

pgayvallet Jan 27, 2021

Choose a reason for hiding this comment

Bamieh Jan 27, 2021

Choose a reason for hiding this comment

pgayvallet Jan 27, 2021

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 27, 2021

💚 Build Succeeded

Metrics [docs]

History

elasticmachine commented Oct 1, 2021

Bamieh commented Jan 19, 2021 •

edited

Loading

afharo Jan 26, 2021 •

edited

Loading

pgayvallet left a comment •

edited

Loading