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): CreationRole has a more excessive trust policy than it needs #25268

Closed
wants to merge 14 commits into from
Closed
10 changes: 1 addition & 9 deletions packages/aws-cdk-lib/aws-eks/lib/cluster-resource-provider.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,15 @@
import * as path from 'path';
import { Construct } from 'constructs';
import * as ec2 from '../../aws-ec2';
import * as iam from '../../aws-iam';
import * as lambda from '../../aws-lambda';
import { Duration, NestedStack, Stack } from '../../core';
import * as cr from '../../custom-resources';
import { NodeProxyAgentLayer } from '../../lambda-layer-node-proxy-agent';
import { Construct } from 'constructs';

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 @@ -114,9 +109,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
21 changes: 11 additions & 10 deletions packages/aws-cdk-lib/aws-eks/lib/cluster-resource.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { Construct } from 'constructs';
import { CLUSTER_RESOURCE_TYPE } from './cluster-resource-handler/consts';
import { ClusterResourceProvider } from './cluster-resource-provider';
import { CfnCluster } from './eks.generated';
import * as ec2 from '../../aws-ec2';
import * as iam from '../../aws-iam';
import * as kms from '../../aws-kms';
import * as lambda from '../../aws-lambda';
import { ArnComponents, CustomResource, Token, Stack, Lazy } from '../../core';
import { Construct } from 'constructs';
import { CLUSTER_RESOURCE_TYPE } from './cluster-resource-handler/consts';
import { ClusterResourceProvider } from './cluster-resource-provider';
import { CfnCluster } from './eks.generated';

export interface ClusterResourceProps {
readonly resourcesVpcConfig: CfnCluster.ResourcesVpcConfigProperty;
Expand All @@ -25,7 +25,7 @@ export interface ClusterResourceProps {
readonly onEventLayer?: lambda.ILayerVersion;
readonly clusterHandlerSecurityGroup?: ec2.ISecurityGroup;
readonly tags?: { [key: string]: string };
readonly logging?: { [key: string]: [ { [key: string]: any } ] };
readonly logging?: { [key: string]: [{ [key: string]: any }] };
}

/**
Expand Down Expand Up @@ -57,17 +57,16 @@ export class ClusterResource extends Construct {
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 @@ -113,13 +112,15 @@ export class ClusterResource extends Construct {
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
7 changes: 3 additions & 4 deletions packages/aws-cdk-lib/aws-eks/lib/fargate-profile.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import * as ec2 from '../../aws-ec2';
import * as iam from '../../aws-iam';
import { Annotations, CustomResource, ITaggable, Lazy, TagManager, TagType } from '../../core';
import { Construct } from 'constructs';
import { Cluster } from './cluster';
import { FARGATE_PROFILE_RESOURCE_TYPE } from './cluster-resource-handler/consts';
import { ClusterResourceProvider } from './cluster-resource-provider';
import * as ec2 from '../../aws-ec2';
import * as iam from '../../aws-iam';
import { Annotations, CustomResource, ITaggable, Lazy, TagManager, TagType } from '../../core';

/**
* Options for defining EKS Fargate Profiles.
Expand Down Expand Up @@ -144,7 +144,6 @@ export class FargateProfile extends Construct implements ITaggable {
super(scope, id);

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

Expand Down
34 changes: 28 additions & 6 deletions packages/aws-cdk-lib/aws-eks/lib/kubectl-provider.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import * as path from 'path';
import { Construct, IConstruct } from 'constructs';
import { ICluster, Cluster } from './cluster';
import * as iam from '../../aws-iam';
import * as lambda from '../../aws-lambda';
import { Duration, Stack, NestedStack, Names, CfnCondition, Fn, Aws } from '../../core';
import * as cr from '../../custom-resources';
import { AwsCliLayer } from '../../lambda-layer-awscli';
import { KubectlLayer } from '../../lambda-layer-kubectl';
import { Construct, IConstruct } from 'constructs';
import { ICluster, Cluster } from './cluster';

/**
* Properties for a KubectlProvider
Expand Down Expand Up @@ -123,6 +123,11 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider {

const cluster = props.cluster;

const handlerRole = cluster.kubectlLambdaRole ?? new iam.Role(scope, 'KubectlHandlerRole', {
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
managedPolicies: [iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole')],
});

if (!cluster.kubectlRole) {
throw new Error('"kubectlRole" is not defined, cannot issue kubectl commands against this cluster');
}
Expand All @@ -141,7 +146,7 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider {
description: 'onEvent handler for EKS kubectl resource provider',
memorySize,
environment: cluster.kubectlEnvironment,
role: cluster.kubectlLambdaRole ? cluster.kubectlLambdaRole : undefined,
role: handlerRole,

// defined only when using private access
vpc: cluster.kubectlPrivateSubnets ? cluster.vpc : undefined,
Expand All @@ -160,6 +165,10 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider {
resources: [cluster.clusterArn],
}));

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 All @@ -169,7 +178,7 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider {
* For OCI helm chart public ECR authorization. As ECR public is only available in `aws` partition,
* we conditionally attach this policy when the AWS partition is `aws`.
*/
const hasEcrPublicCondition = new CfnCondition(this, 'HasEcrPublic', {
const hasEcrPublicCondition = new CfnCondition(this.handlerRole.node.scope!, 'HasEcrPublic', {
expression: Fn.conditionEquals(Aws.PARTITION, 'aws'),
});

Expand All @@ -181,8 +190,21 @@ export class KubectlProvider extends NestedStack implements IKubectlProvider {

this.handlerRole.addManagedPolicy(iam.ManagedPolicy.fromManagedPolicyArn(this, 'conditionalPolicy', conditionalPolicy.managedPolicyArn));

// allow this handler to assume the kubectl role
cluster.kubectlRole.grant(this.handlerRole, 'sts:AssumeRole');
// the handler is going to run 'aws eks update-kubeconfig' with the
// kubectl role, so its needs to be able to assume it.
// there are two options for the kubectl role.

if (iam.Role.isRole(cluster.kubectlRole)) {
// managed role, we can add ourselves to its trust policy
cluster.kubectlRole.assumeRolePolicy?.addStatements(new iam.PolicyStatement({
actions: ['sts:AssumeRole'],
principals: [this.handlerRole],
}));
} else {
// imported role, we can only hope its trust policy will allow
// us to assume it.
cluster.kubectlRole.grant(this.handlerRole, 'sts:AssumeRole');
}

const provider = new cr.Provider(this, 'Provider', {
onEventHandler: handler,
Expand Down