add security group functionality

[chadsbrown]

Presently, this cookbook lacks the ability to affect an EC2 instance’s security groups. Given that the AWS API allows this sort of change to instances (in a VPC), this doesn’t seem to be a provisioning-specific detail. Consequently, I see no reason that chef shouldn’t manage this (create SGs and associate them with an instance). I’m interested in working on adding this to this cookbook, but wondered what the group’s opinion would be prior to starting work.

[tas50]

Security group functionality is certainly something we'd like to get into this cookbook. At the moment I'd suggest the aws_security cookbook which has a resorce for managing security groups.

https://supermarket.chef.io/cookbooks/aws_security/versions/0.1.1

[majormoses]

@tas50 I wonder if this is a bit anti pattern from a security stance. In order to facilitate this the instance running the chef-client will require the ability to create/delete security groups which means that any node would be able to do it. IMHO a node should not manage its own security group and should be handled by the provisioner of the instance via terraform, cloudformation, etc. If you are already granting specific permissions to allow it create/delete/update security groups matching a namespace/prefix then what's the value of doing this?

[majormoses]

I suppose if you had a dedicated node for updating security groups that would be slightly better but IMO letting a node update it's own security groups is a security risk.

[chadsbrown]

Back in the day, using chef provisioning nodes was more common.

[majormoses]

Back in the day, using chef provisioning nodes was more common.

I had actually forgotten about chef provisioning being a thing.

[smcavallo]

Many provisioning tools like cloudformation and terraform allow managing/associating security groups.
The use case here is for ec2 instances which are not provisioned with any of those tools.

I would argue that the majority of the providers in this cookbook are an anti pattern from a security stance. There are providers which can delete cloudformation stacks, delete dynamodb tables, delete ebs volumes, delete elbs, manage IAM users, etc. This cookbook presents providers which could easily be poorly implemented, abused or exploited. Most of what this cookbook does would be better handled using cloudformation or terraform in general.
We are trusting that consumers know what they are doing, AND most importantly when they are calling the providers they are using aws credentials having policies which follow the principles of least privilege.

I know it feels like an anti-pattern to do this - and it does for me as well in many ways.
However, for us it gives us a way to manage security groups/rules on a subset of infrastructure which is not covered by cloudformation yet. It lets us define the rules using infrastructure as code, version these config changes, and have these rules deployed to our various environments via our chef deployment pipeline.
We can use this chef provider and not have to write or maintain code or unit tests to do any of this.
Anytime we can use a chef or community supported cookbook instead of creating and maintaining our own custom code is a win for us.

[majormoses]

Ya I certainly see where you are are coming from and agree that there are quite a few security concerns when using this cookbook to its full potential. I'd recommend importing it into your (cloudformation|terraform) and avoid giving such access to the node to be able to make such changes. I still see the value in what you are trying to do and it's not my place to tell you you can't. I think the best thing we can do is put something in the readme about the dangers of giving overly permissive IAM access to the node. I am only really using this cookbook for pulling secrets from ssm and am wondering if we should have a cookbook scoped down to non destructive (list, describe, read only, attach self to LB, etc) stuff in aws so that more security conscious setups can avoid bringing in the extra dangers. Obviously at the end of the day the real protection comes into play at the IAM level. I just have seen way too many overly permissive to not want to try to save people from themselves.

[smcavallo]

@majormoses - here is our use case where this cookbook could actually improve security. We'd like this cookbook to keep our ingress/egress rules in sync. There is a possibility that someone or something could add a security group rule to a node for testing, and open something up accidentally, or forget to close it. Actually it happens all the time - someone is testing or makes a change to allow a port.
This cookbook would ensure that upon converge the ingress/egress would get back in sync.
This feature has the potential to remediate any manual changes to security groups, which could potentially prevent something from being exposed/compromised.

[majormoses]

I see, I am OK with moving forward at the end of the day IAM is where the real protection into place to prevent this from being abused and its up to each implementor to decide their level of comfort. I would still like there to be somewhere in the README where we talk about the dangers of this.

[smcavallo]

@majormoses - See #381
Feel free to modify as you feel appropriate.