Firewall: Fix issue #428 - DSC_Firewall #518

brajjan · 2022-09-03T20:48:39Z

Pull Request (PR) description

Added PolicyStore parameter and read only property PolicyStoreSourceType - 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 for Test-TargetResource and Set-TargetResource for comparing if it is in desired state.

Parameter PolicyStore set to localhost returns PolicyStoreSourceType: GPO
Parameter PolicyStore set to PersistentStore returns PolicyStoreSourceType: Local

Not updated Pester tests as my knowledge of Pester tests are close to 0

This Pull Request (PR) fixes the following issues

Fixes Firewall and FirewallProfile: Add PolicyStore to allow targeting local group policy #428

Task list

Added an entry to the change log under the Unreleased section of the
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Community Testing Guidelines.
Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
New/changed code adheres to DSC Community Style Guidelines.

This change is

johlju · 2022-09-27T18:15:14Z

/AzurePipelines run

azure-pipelines · 2022-09-27T18:15:25Z

Azure Pipelines successfully started running 1 pipeline(s).

codecov · 2022-09-28T12:23:21Z

Codecov Report

Merging #518 (41abbf9) into main (9d6e70d) will not change coverage.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           main   #518   +/-   ##
===================================
  Coverage    97%    97%           
===================================
  Files        28     28           
  Lines      2078   2078           
===================================
  Hits       2028   2028           
  Misses       50     50

Impacted Files	Coverage Δ
source/DSCResources/DSC_Firewall/DSC_Firewall.psm1	`96% <100%> (ø)`

johlju · 2022-10-01T19:42:24Z

@brajjan when you are ready, take this PR out of draft so it can be reviewed.

brajjan · 2022-10-02T11:23:14Z

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
https://learn.microsoft.com/en-us/powershell/module/netsecurity/new-netfirewallrule?view=windowsserver2022-ps#-policystore

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.

$invokeParams = @{
  Name   = 'Firewall'
  Module = 'NetworkingDsc'
  Method = 'GET'
  Property = @{
    Name          = '_GPOTEST'
    PolicyStore   = 'localhost'
    LocalPort     = @('1234')
    RemoteAddress = @('1.1.1.1')
    Protocol      = 'TCP'
  }
}

Invoke-DscResource @invokeParams                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  ConfigurationName     : 
ModuleName            : NetworkingDsc
ModuleVersion         : 0.0.1
PsDscRunAsCredential  :
ResourceId            :
SourceInfo            :
Action                : Allow
Authentication        : NotRequired
Description           :
Direction             : Inbound
DisplayGroup          :
DisplayName           : _GPOTEST
DynamicTransport      : Any
EdgeTraversalPolicy   : Block
Enabled               : True
Encryption            : NotRequired
Ensure                : Present
Group                 :
IcmpType              : {Any}
InterfaceAlias        : {Any}
InterfaceType         : Any
LocalAddress          : {Any}
LocalOnlyMapping      : False
LocalPort             : {1234}
LocalUser             : Any
LooseSourceMapping    : False
Name                  : _GPOTEST
OverrideBlockRules    : False
Owner                 :
Package               :
Platform              : {}
PolicyStore           :
PolicyStoreSourceType : GroupPolicy
Profile               : {Any}
Program               : Any
Protocol              : TCP
RemoteAddress         : {1.1.1.1}
RemoteMachine         : Any
RemotePort            : {Any}
RemoteUser            : Any
Service               : Any
PSComputerName        : localhost

johlju · 2022-10-02T12:39:35Z

