Bugfix/issue 416

doc/_resource_types/security_group.md

@@ -19,13 +19,30 @@ end
 ```ruby
 describe security_group('my-security-group-name') do
   its(:outbound) { should be_opened }
+  its(:outbound) { should be_opened(443).protocol('tcp').target('203.0.113.1/32') }
   its(:inbound) { should be_opened(80) }
   its(:inbound) { should be_opened(80).protocol('tcp').for('203.0.113.1/32') }
   its(:inbound) { should be_opened(22).protocol('tcp').for('sg-5a6b7cd8') }
 end
 ```
 
-### advanced
+#### Defaults for `:inbound` and `:outbound`
+
+Beware for defaults when not specifying protocol (`protocol()`) and/or the CIDR
+(`for()` or `target()`).
+
+If no protocol is provided, it is assumed to be `-1`.
+If no CIDR is provided, it is assumed to be `0.0.0.0/0`.
+
+Of course, if your current security group being tested does not include those
+defaults your spec will fail to match with `should be_opened`.
+
+In most cases, you will want to at least provide the port and protocol.
+
+If no port number is provided, the `MissingPortSpecification` exception will be
+raised.
+
+### Advanced
 
 `security_group` can use `Aws::EC2::SecurityGroup` resource (see http://docs.aws.amazon.com/sdkforruby/api/Aws/EC2/SecurityGroup.html).
 

doc/resource_types.md

@@ -3349,32 +3349,31 @@ end
 ```ruby
 describe security_group('my-security-group-name') do
   its(:outbound) { should be_opened }
+  its(:outbound) { should be_opened(443).protocol('tcp').target('203.0.113.1/32') }
   its(:inbound) { should be_opened(80) }
   its(:inbound) { should be_opened(80).protocol('tcp').for('203.0.113.1/32') }
   its(:inbound) { should be_opened(22).protocol('tcp').for('sg-5a6b7cd8') }
 end
 ```
 
+#### Defaults for `:inbound` and `:outbound`
 
-### its(:inbound_rule_count), its(:outbound_rule_count), its(:inbound_permissions_count), its(:outbound_permissions_count), its(:description), its(:group_name), its(:owner_id), its(:group_id), its(:vpc_id)
-### :unlock: Advanced use
+Beware for defaults when not specifying protocol (`protocol()`) and/or the CIDR
+(`for()` or `target()`).
 
-`security_group` can use `Aws::EC2::SecurityGroup` resource (see http://docs.aws.amazon.com/sdkforruby/api/Aws/EC2/SecurityGroup.html).
+If no protocol is provided, it is assumed to be `-1`.
+If no CIDR is provided, it is assumed to be `0.0.0.0/0`.
 
-```ruby
-describe security_group('my-security-group-name') do
-  its('group_name') { should eq 'my-security-group-name' }
-end
-```
+Of course, if your current security group being tested does not include those
+defaults your spec will fail to match with `should be_opened`.
 
-or
+In most cases, you will want to at least provide the port and protocol.
+
+If no port number is provided, the `MissingPortSpecification` exception will be
+raised.
 
-```ruby
-describe security_group('my-security-group-name') do
-  its('resource.group_name') { should eq 'my-security-group-name' }
-end
-```
 
+### its(:inbound_rule_count), its(:outbound_rule_count), its(:inbound_permissions_count), its(:outbound_permissions_count), its(:description), its(:group_name), its(:owner_id), its(:group_id), its(:vpc_id)
 ## <a name="ses_identity">ses_identity</a>
 
 SesIdentity resource type.

lib/awspec/error.rb

@@ -29,4 +29,18 @@ def initialize(resource_type, id)
   # Let the user know the configuration they provided is not known.
   class UnknownConfiguration < StandardError
   end
+
+  class MissingPortSpecification < StandardError
+    def initialize(default_protocol)
+      msg = "Port must be specificed unless protocol is #{default_protocol}"
+      super msg
+    end
+  end
+
+  class InvalidPortRange < StandardError
+    def initialize(port, from_port, to_port)
+      msg = "Found a port range from #{from_port} to #{to_port}, but cannot compare it with '#{port}'"
+      super msg
+    end
+  end
 end

lib/awspec/matcher/be_opened.rb

@@ -1,8 +1,17 @@
+DEFAULT_PROTOCOL = '-1'
+DEFAULT_ROUTE = '0.0.0.0/0'
+
 RSpec::Matchers.define :be_opened do |port|
   match do |sg|
     sg.opened?(port, @protocol, @cidr)
   end
 
+  description do
+    @protocol = DEFAULT_PROTOCOL if @protocol.nil?
+    @cidr = DEFAULT_ROUTE if @cidr.nil?
+    "to be opened on port #{port}, protocol \"#{@protocol}\", to/from #{@cidr}"
+  end
+
   chain :protocol do |protocol|
     @protocol = protocol
   end

lib/awspec/stub/security_group.rb

@@ -112,6 +112,16 @@
                 }
               ]
             },
