elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.73k stars 8.14k forks source link

Role index access syntax #95009

Open wildc4t opened 3 years ago

wildc4t commented 3 years ago

Hi guys!

Describe the bug:

Then adding new custom role index access we can insert indexes in such a syntax (like in default index pattern)

filebeat-nginx-frontend-staffplan,filebeat-app-staffplan

but in this case syntax is accepted, setting is stored with no error, but no real access given we have to enter it separately as

filebeat-nginx-frontend-staffplan filebeat-app-staffplan

then all is worked like charm.

Maybe some input control needed here? or text description above

Thanks!

Kibana/Elasticsearch Stack version: v 7.10.0

Server OS version: CentOS Linux release 7.5.1804 (Core)

Browser and Browser OS versions: Google Chrome Version 88.0.4324.182 (Official Build) (64-bit)

Elastic Endpoint version:

Original install method (e.g. download page, yum, from source, etc.): yum

Functional Area (e.g. Endpoint management, timelines, resolver, etc.):

Steps to reproduce:

Then adding new custom role index access we can insert indexes in such a syntax (like in default index pattern)

filebeat-nginx-frontend-staffplan,filebeat-app-staffplan

but in this case syntax is accepted, setting is stored with no error, but no real access given we have to enter it separately as

filebeat-nginx-frontend-staffplan filebeat-app-staffplan

then all is worked like charm.

Current behavior:

no access to index

Expected behavior:

given access to index

Screenshots (if relevant):

Errors in browser console (if relevant):

Provide logs and/or server output (if relevant):

Any additional context (logs, chat logs, magical formulas, etc.):

elasticmachine commented 3 years ago

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine commented 3 years ago

Pinging @elastic/kibana-security (Team:Security)

jportner commented 3 years ago

Hi @wildc4t , thanks for opening this issue. It was tagged as Security Solution -- but just to be clear, you mean the Stack Management / Roles / Create page, is that right?

image

The input box's options are auto-populated with available indices, derived from the Index Pattern saved objects that have been created in Kibana.

Here's what I'm seeing when I type in what you described:

image

This is intentionally customizable as there might be ES indices / index patterns that don't have a correlating Index Pattern saved object in Kibana. I don't think we want to attempt to validate whether what the user entered will grant access to anything.

Perhaps we could show an error if the user enters invalid characters? I see this error from ES when trying to create an index with a comma in the name:

Invalid index name [a,b], must not contain the following characters [ , \", *, \\, <, |, ,, >, /, ?]

It seems like it's safe to validate input and exclude all of those (besides * which is valid in an index pattern).

legrego commented 3 years ago

Perhaps we could show an error if the user enters invalid characters? I see this error from ES when trying to create an index with a comma in the name:

Invalid index name [a,b], must not contain the following characters [ , \", *, \\, <, |, ,, >, /, ?]

It seems like it's safe to validate input and exclude all of those (besides * which is valid in an index pattern).

I strongly prefer that Elasticsearch perform this validation instead of Kibana. Kibana is not the only way to create a role, and Elasticsearch is the authoritative source on what constitutes what is valid or invalid.

While * is the most common type of pattern match applied here, Elasticsearch also allows users to enter an arbitrary Lucene Regular Expression to describe a pattern of indices.

Lucene's regular expression engine is not compatible with JavaScript's regular expression engine, so we wouldn't be able to "just test" the expression within Kibana to see if it was valid or not.

legrego commented 3 years ago

@elastic/es-security if you agree with my remarks about Elasticsearch performing this validation as part of the role create/update process, then I'll transfer this to your repository for tracking and prioritization

ywangd commented 3 years ago

I agree that if we want to support this, it is better done at the Elasticsearch level. Hence please feel free to transfer this issue to us, though it may not get prioritised for while.

tvernum commented 3 years ago

I'm not 100% on that. Yes, ES should perform index pattern validation, but part of this report is a UI issue.

Should the UI interpret "a,b" as a single index pattern containing 3 characters, or 2 separate patterns of "a" and "b" ? I think it's the latter (especially since ES doesn't support "," in index names).

We can (and probably should) do validation in ES and reject a pattern of "a,b", which will prevent people from creating roles that don't work, but I don't think that's the right UX. The right UX would be to understand that if someone enters "," they are probably trying to enter a list of patterns.

bytebilly commented 3 years ago

I almost agree with Tim.

To make it clearer to the user, we could accept the comma separated list in the UI, but we should turn it into separated values even before creating the role.

So, as soon as the user hits enter, instead of

image

we should parse and show

image

legrego commented 3 years ago

Should the UI interpret "a,b" as a single index pattern containing 3 characters, or 2 separate patterns of "a" and "b" ? I think it's the latter (especially since ES doesn't support "," in index names).

