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

feat(lambda): grant function permissions to an AWS organization #19975

Merged
merged 24 commits into from
Jun 28, 2022

Conversation

kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Apr 19, 2022

Closes #19538, also fixes #20146. I combined them because they touch the same surface area and it would be too hairy to separate them out.

See lambda docs for this feature.

Introduces functionality to grant permissions to an organization in the following ways:

declare const  fn = new lambda.Function;

// grant to an organization
fn.grantInvoke(iam.OrganizationPrincipal('o-xxxxxxxxxx');

// grant to an account in an organization
fn.grantInvoke(iam.AccountPrincipal('123456789012').inOrganization('o-xxxxxxxxxx'));

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Apr 19, 2022

@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Apr 19, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 19, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team April 19, 2022 18:28
@kaizencc kaizencc self-assigned this Apr 19, 2022
packages/@aws-cdk/aws-lambda/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/function-base.ts Outdated Show resolved Hide resolved
@@ -566,6 +576,15 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC
return (principal as iam.ArnPrincipal).arn;
}

if (principal instanceof iam.OrganizationPrincipal) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these instanceof's okay? Reading the comment on line 551/561, "after v2, we can change these to instanceof" which makes me think that new additions can use instanceof directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually not true. That comment is a lie.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the "lie" then, and used duck-typing

runtime: lambda.Runtime.NODEJS_14_X,
});

// 3 different ways to configure the same permission
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update this integ test to use inOrganization if/when #20109 is merged

@kaizencc kaizencc requested a review from rix0rrr April 27, 2022 22:09
// Lambda does not actually accept an organization principal here.
// so the principal becomes '*' and the organization ID will be sent to
// the 'principalOrgID' property.
if (permission.principal.toString().startsWith('OrganizationPrincipal')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not do without this type test? Can we not parse the policyfragment conditions and detect the principal from there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better yet, doesn't validateCombinations return this value already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. It's redundant, but I had to fix a bug in isPrincipalWithConditions to make it happen.

mergify bot pushed a commit that referenced this pull request Apr 28, 2022
…20109)

Add a convenience method to ArnPrincipal. ArnPrincipal is extended by AccountPrincipal and AnyPrincipal, which are the only principals that could reasonably want to add a condition on organization.

```ts
new AccountPrincipal('123456789012').inOrganization('o-xxxxxxxxxx');
```

Related: #19975 (comment). With this method, the API in #19975 will look like:

```ts
fn.grantInvoke(new AccountPrincipal('123456789012').inOrganization('o-xxxxxxxxxx');
```  

Which is really slick!

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@kaizencc kaizencc requested a review from rix0rrr April 28, 2022 18:57
@github-actions github-actions bot added bug This issue is a bug. and removed feature-request A feature should be added or improved. labels Apr 29, 2022
@kaizencc kaizencc requested a review from a team April 29, 2022 17:49
@@ -566,6 +568,11 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC
return (principal as iam.ArnPrincipal).arn;
}

if ('organizationId' in principal) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels nasty, and should not be necessary? OrganizationPrincipals fragment looks like this:

  public get policyFragment(): PrincipalPolicyFragment {
    return new PrincipalPolicyFragment(
      { AWS: ['*'] },
      { StringEquals: { 'aws:PrincipalOrgID': this.organizationId } },
    );
  }

So it should already fall properly into the cases below, where we parse the output of policyFragment and parse the AWS key out?

(Honestly -- that should be the only thing we do. The duck typing here above is 🤮)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm down to move it out, but the case below has the comment

// The principal cannot have conditions and must have a single { AWS: [arn] } entry.

OrganizationPrincipal has the condition StringEquals, so it fails. @rix0rrr since you were the one who introduced that comment, can you explain why the principal cannot have conditions? maybe we can loosen the logic to allow for something like OrganizationPrincipal to be parsed, but until then, the duck typing stays :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    // Try a best-effort approach to support simple principals that are not any of the predefined
    // classes, but are simple enough that they will fit into the Permission model. Main target
    // here: imported Roles, Users, Groups.
    //
    // The principal cannot have conditions and must have a single { AWS: [arn] } entry.
    const json = principal.policyFragment.principalJson;
    if (Object.keys(principal.policyFragment.conditions).length === 0 && json.AWS) {
      if (typeof json.AWS === 'string') { return json.AWS; }
      if (Array.isArray(json.AWS) && json.AWS.length === 1 && typeof json.AWS[0] === 'string') {
        return json.AWS[0];
      }
    }

