From de4bcdb9d9a0fbd135c0f179c2f28f25134544d7 Mon Sep 17 00:00:00 2001 From: John Schulz Date: Wed, 14 Apr 2021 19:27:06 -0400 Subject: [PATCH] [Fleet] Rename `force` to `revoke` agent unenroll APIs (#97041) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - fcbc9d9 Rename `force` param to `revoke` for `/agents/{agent_id}/unenroll` & `/agents/bulk_unenroll` - 03b9b90 Add new `force` param See https://github.com/elastic/kibana/issues/96873 for background
Unenroll AgentRevoke API Keys
RegularHosted
Rename force to revoke
Current force=false|undefined✅❌❌
Proposed revoke=false|undefined✅❌❌
Current force=true✅❌✅
Proposed revoke=true✅❌✅
Change force param
Proposed force=false|undefined✅❌❌
Proposed force=true✅✅❌
Proposed force=true & revoke=true✅✅✅
### 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 ### Changes required for consumers Any call to `/agents/{agent_id}/unenroll` & `/agents/bulk_unenroll` which passes the `force` param should change to `revoke` to maintain the current behavior. --- .../server/routes/agent/unenroll_handler.ts | 10 +- .../fleet/server/services/agents/acks.ts | 2 +- .../server/services/agents/unenroll.test.ts | 140 +++++++++++++++++- .../fleet/server/services/agents/unenroll.ts | 126 +++++++++------- .../fleet/server/types/rest_spec/agent.ts | 4 +- .../apis/agents/reassign.ts | 2 +- .../apis/agents/unenroll.ts | 14 +- .../apis/agents/upgrade.ts | 4 +- 8 files changed, 224 insertions(+), 78 deletions(-) diff --git a/x-pack/plugins/fleet/server/routes/agent/unenroll_handler.ts b/x-pack/plugins/fleet/server/routes/agent/unenroll_handler.ts index 150595521551..40dbc2fc49e6 100644 --- a/x-pack/plugins/fleet/server/routes/agent/unenroll_handler.ts +++ b/x-pack/plugins/fleet/server/routes/agent/unenroll_handler.ts @@ -28,11 +28,10 @@ export const postAgentUnenrollHandler: RequestHandler< const soClient = context.core.savedObjects.client; const esClient = context.core.elasticsearch.client.asInternalUser; try { - if (request.body?.force === true) { - await AgentService.forceUnenrollAgent(soClient, esClient, request.params.agentId); - } else { - await AgentService.unenrollAgent(soClient, esClient, request.params.agentId); - } + await AgentService.unenrollAgent(soClient, esClient, request.params.agentId, { + force: request.body?.force, + revoke: request.body?.revoke, + }); const body: PostAgentUnenrollResponse = {}; return response.ok({ body }); @@ -62,6 +61,7 @@ export const postBulkAgentsUnenrollHandler: RequestHandler< try { const results = await AgentService.unenrollAgents(soClient, esClient, { ...agentOptions, + revoke: request.body?.revoke, force: request.body?.force, }); const body = results.items.reduce((acc, so) => { diff --git a/x-pack/plugins/fleet/server/services/agents/acks.ts b/x-pack/plugins/fleet/server/services/agents/acks.ts index a29937e1257e..3acdfc2708ea 100644 --- a/x-pack/plugins/fleet/server/services/agents/acks.ts +++ b/x-pack/plugins/fleet/server/services/agents/acks.ts @@ -81,7 +81,7 @@ export async function acknowledgeAgentActions( const isAgentUnenrolled = actions.some((action) => action.type === 'UNENROLL'); if (isAgentUnenrolled) { - await forceUnenrollAgent(soClient, esClient, agent.id); + await forceUnenrollAgent(soClient, esClient, agent); } const upgradeAction = actions.find((action) => action.type === 'UPGRADE'); diff --git a/x-pack/plugins/fleet/server/services/agents/unenroll.test.ts b/x-pack/plugins/fleet/server/services/agents/unenroll.test.ts index 3d0692c24209..938ece1364b4 100644 --- a/x-pack/plugins/fleet/server/services/agents/unenroll.test.ts +++ b/x-pack/plugins/fleet/server/services/agents/unenroll.test.ts @@ -46,7 +46,7 @@ describe('unenrollAgent (singular)', () => { expect(calledWith[0]?.body).toHaveProperty('doc.unenrollment_started_at'); }); - it('cannot unenroll from managed policy', async () => { + it('cannot unenroll from managed policy by default', async () => { const { soClient, esClient } = createClientMock(); await expect(unenrollAgent(soClient, esClient, agentInManagedDoc._id)).rejects.toThrowError( AgentUnenrollmentError @@ -54,6 +54,35 @@ describe('unenrollAgent (singular)', () => { // does not call ES update expect(esClient.update).toBeCalledTimes(0); }); + + it('cannot unenroll from managed policy with revoke=true', async () => { + const { soClient, esClient } = createClientMock(); + await expect( + unenrollAgent(soClient, esClient, agentInManagedDoc._id, { revoke: true }) + ).rejects.toThrowError(AgentUnenrollmentError); + // does not call ES update + expect(esClient.update).toBeCalledTimes(0); + }); + + it('can unenroll from managed policy with force=true', async () => { + const { soClient, esClient } = createClientMock(); + await unenrollAgent(soClient, esClient, agentInManagedDoc._id, { force: true }); + // calls ES update with correct values + expect(esClient.update).toBeCalledTimes(1); + const calledWith = esClient.update.mock.calls[0]; + expect(calledWith[0]?.id).toBe(agentInManagedDoc._id); + expect(calledWith[0]?.body).toHaveProperty('doc.unenrollment_started_at'); + }); + + it('can unenroll from managed policy with force=true and revoke=true', async () => { + const { soClient, esClient } = createClientMock(); + await unenrollAgent(soClient, esClient, agentInManagedDoc._id, { force: true, revoke: true }); + // calls ES update with correct values + expect(esClient.update).toBeCalledTimes(1); + const calledWith = esClient.update.mock.calls[0]; + expect(calledWith[0]?.id).toBe(agentInManagedDoc._id); + expect(calledWith[0]?.body).toHaveProperty('doc.unenrolled_at'); + }); }); describe('unenrollAgents (plural)', () => { @@ -68,13 +97,12 @@ describe('unenrollAgents (plural)', () => { .filter((i: any) => i.update !== undefined) .map((i: any) => i.update._id); const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc); - expect(ids).toHaveLength(2); expect(ids).toEqual(idsToUnenroll); for (const doc of docs) { expect(doc).toHaveProperty('unenrollment_started_at'); } }); - it('cannot unenroll from a managed policy', async () => { + it('cannot unenroll from a managed policy by default', async () => { const { soClient, esClient } = createClientMock(); const idsToUnenroll = [ @@ -91,12 +119,116 @@ describe('unenrollAgents (plural)', () => { .filter((i: any) => i.update !== undefined) .map((i: any) => i.update._id); const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc); - expect(ids).toHaveLength(onlyUnmanaged.length); expect(ids).toEqual(onlyUnmanaged); for (const doc of docs) { expect(doc).toHaveProperty('unenrollment_started_at'); } }); + + it('cannot unenroll from a managed policy with revoke=true', async () => { + const { soClient, esClient } = createClientMock(); + + const idsToUnenroll = [ + agentInUnmanagedDoc._id, + agentInManagedDoc._id, + agentInUnmanagedDoc2._id, + ]; + + const unenrolledResponse = await unenrollAgents(soClient, esClient, { + agentIds: idsToUnenroll, + revoke: true, + }); + + expect(unenrolledResponse.items).toMatchObject([ + { + id: 'agent-in-unmanaged-policy', + success: true, + }, + { + id: 'agent-in-managed-policy', + success: false, + }, + { + id: 'agent-in-unmanaged-policy2', + success: true, + }, + ]); + + // calls ES update with correct values + const onlyUnmanaged = [agentInUnmanagedDoc._id, agentInUnmanagedDoc2._id]; + const calledWith = esClient.bulk.mock.calls[0][0]; + const ids = calledWith?.body + .filter((i: any) => i.update !== undefined) + .map((i: any) => i.update._id); + const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc); + expect(ids).toEqual(onlyUnmanaged); + for (const doc of docs) { + expect(doc).toHaveProperty('unenrolled_at'); + } + }); + + it('can unenroll from managed policy with force=true', async () => { + const { soClient, esClient } = createClientMock(); + const idsToUnenroll = [ + agentInUnmanagedDoc._id, + agentInManagedDoc._id, + agentInUnmanagedDoc2._id, + ]; + await unenrollAgents(soClient, esClient, { agentIds: idsToUnenroll, force: true }); + + // calls ES update with correct values + const calledWith = esClient.bulk.mock.calls[1][0]; + const ids = calledWith?.body + .filter((i: any) => i.update !== undefined) + .map((i: any) => i.update._id); + const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc); + expect(ids).toEqual(idsToUnenroll); + for (const doc of docs) { + expect(doc).toHaveProperty('unenrollment_started_at'); + } + }); + + it('can unenroll from managed policy with force=true and revoke=true', async () => { + const { soClient, esClient } = createClientMock(); + + const idsToUnenroll = [ + agentInUnmanagedDoc._id, + agentInManagedDoc._id, + agentInUnmanagedDoc2._id, + ]; + + const unenrolledResponse = await unenrollAgents(soClient, esClient, { + agentIds: idsToUnenroll, + revoke: true, + force: true, + }); + + expect(unenrolledResponse.items).toMatchObject([ + { + id: 'agent-in-unmanaged-policy', + success: true, + }, + { + id: 'agent-in-managed-policy', + success: true, + }, + { + id: 'agent-in-unmanaged-policy2', + success: true, + }, + ]); + + // calls ES update with correct values + const calledWith = esClient.bulk.mock.calls[0][0]; + const ids = calledWith?.body + .filter((i: any) => i.update !== undefined) + .map((i: any) => i.update._id); + const docs = calledWith?.body.filter((i: any) => i.doc).map((i: any) => i.doc); + expect(ids).toEqual(idsToUnenroll); + for (const doc of docs) { + expect(doc).toHaveProperty('unenrolled_at'); + } + }); }); function createClientMock() { diff --git a/x-pack/plugins/fleet/server/services/agents/unenroll.ts b/x-pack/plugins/fleet/server/services/agents/unenroll.ts index 59ec3a0a6320..85bc5eecd78b 100644 --- a/x-pack/plugins/fleet/server/services/agents/unenroll.ts +++ b/x-pack/plugins/fleet/server/services/agents/unenroll.ts @@ -39,10 +39,18 @@ async function unenrollAgentIsAllowed( export async function unenrollAgent( soClient: SavedObjectsClientContract, esClient: ElasticsearchClient, - agentId: string + agentId: string, + options?: { + force?: boolean; + revoke?: boolean; + } ) { - await unenrollAgentIsAllowed(soClient, esClient, agentId); - + if (!options?.force) { + await unenrollAgentIsAllowed(soClient, esClient, agentId); + } + if (options?.revoke) { + return forceUnenrollAgent(soClient, esClient, agentId); + } const now = new Date().toISOString(); await createAgentAction(soClient, esClient, { agent_id: agentId, @@ -57,15 +65,17 @@ export async function unenrollAgent( export async function unenrollAgents( soClient: SavedObjectsClientContract, esClient: ElasticsearchClient, - options: GetAgentsOptions & { force?: boolean } + options: GetAgentsOptions & { + force?: boolean; + revoke?: boolean; + } ): Promise<{ items: BulkActionResult[] }> { // start with all agents specified const givenAgents = await getAgents(esClient, options); - const outgoingErrors: Record = {}; // Filter to those not already unenrolled, or unenrolling const agentsEnrolled = givenAgents.filter((agent) => { - if (options.force) { + if (options.revoke) { return !agent.unenrolled_at; } return !agent.unenrollment_started_at && !agent.unenrolled_at; @@ -76,34 +86,23 @@ export async function unenrollAgents( unenrollAgentIsAllowed(soClient, esClient, agent.id).then((_) => agent) ) ); - const agentsToUpdate = agentResults.reduce((agents, result, index) => { - if (result.status === 'fulfilled') { - agents.push(result.value); - } else { - const id = givenAgents[index].id; - outgoingErrors[id] = result.reason; - } - return agents; - }, []); + const outgoingErrors: Record = {}; + const agentsToUpdate = options.force + ? agentsEnrolled + : agentResults.reduce((agents, result, index) => { + if (result.status === 'fulfilled') { + agents.push(result.value); + } else { + const id = givenAgents[index].id; + outgoingErrors[id] = result.reason; + } + return agents; + }, []); const now = new Date().toISOString(); - if (options.force) { + if (options.revoke) { // Get all API keys that need to be invalidated - const apiKeys = agentsToUpdate.reduce((keys, agent) => { - if (agent.access_api_key_id) { - keys.push(agent.access_api_key_id); - } - if (agent.default_api_key_id) { - keys.push(agent.default_api_key_id); - } - - return keys; - }, []); - - // Invalidate all API keys - if (apiKeys.length) { - await APIKeyService.invalidateAPIKeys(apiKeys); - } + await invalidateAPIKeysForAgents(agentsToUpdate); } else { // Create unenroll action for each agent await bulkCreateAgentActions( @@ -118,7 +117,7 @@ export async function unenrollAgents( } // Update the necessary agents - const updateData = options.force + const updateData = options.revoke ? { unenrolled_at: now, active: false } : { unenrollment_started_at: now }; @@ -127,39 +126,52 @@ export async function unenrollAgents( agentsToUpdate.map(({ id }) => ({ agentId: id, data: updateData })) ); - const out = { - items: givenAgents.map((agent, index) => { - const hasError = agent.id in outgoingErrors; - const result: BulkActionResult = { - id: agent.id, - success: !hasError, - }; - if (hasError) { - result.error = outgoingErrors[agent.id]; - } - return result; - }), + const getResultForAgent = (agent: Agent) => { + const hasError = agent.id in outgoingErrors; + const result: BulkActionResult = { + id: agent.id, + success: !hasError, + }; + if (hasError) { + result.error = outgoingErrors[agent.id]; + } + return result; }; - return out; + + return { + items: givenAgents.map(getResultForAgent), + }; +} + +export async function invalidateAPIKeysForAgents(agents: Agent[]) { + const apiKeys = agents.reduce((keys, agent) => { + if (agent.access_api_key_id) { + keys.push(agent.access_api_key_id); + } + if (agent.default_api_key_id) { + keys.push(agent.default_api_key_id); + } + + return keys; + }, []); + + if (apiKeys.length) { + await APIKeyService.invalidateAPIKeys(apiKeys); + } } export async function forceUnenrollAgent( soClient: SavedObjectsClientContract, esClient: ElasticsearchClient, - agentId: string + agentIdOrAgent: string | Agent ) { - const agent = await getAgentById(esClient, agentId); + const agent = + typeof agentIdOrAgent === 'string' + ? await getAgentById(esClient, agentIdOrAgent) + : agentIdOrAgent; - await Promise.all([ - agent.access_api_key_id - ? APIKeyService.invalidateAPIKeys([agent.access_api_key_id]) - : undefined, - agent.default_api_key_id - ? APIKeyService.invalidateAPIKeys([agent.default_api_key_id]) - : undefined, - ]); - - await updateAgent(esClient, agentId, { + await invalidateAPIKeysForAgents([agent]); + await updateAgent(esClient, agent.id, { active: false, unenrolled_at: new Date().toISOString(), }); diff --git a/x-pack/plugins/fleet/server/types/rest_spec/agent.ts b/x-pack/plugins/fleet/server/types/rest_spec/agent.ts index e74a4e6ed55b..a58849ee4ab4 100644 --- a/x-pack/plugins/fleet/server/types/rest_spec/agent.ts +++ b/x-pack/plugins/fleet/server/types/rest_spec/agent.ts @@ -172,7 +172,8 @@ export const PostAgentUnenrollRequestSchema = { }), body: schema.nullable( schema.object({ - force: schema.boolean(), + force: schema.maybe(schema.boolean()), + revoke: schema.maybe(schema.boolean()), }) ), }; @@ -181,6 +182,7 @@ export const PostBulkAgentUnenrollRequestSchema = { body: schema.object({ agents: schema.oneOf([schema.arrayOf(schema.string()), schema.string()]), force: schema.maybe(schema.boolean()), + revoke: schema.maybe(schema.boolean()), }), }; diff --git a/x-pack/test/fleet_api_integration/apis/agents/reassign.ts b/x-pack/test/fleet_api_integration/apis/agents/reassign.ts index 5737794eefea..f8a38913ecfe 100644 --- a/x-pack/test/fleet_api_integration/apis/agents/reassign.ts +++ b/x-pack/test/fleet_api_integration/apis/agents/reassign.ts @@ -181,7 +181,7 @@ export default function (providerContext: FtrProviderContext) { .post(`/api/fleet/agents/bulk_reassign`) .set('kbn-xsrf', 'xxx') .send({ - agents: 'fleet-agents.active: true', + agents: 'active: true', policy_id: 'policy2', }) .expect(200); diff --git a/x-pack/test/fleet_api_integration/apis/agents/unenroll.ts b/x-pack/test/fleet_api_integration/apis/agents/unenroll.ts index d7e16b7e7224..64665d87c82d 100644 --- a/x-pack/test/fleet_api_integration/apis/agents/unenroll.ts +++ b/x-pack/test/fleet_api_integration/apis/agents/unenroll.ts @@ -94,12 +94,12 @@ export default function (providerContext: FtrProviderContext) { await supertest.post(`/api/fleet/agents/agent1/unenroll`).set('kbn-xsrf', 'xxx').expect(200); }); - it('/agents/{agent_id}/unenroll { force: true } should invalidate related API keys', async () => { + it('/agents/{agent_id}/unenroll { revoke: true } should invalidate related API keys', async () => { await supertest .post(`/api/fleet/agents/agent1/unenroll`) .set('kbn-xsrf', 'xxx') .send({ - force: true, + revoke: true, }) .expect(200); @@ -116,7 +116,7 @@ export default function (providerContext: FtrProviderContext) { expect(outputAPIKeys[0].invalidated).eql(true); }); - it('/agents/{agent_id}/bulk_unenroll should not allow unenroll from managed policy', async () => { + it('/agents/bulk_unenroll should not allow unenroll from managed policy', async () => { // set policy to managed await supertest .put(`/api/fleet/agent_policies/policy1`) @@ -157,7 +157,7 @@ export default function (providerContext: FtrProviderContext) { expect(agent2data.body.item.active).to.eql(true); }); - it('/agents/{agent_id}/bulk_unenroll should allow to unenroll multiple agents by id from an unmanaged policy', async () => { + it('/agents/bulk_unenroll should allow to unenroll multiple agents by id from an unmanaged policy', async () => { // set policy to unmanaged await supertest .put(`/api/fleet/agent_policies/policy1`) @@ -187,13 +187,13 @@ export default function (providerContext: FtrProviderContext) { expect(agent2data.body.item.active).to.eql(true); }); - it('/agents/{agent_id}/bulk_unenroll should allow to unenroll multiple agents by kuery', async () => { + it('/agents/bulk_unenroll should allow to unenroll multiple agents by kuery', async () => { await supertest .post(`/api/fleet/agents/bulk_unenroll`) .set('kbn-xsrf', 'xxx') .send({ - agents: 'fleet-agents.active: true', - force: true, + agents: 'active: true', + revoke: true, }) .expect(200); diff --git a/x-pack/test/fleet_api_integration/apis/agents/upgrade.ts b/x-pack/test/fleet_api_integration/apis/agents/upgrade.ts index 008614f07551..545399134c79 100644 --- a/x-pack/test/fleet_api_integration/apis/agents/upgrade.ts +++ b/x-pack/test/fleet_api_integration/apis/agents/upgrade.ts @@ -167,7 +167,7 @@ export default function (providerContext: FtrProviderContext) { it('should respond 400 if trying to upgrade an agent that is unenrolling', async () => { const kibanaVersion = await kibanaServer.version.get(); await supertest.post(`/api/fleet/agents/agent1/unenroll`).set('kbn-xsrf', 'xxx').send({ - force: true, + revoke: true, }); await supertest .post(`/api/fleet/agents/agent1/upgrade`) @@ -331,7 +331,7 @@ export default function (providerContext: FtrProviderContext) { it('should not upgrade an unenrolling agent during bulk_upgrade', async () => { const kibanaVersion = await kibanaServer.version.get(); await supertest.post(`/api/fleet/agents/agent1/unenroll`).set('kbn-xsrf', 'xxx').send({ - force: true, + revoke: true, }); await es.update({ id: 'agent1',