Implement an AWSLambdaBasicExecutionRole with the least privilege principle #6692

azizur · 2019-09-15T21:43:45Z

What did you implement:

Closes #6241 Implement an AWSLambdaBasicExecutionRole with the least privilege principle

How did you implement it:

Take the default AWSLambdaBasicExecutionRole and reduce permission scope the function resources.
Reduce duplications in resources list by combining multiple actions into a single policy statement.

How can we verify it:

Previously the inline policy would have looked something like this:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "logs:CreateLogStream"
      ],
      "Resource": [
        "arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{stage}-{function-a}:*",
        "arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{stage}-{function-b}:*",
        "arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{stage}-{function-c}:*",
      ],
      "Effect": "Allow"
    },
    {
      "Action": [
        "logs:PutLogEvents"
      ],
      "Resource": [
        "arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{stage}-{function-a}:*:*",
        "arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{stage}-{function-b}:*:*",
        "arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{stage}-{function-c}:*:*",
      ],
      "Effect": "Allow"
    }
  ]
}

Now

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "logs:CreateLogGroup",
        "logs:CreateLogStream",
        "logs:PutLogEvents"
      ],
      "Resource": [
        "arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{stage}-{function-a}:*",
        "arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{stage}-{function-b}:*",
        "arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{stage}-{function-c}:*",
      ],
      "Effect": "Allow"
    }
  ]
}

Todos:

Note: Run npm run test-ci to run all validation checks on proposed changes

Write tests and confirm existing functionality is not broken.
Validate via npm test
Write documentation
Ensure there are no lint errors.
Validate via npm run lint-updated
Note: Some reported issues can be automatically fixed by running npm run lint:fix
Ensure introduced changes match Prettier formatting.
Validate via npm run prettier-check-updated
Note: All reported issues can be automatically fixed by running npm run prettify-updated
Make sure code coverage hasn't dropped
Provide verification config / commands / resources
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

…ciple Fixes serverless#6241 and reduces the size of the inline policy

evheniy · 2019-09-18T07:28:52Z

Any news when it will be merged?

pmuens

Hey @azizur thanks a lot for working on this 👍

I just did a review and test today and everything works as expected.

I tested a fresh deployment and an "upgrade" deployment (deploying with the current master, then switching to your PR branch and re-deploying). Both worked without any problems.

I'd say this is GTM

@medikoo could you also take a quick look at this?

medikoo

Looks as great improvement.

Left one remark, as in previously merged PR, one of the changes proposed here, was rejected then (it'll be good to double ensure it's 100% safe)

Also I think it should not be marked to close #6241 as that one proposes we rely on AWS police directly, and here we're just reflecting permissions it sets up.

medikoo · 2019-09-18T10:30:13Z

lib/plugins/aws/package/lib/iam-role-lambda-execution-template.json