+            {
+              from_port: 443,
+              to_port: 443,
+              ip_protocol: 'tcp',
+              ip_ranges: [
+                {
+                  cidr_ip: '0.0.0.0/0'
+                }
+              ]
+            },
             {
               from_port: 8080,
               to_port: 8080,

lib/awspec/type/security_group.rb

@@ -4,6 +4,9 @@ class SecurityGroup < ResourceBase
     aws_resource Aws::EC2::SecurityGroup
     tags_allowed
 
+    DEFAULT_PROTOCOL = '-1'
+    DEFAULT_ROUTE = '0.0.0.0/0'
+
     def resource_via_client
       @resource_via_client ||= find_security_group(@display_name)
     end
@@ -22,10 +25,18 @@ def opened_only?(port = nil, protocol = nil, cidr = nil)
       outbound_opened_only?(port, protocol, cidr)
     end
 
-    def inbound_opened?(port = nil, protocol = nil, cidr = nil)
+    def inbound_opened?(port, protocol, cidr = DEFAULT_ROUTE)
+      # https://stackoverflow.com/questions/39021545/how-to-specify-all-ports-in-security-group-cloudformation
+      raise Awspec::MissingPortSpecification, DEFAULT_PROTOCOL if port.nil? && protocol != DEFAULT_PROTOCOL
+      # a security group will accept everything from itself
+      return true if cidr == id
+
       resource_via_client.ip_permissions.find do |permission|
-        cidr_opened?(permission, cidr) && protocol_opened?(permission, protocol) && port_opened?(permission, port)
+        return true if cidr_opened?(permission, cidr) && protocol_opened?(permission, protocol) && port_opened?(
+          permission, port)
       end
+
+      false
     end
 
     def inbound_opened_only?(port = nil, protocol = nil, cidr = nil)
@@ -40,6 +51,12 @@ def inbound_opened_only?(port = nil, protocol = nil, cidr = nil)
     end
 
     def outbound_opened?(port = nil, protocol = nil, cidr = nil)
+      # https://stackoverflow.com/questions/39021545/how-to-specify-all-ports-in-security-group-cloudformation
+      raise Awspec::MissingPortSpecification, DEFAULT_PROTOCOL if port.nil? && protocol != DEFAULT_PROTOCOL
+
+      # a security group will accept everything from itself
+      return true if cidr == id
+
       resource_via_client.ip_permissions_egress.find do |permission|
         cidr_opened?(permission, cidr) && protocol_opened?(permission, protocol) && port_opened?(permission, port)
       end
@@ -103,11 +120,13 @@ def outbound_rule_count
     private
 
     def cidr_opened?(permission, cidr)
-      return true unless cidr
+      cidr = DEFAULT_ROUTE if cidr.nil?
+
       ret = permission.prefix_list_ids.select do |prefix_list_id|
         prefix_list_id.prefix_list_id == cidr
       end
       return true if ret.count > 0
+
       ret = permission.ip_ranges.select do |ip_range|
         # if the cidr is an IP address then do a true CIDR match
         if cidr =~ /^\d+\.\d+\.\d+\.\d+/
@@ -118,6 +137,7 @@ def cidr_opened?(permission, cidr)
         end
       end
       return true if ret.count > 0
+
       ret = permission.user_id_group_pairs.select do |sg|
         # Compare the sg group_name if the remote group is in another account.
         # find_security_group call doesn't return info on a remote security group.
@@ -136,23 +156,25 @@ def cidr_opened?(permission, cidr)
     end
 
     def protocol_opened?(permission, protocol)
-      return true unless protocol
       return false if protocol == 'all' && permission.ip_protocol != '-1'
       return true if permission.ip_protocol == '-1'
       permission.ip_protocol == protocol
     end
 
     def port_opened?(permission, port)
-      return true unless port
       return true unless permission.from_port
       return true unless permission.to_port
       port_between?(port, permission.from_port, permission.to_port)
     end
 
     def port_between?(port, from_port, to_port)
-      if port.is_a?(String) && port.include?('-')
+      if port.is_a?(String) && port.include?('-') && port =~ /^\d+-\d+$/
         f, t = port.split('-')
         from_port == f.to_i && to_port == t.to_i
+      elsif port.is_a?(String)
+        raise Awspec::InvalidPortRange.new(port, from_port, to_port) unless port =~ /^\d+$/
+        converted = port.to_i
+        converted.between?(from_port, to_port)
       else
         port.between?(from_port, to_port)
       end

spec/generator/spec/security_group_spec.rb

@@ -20,11 +20,12 @@
   its(:inbound) { should be_opened('50000-50009').protocol('tcp').for('123.45.67.89/32') }
   its(:inbound) { should be_opened.protocol('all').for('sg-3a4b5cd6') }
   its(:outbound) { should be_opened(50000).protocol('tcp').for('100.45.67.12/32') }
+  its(:outbound) { should be_opened(443).protocol('tcp').for('0.0.0.0/0') }
   its(:outbound) { should be_opened(8080).protocol('tcp').for('group-in-other-aws-account-with-vpc-peering') }
   its(:inbound_rule_count) { should eq 8 }
-  its(:outbound_rule_count) { should eq 2 }
+  its(:outbound_rule_count) { should eq 3 }
   its(:inbound_permissions_count) { should eq 7 }
-  its(:outbound_permissions_count) { should eq 3 }
+  its(:outbound_permissions_count) { should eq 4 }
   it { should belong_to_vpc('my-vpc') }
 end
 EOF

spec/type/security_group_spec.rb

@@ -1,31 +1,65 @@
 require 'spec_helper'
+require 'awspec/error'
+
 Awspec::Stub.load 'security_group'
 
 describe security_group('sg-1a2b3cd4') do
   it { should exist }
-  its(:inbound) { should be_opened(80) }
+  it do
+    # only way found to force and test the exception
+    subject.inbound
+    expect { subject.opened?(nil) }.to raise_error(Awspec::MissingPortSpecification, /protocol\sis\s-1$/)
+  end
+  its(:inbound) { should_not be_opened(80) }
+  its(:inbound) { should_not be_opened(80).protocol(-1) }
+  its(:inbound) { should be_opened(80).protocol(-1).for('sg-3a4b5cd6') }
   its(:inbound) { should be_opened(80).protocol('tcp').for('123.45.68.89/32') }
   its(:inbound) { should be_opened(80).protocol('tcp').for('123.45.67.0/25') }
   its(:inbound) { should be_opened(80).protocol('tcp').for('123.45.67.1/32') }
   its(:inbound) { should_not be_opened(80).protocol('tcp').for('123.45.0.0/16') }
-  its(:inbound) { should be_opened(22) }
+
+  its(:inbound) { should be_opened(22).protocol('tcp').for('sg-1a2b3cd4') }
+  its(:inbound) { should be_opened(22).protocol('udp').for('sg-1a2b3cd4') }
+  its(:inbound) { should be_opened(22).protocol('-1').for('sg-1a2b3cd4') }
+  its(:inbound) { should be_opened(22).protocol('icmp').for('sg-1a2b3cd4') }
+
   its(:inbound) { should be_opened(22).protocol('tcp').for('sg-5a6b7cd8') }
   its(:inbound) { should be_opened('50000-50009').protocol('tcp').for('123.45.67.89/32') }
   its(:inbound) { should_not be_opened('50010-50019').protocol('tcp').for('123.45.67.89/32') }
-  its(:outbound) { should be_opened(50_000) }
+
+  its(:outbound) { should be_opened(50_000).protocol('tcp').for('sg-1a2b3cd4') }
+  its(:outbound) { should be_opened(50_000).protocol('udp').for('sg-1a2b3cd4') }
+  its(:outbound) { should be_opened(50_000).protocol('icmp').for('sg-1a2b3cd4') }
+  its(:outbound) { should be_opened(50_000).protocol('-1').for('sg-1a2b3cd4') }
+
   its(:outbound) { should be_opened(8080).protocol('tcp').for('sg-9a8b7c6d') }
   its(:outbound) { should be_opened(8080).protocol('tcp').for('group-in-other-aws-account-with-vpc-peering') }
   its(:inbound) { should be_opened_only(60_000).protocol('tcp').for('100.45.67.12/32') }
   its(:inbound) { should be_opened_only(70_000).protocol('tcp').for(['100.45.67.89/32', '100.45.67.12/32']) }
   its(:outbound) { should be_opened_only(50_000).protocol('tcp').for('100.45.67.12/32') }
-  its(:inbound) { should be_opened.protocol('all').for('sg-3a4b5cd6') }
-  its(:outbound) { should be_opened.protocol('tcp').for('pl-a5321fa3') }
+  its(:inbound) { should be_opened.protocol('-1').for('sg-3a4b5cd6') }
+
+  it do
+    # only way found to force and test the exception
+    subject.outbound
+    expect { subject.opened?(nil, 'tcp', 'pl-a5321fa3') }.to raise_error(
+      Awspec::MissingPortSpecification, /protocol\sis\s-1$/)
+  end
+  its(:outbound) { should be_opened('443').protocol('tcp').for('pl-a5321fa3') }
+  its(:outbound) { should be_opened(443).protocol('tcp').for('pl-a5321fa3') }
+  its(:outbound) { should be_opened('443-443').protocol('tcp').for('pl-a5321fa3') }
+  it do
+    # only way found to force and test the exception
+    subject.outbound
+    expect { subject.opened?('yada-443-yada', 'tcp', 'pl-a5321fa3') }.to raise_error(Awspec::InvalidPortRange)
+  end
+
   its(:inbound_permissions_count) { should eq 7 }
   its(:ip_permissions_count) { should eq 7 }
-  its(:outbound_permissions_count) { should eq 3 }
-  its(:ip_permissions_egress_count) { should eq 3 }
+  its(:outbound_permissions_count) { should eq 4 }
+  its(:ip_permissions_egress_count) { should eq 4 }
   its(:inbound_rule_count) { should eq 8 }
-  its(:outbound_rule_count) { should eq 2 }
+  its(:outbound_rule_count) { should eq 3 }
 
   it { should belong_to_vpc('vpc-ab123cde') }
   it { should belong_to_vpc('my-vpc') }
@@ -95,14 +129,15 @@
 
 describe security_group('my-security-group-name') do
   it { should exist }
-  its(:outbound) { should be_opened(50_000) }
-  its(:inbound) { should be_opened(80) }
+  its(:outbound) { should_not be_opened(50_000) }
+  its(:outbound) { should_not be_opened(50_000).protocol('tcp') }
+  its(:outbound) { should be_opened(50_000).protocol('tcp').target('100.45.67.12/32') }
+  its(:outbound) { should be_opened(443).protocol('tcp').target('0.0.0.0/0') }
+  its(:outbound) { should be_opened(443).protocol('tcp') }
+  its(:inbound) { should_not be_opened(80) }
+  its(:inbound) { should_not be_opened(80).protocol('tcp') }
+  its(:inbound) { should be_opened(80).protocol('tcp').for('123.45.67.0/24') }
+  its(:inbound) { should be_opened(80).protocol('tcp').for('123.45.68.89/32') }
   it { should belong_to_vpc('my-vpc') }
   it { should have_tag('env').value('dev') }
 end
-
-describe security_group('my-security-tag-name') do
-  its(:outbound) { should be_opened(50_000) }
-  its(:inbound) { should be_opened(80) }
-  it { should belong_to_vpc('my-vpc') }
-end