I generally agree, but still have concerns around regular expression parsing (e.g. a{2,4}), Perhaps it's not worth optimizing for that, and assume within the UI that a , should always be treated as a delimiter?

bytebilly commented 3 years ago

I generally agree, but still have concerns around regular expression parsing (e.g. a{2,4}), Perhaps it's not worth optimizing for that, and assume within the UI that a , should always be treated as a delimiter?

Regex patterns must be delimited by /, could we do the "split" only for non-regex input?

ywangd commented 3 years ago

I generally agree, but still have concerns around regular expression parsing (e.g. a{2,4}), Perhaps it's not worth optimizing for that, and assume within the UI that a , should always be treated as a delimiter?

Regex patterns must be delimited by /, could we do the "split" only for non-regex input?

What about this then?

Screen Shot 2021-03-27 at 8 09 29 am
bytebilly commented 3 years ago

Given our docs

Any pattern starting with / and not ending with / is considered to be malformed.

I assume /a{2,5}/ ☓ b ☓

ywangd commented 3 years ago

This just one example. We can still have a,/b{2,5}/,c and many other variations. What I am trying to say is that the splitting may not be as simple as it seems. It is likely require certain level of parsing. If this is true, I think we should do it in one place, that is Elasticsearch. Also there are other fields on the same UI page (or even on other pages), do we need make them splitting on comma as well? It could be an issue because some of the fields support using comma in their names, e.g. username, index field name. But if we only split for indices, it feels to be inconsistent. I am no UI expert, but since we already have the Enter key as the unambiguous seperator, I'd be hesitate to add another half working one.

bytebilly commented 3 years ago

If it's challenging to do "live" parsing in Kibana to detect , as a pattern separator, then we should not support that behavior. Users should be fully aware of patterns before they save the role, as they may not see the "fixed" version anymore.

However, pattern validation on the Elasticsearch side, with proper handling of errors in the UI, is something we should do anyway. I see that we also don't raise errors for "malformed" regex patterns.

spalger commented 2 years ago

I ran into this user trap today and would like to call out that we should at least try to help users who are getting caught in this trap. When index patterns with ,'s in them are added in the UI we should be showing a warning or something that tells people "this might not work as you expect, here are some docs".

alstolten commented 2 years ago

Hi there, I was brought here by #139343.

What I do not fully understand is that at other places Kibana does a validation for Elasticsearch index pattern names. An example would be this screenshot:

Screenshot from 2022-08-24 15-29-28

On that page the code from this https://github.com/elastic/kibana/blob/main/src/plugins/es_ui_shared/static/forms/helpers/field_validators/index_pattern_field.ts is used.

Might that not also be a solution to the problem on this issue? Or am I missing something?

azasypkin commented 2 years ago

What I do not fully understand is that at other places Kibana does a validation for Elasticsearch index pattern names

I'm not sure if the index_pattern of the index template is supposed to support the same syntax Elasticsearch supports for the names field of the index privileges. @elastic/es-security or @elastic/platform-deployment-management do you know?

ywangd commented 2 years ago

The index_pattern field only supports simple wildcards.