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][Agent Policy][Agent Tamper Protection] UI / API guard agent tamper protection only available if security defend integration present #162196

Merged
merged 41 commits into from
Aug 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
022584f
disable switch and show conditional icon tip if defend integration no…
parkiino Jul 12, 2023
ca2fc46
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Jul 13, 2023
e197b47
unit tests for ui
parkiino Jul 14, 2023
ee9da8b
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Jul 17, 2023
2723282
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Jul 18, 2023
b3c30d0
change tamper protection to false when endpoint package is deleted
parkiino Jul 25, 2023
23beb0c
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Jul 25, 2023
c4e900e
fix type errors
parkiino Jul 25, 2023
64ab92d
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Jul 25, 2023
06450af
wip remove is protected from agent policy create
parkiino Jul 27, 2023
bafa87e
fix type, fix test
parkiino Jul 27, 2023
72b1cff
update index
parkiino Jul 27, 2023
55b797f
fixing tests
parkiino Jul 27, 2023
e6bd9ff
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Jul 28, 2023
1eed50b
merge origin'
parkiino Jul 28, 2023
066cf6a
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Jul 31, 2023
2c602b5
remove is_protected from create agent policy tests
parkiino Jul 31, 2023
2be3016
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Jul 31, 2023
e928028
fix jest tests
parkiino Aug 1, 2023
dc92fba
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 1, 2023
9b01bd6
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 1, 2023
a201f24
fix logic and memoize
parkiino Aug 1, 2023
4cb96e1
fix agent tamper ui
parkiino Aug 2, 2023
f5b03dd
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 2, 2023
0e6ab67
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 3, 2023
f22f105
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 3, 2023
e8b51a6
fix tests please?
parkiino Aug 3, 2023
b600011
import
parkiino Aug 3, 2023
b792b7f
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 8, 2023
3f0b851
use agent policy mock
parkiino Aug 8, 2023
acc3a36
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 9, 2023
4d9ee38
fix package.json and one api integration test
parkiino Aug 10, 2023
a6f4af3
add back is_protected in newagentpolicy and SO, set to false in new a…
parkiino Aug 10, 2023
c3ac2ca
reenable more is protected stuff
parkiino Aug 10, 2023
c28a708
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 10, 2023
30c1bb9
add kibana warning message
parkiino Aug 10, 2023
cb807eb
adjust tests
parkiino Aug 11, 2023
4fb9e69
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 11, 2023
165c68c
update text to something more appropriate
parkiino Aug 11, 2023
ecb40eb
add a logger for when agent policy tamper protection is set to false
parkiino Aug 14, 2023
f9e7abe
Merge remote-tracking branch 'upstream/main' into task/agent-tamperin…
parkiino Aug 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions x-pack/plugins/fleet/common/services/agent_policies_helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,13 @@
* 2.0.
*/

import type { AgentPolicy } from '../types';
import { FLEET_SERVER_PACKAGE, FLEET_APM_PACKAGE, FLEET_SYNTHETICS_PACKAGE } from '../constants';
import type { NewAgentPolicy, AgentPolicy } from '../types';
import {
FLEET_SERVER_PACKAGE,
FLEET_APM_PACKAGE,
FLEET_SYNTHETICS_PACKAGE,
FLEET_ENDPOINT_PACKAGE,
} from '../constants';

