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(aws-iam): grants support non-identity principals #1623

Merged
merged 40 commits into from
Apr 3, 2019
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
19ad316
Introduce Role -> IIdentity -> IPrincipal
Jan 27, 2019
be0ec5d
feat(aws-iam): grants support non-identity principals
Jan 28, 2019
fcce210
Forgot to add new file
Jan 28, 2019
f3c7827
Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor
rix0rrr Feb 11, 2019
d53d794
Undo move of principals to their own file
rix0rrr Feb 11, 2019
a445780
Change grants API
rix0rrr Feb 12, 2019
a43bf24
Make key interface work with jsii
rix0rrr Feb 13, 2019
2a75620
Splat in the consumer
rix0rrr Feb 13, 2019
dd03daf
Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor
rix0rrr Feb 13, 2019
09cffbd
Can't have the same function in 2 interfaces in C#
rix0rrr Feb 13, 2019
295d698
Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor
rix0rrr Feb 13, 2019
554816d
Respect the refactoring
rix0rrr Feb 13, 2019
cf68f7d
Add awslint rule to force grant() methods to use helpers
rix0rrr Feb 27, 2019
df46221
WIP
rix0rrr Feb 27, 2019
8428937
Update API
rix0rrr Feb 28, 2019
81de979
Update ECS expectations
rix0rrr Feb 28, 2019
0d4ac85
Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor
Feb 28, 2019
28945ee
Make statics also return a Grant
Feb 28, 2019
c7391f4
Review comments
rix0rrr Mar 1, 2019
53f103b
Merge branch 'huijbers/iam-refactor' of github.com:awslabs/aws-cdk in…
rix0rrr Mar 1, 2019
2006891
Review comments
rix0rrr Mar 1, 2019
3323f08
Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor
rix0rrr Mar 1, 2019
b195ac3
IRole implementing both IConstruct and IIdentity leads to a C# build …
rix0rrr Mar 1, 2019
49f996e
Fix unused import
rix0rrr Mar 1, 2019
a65d805
awslint should also find indirect interface extensions
rix0rrr Mar 4, 2019
f2eab4e
Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor
rix0rrr Mar 20, 2019
77865f5
Fixes
rix0rrr Mar 20, 2019
0192f0f
Merge remote-tracking branch 'origin/master' into huijbers/iam-refactor
rix0rrr Apr 1, 2019
c9349f6
Make it build
rix0rrr Apr 1, 2019
929c6fb
Fix failing tests
rix0rrr Apr 2, 2019
753eb84
Make principal in grant methods nonoptional
rix0rrr Apr 2, 2019
c2ca705
Adding IGrantable (WIP)
rix0rrr Apr 2, 2019
e53474b
Adding IGrantable (WIP)
rix0rrr Apr 2, 2019
1b707b1
Merge branch 'huijbers/iam-refactor' of github.com:awslabs/aws-cdk in…
Apr 3, 2019
74bbf35
Finish introduction of IGrantable, rename Grant.withResource -> Grant…
rix0rrr Apr 3, 2019
71964e7
Rename grant methods to be more explicit
rix0rrr Apr 3, 2019
361a92e
Remove dynamodb global
rix0rrr Apr 3, 2019
3772ba4
Update IParameter
rix0rrr Apr 3, 2019
0d24421
Fix stray unrenamed call
rix0rrr Apr 3, 2019
b51c76d
Make sure JSON.stringify(principal) doesn't recurse indefinitely
rix0rrr Apr 3, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,10 @@ class RoleDouble extends iam.Role {
super(scope, id, props);
}

public addToPolicy(statement: iam.PolicyStatement) {
public addToPolicy(statement: iam.PolicyStatement): boolean {
super.addToPolicy(statement);
this.statements.push(statement);
return true;
}
}

