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

Add windows defender problem detection custom plugin #555

Merged

Conversation

mcshooter
Copy link
Contributor

@mcshooter mcshooter commented May 10, 2021

Windows Defender helps detect any malware, viruses, etc on a windows node. If windows defender detects any threats, we should indicate that there is an issue on the node. So, a custom plugin has been added to monitor this.

Get-MpThreat currently will output an empty string if no threats are detected. Anything string output that is returned will indicate that there is a threat.
If a threat has been detected, we can check if that threat had been executed or is currently active to determine the health of out node. If either is true, we return with exit 1 to indicate a problem detected from windows defender

CategoryID       : 27
DidThreatExecute : False
IsActive         : False
Resources        : {file:_C:\Users\michelletandya\Downloads\PotentiallyUnwanted.exe, file:_C:\Users\michelletandya\Downloads\potentiallyUnwanted2.exe,
                   file:_C:\Users\michelletandya\Downloads\potentiallyUnwanted2.exe, file:_C:\Users\michelletandya\Downloads\PotentiallyUnwanted (1).exe}
RollupStatus     : 33
SchemaVersion    : 1.0.0.0
SeverityID       : 5
ThreatID         : 224688
ThreatName       : PUA:Win32/EICAR_Test_File
TypeID           : 0
PSComputerName   :

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 10, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @mcshooter. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 10, 2021
@mcshooter
Copy link
Contributor Author

/cc @jeremyje

@k8s-ci-robot k8s-ci-robot requested a review from jeremyje May 10, 2021 23:24
{
"plugin": "custom",
"pluginConfig": {
"invoke_interval": "30s",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be appropriate to make this run every 10 minutes.

$OK=0
$NONOK=1

$windowsDefenderThreats = Get-MpThreatDetection
Copy link
Contributor

Choose a reason for hiding this comment

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

https://docs.microsoft.com/en-us/powershell/module/defender/get-mpthreatdetection?view=windowsserver2019-ps

The Get-MpThreatDetection cmdlet gets active and past malware threats that Windows Defender detected on the computer.

We only want active threats unless past here means that if a threat is handled by Defender then it's no longer a problem right?

The description in the docs aren't precise enough to make a judgement here. Perhaps check with sig-windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use Get-MpThreat because there are fields we can use that can help us determine whether a thread is active or has been executed on the windows node.

$windowsDefenderThreats = Get-MpThreatDetection

if ($windowsDefenderThreats.length -ne 0) {
exit $NONOK
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's clear to just return 0 vs 1 here. Aliasing the variables adds unnecessary indirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it common knowledge that 1 indicates a bad exit, and 0 is an OK exit? I haven't had to deal with exits before. I was thinking that the variables makes it easy to read and understand what it indicates, but if it's common knowledge then I am good to remove them and return just 0 or 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

exit code 0 indicates success thats common

config/plugin/windows_defender_problem.ps1 Show resolved Hide resolved
"rules": [
{
"type": "temporary",
"reason": "WindowsDefenderProblemsDetected",
Copy link
Contributor

Choose a reason for hiding this comment

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

WindowsDefenderThreatsDetected

@jeremyje
Copy link
Contributor

/ok-to-test
/sig windows
/sig node

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/windows Categorizes an issue or PR as relevant to SIG Windows. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 11, 2021
@jeremyje
Copy link
Contributor

#461

@mcshooter mcshooter force-pushed the addWindowsDefenderProblemDetection branch 2 times, most recently from cb785e1 to d26eed2 Compare May 11, 2021 06:08
@jeremyje
Copy link
Contributor

/ok-to-test

@mcshooter
Copy link
Contributor Author

/retest

1 similar comment
@mcshooter
Copy link
Contributor Author

/retest

@mcshooter mcshooter force-pushed the addWindowsDefenderProblemDetection branch from d26eed2 to 767f0c1 Compare May 12, 2021 19:49
config/plugin/windows_defender_problem.ps1 Outdated Show resolved Hide resolved
config/plugin/windows_defender_problem.ps1 Outdated Show resolved Hide resolved
@mcshooter mcshooter force-pushed the addWindowsDefenderProblemDetection branch from 767f0c1 to 01fa5b3 Compare May 12, 2021 20:28
@mcshooter
Copy link
Contributor Author

/retest

@jeremyje
Copy link
Contributor

/lgtm
/approve
/sig windows
/sig node

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2021
@Random-Liu
Copy link
Member

/lgtm
/approve

This looks pretty useful! Thanks for adding this!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeremyje, mcshooter, Random-Liu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2021
@k8s-ci-robot k8s-ci-robot merged commit f0ab653 into kubernetes:master May 13, 2021
@mcshooter mcshooter deleted the addWindowsDefenderProblemDetection branch May 13, 2021 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants