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

Module wcc added some defender checks #306

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jubeaz
Copy link

@jubeaz jubeaz commented May 15, 2024

Hello,

I've added some checks regarding Defender AV.

I've corrected a bug inside check_registry regarding the value of the op var.

I did not want touch too much to the check_registry, but I think it can be merged with the function that I've created check_single_registry_with_policy .

in order to allow the base tuple of check_registry to include the policies registry or None.

regards,

@mpgn
Copy link
Collaborator

mpgn commented May 15, 2024

Any screenshot possible @jubeaz ? :)

@jubeaz
Copy link
Author

jubeaz commented May 16, 2024

sure this is what you get visually (the screenshot is restricted to my checks)

2024-05-16_03-49

One thing that might be confusing is that when I do perform checks on defender parameters (exclusions, IOAV...) I do not check the state of defender itself. If you look at FENRIS this is a server without defender installed but as there is a GPO that apply some parameter to defender the policy is taken into account when computing parameters.

For registry I do check value set on a computer only if the value is not set by policies.

To mitigate that I have decided to write detailed reasons inside the DB

2024-05-16_04-07

If you prefer I can correct the reason could be KO with reason N/A if defender is not running but it will be slower.

@Marshall-Hallenbeck
Copy link
Collaborator

@jubeaz can you run Ruff against this?

@Marshall-Hallenbeck Marshall-Hallenbeck added the enhancement New feature or request label May 16, 2024
@NeffIsBack
Copy link
Contributor

@jubeaz can you run Ruff against this?

Also, why the heck is the pipeline not running sometimes

@Marshall-Hallenbeck
Copy link
Collaborator

@jubeaz can you run Ruff against this?

Also, why the heck is the pipeline not running sometimes

It runs when an owner commits or we approve, I believe, otherwise we'd overuse our pipeline quota pretty fast.

@NeffIsBack
Copy link
Contributor

@jubeaz can you run Ruff against this?

Also, why the heck is the pipeline not running sometimes

It runs when an owner commits or we approve, I believe, otherwise we'd overuse our pipeline quota pretty fast.

Ah you are kinda right, it blocks runs for first time contributors. If code from that contributor has been merged before, it will trigger the pipeline.
image

@jubeaz
Copy link
Author

jubeaz commented May 17, 2024

ok I've applied the linter.

Sorry I'm kind of new in development process and I did not carefully enough read the CONTRIBUTING.md

hope I'm not giving you too much work

@Marshall-Hallenbeck
Copy link
Collaborator

@jubeaz don't worry about it :D we hadn't updated the PR template until after you filed this.

@Marshall-Hallenbeck
Copy link
Collaborator

@jubeaz this looks great except for the final two checks aren't logging the policy and specific reason to the log inside ~/.nxc/logs/$date/wcc_$date.log:

image

@Marshall-Hallenbeck Marshall-Hallenbeck added this to the v1.3.0 milestone May 17, 2024
@jubeaz
Copy link
Author

jubeaz commented May 19, 2024

Hello,

This is not an error this is because there are no exclusion set either directly or by policies and this is the way I log it (same way in db). In my lab you ca see the difference.

2024-05-20_00-57

would yo have preferred another way to log ?

@Dfte
Copy link
Contributor

Dfte commented Jun 6, 2024

@fpreynaud take a look at this man :P

@Marshall-Hallenbeck
Copy link
Collaborator

@jubeaz sorry for the late response. That makes sense to me. If you can fix the conflicts we can get this merged.

@jubeaz
Copy link
Author

jubeaz commented Jun 19, 2024

Hello,

done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants