security_group be_opened matches on nil

[rhysbarrett]

I suspect this is a rather serious bug, best demonstrated by example:

describe security_group('dpp-prod-user_api') do
    its(:inbound) { should be_opened(nil) }
end

This test passes when I assume it really shouldn't - bearing in mind the security group I'm testing against has a rule for ports 5001-5003 (verified with a passing test case of its(:inbound) { should be_opened("5001 - 5003") }.

The implication of this is that malformed configuration files supplying test data to this test will cause the test to pass, regardless of whether it should pass or not.

This is my first Git Issue so I'm sorry if I haven't done this properly or if I've missed something obvious.

[glasswalk3r]

I can confirm the bug:

$ git diff
diff --git a/spec/type/security_group_spec.rb b/spec/type/security_group_spec.rb
index 4587d95..961c3fc 100644
--- a/spec/type/security_group_spec.rb
+++ b/spec/type/security_group_spec.rb
@@ -3,6 +3,7 @@ Awspec::Stub.load 'security_group'
 
 describe security_group('sg-1a2b3cd4') do
   it { should exist }
+  its(:inbound) { should be_opened(nil) }
   its(:inbound) { should be_opened(80) }
   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') }

Now running the related the spec:

$ bundle exec rspec -f d spec/type/security_group_spec.rb 

security_group 'sg-1a2b3cd4'
  is expected to exist
  is expected to belong to vpc "vpc-ab123cde"
  is expected to belong to vpc "my-vpc"
  is expected to have tag "env" value "dev"
  inbound
    is expected to be opened nil
  inbound
    is expected to be opened 80
  inbound
    is expected to be opened 80
  inbound
    is expected to be opened 80
...

I found that there are also other issues with security_group:

• Awspec::Helper::Finder::SecurityGroup::describe_security_groups does not check for empty results:

      def describe_security_groups(param)
        ec2_client.describe_security_groups(param).security_groups
      end

• The stub response lib/awspec/stub/security_group.rb in use for the Spec is being reused for all queries, even those that should work, which is the case of the last example group below:

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

There is no such label my-security-tag-name in the stub, but all example groups will reuse the single security group available in the stub.

[glasswalk3r]

Just created the PR #546 to address this issue. @rhysbarrett , could you please give a shot with this PR?

@k1LoW , the promoted changes might be incompatible with older specs written by end users.

The reason for that is when writing a spec, just checking if port 80 is opened means basically nothing, AFAIK, that's not how a security group is expect to work, unless there is a rule where the protocol is "-1" (everything) and the CIDR is defined as 0.0.0.0/0 (from/to anywhere).

The current implementation will raise an exception if a port number is no given, so this method:

    def port_opened?(permission, port)
      return true unless permission.from_port
      return true unless permission.to_port
      port_between?(port, permission.from_port, permission.to_port)
    end

Doesn't make much sense the way is defined right now.
I also reviewed the stub and all the tests. The last spec was "miraculously" working even though it was referring to an nonexistent stub values.

Finally, I'm not sure the best place to include this constants:

DEFAULT_PROTOCOL = '-1'
DEFAULT_ROUTE = '0.0.0.0/0'

Currently they are defined in both lib/awspec/type/security_group.rb and lib/awspec/matcher/be_opened.rb. Please advise.