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

Only allow admins (and Read-Only Admins) to fetch unencrypted telemetry #96538

Closed
afharo opened this issue Apr 8, 2021 · 7 comments · Fixed by #126238
Closed

Only allow admins (and Read-Only Admins) to fetch unencrypted telemetry #96538

afharo opened this issue Apr 8, 2021 · 7 comments · Fixed by #126238
Assignees
Labels
Feature:Telemetry impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@afharo
Copy link
Member

afharo commented Apr 8, 2021

As agreed in #95143, once #96536 is completed, we'll need to change the Telemetry Fetch APIs (POST /api/telemetry/v2/clusters/_stats and GET /api/stats?extended) to only allow Admins and Read-Only Admins (#96536) to fetch the unencrypted version of the usage.

This will allow us to remove the scoped clients from the Usage Collection APIs and to always fetch the usage as kibana_system.

Need to be sure to test the reported telemetry before and after the change for consistency

@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Telemetry Team:KibanaTelemetry labels Apr 8, 2021
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@legrego
Copy link
Member

legrego commented Apr 12, 2021

@afharo what is your timeline for this? The work required to expose the authorization service into OSS is non-trivial for us, so this is an initiative that we'd have to plan for in our roadmap against our other priorities

@afharo
Copy link
Member Author

afharo commented Apr 14, 2021

@legrego there's no clear timeline for this yet. I'll need some help from @joshdover to set the right priorities.

We already have an X-Pack extension of the telemetry plugin. With some work (#93185), we could lean on it to extend this validation in the X-Pack side :)

I'm not entirely sure about how we'd do that for GET /api/stats?extended in this form though, but Progress, simple, perfection 😇

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Nov 4, 2021
@exalate-issue-sync exalate-issue-sync bot added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort and removed impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Jan 18, 2022
@pgayvallet
Copy link
Contributor

The work required to expose the authorization service into OSS is non-trivial for us, so this is an initiative that we'd have to plan for in our roadmap against our other priorities

@afharo Reopening the discussion, as this was put into our current sprint.

As of today, do we have any way to perform such check from an OSS endpoint?

If not, the only option I see is to perform IOC by having an xpack plugin register a checkPermission or whatever api to the telemetry plugin

@afharo
Copy link
Member Author

afharo commented Feb 23, 2022

@pgayvallet I think you got it right! The Security team (aka, @legrego) added the permission decryptedTelemetry in #96571. IMO, checking for that permission would be enough.

@afharo
Copy link
Member Author

afharo commented Feb 23, 2022

That said, I just tried to add security as a dependency to the telemetry plugin and there's a circular dependency because security depends on home, and home depends on telemetry to show the privacy banners in the Welcome screen:

private renderTelemetryEnabledOrDisabledText = () => {
const { telemetry } = this.props;
if (
!telemetry ||
!telemetry.telemetryService.userCanChangeSettings ||
!telemetry.telemetryService.getCanChangeOptInStatus()
) {
return null;
}
const isOptedIn = telemetry.telemetryService.getIsOptedIn();
if (isOptedIn) {
return (
<Fragment>
<FormattedMessage
id="home.dataManagementDisableCollection"
defaultMessage=" To stop collection, "
/>
<EuiLink href={this.services.addBasePath('management/kibana/settings')}>
<FormattedMessage
id="home.dataManagementDisableCollectionLink"
defaultMessage="disable usage data here."
/>
</EuiLink>
</Fragment>
);
} else {
return (
<Fragment>
<FormattedMessage
id="home.dataManagementEnableCollection"
defaultMessage=" To start collection, "
/>
<EuiLink href={this.services.addBasePath('management/kibana/settings')}>
<FormattedMessage
id="home.dataManagementEnableCollectionLink"
defaultMessage="enable usage data here."
/>
</EuiLink>
</Fragment>
);
}
};

We might need to change the way home implements it (probably providing extension points that are called by the telemetry plugin) to remove such dependency. Then, we can add this piece of logic to the HTTP request retrieving telemetry:

  router.post(
    {
      path: '/api/telemetry/v2/clusters/_stats',
      validate: {
        body: schema.object({
          unencrypted: schema.boolean({ defaultValue: false }),
          refreshCache: schema.boolean({ defaultValue: false }),
        }),
      },
    },
    async (context, req, res) => {
      const { unencrypted, refreshCache } = req.body;

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Telemetry impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
Development

Successfully merging a pull request may close this issue.

5 participants