-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
@@ -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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
// 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')) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
…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*
@@ -566,6 +568,11 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC | |||
return (principal as iam.ArnPrincipal).arn; | |||
} | |||
|
|||
if ('organizationId' in principal) { |
There was a problem hiding this comment.
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 🤮)
There was a problem hiding this comment.
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 :).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
…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*
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). |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
…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*
Hi! Upgrading from
Is there any super-easy fix that I am missing? Surprised to see my project break with a minor upgrade :( |
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? |
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:
All Submissions:
Adding new Unconventional Dependencies:
New Features
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