Feature to add Windows firewall logging #144
Conversation
… different recipe
…ut should be empty instead
| @@ -18,7 +18,12 @@ class Resource::FirewallRule < Chef::Resource::LWRPBase | |||
| !!(p.to_s =~ /(udp|tcp|icmp|icmpv6|ipv6-icmp|none)/ || (p.to_s =~ /^\d+$/ && p.between?(0, 142))) | |||
| end }) | |||
| attribute(:direction, kind_of: Symbol, equal_to: [:in, :out, :pre, :post], default: :in) | |||
| attribute(:logging, kind_of: Symbol, equal_to: [:connections, :packets]) | |||
|
|
|||
| if Chef::Platform.windows? | |||
There was a problem hiding this comment.
Hi there -- if a downstream cookbook decides to use one of these attributes, to support windows and linux both, this will break it, right? I'd rather allow extra attribute values than prevent someone from supporting windows and linux when providing additional firewall rules in their community cookbook.
There was a problem hiding this comment.
If I did not understand what you meant, you cannot that easily use the same rule for Linux and Windows. But this can be doable.. I will work on it.
There was a problem hiding this comment.
If we're not running on Windows, we should still allow someone to declare a Windows firewall rule. For example, if someone makes amysql cookbook, I would want them to be able to ship Windows and Linux firewall rules, with only_of { <platform_check> }. This wouldn't compile if we have a conditional like what you've got here.
| @@ -57,6 +57,14 @@ def whyrun_supported? | |||
| end | |||
| end | |||
|
|
|||
| # If logging rules are not provided, then create a rule to disable them (windows default) | |||
| unless new_resource.rules['windows'].key?('set currentprofile logging droppedconnections enable') | |||
There was a problem hiding this comment.
Shouldn't we do nothing if there aren't existing logging rules provided? I don't want to maintain Windows defaults in this cookbook long term.
There was a problem hiding this comment.
Logging rules are not like standard firewall rules. So it is there or not. If it is not there, this means we should revert it to default windows behaviour. Otherwise a user will remove logging attributes expecting the logging will be changed to default but in reality it wont if we don't do this.
| parameters['remoteip'] = new_resource.destination ? fixup_cidr(new_resource.destination) : 'any' | ||
| parameters['remoteport'] = new_resource.dest_port ? port_to_s(new_resource.dest_port) : 'any' | ||
| if type == :log | ||
| "set currentprofile logging #{new_resource.logging} enable" |
There was a problem hiding this comment.
This seems like an unexpected side effect of any rule that has logging. I don't think users will expect this to be run, if they haven't explicitly enabled logging.
There was a problem hiding this comment.
This is exactly what should be done when someone coded logging attribute in a provider call. This is the only way to change windows firewall logging.
And logging is not per rule in Windows Firewall. It is system wide. You either define logging or not, globally.
There was a problem hiding this comment.
And logging is not per rule in Windows Firewall. It is system wide. You either define logging or not, globally.
Shouldn't we have the user enable it on the firewall resource, then?
There was a problem hiding this comment.
Hi there -- I've made some comments here. I think
- there are some extra commands being run, that are going to surprise users
- some platform-specific things that break compilation, vs. degrading gracefully on other platforms
I'd like to merge this, if you don't mind working on those two issues. Thank you!
martinb3
added
Type: Enhancement
Priority: Medium
tas50
added
Triage: Needs Information
and removed
Status: Pending Contributor Response
labels
Description
Feature to enable logging for Windows Firewall.
Issues Resolved
Check List