aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.35k stars 3.76k forks source link

Secrets Manager: raises a security issue when adding rotationLambda #28406

Open asenousy opened 6 months ago

asenousy commented 6 months ago

Describe the bug

The following cloudformation guard rule fails https://docs.aws.amazon.com/controltower/latest/userguide/lambda-rules.html#ct-lambda-pr-2-description

This is due to the fact that it is missing a SourceAccount in the service principal

A resource policy for rotation lambda is created here and this causes the cfn guard rule to fail.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-secretsmanager/lib/rotation-schedule.ts#L107

Expected Behavior

I should be allowed to add or override the service principal created here, to address cfn guard rule failing

Current Behavior

I have no way to address the cloudformation guard rule, created due to this line

Reproduction Steps

add a rotation lambda to secrets manager, and run cdk cfn guard validator

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.104.0

Framework Version

No response

Node.js Version

v16.20.0

OS

mac

Language

TypeScript

Language Version

No response

Other information

No response

pahud commented 6 months ago

OK I can see the violation messaegs now.

export class DemoStack extends MyStack {
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);

    // get a dummy func
    const fn = getLambdaFunction(this);
    const secret = new secrets.Secret(this, 'Secret');
    secret.addRotationSchedule('Schedule', {
      rotationLambda: fn,
    })
  }
}

And the violation report:

(Violations)

lambda_function_public_access_prohibited_check (1 occurrences)

  Occurrences:

    - Construct Path: DemoStack2/Func/InvokeN0--a2GKfZP0JmDqDE--Vhhu6+A0TUv3NyNbk4YM+FKNc=
    - Template Path: DemoStack2.template.json
    - Creation Stack:
        └──  DemoStack2 (DemoStack2)
             │ Construct: aws-cdk-lib.Stack
             │ Library Version: 2.113.0
             │ Location: Run with '--debug' to include location info
             └──  Func (DemoStack2/Func)
                  │ Construct: aws-cdk-lib.aws_lambda.Function
                  │ Library Version: 2.113.0
                  │ Location: Run with '--debug' to include location info
                  └──  InvokeN0--a2GKfZP0JmDqDE--Vhhu6+A0TUv3NyNbk4YM+FKNc= (DemoStack2/Func/InvokeN0--a2GKfZP0JmDqDE--Vhhu6+A0TUv3NyNbk4YM+FKNc=)
                       │ Construct: aws-cdk-lib.aws_lambda.CfnPermission
                       │ Library Version: 2.113.0
                       │ Location: Run with '--debug' to include location info
    - Resource ID: FuncInvokeN0a2GKfZP0JmDqDEVhhu6A0TUv3NyNbk4YMFKNcA632017C
    - Template Locations:
      > /Resources/FuncInvokeN0a2GKfZP0JmDqDEVhhu6A0TUv3NyNbk4YMFKNcA632017C/Properties
      > /Resources/FuncInvokeN0a2GKfZP0JmDqDEVhhu6A0TUv3NyNbk4YMFKNcA632017C/Properties
      > /Resources/FuncInvokeN0a2GKfZP0JmDqDEVhhu6A0TUv3NyNbk4YMFKNcA632017C/Properties
      > /Resources/FuncInvokeN0a2GKfZP0JmDqDEVhhu6A0TUv3NyNbk4YMFKNcA632017C/Properties

  Description: [CT.LAMBDA.PR.2]: Require AWS Lambda function policies to prohibit public access
  How to fix: [FIX]: When setting 'Principal' to '*', provide one of 'SourceAccount', 'SourceArn', or 'PrincipalOrgID'. When setting 'Principal' to a service principal (for example, s3.amazonaws.com), provide one of 'SourceAccount' or 'SourceArn'.
  Rule Metadata: 
        DocumentationUrl: https://github.com/cdklabs/cdk-validator-cfnguard#bundled-control-tower-rules

And the affected CFN resource

  "FuncInvokeN0a2GKfZP0JmDqDEVhhu6A0TUv3NyNbk4YMFKNcA632017C": {
   "Type": "AWS::Lambda::Permission",
   "Properties": {
    "Action": "lambda:InvokeFunction",
    "FunctionName": {
     "Fn::GetAtt": [
      "Func217E03A4",
      "Arn"
     ]
    },
    "Principal": "secretsmanager.amazonaws.com"
   },
   "Metadata": {
    "aws:cdk:path": "DemoStack2/Func/InvokeN0--a2GKfZP0JmDqDE--Vhhu6+A0TUv3NyNbk4YM+FKNc="
   }
  },

And due to this message:

When setting 'Principal' to a service principal (for example, s3.amazonaws.com), provide one of 'SourceAccount' or 'SourceArn'

It will need the SourceAccount or SourceArn with secretsmanager.amazonaws.com as the service principal.

And according to the document, the SourceAccount condition is optional but recommended. https://docs.aws.amazon.com/secretsmanager/latest/userguide/rotate-secrets_turn-on-for-other.html#rotate-secrets_turn-on-for-other_step3

Yes I think we should include that as it's recommended.

Maybe we should fix this with a tiny PR like

const grantee = new iam.ServicePrincipal('secretsmanager.amazonaws.com', {
    conditions: {
      'SourceAccount': Aws.ACCOUNT_ID,
    }
  });

const grant = props.rotationLambda.grantInvoke(grantee);