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] Check permissions when requesting telemetry #126238

Merged
merged 27 commits into from
Mar 7, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
ca98c07
[Telemetry] Include `security` dependency
afharo Feb 23, 2022
da5c756
Clean usage collection types (removal of KibanaRequest context extens…
afharo Feb 23, 2022
de282e2
Fix premissions usage
afharo Feb 23, 2022
7d4aa84
Remove `home` dependency on `telemetry`
afharo Feb 23, 2022
64f6fd3
Fix types
afharo Feb 23, 2022
0342826
Merge branch 'main' of github.com:elastic/kibana into telemetry-depen…
afharo Feb 23, 2022
eea162b
Fix more types and tests
afharo Feb 23, 2022
93aaf41
Fix TS refs (no idea why it failed in this PR)
afharo Feb 23, 2022
543b712
Fix broken tests
afharo Feb 24, 2022
260aa2a
Fix translation files
afharo Feb 24, 2022
35dd344
Remove unused variable
afharo Feb 24, 2022
2a5497b
Fix `home` public API missing export
afharo Feb 24, 2022
6b200bf
Only validate the permissions on unencrypted requests
afharo Feb 24, 2022
5b48f6e
@jportner is our saviour
afharo Feb 24, 2022
738cf14
Merge branch 'main' into telemetry-depending-on-security
kibanamachine Feb 27, 2022
effa7f4
Merge branch 'main' into telemetry-depending-on-security
kibanamachine Mar 1, 2022
7f76422
Merge branch 'main' of github.com:elastic/kibana into telemetry-depen…
afharo Mar 4, 2022
6788d62
nit: test 2 different onRendered handlers
afharo Mar 4, 2022
65d7835
nit: Use `file.test.mocks.ts` + `jest.doMock`
afharo Mar 4, 2022
6f7e648
Only allow one telemetry notice renderer
afharo Mar 4, 2022
40578d1
Make `home` dependency optional
afharo Mar 4, 2022
8639532
Use security mock instead of creating our own
afharo Mar 4, 2022
b50da6b
Add functional tests
afharo Mar 4, 2022
8802cd8
try/catch externally registered handlers
afharo Mar 4, 2022
90e7bee
Merge branch 'main' of github.com:elastic/kibana into telemetry-depen…
afharo Mar 4, 2022
51b02f0
Fix type check
afharo Mar 4, 2022
d15a542
Merge branch 'main' into telemetry-depending-on-security
kibanamachine Mar 7, 2022
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
2 changes: 1 addition & 1 deletion src/plugins/home/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@
"server": true,
"ui": true,
"requiredPlugins": ["dataViews", "share", "urlForwarding"],
"optionalPlugins": ["usageCollection", "telemetry", "customIntegrations"],
"optionalPlugins": ["usageCollection", "customIntegrations"],
afharo marked this conversation as resolved.
Show resolved Hide resolved
"requiredBundles": ["kibanaReact"]
}
1 change: 1 addition & 0 deletions src/plugins/telemetry/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"server": true,
"ui": true,
"requiredPlugins": ["telemetryCollectionManager", "usageCollection", "screenshotMode"],
"optionalPlugins": ["security"],
"extraPublicDirs": ["common/constants"],
"requiredBundles": ["kibanaUtils", "kibanaReact"]
}
13 changes: 12 additions & 1 deletion src/plugins/telemetry/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import type {
Plugin,
Logger,
} from 'src/core/server';
import type { SecurityPluginStart } from '../../../../x-pack/plugins/security/server';
import { SavedObjectsClient } from '../../../core/server';
import { registerRoutes } from './routes';
import { registerCollection } from './telemetry_collection';
Expand All @@ -42,6 +43,7 @@ interface TelemetryPluginsDepsSetup {

interface TelemetryPluginsDepsStart {
telemetryCollectionManager: TelemetryCollectionManagerPluginStart;
security?: SecurityPluginStart;
}

/**
Expand Down Expand Up @@ -90,6 +92,8 @@ export class TelemetryPlugin implements Plugin<TelemetryPluginSetup, TelemetryPl
*/
private savedObjectsInternalClient$ = new ReplaySubject<SavedObjectsClient>(1);

private security?: SecurityPluginStart;

constructor(initializerContext: PluginInitializerContext<TelemetryConfigType>) {
this.logger = initializerContext.logger.get();
this.isDev = initializerContext.env.mode.dev;
Expand Down Expand Up @@ -119,6 +123,7 @@ export class TelemetryPlugin implements Plugin<TelemetryPluginSetup, TelemetryPl
router,
telemetryCollectionManager,
savedObjectsInternalClient$: this.savedObjectsInternalClient$,
getSecurity: () => this.security,
});

