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 all 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
8 changes: 4 additions & 4 deletions design/aws-guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -326,9 +326,9 @@ export interface IFoo extends cdk.IConstruct, ISomething {
readonly connections: ec2.Connections;

// permission grants (adds statements to the principal's policy)
grant(principal?: iam.IPrincipal, ...actions: string[]): void;
grantFoo(principal?: iam.IPrincipal): void;
grantBar(principal?: iam.IPrincipal): void;
grant(grantee?: iam.IGrantable, ...actions: string[]): void;
grantFoo(grantee?: iam.IGrantable): void;
grantBar(grantee?: iam.IGrantable): void;

// resource policy (if applicable)
addToResourcePolicy(statement: iam.PolicyStatement): void;
Expand Down Expand Up @@ -364,7 +364,7 @@ export abstract class FooBase extends cdk.Construct implements IFoo {
public abstract export(): FooAttributes;

// grants can usually be shared
public grantYyy(principal?: iam.IPrincipal) {
public grantYyy(grantee?: iam.IGrantable) {
// ...
}

Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
"include": "dependencies/node-version"
},
"scripts": {
"pkglint": "tools/pkglint/bin/pkglint -f ."
"pkglint": "tools/pkglint/bin/pkglint -f .",
"build-all": "tsc -b"
},
"devDependencies": {
"@types/node": "8.10.40",
Expand Down
13 changes: 2 additions & 11 deletions packages/@aws-cdk/assets-docker/test/test.image-asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,22 +61,13 @@ export = {
":",
{ "Ref": "AWS::AccountId" },
":repository/",
{
"Fn::GetAtt": [
"ImageAdoptRepositoryE1E84E35",
"RepositoryName"
]
}
{ "Fn::GetAtt": [ "ImageAdoptRepositoryE1E84E35", "RepositoryName" ] }
]
]
}
},
{
"Action": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense that we lost these permissions?

Copy link
Contributor Author

@rix0rrr rix0rrr Apr 3, 2019

Choose a reason for hiding this comment

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

Yes, the user pulling from ECR doesn't need to create logs (they should have never have been part of that grant in the first place).

"ecr:GetAuthorizationToken",
"logs:CreateLogStream",
"logs:PutLogEvents"
],
"Action": "ecr:GetAuthorizationToken",
"Effect": "Allow",
"Resource": "*"
}
Expand Down
10 changes: 5 additions & 5 deletions packages/@aws-cdk/assets/lib/asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export interface GenericAssetProps {
* A list of principals that should be able to read this asset from S3.
* You can use `asset.grantRead(principal)` to grant read permissions later.
*/
readonly readers?: iam.IPrincipal[];
readonly readers?: iam.IGrantable[];
}

/**
Expand Down Expand Up @@ -171,12 +171,12 @@ export class Asset extends cdk.Construct {
/**
* Grants read permissions to the principal on the asset's S3 object.
*/
public grantRead(principal?: iam.IPrincipal) {
public grantRead(grantee: iam.IGrantable) {
// We give permissions on all files with the same prefix. Presumably
// different versions of the same file will have the same prefix
// and we don't want to accidentally revoke permission on old versions
// when deploying a new version.
this.bucket.grantRead(principal, `${this.s3Prefix}*`);
this.bucket.grantRead(grantee, `${this.s3Prefix}*`);
}
}

Expand All @@ -190,7 +190,7 @@ export interface FileAssetProps {
* A list of principals that should be able to read this file asset from S3.
* You can use `asset.grantRead(principal)` to grant read permissions later.
*/
readonly readers?: iam.IPrincipal[];
readonly readers?: iam.IGrantable[];
}

/**
Expand All @@ -212,7 +212,7 @@ export interface ZipDirectoryAssetProps {
* A list of principals that should be able to read this ZIP file from S3.
* You can use `asset.grantRead(principal)` to grant read permissions later.
*/
readonly readers?: iam.IPrincipal[];
readonly readers?: iam.IGrantable[];
}

/**
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 grantee 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(grantee: iam.IGrantable): iam.Grant {
return iam.Grant.addToPrincipal({
grantee,
actions: ['cloudwatch:PutMetricData'],
resourceArns: ['*']
});
}

public readonly dimensions?: DimensionHash;
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-codebuild/lib/artifacts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export class S3BucketBuildArtifacts extends BuildArtifacts {
* @internal
*/
public _bind(project: Project) {
this.props.bucket.grantReadWrite(project.role);
this.props.bucket.grantReadWrite(project);
}

