Lambda: Fix CloudWatch event rule permissions #558
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lambda permissions granted when it was added as an event rule target did not include "SourceArn" as required. This allowed any event rule to trigger the function, and also did not show as a trigger in the AWS Lambda console. Added a integration test to verify. BREAKING CHANGE To fix this, we needed to modify `IEventRuleTarget` to pass the ARN of the rule and a unique ID to the registered target in order to allow it to specify the Source ARN. This required fixing all existing event rule targets (which, so far would return a role to be assumed by CWE, so the source ARN was not required). Fixes #555
rix0rrr
reviewed
Aug 14, 2018
| @@ -254,7 +254,7 @@ export abstract class ProjectRef extends cdk.Construct implements events.IEventR | |||
| /** | |||
| * Allows using build projects as event rule targets. | |||
| */ | |||
| public get eventRuleTarget(): events.EventRuleTargetProps { | |||
| public asEventRuleTarget(_ruleArn: events.RuleArn, _ruleId: string): events.EventRuleTargetProps { | |||
There was a problem hiding this comment.
Maybe nicer to pass the Rule itself, no?
There was a problem hiding this comment.
There's a good chance we would want to extract IEventRuleTarget into a separate module to prevent cyclic references (which is what I am doing with bucket notifications), so I am trying to standardize on a low-level signature for event targets, and this is the same signature as IBucketNotificationDestination will have.
RomainMuller
approved these changes
Aug 14, 2018
But it did work, right? Irrespective of the security issue, is that not a failing of the console? |
|
@rix0rrr correct, the events would trigger Lambda. |
rix0rrr
added a commit
that referenced
this pull request
Aug 15, 2018
### Features * __@aws-cdk/cdk__: Tokens can now be transparently embedded into strings and encoded into JSON without losing their semantics. This makes it possible to treat late-bound (deploy-time) values as if they were regular strings ([@rix0rrr] in [#518](#518)). * __@aws-cdk/aws-s3__: add support for bucket notifications to Lambda, SNS, and SQS targets ([@eladb] in [#201](#201), [#560](#560), [#561](#561), [#564](#564)) * __@aws-cdk/cdk__: non-alphanumeric characters can now be used as construct identifiers ([@eladb] in [#556](#556)) * __@aws-cdk/aws-iam__: add support for `maxSessionDuration` for Roles ([@eladb] in [#545](#545)). ### Changes * __@aws-cdk/aws-lambda__ (_**BREAKING**_): most classes renamed to be shorter and more in line with official service naming (`Lambda` renamed to `Function` or ommitted) ([@eladb] in [#550](#550)) * __@aws-cdk/aws-codepipeline__ (_**BREAKING**_): move all CodePipeline actions from `@aws-cdk/aws-xxx-codepipeline` packages into the regular `@aws-cdk/aws-xxx` service packages ([@skinny85] in [#459](#459)). * __@aws-cdk/aws-custom-resources__ (_**BREAKING**_): package was removed, and the Custom Resource construct added to the __@aws-cdk/aws-cloudformation__ package ([@rix0rrr] in [#513](#513)) ### Fixes * __@aws-cdk/aws-lambda__: Lambdas that are triggered by CloudWatch Events now show up in the console, and can only be triggered the indicated Event Rule. _**BREAKING**_ for middleware writers (as this introduces an API change), but transparent to regular consumers ([@eladb] in [#558](#558)) * __@aws-cdk/aws-codecommit__: fix a bug where `pollForSourceChanges` could not be set to `false` ([@maciejwalkowiak] in [#534](#534)) * __aws-cdk__: don't fail if the `~/.aws/credentials` file is missing ([@RomainMuller] in [#541](#541)) * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support TemplateConfiguration ([@mindstorms6] in [#571](#571)). * __@aws-cdk/aws-cloudformation__: fix a bug in the CodePipeline actions to correctly support ParameterOverrides ([@mindstorms6] in [#574](#574)). ### Known Issues * `cdk init` will try to init a `git` repository and fail if no global `user.name` and `user.email` have been configured.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Lambda permissions granted when it was added as an event rule target did not include "SourceArn" as required. This allowed any event rule to trigger the function, and also did not show as a trigger in the AWS Lambda console.
Added a integration test to verify.
BREAKING CHANGE
To fix this, we needed to modify
IEventRuleTargetto pass the ARN of the rule and a unique ID to the registered target in order to allow it to specify the Source ARN. This required fixing all existing event rule targets (which, so far would return a role to be assumed by CWE, so the source ARN was not required).Fixes #555
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.