this.registerMappings((opts) => savedObjects.registerType(opts));
Expand All @@ -137,11 +142,17 @@ export class TelemetryPlugin implements Plugin<TelemetryPluginSetup, TelemetryPl
};
}

public start(core: CoreStart, { telemetryCollectionManager }: TelemetryPluginsDepsStart) {
public start(
core: CoreStart,
{ telemetryCollectionManager, security }: TelemetryPluginsDepsStart
) {
const { savedObjects } = core;
const savedObjectsInternalRepository = savedObjects.createInternalRepository();
this.savedObjectsInternalRepository = savedObjectsInternalRepository;
this.savedObjectsInternalClient$.next(new SavedObjectsClient(savedObjectsInternalRepository));

this.security = security;

this.startFetcher(core, telemetryCollectionManager);

return {
Expand Down
8 changes: 5 additions & 3 deletions src/plugins/telemetry/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type { Observable } from 'rxjs';
import type { IRouter, Logger, SavedObjectsClient } from 'kibana/server';
import type { TelemetryCollectionManagerPluginSetup } from 'src/plugins/telemetry_collection_manager/server';
import { registerTelemetryOptInRoutes } from './telemetry_opt_in';
import { registerTelemetryUsageStatsRoutes } from './telemetry_usage_stats';
import { registerTelemetryUsageStatsRoutes, SecurityGetter } from './telemetry_usage_stats';
import { registerTelemetryOptInStatsRoutes } from './telemetry_opt_in_stats';
import { registerTelemetryUserHasSeenNotice } from './telemetry_user_has_seen_notice';
import type { TelemetryConfigType } from '../config';
Expand All @@ -24,12 +24,14 @@ interface RegisterRoutesParams {
router: IRouter;
telemetryCollectionManager: TelemetryCollectionManagerPluginSetup;
savedObjectsInternalClient$: Observable<SavedObjectsClient>;
getSecurity: SecurityGetter;
}

export function registerRoutes(options: RegisterRoutesParams) {
const { isDev, telemetryCollectionManager, router, savedObjectsInternalClient$ } = options;
const { isDev, telemetryCollectionManager, router, savedObjectsInternalClient$, getSecurity } =
options;
registerTelemetryOptInRoutes(options);
registerTelemetryUsageStatsRoutes(router, telemetryCollectionManager, isDev);
registerTelemetryUsageStatsRoutes(router, telemetryCollectionManager, isDev, getSecurity);
registerTelemetryOptInStatsRoutes(router, telemetryCollectionManager);
registerTelemetryUserHasSeenNotice(router);
registerTelemetryLastReported(router, savedObjectsInternalClient$);
Expand Down
17 changes: 15 additions & 2 deletions src/plugins/telemetry/server/routes/telemetry_usage_stats.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,15 @@ import {
TelemetryCollectionManagerPluginSetup,
StatsGetterConfig,
} from 'src/plugins/telemetry_collection_manager/server';
import type { SecurityPluginStart } from '../../../../../x-pack/plugins/security/server';

export type SecurityGetter = () => SecurityPluginStart | undefined;

export function registerTelemetryUsageStatsRoutes(
router: IRouter,
telemetryCollectionManager: TelemetryCollectionManagerPluginSetup,
isDev: boolean
isDev: boolean,
getSecurity: SecurityGetter
) {
router.post(
{
Expand All @@ -31,9 +35,18 @@ export function registerTelemetryUsageStatsRoutes(
async (context, req, res) => {
const { unencrypted, refreshCache } = req.body;

const security = getSecurity();
if (security) {
const hasEnoughPrivileges = await security.authz
.checkPrivilegesWithRequest(req)
.globally({ kibana: 'decryptedTelemetry' });
if (!hasEnoughPrivileges) {
return res.forbidden();
}
}

try {
const statsConfig: StatsGetterConfig = {
request: req,
unencrypted,
refreshCache: unencrypted || refreshCache,
};
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/telemetry/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
{ "path": "../../plugins/kibana_utils/tsconfig.json" },
{ "path": "../../plugins/screenshot_mode/tsconfig.json" },
{ "path": "../../plugins/telemetry_collection_manager/tsconfig.json" },
{ "path": "../../plugins/usage_collection/tsconfig.json" }
{ "path": "../../plugins/usage_collection/tsconfig.json" },
{ "path": "../../../x-pack/plugins/security/tsconfig.json" }
]
}
13 changes: 3 additions & 10 deletions src/plugins/telemetry_collection_manager/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,10 @@ export class TelemetryCollectionManagerPlugin
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;
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

const refreshCache = config.unencrypted ? true : !!config.refreshCache;

if (esClient && soClient) {
return { usageCollection, esClient, soClient, kibanaRequest, refreshCache };
return { usageCollection, esClient, soClient, refreshCache };
}
}

Expand All @@ -142,9 +141,7 @@ export class TelemetryCollectionManagerPlugin
* @private
*/
private getElasticsearchClient(config: StatsGetterConfig): ElasticsearchClient | undefined {
return config.unencrypted
? this.elasticsearchClient?.asScoped(config.request).asCurrentUser
: this.elasticsearchClient?.asInternalUser;
return this.elasticsearchClient?.asInternalUser;
Copy link
Member Author

Choose a reason for hiding this comment

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

We can now rely on the internal user only 🎉

}

/**
Expand All @@ -155,11 +152,7 @@ export class TelemetryCollectionManagerPlugin
* @private
*/
private getSavedObjectsClient(config: StatsGetterConfig): SavedObjectsClientContract | undefined {
if (config.unencrypted) {
// Intentionally using the scoped client here to make use of all the security wrappers.
// It also returns spaces-scoped telemetry.
return this.savedObjectsService?.getScopedClient(config.request);
} else if (this.savedObjectsService) {
if (this.savedObjectsService) {
// Wrapping the internalRepository with the `TelemetrySavedObjectsClient`
// to ensure some best practices when collecting "all the telemetry"
// (i.e.: `.find` requests should query all spaces)
Expand Down
14 changes: 3 additions & 11 deletions src/plugins/telemetry_collection_manager/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,9 @@
* Side Public License, v 1.
*/

import {
ElasticsearchClient,
Logger,
KibanaRequest,
SavedObjectsClientContract,
} from 'src/core/server';
import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
import { TelemetryCollectionManagerPlugin } from './plugin';
import type { ElasticsearchClient, Logger, SavedObjectsClientContract } from 'src/core/server';
import type { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
import type { TelemetryCollectionManagerPlugin } from './plugin';

export interface TelemetryCollectionManagerPluginSetup {
setCollectionStrategy: <T extends BasicStatsPayload>(
Expand All @@ -36,7 +31,6 @@ export interface TelemetryOptInStats {
export interface BaseStatsGetterConfig {
unencrypted: boolean;
refreshCache?: boolean;
request?: KibanaRequest;
}

export interface EncryptedStatsGetterConfig extends BaseStatsGetterConfig {
Expand All @@ -45,7 +39,6 @@ export interface EncryptedStatsGetterConfig extends BaseStatsGetterConfig {

export interface UnencryptedStatsGetterConfig extends BaseStatsGetterConfig {
unencrypted: true;
request: KibanaRequest;
}

export interface ClusterDetails {
Expand All @@ -56,7 +49,6 @@ export interface StatsCollectionConfig {
usageCollection: UsageCollectionSetup;
esClient: ElasticsearchClient;
soClient: SavedObjectsClientContract;
kibanaRequest: KibanaRequest | undefined; // intentionally `| undefined` to enforce providing the parameter
refreshCache: boolean;
}

Expand Down