-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][Agent Policy][Agent Tamper Protection] UI / API guard agent tamper protection only available if security defend integration present #162196
Changes from 38 commits
022584f
ca2fc46
e197b47
ee9da8b
2723282
b3c30d0
23beb0c
c4e900e
64ab92d
06450af
bafa87e
72b1cff
55b797f
e6bd9ff
1eed50b
066cf6a
2c602b5
2be3016
e928028
dc92fba
9b01bd6
a201f24
4cb96e1
f5b03dd
0e6ab67
f22f105
e8b51a6
b600011
b792b7f
3f0b851
acc3a36
4d9ee38
a6f4af3
c3ac2ca
c28a708
30c1bb9
cb807eb
4fb9e69
165c68c
ecb40eb
f9e7abe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,13 @@ import { allowedExperimentalValues } from '../../../../../../../common/experimen | |
|
||
import { ExperimentalFeaturesService } from '../../../../../../services/experimental_features'; | ||
|
||
import type { NewAgentPolicy, AgentPolicy } from '../../../../../../../common/types'; | ||
import { createAgentPolicyMock, createPackagePolicyMock } from '../../../../../../../common/mocks'; | ||
import type { AgentPolicy, NewAgentPolicy } from '../../../../../../../common/types'; | ||
|
||
import { useLicense } from '../../../../../../hooks/use_license'; | ||
|
||
import type { LicenseService } from '../../../../../../../common/services'; | ||
import { generateNewAgentPolicyWithDefaults } from '../../../../../../../common/services'; | ||
|
||
import type { ValidationResults } from '../agent_policy_validation'; | ||
|
||
|
@@ -34,12 +36,7 @@ const mockedUseLicence = useLicense as jest.MockedFunction<typeof useLicense>; | |
describe('Agent policy advanced options content', () => { | ||
let testRender: TestRenderer; | ||
let renderResult: RenderResult; | ||
|
||
const mockAgentPolicy: Partial<NewAgentPolicy | AgentPolicy> = { | ||
name: 'some-agent-policy', | ||
is_managed: false, | ||
}; | ||
|
||
let mockAgentPolicy: Partial<NewAgentPolicy | AgentPolicy>; | ||
const mockUpdateAgentPolicy = jest.fn(); | ||
const mockValidation = jest.fn() as unknown as ValidationResults; | ||
const usePlatinumLicense = () => | ||
|
@@ -48,16 +45,34 @@ describe('Agent policy advanced options content', () => { | |
isPlatinum: () => true, | ||
} as unknown as LicenseService); | ||
|
||
const render = ({ isProtected = false, policyId = 'agent-policy-1' } = {}) => { | ||
const render = ({ | ||
isProtected = false, | ||
policyId = 'agent-policy-1', | ||
newAgentPolicy = false, | ||
packagePolicy = [createPackagePolicyMock()], | ||
} = {}) => { | ||
// remove when feature flag is removed | ||
ExperimentalFeaturesService.init({ | ||
...allowedExperimentalValues, | ||
agentTamperProtectionEnabled: true, | ||
}); | ||
|
||
if (newAgentPolicy) { | ||
mockAgentPolicy = generateNewAgentPolicyWithDefaults(); | ||
} else { | ||
mockAgentPolicy = { | ||
...createAgentPolicyMock(), | ||
package_policies: packagePolicy, | ||
id: policyId, | ||
}; | ||
} | ||
|
||
renderResult = testRender.render( | ||
<AgentPolicyAdvancedOptionsContent | ||
agentPolicy={{ ...mockAgentPolicy, is_protected: isProtected, id: policyId }} | ||
agentPolicy={{ | ||
...mockAgentPolicy, | ||
is_protected: isProtected, | ||
}} | ||
updateAgentPolicy={mockUpdateAgentPolicy} | ||
validation={mockValidation} | ||
/> | ||
|
@@ -118,5 +133,33 @@ describe('Agent policy advanced options content', () => { | |
}); | ||
expect(mockUpdateAgentPolicy).toHaveBeenCalledWith({ is_protected: true }); | ||
}); | ||
describe('when the defend integration is not installed', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have test cases for when it has Defend Integration? If not, can we add those? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, the other tests above have the defend integration inside the package policy |
||
beforeEach(() => { | ||
usePlatinumLicense(); | ||
render({ | ||
packagePolicy: [ | ||
{ | ||
...createPackagePolicyMock(), | ||
package: { name: 'not-endpoint', title: 'Not Endpoint', version: '0.1.0' }, | ||
}, | ||
], | ||
isProtected: true, | ||
}); | ||
}); | ||
it('should disable the switch and uninstall command link', () => { | ||
expect(renderResult.getByTestId('tamperProtectionSwitch')).toBeDisabled(); | ||
expect(renderResult.getByTestId('uninstallCommandLink')).toBeDisabled(); | ||
}); | ||
it('should show an icon tip explaining why the switch is disabled', () => { | ||
expect(renderResult.getByTestId('tamperMissingIntegrationTooltip')).toBeTruthy(); | ||
}); | ||
}); | ||
describe('when the user is creating a new agent policy', () => { | ||
it('should be disabled, since it has no package policies and therefore elastic defend integration is not installed', async () => { | ||
usePlatinumLicense(); | ||
render({ newAgentPolicy: true }); | ||
expect(renderResult.getByTestId('tamperProtectionSwitch')).toBeDisabled(); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
* 2.0. | ||
*/ | ||
|
||
import React, { useState } from 'react'; | ||
import React, { useState, useMemo } from 'react'; | ||
import { | ||
EuiDescribedFormGroup, | ||
EuiFormRow, | ||
|
@@ -46,6 +46,8 @@ import type { ValidationResults } from '../agent_policy_validation'; | |
|
||
import { ExperimentalFeaturesService, policyHasFleetServer } from '../../../../services'; | ||
|
||
import { policyHasEndpointSecurity as hasElasticDefend } from '../../../../../../../common/services'; | ||
|
||
import { | ||
useOutputOptions, | ||
useDownloadSourcesOptions, | ||
|
@@ -106,6 +108,7 @@ export const AgentPolicyAdvancedOptionsContent: React.FunctionComponent<Props> = | |
const { agentTamperProtectionEnabled } = ExperimentalFeaturesService.get(); | ||
const licenseService = useLicense(); | ||
const [isUninstallCommandFlyoutOpen, setIsUninstallCommandFlyoutOpen] = useState(false); | ||
const policyHasElasticDefend = useMemo(() => hasElasticDefend(agentPolicy), [agentPolicy]); | ||
|
||
return ( | ||
<> | ||
|
@@ -317,13 +320,31 @@ export const AgentPolicyAdvancedOptionsContent: React.FunctionComponent<Props> = | |
} | ||
> | ||
<EuiSwitch | ||
label={i18n.translate('xpack.fleet.agentPolicyForm.tamperingSwitchLabel', { | ||
defaultMessage: 'Prevent agent tampering', | ||
})} | ||
label={ | ||
<> | ||
<FormattedMessage | ||
id="xpack.fleet.agentPolicyForm.tamperingSwitchLabel" | ||
defaultMessage="Prevent agent tampering" | ||
/>{' '} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this empty space needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, otherwise the tooltip icon would be right up against the text |
||
{!policyHasElasticDefend && ( | ||
<span data-test-subj="tamperMissingIntegrationTooltip"> | ||
<EuiIconTip | ||
type="iInCircle" | ||
color="subdued" | ||
content={i18n.translate( | ||
'xpack.fleet.agentPolicyForm.tamperingSwitchLabel.disabledWarning', | ||
{ defaultMessage: 'this is why its disabled' } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we are waiting for a proper message here, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @roxana-gheorghe Do we know what we want to have for the disabled messaging when the defend integration is not present? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taking this back @roxana-gheorghe, do we have a proper text for this one? |
||
)} | ||
/> | ||
</span> | ||
)} | ||
</> | ||
} | ||
checked={agentPolicy.is_protected ?? false} | ||
onChange={(e) => { | ||
updateAgentPolicy({ is_protected: e.target.checked }); | ||
}} | ||
disabled={!policyHasElasticDefend} | ||
data-test-subj="tamperProtectionSwitch" | ||
/> | ||
{agentPolicy.id && ( | ||
|
@@ -333,7 +354,7 @@ export const AgentPolicyAdvancedOptionsContent: React.FunctionComponent<Props> = | |
onClick={() => { | ||
setIsUninstallCommandFlyoutOpen(true); | ||
}} | ||
disabled={agentPolicy.is_protected === false} | ||
disabled={!agentPolicy.is_protected || !policyHasElasticDefend} | ||
data-test-subj="uninstallCommandLink" | ||
> | ||
{i18n.translate('xpack.fleet.agentPolicyForm.tamperingUninstallLink', { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ import type { BulkResponseItem } from '@elastic/elasticsearch/lib/api/typesWithB | |
|
||
import { DEFAULT_SPACE_ID } from '@kbn/spaces-plugin/common/constants'; | ||
|
||
import { policyHasEndpointSecurity } from '../../common/services'; | ||
|
||
import { populateAssignedAgentsCount } from '../routes/agent_policy/handlers'; | ||
|
||
import type { HTTPAuthorizationHeader } from '../../common/http_authorization_header'; | ||
|
@@ -113,7 +115,10 @@ class AgentPolicyService { | |
id: string, | ||
agentPolicy: Partial<AgentPolicySOAttributes>, | ||
user?: AuthenticatedUser, | ||
options: { bumpRevision: boolean } = { bumpRevision: true } | ||
options: { bumpRevision: boolean; removeProtection: boolean } = { | ||
bumpRevision: true, | ||
removeProtection: false, | ||
} | ||
): Promise<AgentPolicy> { | ||
auditLoggingService.writeCustomSoAuditLog({ | ||
action: 'update', | ||
|
@@ -145,11 +150,14 @@ class AgentPolicyService { | |
await soClient.update<AgentPolicySOAttributes>(SAVED_OBJECT_TYPE, id, { | ||
...agentPolicy, | ||
...(options.bumpRevision ? { revision: existingAgentPolicy.revision + 1 } : {}), | ||
...(options.removeProtection | ||
? { is_protected: false } | ||
: { is_protected: agentPolicy.is_protected }), | ||
updated_at: new Date().toISOString(), | ||
updated_by: user ? user.username : 'system', | ||
}); | ||
|
||
if (options.bumpRevision) { | ||
if (options.bumpRevision || options.removeProtection) { | ||
await this.triggerAgentPolicyUpdatedEvent(soClient, esClient, 'updated', id); | ||
} | ||
|
||
|
@@ -239,6 +247,14 @@ class AgentPolicyService { | |
|
||
this.checkTamperProtectionLicense(agentPolicy); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we remove this here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since we are no longer sending in agent tamper protection on agent policy create, there is no need for a license check since agent tamper protection is the only licensed-gated feature |
||
|
||
const logger = appContextService.getLogger(); | ||
|
||
if (agentPolicy?.is_protected) { | ||
logger.warn( | ||
'Agent policy requires Elastic Defend integration to set tamper protection to true' | ||
); | ||
} | ||
|
||
await this.requireUniqueName(soClient, agentPolicy); | ||
|
||
await validateOutputForPolicy(soClient, agentPolicy); | ||
|
@@ -253,7 +269,7 @@ class AgentPolicyService { | |
updated_at: new Date().toISOString(), | ||
updated_by: options?.user?.username || 'system', | ||
schema_version: FLEET_AGENT_POLICIES_SCHEMA_VERSION, | ||
is_protected: agentPolicy.is_protected ?? false, | ||
is_protected: false, | ||
} as AgentPolicy, | ||
options | ||
); | ||
|
@@ -491,6 +507,16 @@ class AgentPolicyService { | |
|
||
this.checkTamperProtectionLicense(agentPolicy); | ||
|
||
const logger = appContextService.getLogger(); | ||
|
||
if (agentPolicy?.is_protected && !policyHasEndpointSecurity(existingAgentPolicy)) { | ||
logger.warn( | ||
'Agent policy requires Elastic Defend integration to set tamper protection to true' | ||
); | ||
// force agent policy to be false if elastic defend is not present | ||
agentPolicy.is_protected = false; | ||
} | ||
|
||
if (existingAgentPolicy.is_managed && !options?.force) { | ||
Object.entries(agentPolicy) | ||
.filter(([key]) => !KEY_EDITABLE_FOR_MANAGED_POLICIES.includes(key)) | ||
|
@@ -586,9 +612,12 @@ class AgentPolicyService { | |
soClient: SavedObjectsClientContract, | ||
esClient: ElasticsearchClient, | ||
id: string, | ||
options?: { user?: AuthenticatedUser } | ||
options?: { user?: AuthenticatedUser; removeProtection?: boolean } | ||
): Promise<AgentPolicy> { | ||
const res = await this._update(soClient, esClient, id, {}, options?.user); | ||
const res = await this._update(soClient, esClient, id, {}, options?.user, { | ||
bumpRevision: true, | ||
removeProtection: options?.removeProtection ?? false, | ||
}); | ||
|
||
return res; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add this again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could add with
false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could, but I wonder if it makes sense to have a test that changes this since we don't want to generate a new policy with
is_protected
being true since elastic defend is not present in a New Agent Policy. But I guess if we are just testing functionality, I could? Or maybe I should enforce the code so that the user can't modifyis_protected
as an override prop? What do you think @dasansol92There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could test both scenarios, when user sets it to false (it is ok) and when user sets it to true (and we turn it back to false). Also when the prop is not set, and we set it to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parkiino you were right, since this is only testing the
generateNewAgentPolicyWithDefaults
and not the API functionality, it's ok to keep it as it is (is_protected
set to false by default when it is not assigned). @juliaElastic let me know if you think different.