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

Add read permissions to the Kibana system user for the diagnostic telemetry #85391

Closed
tsg opened this issue Dec 9, 2020 · 12 comments · Fixed by elastic/elasticsearch#66135
Closed
Labels
Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.

Comments

@tsg
Copy link
Contributor

tsg commented Dec 9, 2020

Diagnostic Alert Telemetry is being added in #84422. The code uses the kibana_system user to query the hidden data stream logs-endpoint.diagnostic.collection-*.

We'd like to give kibana_system read-only access to these indices.

FYI @kobelb @peteharverson @stevewritescode @joe-desimone

@kobelb
Copy link
Contributor

kobelb commented Dec 10, 2020

There's been a bit of communication regarding this request outside of this GitHub issue, primarily around this using logs-endpoint.diagnostic.collection-* as opposed to .logs-endpoint.diagnostic.collection-*. @ruflin, is my understanding correct that we'd like to use logs-endpoint.diagnostic.collection-* for 7.11, and switch to using .logs-endpoint.diagnostic.collection-* starting in 7.12? If so, do we need to consider what our migration path looks like because we'd ideally remove the privileges from the kibana_system role to the logs-endpoint.diagnostic.collection-* indices as soon as possible, but I'm not sure how this would work in practice.

I've prepared elastic/elasticsearch#66135 to grant the kibana_system read access to the logs-endpoint.diagnostic.collection-* endpoint incase we need it. It can easily be revised to use .logs-endpoint.diagnostic.collection-* instead.

@ruflin
Copy link
Member

ruflin commented Dec 14, 2020

As far as I know we should already have the . prefix potentially in 7.11 if all the PRs make it on time.