-            {
-              "Effect": "Allow",
-              "Action": ["logs:PutLogEvents"],
+              "Action": ["logs:CreateLogGroup", "logs:CreateLogStream", "logs:PutLogEvents"],


Interesingly in #6241, it was assumed that resource definition for logs:CreateLogGroup and logs:PutLogEvents cannot be merged -> #6212 (comment)
(cc @rdsedmundo @pmuens )

I take, that after all they can be merged (so for logs:PutLogEvents, :* trailing can be stripped)

Yeah, I thought that it could be a problem, but if you tested and it worked without the :* we should be good.

I didn't :)

@pmuens @azizur when testing, did you cover the case of creating new function, invoking it, and confirming that logs for it are visible in CloudWatch?

Hi everyone, this is my first PR, your feedback and support are really appreciated.

@medikoo I have not tested this specific scenario yet. I will test it and update in this thread.

I have tested it using this simple repo: https://github.com/azizur/serverless-pull-6692 a completely new project and it worked. I have previously tested with my existing project already, so there was no need to test again.

I have tested with additional modification too:

remove *:* from

serverless/lib/plugins/aws/customResources/index.js

Line 136 in 15dce40

`:log-group:${logGroupsPrefix}*:*`,

It worked since it generated policy resource refrences like:

"arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{function-a}-{stage}*:*"

Then I tried with additional modifications with mergeIamTemplates.js

remove :* from

serverless/lib/plugins/aws/package/lib/mergeIamTemplates.js

Line 104 in 15dce40

`:log-group:${customFunctionNamelogGroupsPrefix}:*`,

remove *:* from

serverless/lib/plugins/aws/package/lib/mergeIamTemplates.js

Line 113 in 15dce40

`:log-group:${logGroupsPrefix}*:*`,

It did NOT worked since it generated policy resource refrence like:~

"arn:aws:logs:{AWS_REGION}:{AWS_ACCOUNT}:log-group:/aws/lambda/{function-a}-{stage}"

I think we can go with the additional modifications in

serverless/lib/plugins/aws/customResources/index.js

Line 136 in 15dce40

`:log-group:${logGroupsPrefix}*:*`,

and it will work. Looking forward to feedback from others.

Apologies for change to my previous message. I did not fully understand all the test scenarios. I have now checked the following policies and I can confirm that:

"PolicyDocument": { "Version": "2012-10-17", "Statement": [ { "Effect": "Allow", "Action": [ "logs:CreateLogGroup", "logs:CreateLogStream", "logs:PutLogEvents" ], "Resource": [ { "Fn::Sub": "arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}:log-group:/aws/lambda/{function-a}*:*" } ] } ] }

Works:

✅ creates log groups

✅ creates log streams

✅ Log stream as logs entries. <<< This was missed from my above testing.

When I modified https://github.com/serverless/serverless/blob/15dce40ce0284cf04e4c92754e85118a81cfec47/lib/plugins/aws/package/lib/mergeIamTemplates.js Line 104 and Line 113 in 15dce40.

Works:

✅ creates log groups

✅ creates log streams

Did not work:

❌ Log stream as logs entries. <<< This was missed from my above testing.

Great thanks @azizur for thorough check, we really appreciate it.

Let us know on final outcome.

medikoo · 2019-09-18T10:42:08Z

This one actually fixes #6698

azizur · 2019-09-20T22:44:24Z

DO NOT MERGE YET
I would like to simulate some of the policies in AWS Console to test out what might be a good policy.

jameswoodley · 2019-09-25T10:42:58Z

How we looking on this?

azizur · 2019-09-25T19:57:38Z

Apologies for the lack of update on this. I have got really busy week at work this week. Will try to take a look this weekend.

jameswoodley · 2019-09-27T07:55:22Z

@azizur not at all, thanks for taking the time!

pmuens · 2019-10-10T10:32:30Z

@azizur thanks again for taking the time to work on this! 👌 🥇

I don't want to hurry you and just want to check-in if you have any news on this PR.

Let us know if you need help with anything else or if we should take it from here.

pmuens · 2019-10-24T10:45:58Z

Just wanted to do another follow-up here. Were you able to make any progress on this PR @azizur? Do you need any help here?

Looks like the PR has some conflicts we need to resolve e.g. via a rebase.

michelesr · 2019-12-12T13:15:36Z

This looks like a very useful feature. Any reason it hasn't been merged yet?

Fixes #6241 Closes #6692

medikoo · 2019-12-23T13:02:10Z

Taken in safer implementation at #7120

Fixes #6241 Closes #6692

Implement a AWSLambdaBasicExecutionRole with the least privilege prin…

15dce40

…ciple Fixes serverless#6241 and reduces the size of the inline policy

azizur force-pushed the master branch from fc2188a to 15dce40 Compare September 15, 2019 22:41

azizur marked this pull request as ready for review September 15, 2019 22:56

azizur changed the title ~~Implement a AWSLambdaBasicExecutionRole with the least privilege principle~~ Implement an AWSLambdaBasicExecutionRole with the least privilege principle Sep 15, 2019

pmuens added cat/packaging enhancement pr/in-review labels Sep 16, 2019

pmuens approved these changes Sep 18, 2019

View reviewed changes

pmuens requested a review from medikoo September 18, 2019 09:15

pmuens self-assigned this Sep 18, 2019

pmuens added this to the 1.53.0 milestone Sep 18, 2019

medikoo approved these changes Sep 18, 2019

View reviewed changes

medikoo modified the milestones: 1.53.0, 1.54.0 Sep 25, 2019

medikoo removed this from the 1.54.0 milestone Oct 9, 2019

medikoo added a commit that referenced this pull request Dec 20, 2019

fix(AWS Lambda): Fix CloudWatch logs creation access

65bc2a8

Fixes #6241 Closes #6692

medikoo mentioned this pull request Dec 20, 2019

Fix access rules for CloudWatch logs creation #7120

Merged

medikoo added a commit that referenced this pull request Dec 23, 2019

fix(AWS Lambda): Fix CloudWatch logs creation access

df776d0

Fixes #6241 Closes #6692

medikoo added a commit that referenced this pull request Dec 23, 2019

fix(AWS Lambda): Fix CloudWatch logs creation access

8422760

Fixes #6241 Closes #6692

medikoo added a commit that referenced this pull request Dec 23, 2019

fix(AWS Lambda): Fix CloudWatch logs creation access

a2db989

Fixes #6241 Closes #6692

medikoo closed this in 4a947b2 Dec 23, 2019

astuyve pushed a commit that referenced this pull request Apr 15, 2020

fix(AWS Lambda): Fix CloudWatch logs creation access

c5fcdf0

Fixes #6241 Closes #6692

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement an AWSLambdaBasicExecutionRole with the least privilege principle #6692

Implement an AWSLambdaBasicExecutionRole with the least privilege principle #6692

azizur commented Sep 15, 2019 •

edited

evheniy commented Sep 18, 2019

pmuens left a comment

medikoo left a comment •

edited

medikoo Sep 18, 2019

rdsedmundo Sep 18, 2019

medikoo Sep 18, 2019 •

edited

azizur Sep 18, 2019

azizur Sep 20, 2019 •

edited

azizur Sep 20, 2019

medikoo Sep 23, 2019

medikoo commented Sep 18, 2019

azizur commented Sep 20, 2019

jameswoodley commented Sep 25, 2019

azizur commented Sep 25, 2019

jameswoodley commented Sep 27, 2019

pmuens commented Oct 10, 2019

pmuens commented Oct 24, 2019 •

edited

michelesr commented Dec 12, 2019

medikoo commented Dec 23, 2019

Implement an AWSLambdaBasicExecutionRole with the least privilege principle #6692

Implement an AWSLambdaBasicExecutionRole with the least privilege principle #6692

Conversation

azizur commented Sep 15, 2019 • edited

What did you implement:

How did you implement it:

How can we verify it:

Todos:

evheniy commented Sep 18, 2019

pmuens left a comment

Choose a reason for hiding this comment

medikoo left a comment • edited

Choose a reason for hiding this comment

medikoo Sep 18, 2019

Choose a reason for hiding this comment

rdsedmundo Sep 18, 2019

Choose a reason for hiding this comment

medikoo Sep 18, 2019 • edited

Choose a reason for hiding this comment

azizur Sep 18, 2019

Choose a reason for hiding this comment

azizur Sep 20, 2019 • edited

Choose a reason for hiding this comment

azizur Sep 20, 2019

Choose a reason for hiding this comment

medikoo Sep 23, 2019

Choose a reason for hiding this comment

medikoo commented Sep 18, 2019

azizur commented Sep 20, 2019

jameswoodley commented Sep 25, 2019

azizur commented Sep 25, 2019

jameswoodley commented Sep 27, 2019

pmuens commented Oct 10, 2019

pmuens commented Oct 24, 2019 • edited

michelesr commented Dec 12, 2019

medikoo commented Dec 23, 2019

azizur commented Sep 15, 2019 •

edited

medikoo left a comment •

edited

medikoo Sep 18, 2019 •

edited

azizur Sep 20, 2019 •

edited

pmuens commented Oct 24, 2019 •

edited