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

feat: Cross account ECR for lambda functions #88

Merged
merged 11 commits into from
Mar 21, 2022

Conversation

dsme94
Copy link
Contributor

@dsme94 dsme94 commented Dec 2, 2021

what

  • With the introduction of cross-account ECR for lambda functions, I have put together the necessary code to allow for this functionality

why

  • Cross-account ECR is a feature many would use as it doesn't require you to duplicate your ECR repositories in the same account where the lambda function resides saving money

references

https://aws.amazon.com/blogs/compute/introducing-cross-account-amazon-ecr-access-for-aws-lambda/

@dsme94 dsme94 requested review from a team as code owners December 2, 2021 10:17
@dsme94 dsme94 requested review from r351574nc3 and florian0410 and removed request for a team December 2, 2021 10:17
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@dsme94
Copy link
Contributor Author

dsme94 commented Dec 3, 2021

I've made the changes to this MR locally and tested it, works fine. I am not sure about the changes to using the existing policy document and I'll demonstrate why below.

  dynamic "statement" {
    for_each = var.principals_lambda

    content {
      effect  = "Allow"
      actions = [
        "ecr:BatchGetImage",
        "ecr:GetDownloadUrlForLayer"
      ]

      principals {
        type        = "Service"
        identifiers = ["lambda.amazonaws.com"]
      }

      condition {
        test     = "StringLike"
        values   = formatlist("arn:${data.aws_partition.current.partition}:lambda:*:%s:function:*", var.principals_lambda)
        variable = "aws:sourceArn"
      }
    }
  }

The above is the snippet of code that would be used if using the existing policy document. Having the code written like this in the module would produce a policy like this:

+ {
                      + Action    = [
                          + "ecr:GetDownloadUrlForLayer",
                          + "ecr:BatchGetImage",
                        ]
                      + Condition = {
                          + StringLike = {
                              + aws:sourceArn = [
                                  + "arn:aws:lambda:*:222222222222:function:*",
                                  + "arn:aws:lambda:*:333333333333:function:*",
                                  + "arn:aws:lambda:*:444444444444:function:*",
                                ]
                            }
                        }
                      + Effect    = "Allow"
                      + Principal = {
                          + Service = "lambda.amazonaws.com"
                        }
                      + Sid       = ""
                    },
                  + {
                      + Action    = [
                          + "ecr:GetDownloadUrlForLayer",
                          + "ecr:BatchGetImage",
                        ]
                      + Condition = {
                          + StringLike = {
                              + aws:sourceArn = [
                                  + "arn:aws:lambda:*:222222222222:function:*",
                                  + "arn:aws:lambda:*:333333333333:function:*",
                                  + "arn:aws:lambda:*:444444444444:function:*",
                                ]
                            }
                        }
                      + Effect    = "Allow"
                      + Principal = {
                          + Service = "lambda.amazonaws.com"
                        }
                      + Sid       = ""
                    },
                  + {
                      + Action    = [
                          + "ecr:GetDownloadUrlForLayer",
                          + "ecr:BatchGetImage",
                        ]
                      + Condition = {
                          + StringLike = {
                              + aws:sourceArn = [
                                  + "arn:aws:lambda:*:222222222222:function:*",
                                  + "arn:aws:lambda:*:333333333333:function:*",
                                  + "arn:aws:lambda:*:444444444444:function:*",
                                ]
                            }
                        }
                      + Effect    = "Allow"
                      + Principal = {
                          + Service = "lambda.amazonaws.com"
                        }
                      + Sid       = ""
                    },

We would have duplicate statements for each item in principals_lambda.

Sticking to the existing method would allow us to write the policy like so:

+ {
                      + Action    = [
                          + "ecr:GetDownloadUrlForLayer",
                          + "ecr:BatchGetImage",
                        ]
                      + Condition = {
                          + StringLike = {
                              + aws:sourceArn = [
                                  + "arn:aws:lambda:*:222222222222:function:*",
                                  + "arn:aws:lambda:*:333333333333:function:*",
                                  + "arn:aws:lambda:*:444444444444:function:*",
                                ]
                            }
                        }
                      + Effect    = "Allow"
                      + Principal = {
                          + Service = "lambda.amazonaws.com"
                        }
                      + Sid       = "LambdaECRImageCrossAccountRetrievalPolicy"
                    },
                  + {
                      + Action    = [
                          + "ecr:GetDownloadUrlForLayer",
                          + "ecr:BatchGetImage",
                        ]
                      + Effect    = "Allow"
                      + Principal = {
                          + AWS = [
                              + "444444444444",
                              + "333333333333",
                              + "222222222222",
                            ]
                        }
                      + Sid       = "CrossAccountPermission"
                    },

I've tried for a few hours to get this to loop once but just can't seem to get it, and you can't pass a true/false value into for_each nor a string, must be a list.

My concern really is that the policy could grow exponentially in size hitting the AWS policy limit.

If there is a way to do the dynamic statement in the existing aws_policy_document I'd love to know!

@dsme94 dsme94 requested review from nitrocode and removed request for a team December 3, 2021 13:28
dsme94 and others added 3 commits December 3, 2021 14:09
Co-authored-by: nitrocode <nitrocode@users.noreply.github.com>
@dsme94
Copy link
Contributor Author

dsme94 commented Dec 6, 2021

@lorengordon suggested using the length function with the dynamic statement which works perfectly. Updating MR.

  dynamic "statement" {
    for_each = length(var.principals_lambda) > 0 ? [1] : []

    content {
      sid = "LambdaECRImageCrossAccountRetrievalPolicy"
      effect  = "Allow"
      actions = [
        "ecr:BatchGetImage",
        "ecr:GetDownloadUrlForLayer"
      ]

      principals {
        type        = "Service"
        identifiers = ["lambda.amazonaws.com"]
      }

      condition {
        test     = "StringLike"
        values   = formatlist("arn:${data.aws_partition.current.partition}:lambda:*:%s:function:*", var.principals_lambda)
        variable = "aws:sourceArn"
      }
    }
  }

@dsme94 dsme94 mentioned this pull request Dec 7, 2021
@matty-rose
Copy link

matty-rose commented Feb 10, 2022

Hey is there anything blocking this PR? I'd love to be able to use this functionality and would be happy to assist in any way if needed @r351574nc3 @florian0410

@jamengual
Copy link
Contributor

/test all

@jamengual
Copy link
Contributor

please run

make init
make readme

and commit the changes

@matty-rose
Copy link

@dsme94 are you able to help with the above fixes?

@dsme94 dsme94 requested a review from a team as a code owner March 21, 2022 10:36
@dsme94
Copy link
Contributor Author

dsme94 commented Mar 21, 2022

@jamengual @matty-rose - have pushed the new README created using make

Edit:

Have noticed:

identifiers = formatlist("arn:${data.aws_partition.current.partition}:iam::%s:root", var.principals_lambda)

Could be written better like so:

identifiers = formatlist("arn:%s:iam::%s:root", data.aws_partition.current.partition, var.principals_lambda)

Let me know if you would like me to update

@jamengual
Copy link
Contributor

/test all

@jamengual
Copy link
Contributor

on.curre

yes, please do

@dsme94
Copy link
Contributor Author

dsme94 commented Mar 21, 2022

@jamengual That's it done 👍

@jamengual
Copy link
Contributor

/test all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants