feat(eks): improve security using IRSA conditions #8084

vlesierse · 2020-05-19T18:57:32Z

Currently the ServiceAccountconstruct creates a role with no conditions to the trust relationship or assume role policy. Without this it is possible for other pods in the same namespace to assume the role. To tighten this security the conditions needs to be set.

Documentation: https://docs.aws.amazon.com/eks/latest/userguide/create-service-account-iam-policy-and-role.html#create-service-account-iam-role

Add condition to the policy document using a custom resource
Add unit tests
Add integration tests
Adjust README and remove warning

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

vlesierse · 2020-05-19T19:00:59Z

@eladb I've chosen to put the custom resource in the aws-eks module as it is currently specific for this use case. Having a patching construct in the aws-iam module didn't feel right. I've tried it in this branch: https://github.com/vlesierse/aws-cdk/tree/iam/policytemplate

aws-cdk-automation · 2020-05-19T19:43:58Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 60ab001
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-19T20:28:16Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 1f147cd
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Rarely there is a need to use a CloudFormation intrinsic in a JSON hash key. This is impossible since intrinsics are not strings. To circumvent that, CfnJson can be used to "delay" the rendition of the object using a simple custom resource which simply parses and reflects the input value as an attribute. Required in order to implement EKS IRSA (#8084) and other federated identity use cases.

eladb

This is what I meant when I proposed a more generic solution:
#8099

Let's wait for this to be merged and then this one is going to be easy...

vlesierse · 2020-05-20T12:54:50Z

Yep, this looks lot better. Probably the lack of knowledge on how the tokenisation works within CDK. I'll dive a bit deep to understand it. Meanwhile I'll do my adjustments when this lands on the master branch.

Rarely there is a need to use a CloudFormation intrinsic in a JSON hash key. This is impossible since intrinsics are not strings. To circumvent that, CfnJson can be used to "delay" the rendition of the object using a simple custom resource which simply parses and reflects the input value as an attribute. Required in order to implement EKS IRSA (#8084) and other federated identity use cases. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Pull request has been modified.

aws-cdk-automation · 2020-05-21T09:33:23Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ab744f1
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-21T10:11:17Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b076d70
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/@aws-cdk/aws-eks/lib/cluster.ts

packages/@aws-cdk/aws-eks/lib/irsa-conditions/external.ts

packages/@aws-cdk/aws-eks/lib/irsa-conditions/index.ts

packages/@aws-cdk/aws-eks/lib/service-account.ts

Pull request has been modified.

eladb

Minor comment. I think you can convert to a non-draft PR.

packages/@aws-cdk/aws-eks/lib/service-account.ts

eladb

sorry one more comment

eladb · 2020-05-21T19:23:18Z

packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts

@@ -198,7 +198,7 @@ export class ClusterResourceHandler extends ResourceHandler {
          Endpoint: cluster.endpoint,
          Arn: cluster.arn,
          CertificateAuthorityData: cluster.certificateAuthority?.data,
-          OpenIdConnectIssuerUrl: cluster.identity?.oidc?.issuer,
+          OpenIdConnectIssuerUrl: cluster.identity?.oidc?.issuer?.substring(8), // Strips off https:// from the issuer url


This is technically a breaking change. Perhaps the correct approach is to add another attribute OpenIdConnectIssuer which will return the value without the https:// prefix.

Ah yeah. Good one. I'll make the change.

Should I create the additional property on the Cluster construct as well?

aws-cdk-automation · 2020-05-21T19:37:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: d71c897
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-21T19:38:05Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 5e9e726
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-21T20:04:26Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 69a82b8
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Pull request has been modified.

aws-cdk-automation · 2020-05-21T21:35:03Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 1b249bf
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-05-21T21:35:24Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-05-21T22:25:08Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9315fba
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-05-21T22:25:31Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Rarely there is a need to use a CloudFormation intrinsic in a JSON hash key. This is impossible since intrinsics are not strings. To circumvent that, CfnJson can be used to "delay" the rendition of the object using a simple custom resource which simply parses and reflects the input value as an attribute. Required in order to implement EKS IRSA (aws#8084) and other federated identity use cases. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

pahud · 2020-05-27T06:02:52Z

packages/@aws-cdk/aws-eks/lib/cluster.ts

@@ -708,7 +723,7 @@ export class Cluster extends Resource implements ICluster {
   * @param id the id of this service account
   * @param options service account options
   */
-  public addServiceAccount(id: string, options: ServiceAccountOptions) {
+  public addServiceAccount(id: string, options: ServiceAccountOptions = { }) {


the options is still mandatory here and user will still need to run addServiceAccont(id, {}) if no options.

Maybe this?

public addServiceAccount(id: string, options?: ServiceAccountOptions = { }) {

The default value turns it into optional. The “?” Is not needed

Add conditions to Service Account role

60ab001

SomayaB assigned rix0rrr and eladb and unassigned rix0rrr May 19, 2020

vlesierse added 2 commits May 19, 2020 21:40

Fixed casing

1f147cd

Remove warning in README

499d97d

eladb mentioned this pull request May 20, 2020

feat(core): CfnJson enables intrinsics in hash keys #8099

Merged

eladb previously requested changes May 20, 2020

View reviewed changes

Prepare for CfnJson construct

476ffd3

vlesierse added 2 commits May 21, 2020 10:56

Merge branch 'master' into eks/irsa-conditions

d463550

Add CfnJson Contruct for IRSA conditions

ab744f1

vlesierse changed the title ~~feat(iam): improve security using IRSA conditions~~ feat(aws-eks): improve security using IRSA conditions May 21, 2020

Change unit tests

b076d70

eladb previously requested changes May 21, 2020

View reviewed changes

Changes after code review

5e9e726

eladb reviewed May 21, 2020

View reviewed changes

packages/@aws-cdk/aws-eks/lib/service-account.ts Show resolved Hide resolved

eladb changed the title ~~feat(aws-eks): improve security using IRSA conditions~~ feat(eks): improve security using IRSA conditions May 21, 2020

vlesierse marked this pull request as ready for review May 21, 2020 19:05

vlesierse added 2 commits May 21, 2020 21:13

Add comment with documentation reference

d71c897

Merge branch 'master' into eks/irsa-conditions

69a82b8

eladb previously requested changes May 21, 2020

View reviewed changes

Introduce OpenIdConnectIssuer non breaking change

1b249bf

eladb approved these changes May 21, 2020

View reviewed changes

Merge branch 'master' into eks/irsa-conditions

9315fba

mergify bot merged commit 35a01a0 into aws:master May 21, 2020

vlesierse mentioned this pull request May 26, 2020

chore(aws-eks): update README with correct service account syntax #8112

Merged

pahud reviewed May 27, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eks): improve security using IRSA conditions #8084

feat(eks): improve security using IRSA conditions #8084

vlesierse commented May 19, 2020 •

edited

Loading

vlesierse commented May 19, 2020

aws-cdk-automation commented May 19, 2020

aws-cdk-automation commented May 19, 2020

eladb left a comment

vlesierse commented May 20, 2020

aws-cdk-automation commented May 21, 2020

aws-cdk-automation commented May 21, 2020

eladb left a comment

eladb left a comment

eladb May 21, 2020

vlesierse May 21, 2020

vlesierse May 21, 2020

eladb May 21, 2020

aws-cdk-automation commented May 21, 2020

aws-cdk-automation commented May 21, 2020

aws-cdk-automation commented May 21, 2020

aws-cdk-automation commented May 21, 2020

mergify bot commented May 21, 2020

aws-cdk-automation commented May 21, 2020

mergify bot commented May 21, 2020

pahud May 27, 2020 •

edited

Loading

eladb May 27, 2020

feat(eks): improve security using IRSA conditions #8084

feat(eks): improve security using IRSA conditions #8084

Conversation

vlesierse commented May 19, 2020 • edited Loading

vlesierse commented May 19, 2020

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 19, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

vlesierse commented May 20, 2020

aws-cdk-automation commented May 21, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 21, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

eladb May 21, 2020

Choose a reason for hiding this comment

vlesierse May 21, 2020

Choose a reason for hiding this comment

vlesierse May 21, 2020

Choose a reason for hiding this comment

eladb May 21, 2020

Choose a reason for hiding this comment

aws-cdk-automation commented May 21, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 21, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 21, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 21, 2020

AWS CodeBuild CI Report

mergify bot commented May 21, 2020

aws-cdk-automation commented May 21, 2020

AWS CodeBuild CI Report

mergify bot commented May 21, 2020

pahud May 27, 2020 • edited Loading

Choose a reason for hiding this comment

eladb May 27, 2020

Choose a reason for hiding this comment

vlesierse commented May 19, 2020 •

edited

Loading

pahud May 27, 2020 •

edited

Loading