Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make "owners" field of source_ami_filter required: RFC #6584

Closed
SwampDragons opened this issue Aug 13, 2018 · 10 comments · Fixed by #6585
Closed

Make "owners" field of source_ami_filter required: RFC #6584

SwampDragons opened this issue Aug 13, 2018 · 10 comments · Fixed by #6585

Comments

@SwampDragons
Copy link
Contributor

HashiCorp's security team pointed out an interesting potential exploit where if you request an amazon AMI via a source_ami_filter, but don't have "owner" selected, you can accidentally use a malicious base image instead of one from your own organization or a trusted vendor.

The impact of making the "owner" field required would be low for the users (I strongly suspect that a majority of people using the filter already define the owner field) but could save users from making a potentially dangerous mistake. I think we should have Packer fail during validation with an error that says this field must be designated; if we decide that there is a valid use case where people may not want to have to set this field, we could potentially allow an opt out like setting "owner" to "any" but honestly I can't think of a situation where this really makes a great deal of sense.

I'm going to slate this for v1.3.0 but I'd like to hear community thoughts.

@007
Copy link

007 commented Aug 14, 2018

👍 to this, absolutely.

I encountered exactly this bug last week, ended up with a Monero miner instead of a vanilla Ubuntu AMI when the owner filter was missed.

@SwampDragons
Copy link
Contributor Author

SwampDragons commented Aug 15, 2018

@007 Can you shoot me an email? I have a couple of questions about your experience with this issue.

@007
Copy link

007 commented Aug 15, 2018

Done

@nodesocket
Copy link

@SwampDragons I can't upgrade to Packer 1.3.0 because of this change. Essentially we use the same JSON Packer definitions for production AWS account as development AWS account, and only have to modify the profile to toggle between production and development AWS credentials.

The problem is the owners field id now is different as well for production and development AWS accounts. Can you provide a solution to this? Is there an abstraction I am missing? Thanks.

@SwampDragons
Copy link
Contributor Author

@nodesocket the "owners" field is a list, so I believe you should be able to simply provide both possible "owner" IDs and have the filter still work for both. Then no matter which (prod or dev) owner owns the AMI, you should be able to find one.

@nodesocket
Copy link

Ahh, didn't realize owners take a list. That worked. Thanks!

@SwampDragons
Copy link
Contributor Author

Awesome! Super glad it isn't a blocker.

@rickard-von-essen
Copy link
Collaborator

You can also use self.

We should probably add some more info in the docs. From the AWS docs:

Filters the images by the owner. Specify an AWS account ID, self (owner is the sender of the request), or an AWS owner alias (valid values are amazon | aws-marketplace | microsoft ). Omitting this option returns all images for which you have launch permissions, regardless of ownership

https://docs.aws.amazon.com/cli/latest/reference/ec2/describe-images.html

@SwampDragons
Copy link
Contributor Author

Good point. I've opened a PR updating docs: #6694

@ghost
Copy link

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants