-
Notifications
You must be signed in to change notification settings - Fork 90
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
Firewall: Fix issue #428 - DSC_Firewall #518
base: main
Are you sure you want to change the base?
Conversation
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## main #518 +/- ##
===================================
Coverage 97% 97%
===================================
Files 28 28
Lines 2078 2078
===================================
Hits 2028 2028
Misses 50 50
|
@brajjan when you are ready, take this PR out of draft so it can be reviewed. |
Not sure if the PolicyStoreSourceType property needs to be there. My initial thought was to include that one to compare with the value of the PolicyStore parameter as the PolicyStore property does not get returned when running Get-TargetResource (or Get-NetFirewallRule). The updated code works as intended. If you do not set the PolicyStore parameter it will default to 'PersistentStore' (= local rule, default before adding the PolicyStore property - so no breaking change) If you set the PolicyStore to 'localhost' it will create a local GPO rule on the active node. Verify with looking at rsop or gpedit or looking at the rule in wf.msc. Get, Set and Test all works with PersistentStore or localhost Info about the different PolicyStores can be found here As the property does not get returned from Get-TargetResource I am not sure how to do the proper unit tests. Also my pester skills are not great but if you could have a look it would be great. Also worth mentioning - The node that gets the config for a GPO rule needs a GPO refreh (gpupdate /force) for the rule to be applied directly.
|
In Get we must evaluate the correct value for |
Yes, the PolicyStoreSourceType will return The PolicyStore parameter can be set to target central GPOs as well. I have not included that but it could be done I guess. One problem that could occur is that the But targeting the PersistentStore and Local GPO will always work in Get/Set/Test on Windows - and targeting the GPO store (local or central does not matter) is the main objective here as many AD-joined machines does not allow local rules to be applied.
|
Cross posted from Slack: My thinking is that there are two parameters here that can be treated separately: Policy StoreFrom my reading of the docs, this parameter could take on a lot of different values (as I think @johlju points out): https://learn.microsoft.com/en-us/powershell/module/netsecurity/get-netfirewallrule?view=windowsserver2019-ps#-policystore However, this potentially should be a key value (which would be a breaking change because can't have a default value for a key). Technically the same rule could need to be created in different policy stores. It's an edge case, but if we're adding the parameter then people may try to do this - which would result in invalid config (unless you created the rule with a different name for each policy store). Either way, we'd want to add some specific tests to cover this because I don't think the data driven tests will cut it. An integration test to verify usage of different policy stores (if possible - not sure though). PolicyStoreSourceTypeThis just seems like a simple read only property - so not a big deal to add. Just my initial thoughts on this though - haven't dug in too deep on this. |
If so then I agree it have to be a key. Just athought. To prevent making a breaking change, could we do new (preferably class) resources instead, having one resource for each policy store type? Then we could extend to more policy store types by adding more resources. |
That's an interesting approach @johlju. Didn't consider this case as a side-benefit of implementing class-based resources. I'd definitely like to see this happen. However, given the scope of a change to class-based (for just the DSC_Firewall resource), I'd suggest that it's split into two PRs:
From memory, the DSC_Firewall is reasonably well abstracted already, so much of the code is not in the Get, Test, Set functions, which should make it easier to convert. |
I agree with that approach, that it is best to convert the existing resources as-is and re-use the existing tests also as-is (though they will need to change somewhat because how the methods are called) to verify that no functionality was broken. |
@brajjan - does what we're talking about make sense? The proposal is to convert the Firewall resource to class-based and then enable the creation of a new class-based resources for each Firewall Policy source (presumably only one for now). These class-based resources would share 99% code. This would eliminate the breaking change and would improve the overall resource structure. It will mean that the conversion of Firewall to class-based would need to happen first. Would happily take a PR for this as I'm not likely to get to this any time soon due to too many other commitments. |
Yeah sure, that would be great. I'll try to get some time of to look at it closer and convert it to a class based resource. Might take some time though |
Pull Request (PR) description
Added
PolicyStore
parameter and read only propertyPolicyStoreSourceType
- fixes Issue #428. As Get-NetFirewallRule does not return the PolicyStore property I added the PolicyStoreSourceType as a read only property in the MOF file, added it to DSC_Firewall.data.psd1 as well as a parameter forTest-TargetResource
andSet-TargetResource
for comparing if it is in desired state.Not updated Pester tests as my knowledge of Pester tests are close to 0
This Pull Request (PR) fixes the following issues
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
and comment-based help.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)