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

fix(eks): overly permissive trust policies #25580

Merged
merged 8 commits into from
May 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 8 deletions packages/@aws-cdk/aws-eks/lib/cluster-resource-provider.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import * as path from 'path';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import * as lambda from '@aws-cdk/aws-lambda';
import { Duration, NestedStack, Stack } from '@aws-cdk/core';
import * as cr from '@aws-cdk/custom-resources';
Expand All @@ -15,10 +14,6 @@ const HANDLER_DIR = path.join(__dirname, 'cluster-resource-handler');
const HANDLER_RUNTIME = lambda.Runtime.NODEJS_14_X;

export interface ClusterResourceProviderProps {
/**
* The IAM role to assume in order to interact with the cluster.
*/
readonly adminRole: iam.IRole;

/**
* The VPC to provision the functions in.
Expand Down Expand Up @@ -118,9 +113,6 @@ export class ClusterResourceProvider extends NestedStack {
vpcSubnets: props.subnets ? { subnets: props.subnets } : undefined,
securityGroups: props.securityGroup ? [props.securityGroup] : undefined,
});

props.adminRole.grant(onEvent.role!, 'sts:AssumeRole');
props.adminRole.grant(isComplete.role!, 'sts:AssumeRole');
}

/**
Expand Down
11 changes: 6 additions & 5 deletions packages/@aws-cdk/aws-eks/lib/cluster-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,16 @@ export class ClusterResource extends CoreConstruct {
throw new Error('"roleArn" is required');
}

this.adminRole = this.createAdminRole(props);

const provider = ClusterResourceProvider.getOrCreate(this, {
adminRole: this.adminRole,
subnets: props.subnets,
vpc: props.vpc,
environment: props.environment,
onEventLayer: props.onEventLayer,
securityGroup: props.clusterHandlerSecurityGroup,
});

this.adminRole = this.createAdminRole(provider, props);

const resource = new CustomResource(this, 'Resource', {
resourceType: CLUSTER_RESOURCE_TYPE,
serviceToken: provider.serviceToken,
Expand Down Expand Up @@ -117,13 +116,15 @@ export class ClusterResource extends CoreConstruct {
this.attrOpenIdConnectIssuer = Token.asString(resource.getAtt('OpenIdConnectIssuer'));
}

private createAdminRole(props: ClusterResourceProps) {
private createAdminRole(provider: ClusterResourceProvider, props: ClusterResourceProps) {
const stack = Stack.of(this);

// the role used to create the cluster. this becomes the administrator role
// of the cluster.
const creationRole = new iam.Role(this, 'CreationRole', {
assumedBy: new iam.AccountRootPrincipal(),
// the role would be assumed by the provider handlers, as they are the ones making
// the requests.
assumedBy: new iam.CompositePrincipal(provider.provider.onEventHandler.role!, provider.provider.isCompleteHandler!.role!),
});

// the CreateCluster API will allow the cluster to assume this role, so we
Expand Down
39 changes: 24 additions & 15 deletions packages/@aws-cdk/aws-eks/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1325,7 +1325,14 @@ export class Cluster extends ClusterBase {
this.prune = props.prune ?? true;
this.vpc = props.vpc || new ec2.Vpc(this, 'DefaultVpc');
this.version = props.version;
this.kubectlLambdaRole = props.kubectlLambdaRole ? props.kubectlLambdaRole : undefined;

// since this lambda role needs to be added to the trust policy of the creation role,
// we must create it in this scope (instead of the KubectlProvider nested stack) to avoid
// a circular dependency.
this.kubectlLambdaRole = props.kubectlLambdaRole ? props.kubectlLambdaRole : new iam.Role(this, 'KubectlHandlerRole', {
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole')],
});

this.tagSubnets();

Expand Down Expand Up @@ -1479,6 +1486,11 @@ export class Cluster extends ClusterBase {
// and configured to allow connections from itself.
this.kubectlSecurityGroup = this.clusterSecurityGroup;

this.adminRole.assumeRolePolicy?.addStatements(new iam.PolicyStatement({
actions: ['sts:AssumeRole'],
principals: [this.kubectlLambdaRole],
}));

// use the cluster creation role to issue kubectl commands against the cluster because when the
// cluster is first created, that's the only role that has "system:masters" permissions
this.kubectlRole = this.adminRole;
Expand All @@ -1493,22 +1505,19 @@ export class Cluster extends ClusterBase {
new CfnOutput(this, 'ClusterName', { value: this.clusterName });
}

// if an explicit role is not configured, define a masters role that can
// be assumed by anyone in the account (with sts:AssumeRole permissions of
// course)
const mastersRole = props.mastersRole ?? new iam.Role(this, 'MastersRole', {
assumedBy: new iam.AccountRootPrincipal(),
});

// map the IAM role to the `system:masters` group.
this.awsAuth.addMastersRole(mastersRole);
// do not create a masters role if one is not provided. Trusting the accountRootPrincipal() is too permissive.
if (props.mastersRole) {
const mastersRole = props.mastersRole;

if (props.outputMastersRoleArn) {
new CfnOutput(this, 'MastersRoleArn', { value: mastersRole.roleArn });
}
// map the IAM role to the `system:masters` group.
this.awsAuth.addMastersRole(mastersRole);

commonCommandOptions.push(`--role-arn ${mastersRole.roleArn}`);
if (props.outputMastersRoleArn) {
new CfnOutput(this, 'MastersRoleArn', { value: mastersRole.roleArn });
}

commonCommandOptions.push(`--role-arn ${mastersRole.roleArn}`);
}
if (props.albController) {
this.albController = AlbController.create(this, { ...props.albController, cluster: this });
}
Expand All @@ -1524,7 +1533,7 @@ export class Cluster extends ClusterBase {
this.addNodegroupCapacity('DefaultCapacity', { instanceTypes: [instanceType], minSize: minCapacity }) : undefined;
}

const outputConfigCommand = props.outputConfigCommand ?? true;
const outputConfigCommand = (props.outputConfigCommand ?? true) && props.mastersRole;
if (outputConfigCommand) {
const postfix = commonCommandOptions.join(' ');
new CfnOutput(this, 'ConfigCommand', { value: `${updateConfigCommandPrefix} ${postfix}` });
Expand Down
1 change: 0 additions & 1 deletion packages/@aws-cdk/aws-eks/lib/fargate-profile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ export class FargateProfile extends CoreConstruct implements ITaggable {
super(scope, id);

const provider = ClusterResourceProvider.getOrCreate(this, {
adminRole: props.cluster.adminRole,
onEventLayer: props.cluster.onEventLayer,
});

Expand Down
6 changes: 6 additions & 0 deletions packages/@aws-cdk/aws-eks/lib/kubectl-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,12 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider {
resources: [cluster.clusterArn],
}));

// taken from the lambda default role logic.
// makes it easier for roles to be passed in.
if (handler.isBoundToVpc) {
handler.role?.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaVPCAccessExecutionRole'));
}

// For OCI helm chart authorization.
this.handlerRole.addManagedPolicy(
iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonEC2ContainerRegistryReadOnly'),
Expand Down
Loading