Expand Down
14 changes: 7 additions & 7 deletions packages/@aws-cdk/aws-cloudwatch/lib/metric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,14 @@ export class Metric {
/**
* Grant permissions to the given identity to write metrics.
*
* @param identity The IAM identity to give permissions to.
* @param principal The IAM identity to give permissions to.
*/
public static grantPutMetricData(identity?: iam.IPrincipal) {
if (!identity) { return; }

identity.addToPolicy(new iam.PolicyStatement()
.addAllResources()
.addAction("cloudwatch:PutMetricData"));
public static grantPutMetricData(principal?: iam.IPrincipal) {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this return a Grant?

if (principal) {
principal.addToPolicy(new iam.PolicyStatement()
.addAction('cloudwatch:PutMetricData')
.addAllResources());
}
}

public readonly dimensions?: DimensionHash;
Expand Down
34 changes: 19 additions & 15 deletions packages/@aws-cdk/aws-dynamodb/lib/table.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import appscaling = require('@aws-cdk/aws-applicationautoscaling');
import iam = require('@aws-cdk/aws-iam');
import { PolicyStatement } from '@aws-cdk/aws-iam';
import cdk = require('@aws-cdk/cdk');
import { Construct, Token } from '@aws-cdk/cdk';
import { CfnTable } from './dynamodb.generated';
Expand Down Expand Up @@ -180,11 +181,11 @@ export class Table extends Construct {
*/
public static grantListStreams(principal?: iam.IPrincipal): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

should the awslint rule also be applied to static methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. One problem with the statics is that we won't have a scope to attach a warning to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not the end of the world

if (principal) {
principal.addToPolicy(new iam.PolicyStatement()
principal.addToPolicy(new PolicyStatement()
.addAction('dynamodb:ListStreams')
.addResource("*"));
.addAllResources());
}
}
}

public readonly tableArn: string;
public readonly tableName: string;
Expand Down Expand Up @@ -408,12 +409,15 @@ export class Table extends Construct {
* @param actions The set of actions to allow (i.e. "dynamodb:PutItem", "dynamodb:GetItem", ...)
*/
public grant(principal?: iam.IPrincipal, ...actions: string[]) {
if (!principal) {
return;
}
principal.addToPolicy(new iam.PolicyStatement()
.addResources(this.tableArn, new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString())
.addActions(...actions));
iam.Permissions.grant({
principal,
actions,
resourceArns: [
this.tableArn,
new cdk.Token(() => this.hasIndex ? `${this.tableArn}/index/*` : new cdk.Aws().noValue).toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

Rebase

],
scope: this,
});
}

/**
Expand All @@ -423,12 +427,12 @@ export class Table extends Construct {
* @param actions The set of actions to allow (i.e. "dynamodb:DescribeStream", "dynamodb:GetRecords", ...)
*/
public grantStream(principal?: iam.IPrincipal, ...actions: string[]) {
if (!principal) {
return;
}
principal.addToPolicy(new iam.PolicyStatement()
.addResource(this.tableStreamArn)
.addActions(...actions));
iam.Permissions.grant({
principal,
actions,
resourceArns: [this.tableStreamArn],
scope: this,
});
}

/**
Expand Down
44 changes: 23 additions & 21 deletions packages/@aws-cdk/aws-ecr/lib/repository-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,38 +206,40 @@ export abstract class RepositoryBase extends cdk.Construct implements IRepositor
/**
* Grant the given principal identity permissions to perform the actions on this repository
*/
public grant(identity?: iam.IPrincipal, ...actions: string[]) {
if (!identity) {
return;
}
identity.addToPolicy(new iam.PolicyStatement()
.addResource(this.repositoryArn)
.addActions(...actions));
public grant(principal?: iam.IPrincipal, ...actions: string[]) {
iam.Permissions.grant({
principal,
actions,
resourceArns: [this.repositoryArn],
resource: this,
});
}

/**
* Grant the given identity permissions to use the images in this repository
*/
public grantPull(identity?: iam.IPrincipal) {
this.grant(identity, "ecr:BatchCheckLayerAvailability", "ecr:GetDownloadUrlForLayer", "ecr:BatchGetImage");

if (identity) {
identity.addToPolicy(new iam.PolicyStatement()
.addActions("ecr:GetAuthorizationToken", "logs:CreateLogStream", "logs:PutLogEvents")
.addAllResources());
}
public grantPull(principal?: iam.IPrincipal) {
this.grant(principal, "ecr:BatchCheckLayerAvailability", "ecr:GetDownloadUrlForLayer", "ecr:BatchGetImage");

iam.Permissions.grant({
principal,
actions: ["ecr:GetAuthorizationToken", "logs:CreateLogStream", "logs:PutLogEvents"],
resourceArns: ['*'],
skipResourcePolicy: true,
scope: this,
});
}

/**
* Grant the given identity permissions to pull and push images to this repository.
*/
public grantPullPush(identity?: iam.IPrincipal) {
this.grantPull(identity);
this.grant(identity,
"ecr:PutImage",
"ecr:InitiateLayerUpload",
"ecr:UploadLayerPart",
"ecr:CompleteLayerUpload");
this.grantPull(identity);
this.grant(identity,
"ecr:PutImage",
"ecr:InitiateLayerUpload",
"ecr:UploadLayerPart",
"ecr:CompleteLayerUpload");
}
}

Expand Down
8 changes: 8 additions & 0 deletions packages/@aws-cdk/aws-iam/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ an `ExternalId` works like this:

[supplying an external ID](test/example.external-id.lit.ts)

### Principals vs Identities

When we say *Principal*, we mean an entity you grant permissions to. This
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
entity can be an AWS Service, a Role, or something more abstract such as "all
users in this account" or even "all users in this organization". An
*Identity* is an IAM representing a single IAM entity that can have
Copy link
Contributor

Choose a reason for hiding this comment

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

"is an IAM resource representing"

a policy attached, one of `Role`, `User`, or `Group`.

### IAM Principals

When defining policy statements as part of an AssumeRole policy or as part of a
Expand Down
129 changes: 129 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/grant.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import cdk = require('@aws-cdk/cdk');
import { PolicyStatement } from "./policy-document";
import { IPrincipal } from "./principals";

/**
* Properties for a grant operation
*/
export interface GrantOptions {
/**
* The principal to grant to
*
* @default No work is done
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
*/
principal: IPrincipal | undefined;

/**
* The actions to grant
*/
actions: string[];

/**
* The resource ARNs to grant to
*/
resourceArns: string[];

/**
* Adder to the resource policy
Copy link
Contributor

Choose a reason for hiding this comment

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

Rephrase, it's not an "adder" anymore

*
* Either 'scope' or 'resource' must be supplied.
*
* An error will be thrown if the policy could not be added to the principal,
* no resource is supplied given and `skipResourcePolicy` is false.
Copy link
Contributor

Choose a reason for hiding this comment

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

"supplied given"

Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda feels like it should be possible to supply only resource without resourceArns somehow. Maybe IResourceWithPolicy can have a property resourceArn which will be the canonic resource ARN to be used?

*/
resource?: IResourceWithPolicy;

/**
* When referring to the resource in a resource policy, use this as ARN.
*
* (Depending on the resource type, this needs to be '*' in a resource policy).
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
*
* @default Same as regular resource ARNs
*/
resourceSelfArns?: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this make more sense as an enum or boolean?


/**
* If we wanted to add to the resource policy but there is no resource, ignore the error.
*
* @default false
*/
skipResourcePolicy?: boolean;

/**
* Construct to report warnings on in case grant could not be registered
*
* @default resource
*/
scope?: cdk.IConstruct;
}

export class Permissions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Something feels wrong with this and I am not sure why. Can we talk? 📲

/**
* Helper function to implement grants.
*
* The pattern is the same every time. We try to add to the principal
* first, then add to the resource afterwards.
*/
public static grant(options: GrantOptions): GrantResult {
let addedToPrincipal = false;
let addedToResource = false;

const scope = options.scope || options.resource;
if (!scope) {
throw new Error(`Either 'scope' or 'resource' must be supplied.`);
}

// One-iteration loop to be able to skip to end of function easily
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with using an inner function then? 🤷🏻‍♂️ You can return from that whenever you please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although this is creative and nice, I rather we keep all our IAM code very simple and straightforward, so it will be dead easy to maintain and reason about.

do {
if (!options.principal) {
// tslint:disable-next-line:max-line-length
scope.node.addWarning(`Could not add grant for '${options.actions}' on '${options.resourceArns}' because the principal was not available. Add the permissions by hand.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

hell yeah!

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this should be a warning, but okay to start

break;
}

addedToPrincipal = options.principal.addToPolicy(new PolicyStatement()
.addActions(...options.actions)
.addResources(...options.resourceArns));

if (addedToPrincipal || options.skipResourcePolicy) { break; }

if (!options.resource) {
throw new Error('Could not add permissions to Principal without policy, and resource does not have policy either. Grant to a Role instead.');
}

options.resource.addToResourcePolicy(new PolicyStatement()
.addActions(...options.actions)
.addResources(...(options.resourceSelfArns || options.resourceArns))
.addPrincipal(options.principal));
addedToResource = true;

} while (false);

return { addedToPrincipal, addedToResource };
}
}

/**
* The result of the grant() operation
*/
export interface GrantResult {
/**
* The grant was added to the principal's policy
*/
addedToPrincipal: boolean;

/**
* The grant was added to the resource policy
*/
addedToResource: boolean;
}

/**
* A resource with a resource policy that can be added to
*/
export interface IResourceWithPolicy extends cdk.IConstruct {
/**
* Add a statement to the resource's resource policy
*/
addToResourcePolicy(statement: PolicyStatement): void;
}
18 changes: 9 additions & 9 deletions packages/@aws-cdk/aws-iam/lib/group.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { Construct } from '@aws-cdk/cdk';
import { CfnGroup } from './iam.generated';
import { IPrincipal, Policy } from './policy';
import { ArnPrincipal, PolicyPrincipal, PolicyStatement } from './policy-document';
import { IIdentity } from './identity-base';
import { Policy } from './policy';
import { ArnPrincipal, PolicyStatement, PrincipalPolicyFragment } from './policy-document';
import { User } from './user';
import { AttachedPolicies, undefinedIfEmpty } from './util';

Expand Down Expand Up @@ -34,7 +35,8 @@ export interface GroupProps {
path?: string;
}

export class Group extends Construct implements IPrincipal {
export class Group extends Construct implements IIdentity {
public readonly assumeRoleAction: string = 'sts:AssumeRole';
/**
* The runtime name of this group.
*/
Expand All @@ -45,10 +47,7 @@ export class Group extends Construct implements IPrincipal {
*/
public readonly groupArn: string;

/**
* An "AWS" policy principal that represents this group.
*/
public readonly principal: PolicyPrincipal;
public readonly policyFragment: PrincipalPolicyFragment;

private readonly managedPolicies: any[];
private readonly attachedPolicies = new AttachedPolicies();
Expand All @@ -67,7 +66,7 @@ export class Group extends Construct implements IPrincipal {

this.groupName = group.groupName;
this.groupArn = group.groupArn;
this.principal = new ArnPrincipal(this.groupArn);
this.policyFragment = new ArnPrincipal(this.groupArn).policyFragment;
}

/**
Expand Down Expand Up @@ -97,12 +96,13 @@ export class Group extends Construct implements IPrincipal {
/**
* Adds an IAM statement to the default policy.
*/
public addToPolicy(statement: PolicyStatement) {
public addToPolicy(statement: PolicyStatement): boolean {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
if (!this.defaultPolicy) {
this.defaultPolicy = new Policy(this, 'DefaultPolicy');
this.defaultPolicy.attachToGroup(this);
}

this.defaultPolicy.addStatement(statement);
return true;
}
}
20 changes: 20 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/identity-base.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { Policy } from "./policy";
import { IPrincipal } from "./principals";

/**
* A construct that represents an IAM principal, such as a user, group or role.
*/
export interface IIdentity extends IPrincipal {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
/**
* Attaches an inline policy to this principal.
* This is the same as calling `policy.addToXxx(principal)`.
* @param policy The policy resource to attach to this principal.
*/
attachInlinePolicy(policy: Policy): void;

/**
* Attaches a managed policy to this principal.
* @param arn The ARN of the managed policy
*/
attachManagedPolicy(arn: string): void;
}
3 changes: 3 additions & 0 deletions packages/@aws-cdk/aws-iam/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ export * from './policy';
export * from './user';
export * from './group';
export * from './lazy-role';
export * from './principals';
export * from './identity-base';
export * from './grant';

// AWS::IAM CloudFormation Resources:
export * from './iam.generated';
Loading