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.6k stars 3.9k forks source link

(logs): Log Group ARN has extra `:*` #18253

Closed laurelmay closed 6 months ago

laurelmay commented 2 years ago

What is the problem?

The logGroupArn attribute for a LogGroup L2 construct has an extra :* at the end of the ARN. While this matches the behavior of CloudFormation's Arn attribute for an AWS::Logs::LogGroup, it does not match the documented structure for a Log Group. This breaks integration with other services, such as the creation of an AWS::WAFv2::LoggingConfiguration, which requires the ARN of the Log Group without :*.

Reproduction Steps

Build the following constructs within a Stack. (Sorry for the length; building a Web ACL is noisy)

import * as wafv2 from "aws-cdk-lib/aws-wafv2";
import * as logs from "aws-cdk-lib/aws-logs";

const acl = new waf.CfnWebACL(this, "ACL", {
  scope: "REGIONAL",
  defaultAction: {
    allow: {},
  },
  visibilityConfig: {
    sampledRequestsEnabled: true,
    cloudWatchMetricsEnabled: true,
    metricName: "SampleACLMetric",
  },
  rules: [
    {
      name: "RuleWithAWSManagedRules",
      priority: 0,
      overrideAction: {
        none: {},
      },
      statement: {
        managedRuleGroupStatement: {
          vendorName: "AWS",
          name: "AWSManagedRulesCommonRuleSet",
          excludedRules: [],
        },
      },
      visibilityConfig: {
        sampledRequestsEnabled: true,
        cloudWatchMetricsEnabled: true,
        metricName: "SampleAWSManagedRulesMetric",
      },
    },
  ],
});

const aclLogGroup = new logs.LogGroup(this, "ACLLogs", {
  // WAFv2 Log Groups must begin with `aws-waf-logs`. Including the ACL's
  // ID hopefully prevents name conflicts.
  logGroupName: `aws-waf-logs-${acl.attrId}`,
});

const logConfig = new wafv2.CfnLoggingConfiguration(scope, "WebAclLogging", {
  logDestinationConfigs: [aclLogGroup.logGroupArn],
  resourceArn: acl.attrArn,
});

What did you expect to happen?

The AWS::WAFv2::LoggingConfiguration resource should create successfully, referencing the logGroupArn attribute directly.

The LogGroup construct should provide the ARN in the format specified by the documentation, including

What actually happened?

The AWS::WAFv2::LoggingConfiguration resource failed to create with the error:

Resource handler returned message: "Error reason: The ARN isn't valid. A valid ARN begins with arn: and includes other information separated by colons or slashes., field: LOG_DESTINATION, parameter: arn:aws:logs:us-east-1:1234657890:log-group:aws-waf-logs-foo:*

The LogGroup.logGroupArn had an :* that was not documented as part of the ARN according to most documentation for the ARN of a Log Group.

CDK CLI Version

2.3.0 (build beaa5b2)

Framework Version

No response

Node.js Version

v14.18.2

OS

Linux

Language

Typescript

Language Version

No response

Other information

It may be more viable to fix this in other L2 constructs that need a log group than it is to fix it in the LogGroup construct itself at this point (and it doesn't seem that L2 constructs for the Web ACL logging configuration exist yet).

A workaround is to construct the ARN manually (with adjustments if the Log Group is in another account/region):

// The `.logGroupArn` attribute of `LogGroup` contains a `:*` at the end,
// which causes conflicts with the Web ACL Logging Configuration
const logGroupArn = Stack.of(this).formatArn({
  arnFormat: ArnFormat.COLON_RESOURCE_NAME,
  service: "logs",
  resource: "log-group",
  resourceName: aclLogGroup.logGroupName,
});
peterwoodworth commented 2 years ago

I believe that this is intentional, since the arn comes directly from CloudFormation.

Here's where the arn is set https://github.com/aws/aws-cdk/blob/bfbdf64696a05a9a1b9dcad30904b17ff1a83f8c/packages/%40aws-cdk/aws-logs/lib/log-group.ts#L403-L417

I'm not sure what we would want to do here, since I'm not sure why CloudFormation includes this in the arn in the first place. @comcalvi what do you think?

comcalvi commented 2 years ago

This is likely a bug in WAFv2. The CFN property that causes the issue, LogDestinationConfigs, is rendered to this CFN when I synthesize the provided template:

          {
            "Fn::GetAtt": [
              "ACLLogs29F73701",
              "Arn"
            ]
          }
        ],

In the console, that log group has the :* at the end of their ARN. Since CloudFormation is retrieving the correct ARN, WAFv2 must not be handling the ARN parameter correctly. I'll contact the WAFv2 team.

Another workaround is to use Fn.split(':*') plus Fn.select(0) to get the ARN in the right format.

ryanotella commented 2 years ago

Workaround example in CDKv2

const logGroup = new LogGroup(this, 'LogGroup', {
  logGroupName: `aws-waf-logs-firewall`,
  retention: RetentionDays.SIX_MONTHS,
  removalPolicy: RemovalPolicy.DESTROY,
});

const logDestination = Fn.select(0, Fn.split('*', logGroup.logGroupArn));
BeatKO commented 2 years ago

Someone managed to solve it? i can't use the provided ARN, i get this error Access log destination must only contain characters a-z, A-Z, 0-9, '_', '-', '/', '.': 'IDIDIDIDIDIDF:*'

csumpter commented 2 years ago

@BeatKO

import * as cdk from "@aws-cdk/core";
import * as waf from "@aws-cdk/aws-wafv2";
import * as cwLogs from "@aws-cdk/aws-logs";

...

const loggingGroup = new cwLogs.LogGroup(this, "LogGroup", {
  // waf log group names must start with "aws-waf-logs-" or the stack will not deploy
  logGroupName: "aws-waf-logs-myname",
  retention: cwLogs.RetentionDays.ONE_MONTH,
  removalPolicy: cdk.RemovalPolicy.DESTROY,
});
new waf.CfnLoggingConfiguration(this, "LoggingConfiguration", {
  // the arn that comes back natively from the LogGroup has ":*" appended on the end of it, including it on the destination will cause a deploy time error
  logDestinationConfigs: [cdk.Fn.select(0, cdk.Fn.split(":*", loggingGroup.logGroupArn))],
  resourceArn: this.webAcl.attrArn,
});
lacteolus commented 2 years ago

It seems that workaround doesn't work anymore. We started getting error for newly created stacks.

Resource handler returned message: "Unable to deliver logs to the configured destination. You might need to grant log delivery permissions for the destination. If you're using S3 as your log destination, you might have exceeded your bucket limit.

Old stacks deployed before are updated without issues. It's hard to exact date or CDK version when it happened, but I guess it started a few days ago with CDK >2.12.0 Tried to switch back to using ARN:

Resource handler returned message: "Error reason: The ARN isn't valid. A valid ARN begins with arn: and includes other information separated by colons or slashes., field: LOG_DESTINATION, parameter

Can anyone confirm this issue even with workraround applied?

newair commented 2 years ago

It seems that workaround doesn't work anymore. We started getting error for newly created stacks.

Resource handler returned message: "Unable to deliver logs to the configured destination. You might need to grant log delivery permissions for the destination. If you're using S3 as your log destination, you might have exceeded your bucket limit.

Old stacks deployed before are updated without issues. It's hard to exact date or CDK version when it happened, but I guess it started a few days ago with CDK >2.12.0 Tried to switch back to using ARN:

Resource handler returned message: "Error reason: The ARN isn't valid. A valid ARN begins with arn: and includes other information separated by colons or slashes., field: LOG_DESTINATION, parameter

Can anyone confirm this issue even with workaround applied?

We faced the same issue. Started suddenly getting

Resource handler returned message: "Unable to deliver logs to the configured destination. You might need to grant log delivery permissions for the destination. If you're using S3 as your log destination, you might have exceeded your bucket limit

@lacteolus Did you find a solution for this?

Thanks !

aidbal commented 2 years ago

I just had the exact same issue.

We were tyring to deploy stack with StateMachine configured with logging but it kept failing with

Resource handler returned message: "The state machine IAM Role is not authorized to access the Log Destination
(Service: AWSStepFunctions; Status Code: 400; Error Code: AccessDeniedException;
Request ID: 1bcfb19b-43a2-4007-a3d4-9eb4b30e7a96; Proxy: null)"
(RequestToken: 4a54f63f-a4df-7112-c6e6-ae7f9fd45669, HandlerErrorCode: AccessDenied)

For CloudFormation stacks we are using L1 and L2 AWS CDK Constructs.

We create an L1 LogGroup with CfnLogGroup construct.

But we are using L2 StateMachine construct.

Since L2 StateMachine construct only accepts ILogGroup, we have to use either LogGroup.fromLogGroupArn or LogGroup.fromLogGroupName.

We were using it like this:

const l1LogGroup = new CfnLogGroup(this, "LogGroupL1")

const stateMachine = new StateMachine(this, "StateMachine", {
  ...
  logs: {
    destination: LogGroup.fromLogGroupArn(this, "StateMachineLogGroupL2", l1LogGroup.attrArn),
    level: LogLevel.ALL
  }
})

Turns out, CfnLogGroup.attrArn returns ARN with :* attached at the end (https://docs.aws.amazon.com/cdk/api/v1/docs/@aws-cdk_aws-logs.CfnLogGroup.html#attrarn ):

attrArn
Type: string
The ARN of the log group, such as arn:aws:logs:us-west-1:123456789012:log-group:/mystack-testgroup-12ABC1AB12A1:*.

But the problem arises when LogGroup.fromLogGroupArn also attaches :* at the end:

declare abstract class LogGroupBase extends Resource implements ILogGroup {
    /**
     * The ARN of this log group, with ':*' appended
     */
    abstract readonly logGroupArn: string;

Which is in my opinion wrong, in the AWS console, the LogGroup ARN has the :* attached, so I don't understand why LogGroup.fromLogGroupArn also appends one more :* at the end. (You should take resource ARN as it is in the console and create a construct, not do more magic and append some trivial values at the end)

So when we made this call:

LogGroup.fromLogGroupArn(this, "StateMachineLogGroupL2", l1LogGroup.attrArn)

The Log Group ARN passed to StateMachine had :*:* at the end. Which was the reason of the failed deployments.

I have fixed this issue by constructing L2 LogGroup construct from name:

LogGroup.fromLogGroupName(this, "StateMachineLogGroupL2", logGroup.logGroupName)

But in my opinion LogGroup.fromLogGroupArn should be fixed.

comcalvi commented 2 years ago

@aidbal the behavior you're describing is working as intended. In the past, fromLogGroupArn() would return the log group ARNs without any :* at the end, which would result in log groups having incorrect permissions. Unfortunately this is one of those cases where the L1 of one service and the L2 of another don't play nice, so the solution you described (creating the L2 from the L1, and passing that instead) is the intended behavior here. See this PR for details.

aidbal commented 2 years ago

Hi @comcalvi, thanks for linking me the PR.

I agree that there was a need to patch the bug which caused incorrect permissions.

I think that your patch created another bug (the one I mentioned) unintentionally.

And it's not about mixing L1 and L2, I can easily break your bug fix by only using L2 constructs (explained at the end)

Why the bug fix is not complete

Here you guys check if the arn has :* attached, if so, you replace it with empty string.

However, you don't put into the consideration that the ARN might be a token and so your regex might not find anything.

I think the better approach there would have been to check if it's a token. If so, don't append any :*. If not, search for :* and replace it with empty string, then append :* at the end.

I think this makes the most sense as the LogGroupArn in AWS Console shows up with :* at the end:

AWS Console LogGroup ARN

Meaning the user expects the ARN of the LogGroup to always be the one with :* attached in the end. Thus, any CloudFormation values (i.e. Tokens), referring to LogGroupArn, will always represent the value with :* at the end (at least to the customer's eyes). No need to append any additional values.

Breaking your bug fix with L2 only

If you don't fix this bug and say that it only affects it when L1 and L2 are mixed together, I can easily create L2 Log group and get the ARN from that:

const logGroupL2 = new LogGroup(this, "LogGroupL2")

const stateMachine = new StateMachine(this, "StateMachine", {
  ...
  logs: {
    destination: LogGroup.fromLogGroupArn(this, "LogGroupFromArn", logGroupL2.logGroupArn)
    level: LogLevel.ALL
  }
})

This results in the following CloudFormation code:

              "CloudWatchLogsLogGroup": {
                "LogGroupArn": {
                  "Fn::Join": [
                    "",
                    [
                      {
                        "Fn::GetAtt": [
                          "LogGroupL255E4F451",
                          "Arn"
                        ]
                      },
                      ":*"
                    ]
                  ]
                }
              }

No L1 construct being mixed in, but still breaks your bug fix.

alexandervandekleutab commented 2 years ago

What is the status of this bug? Is it still not possible to create an ACL via CDK with a cloudwatch log group?

stanmarsh2 commented 2 years ago

Resource handler returned message: "Unable to deliver logs to the configured destination. You might need to grant log delivery permissions for the destination. If you're using S3 as your log destination, you might have exceeded your bucket limit

@lacteolus Did you find a solution for this?

Yes, it's a different issue. AWS creates resource based policies for us, but it has length limit. Later after limit is exceeded we need to create it by ourselves or clean it up.

richard-cp commented 7 months ago

Did anyone find a solution to this?

I get the same error:

Resource handler returned message: "Error reason: The ARN isn't valid. A valid ARN begins with arn: and includes other information separated by colons or slashes., field: RESOURCE_ARN

No matter how I format the ARN.

I have tried the mentioned workaround [cdk.Fn.select(0, cdk.Fn.split(":*", loggingGroup.logGroupArn))] but get the same error.

In my case the log group already exists, and have tried specifying the ARN 6 different ways, hardcoding it to get around the L1 & L2 constructs "not playing nice" - as mentioned in a post above.

Example CDK snippet with the different ARNs attempted:

const loggingConfig = new wafv2.CfnLoggingConfiguration(
this,
`webacl-logging-config`,
  {
      resourceArn: webAcl.ref,
      logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:app-webacl-logs'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:app-webacl-logs:'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:app-webacl-logs:*'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:/app-webacl-logs'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:/app-webacl-logs:'],
      // logDestinationConfigs: ['arn:aws:logs:us-east-1:xxxxxxxxx:log-group:/app-webacl-logs:*'],
  },
);

I have also tried another approach of using an s3 bucket as the logDestinationConfigs, for this approach I use a Kinesis stream for log delivery to s3, the stream has the managed policy AWSWAFFullAccess on it role, and the log destination is configured as follows: logDestinationConfigs: [deliveryStream.attrArn] - However, this also throws the same error as the cloudwatch log group as above.

Not sure I understand what the problem is here - has anyone got a workaround that still works?

Update: I have resolved this. I was referencing the resourceArn: webAcl.ref incorrectly. The ref does not return the arn in CDK. You must instead reference resourceArn: webAcl.attrArn. For the logDestinationConfigs, I did not need to split or strip the * out. Referencing the cloudwatch log group like this logDestinationConfigs: [logGroup.logGroupArn], worked fine. Even with a mix of L1 & L2 constructs. (LogGroup L2, LoggingConfiguration L1)

ryanotella commented 7 months ago

Probably related to the Log Group name requirement?

https://docs.aws.amazon.com/waf/latest/developerguide/logging-cw-logs.html#logging-cw-logs-naming

arash-cid commented 7 months ago

This is so stupid.

The error is the Log group Name must start with aws-waf-logs-

F AWS, F Microsoft

pahud commented 7 months ago

Just bumped this issue to p1 and self-assigned. I will be looking into this issue and escalate to the team when necessary.

pahud commented 7 months ago

I am able to cdk deploy with the provided sample in the original post.

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

    const acl = new wafv2.CfnWebACL(this, "ACL", {
      scope: "REGIONAL",
      defaultAction: {
        allow: {},
      },
      visibilityConfig: {
        sampledRequestsEnabled: true,
        cloudWatchMetricsEnabled: true,
        metricName: "SampleACLMetric",
      },
      rules: [
        {
          name: "RuleWithAWSManagedRules",
          priority: 0,
          overrideAction: {
            none: {},
          },
          statement: {
            managedRuleGroupStatement: {
              vendorName: "AWS",
              name: "AWSManagedRulesCommonRuleSet",
              excludedRules: [],
            },
          },
          visibilityConfig: {
            sampledRequestsEnabled: true,
            cloudWatchMetricsEnabled: true,
            metricName: "SampleAWSManagedRulesMetric",
          },
        },
      ],
    });

    const aclLogGroup = new logs.LogGroup(this, "ACLLogs", {
      // WAFv2 Log Groups must begin with `aws-waf-logs`. Including the ACL's
      // ID hopefully prevents name conflicts.
      logGroupName: `aws-waf-logs-${acl.attrId}`,
    });

    const logConfig = new wafv2.CfnLoggingConfiguration(this, "WebAclLogging", {
      logDestinationConfigs: [aclLogGroup.logGroupArn],
      resourceArn: acl.attrArn,
    });

  }
}

The AWS::WAFv2::LoggingConfiguration resource was created with no error.

I assume this issue does not exist anymore.

Please comment if does.

% npx cdk --version 2.132.0 (build 9a51c89)

mo7ty commented 7 months ago

I assume this issue does not exist anymore.

Probably CDK might be already handling LogGroup's ARN with extra :*, or AWS::WAFv2::LoggingConfiguration accepting it, because the ARN of a LogGroup still carries it.

emanserav commented 6 months ago

hi @pahud, I confirm is also working for me

cdk version 
2.133.0 (build dcc1e75)

PS: I had a typo before in the WAFv2 Log Group name, I am mentioning this for others as well even if it was already mentioned in this thread at least a few times, please take note on: !!! WAFv2 CloudWatch LogGroup must begin with aws-waf-logs- !!!

pahud commented 6 months ago

As this issue doesn't seem to existing anymore. I am closing it now. Feel free to create a new issue if there's any concern.

github-actions[bot] commented 6 months ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

MyNameIsTakenOMG commented 2 months ago

Same here, I bumped into this issue today using L2 construct to create a log group for my cognito service LogConfiguration, and I got cloudformation yelled at me, saying that this :* is invalid.

Besides, I also found a open issue on Re: Post, CloudFormation regex validation error in Cognito::LogDeliveryConfiguration.

Unluckily, by the time I am writing, this issue still exists though