-
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
[Fleet][Agent Policy][Agent Tamper Protection] UI / API guard agent tamper protection only available if security defend integration present #162196
Conversation
…g-with-defend-only
…g-with-defend-only
…g-with-defend-only
…g-with-defend-only
…g-with-defend-only
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.
Looking good! Thanks for work on this! Left few suggestions and questions. Let me know your thoughts.
export function isAgentPolicy( | ||
policy: Partial<NewAgentPolicy | AgentPolicy> | ||
): policy is AgentPolicy { | ||
return (policy as AgentPolicy).is_protected !== undefined; |
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.
Can we use other more specific prop in Agent policy to check if it is an agent policy or not?
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.
do you think revision
in AgentPolicy
works better maybe?
@@ -17,7 +17,6 @@ describe('generateNewAgentPolicyWithDefaults', () => { | |||
namespace: 'default', | |||
monitoring_enabled: ['logs', 'metrics'], | |||
inactivity_timeout: 1209600, | |||
is_protected: 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.
why are we removing this?
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 want to take this out since NewPackagePolicy
doesn't have package_policies
, and therefore the defend integration, during creation, and to prevent users from modifying this field. Even if a user were to add the defend integration to a new agent policy, as opposed to an existing one, the new agent policy is created first before adding the defend package policy, and then at this point, the is_protected
field would be available but defaulted to false.
@@ -21,7 +21,6 @@ export function generateNewAgentPolicyWithDefaults( | |||
namespace: 'default', | |||
monitoring_enabled: Object.values(dataTypes), | |||
inactivity_timeout: TWO_WEEKS_SECONDS, | |||
is_protected: 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.
same question here
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.
same as above - removing is_protected
from the NewAgentPolicy
type
@@ -33,7 +33,6 @@ export interface NewAgentPolicy { | |||
fleet_server_host_id?: string | null; | |||
schema_version?: string; | |||
agent_features?: Array<{ name: string; enabled: boolean }>; | |||
is_protected?: boolean; |
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.
This was optional, why are we removing this?
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.
same as above
@@ -118,5 +128,26 @@ 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 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?
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.
yes, the other tests above have the defend integration inside the package policy
id="xpack.fleet.agentPolicyForm.tamperingSwitchLabel" | ||
defaultMessage="Prevent agent tampering" | ||
/>{' '} | ||
{!policyHasEndpointSecurity(agentPolicy) && ( |
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.
Can we store and memoize the !policyHasEndpointSecurity(agentPolicy)
in a const and use it below?
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.
Same thing for the isAgentPolicy...
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 comment
The 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 comment
The 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?
@@ -237,8 +245,6 @@ class AgentPolicyService { | |||
savedObjectType: AGENT_POLICY_SAVED_OBJECT_TYPE, | |||
}); | |||
|
|||
this.checkTamperProtectionLicense(agentPolicy); |
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.
Why we remove this here?
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.
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 agentPoliciesWithEndpointPackagePolicies = [ | ||
...new Set( | ||
result | ||
.filter((r) => r.success && r.policy_id && r.package?.name === 'endpoint') |
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.
Could we use a single reduce
here instead?
@@ -1162,13 +1162,23 @@ class PackagePolicyClientImpl implements PackagePolicyClient { | |||
...new Set(result.filter((r) => r.success && r.policy_id).map((r) => r.policy_id!)), | |||
]; | |||
|
|||
const agentPoliciesWithEndpointPackagePolicies = [ |
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.
This could be a Set
, so no array transformation is needed. Then below you can use the .has
method from Set
instead of array inculdes
…g-with-defend-only
…g-with-defend-only
…g-with-defend-only
…g-with-defend-only
…g-with-defend-only
…g-with-defend-only
…g-with-defend-only
…g-with-defend-only
Pinging @elastic/security-defend-workflows (Team:Defend Workflows) |
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.
Fleet changes LGTM
@@ -27,7 +27,6 @@ describe('generateNewAgentPolicyWithDefaults', () => { | |||
description: 'test description', | |||
namespace: 'test-namespace', | |||
monitoring_enabled: ['logs'], | |||
is_protected: true, |
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 modify is_protected
as an override prop? What do you think @dasansol92
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 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.
<FormattedMessage | ||
id="xpack.fleet.agentPolicyForm.tamperingSwitchLabel" | ||
defaultMessage="Prevent agent tampering" | ||
/>{' '} |
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.
Is this empty space needed?
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.
Yes, otherwise the tooltip icon would be right up against the text
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 comment
The 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?
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.
LGTM
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.
Pending to update tooltip text. Other than that, it looks good! Thanks for working on this 🔥
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
…amper protection only available if security defend integration present (elastic#162196) ## Summary UI - [x] When there is no elastic defend integration present, the agent tamper protection (`is_protected`) switch and instruction link are disabled and there is an info tooltip explaining why the switch is disabled API - [x] Requires the elastic defend integration to be present, in order to set `is_protected` to true. Will allow the user to create the agent policy and not throw an error, but will keep `is_protected` as false and log a warning in the kibana server. In the next release, the response will be modified to send back a 201 with the relevant messaging. - [x] Sets `is_protected` to false when a user deletes the elastic defend package policy ## Screenshots ### No Elastic Defend integration installed <img width="970" alt="image" src="https://github.com/elastic/kibana/assets/56409205/910be766-1a1e-4580-9ace-306089b4626d">
…amper protection only available if security defend integration present (elastic#162196) ## Summary UI - [x] When there is no elastic defend integration present, the agent tamper protection (`is_protected`) switch and instruction link are disabled and there is an info tooltip explaining why the switch is disabled API - [x] Requires the elastic defend integration to be present, in order to set `is_protected` to true. Will allow the user to create the agent policy and not throw an error, but will keep `is_protected` as false and log a warning in the kibana server. In the next release, the response will be modified to send back a 201 with the relevant messaging. - [x] Sets `is_protected` to false when a user deletes the elastic defend package policy ## Screenshots ### No Elastic Defend integration installed <img width="970" alt="image" src="https://github.com/elastic/kibana/assets/56409205/910be766-1a1e-4580-9ace-306089b4626d">
Summary
UI
is_protected
) switch and instruction link are disabled and there is an info tooltip explaining why the switch is disabledAPI
is_protected
to true. Will allow the user to create the agent policy and not throw an error, but will keepis_protected
as false and log a warning in the kibana server. In the next release, the response will be modified to send back a 201 with the relevant messaging.is_protected
to false when a user deletes the elastic defend package policyScreenshots
No Elastic Defend integration installed