-
Notifications
You must be signed in to change notification settings - Fork 88
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
xFirewall: Fails on port keywords #34
Comments
I'll take a look and see if I can figure out what the issue is. |
Hi @nebffa , I did a bit of digging around and this isn't problem with the resource. The issue is that the LocalPort parameter can only take on the parameter of PlayToDiscovery if the protocol is UDP. PlayToDiscovery is not supported by the TCP protocol in WFAS. You can confirm this by executing: New-NetFirewallRule `
-Name "IIS Remote Administration" `
-DisplayName "IIS Remote Administration" `
-Action "Allow" `
-Profile "Domain" `
-Direction "InBound" `
-RemotePort "Any" `
-LocalPort "PlayToDiscovery" `
-Protocol "TCP" It will fail with the Error:
But: New-NetFirewallRule `
-Name "IIS Remote Administration" `
-DisplayName "IIS Remote Administration" `
-Action "Allow" `
-Profile "Domain" `
-Direction "InBound" `
-RemotePort "Any" `
-LocalPort "PlayToDiscovery" `
-Protocol "UDP" will succeed. You can also confirm this in the Windows Firewall with Advanced Security management interface. So either don't use the PlayToDiscovery for LocalPort or change the protocol to UDP. HTH. |
@PlagueHO Is there anything we can do to clear up the error logs to make that clearer for the next user? |
Hey @tysonjhayes , thanks that was really quick. The Microsoft documentation on that page I linked (not in this DSC resource) is clearly erroneous in that it says the keyword would work with TCP. I noticed in other error messages that xFirewall returns, it displays the possible parameters that you could supply instead. I think that would also be useful in this case. Interestingly, what I was trying to do from the start was set a firewall rule with LocalPort being |
Sorry, you're right @tysonjhayes . Thanks @PlagueHO as well for your help. I played around with it a bit more. All I needed to do was use The Microsoft docs say Perhaps an option for clearer error logs would be something like:
|
Hi @tysonjhayes and @nebffa , When I was investigating this I found the New-NetFirewallRule cmdlet docs are definitely not too clear on this - which is what had be scratching my head as well. I've added a community addition to the documentation on this page. @tysonjhayes, it would definitely be possible to sanitize the parameters in the Test-TargetResource (perhaps call out to a Test-Parameters function like we do in the other resources) and throw a slightly more descriptive error message should this be encountered. The only problem with this is that if an existing rule is being modified then might still slip through. E.g. if a rule exists: And a resource is created that changes the Protocol to TCP (without modifying the LocalPort) then this test wouldn't flag the issue. It could be made to do this (compare both existing values and new values) but it would increase complexity. A better way might be to just try and catch that specific exception when creating/updating the firewall rule. But perhaps the best solution is just to document this in the Readme.md in a list of known bad combinations (because I'm sure there are a few of them that aren't documented). :) |
We could start with a list of known bad combinations, and expand from there, I don't really want to write a bunch of code to handle everything as one off combinations just to bubble up the excepton properly. I was more thinking could we get it to say "One of the port keywords is invalid." which was the actual exception. I'll look at the code later. Side note, I was noticing that this came out of the verbose logs: VERBOSE: [PERFTEST01]: [[xFirewall]IIS Remote Administration] Test-RuleProperties: LocalPort
property value 'System.String[]' does not match desired state 'System.String[]'. Which suggests we may not be comparing that array correctly or at least not displaying it in the verbose stream. I'd expect it to say 'LocalPort property value 123456 does not match desired state 80' or something to that effect. |
Regarding the "One of the port keywords is invalid" exception, when I actually tested this problem and ran the config through my test systems, that was actually the error that was reported to me in the Verbose log. This did strike me as odd given that the error the logs @nebffa posted shows a different error. I might do some additional investigation on this as I don't like mysteries... And good catch with the bad messages - I ran into and fixed the same thing in the last PR I raised (pushing an array into a format string must be first joined). I can pop a fix through for this as well and see if I can identify any others if you like? |
After further experimentation, I found that the message "One of the port keywords is invalid" message is reported in the Verbose log if the firewall rule does not yet exist:
If a version of the firewall rule already exists (regardless of how that rule is configured), then that is when the error that @nebffa's log is showing. So in summary, the odd error message only occurs when updating an existing rule with a bad combination. I'll see if I can figure out a way to resolve it. |
I've found and fixed the issue where the error is being misreported. It is actually a slightly more significant bug in that any time the DisplayName is passed when the rule already exists an error would be thrown. I'm going to add some pester tests to check for this issue in future, but I'm away for the rest of this week so I'll do it first thing next week. Thanks all. |
No rush, I think you've documented it fairly well up to this point. I'm sure anyone who runs into this until we've gotten a fix in will be able to find this issue and sort it out. 😁 Thanks for looking into this. |
This was fixed in #37 which is now merged. Please reopen this if you are still blocked on the latest code. |
The following DSC script:
fails with the logs:
At this page on
New-NetFirewallRule
, the documentation says thatPlayToDiscovery
is a valid keyword when the procotol isTCP
, so this operation should not fail.The text was updated successfully, but these errors were encountered: