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

cdk nag suppression error in cdk pipelines #84

Closed
blakegreendev opened this issue Aug 16, 2024 · 9 comments · Fixed by #90
Closed

cdk nag suppression error in cdk pipelines #84

blakegreendev opened this issue Aug 16, 2024 · 9 comments · Fixed by #90
Assignees
Labels
bug Something isn't working

Comments

@blakegreendev
Copy link

Describe the bug

Error: Suppression path "/controlplane-stack/AWS679f53fac002430cb0da5b1234567/ServiceRole/Resource" did not match any resource. This can occur when a resource does not exist or if a suppression is applied before a resource is created.

Expected Behavior

sbt control plane deploys in a stack from cdk pipelines

Current Behavior

when i run cdk synth it will throw an error. when sbt resources are commented out, it will synth successfully.

Reproduction Steps

none

Possible Solution

remove cdk nag and let users of SBT apply nag rules to the resources deployed from the construct

Additional Information/Context

this is a known issue and i've followed these instructions as well:
https://constructs.dev/packages/cdk-nag/v/2.27.136?lang=typescript#suppressing-awscdklibpipelines-violations

cdklabs/cdk-nag#1726

CDK CLI Version

2.151.0

Framework Version

No response

Node.js Version

18

OS

macos

Language

TypeScript

Language Version

No response

Other information

No response

@blakegreendev blakegreendev added the bug Something isn't working label Aug 16, 2024
@suhussai suhussai self-assigned this Aug 16, 2024
@suhussai
Copy link
Contributor

Thanks for reporting this. Can you share steps to reproduce this?

I've tried synthesizing both control-plane and app-plane constructs, but couldn't find the AWS679f53fac002430cb0da5b1234567 resource in either of the resulting CFN templates.

@blakegreendev
Copy link
Author

blakegreendev commented Aug 19, 2024

Hi, @suhussai thanks for taking a look, the problem is when you add any sbt resources in stack that's deployed in a stage from a cdk pipeline. Since SBT is using cdk nag under the hood of the construct, it appears the cdk pipelines can't synth the resources because it errors out due to the error in the nag rule.

This is very limiting especially for multi-account and multi environment deployments.

// ./lib/cdktest-stack.ts
import * as cdk from "aws-cdk-lib";
import { Construct } from "constructs";
import * as sbt from '@cdklabs/sbt-aws';

export class CdktestStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const cognitoAuth = new sbt.CognitoAuth(this, 'CognitoAuth', {
      setAPIGWScopes: false,
    });

    const controlPlane = new sbt.ControlPlane(this, 'ControlPlane', {
      auth: cognitoAuth,
      systemAdminEmail: 'ENTER YOUR EMAIL HERE',
    });
  }
}

// ./lib/cdktest-stage.ts
import * as cdk from "aws-cdk-lib";
import { Construct } from "constructs";
import { CdktestStack } from "./cdktest-stack";

export class CdkTestStage extends cdk.Stage {
  constructor(scope: Construct, id: string, props: cdk.StageProps) {
    super(scope, id, props);
    new CdktestStack(this, "cdk-test-stack");
  }
}

// ./lib/pipeline.ts
import * as cdk from "aws-cdk-lib";
import { Construct } from "constructs";
import * as pipeline from "aws-cdk-lib/pipelines";
import { CdkTestStage } from "./cdktest-stage";

export class PipelineStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props: cdk.StackProps) {
    super(scope, id, props);

    const synthBuildStep = new pipeline.CodeBuildStep("Synth", {
      input: sourceCodeCommit,
      installCommands: ["npm install -g aws-cdk"],
      commands: [],
      primaryOutputDirectory: "pipeline/cdk.out",
    });

    //Pipeline de base
    const pipelineInfra = new pipeline.CodePipeline(this, "pipelineInfra", {
      synth: synthBuildStep,
    });

    pipelineInfra.addStage(new CdkTestStage(this, "cdk-test-stage", props));

    pipelineInfra.buildPipeline();
  }
}

// bin/main.ts
#!/usr/bin/env node
import "source-map-support/register";
import * as cdk from "aws-cdk-lib";
import { AwsSolutionsChecks } from "cdk-nag";
import { PipelineStack } from "../lib/pipeline";
const app = new cdk.App();
new PipelineStack(app, "PipelineStack", {
  env: {
    region: "us-east-1",
    account: "111111111",
  },
});
cdk.Aspects.of(app).add(new AwsSolutionsChecks());

@suhussai
Copy link
Contributor

Thanks for listing out the steps.

When I synth the cdk app you described above, I got a similar error, but it was referring to a different resource:

Error: Suppression path "/cdk-test-stage-cdk-test-stack/AWS679f53fac002430cb0da5b7982bd2287/ServiceRole/Resource" did not match any resource. This can occur when a resource does not exist or if a suppression is applied before a resource is created.
    at .../node_modules/cdk-nag/src/nag-suppressions.ts:115:15
    at Array.forEach (<anonymous>)
    at Function.addResourceSuppressionsByPath (.../node_modules/cdk-nag/src/nag-suppressions.ts:98:15)
    at new CognitoAuth (.../src/control-plane/auth/cognito-auth.ts:482:23)
    at new CdktestStack (.../lib/cdktest-stack.ts:9:25)
    at new CdkTestStage (.../lib/cdktest-stage.ts:8:5)
    at new PipelineStack (.../lib/pipeline.ts:22:28)
    at Object.<anonymous> (.../bin/main.ts:20:1)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module.m._compile (.../node_modules/ts-node/src/index.ts:1618:23)

Can you paste the entire the error? I'd like to see where in the SBT code that NagSuppression exists that is causing the error in your example.

Assuming that's just a minor thing, I looked at different ways to solve this like enclosing the problematic NagSuppression in a condition to first check if the resource in question (AWS679f53fac002430cb0da5b7982bd2287) even exists:

    if (cdk.Stack.of(this).node.tryFindChild('AWS679f53fac002430cb0da5b7982bd2287')) {
      // https://github.com/aws/aws-cdk/issues/23204
      NagSuppressions.addResourceSuppressionsByPath(
        cdk.Stack.of(this),
        [
          `/${cdk.Stack.of(this).stackName}/AWS679f53fac002430cb0da5b7982bd2287/ServiceRole/Resource`,
          `/${cdk.Stack.of(this).stackName}/AWS679f53fac002430cb0da5b7982bd2287/Resource`,
        ],
        [
          {
            id: 'AwsSolutions-IAM4',
            reason: 'Suppress usage of AWSLambdaBasicExecutionRole.',
            appliesTo: [
              'Policy::arn:<AWS::Partition>:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole',
            ],
          },
          {
            id: 'AwsSolutions-L1',
            reason: 'NODEJS 18 is the version used in the official quickstart CFN template.',
          },
        ]
      );
    }

Unfortunately, that still didn't work, which is confusing because the NagSuppression shouldn't apply unless that resource exists, but for some reason it does apply, but then it throws an error saying that the related ServiceRole/Resource resource does not exist.

I also tried upgrading the cdk-nag and cdk versions to make sure they weren't the issue, but that didn't help.

Given this, I think a simple (but not ideal) solution that should fix this is to use a flag to disable the problematic NagSuppression. It can be a boolean called runAsStage and if it is set, we skip creating that specific NagSuppression in the cognito-auth.ts file, as all of the other ones work fine.

@tobuck-aws , do you have any thoughts here?

@blakegreendev
Copy link
Author

@suhussai thanks for looking into this further. I got the same resource but I redacted it in case it was a sensitive ID or something. Sorry about that, I should have mentioned that before.

I think that is just the tip of the iceberg and if you skip that NagSuppression, there will be several others behind it. That's just my assumption based on how CDK pipelines synthesizes the resources but I could be wrong.

Is there a reason the SBT construct needs to use cdk-nag in the first place? As a consumer of the construct, wouldn't it make more sense for me to apply the checks at the various points of the CDK app and resources that are provided by the constructs I'm using? In other words, I just want the resources from the construct, and then let me apply the NagSuppressions at the CDK app level. I'm not sure if that makes sense but it would be nice to have more control than the suppression rules being abstracted away as well.

@suhussai
Copy link
Contributor

I think that is just the tip of the iceberg and if you skip that NagSuppression, there will be several others behind it. That's just my assumption based on how CDK pipelines synthesizes the resources but I could be wrong.

I could be mistaken, but when I removed the NagSuppression here:

NagSuppressions.addResourceSuppressionsByPath(
cdk.Stack.of(this),
[
`/${cdk.Stack.of(this).stackName}/AWS679f53fac002430cb0da5b7982bd2287/ServiceRole/Resource`,
`/${cdk.Stack.of(this).stackName}/AWS679f53fac002430cb0da5b7982bd2287/Resource`,
],
[
{
id: 'AwsSolutions-IAM4',
reason: 'Suppress usage of AWSLambdaBasicExecutionRole.',
appliesTo: [
'Policy::arn:<AWS::Partition>:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole',
],
},
{
id: 'AwsSolutions-L1',
reason: 'NODEJS 18 is the version used in the official quickstart CFN template.',
},
]
);

I was able to successfully synth the template you sent. Of course, cdk-nag reported several other findings, but it didn't fail with the same error and the findings it showed were related to resources created as part of the template, and not SBT.

Is there a reason the SBT construct needs to use cdk-nag in the first place? As a consumer of the construct, wouldn't it make more sense for me to apply the checks at the various points of the CDK app and resources that are provided by the constructs I'm using? In other words, I just want the resources from the construct, and then let me apply the NagSuppressions at the CDK app level. I'm not sure if that makes sense but it would be nice to have more control than the suppression rules being abstracted away as well.

We added cdk-nag to the project in order to ensure we were adhering to best practices as we build out the necessary functionality. As for the NagSuppression, they are there to suppress findings that we did not implement and provide documentation/justification for why it was not implemented.

@suhussai
Copy link
Contributor

I updated the NagSuppression as part of this PR #90 and when I tested it using the template you sent, the error did not show up.

I'll try and get it merged today. If you have a minute, can you confirm whether the fix worked for you when we've merged it in?

@suhussai
Copy link
Contributor

Auto-closed.

Reopening until we get confirmation that the issue has been resolved.

@suhussai suhussai reopened this Aug 30, 2024
@blakegreendev
Copy link
Author

@suhussai that worked! Thank you very much! This will be very helpful for multi-account deployments!

@suhussai
Copy link
Contributor

suhussai commented Sep 5, 2024

Thanks for the feedback and for closing the loop on this. Glad it worked out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants