Merge pull request #7879 from chef/firewall

Allow passing multiple ports in windows_firewall

2 files changed, 69 insertions, 24 deletions

diff --git a/lib/chef/resource/windows_firewall_rule.rb b/lib/chef/resource/windows_firewall_rule.rb
index a7e4609605..0e313eb14a 100644
--- a/lib/chef/resource/windows_firewall_rule.rb
+++ b/lib/chef/resource/windows_firewall_rule.rb
@@ -40,18 +40,21 @@ class Chef
       property :local_address, String,
                description: "The local address the firewall rule applies to."
 
-      property :local_port, String,
+      property :local_port, [String, Integer, Array],
+               # split various formats of comma separated lists and provide a sorted array of strings to match PS output
+               coerce: proc { |d| d.is_a?(String) ? d.split(/\s*,\s*/).sort : Array(d).sort.map { |x| x.to_s } },
                description: "The local port the firewall rule applies to."
 
       property :remote_address, String,
                description: "The remote address the firewall rule applies to."
 
-      property :remote_port, String,
+      property :remote_port, [String, Integer, Array],
+               # split various formats of comma separated lists and provide a sorted array of strings to match PS output
+               coerce: proc { |d| d.is_a?(String) ? d.split(/\s*,\s*/).sort : Array(d).sort.map { |x| x.to_s } },
                description: "The remote port the firewall rule applies to."
 
       property :direction, [Symbol, String],
-               default: :inbound,
-               equal_to: [:inbound, :outbound],
+               default: :inbound, equal_to: [:inbound, :outbound],
                description: "The direction of the firewall rule. Direction means either inbound or outbound traffic.",
                coerce: proc { |d| d.is_a?(String) ? d.downcase.to_sym : d }
 
@@ -60,14 +63,12 @@ class Chef
                description: "The protocol the firewall rule applies to."
 
       property :firewall_action, [Symbol, String],
-               default: :allow,
-               equal_to: [:allow, :block, :notconfigured],
+               default: :allow, equal_to: [:allow, :block, :notconfigured],
                description: "The action of the firewall rule.",
                coerce: proc { |f| f.is_a?(String) ? f.downcase.to_sym : f }
 
       property :profile, [Symbol, String],
-               default: :any,
-               equal_to: [:public, :private, :domain, :any, :notapplicable],
+               default: :any, equal_to: [:public, :private, :domain, :any, :notapplicable],
                description: "The profile the firewall rule applies to.",
                coerce: proc { |p| p.is_a?(String) ? p.downcase.to_sym : p }
 
@@ -78,8 +79,7 @@ class Chef
                description: "The service the firewall rule applies to."
 
       property :interface_type, [Symbol, String],
-               default: :any,
-               equal_to: [:any, :wireless, :wired, :remoteaccess],
+               default: :any, equal_to: [:any, :wireless, :wired, :remoteaccess],
                description: "The interface type the firewall rule applies to.",
                coerce: proc { |i| i.is_a?(String) ? i.downcase.to_sym : i }
 
@@ -102,9 +102,9 @@ class Chef
           state = Chef::JSONCompat.from_json(output.stdout)
         end
         local_address state["local_address"]
-        local_port state["local_port"]
+        local_port Array(state["local_port"]).sort
         remote_address state["remote_address"]
-        remote_port state["remote_port"]
+        remote_port Array(state["remote_port"]).sort
         direction state["direction"]
         protocol state["protocol"]
         firewall_action state["firewall_action"]
@@ -116,6 +116,8 @@ class Chef
       end
 
       action :create do
+        description "Create a Windows firewall entry."
+
         if current_resource
           converge_if_changed :rule_name, :local_address, :local_port, :remote_address, :remote_port, :direction,
                               :protocol, :firewall_action, :profile, :program, :service, :interface_type, :enabled do
@@ -131,6 +133,8 @@ class Chef
       end
 
       action :delete do
+        description "Delete an existing Windows firewall entry."
+
         if current_resource
           converge_by("delete firewall rule #{new_resource.rule_name}") do
             powershell_out!("Remove-NetFirewallRule -Name '#{new_resource.rule_name}'")
