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

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

Open
asenousy opened this issue Dec 18, 2023 · 1 comment
Open

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

asenousy opened this issue Dec 18, 2023 · 1 comment
Labels
@aws-cdk/aws-cloudformation Related to AWS CloudFormation bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@asenousy
Copy link

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

@asenousy asenousy added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 18, 2023
@github-actions github-actions bot added the @aws-cdk/aws-cloudformation Related to AWS CloudFormation label Dec 18, 2023
@pahud pahud self-assigned this Dec 18, 2023
@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Dec 18, 2023
@pahud
Copy link
Contributor

pahud commented Dec 18, 2023

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);

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Dec 18, 2023
@pahud pahud removed their assignment Dec 18, 2023
@pahud pahud added needs-review p1 and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p2 labels Dec 18, 2023
@ConnorRobertson ConnorRobertson self-assigned this Jan 31, 2024
@pahud pahud added p2 and removed p1 labels Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudformation Related to AWS CloudFormation bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
4 participants