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

Index Pattern UI - remove _remote/info query #27345

Merged

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Dec 17, 2018

Summary

Index Pattern UI - remove _remote/info query and replace with search for *:* which we can also use for determining whether there are remote clusters

Resolves "kibana_user sees "Failed to load remote clusters" when creating index pattern" - #27093

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@mattkime mattkime requested a review from kobelb December 17, 2018 21:40
@mattkime mattkime added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Dec 17, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@mattkime mattkime added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Dec 17, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security

@mattkime mattkime added bug Fixes for quality problems that affect the customer experience regression labels Dec 17, 2018
@kobelb
Copy link
Contributor

kobelb commented Dec 17, 2018

Which privilege is necessary to use this endpoint? Are we sure that the kibana server user will have this?

@mattkime
Copy link
Contributor Author

@kobelb We can expect the user to have kibana_user, although I suspect I haven't implemented this. I could use some guidance.

Are we sure that the kibana server user will have this?

Uh, worked for me. But perhaps you know of a more rigorous method of determining the answer.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego
Copy link
Member

legrego commented Dec 18, 2018

We can expect the user to have kibana_user, although I suspect I haven't implemented this. I could use some guidance.

We can't assume that users will have the kibana_user role anymore, with the adoption of Kibana's new authorization model ("application privileges").

Based on my testing, it appears that the _remote/info endpoint requires the monitor cluster privilege. The kibana_system role (which the callWithInternalUser account has) is granted monitor, so that should be sufficient:
image

@mattkime
Copy link
Contributor Author

@legrego

We can't assume that users will have the kibana_user role anymore, with the adoption of Kibana's new authorization model ("application privileges").

What privileges can we assume a basic user will have?

@kobelb
Copy link
Contributor

kobelb commented Dec 18, 2018

Based on my testing, it appears that the _remote/info endpoint requires the monitor cluster privilege. The kibana_system role (which the callWithInternalUser account has) is granted monitor, so that should be sufficient:

Thanks for researching this Larry. So, at this point we're in a situation where the greatest risk to using callWithInternalUser is potentially informing a user indirectly of the existence of a remote cluster, which they wouldn't have permission to see otherwise. How important is changing the UI to denote this situation?

What privileges can we assume a basic user will have?

We should be assuming that users with access to Kibana only have access to Kibana using the ES application privileges: https://www.elastic.co/guide/en/elastic-stack-overview/6.5/security-privileges.html#application-privileges

These do not grant them any additional access to ES APIs, and only indirectly grant them access to Kibana which Kibana enforces.

@legrego
Copy link
Member

legrego commented Dec 18, 2018

It's possible that a Kibana end user will only have Kibana application privileges defined. It's technically possible for a user to access Kibana without any cluster or index privileges, although it'd be of limited use.

Users will typically have Kibana application privileges, and then privileges to their own data indices (not necessarily system indices). Certain cluster/index privileges are required to enable certain features within Kibana, but they aren't required to use Kibana in general.

@kobelb
Copy link
Contributor

kobelb commented Dec 18, 2018

@mattkime
Copy link
Contributor Author

I'm going to discuss this with design. The design does not lend itself to 'we don't have access to determine if you have any indexes'

@mattkime mattkime changed the title Index Pattern UI - use internal user to query _remote/info Index Pattern UI - remove _remote/info query Dec 19, 2018
@mattkime
Copy link
Contributor Author

Refactored to only use search api.

@kobelb might be interested

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@LeeDr LeeDr self-requested a review December 20, 2018 04:17
Copy link
Contributor

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

Works for me.

I tested by copying the file /src/legacy/core_plugins/kibana/public/management/sections/index_patterns/create_index_pattern_wizard/create_index_pattern_wizard.js from this PR to my 7.0.0-alpha2 install, removed everything from my optimize directory and restarted Kibana.

Now as the same user as originally described in the issue, I can create index patterns (both local and cross cluster) without the error. So I think this fix is good.

There is a different problem in this same area but I don't think this PR caused it or fixes it. The problem is if a normal kibana_user enters something like logstash- and removes the trailing *, then Kibana just churns and churns on Looking for matching indices
image

While the console shows;

Detected an unhandled Promise rejection.
[security_exception] action [indices:admin/shards/search_shards] is unauthorized for user [power] :: {"path":"/*%3Am/_search","query":{},"body":"{\"size\":0,\"aggs\":{\"indices\":{\"terms\":{\"field\":\"_index\",\"size\":200}}}}","statusCode":403,"response":"{\"error\":{\"root_cause\":[{\"type\":\"security_exception\",\"reason\":\"action [indices:admin/shards/search_shards] is unauthorized for user [power]\"}],\"type\":\"security_exception\",\"reason\":\"action [indices:admin/shards/search_shards] is unauthorized for user [power]\"},\"status\":403}"}
vendors.bundle.dll.js:95 Uncaught (in promise) StatusCodeError {status: 403, displayName: "AuthorizationException", message: "[security_exception] action [indices:admin/shards/search_shards] is unauthorized for user [power]", path: "/*%3Am/_search", query: {…}, …}
step @ kibana.bundle.js:2
(anonymous) @ kibana.bundle.js:2
Promise.then (async)
step @ kibana.bundle.js:2
(anonymous) @ kibana.bundle.js:2
(anonymous) @ kibana.bundle.js:2
fetchIndices @ kibana.bundle.js:2
onQueryChanged @ kibana.bundle.js:2
ka @ vendors.bundle.dll.js:203
invokeGuardedCallback @ vendors.bundle.dll.js:203
invokeGuardedCallbackAndCatchFirstError @ vendors.bundle.dll.js:203
Fa @ vendors.bundle.dll.js:203
Ja @ vendors.bundle.dll.js:203
La @ vendors.bundle.dll.js:203
Ha @ vendors.bundle.dll.js:203
Oa @ vendors.bundle.dll.js:203
Pa @ vendors.bundle.dll.js:203
Td @ vendors.bundle.dll.js:203
batchedUpdates @ vendors.bundle.dll.js:203
dc @ vendors.bundle.dll.js:203
Xd @ vendors.bundle.dll.js:203
interactiveUpdates @ vendors.bundle.dll.js:203
Wd @ vendors.bundle.dll.js:203
VM6529:1 POST https://localhost:5601/elasticsearch/*%3Ama/_search 403 (Forbidden)

I'm not sure when this issue was introduced. I'll try to figure that out and file an issue on it.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -46,6 +46,7 @@ export async function getIndices(es, indexPatternCreationType, rawPattern, limit
}

const params = {
ignoreUnavailable: true,
Copy link
Contributor Author

@mattkime mattkime Dec 20, 2018

Choose a reason for hiding this comment

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

This addresses @LeeDr 's concern #27345 (review)

Requests without wildcards that failed to match were returning 500 errors. It seems that they should be returning 404 errors but its easier to apply this fix than track that down.

Its an unrelated bug but I thought I might as well address it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same issue is described here - #27218

@elasticmachine
Copy link
Contributor

💔 Build Failed

@LeeDr
Copy link
Contributor

LeeDr commented Dec 21, 2018

darn pie chart test! retest

@LeeDr
Copy link
Contributor

LeeDr commented Dec 21, 2018

jenkins test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukeelmers lukeelmers self-requested a review January 2, 2019 16:08
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Code LGTM. Added two low-priority comments, but nothing that should block this from being merged.

I haven't yet been able to test locally, but since @LeeDr already has, I will approve now so we can get this in.

}
this.catchAndWarn(
// if we get an error from remote cluster query, supply fallback value that allows user entry
getIndices(services.es, this.indexPatternCreationType, `*:*`, 1), ['a'], clustersFailMsg
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused by the ['a'] argument to catchAndWarn here... Does it serve a purpose, or is it just an arbitrary placeholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its just a placeholder value. I'll make my comment more specific

// if we get an error from remote cluster query, supply fallback value that allows user entry
getIndices(services.es, this.indexPatternCreationType, `*:*`, 1), ['a'], clustersFailMsg
).then(remoteIndices => this.setState({ remoteClustersExist: !!remoteIndices.length }));
};
Copy link
Member

Choose a reason for hiding this comment

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

nit: Any particular reason for changing the async/await in the two queries above back to Promise().then()? vs keeping the same convention, e.g.

const allIndices = await ensureMinimumTime(this.catchAndWarn(...));
this.setState({ allIndices, isInitiallyLoadingIndices: false });

const remoteIndices = await ensureMinimumTime(this.catchAndWarn(...));
this.setState({ remoteClustersExist: !!remoteIndices.length });

Copy link
Member

Choose a reason for hiding this comment

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

Ah, of course. I assume it is because you don’t care about what order those promises are resolved in, so you are letting them run asynchronously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukeelmers you got it!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mattkime mattkime merged commit 20ed7d4 into elastic:master Jan 4, 2019
mattkime added a commit to mattkime/kibana that referenced this pull request Jan 4, 2019
* use internal user to query _remote/info

* query both * and *:* rather than querying for clusters

* Update create_index_pattern_wizard.js

* Update create_index_pattern_wizard.js

* add a closing curly bracket

* fix for failed matches

* Update create_index_pattern_wizard.js
mattkime added a commit to mattkime/kibana that referenced this pull request Jan 4, 2019
* use internal user to query _remote/info

* query both * and *:* rather than querying for clusters

* Update create_index_pattern_wizard.js

* Update create_index_pattern_wizard.js

* add a closing curly bracket

* fix for failed matches

* Update create_index_pattern_wizard.js
mattkime added a commit to mattkime/kibana that referenced this pull request Jan 4, 2019
* use internal user to query _remote/info

* query both * and *:* rather than querying for clusters

* Update create_index_pattern_wizard.js

* Update create_index_pattern_wizard.js

* add a closing curly bracket

* fix for failed matches

* Update create_index_pattern_wizard.js
mattkime added a commit that referenced this pull request Jan 4, 2019
* use internal user to query _remote/info

* query both * and *:* rather than querying for clusters

* Update create_index_pattern_wizard.js

* Update create_index_pattern_wizard.js

* add a closing curly bracket

* fix for failed matches

* Update create_index_pattern_wizard.js
mattkime added a commit that referenced this pull request Jan 4, 2019
* use internal user to query _remote/info

* query both * and *:* rather than querying for clusters

* Update create_index_pattern_wizard.js

* Update create_index_pattern_wizard.js

* add a closing curly bracket

* fix for failed matches

* Update create_index_pattern_wizard.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience regression Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.6.0 v6.7.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants