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

[Fleet] Add scoped AgentService #119017

Merged
merged 18 commits into from
Dec 1, 2021
Merged

[Fleet] Add scoped AgentService #119017

merged 18 commits into from
Dec 1, 2021

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Nov 18, 2021

Summary

Closes #116182

This updates the agent service currently exposed on Fleet's server-side plugin API to now supply a "scoped" and "internal user" interface of the same client to allow plugins and Fleet's internal implementation code to get authz out-of-the-box on these actions and support the switch to system indices.

In this first iteration, I have only included the changes to support external usages in other plugins of the 4 currently supported APIs. In the next PR, I'll add the additional APIs we use internally in Fleet and switch over our implementation to use this service internally.

Example usage, from a HTTP request handler

const handler = async (context, req, res) => {
  // Will automatically throw a `FleetUnauthorizedError` if user does not have required access.
  // Consumers should catch `FleetUnauthorizedError` errors and return unauthorized HTTP responses
  const agents = await context.fleet.agentClient.asCurrentUser.listAgents();
}

From a server-side background process, for example in telemetry collectors:

const agentClient = startDeps.fleet.agentService.asInternalUser; // uses kibana_system user and never throws authz errors
const agents = await agentClient.listAgents();

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@nchaulet
Copy link
Member

Love that approach it could works well with Fleet superuser privilege requirement removal, and it's seems easy enough to consume.

@joshdover
Copy link
Contributor Author

@paul-tavares Would you be able to take over fixing the security solution issues in this PR?

@paul-tavares
Copy link
Contributor

@joshdover yes, I can do that. Will start now, but may need to go into Monday

@paul-tavares
Copy link
Contributor

Hi @joshdover ,
I just noticed that AgentService now requires us to pass in the KibanaRequest in order to get back a scoped AgentClient (the method asScoped) which makes it a little harder for us to share services outside of a HTTP Handler. Any way we can change that? I'm still looking at all the changes, so I don't have any good ideas just yet, but figure I would ask.

I'm also going to see if we (endpoint) can maybe wrap this. Maybe I'll change our services interface to instead receive in an AgentClient on input to the methods that needed it. that will make it easier to move off of asScoped in the future if find the need to.

) {}

asScoped(req: KibanaRequest): EndpointScopedFleetServicesInterface {
const {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: Endpoint team:
I would like at some point to maybe use this to also scope the the other services from fleet. Each of those normally takes in either an ES scoped client or a scoped SO client, so we could adjust the other services here so that we no longer have to pass in these client explicitly. Example for the packagePolicy.list() method:

{
  list: packagePolicy.list.bind(packagePolicy, soScopedClient)
}

and then we could just packagePolicy.list(options) - no more need to pass in the decencies (in this case the SO Client)

@joshdover
Copy link
Contributor Author

I just noticed that AgentService now requires us to pass in the KibanaRequest in order to get back a scoped AgentClient (the method asScoped) which makes it a little harder for us to share services outside of a HTTP Handler. Any way we can change that? I'm still looking at all the changes, so I don't have any good ideas just yet, but figure I would ask.

This is the purpose of the asInternalUser API which returns a client that will use the internal user without any explicit authz checks. Does that solve the use case?

@joshdover
Copy link
Contributor Author

I'm also going to see if we (endpoint) can maybe wrap this. Maybe I'll change our services interface to instead receive in an AgentClient on input to the methods that needed it. that will make it easier to move off of asScoped in the future if find the need to.

++ that was what I was thinking as well. Switch over to using AgentClient and no longer rely on the AgentService interface.

paul-tavares and others added 6 commits November 29, 2021 09:48
…coped-agent-service

# Conflicts:
#	x-pack/plugins/fleet/server/mocks/index.ts
#	x-pack/plugins/fleet/server/plugin.ts
#	x-pack/plugins/fleet/server/routes/setup/handlers.test.ts
#	x-pack/plugins/fleet/server/types/request_context.ts
#	x-pack/plugins/security_solution/server/endpoint/routes/metadata/handlers.ts
#	x-pack/plugins/security_solution/server/endpoint/services/metadata/endpoint_metadata_service.ts
@joshdover
Copy link
Contributor Author

@paul-tavares I noticed that endpoint has a few other code paths that query .fleet-actions and .fleet-action-results indices that are not using Fleet services (probably because we don't provide an API for these).

I plan to add a follow up PR that introduces APIs for querying these and I think it will be necessary to switch over those usages as well in order to support these indices becoming system indices (so the end user cannot access them out of the box, not even superuser).

@paul-tavares
Copy link
Contributor

Re: usage of .fleet-actions-* indexes

I'm not too familiar with discussions around those indexes, but I know that @pzl worked closely with the OSQuery team when we started to use them (I think OSQuery was the first). you mentioned creating an API, but I wonder if you mean providing a server-side service that we can consume - is that what you meant?

cc/ @ashokaditya FYI

@paul-tavares
Copy link
Contributor

@joshdover
FYI - we also interact with the .fleet-artifacts index, but do so using a Fleet provided server side service - this one:

https://github.com/elastic/kibana/blob/7ebffc3728a04fa806a29e39f5fd0eb87f514368/x-pack/plugins/fleet/server/services/artifacts/client.ts

@joshdover joshdover requested review from a team as code owners November 30, 2021 14:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@joshdover joshdover changed the title [WIP] Add scoped AgentService Add scoped AgentService Nov 30, 2021
@joshdover joshdover changed the title Add scoped AgentService [Fleet] Add scoped AgentService Nov 30, 2021
Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

Asset management LGTM
Thank you @joshdover

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

🚢 it 😄

x-pack/plugins/fleet/server/mocks/index.ts Outdated Show resolved Hide resolved
x-pack/plugins/fleet/server/plugin.ts Outdated Show resolved Hide resolved
Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

🚀

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
fleet 1145 1143 -2
Unknown metric groups

API count

id before after diff
fleet 1248 1249 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @joshdover @paul-tavares

@@ -32,7 +32,7 @@ import {

export class TelemetryReceiver {
private readonly logger: Logger;
private agentService?: AgentService;
private agentClient?: AgentClient;
Copy link
Contributor

Choose a reason for hiding this comment

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

Security Telemetry change looks good!

Copy link
Contributor

@pjhampton pjhampton left a comment

Choose a reason for hiding this comment

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

🌔 🚀 ✨ LGTM ✨ 🚀 🌔

Approving minor import change on security telemetry instrumentation.

@joshdover joshdover merged commit ad5afe1 into elastic:main Dec 1, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 1, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Dec 1, 2021
Co-authored-by: Paul Tavares <[email protected]>

Co-authored-by: Josh Dover <[email protected]>
Co-authored-by: Paul Tavares <[email protected]>
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
ashokaditya added a commit to ashokaditya/kibana that referenced this pull request Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed OLM Sprint release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Exposed service to manipulate fleet-* should use internal ES client
8 participants