export function policyHasFleetServer(agentPolicy: AgentPolicy) {
if (!agentPolicy.package_policies) {
Expand All @@ -26,6 +31,10 @@ export function policyHasSyntheticsIntegration(agentPolicy: AgentPolicy) {
return policyHasIntegration(agentPolicy, FLEET_SYNTHETICS_PACKAGE);
}

export function policyHasEndpointSecurity(agentPolicy: Partial<NewAgentPolicy | AgentPolicy>) {
return policyHasIntegration(agentPolicy as AgentPolicy, FLEET_ENDPOINT_PACKAGE);
}

function policyHasIntegration(agentPolicy: AgentPolicy, packageName: string) {
if (!agentPolicy.package_policies) {
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@
* 2.0.
*/

import { licenseMock } from '@kbn/licensing-plugin/common/licensing.mock';
import { pick } from 'lodash';

import { licenseMock } from '@kbn/licensing-plugin/common/licensing.mock';
import { createAgentPolicyMock } from '../mocks';

import {
isAgentPolicyValidForLicense,
unsetAgentPolicyAccordingToLicenseLevel,
} from './agent_policy_config';
import { generateNewAgentPolicyWithDefaults } from './generate_new_agent_policy';

describe('agent policy config and licenses', () => {
const Platinum = licenseMock.createLicense({ license: { type: 'platinum', mode: 'platinum' } });
Expand All @@ -34,13 +34,13 @@ describe('agent policy config and licenses', () => {
});
describe('unsetAgentPolicyAccordingToLicenseLevel', () => {
it('resets all paid features to default if license is gold', () => {
const defaults = pick(generateNewAgentPolicyWithDefaults(), 'is_protected');
const defaults = pick(createAgentPolicyMock(), 'is_protected');
const partialPolicy = { is_protected: true };
const retPolicy = unsetAgentPolicyAccordingToLicenseLevel(partialPolicy, Gold);
expect(retPolicy).toEqual(defaults);
});
it('does not change paid features if license is platinum', () => {
const expected = pick(generateNewAgentPolicyWithDefaults(), 'is_protected');
const expected = pick(createAgentPolicyMock(), 'is_protected');
const partialPolicy = { is_protected: false };
const expected2 = { is_protected: true };
const partialPolicy2 = { is_protected: true };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ describe('generateNewAgentPolicyWithDefaults', () => {
description: 'test description',
namespace: 'test-namespace',
monitoring_enabled: ['logs'],
is_protected: true,
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor Author

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 modify is_protected as an override prop? What do you think @dasansol92

Copy link
Contributor

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.

Copy link
Contributor

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.

});

expect(newAgentPolicy).toEqual({
Expand All @@ -36,7 +35,7 @@ describe('generateNewAgentPolicyWithDefaults', () => {
namespace: 'test-namespace',
monitoring_enabled: ['logs'],
inactivity_timeout: 1209600,
is_protected: true,
is_protected: false,
});
});
});
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/common/services/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ export { agentStatusesToSummary } from './agent_statuses_to_summary';
export {
policyHasFleetServer,
policyHasAPMIntegration,
policyHasEndpointSecurity,
policyHasSyntheticsIntegration,
} from './agent_policies_helpers';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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 = () =>
Expand All @@ -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}
/>
Expand Down Expand Up @@ -118,5 +133,33 @@ describe('Agent policy advanced options content', () => {
});
expect(mockUpdateAgentPolicy).toHaveBeenCalledWith({ is_protected: true });
});
describe('when the defend integration is not installed', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import React, { useState } from 'react';
import React, { useState, useMemo } from 'react';
import {
EuiDescribedFormGroup,
EuiFormRow,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 (
<>
Expand Down Expand Up @@ -317,13 +320,34 @@ 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"
/>{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this empty space needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:
'Elastic Defend integration is required to enable this feature',
}
)}
/>
</span>
)}
</>
}
checked={agentPolicy.is_protected ?? false}
onChange={(e) => {
updateAgentPolicy({ is_protected: e.target.checked });
}}
disabled={!policyHasElasticDefend}
data-test-subj="tamperProtectionSwitch"
/>
{agentPolicy.id && (
Expand All @@ -333,7 +357,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', {
Expand Down
45 changes: 40 additions & 5 deletions x-pack/plugins/fleet/server/services/agent_policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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',
Expand All @@ -136,6 +141,12 @@ class AgentPolicyService {
);
}

const logger = appContextService.getLogger();

if (options.removeProtection) {
logger.warn(`Setting tamper protection for Agent Policy ${id} to false`);
}

await validateOutputForPolicy(
soClient,
agentPolicy,
Expand All @@ -145,11 +156,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);
}

Expand Down Expand Up @@ -239,6 +253,14 @@ class AgentPolicyService {

this.checkTamperProtectionLicense(agentPolicy);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we remove this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Expand All @@ -253,7 +275,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
);
Expand Down Expand Up @@ -491,6 +513,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))
Expand Down Expand Up @@ -586,9 +618,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;
}
Expand Down
9 changes: 9 additions & 0 deletions x-pack/plugins/fleet/server/services/package_policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1161,13 +1161,22 @@ class PackagePolicyClientImpl implements PackagePolicyClient {
...new Set(result.filter((r) => r.success && r.policy_id).map((r) => r.policy_id!)),
];

const agentPoliciesWithEndpointPackagePolicies = result.reduce((acc, cur) => {
if (cur.success && cur.policy_id && cur.package?.name === 'endpoint') {
return acc.add(cur.policy_id);
}
return acc;
}, new Set());

const agentPolicies = await agentPolicyService.getByIDs(soClient, uniquePolicyIdsR);

for (const policyId of uniquePolicyIdsR) {
const agentPolicy = agentPolicies.find((p) => p.id === policyId);
if (agentPolicy) {
// is the agent policy attached to package policy with endpoint
await agentPolicyService.bumpRevision(soClient, esClient, policyId, {
user: options?.user,
removeProtection: agentPoliciesWithEndpointPackagePolicies.has(policyId),
});
}
}
Expand Down
Loading
Loading