Skip to content

Commit

Permalink
[Fleet] RBAC - Make read agent actions space aware (elastic#189519)
Browse files Browse the repository at this point in the history
## Summary

Relates to elastic#185040

This PR makes the following Fleet agents API space aware (behind
`useSpaceAwareness` feature flag):
* `GET /agents/action_status`
* `POST /agents/{agentId}/actions`

I have already started work on `POST
/agents/{agentId}/actions/{actionId}/cancel` but I think it would best
grouped with `POST /agents/{agentId}/upgrade` and `POST
/agents/bulk_upgrade` in a separate PR.

### Details

#### GET /agents/action_status

⚠️ I have implemented the following logic in the action status service:
* If the `useSpaceAwareness` feature flag is disabled, there is no
change.
* In the default space, actions with `namespaces: ['default']` and those
with no `namespaces` property are returned.
* In a custom space, only actions with `namespaces: ['spaceName']` are
returned.

This is to ensure older actions with no `namespaces` property are not
lost. Feedback on this approach would be awesome.

NB: only tag update agent actions and agent policy update actions have a
`namespaces` property at the moment.

#### POST /agents/{agentId}/actions

* If the `useSpaceAwareness` feature flag is enabled, the action fails
if the agent is not in the current space.
* The `namespaces` property is populated when the action is created.

#### Other

This PR also fixes an issue with setting `namespaces` in agent actions
for tags update in the default space (this is because I didn't realise
`soClient.getCurrentNamespace()` returns `undefined` in the default
space).

⚠️ I also modified the `isAgentInNamespace` helper to return `true` in
the default space for agents with no explicitly set namespaces.

Finally, this PR introduces the following helpers:
* `getCurrentNamespace(soClient)`: this helper returns the string
`default` instead of `undefined` in the default space, which seems to be
the behaviour we want most of the time.
* `addNamespaceFilteringToQuery`: this helper extends the ES queries
used in the action status service to conditionally add filtering by
namespace as described above. It should be reusable for other endpoints
as well.
* The `isAgentInNamespace` and `agentsKueryNamespaceFilter` were moved
into the `fleet/server/services/spaces` folder where other space-related
helpers live.

### Testing

1. In order to test `GET /agents/action_status`, the best would be to
have a custom space and create a mix of agent and agent policy actions
across the default and the custom spaces, for instance:
   * Agent policy updates (change the policy description)
   * Update agent tags (creates agent actions with set `namespaces`)
* Unenroll agents (creates agent actions with no `namespaces` property)
Then check the output of `GET /agents/action_status` from the Dev Tools
and the UI (agent activity flyout):
   * Agent policy actions should only be listed in the relevant space.
* Update agent tags actions should only be listed in the relevant space.
   * Other actions should only be listed in the default space.
2. Test `POST /agents/{agentId}/actions` from the Dev Tools with any
action type, e.g. `"UNENROLL"`:
* If the agent is not in the current space, it should return not found.
* If the agent is in the current space, it should create an action with
the correct `namespaces` property.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
  • Loading branch information
jillguyonnet authored Aug 2, 2024
1 parent e67b460 commit 718f6c3
Show file tree
Hide file tree
Showing 20 changed files with 859 additions and 187 deletions.
2 changes: 2 additions & 0 deletions x-pack/plugins/fleet/server/routes/agent/actions_handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type {
import type { ActionsService } from '../../services/agents';
import type { PostNewAgentActionResponse } from '../../../common/types/rest_spec';
import { defaultFleetErrorHandler } from '../../errors';
import { getCurrentNamespace } from '../../services/spaces/get_current_namespace';

export const postNewAgentActionHandlerBuilder = function (
actionsService: ActionsService
Expand All @@ -39,6 +40,7 @@ export const postNewAgentActionHandlerBuilder = function (
created_at: new Date().toISOString(),
...newAgentAction,
agents: [agent.id],
namespaces: [getCurrentNamespace(soClient)],
});

const body: PostNewAgentActionResponse = {
Expand Down
15 changes: 10 additions & 5 deletions x-pack/plugins/fleet/server/routes/agent/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ import { defaultFleetErrorHandler, FleetNotFoundError } from '../../errors';
import * as AgentService from '../../services/agents';
import { fetchAndAssignAgentMetrics } from '../../services/agents/agent_metrics';
import { getAgentStatusForAgentPolicy } from '../../services/agents';
import { isAgentInNamespace } from '../../services/agents/namespace';
import { isAgentInNamespace } from '../../services/spaces/agent_namespaces';
import { getCurrentNamespace } from '../../services/spaces/get_current_namespace';

function verifyNamespace(agent: Agent, namespace?: string) {
if (!isAgentInNamespace(agent, namespace)) {
Expand All @@ -61,7 +62,7 @@ export const getAgentHandler: FleetRequestHandler<
const esClientCurrentUser = coreContext.elasticsearch.client.asCurrentUser;

let agent = await fleetContext.agentClient.asCurrentUser.getAgent(request.params.agentId);
verifyNamespace(agent, coreContext.savedObjects.client.getCurrentNamespace());
verifyNamespace(agent, getCurrentNamespace(coreContext.savedObjects.client));

if (request.query.withMetrics) {
agent = (await fetchAndAssignAgentMetrics(esClientCurrentUser, [agent]))[0];
Expand Down Expand Up @@ -91,7 +92,7 @@ export const deleteAgentHandler: FleetRequestHandler<

try {
const agent = await fleetContext.agentClient.asCurrentUser.getAgent(request.params.agentId);
verifyNamespace(agent, coreContext.savedObjects.client.getCurrentNamespace());
verifyNamespace(agent, getCurrentNamespace(coreContext.savedObjects.client));

await AgentService.deleteAgent(esClient, request.params.agentId);

Expand Down Expand Up @@ -131,7 +132,7 @@ export const updateAgentHandler: FleetRequestHandler<

try {
const agent = await fleetContext.agentClient.asCurrentUser.getAgent(request.params.agentId);
verifyNamespace(agent, coreContext.savedObjects.client.getCurrentNamespace());
verifyNamespace(agent, getCurrentNamespace(soClient));

await AgentService.updateAgent(esClient, request.params.agentId, partialAgent);
const body = {
Expand Down Expand Up @@ -387,7 +388,11 @@ export const getActionStatusHandler: RequestHandler<
const esClient = coreContext.elasticsearch.client.asInternalUser;

try {
const actionStatuses = await AgentService.getActionStatuses(esClient, request.query);
const actionStatuses = await AgentService.getActionStatuses(
esClient,
request.query,
getCurrentNamespace(coreContext.savedObjects.client)
);
const body: GetActionStatusResponse = { items: actionStatuses };
return response.ok({ body });
} catch (error) {
Expand Down
12 changes: 4 additions & 8 deletions x-pack/plugins/fleet/server/routes/enrollment_api_key/handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import { type RequestHandler, SavedObjectsErrorHelpers } from '@kbn/core/server';
import type { TypeOf } from '@kbn/config-schema';
import { DEFAULT_NAMESPACE_STRING } from '@kbn/core-saved-objects-utils-server';

import type {
GetEnrollmentAPIKeysRequestSchema,
Expand All @@ -25,6 +24,7 @@ import * as APIKeyService from '../../services/api_keys';
import { agentPolicyService } from '../../services/agent_policy';
import { defaultFleetErrorHandler, AgentPolicyNotFoundError } from '../../errors';
import { appContextService } from '../../services';
import { getCurrentNamespace } from '../../services/spaces/get_current_namespace';

export const getEnrollmentApiKeysHandler: RequestHandler<
undefined,
Expand All @@ -40,9 +40,7 @@ export const getEnrollmentApiKeysHandler: RequestHandler<
page: request.query.page,
perPage: request.query.perPage,
kuery: request.query.kuery,
spaceId: useSpaceAwareness
? soClient.getCurrentNamespace() ?? DEFAULT_NAMESPACE_STRING
: undefined,
spaceId: useSpaceAwareness ? getCurrentNamespace(soClient) : undefined,
});
const body: GetEnrollmentAPIKeysResponse = {
list: items, // deprecated
Expand Down Expand Up @@ -96,8 +94,7 @@ export const deleteEnrollmentApiKeyHandler: RequestHandler<
const { useSpaceAwareness } = appContextService.getExperimentalFeatures();
const coreContext = await context.core;
const esClient = coreContext.elasticsearch.client.asInternalUser;
const currentNamespace =
coreContext.savedObjects.client.getCurrentNamespace() ?? DEFAULT_NAMESPACE_STRING;
const currentNamespace = getCurrentNamespace(coreContext.savedObjects.client);
await APIKeyService.deleteEnrollmentApiKey(
esClient,
request.params.keyId,
Expand Down Expand Up @@ -126,8 +123,7 @@ export const getOneEnrollmentApiKeyHandler: RequestHandler<
try {
const coreContext = await context.core;
const esClient = coreContext.elasticsearch.client.asInternalUser;
const currentNamespace =
coreContext.savedObjects.client.getCurrentNamespace() ?? DEFAULT_NAMESPACE_STRING;
const currentNamespace = getCurrentNamespace(coreContext.savedObjects.client);
const { useSpaceAwareness } = appContextService.getExperimentalFeatures();

const apiKey = await APIKeyService.getEnrollmentAPIKey(
Expand Down
135 changes: 70 additions & 65 deletions x-pack/plugins/fleet/server/services/agents/action_status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
AGENT_POLICY_INDEX,
} from '../../../common';
import { appContextService } from '..';
import { addNamespaceFilteringToQuery } from '../spaces/query_namespaces_filtering';

/**
* Return current bulk actions.
Expand All @@ -35,10 +36,11 @@ import { appContextService } from '..';
*/
export async function getActionStatuses(
esClient: ElasticsearchClient,
options: ActionStatusOptions
options: ActionStatusOptions,
namespace?: string
): Promise<ActionStatus[]> {
const actionResults = await getActionResults(esClient, options);
const policyChangeActions = await getPolicyChangeActions(esClient, options);
const actionResults = await getActionResults(esClient, options, namespace);
const policyChangeActions = await getPolicyChangeActions(esClient, options, namespace);
const actionStatuses = [...actionResults, ...policyChangeActions]
.sort((a: ActionStatus, b: ActionStatus) => (b.creationTime > a.creationTime ? 1 : -1))
.slice(getPage(options), getPerPage(options));
Expand All @@ -47,9 +49,10 @@ export async function getActionStatuses(

async function getActionResults(
esClient: ElasticsearchClient,
options: ActionStatusOptions
options: ActionStatusOptions,
namespace?: string
): Promise<ActionStatus[]> {
const actions = await getActions(esClient, options);
const actions = await getActions(esClient, options, namespace);
const cancelledActions = await getCancelledActions(esClient);
let acks: any;

Expand Down Expand Up @@ -200,41 +203,41 @@ export function getPerPage(options: ActionStatusOptions) {

async function getActions(
esClient: ElasticsearchClient,
options: ActionStatusOptions
options: ActionStatusOptions,
namespace?: string
): Promise<ActionStatus[]> {
const query = {
bool: {
must_not: [
{
term: {
type: 'CANCEL',
},
},
],
...(options.date || options.latest
? {
filter: [
{
range: {
'@timestamp': {
// options.date overrides options.latest
gte: options.date ?? `now-${(options.latest ?? 0) / 1000}s/s`,
lte: options.date ? moment(options.date).add(1, 'days').toISOString() : 'now/s',
},
},
},
],
}
: {}),
},
};
const res = await esClient.search<FleetServerAgentAction>({
index: AGENT_ACTIONS_INDEX,
ignore_unavailable: true,
from: 0,
size: getPerPage(options),
query: {
bool: {
must_not: [
{
term: {
type: 'CANCEL',
},
},
],
...(options.date || options.latest
? {
filter: [
{
range: {
'@timestamp': {
// options.date overrides options.latest
gte: options.date ?? `now-${(options.latest ?? 0) / 1000}s/s`,
lte: options.date
? moment(options.date).add(1, 'days').toISOString()
: 'now/s',
},
},
},
],
}
: {}),
},
},
query: addNamespaceFilteringToQuery(query, namespace),
body: {
sort: [{ '@timestamp': 'desc' }],
},
Expand Down Expand Up @@ -340,49 +343,51 @@ export const hasRolloutPeriodPassed = (source: FleetServerAgentAction) =>

async function getPolicyChangeActions(
esClient: ElasticsearchClient,
options: ActionStatusOptions
options: ActionStatusOptions,
namespace?: string
): Promise<ActionStatus[]> {
// option.latest is used to fetch recent errors, which policy change actions do not contain
if (options.latest) {
return [];
}

const agentPoliciesRes = await esClient.search({
index: AGENT_POLICY_INDEX,
size: getPerPage(options),
query: {
bool: {
filter: [
{
range: {
revision_idx: {
gt: 1,
},
const query = {
bool: {
filter: [
{
range: {
revision_idx: {
gt: 1,
},
},
// This filter is for retrieving docs created by Kibana, as opposed to Fleet Server (coordinator_idx: 1).
// Note: the coordinator will be removed from Fleet Server (https://github.com/elastic/fleet-server/pull/3131),
// so this filter will eventually not be needed.
{
term: {
coordinator_idx: 0,
},
},
// This filter is for retrieving docs created by Kibana, as opposed to Fleet Server (coordinator_idx: 1).
// Note: the coordinator will be removed from Fleet Server (https://github.com/elastic/fleet-server/pull/3131),
// so this filter will eventually not be needed.
{
term: {
coordinator_idx: 0,
},
...(options.date
? [
{
range: {
'@timestamp': {
gte: options.date,
lte: moment(options.date).add(1, 'days').toISOString(),
},
},
...(options.date
? [
{
range: {
'@timestamp': {
gte: options.date,
lte: moment(options.date).add(1, 'days').toISOString(),
},
},
]
: []),
],
},
},
]
: []),
],
},
};
const agentPoliciesRes = await esClient.search({
index: AGENT_POLICY_INDEX,
size: getPerPage(options),
query: addNamespaceFilteringToQuery(query, namespace),
sort: [
{
'@timestamp': {
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/fleet/server/services/agents/agent_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey';

import type { SortResults } from '@elastic/elasticsearch/lib/api/types';

import { DEFAULT_NAMESPACE_STRING } from '@kbn/core-saved-objects-utils-server';

import type { AgentStatus, ListWithKuery } from '../../types';
import type { Agent, GetAgentStatusResponse } from '../../../common/types';
import { getAuthzFromRequest } from '../security';
import { appContextService } from '../app_context';
import { FleetUnauthorizedError } from '../../errors';

import { getCurrentNamespace } from '../spaces/get_current_namespace';

import { getAgentsByKuery, getAgentById } from './crud';
import { getAgentStatusById, getAgentStatusForAgentPolicy } from './status';
import { getLatestAvailableAgentVersion } from './versions';
Expand Down Expand Up @@ -183,7 +183,7 @@ export class AgentServiceImpl implements AgentService {
this.internalEsClient,
soClient,
preflightCheck,
soClient.getCurrentNamespace() ?? DEFAULT_NAMESPACE_STRING
getCurrentNamespace(soClient)
);
}

Expand Down
8 changes: 6 additions & 2 deletions x-pack/plugins/fleet/server/services/agents/crud.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ import {
AgentNotFoundError,
FleetUnauthorizedError,
} from '../../errors';

import { auditLoggingService } from '../audit_logging';
import { isAgentInNamespace } from '../spaces/agent_namespaces';
import { getCurrentNamespace } from '../spaces/get_current_namespace';

import { searchHitToAgent, agentSOAttributesToFleetServerAgentDoc } from './helpers';

import { buildAgentStatusRuntimeField } from './build_status_runtime_field';
import { getLatestAvailableAgentVersion } from './versions';

Expand Down Expand Up @@ -406,6 +406,10 @@ export async function getAgentById(
throw new AgentNotFoundError(`Agent ${agentId} not found`);
}

if (!isAgentInNamespace(agentHit, getCurrentNamespace(soClient))) {
throw new AgentNotFoundError(`${agentHit.id} not found in namespace`);
}

return agentHit;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,13 @@ import { AgentReassignmentError } from '../../errors';

import { SO_SEARCH_LIMIT } from '../../constants';

import { agentsKueryNamespaceFilter, isAgentInNamespace } from '../spaces/agent_namespaces';

import { getCurrentNamespace } from '../spaces/get_current_namespace';

import { getAgentsById, getAgentsByKuery, openPointInTime } from './crud';
import type { GetAgentsOptions } from '.';
import { UpdateAgentTagsActionRunner, updateTagsBatch } from './update_agent_tags_action_runner';
import { agentsKueryNamespaceFilter, isAgentInNamespace } from './namespace';

export async function updateAgentTags(
soClient: SavedObjectsClientContract,
Expand All @@ -26,7 +29,7 @@ export async function updateAgentTags(
): Promise<{ actionId: string }> {
const outgoingErrors: Record<Agent['id'], Error> = {};
const givenAgents: Agent[] = [];
const currentNameSpace = soClient.getCurrentNamespace();
const currentNameSpace = getCurrentNamespace(soClient);

if ('agentIds' in options) {
const maybeAgents = await getAgentsById(esClient, soClient, options.agentIds);
Expand Down
Loading

0 comments on commit 718f6c3

Please sign in to comment.