@@ -148,9 +152,9 @@ class Chef
           cmd << " -DisplayName '#{new_resource.rule_name}'" if cmdlet_type == "New"
           cmd << " -Description '#{new_resource.description}'" if new_resource.description
           cmd << " -LocalAddress '#{new_resource.local_address}'" if new_resource.local_address
-          cmd << " -LocalPort '#{new_resource.local_port}'" if new_resource.local_port
+          cmd << " -LocalPort #{new_resource.local_port.join(',')}" if new_resource.local_port
           cmd << " -RemoteAddress '#{new_resource.remote_address}'" if new_resource.remote_address
-          cmd << " -RemotePort '#{new_resource.remote_port}'" if new_resource.remote_port
+          cmd << " -RemotePort #{new_resource.remote_port.join(',')}" if new_resource.remote_port
           cmd << " -Direction '#{new_resource.direction}'" if new_resource.direction
           cmd << " -Protocol '#{new_resource.protocol}'" if new_resource.protocol
           cmd << " -Action '#{new_resource.firewall_action}'" if new_resource.firewall_action
@@ -170,6 +174,7 @@ class Chef
       # # @return [String] current firewall state
       def load_firewall_state(rule_name)
         <<-EOH
+          Remove-TypeData System.Array # workaround for PS bug here: https://bit.ly/2SRMQ8M
           $rule = Get-NetFirewallRule -Name '#{rule_name}'
           $addressFilter = $rule | Get-NetFirewallAddressFilter
           $portFilter = $rule | Get-NetFirewallPortFilter
diff --git a/spec/unit/resource/windows_firewall_rule_spec.rb b/spec/unit/resource/windows_firewall_rule_spec.rb
index b20425e21b..629e91d74f 100644
--- a/spec/unit/resource/windows_firewall_rule_spec.rb
+++ b/spec/unit/resource/windows_firewall_rule_spec.rb
@@ -53,9 +53,29 @@ describe Chef::Resource::WindowsFirewallRule do
     expect(resource.local_address).to eql("192.168.1.1")
   end
 
+  it "the local_port property accepts integers" do
+    resource.local_port(8080)
+    expect(resource.local_port).to eql(["8080"])
+  end
+
   it "the local_port property accepts strings" do
     resource.local_port("8080")
-    expect(resource.local_port).to eql("8080")
+    expect(resource.local_port).to eql(["8080"])
+  end
+
+  it "the local_port property accepts comma separated lists without spaces" do
+    resource.local_port("8080,8081")
+    expect(resource.local_port).to eql(%w{8080 8081})
+  end
+
+  it "the local_port property accepts comma separated lists with spaces" do
+    resource.local_port("8080, 8081")
+    expect(resource.local_port).to eql(%w{8080 8081})
+  end
+
+  it "the local_port property accepts arrays and coerces to a sorta array of strings" do
+    resource.local_port([8081, 8080])
+    expect(resource.local_port).to eql(%w{8080 8081})
   end
 
   it "the remote_address property accepts strings" do
@@ -65,7 +85,27 @@ describe Chef::Resource::WindowsFirewallRule do
 
   it "the remote_port property accepts strings" do
     resource.remote_port("8081")
-    expect(resource.remote_port).to eql("8081")
+    expect(resource.remote_port).to eql(["8081"])
+  end
+
+  it "the remote_port property accepts integers" do
+    resource.remote_port(8081)
+    expect(resource.remote_port).to eql(["8081"])
+  end
+
+  it "the remote_port property accepts comma separated lists without spaces" do
+    resource.remote_port("8080,8081")
+    expect(resource.remote_port).to eql(%w{8080 8081})
+  end
+
+  it "the remote_port property accepts comma separated lists with spaces" do
+    resource.remote_port("8080, 8081")
+    expect(resource.remote_port).to eql(%w{8080 8081})
+  end
+
+  it "the remote_port property accepts arrays and coerces to a sorta array of strings" do
+    resource.remote_port([8081, 8080])
+    expect(resource.remote_port).to eql(%w{8080 8081})
   end
 
   it "the direction property accepts :inbound and :outbound" do
@@ -162,12 +202,12 @@ describe Chef::Resource::WindowsFirewallRule do
 
   it "aliases :localport to :local_port" do
     resource.localport("80")
