ECR support #730
ECR support #730
Conversation
|
Awesome! Overall this looks good. I have some minor nits around style consistency, but nothing major I think. |
| svc ECR | ||
| } | ||
|
|
||
| func NewEcrAuthProvider(svc ECR) *ecrAuthProvider { |
There was a problem hiding this comment.
Can we use ECR here to be consistent? NewECRAuthProvider.
|
Thanks for the feedback. The things I haven't commented on are already done or have to do with my other comments. I'd appreciate a final round of thoughts from you. |
|
Thanks for the update. I'll go ahead and implement your suggestions shortly. |
|
I've added some commits implementing these changes. If this looks good to you, I'll squash. |
| @@ -298,6 +298,13 @@ | |||
| "ec2:Describe*", | |||
| "elasticloadbalancing:*", | |||
| "ecs:*", | |||
| "ecr:GetAuthorizationToken", | |||
| "ecr:BatchCheckLayerAvailability", | |||
There was a problem hiding this comment.
Minor but, ecr:GetAuthorizationToken looks like it should be the only permission Empire needs?
There was a problem hiding this comment.
I believe anything that wants to pull from ECR using the Docker API needs GetAuthorizationToken, GetDownloadUrlForLayer, BatchGetImage and BatchCheckLayerAvailability. These are included in AmazonEC2ContainerServiceforEC2Role, and also as "pull action permissions" within the ECR console. GetRepositoryPolicy, DescribeRepositories and ListImages shouldn't be necessary, as these pertain to AWS SDK operations. I copied from AmazonEC2ContainerRegistryReadOnly, which is why they're there.
There was a problem hiding this comment.
Cool, makes sense.
On Thursday, January 21, 2016, Martin Häger notifications@github.com
wrote:
In docs/cloudformation.json
#730 (comment):@@ -298,6 +298,13 @@
"ec2:Describe_",
"elasticloadbalancing:_",
"ecs:*",
"ecr:GetAuthorizationToken","ecr:BatchCheckLayerAvailability",I believe anything that wants to pull from ECR using the Docker API needs
GetAuthorizationToken, GetDownloadUrlForLayer, BatchGetImage and
BatchCheckLayerAvailability. These are included in
AmazonEC2ContainerServiceforEC2Role
http://docs.aws.amazon.com/AmazonECS/latest/developerguide/instance_IAM_role.html.
GetRepositoryPolicy, DescribeRepositories and ListImages shouldn't be
necessary, as these pertain to AWS SDK operations. I copied from
AmazonEC2ContainerRegistryReadOnly
http://docs.aws.amazon.com/AmazonECR/latest/userguide/ecr_managed_policies.html#AmazonEC2ContainerRegistryReadOnly,
which is why they're there.—
Reply to this email directly or view it on GitHub
https://github.com/remind101/empire/pull/730/files#r50382480.
|
Awesome. Looks good to me 👍 Feel free to squash and I'll merge. |
|
Squashed and removed the three unnecessary permissions as per above. As expected, Empire works fine without them. |
This PR introduces multiple authentication backends, and adds support for the EC2 Container Registry. This addresses #703.