[Feature Controls] - Secure Features API #35841

legrego · 2019-04-30T21:51:19Z

Summary

Secures access to the /api/features/v1 API endpoint by requiring "Global All" privileges (those granted by kibana_user).

Prior to this change, any user could access the endpoint to get a list of features. The Role Management and Spaces Management screens relied on this behavior when rendering. Once the endpoint was secured, these screens crashed on render for underprivileged users.

To solve this, xpack_main introduces a featureControls.manage UI Capability, which is consistent with space's spaces.manage UI Capability. Additionally, xpack_main exposes a simple client-side service to allow consumers to safely get the list of features if the user is able to do so.

…res api

elasticmachine · 2019-04-30T21:51:31Z

Pinging @elastic/kibana-security

elasticmachine · 2019-04-30T22:43:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-01T12:30:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kobelb

Adding a ui capability of featureControls.manage feels wrong here... We've tried to keep the ui capabilities to be geared towards their consumption, and there isn't a "feature controls" section of the ui itself.

IIRC, we only use the client-side FeaturesService.getFeatures() and rely on that check for the role management screen. I wonder if we should just be handling the possibility of a 404 here?

kobelb · 2019-05-01T16:16:26Z

x-pack/plugins/xpack_main/server/routes/api/v1/features/features.ts

@@ -10,6 +10,9 @@ export function featuresRoute(server: Record<string, any>) {
  server.route({
    path: '/api/features/v1',
    method: 'GET',
+    options: {
+      tags: ['access:manage_feature_controls'],


Suggested change

tags: ['access:manage_feature_controls'],

tags: ['access:features'],

We're currently only using this API in the context of "managing feature controls", but there's potential we'll want to consume this elsewhere, hence the suggestion.

We're currently only using this API in the context of "managing feature controls", but there's potential we'll want to consume this elsewhere, hence the suggestion.

I opted for the more specific tag name because we don't scope/namespace these by feature, so it's possible for two features to define the same access tag. That could unintentionally grant a user access to an API that they shouldn't have access to.
It is rather unlikely that another feature would register a tag called "features", so I can change this if you don't have concerns around this.

It is rather unlikely that another feature would register a tag called "features", so I can change this if you don't have concerns around this.

It's a fair point... I still feel like "manage_feature_controls" is just the wrong tag for it though, but I don't want to be ignoring your concern for tag collisions. It feels like the point you've brought up is something that warrants more consideration.

How would you feel about renaming this to features and creating an issue for us to discuss other solutions to potentially prevent unintentional collisions. Perhaps the changes we're making to port this to the new platform will provide us more opportunities than the "tag" based approach which we're employing because of Hapi.

That sounds good to me, I'll make it so

legrego · 2019-05-01T17:49:33Z

Adding a ui capability of featureControls.manage feels wrong here... We've tried to keep the ui capabilities to be geared towards their consumption, and there isn't a "feature controls" section of the ui itself.

That's a fair criticism. I was thinking ahead (YAGNI) to use cases such as #35865, where admins would be able to control their feature controls. At that point, there could potentially be a UI in place for FC management. I'm happy to revisit this, I'm just documenting my thought process.

IIRC, we only use the client-side FeaturesService.getFeatures() and rely on that check for the role management screen.

Yep - it's used on the Spaces Management screen too.

I wonder if we should just be handling the possibility of a 404 here?

I had something like this in my first implementation, but it felt "off" to me:

async function getFeatures() {
  try {
    return kfetch('/api/features/v1');
  } catch (e) {
     if (/*is404*/) {
       return [];
     }
	 // else ?
  }
}

It's not clear to the casual maintainer why this documented API endpoint would ever come back with a 404. I felt that guarding this behind a UI Capability was at least a bit more readable. I could improve readability with a comment, but that feels like I'm explaining away a poor implementation.

Given all that, what are your thoughts?

kobelb · 2019-05-01T18:08:28Z

That's a fair criticism. I was thinking ahead (YAGNI) to use cases such as #35865, where admins would be able to control their feature controls. At that point, there could potentially be a UI in place for FC management. I'm happy to revisit this, I'm just documenting my thought process.

In that situation, we wouldn't want to be using this ui capability to not load the features for the spaces management though, right? We'll have users which are able to create spaces because of the global base all privilege, but won't be able to define the features which are part of the base privileges because they don't have manage_security.

Yep - it's used on the Spaces Management screen too.

Isn't this preempted by the user not having uiCapabilities.spaces.manage so we don't make this api call in the first place?

It's not clear to the casual maintainer why this documented API endpoint would ever come back with a 404. I felt that guarding this behind a UI Capability was at least a bit more readable. I could improve readability with a comment, but that feels like I'm explaining away a poor implementation.

I agree that the 404 could be easily misinterpreted. Another option is to start throwing 403s in these situations, which would disclose the existence of the API, but make it more understandable.

If we want to continue using ui capabilities, it feels like they should be scoped to the consumption, like we've done elsewhere. So we could potentially add a security.roleManagement.features ui capability.

legrego · 2019-05-01T18:24:41Z

In that situation, we wouldn't want to be using this ui capability to not load the features for the spaces management though, right? We'll have users which are able to create spaces because of the global base all privilege, but won't be able to define the features which are part of the base privileges because they don't have manage_security.

good point.

Isn't this preempted by the user not having uiCapabilities.spaces.manage so we don't make this api call in the first place?

Not with the current order of operations, but it could be done. uiCapabilities.spaces.manage determines in React-land if we should load the grid/page, or an unauthorized prompt. The feature list is loaded in angular-land before that takes place.

If we want to continue using ui capabilities, it feels like they should be scoped to the consumption, like we've done elsewhere. So we could potentially add a security.roleManagement.features ui capability.

I'm thinking maybe we don't use ui capabilities for this then. security.roleManagement.features (and the current featureControls.manage) both happen to work for the role management screen because that flag is always set to the same value as spaces.manage. When spaces.manage is false, then the entire Kibana section of the role management page is unavailable. If that section of the page is available, then it is expected that features will also be available.

I could do away with all of this by changing the initialization order for these screens. Instead of loading the features in angular-land, I can load them in React-land once we determine that the user isn't going to see an Unauthorized prompt or disabled section.

kobelb · 2019-05-01T18:39:00Z

I'm thinking maybe we don't use ui capabilities for this then. security.roleManagement.features (and the current featureControls.manage) both happen to work for the role management screen because that flag is always set to the same value as spaces.manage. When spaces.manage is false, then the entire Kibana section of the role management page is unavailable. If that section of the page is available, then it is expected that features will also be available.
I could do away with all of this by changing the initialization order for these screens. Instead of loading the features in angular-land, I can load them in React-land once we determine that the user isn't going to see an Unauthorized prompt or disabled section

I'm not opposed to either of these! Thanks for putting up with my pedantic wishes.

…to features api" This reverts commit addc149.

…tures-api

elasticmachine · 2019-05-02T13:49:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

kobelb

LGTM on green CI

legrego · 2019-05-02T20:18:19Z

The build is going to fail with a CI connection error 😢

elasticmachine · 2019-05-02T20:49:41Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

legrego · 2019-05-02T21:08:28Z

retest

elasticmachine · 2019-05-02T22:07:32Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…tures-api

elasticmachine · 2019-05-03T10:46:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-03T11:56:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* restrict access to Features API * introduce featureControls.manage capability to control calls to features api * add snapshots * rename manage_feature_controls api tag to features * Revert "introduce featureControls.manage capability to control calls to features api" This reverts commit addc149. * update spaces management to only call APIs if authorized * handle 404 response when requesting features on role management page * better variable naming * remove unnecessary mock * remove unused code * remove unnecessary snapshots

Assigned features due to elastic#35841 Assigned file_upload due to https://github.com/elastic/kibana/blob/c79f0ae78633c81beebd3f95735326cc872be7f6/x-pack/plugins/file_upload/kibana.jsonc#L4 Assigned grok debugger due to https://github.com/elastic/kibana/blob/c79f0ae78633c81beebd3f95735326cc872be7f6/x-pack/plugins/grokdebugger/kibana.jsonc#L4 Assigned stats due to https://github.com/elastic/kibana/blob/c79f0ae78633c81beebd3f95735326cc872be7f6/src/plugins/usage_collection/kibana.jsonc#L4 - and I found the GET route within `src/plugins/usage_collection/server/routes/stats/stats.ts` Assigned kql_telemetry due to https://github.com/elastic/kibana/blob/3bfa7c00181599541c924d36b593205fd5d9fed4/src/plugins/data/kibana.jsonc#L4 - the service is exposed here: `src/plugins/data/server/kql_telemetry/kql_telemetry_service.ts` Contributes to: elastic#194817

…196709) ## Summary Assign ownership to some files that hopefully result in less than 5 reviewers ### Assignment Reasons Assigned features due to #35841 Assigned file_upload due to https://github.com/elastic/kibana/blob/c79f0ae78633c81beebd3f95735326cc872be7f6/x-pack/plugins/file_upload/kibana.jsonc#L4 Assigned grok debugger due to https://github.com/elastic/kibana/blob/c79f0ae78633c81beebd3f95735326cc872be7f6/x-pack/plugins/grokdebugger/kibana.jsonc#L4 Assigned stats due to https://github.com/elastic/kibana/blob/c79f0ae78633c81beebd3f95735326cc872be7f6/src/plugins/usage_collection/kibana.jsonc#L4 - and I found the GET route within `src/plugins/usage_collection/server/routes/stats/stats.ts` Assigned kql_telemetry due to https://github.com/elastic/kibana/blob/3bfa7c00181599541c924d36b593205fd5d9fed4/src/plugins/data/kibana.jsonc#L4 - the service is exposed here: `src/plugins/data/server/kql_telemetry/kql_telemetry_service.ts` Contributes to: #194817 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

legrego added 2 commits April 30, 2019 16:06

restrict access to Features API

5653338

introduce featureControls.manage capability to control calls to featu…

addc149

…res api

legrego requested a review from a team as a code owner April 30, 2019 21:51

legrego added the Feature:Security/Feature Controls Platform Security - Spaces & Role Mgmt feature controls label Apr 30, 2019

add snapshots

21918d5

kobelb reviewed May 1, 2019

View reviewed changes

legrego mentioned this pull request May 1, 2019

Feature Controls - Prevent API tag collisions #35881

Open

rename manage_feature_controls api tag to features

27f923c

legrego added 6 commits May 2, 2019 07:20

Revert "introduce featureControls.manage capability to control calls …

ce58612

…to features api" This reverts commit addc149.

Merge branch 'master' of github.com:elastic/kibana into fc/secure-fea…

9d02bec

…tures-api

update spaces management to only call APIs if authorized

f6fa67f

handle 404 response when requesting features on role management page

93d2f14

better variable naming

9cb5867

remove unnecessary mock

7efdc49

remove unused code

542f037

kobelb approved these changes May 2, 2019

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into fc/secure-fea…

35dbdb3

…tures-api

remove unnecessary snapshots

b01caa7

legrego merged commit 91b3404 into elastic:master May 3, 2019

legrego mentioned this pull request May 3, 2019

[7.x] [Feature Controls] - Secure Features API (#35841) #36019

Merged

legrego added v7.2.0 v8.0.0 labels May 3, 2019

legrego deleted the fc/secure-features-api branch May 3, 2019 12:05

legrego added the release_note:skip Skip the PR/issue when compiling release notes label May 3, 2019

legrego mentioned this pull request Jan 6, 2020

Migrate Management views to Kibana Platform plugin #53880

Merged

wayneseymour mentioned this pull request Oct 17, 2024

[FTR][Ownership] Features, File Upload, Grok Dbgr, Stats & Kql Telem #196709

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature Controls] - Secure Features API #35841

[Feature Controls] - Secure Features API #35841

legrego commented Apr 30, 2019

elasticmachine commented Apr 30, 2019

elasticmachine commented Apr 30, 2019

elasticmachine commented May 1, 2019

kobelb left a comment

kobelb May 1, 2019

legrego May 1, 2019

kobelb May 1, 2019

legrego May 1, 2019

legrego commented May 1, 2019

kobelb commented May 1, 2019

legrego commented May 1, 2019

kobelb commented May 1, 2019

elasticmachine commented May 2, 2019

kobelb left a comment

legrego commented May 2, 2019

elasticmachine commented May 2, 2019

legrego commented May 2, 2019

elasticmachine commented May 2, 2019

elasticmachine commented May 3, 2019

elasticmachine commented May 3, 2019

	tags: ['access:manage_feature_controls'],
	tags: ['access:features'],

[Feature Controls] - Secure Features API #35841

[Feature Controls] - Secure Features API #35841

Conversation

legrego commented Apr 30, 2019

Summary

elasticmachine commented Apr 30, 2019

elasticmachine commented Apr 30, 2019

💔 Build Failed

elasticmachine commented May 1, 2019

💚 Build Succeeded

kobelb left a comment

Choose a reason for hiding this comment

kobelb May 1, 2019

Choose a reason for hiding this comment

legrego May 1, 2019

Choose a reason for hiding this comment

kobelb May 1, 2019

Choose a reason for hiding this comment

legrego May 1, 2019

Choose a reason for hiding this comment

legrego commented May 1, 2019

kobelb commented May 1, 2019

legrego commented May 1, 2019

kobelb commented May 1, 2019

elasticmachine commented May 2, 2019

💔 Build Failed

kobelb left a comment

Choose a reason for hiding this comment

legrego commented May 2, 2019

elasticmachine commented May 2, 2019

💔 Build Failed

legrego commented May 2, 2019

elasticmachine commented May 2, 2019

💔 Build Failed

elasticmachine commented May 3, 2019

💔 Build Failed

elasticmachine commented May 3, 2019

💚 Build Succeeded