In Get we must evaluate the correct value for $PolicyStore. If the current state is that it is a GPO rule then that should return the value localhost. But should we hardcode the value to 'localhost'? According to the link it can be set to a "path" as well? Maybe it should be a separate property for the "path" that is used when PolicyStore is set to ActiveStore? I see that Get-NetFirewallRuletakes the parameterPolicyStoreSourceType`. Can that be used to determain what store it is and return the correct value.

brajjan · 2022-10-03T06:30:57Z

Yes, the PolicyStoreSourceType will return GroupPolicy if set to localhost and Local if set to PersistentStore. So that could be used to check the correct PolicyStore.

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 New-NetFirewallRule command does not include sending credentials. Will probably be a problem as the credentials used for applying the code probably will not be the same as an account with GPO-write permissions in AD. Could target a few more stores. See test below - they will all list what PolicyStoreSourceType will return

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.

foreach ($obj in $stores) { New-NetFirewallRule -Name "_Test-$obj" -DisplayName "_Test-$obj" -PolicyStore $obj -Enabled False -Direction Inbound -LocalPort '1234' -RemoteAddress '1.1.1.1' -Protocol TCP -Verbose }
VERBOSE: New-NetFirewallRule DisplayName: _Test-PersistentStore

Name                          : _Test-PersistentStore
DisplayName                   : _Test-PersistentStore
Description                   :
DisplayGroup                  :
Group                         :
Enabled                       : False
Profile                       : Any
Platform                      : {}
Direction                     : Inbound
Action                        : Allow
EdgeTraversalPolicy           : Block
LooseSourceMapping            : False
LocalOnlyMapping              : False
Owner                         :
PrimaryStatus                 : OK
Status                        : The rule was parsed successfully from the store. (65536)
EnforcementStatus             : NotApplicable
PolicyStoreSource             : PersistentStore
PolicyStoreSourceType         : Local
RemoteDynamicKeywordAddresses :

VERBOSE: New-NetFirewallRule DisplayName: _Test-ActiveStore
Name                          : _Test-ActiveStore
DisplayName                   : _Test-ActiveStore
Description                   :
DisplayGroup                  :
Group                         :
Enabled                       : False
Profile                       : Any
Platform                      : {}
Direction                     : Inbound
Action                        : Allow
EdgeTraversalPolicy           : Block
LooseSourceMapping            : False
LocalOnlyMapping              : False
Owner                         :
PrimaryStatus                 : OK
Status                        : The rule was parsed successfully from the store. (65536)
EnforcementStatus             : NotApplicable
PolicyStoreSource             : ActiveStore
PolicyStoreSourceType         : Dynamic
RemoteDynamicKeywordAddresses :

VERBOSE: New-NetFirewallRule DisplayName: _Test-localhost
Name                          : _Test-localhost
DisplayName                   : _Test-localhost
Description                   :
DisplayGroup                  :
Group                         :
Enabled                       : False
Profile                       : Any
Platform                      : {}
Direction                     : Inbound
Action                        : Allow
EdgeTraversalPolicy           : Block
LooseSourceMapping            : False
LocalOnlyMapping              : False
Owner                         :
PrimaryStatus                 : OK
Status                        : The rule was parsed successfully from the store. (65536)
EnforcementStatus             : NotApplicable
PolicyStoreSource             :
PolicyStoreSourceType         : GroupPolicy
RemoteDynamicKeywordAddresses :

VERBOSE: New-NetFirewallRule DisplayName: _Test-RSOP
New-NetFirewallRule: Access is denied.
VERBOSE: New-NetFirewallRule DisplayName: _Test-SystemDefaults
New-NetFirewallRule: Access is denied.
VERBOSE: New-NetFirewallRule DisplayName: _Test-StaticServiceStore
New-NetFirewallRule: Access is denied.
VERBOSE: New-NetFirewallRule DisplayName: _Test-ConfigurableServiceStore
Name                          : _Test-ConfigurableServiceStore
DisplayName                   : _Test-ConfigurableServiceStore
Description                   :
DisplayGroup                  :
Group                         :
Enabled                       : False
Profile                       : Any
Platform                      : {}
Direction                     : Inbound
Action                        : Allow
EdgeTraversalPolicy           : Block
LooseSourceMapping            : False
LocalOnlyMapping              : False
Owner                         :
PrimaryStatus                 : OK
Status                        : The rule was parsed successfully from the store. (65536)
EnforcementStatus             : NotApplicable
PolicyStoreSource             : No Policy Store (Invalid)
PolicyStoreSourceType         : None
RemoteDynamicKeywordAddresses :

PlagueHO · 2022-10-03T07:15:29Z

Cross posted from Slack:
Hi @brajjan, Thank you for submitting the PR's! I apologize it's taking me so long to get to these (unfortunately completely slammed in my day job).

My thinking is that there are two parameters here that can be treated separately: PolicyStore and PolicyStoreSourceType.

Policy Store

From 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).

PolicyStoreSourceType

This 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.

johlju · 2022-10-04T11:09:20Z

Technically the same rule could need to be created in different policy stores.

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.

PlagueHO · 2022-10-05T05:12:04Z

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:

Convert DSC_Firewall to class-based.
Add the new Policy version.

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.

johlju · 2022-10-05T11:34:35Z

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.

PlagueHO · 2022-10-07T21:03:41Z

@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.

brajjan · 2022-10-08T11:05:15Z

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

brajjan added 7 commits September 2, 2022 22:46

Allow targeting local group policy

6fe7a3d

fix syntax

18fbd2f

Removal of rule should target PolicyStore

9218fd2

change PolicyStoreSourceType to read only

bb81c77

update changelog

56249c9

update description of added feature

89eb83a

Update comment based help

81c4e5a

brajjan added 3 commits September 28, 2022 13:53

add missing PolicyStoreSourceType parameter

24d2548

correct the validateset for PolicyStoreSourceType

fe4855e

update parameter documentation

ff91f41

brajjan added 3 commits September 28, 2022 14:27

fix spelling

6ea1c8f

remove read only property from test and set

c347786

fix additional spelling

41abbf9

brajjan marked this pull request as ready for review October 2, 2022 10:07

PlagueHO added the needs review The pull request needs a code review. label Oct 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firewall: Fix issue #428 - DSC_Firewall #518

Firewall: Fix issue #428 - DSC_Firewall #518

brajjan commented Sep 3, 2022 •

edited by johlju

johlju commented Sep 27, 2022

azure-pipelines bot commented Sep 27, 2022

codecov bot commented Sep 28, 2022 •

edited

johlju commented Oct 1, 2022

brajjan commented Oct 2, 2022 •

edited

johlju commented Oct 2, 2022

brajjan commented Oct 3, 2022

PlagueHO commented Oct 3, 2022 •

edited

johlju commented Oct 4, 2022

PlagueHO commented Oct 5, 2022

johlju commented Oct 5, 2022

PlagueHO commented Oct 7, 2022

brajjan commented Oct 8, 2022

Firewall: Fix issue #428 - DSC_Firewall #518

Are you sure you want to change the base?

Firewall: Fix issue #428 - DSC_Firewall #518

Conversation

brajjan commented Sep 3, 2022 • edited by johlju

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

johlju commented Sep 27, 2022

azure-pipelines bot commented Sep 27, 2022

codecov bot commented Sep 28, 2022 • edited

Codecov Report

johlju commented Oct 1, 2022

brajjan commented Oct 2, 2022 • edited

johlju commented Oct 2, 2022

brajjan commented Oct 3, 2022

PlagueHO commented Oct 3, 2022 • edited

Policy Store

PolicyStoreSourceType

johlju commented Oct 4, 2022

PlagueHO commented Oct 5, 2022

johlju commented Oct 5, 2022

PlagueHO commented Oct 7, 2022

brajjan commented Oct 8, 2022

brajjan commented Sep 3, 2022 •

edited by johlju

codecov bot commented Sep 28, 2022 •

edited

brajjan commented Oct 2, 2022 •

edited

PlagueHO commented Oct 3, 2022 •

edited