feat: Create construct for cross account role

[DavidLawes]

What does this change?

This change adds a new experimental construct: GuCrossAccountRoleExperimental.

The intention is to provide an abstraction to support the creation of cross account roles in accordance with (what we considered to be) best practice. Specifically, the role is scoped to a specific role ARN, tightly controlling exactly what resources can assume the cross account role.

Creating a construct for cross account roles should help in situations such as this.

How to test

-[x] Run the tests

How can we measure success?

Consumers can create and use a cross account role using the new construct.

Have we considered potential risks?

Hopefully the risks are low/minimal - we used an existing AWS cdk definition of a cross-account role to inspire this construct.

Checklist

I have listed any breaking changes, along with a migration path ¹ <-- I don't believe this is necessary because we're only adding a new (experimental) construct.

• I have updated the documentation as required for the described changes ²

Footnotes

1. Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs. ↩

2. If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid? ↩

[DavidLawes]

Please let me know if I should be adding documentation anywhere else!

[jacobwinch]

This is the correct place to add the docs, thanks!

It looks like there may be something wrong with the documentation setup, because I can't see the docs for some of the other experimental constructs either.

I think we might need to update https://github.com/guardian/cdk/blob/main/src/experimental/constructs/index.ts and/or https://github.com/guardian/cdk/blob/main/src/experimental/constructs/iam/index.ts to fix this - perhaps one for another PR unless it's simple!

[DavidLawes]

Thanks for pointing this out! I think it was relatively simple fix, I added the exports to constructs/index.ts and could then see the constructs when generating the typedocs locally:

[DavidLawes]

Running ./script/lint locally didn't result in any lint errors/changes (neither did running npm run lint --fix). I needed to run npm run format instead.

[jacobwinch]

I wonder if this might be a bit easier to follow as:

Suggested change

	nameOfRoleWhichCanAssumeThisRole: "nameOfRoleInOtherAccountWhichCanAssumeThisNewlyCreatedOne-CODE",
	nameOfRoleWhichCanAssumeThisRole: "roleInAccountA-CODE",

[jacobwinch]

Suggested change

	roleName: "crossAccountRole",
	roleName: `roleinAccountB-CODE`,

[jacobwinch]

Suggested change

	accountId: "1234",
	accountId: "idForAccountA",

[jacobwinch]

Suggested change

	roleName: "nameOfRoleInOtherAccountWhichCanAssumeThisNewlyCreatedOne-CODE",
	roleName: "roleInAccountA-CODE",

[akash1810]

I think there is overlap with AWS CDK's Role and the grantAssumeRole function. Did we consider using this over creating a new class?

const myRole = new Role(); // Or `new GuRole`, as `GuRole` extends from `Role`

// https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.ArnComponents.html
const externalRoleArn: string = Arn.from({
  service: 'iam',
  account: '123456789012',
  resource: 'role',
  resourceName: 'TheOtherRole',
});

// https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_iam.Role.html#static-fromwbrrolewbrarnscope-id-rolearn-options
const externalRole: Role = Role.fromRoleArn(this, 'AnExternalRole', externalRoleArn)

// https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_iam.Role.html#grantwbrassumewbrroleidentity
myRole.grantAssumeRole(externalRole);

If grantAssumeRole does not fit the use-case, should we document the reasons?

Given the class name (CrossAccount) I also wonder if we want to validate the accountId prop does not match the accountId of the "host"?

[DavidLawes]

Thanks @akash1810 - I didn't know about grantAssumeRole and I think it could be used to simplify the tests (i.e. granting the assume role instead of explicitly adding an assume role policy - however when doing this my tests failed, I'm not too sure why just yet).

In terms of using grantAssumeRole instead of the new class I think that could be used. In fact we never needed the cross account role class to correctly create the roles - the new class was aimed at trying to simplify the process and provide more of a skeleton for how to go about this. Does that make sense?

[jacobwinch]

the new class was aimed at trying to simplify the process and provide more of a skeleton for how to go about this

I think this the main thing that we're trying to add over grantAssumeRole (although I was also not aware that existed!)

When using that function you pass in an IPrincipal, so you first have to a) try to understand what all of those different options mean and b) make a secure choice (i.e. pass a specific ARN / role) rather than just any choice that will work (e.g. I think AccountPrincipal would work for this use-case, but it breaks the principle of least privilege).

[DavidLawes]

@akash1810 @jacobwinch

Hi there, thinking about the use of grantAssumeRole I tried to incorporate this in my tests but it looks like this doesn't work as expected. For example, when using grantAssumeRole I couldn't see the expected policy in the resulting stack. It looks like this issue has been encountered before - in particular this comment which says:

"The grantAssumeRole function is a bit misleading here in that it isn't updating the trust policy of the role but rather granting the principal passed in to this action sts:AssumeRole permission. This ends up not doing anything because the principal here is a service who doesn't need to be granted this action, but rather needs to be in the trust policy."

So, perhaps for now, I could use something like assumeRolePolicy which results in code like this:

    role.assumeRolePolicy?.addStatements(
      new PolicyStatement({
        effect: Effect.ALLOW,
        actions: ["sts:AssumeRole"],
        principals: [new ArnPrincipal("arn:aws:iam::idForAccountB:role/roleInAccountB-CODE")]
      })
    )

Giving a policy statement (for my specific tests):

    Template.fromStack(stackInAccountA).hasResourceProperties("AWS::IAM::Role", {
      AssumeRolePolicyDocument: {
        Statement: [
          {
            Action: "sts:AssumeRole",
            Effect: "Allow",
            Principal: {
              Service: "ec2.amazonaws.com"
            }
          },
          {
            Action: "sts:AssumeRole",
            Effect: "Allow",
            Principal: {
              AWS: "arn:aws:iam::idForAccountB:role/roleInAccountB-CODE"
            }
          }
        ],
      },
    });