albertzaharovits pushed a commit to elastic/elasticsearch that referenced this issue Dec 16, 2020
…int.diagnostic.collection-*` (#66135)

The endpoint protections team is storing diagnostic information
in the .logs-endpoint.diagnostic.collection-* indices, which Kibana
will read from to send the data to the remote telemetry service.

Resolves elastic/kibana#85391
albertzaharovits pushed a commit to albertzaharovits/elasticsearch that referenced this issue Dec 16, 2020
…int.diagnostic.collection-*` (elastic#66135)

The endpoint protections team is storing diagnostic information
in the .logs-endpoint.diagnostic.collection-* indices, which Kibana
will read from to send the data to the remote telemetry service.

Resolves elastic/kibana#85391
albertzaharovits added a commit to elastic/elasticsearch that referenced this issue Dec 16, 2020
…nt.diagnostic.collection-*` (#66135)

The endpoint protections team is storing diagnostic information
in the .logs-endpoint.diagnostic.collection-* indices, which Kibana
will read from to send the data to the remote telemetry service.

Resolves elastic/kibana#85391

Co-authored-by: Brandon Kobel <brandon.kobel@elastic.co>
@pjhampton
Copy link
Contributor

pjhampton commented Dec 16, 2020

@ruflin @kobelb @tsg

QQ on reading from this index - should the leading period be specified or not for 7.11?
Ref: https://github.com/elastic/kibana/pull/84422/files#diff-42eb463cfaca4b263f4d6543fd550415c343c5f004a3c72312c8e0dd704ef1caR107

@kobelb
Copy link
Contributor

kobelb commented Dec 16, 2020

@pjhampton FWIW, I was operating on the assumption that we'd have the leading period per #85391 (comment) and the kibana_system role now has access to read from the .logs-endpoint.diagnostic.collection-* indices.

@ruflin
Copy link
Member

ruflin commented Dec 18, 2020

@kevinlog @kobelb Can you sync up to make sure . or no . are in sync?

@kevinlog
Copy link
Contributor

Thanks @ruflin

@kobelb @pjhampton

I was operating under the impression that there would be no leading . in 7.11 originally as I recalled as much from our conversations on index naming during our discussions with the working group.

I understand that the preference is to add the leading .. As a result, I'm happy to take a look at what needs to be done to accomplish this.

Yesterday, I was working on e2e testing everything. I had these observations requiring at least 3 changes:

  1. We'll need to have Fleet add the . when a data_stream is marked as hidden. I prepared this PR to do that: [Fleet] add leading dot to hidden indices #86277

  2. @pjhampton we'll need to merge the change you prepared: [7.11][Telemetry] Fix diagnostic index name #86134

  3. From reading Endpoint logs, I'm pretty sure the Endpoint sends documents to an index without the leading ., so we'll need a change there. @ferullo let me know if that's the case. I can edit this comment in a bit with some example logs I was seeing.

Making the Kibana changes locally and testing would be easy. I'd like for us to also test with a dev Endpoint which will stream docs to the index with a leading . before merging all the changes again.

In the meantime, I'll work on a full e2e with the existing workflow. We needed the team to enable diagnostic alerts globally before the full e2e was possible. This tells the Endpoint that it can stream diagnostic alerts. From the conversation yesterday, that should be enabled today.

FYI @joe-desimone @tsg @stevewritescode

@pjhampton
Copy link
Contributor

pjhampton commented Dec 18, 2020

Thanks, @kevinlog - re-opened that PR here: #86468
Let me know if there is anything I can help with in regards to the e2e testing

@ferullo
Copy link
Contributor

ferullo commented Dec 18, 2020

@kevinlog you are correct, endpoint sends without the leading dot. All indices Endpoint sends to do not have a leading dot when it sends, i think the pipeline handles adding it.

If this does need to be changed on the Endpoint side can you just make an endpoint-dev issue? I'll jump on it right away. It's easy for Endpoint to make this change, I just want validate that Endpoint should make the change.

@kevinlog
Copy link
Contributor

kevinlog commented Dec 22, 2020

I have an update regarding the testing the leading ..

Right now, it looks like the fleet_enroll user that the Endpoint uses to create indices needs additional permissions. More details below.

Locally, I tested with the index being named as .logs-endpoint.diagnostic.collection-*, see this PR: #86277
I also tested with a dev Endpoint from Dan's PR here: https://github.com/elastic/endpoint-dev/issues/8225

I generated the alerts on my Endpoint and confirmed that the Endpoint attempted to create the alert and send it to .logs-endpoint.diagnostic.collection-*.

However, the it failed to create the index with this in the log message:

{\"ruleset\":\"diagnostic\"}}) reason (action [indices:admin/auto_create] is unauthorized for API key id [KrvCi3YB96EBa5C9k2Ce] of user [fleet_enroll] on indices [.logs-endpoint.diagnostic.collection-default], this action is granted by the privileges [auto_configure,create_index,manage,all]) status (403)","process":{"pid":2212,"thread":{"id":2688}}}

In the UI, I can see that the index template is a system template when I add the leading .
image

The index template is a fleet managed template when I leave off the leading .
image

We've confirmed e2e that what we have worked WITHOUT the leading .

I think if we could get the fleet_enroll user permission to create the index WITH the leading ., the it would still work.

cc @pjhampton @tsg @ruflin @ferullo - let me know if the above makes sense

@ruflin
Copy link
Member

ruflin commented Dec 23, 2020

I didn't know that the UI would show a System template when prefixed. This is interesting because I'm pretty sure it also contains the managed flag.

For the permissions, the place you are looking for is here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/fleet/server/services/setup.ts#L140 I would suggest to add very specific permissions for this data stream in there. But there is a catch with this. All the existing API keys will not get their permissions updated, so only newly enrolled endpoints will have the permissions :-( We plan to implement something around "refresh" API Keys but not there yet. @nchaulet One more use cause of this.

@kevinlog
Copy link
Contributor

@ruflin thanks for the input. I tried adding some additional indices and permissions here: f62bf54

I tried to be more specific since we just want to target a particular data_stream.

I was running into the same problems as before in my testing (same log output as in this comment #85391 (comment)).

I'm going to keep trying, but let me know if there's anything that looks wrong in the code sample I pushed up. As of right now, if we merge these changes, I believe there will be a regression since the Endpoint can't create the . version of the index.

cc @joe-desimone @stevewritescode @tsg @pjhampton

@kevinlog
Copy link
Contributor

Good news, I've got the leading . change to work e2e.

My PR now has the correct changes in permissions to allow the Endpoint to create the hidden/. index. Here are my changes (Same PR as before): #86277

How I tested/results
I've run the dev Endpoint that included @ferullo change here: https://github.com/elastic/endpoint-dev/pull/8228

After executing the ransomware script, the Endpoint sent the alert to the diagnostic index.

The index template created by Fleet:
image

The data_stream created by the Endpoint after the ransomware alert is sent:
image

image

The index:
image

Here are the 3 PRs that would need to merge to include the . in the index pattern.

This looks like it's working, so I would be OK to merge these changes. If we determine the risk is too high, the alternative is to change the index name in the ES PR: elastic/elasticsearch#66135

Only newly enrolled Endpoints would ship diagnostic alerts
The one caveat here as @ruflin pointed out, is that the existing API keys will NOT update with this change. So only newly enrolled Endpoints will have the correct permissions to ship the diagnostic alerts. This means that we'll only get data from new Agents that users stand up. AFAIK, even upgraded Endpoints won't be able to ship the diagnostic alerts, because the older API keys will still be around. Ruflin can correct me if I'm wrong.

cc\ @joe-desimone @stevewritescode @tsg @pjhampton @ferullo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.
Projects
None yet
7 participants