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

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

Merged
merged 18 commits into from
Jan 27, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export interface UiSettingsParams<T = unknown>
| [readonly](./kibana-plugin-core-public.uisettingsparams.readonly.md) | <code>boolean</code> | a flag indicating that value cannot be changed |
| [requiresPageReload](./kibana-plugin-core-public.uisettingsparams.requirespagereload.md) | <code>boolean</code> | a flag indicating whether new value applying requires page reloading |
| [schema](./kibana-plugin-core-public.uisettingsparams.schema.md) | <code>Type&lt;T&gt;</code> | |
| [sensitive](./kibana-plugin-core-public.uisettingsparams.sensitive.md) | <code>boolean</code> | a flag indicating that value might contain user sensitive data. used by telemetry to mask the value of the setting when sent. |
| [type](./kibana-plugin-core-public.uisettingsparams.type.md) | <code>UiSettingsType</code> | defines a type of UI element [UiSettingsType](./kibana-plugin-core-public.uisettingstype.md) |
| [validation](./kibana-plugin-core-public.uisettingsparams.validation.md) | <code>ImageValidation &#124; StringValidation</code> | |
| [value](./kibana-plugin-core-public.uisettingsparams.value.md) | <code>T</code> | default value to fall back to if a user doesn't provide any |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-public](./kibana-plugin-core-public.md) &gt; [UiSettingsParams](./kibana-plugin-core-public.uisettingsparams.md) &gt; [sensitive](./kibana-plugin-core-public.uisettingsparams.sensitive.md)

## UiSettingsParams.sensitive property

a flag indicating that value might contain user sensitive data. used by telemetry to mask the value of the setting when sent.

<b>Signature:</b>

```typescript
sensitive?: boolean;
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [IUiSettingsClient](./kibana-plugin-core-server.iuisettingsclient.md) &gt; [isSensitive](./kibana-plugin-core-server.iuisettingsclient.issensitive.md)

## IUiSettingsClient.isSensitive property

Shows whether the uiSetting is a sensitive value. Used by telemetry to not send sensitive values.

<b>Signature:</b>

```typescript
isSensitive: (key: string) => boolean;
```
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface IUiSettingsClient
| [getRegistered](./kibana-plugin-core-server.iuisettingsclient.getregistered.md) | <code>() =&gt; Readonly&lt;Record&lt;string, PublicUiSettingsParams&gt;&gt;</code> | Returns registered uiSettings values [UiSettingsParams](./kibana-plugin-core-server.uisettingsparams.md) |
| [getUserProvided](./kibana-plugin-core-server.iuisettingsclient.getuserprovided.md) | <code>&lt;T = any&gt;() =&gt; Promise&lt;Record&lt;string, UserProvidedValues&lt;T&gt;&gt;&gt;</code> | Retrieves a set of all uiSettings values set by the user. |
| [isOverridden](./kibana-plugin-core-server.iuisettingsclient.isoverridden.md) | <code>(key: string) =&gt; boolean</code> | Shows whether the uiSettings value set by the user. |
| [isSensitive](./kibana-plugin-core-server.iuisettingsclient.issensitive.md) | <code>(key: string) =&gt; boolean</code> | Shows whether the uiSetting is a sensitive value. Used by telemetry to not send sensitive values. |
| [remove](./kibana-plugin-core-server.iuisettingsclient.remove.md) | <code>(key: string) =&gt; Promise&lt;void&gt;</code> | Removes uiSettings value by key. |
| [removeMany](./kibana-plugin-core-server.iuisettingsclient.removemany.md) | <code>(keys: string[]) =&gt; Promise&lt;void&gt;</code> | Removes multiple uiSettings values by keys. |
| [set](./kibana-plugin-core-server.iuisettingsclient.set.md) | <code>(key: string, value: any) =&gt; Promise&lt;void&gt;</code> | Writes uiSettings value and marks it as set by the user. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export interface UiSettingsParams<T = unknown>
| [readonly](./kibana-plugin-core-server.uisettingsparams.readonly.md) | <code>boolean</code> | a flag indicating that value cannot be changed |
| [requiresPageReload](./kibana-plugin-core-server.uisettingsparams.requirespagereload.md) | <code>boolean</code> | a flag indicating whether new value applying requires page reloading |
| [schema](./kibana-plugin-core-server.uisettingsparams.schema.md) | <code>Type&lt;T&gt;</code> | |
| [sensitive](./kibana-plugin-core-server.uisettingsparams.sensitive.md) | <code>boolean</code> | a flag indicating that value might contain user sensitive data. used by telemetry to mask the value of the setting when sent. |
| [type](./kibana-plugin-core-server.uisettingsparams.type.md) | <code>UiSettingsType</code> | defines a type of UI element [UiSettingsType](./kibana-plugin-core-server.uisettingstype.md) |
| [validation](./kibana-plugin-core-server.uisettingsparams.validation.md) | <code>ImageValidation &#124; StringValidation</code> | |
| [value](./kibana-plugin-core-server.uisettingsparams.value.md) | <code>T</code> | default value to fall back to if a user doesn't provide any |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [UiSettingsParams](./kibana-plugin-core-server.uisettingsparams.md) &gt; [sensitive](./kibana-plugin-core-server.uisettingsparams.sensitive.md)

## UiSettingsParams.sensitive property

a flag indicating that value might contain user sensitive data. used by telemetry to mask the value of the setting when sent.

<b>Signature:</b>

```typescript
sensitive?: boolean;
```
21 changes: 19 additions & 2 deletions packages/kbn-telemetry-tools/src/tools/serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export enum TelemetryKinds {
Date = 10001,
}

interface DescriptorValue {
export interface DescriptorValue {
kind: ts.SyntaxKind | TelemetryKinds;
type: keyof typeof ts.SyntaxKind | keyof typeof TelemetryKinds;
}
Expand All @@ -42,6 +42,13 @@ export function isObjectDescriptor(value: any) {
return false;
}

export function descriptorToObject(descriptor: Descriptor | DescriptorValue) {
return Object.entries(descriptor).reduce((acc, [key, value]) => {
acc[key] = value.kind ? kindToDescriptorName(value.kind) : descriptorToObject(value);
return acc;
}, {} as Record<string, any>);
}

export function kindToDescriptorName(kind: number) {
switch (kind) {
case ts.SyntaxKind.StringKeyword:
Expand Down Expand Up @@ -158,6 +165,16 @@ export function getDescriptor(node: ts.Node, program: ts.Program): Descriptor |
if (symbolName === 'Date') {
return { kind: TelemetryKinds.Date, type: 'Date' };
}

// Support Array<T>
if (symbolName === 'Array') {
if (node.typeArguments?.length !== 1) {
throw Error('Array type only supports 1 type parameter Array<T>');
}
Comment on lines +170 to +173
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

const typeArgument = node.typeArguments[0];
return { items: getDescriptor(typeArgument, program) };
}

// Support `Record<string, SOMETHING>`
if (symbolName === 'Record') {
const descriptor = getDescriptor(node.typeArguments![1], program);
Expand Down Expand Up @@ -243,7 +260,7 @@ export function getDescriptor(node: ts.Node, program: ts.Program): Descriptor |
case ts.SyntaxKind.UnionType:
case ts.SyntaxKind.AnyKeyword:
default:
throw new Error(`Unknown type ${ts.SyntaxKind[node.kind]}; ${node.getText()}`);
throw new Error(`Unknown type ${ts.SyntaxKind[node.kind]}`);
}
}

Expand Down
1 change: 1 addition & 0 deletions src/core/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,7 @@ export interface UiSettingsParams<T = unknown> {
requiresPageReload?: boolean;
// (undocumented)
schema: Type<T>;
sensitive?: boolean;
type?: UiSettingsType;
// (undocumented)
validation?: ImageValidation | StringValidation;
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,7 @@ export interface IUiSettingsClient {
getRegistered: () => Readonly<Record<string, PublicUiSettingsParams>>;
getUserProvided: <T = any>() => Promise<Record<string, UserProvidedValues<T>>>;
isOverridden: (key: string) => boolean;
isSensitive: (key: string) => boolean;
remove: (key: string) => Promise<void>;
removeMany: (keys: string[]) => Promise<void>;
set: (key: string, value: any) => Promise<void>;
Expand Down Expand Up @@ -3100,6 +3101,7 @@ export interface UiSettingsParams<T = unknown> {
requiresPageReload?: boolean;
// (undocumented)
schema: Type<T>;
sensitive?: boolean;
type?: UiSettingsType;
// (undocumented)
validation?: ImageValidation | StringValidation;
Expand Down
1 change: 1 addition & 0 deletions src/core/server/ui_settings/settings/notifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export const getNotificationsSettings = (): Record<string, UiSettingsParams> =>
},
}),
category: ['notifications'],
sensitive: true,
schema: schema.string(),
},
'notifications:lifetime:banner': {
Expand Down
4 changes: 4 additions & 0 deletions src/core/server/ui_settings/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ export interface IUiSettingsClient {
* Shows whether the uiSettings value set by the user.
*/
isOverridden: (key: string) => boolean;
/**
* Shows whether the uiSetting is a sensitive value. Used by telemetry to not send sensitive values.
*/
isSensitive: (key: string) => boolean;
}

/** @internal */
Expand Down
32 changes: 32 additions & 0 deletions src/core/server/ui_settings/ui_settings_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,38 @@ describe('ui settings', () => {
});
});

describe('#isSensitive()', () => {
it('returns false if sensitive config is not set', () => {
const defaults = {
foo: {
schema: schema.string(),
value: '1',
},
};

const { uiSettings } = setup({ defaults });
expect(uiSettings.isSensitive('foo')).toBe(false);
});

it('returns false if key is not in the settings', () => {
const { uiSettings } = setup();
expect(uiSettings.isSensitive('baz')).toBe(false);
});

it('returns true if overrides defined and key is overridden', () => {
const defaults = {
foo: {
schema: schema.string(),
sensitive: true,
value: '1',
},
};

const { uiSettings } = setup({ defaults });
expect(uiSettings.isSensitive('foo')).toBe(true);
});
});

describe('#isOverridden()', () => {
it('returns false if no overrides defined', () => {
const { uiSettings } = setup();
Expand Down
6 changes: 5 additions & 1 deletion src/core/server/ui_settings/ui_settings_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ export class UiSettingsClient implements IUiSettingsClient {

constructor(options: UiSettingsServiceOptions) {
const { type, id, buildNum, savedObjectsClient, log, defaults = {}, overrides = {} } = options;

this.type = type;
this.id = id;
this.buildNum = buildNum;
Expand Down Expand Up @@ -132,6 +131,11 @@ export class UiSettingsClient implements IUiSettingsClient {
return this.overrides.hasOwnProperty(key);
}

isSensitive(key: string): boolean {
const definition = this.defaults[key];
return !!definition?.sensitive;
Comment on lines +135 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 might change my opinion about this in the future but the I think using ?? with false is redundan. I find !! more explicit and succent

}

private assertUpdateAllowed(key: string) {
if (this.isOverridden(key)) {
throw new CannotOverrideError(`Unable to update "${key}" because it is overridden`);
Expand Down
1 change: 1 addition & 0 deletions src/core/server/ui_settings/ui_settings_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const createClientMock = () => {
remove: jest.fn(),
removeMany: jest.fn(),
isOverridden: jest.fn(),
isSensitive: jest.fn(),
};
mocked.get.mockResolvedValue(false);
mocked.getAll.mockResolvedValue({});
Expand Down
5 changes: 5 additions & 0 deletions src/core/types/ui_settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ export interface UiSettingsParams<T = unknown> {
requiresPageReload?: boolean;
/** a flag indicating that value cannot be changed */
readonly?: boolean;
/**
* a flag indicating that value might contain user sensitive data.
* used by telemetry to mask the value of the setting when sent.
*/
sensitive?: boolean;
/** defines a type of UI element {@link UiSettingsType} */
type?: UiSettingsType;
/** optional deprecation information. Used to generate a deprecation warning. */
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/kibana_usage_collection/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ This plugin registers the basic usage collectors from Kibana:
- UI Metrics
- Ops stats
- Number of Saved Objects per type
- Non-default UI Settings
- [User-changed UI Settings](./server/collectors/management/README.md)
- CSP configuration
- Core Metrics
4 changes: 4 additions & 0 deletions src/plugins/kibana_usage_collection/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,7 @@ export const PLUGIN_NAME = 'kibana_usage_collection';
* The type name used to publish Kibana usage stats in the formatted as bulk.
*/
export const KIBANA_STATS_TYPE = 'kibana_stats';
/**
* Redacted keyword; used as a value for sensitive ui settings
*/
export const REDACTED_KEYWORD = '[REDACTED]';
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# User-changed UI Settings - Management Collector

The Usage Collector `stack_management` reports user changed settings.
All user changed UI Settings are automatically collected.

After adding a new setting you will be required -via our api_integration tests- to update the [schema](./schema.ts) of the management collector and the [UsageStats](./types.ts) interface.
Bamieh marked this conversation as resolved.
Show resolved Hide resolved

If you forget our telemetry check will help you through the process! You can then run the checker with `--fix` flag to automatically fix the mappings

```
node scripts/telemetry_check --fix
```

## Sensitive fields

If the configured UI setting might contain user sensitive information simply add the property `sensitive: true` to the ui setting registration config.

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

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.

This file was deleted.

This file was deleted.

Loading