Skip to content

Commit

Permalink
fix(eks): overly permissive trust policies (#25580)
Browse files Browse the repository at this point in the history
Backporting #25473

----

The *CreationRole* and the *default MastersRole* use the account root principal in their trust policy, which is overly permissive. Instead, use the specific lambda handler roles that need it, and remove the default masters role.

BREAKING CHANGE: A masters role is no longer provisioned by default. Use the `mastersRole` property to explicitly pass a role that needs cluster access. In addition, the creation role no longer allows any identity (with the appropriate `sts:AssumeRole` permissions) to assume it. 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
iliapolo authored May 16, 2023
1 parent 615ebcb commit 0251d9a
Show file tree
Hide file tree
Showing 128 changed files with 20,108 additions and 23,247 deletions.
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

0 comments on commit 0251d9a

Please sign in to comment.