^ I'm not sure if this is a preferable approach to what I have now (role.addToPolicy...)?

[jacobwinch]

Wow, that's pretty confusing/surprising, but thanks for looking into that!

I'm not sure if this is a preferable approach to what I have now (role.addToPolicy...)?

Personally I think addToPolicy is just as clear, so I think it's fine to leave this as is!

[jacobwinch]

Minor: as we're adding a test for documentation purposes, it could be worth a comment here to explain that (IIUC!) this corresponds to the AWS service that our app will be running in.

In other words, ec2.amazonaws.com is just an example and it could be something else depending on the user's runtime e.g. lambda.amazonaws.com.

[jacobwinch]

This looks good to me 👍

[akash1810]

Thanks for the exploration to grantAssumeRole @DavidLawes, very surprising results!

What would you think about making this behaviour a function within GuRole itself, rather than a new class? For example grantCrossAccountAssumtion. I think the benefits will include:

• A simpler API, as one thing to instantiate (GuRole)
• Less code to maintain
• Making existing constructs more purposeful; the GuRole construct currently does nothing 😅
• The doc string could reference grantAssumeRole, providing advice to not use it

[DavidLawes]

Thanks for the exploration to grantAssumeRole @DavidLawes, very surprising results!

What would you think about making this behaviour a function within GuRole itself, rather than a new class? For example grantCrossAccountAssumtion. I think the benefits will include:

• A simpler API, as one thing to instantiate (GuRole)
• Less code to maintain
• Making existing constructs more purposeful; the GuRole construct currently does nothing 😅
• The doc string could reference grantAssumeRole, providing advice to not use it

Thanks @akash1810 - the benefits you mentioned are very compelling! I've been looking at how this could work:

I tried adding method grantCrossAccountRoleAssumption to the class GuRole and I found it a little counter-intuitive. When instantiating the GuRole we require RoleProps to be handed to the constructor, a mandatory field of which is assumedBy. This means that when instantiating GuRole we're already asking someone to have defined the principal (a key part of the problem that we were trying to abstract / simplify). Because of this I wondered if a method on the existing class actually wasn't too helpful.

I was thinking about updating the type definition for GuRoleProps to optionally allow someone to pass in the accountId and the name of the role which will do the assuming. It could be something like this:

export class GuRole extends Role {
  constructor(scope: GuStack, id: string, props: GuRoleProps) {
    let { assumedBy} = props;
    const { accountId, nameOfRoleWhichCanAssumeThisRole } = props;
    if (accountId && nameOfRoleWhichCanAssumeThisRole) {
      assumedBy = new ArnPrincipal(`arn:aws:iam::${accountId}:role/${nameOfRoleWhichCanAssumeThisRole}`)
    }

    super(scope, id, {
      ...props,
      assumedBy,
    });
  }
}

Is this closer to what you had in mind?

[akash1810]

It could be something like this:

export class GuRole extends Role {
  constructor(scope: GuStack, id: string, props: GuRoleProps) {
    let { assumedBy} = props;
    const { accountId, nameOfRoleWhichCanAssumeThisRole } = props;
    if (accountId && nameOfRoleWhichCanAssumeThisRole) {
      assumedBy = new ArnPrincipal(`arn:aws:iam::${accountId}:role/${nameOfRoleWhichCanAssumeThisRole}`)
    }

    super(scope, id, {
      ...props,
      assumedBy,
    });
  }
}

Is this closer to what you had in mind?

Placing the logic in the constructor limits GuRole to exactly this use-case. If we instead add a function, current usages of GuRole can remain, and one can opt-in to the added functionality:

export class GuRole extends Role {
  constructor(scope: GuStack, props: GuRoleProps) {
    super(scope, props);
  }

  /**
   * Grants permission for an external role to assume this role.
   * Favour over [[ grantAssumeRole ]] which doesn't behave as you'd expect.
   * 
   * @see https://github.com/aws/aws-cdk/issues/24507
   */
  grantCrossAccountAssumeRole(externalAccountId: string, externalIamRoleName: string) {
    const externalRoleArn: string = Arn.from({
      service: 'iam',
      account: externalAccountId,
      resource: 'role',
      resourceName: externalIamRoleName,
    });

    // https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_iam.Role.html#static-fromwbrrolewbrarnscope-id-rolearn-options
    const externalRole: Role = Role.fromRoleArn(this, 'AnExternalRole', externalRoleArn);

    this.grant(externalRole, "sts:AssumeRole");
  }
}

[DavidLawes]

Thanks @akash1810 - I'm wondering if I haven't done a very good job of explaining the problem I'm trying to solve. The difficulty I had recently was around how to go about allowing a service in one aws account to perform actions in another.

There was a chat in the devx channel and Adam highlighted some great best practice. If there was a change to be added to gu cdk then I thought a nice addition could be to define a new type of role that encapsulates this best practice and provides a structure (with associated docs). At the time I didn't think the confusing part was about the role assumption, it was primarily around how to create the role itself.

With that in mind, I'm not convinced that defining a method on the existing GuRole solves my original problem (albeit might be a nice improvement):

• stack A needs to create a role that allows role in stack B to assume it --> it's this part of the setup that I had problems with.
• stack B assumes the role created by stack A --> it's here that I think the grantCrossAccountAssumeRole method would be useful, but this isn't where I found I was having problems.

However, I could be missing something. If the implementation of method grantCrossAccountAssumeRole is something people think would add value, but not the abstraction of cross account role itself, then perhaps I should close this PR and that method implementation could be addressed somewhere else.

[github-actions]

This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days

[github-actions]

This PR was closed because it has been stalled for 3 days with no activity.