-    expect(resource.local_port).to eql("80")
+    expect(resource.local_port).to eql(["80"])
   end
 
   it "aliases :remoteport to :remote_port" do
     resource.remoteport("8080")
-    expect(resource.remote_port).to eql("8080")
+    expect(resource.remote_port).to eql(["8080"])
   end
 
   it "aliases :interfacetype to :interface_type" do
@@ -197,7 +237,7 @@ describe Chef::Resource::WindowsFirewallRule do
 
       it "sets LocalPort" do
         resource.local_port("80")
-        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule' -DisplayName 'test_rule' -Description 'Firewall rule' -LocalPort '80' -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
+        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule' -DisplayName 'test_rule' -Description 'Firewall rule' -LocalPort 80 -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
       end
 
       it "sets RemoteAddress" do
@@ -207,7 +247,7 @@ describe Chef::Resource::WindowsFirewallRule do
 
       it "sets RemotePort" do
         resource.remote_port("443")
-        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule' -DisplayName 'test_rule' -Description 'Firewall rule' -RemotePort '443' -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
+        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule' -DisplayName 'test_rule' -Description 'Firewall rule' -RemotePort 443 -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
       end
 
       it "sets Direction" do
@@ -265,7 +305,7 @@ describe Chef::Resource::WindowsFirewallRule do
         resource.service("SomeService")
         resource.interface_type(:remoteaccess)
         resource.enabled(false)
-        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule_the_second' -DisplayName 'test_rule_the_second' -Description 'some other rule' -LocalAddress '192.168.40.40' -LocalPort '80' -RemoteAddress '8.8.4.4' -RemotePort '8081' -Direction 'outbound' -Protocol 'UDP' -Action 'notconfigured' -Profile 'domain' -Program '%WINDIR%\\System32\\lsass.exe' -Service 'SomeService' -InterfaceType 'remoteaccess' -Enabled 'false'")
+        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule_the_second' -DisplayName 'test_rule_the_second' -Description 'some other rule' -LocalAddress '192.168.40.40' -LocalPort 80 -RemoteAddress '8.8.4.4' -RemotePort 8081 -Direction 'outbound' -Protocol 'UDP' -Action 'notconfigured' -Profile 'domain' -Program '%WINDIR%\\System32\\lsass.exe' -Service 'SomeService' -InterfaceType 'remoteaccess' -Enabled 'false'")
       end
     end
 
@@ -286,7 +326,7 @@ describe Chef::Resource::WindowsFirewallRule do
 
       it "sets LocalPort" do
         resource.local_port("80")
-        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule' -Description 'Firewall rule' -LocalPort '80' -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
+        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule' -Description 'Firewall rule' -LocalPort 80 -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
       end
 
       it "sets RemoteAddress" do
@@ -296,7 +336,7 @@ describe Chef::Resource::WindowsFirewallRule do
 
       it "sets RemotePort" do
         resource.remote_port("443")
-        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule' -Description 'Firewall rule' -RemotePort '443' -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
+        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule' -Description 'Firewall rule' -RemotePort 443 -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
       end
 
       it "sets Direction" do
@@ -354,7 +394,7 @@ describe Chef::Resource::WindowsFirewallRule do
         resource.service("SomeService")
         resource.interface_type(:remoteaccess)
         resource.enabled(false)
-        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule_the_second' -Description 'some other rule' -LocalAddress '192.168.40.40' -LocalPort '80' -RemoteAddress '8.8.4.4' -RemotePort '8081' -Direction 'outbound' -Protocol 'UDP' -Action 'notconfigured' -Profile 'domain' -Program '%WINDIR%\\System32\\lsass.exe' -Service 'SomeService' -InterfaceType 'remoteaccess' -Enabled 'false'")
+        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule_the_second' -Description 'some other rule' -LocalAddress '192.168.40.40' -LocalPort 80 -RemoteAddress '8.8.4.4' -RemotePort 8081 -Direction 'outbound' -Protocol 'UDP' -Action 'notconfigured' -Profile 'domain' -Program '%WINDIR%\\System32\\lsass.exe' -Service 'SomeService' -InterfaceType 'remoteaccess' -Enabled 'false'")
       end
     end
   end