-
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
Changes from 16 commits
fca493b
34f5cae
d2122d2
71e6bd6
3b4311f
545dc73
1d89444
fb5a9d2
d4c2904
60022d7
b6769bb
da48f1d
4ad0b40
c26d3ad
b5f1e1b
2f64196
8afe9e7
0e2202f
d453215
48488eb
9b8f92e
c432d65
99d9d5a
bbc7420
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -345,8 +345,10 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC | |
return; | ||
} | ||
|
||
const principal = this.parsePermissionPrincipal(permission.principal); | ||
const { sourceAccount, sourceArn } = this.parseConditions(permission.principal) ?? {}; | ||
let principal = this.parsePermissionPrincipal(permission.principal); | ||
|
||
let { sourceArn, sourceAccount, principalOrgID } = this.validateConditionCombinations(permission.principal) ?? {}; | ||
|
||
const action = permission.action ?? 'lambda:InvokeFunction'; | ||
const scope = permission.scope ?? this; | ||
|
||
|
@@ -359,6 +361,7 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC | |
eventSourceToken: permission.eventSourceToken, | ||
sourceAccount: permission.sourceAccount ?? sourceAccount, | ||
sourceArn: permission.sourceArn ?? sourceArn, | ||
principalOrgId: permission.principalOrg?.organizationId ?? principalOrgID, | ||
functionUrlAuthType: permission.functionUrlAuthType, | ||
}); | ||
} | ||
|
@@ -548,7 +551,6 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC | |
private parsePermissionPrincipal(principal: iam.IPrincipal) { | ||
// Try some specific common classes first. | ||
// use duck-typing, not instance of | ||
// @deprecated: after v2, we can change these to 'instanceof' | ||
if ('wrapped' in principal) { | ||
// eslint-disable-next-line dot-notation | ||
principal = principal['wrapped']; | ||
|
@@ -566,6 +568,11 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC | |
return (principal as iam.ArnPrincipal).arn; | ||
} | ||
|
||
if ('organizationId' in principal) { | ||
// we will move the organization id to the `principalOrgId` property of `Permissions`. | ||
return '*'; | ||
} | ||
|
||
// 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. | ||
|
@@ -580,17 +587,56 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC | |
} | ||
|
||
throw new Error(`Invalid principal type for Lambda permission statement: ${principal.constructor.name}. ` + | ||
'Supported: AccountPrincipal, ArnPrincipal, ServicePrincipal'); | ||
'Supported: AccountPrincipal, ArnPrincipal, ServicePrincipal, OrganizationPrincipal'); | ||
} | ||
|
||
private validateConditionCombinations(principal: iam.IPrincipal): { | ||
sourceArn: string | undefined, | ||
sourceAccount: string | undefined, | ||
principalOrgID: string | undefined, | ||
} | undefined { | ||
const conditions = this.validateConditions(principal); | ||
|
||
if (!conditions) { return undefined; } | ||
|
||
const sourceArn = conditions.ArnLike ? conditions.ArnLike['aws:SourceArn'] : undefined; | ||
const sourceAccount = conditions.StringEquals ? conditions.StringEquals['aws:SourceAccount'] : undefined; | ||
const principalOrgID = conditions.StringEquals ? conditions.StringEquals['aws:PrincipalOrgID'] : undefined; | ||
|
||
// PrincipalOrgID cannot be combined with any other conditions | ||
if (principalOrgID && (sourceArn || sourceAccount)) { | ||
throw new Error('PrincipalWithConditions had unsupported condition combinations for Lambda permission statement: principalOrgID cannot be set with other conditions.'); | ||
} | ||
|
||
return { | ||
sourceArn, | ||
sourceAccount, | ||
principalOrgID, | ||
}; | ||
} | ||
|
||
private parseConditions(principal: iam.IPrincipal): { sourceAccount: string, sourceArn: string } | null { | ||
private validateConditions(principal: iam.IPrincipal): iam.Conditions | undefined { | ||
if (this.isPrincipalWithConditions(principal)) { | ||
const conditions: iam.Conditions = principal.policyFragment.conditions; | ||
const conditionPairs = flatMap( | ||
Object.entries(conditions), | ||
([operator, conditionObjs]) => Object.keys(conditionObjs as object).map(key => { return { operator, key }; }), | ||
); | ||
const supportedPrincipalConditions = [{ operator: 'ArnLike', key: 'aws:SourceArn' }, { operator: 'StringEquals', key: 'aws:SourceAccount' }]; | ||
|
||
// These are all the supported conditions. Some combinations are not supported, | ||
// like only 'aws:SourceArn' or 'aws:PrincipalOrgID' and 'aws:SourceAccount'. | ||
// These will be validated through `this.validateConditionCombinations`. | ||
const supportedPrincipalConditions = [{ | ||
operator: 'ArnLike', | ||
key: 'aws:SourceArn', | ||
}, | ||
{ | ||
operator: 'StringEquals', | ||
key: 'aws:SourceAccount', | ||
}, { | ||
operator: 'StringEquals', | ||
key: 'aws:PrincipalOrgID', | ||
}]; | ||
|
||
const unsupportedConditions = conditionPairs.filter( | ||
(condition) => !supportedPrincipalConditions.some( | ||
|
@@ -599,21 +645,18 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC | |
); | ||
|
||
if (unsupportedConditions.length == 0) { | ||
return { | ||
sourceAccount: conditions.StringEquals['aws:SourceAccount'], | ||
sourceArn: conditions.ArnLike['aws:SourceArn'], | ||
}; | ||
return conditions; | ||
} else { | ||
throw new Error(`PrincipalWithConditions had unsupported conditions for Lambda permission statement: ${JSON.stringify(unsupportedConditions)}. ` + | ||
`Supported operator/condition pairs: ${JSON.stringify(supportedPrincipalConditions)}`); | ||
} | ||
} else { | ||
return null; | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This is a bug to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
} | ||
|
||
|
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:So it should already fall properly into the cases below, where we parse the output of
policyFragment
and parse theAWS
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
OrganizationPrincipal
has the conditionStringEquals
, 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 likeOrganizationPrincipal
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.
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:
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
andAccountPrincipal
, since both their policy fragments have just anarn
value.