xDSCWebservice Firewall Exception

[pkoelemij]

Description

The xDSCWebservice Firewall exception automatically creates up a DSCPullServer_IIS_Port rule that opens up port 8080 to the complete internet. I'd like to be able to control this behaviour because now I have to jump through a couple of hoops to delete the firewall rule with a Script resource and create a new rull that only opens it to a certain subnet.

It is hardcoded on this line:
https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/dev/DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1#L382

Proposed properties

Add a parameter called FirewallException = $true or $false that controls the creation of the firewall rules.

[tmeckel]

@mhendric we can support the proposed flag to enable the firewall exception from the original author pretty easy because PSWSIISEndpoint\New-PSWSEndpoint already has a parameter for this -EnableFirewallException. Currently the parameter value is fixed to $true as stated correctly by the original author.

https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/e615bca32a958d705e44cbfe6abc0c0ebbec1b12/DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1#L383

But in constrast to the original author I personally would prefer to remove the support for the Firewall from the xDSCWebService resource completely because we have already a supported DSC resource for handling the firewall.

https://github.com/PowerShell/NetworkingDsc/wiki/Firewall

and thus configuring the firewalll for a DSC Pull Server should be done with this resource. As a benefit we can get rid of code here inside PSWSIISEndpoint.psm1 which would make the code cleaner a much more focused on the actual job: configuring a DSC Pull Server.

Luckily this would be a non breaking change because the handling of the firewall was never a part of the schema.mof

[mhendric]

@tmeckel , that's an interesting suggestion. I tend to agree that since there's already a dedicated Firewall resource, it may be best to just remove this from the xPSDesiredStateConfiguration code entirely. However, even though it's not technically a breaking change (i.e. a change to the schema that makes it so you can't recompile your existing scripts as is), this does seem like it will break the process for anyone that expects the resource to create this rule. I'm not opposed to this, but I think we may want to treat it as a breaking change (increment the major build number and clearly document why we are doing so).

Tagging @PlagueHO and @johlju for their thoughts as well.

[tmeckel]

Okay ... jumped a bit too short ... didn't think about people who might rely on the existing behavior that a firewall rule is created. If my proposal finds support, this should actually be treated as a breaking change.

[tmeckel]

@mhendric after that we finally merged PR #507 I would start working on this here. I would follow @pkoelemij proposal to add an configuration option [write] boolean ConfigureFirewall that has a default value of true which ensures that the firewall exceptions are configured as if thery're configured today. So no breaking change for currently deployed Pull Server configurations.

With a value of false the xDSCWebservice resource won't create a firewall exception for the pull server and you can use the Firewall resource inside your DSC configuration to configure the firewall as it is required for the current setup.

Lemme know what you think.

[mhendric]

Hey @tmeckel . Sounds good to me. We may also want to add a Write-Warning statement that is triggered when ConfigureFirewall is set to True, which states that the Firewall Configuration functionality has been deprecated and will be removed in the future; consider using the xFirewall (
?) resource. Additionally, we'd probably want to add a new example that shows how you could set this parameter to False and configure the firewall with a different DSC resource instead. What do you think?

[johlju]

Agree with both @tmeckel and @mhendric last comment.

[tmeckel]

Okay, then let's go that way. Quick summary for me:

1. Introduce a new Configuration option [write] boolean ConfigureFirewall with a default value of true
2. Adding a warning statement that issues a messages that configuring the firewall with the xDSCWebservice is deprecated.
3. Adding a new sample that shows how to configure a firewall exception using the Firewall resource from the NetworkingDSC module Firewall
4. Adding a statemnet in the README:md for the xDSCWebservice resource that configuring the firewall is deprecated.

Did I miss something?

[tmeckel]

Quick question about work organization: Wouldn't it make sense if we would work with assignments? Or doesn't that make sense for community projects?

[johlju]

@tmeckel If you mean "Assignees" on PR and issues, it's not possible to assign contributors to issues or PR's, unless the are the author of the issue or PR. At least yet, I sent in a request to GitHub for anyone to be able to be assigned to an issue and PR, but that could take time, if it would ever happen. 😄
What we can do for now is to label it as "in progress" so that other contributors know someone is working on it, and preferably the contributor adds a comment saying so (so two contributors don't work on the same thing at the same time). 🙂

[johlju]

Thinking about it, adding a warning statement (list item 2) might be unnecessary. The point would be to remove the functionality, including the property ConfigureFirewall in the future. But that would be a breaking change for anyone using ConfigureFirewall set to $false.
So I think we could just skip list item 2. What do you think @mhendric?

[tmeckel]

What we can do for now is to label it as "in progress" so that other contributors know someone is working on it, and preferably the contributor adds a comment saying so (so two contributors don't work on the same thing at the same time). 🙂

@johlju Thanks for the explanation! @mhendric can you assign the In progress label to this issue?

[tmeckel]

Just to inform you guys: I discovered that the implementation of the Firewall exception inside xDSCWebservice is a mixture of code using the NetSecurity PowerShell Module and netsh and on top of it it's not modularized into a separate code file or to distinct helper functions.

I'll have to clean this up at first to have a robust implementation of the fix for this issue here. I would prefer using netsh for this because the NetSecurity module is only available with systems later than Windows Server 2012 and I don't know the status of this module for PowerShell 4.

[tmeckel]

Thinking about it, adding a warning statement (list item 2) might be unnecessary. The point would be to remove the functionality, including the property ConfigureFirewall in the future. But that would be a breaking change for anyone using ConfigureFirewall set to $false.
So I think we could just skip list item 2. What do you think @mhendric?

@johlju I would prefer to leave the configuration option as is but later change the default value to $false at the next stage of obsolesce. In the end we remove the implementation of the firewall code and issue an error when someone is using $true for ConfigureFirewall. This would only be a breaking change for people explicitly used this feature. All other people won't be effected, well okay except those who relied on the default value $true on ConfigureFirewall. But those people should be aware that they must change their implementation by the warning anyway.

[johlju]

I would prefer to leave the configuration option as is but later change the default value to $false at the next stage of obsolesce. In the end we remove the implementation of the firewall code and issue an error when someone is using $true for ConfigureFirewall

Sounds reasonably. Let us go with that. 🙂

[mhendric]

Agree with @tmeckel . If we set the default value to True right now, we're not actually making a breaking change at the moment; the Warning just sets the stage for a future breaking change. However if we set it to False now, that would definitely be a breaking change.

[tmeckel]

@mhendric thanks for adding the 'in progress' label. I implemented the first version but now many unit tests are broken because, as I mentioned, I modularized the Firewall code and added a strict validation on the -Port parameter. The xDSCWebService resource is a real beast :-D

[mhendric]

It sure is a beast :) . Kind of a related topic, does anyone know of a free way to generate a UML diagram or something based on existing PowerShell code? It would help my understanding of the resource to be able to visualize all the call flows and dependencies. Might help us see the big picture and optimize too.

[johlju]

Only I know of is https://github.com/Stephanevg/PSClassUtils, But not sure it works for anything other than classes? Not used it at all though, so not sure.

[tmeckel]

@mhendric I created three high level pseudo code markdown documents of the flow structure of the three *-TargetResource functions of xDSCWebService to understand what's really going on there and to verify or fix the existing unit tests. I could add them to a branch in my fork if you're interested.

[mhendric]

@tmeckel I wouldn't mind checking that out. Thanks.

[tmeckel]

@mhendric there you go but beware the documents are far from consistent in the notation LOL but I think you might grasp the idea quickly. Unfortunately there's currently (as far I know!) no standard notation for this.

[tmeckel]

@pkoelemij the PR for fixing your issue #614 is in review. Maybe the PR will make it into the next release.

[pkoelemij]

@tmeckel good news :)