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

[EDR Workflows] Option to sync antivirus registration with malware settings #180484

Conversation

gergoabraham
Copy link
Contributor

@gergoabraham gergoabraham commented Apr 10, 2024

Summary

Adds option to Defend integration's Antivirus Registration card to sync registration with Malware settings.

image

sync

Details:

  • it adds a new field to PolicyConfig: antivirus_registration.mode, which can be enabled, disabled or sync_with_malware_prevent
image
  • this field is not used by Endpoint: instead the existing antivirus_registration.enabled field is derived from this field, so it's compatible with older Endpoints, too
  • the calculation of antivirus_registration.enabled happens both on client side and on server side (in Fleet's packagePolicyUpdate ingest callback)
  • default value for new policy is the same: disabled, as in previous version
  • adds migration

Checklist

Delete any items that are not applicable to this PR.

@gergoabraham gergoabraham added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution labels Apr 10, 2024
@gergoabraham gergoabraham self-assigned this Apr 10, 2024
@gergoabraham gergoabraham requested review from a team as code owners April 10, 2024 15:06
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Apr 10, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

… src/core/server/integration_tests/ci_checks'
@gergoabraham gergoabraham requested a review from a team as a code owner April 10, 2024 15:37
Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Fleet changes 🚀

@gergoabraham
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@szwarckonrad szwarckonrad left a comment

Choose a reason for hiding this comment

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

Tested flow locally, everything propagates up to agent policy as expected.
I guess Endpoint doesn't mind this extra field in the config? :)
Great job 🚀 !

@@ -1000,6 +1000,7 @@ export interface PolicyConfig {
};
};
antivirus_registration: {
mode?: AntivirusRegistrationModes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is mode truly optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is optional only for one release cycle, afterwards it will be required - this is for supporting serverless rollout, see this comment: #179176 (comment)

import type { PolicyConfig } from '../types';
import { ProtectionModes, AntivirusRegistrationModes } from '../types';

export const updateAntivirusRegistrationEnabledInPlace = (policy: PolicyConfig): PolicyConfig => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe drop InPlace from the name? Spent way too much time trying to figure out what does it mean :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, also added JSOC 3d1bdba

),
[AntivirusRegistrationModes.sync]: i18n.translate(
'xpack.securitySolution.endpoint.policy.details.antivirusRegistration.syncWithMalwarePrevent',
{ defaultMessage: 'Sync with Malware Prevent' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{ defaultMessage: 'Sync with Malware Prevent' }
{ defaultMessage: 'Sync with Malware Protection level' }

Either way, I believe we could use a popover of sorts to explain what will happen on Prevent and Detect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, also added hint 3f23c6d

if (antivirusRegistrationMode) {
// calculate only if `mode` exists
policy.windows.antivirus_registration.enabled =
modeToEnabled[antivirusRegistrationMode] ?? false;
Copy link
Contributor

Choose a reason for hiding this comment

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

can modeToEnabled[antivirusRegistrationMode] be undefined?

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, if there is an invalid value in antivirusRegistrationMode. so it should not happen, but if it happens because of reasons, the safest is to keep antivirus registration disabled

@gergoabraham
Copy link
Contributor Author

gergoabraham commented Apr 11, 2024

@caitlinbetz @joepeeples @ferullo

what do you think about the tooltip text?
Using this setting will automatically enable antivirus registration if Malware protection is set to prevent. In any other case antivirus registration will be disabled. (update: changed cases to case)

also, I've displayed the current outcome of antivirus registration, so the user can see immediately what setting the antivirus registration to sync with malware results in. should we go with this?
sync

Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

Looks good. Left one question, but nothing that should hold this back. Thanks for all of the test coverage 🤩

'aria-checked',
'false'
);
expect(renderResult.getByTestId(antivirusTestSubj.radioButtons)).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change in this test case? it now just seem to be checking that its in the DOM and not really checking what state it is in (checked or unchecked)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point - I forgot to check it :)
fixed here 2b457bf

Copy link
Contributor

@joepeeples joepeeples left a comment

Choose a reason for hiding this comment

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

Thanks for including me in the review, @gergoabraham! Left a couple suggested edits, just let me know if you need anything else.

@dasansol92 dasansol92 requested a review from joepeeples April 15, 2024 08:19
@dasansol92
Copy link
Contributor

@elasticmachine merge upstream

@dasansol92
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5387 5388 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 17.2MB 17.2MB +4.2KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @gergoabraham

@dasansol92 dasansol92 merged commit 12ec883 into elastic:main Apr 15, 2024
37 checks passed
@gergoabraham gergoabraham deleted the sync-antivirus-registration-with-malware-settings branch April 17, 2024 08:09
gergoabraham added a commit that referenced this pull request May 3, 2024
…n.mode` from optional to required (#181986)

## Summary

`antivirus_registration.mode` field was added to Endpoint integration
policy config in this PR: #180484

It was optional to support roll-out, now it's changed to required as it
has been released since.

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Team:Fleet Team label for Observability Data Collection Fleet team v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.