protected toArtifactsProperty(): any {
Expand Down
11 changes: 9 additions & 2 deletions packages/@aws-cdk/aws-codebuild/lib/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const CODEPIPELINE_TYPE = 'CODEPIPELINE';
const S3_BUCKET_ENV = 'SCRIPT_S3_BUCKET';
const S3_KEY_ENV = 'SCRIPT_S3_KEY';

export interface IProject extends cdk.IConstruct, events.IEventRuleTarget {
export interface IProject extends cdk.IConstruct, events.IEventRuleTarget, iam.IGrantable {
/** The ARN of this Project. */
readonly projectArn: string;

Expand Down Expand Up @@ -156,13 +156,15 @@ export interface ProjectImportProps {
* use the {@link import} method.
*/
export abstract class ProjectBase extends cdk.Construct implements IProject {
public abstract readonly grantPrincipal: iam.IPrincipal;

/** The ARN of this Project. */
public abstract readonly projectArn: string;

/** The human-visible name of this Project. */
public abstract readonly projectName: string;

/** The IAM service Role of this Project. Undefined for imported Projects. */
/** The IAM service Role of this Project. */
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
public abstract readonly role?: iam.IRole;

/** A role used by CloudWatch events to trigger a build */
Expand Down Expand Up @@ -369,6 +371,7 @@ export abstract class ProjectBase extends cdk.Construct implements IProject {
}

class ImportedProject extends ProjectBase {
public readonly grantPrincipal: iam.IPrincipal;
public readonly projectArn: string;
public readonly projectName: string;
public readonly role?: iam.Role = undefined;
Expand All @@ -381,6 +384,7 @@ class ImportedProject extends ProjectBase {
resource: 'project',
resourceName: props.projectName,
});
this.grantPrincipal = new iam.ImportedResourcePrincipal({ resource: this });

this.projectName = props.projectName;
}
Expand Down Expand Up @@ -572,6 +576,8 @@ export class Project extends ProjectBase {
return new ImportedProject(scope, id, props);
}

public readonly grantPrincipal: iam.IPrincipal;

/**
* The IAM role for this project.
*/
Expand Down Expand Up @@ -603,6 +609,7 @@ export class Project extends ProjectBase {
this.role = props.role || new iam.Role(this, 'Role', {
assumedBy: new iam.ServicePrincipal('codebuild.amazonaws.com')
});
this.grantPrincipal = this.role;

let cache: CfnProject.ProjectCacheProperty | undefined;
if (props.cacheBucket) {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-codebuild/lib/source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export class S3BucketSource extends BuildSource {
* @internal
*/
public _bind(project: Project) {
this.bucket.grantRead(project.role);
this.bucket.grantRead(project);
}

protected toSourceProperty(): any {
Expand Down
16 changes: 8 additions & 8 deletions packages/@aws-cdk/aws-codedeploy/lib/lambda/deployment-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export class LambdaDeploymentGroup extends cdk.Construct implements ILambdaDeplo
throw new Error('A pre-hook function is already defined for this deployment group');
}
this.preHook = preHook;
this.grantPutLifecycleEventHookExecutionStatus(this.preHook.role);
this.grantPutLifecycleEventHookExecutionStatus(this.preHook);
this.preHook.grantInvoke(this.role);
}

Expand All @@ -208,7 +208,7 @@ export class LambdaDeploymentGroup extends cdk.Construct implements ILambdaDeplo
throw new Error('A post-hook function is already defined for this deployment group');
}
this.postHook = postHook;
this.grantPutLifecycleEventHookExecutionStatus(this.postHook.role);
this.grantPutLifecycleEventHookExecutionStatus(this.postHook);
this.postHook.grantInvoke(this.role);
}

Expand All @@ -217,12 +217,12 @@ export class LambdaDeploymentGroup extends cdk.Construct implements ILambdaDeplo
* on this deployment group resource.
* @param principal to grant permission to
*/
public grantPutLifecycleEventHookExecutionStatus(principal?: iam.IPrincipal): void {
if (principal) {
principal.addToPolicy(new iam.PolicyStatement()
.addResource(this.deploymentGroupArn)
.addAction('codedeploy:PutLifecycleEventHookExecutionStatus'));
}
public grantPutLifecycleEventHookExecutionStatus(grantee: iam.IGrantable): iam.Grant {
return iam.Grant.addToPrincipal({
grantee,
resourceArns: [this.deploymentGroupArn],
actions: ['codedeploy:PutLifecycleEventHookExecutionStatus'],
});
}

public export(): LambdaDeploymentGroupImportProps {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,9 @@ function setCodeBuildNeededPermissions(info: codepipeline.ActionBind, project: c

// allow the Project access to the Pipline's artifact Bucket
if (needsPipelineBucketWrite) {
info.pipeline.grantBucketReadWrite(project.role);
info.pipeline.grantBucketReadWrite(project);
} else {
info.pipeline.grantBucketRead(project.role);
info.pipeline.grantBucketRead(project);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,11 +317,11 @@ class PipelineDouble extends cdk.Construct implements codepipeline.IPipeline {
throw new Error('asEventRuleTarget() is unsupported in PipelineDouble');
}

public grantBucketRead(_identity?: iam.IPrincipal): void {
public grantBucketRead(_identity?: iam.IGrantable): iam.Grant {
throw new Error('grantBucketRead() is unsupported in PipelineDouble');
}

public grantBucketReadWrite(_identity?: iam.IPrincipal): void {
public grantBucketReadWrite(_identity?: iam.IGrantable): iam.Grant {
throw new Error('grantBucketReadWrite() is unsupported in PipelineDouble');
}
}
Expand Down Expand Up @@ -369,9 +369,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
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-codepipeline/lib/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,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.IGrantable): iam.Grant;

/**
* 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.IGrantable): iam.Grant;
}

/**
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 @@ -293,12 +293,12 @@ export class Pipeline extends cdk.Construct implements IPipeline {
return this.stages.length;
}

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

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

/**
Expand Down
Loading