the parsing logic, for reference.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'd still prefer to see something like:

const stringEquals = matchSingleKey('StringEquals', principal.policyFragment.conditions));
if (stringEquals) {
  const orgId = matchSingleKey('aws:PrincipalOrgID', stringEquals);
  if (orgId) {
    return '*';
  }
}

Right?

What I'm trying to get at is that we look at the existing AWS principal and conditions, and parse and convert those to Lambda Permissions.

Instead of, what the code is historically doing, is looking at what classes you used and using "inside" knowledge of what those classes represent to generate the right output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done this for org principal. I wanted to refactor the others to follow the same approach, but I couldn't get it to work. The approach works for org principal because we can identify the org principal from its policy fragment. That isn't the case for something like ArnPrincipal and AccountPrincipal, since both their policy fragments have just an arn value.

}

private isPrincipalWithConditions(principal: iam.IPrincipal): principal is iam.PrincipalWithConditions {
return 'conditions' in principal;
return Object.keys(principal.policyFragment.conditions).length > 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. But watch out! The return type of this function is a type assertion, which is now no longer true! You need to fix the return type and callsites as well.

packages/@aws-cdk/aws-lambda/lib/permission.ts Outdated Show resolved Hide resolved
wphilipw pushed a commit to wphilipw/aws-cdk that referenced this pull request May 23, 2022
…ws#20109)

Add a convenience method to ArnPrincipal. ArnPrincipal is extended by AccountPrincipal and AnyPrincipal, which are the only principals that could reasonably want to add a condition on organization.

```ts
new AccountPrincipal('123456789012').inOrganization('o-xxxxxxxxxx');
```

Related: aws#19975 (comment). With this method, the API in aws#19975 will look like:

```ts
fn.grantInvoke(new AccountPrincipal('123456789012').inOrganization('o-xxxxxxxxxx');
```  

Which is really slick!

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@kaizencc kaizencc requested review from rix0rrr and a team May 31, 2022 18:02
@kaizencc kaizencc changed the base branch from v1-main to main June 3, 2022 14:52
@mergify
Copy link
Contributor

mergify bot commented Jun 28, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Jun 28, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: bbc7420
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 2566017 into main Jun 28, 2022
@mergify mergify bot deleted the conroy/lambda-perms branch June 28, 2022 08:56
@mergify
Copy link
Contributor

mergify bot commented Jun 28, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

daschaa pushed a commit to daschaa/aws-cdk that referenced this pull request Jul 9, 2022
…19975)

Closes aws#19538, also fixes aws#20146. I combined them because they touch the same surface area and it would be too hairy to separate them out.

See [lambda docs](https://docs.aws.amazon.com/lambda/latest/dg/access-control-resource-based.html#permissions-resource-xorginvoke) for this feature.

Introduces functionality to grant permissions to an organization in the following ways:

```ts
declare const  fn = new lambda.Function;

// grant to an organization
fn.grantInvoke(iam.OrganizationPrincipal('o-xxxxxxxxxx');

// grant to an account in an organization
fn.grantInvoke(iam.AccountPrincipal('123456789012').inOrganization('o-xxxxxxxxxx'));
```

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@albertodiazdorado
Copy link

Hi! Upgrading from 2.29.1 to 2.30.0 breaks my project:

Error: PrincipalWithConditions had unsupported conditions for Lambda permission statement: [{"operator":"ArnEquals","key":"aws:SourceArn"}]. Supported operator/condition pairs: [{"operator":"ArnLike","key":"aws:SourceArn"},{"operator":"StringEquals","key":"aws:SourceAccount"},{"operator":"StringEquals","key":"aws:PrincipalOrgID"}]

Is there any super-easy fix that I am missing? Surprised to see my project break with a minor upgrade :(

@albertodiazdorado
Copy link

Well, I fixed it -- just had to read the error message carefully.

I replaced

    const servicePrincipal = new ServicePrincipal("events.amazonaws.com", {
      conditions: {
        ArnEquals: {
          "aws:SourceArn": myRule.ruleArn
        }
      }
    });

by

    const servicePrincipal = new ServicePrincipal("events.amazonaws.com", {
      conditions: {
        ArnLike: {
          "aws:SourceArn": myRule.ruleArn
        }
      }
    });

Just out of curiosity: you don't follow semantic versioning?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p1
Projects
None yet
5 participants