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 13 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
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-codepipeline-api/lib/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,14 @@ export interface IPipeline extends cdk.IConstruct, events.IEventRuleTarget {
*
* @param identity the IAM Identity to grant the permissions to
*/
grantBucketRead(identity?: iam.IPrincipal): void;
grantBucketRead(identity?: iam.IPrincipal): iam.GrantResult;

/**
* Grants read & write permissions to the Pipeline's S3 Bucket to the given Identity.
*
* @param identity the IAM Identity to grant the permissions to
*/
grantBucketReadWrite(identity?: iam.IPrincipal): void;
grantBucketReadWrite(identity?: iam.IPrincipal): iam.GrantResult;
}

/**
Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,12 +292,12 @@ export class Pipeline extends cdk.Construct implements cpapi.IPipeline {
return this.stages.length;
}

public grantBucketRead(identity?: iam.IPrincipal): void {
this.artifactBucket.grantRead(identity);
public grantBucketRead(identity?: iam.IPrincipal): iam.GrantResult {
return this.artifactBucket.grantRead(identity);
}

public grantBucketReadWrite(identity?: iam.IPrincipal): void {
this.artifactBucket.grantReadWrite(identity);
public grantBucketReadWrite(identity?: iam.IPrincipal): iam.GrantResult {
return this.artifactBucket.grantReadWrite(identity);
}

/**
Expand Down
46 changes: 25 additions & 21 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 @@ -407,13 +408,16 @@ export class Table extends Construct {
* @param principal The principal (no-op if undefined)
* @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));
public grant(principal?: iam.IPrincipal, ...actions: string[]): iam.GrantResult {
return iam.Permissions.grant({
Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the impression that we wanted a specific API for resources with resource policies, no?

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));
return iam.Permissions.grant({
principal,
actions,
resourceArns: [this.tableStreamArn],
scope: this,
});
}

/**
Expand All @@ -437,7 +441,7 @@ export class Table extends Construct {
* @param principal The principal to grant access to
*/
public grantReadData(principal?: iam.IPrincipal) {
this.grant(principal, ...READ_DATA_ACTIONS);
return this.grant(principal, ...READ_DATA_ACTIONS);
}

/**
Expand All @@ -447,7 +451,7 @@ export class Table extends Construct {
* @param principal The principal to grant access to
*/
public grantStreamRead(principal?: iam.IPrincipal) {
this.grantStream(principal, ...READ_STREAM_DATA_ACTIONS);
return this.grantStream(principal, ...READ_STREAM_DATA_ACTIONS);
}

/**
Expand All @@ -456,7 +460,7 @@ export class Table extends Construct {
* @param principal The principal to grant access to
*/
public grantWriteData(principal?: iam.IPrincipal) {
this.grant(principal, ...WRITE_DATA_ACTIONS);
return this.grant(principal, ...WRITE_DATA_ACTIONS);
}

/**
Expand All @@ -466,15 +470,15 @@ export class Table extends Construct {
* @param principal The principal to grant access to
*/
public grantReadWriteData(principal?: iam.IPrincipal) {
this.grant(principal, ...READ_DATA_ACTIONS, ...WRITE_DATA_ACTIONS);
return this.grant(principal, ...READ_DATA_ACTIONS, ...WRITE_DATA_ACTIONS);
}

/**
* Permits all DynamoDB operations ("dynamodb:*") to an IAM principal.
* @param principal The principal to grant access to
*/
public grantFullAccess(principal?: iam.IPrincipal) {
this.grant(principal, 'dynamodb:*');
return this.grant(principal, 'dynamodb:*');
}

/**
Expand Down
49 changes: 26 additions & 23 deletions packages/@aws-cdk/aws-ecr/lib/repository-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,17 @@ export interface IRepository extends cdk.IConstruct {
/**
* Grant the given principal identity permissions to perform the actions on this repository
*/
grant(identity?: iam.IPrincipal, ...actions: string[]): void;
grant(identity?: iam.IPrincipal, ...actions: string[]): iam.GrantResult;

/**
* Grant the given identity permissions to pull images in this repository.
*/
grantPull(identity?: iam.IPrincipal): void;
grantPull(identity?: iam.IPrincipal): iam.GrantResult;

/**
* Grant the given identity permissions to pull and push images to this repository.
*/
grantPullPush(identity?: iam.IPrincipal): void;
grantPullPush(identity?: iam.IPrincipal): iam.GrantResult;

/**
* Defines an AWS CloudWatch event rule that can trigger a target when an image is pushed to this
Expand Down Expand Up @@ -206,38 +206,41 @@ 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[]) {
return 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");
public grantPull(principal?: iam.IPrincipal) {
const ret = this.grant(principal, "ecr:BatchCheckLayerAvailability", "ecr:GetDownloadUrlForLayer", "ecr:BatchGetImage");

iam.Permissions.tryGrantOnIdentity({
principal,
actions: ["ecr:GetAuthorizationToken"],
resourceArns: ['*'],
scope: this,
});

if (identity) {
identity.addToPolicy(new iam.PolicyStatement()
.addActions("ecr:GetAuthorizationToken", "logs:CreateLogStream", "logs:PutLogEvents")
.addAllResources());
}
return ret;
}

/**
* 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);
return 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
Loading