cognito-identity-pool: Can't attach IdentityPoolRoleAttachment even if not present

wz2b commented 1 year ago

Describe the bug

If you create an identity pool like this, without specifying an authenticated and unauthenticated role, it generates default roles rather than just leaving them blank.

const idPool = new IdentityPool(stack, "xxx-cognito-id-pool", {
    identityPoolName: "xxx-id-pool",
    allowUnauthenticatedIdentities: true,
    authenticationProviders: {
        userPools: [new UserPoolAuthenticationProvider({userPool: userPool})]
    }
})

Then, later, when you try to attach your own roles, like this:

const roleAttachment = new IdentityPoolRoleAttachment(stack,
    "xxx-id-pool-attachment",
    {
        identityPool: idPool,
        authenticatedRole: authenticatedRole,
        unauthenticatedRole: unauthenticatedRole
    })

it fails with this error:

CREATE_FAILED
AWS::Cognito::IdentityPoolRoleAttachment
xxx-stack/xxx-id-pool-attachment (xxxidpoolattachmentE8645D23) us-east-1:bc70b990-d038-43b8-8501-e7cd3dda6ae7 already exists in stack

Its beef seems to be that even though I created the identity pool without roles attached, it created default ones on its own. Then later when I go to 'attach' my own, it won't let me because it thinks there is already an attachment.

Expected Behavior

I expected that when I create an identity pool with no roles, it would not generate default roles.

Current Behavior

Testing this with only this code and a brand new stack:

    const idPool = new IdentityPool(stack, "my-cognito-id-pool", {
        identityPoolName: "my-id-pool",
        allowUnauthenticatedIdentities: true,
        authenticationProviders: {
            userPools: [new UserPoolAuthenticationProvider({userPool: userPool})]
        }
    })

it created default IAM roles for authenticated and unauthenticated users. This caused a conflict when I then tried to create my own IdentityPoolRoleAttachment, because it says there was already an attachment.

Reproduction Steps


const idPool = new IdentityPool(stack, "my-cognito-id-pool", {
    identityPoolName: "my-id-pool",
    allowUnauthenticatedIdentities: true,
    authenticationProviders: { userPools: [new UserPoolAuthenticationProvider({userPool: userPool})] }
})

const authenticatedRole = new Role(stack, "my-cognito-authenticated-role", {
    roleName: "my-cognito-authenticated-role",
    assumedBy: new WebIdentityPrincipal('cognito-identity.amazonaws.com',
        {
            StringEquals: {
                'cognito-identity.amazonaws.com:aud': `${stack.region}:${idPool.identityPoolId}`,
            },
            'ForAnyValue:StringLike': {
                'cognito-identity.amazonaws.com:amr': 'unauthenticated',
            },
        })
});

const unauthenticatedRole = new Role(stack, "my-cognito-unauthenticated-role", {
    roleName: "my-cognito-unauthenticated-role",
    assumedBy: new WebIdentityPrincipal('cognito-identity.amazonaws.com',
        {
            StringEquals: {
                'cognito-identity.amazonaws.com:aud': `${stack.region}:${idPool.identityPoolId}`,
            },
            'ForAnyValue:StringLike': {
                'cognito-identity.amazonaws.com:amr': 'authenticated',
            },
        })
});

const roleAttachment = new IdentityPoolRoleAttachment(stack,
    "my-id-pool-attachment",
    {
        identityPool: idPool,
        authenticatedRole: authenticatedRole,
        unauthenticatedRole: unauthenticatedRole
    })

Possible Solution

I think the way to fix this is that if you create an identity pool without specifying roles it actually leaves them empty rather than generating default roles. This document tells me this should be possible as the attachment is something completely separate.

Why this is important is that I want to add constraints to my roles that only allow those role to be assumed by this specific identity pool. To do that, I need a reference to the identity pool. So for this to work you have to do things in this order:

Create the identity pool with no attached roles
Create the authorized and unauthorized roles with a trust policy constrained to that identity pool
Attach these roles to the identity pool

Additional Information/Context

My versions of the pieces I am using from packages.json are:

    "@aws-cdk/aws-apigatewayv2-alpha": "^2.56.0-alpha.0",
    "@aws-cdk/aws-apigatewayv2-authorizers-alpha": "^2.56.0-alpha.0",
    "@aws-cdk/aws-apigatewayv2-integrations-alpha": "^2.56.0-alpha.0",
    "@aws-cdk/aws-cognito-identitypool-alpha": "^2.56.0-alpha.0",
    "aws-cdk": "2.56.0",
    "aws-cdk-lib": "2.56.0",

CDK CLI Version

2.56.0 (build 1485f48)

Framework Version

2.56.0-alpha.0

Node.js Version

v16.18.0

OS

Windows 10

Language

Typescript

Language Version

4.9.3

Other information


    "aws-cdk": "2.56.0",
    "aws-cdk-lib": "2.56.0",

mabreuortega commented 1 year ago

same issue here

mabreuortega commented 1 year ago

I though there was some issue with the existing stack, I went and created a new one and the same issue happened

mabreuortega commented 1 year ago

Error log

Auth/AuthRoleAttachment
us-east-1:09cbad3f-99be-4caf-ab51-584475ea29d2 already exists in stack arn:aws:cloudformation:us-east-1:558378773035:stack/PerformanceDash-dev-Auth/54f4df40-310e-11ec-8546-1264d5675413

Full error log

11:07:43 AM | CREATE_FAILED        | AWS::Cognito::IdentityPoolRoleAttachment | AuthRoleAttachment0
C8CE9CC
us-east-1:09cbad3f-99be-4caf-ab51-584475ea29d2 already exists in stack arn:aws:cloudformation:us-ea
st-1:558378773035:stack/PerformanceDash-dev-Auth/54f4df40-310e-11ec-8546-1264d5675413

 ❌  Auth (PerformanceDash-dev-Auth) failed: Error: The stack named PerformanceDash-dev-Auth failed to deploy: UPDATE_ROLLBACK_COMPLETE: us-east-1:09cbad3f-99be-4caf-ab51-584475ea29d2 already exists in stack arn:aws:cloudformation:us-east-1:558378773035:stack/PerformanceDash-dev-Auth/54f4df40-310e-11ec-8546-1264d5675413
    at FullCloudFormationDeployment.monitorDeployment (/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/aws-cdk/lib/api/deploy-stack.ts:505:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at deployStack2 (/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/aws-cdk/lib/cdk-toolkit.ts:265:24)
    at /Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/aws-cdk/lib/deploy.ts:39:11
    at run (/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/p-queue/dist/index.js:163:29)

 ❌ Deployment failed: Error: Stack Deployments Failed: Error: The stack named PerformanceDash-dev-Auth failed to deploy: UPDATE_ROLLBACK_COMPLETE: us-east-1:09cbad3f-99be-4caf-ab51-584475ea29d2 already exists in stack arn:aws:cloudformation:us-east-1:558378773035:stack/PerformanceDash-dev-Auth/54f4df40-310e-11ec-8546-1264d5675413
    at deployStacks (/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/aws-cdk/lib/deploy.ts:61:11)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at CdkToolkit.deploy (/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/aws-cdk/lib/cdk-toolkit.ts:339:7)
    at initCommandLine (/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/aws-cdk/lib/cli.ts:374:12)

Stack Deployments Failed: Error: The stack named PerformanceDash-dev-Auth failed to deploy: UPDATE_ROLLBACK_COMPLETE: us-east-1:09cbad3f-99be-4caf-ab51-584475ea29d2 already exists in stack arn:aws:cloudformation:us-east-1:558378773035:stack/PerformanceDash-dev-Auth/54f4df40-310e-11ec-8546-1264d5675413

wz2b commented 1 year ago

Yep, same here. I checked a few other things: CreateIdentityPool itself doesn't require you specify the roles. SetIdentityPoolRoles is how you set the roles, but from a quick test, it does not throw an error if roles already exist, it just replaces them

so without knowing much more than that, it looks like it's the CDK itself that's throwing the error, not some underlying AWS API issue.

My workaround for now is this:

Create the identity pool, and let the CDK generate default roles (or whatever it does)
Create my roles, with references to the above identity pool
Create a custom resource that calls SetIdentityPoolRoles

wz2b commented 1 year ago

11:07:43 AM | CREATE_FAILED | AWS::Cognito::IdentityPoolRoleAttachment | AuthRoleAttachment0 C8CE9CC

Ahh okay, I was wrong. What you're showing is that it is actually something in CloudFormation, not the CDK.

I'm not sure how to work around this - you'd need some kind of forward reference. Is that possible?

mabreuortega commented 1 year ago

cdk code

/**
         * CloudFormation parameters
         */
        const adminEmail = new CfnParameter(this, "adminEmail", {
            type: "String",
            description: "Email address for the admin user",
            minLength: 5,
        });
        const pool = new UserPool(this, "UserPool", {
            userInvitation: {
                emailSubject:
                    "Subject of the email",
                emailBody: readFileSync("lib/data/email-template.html").toString(),
            },
            customAttributes: { roles: new StringAttribute({ mutable: true }) },
            passwordPolicy: {
                minLength: 8,
                requireLowercase: true,
                requireUppercase: true,
                requireDigits: true,
                requireSymbols: true,
                tempPasswordValidity: Duration.days(3),
            },
            advancedSecurityMode: AdvancedSecurityMode.ENFORCED,
        });

        const client = pool.addClient("Frontend", {
            preventUserExistenceErrors: true,
        });
        const identityPool = this.buildIdentityPool(pool, client, props.authenticationRequired);

        const stack = Stack.of(this);
        const datasetsBucketArn = `arn:${stack.partition}:s3:::${props.datasetsBucketName}`;
        const contentBucketArn = `arn:${stack.partition}:s3:::${props.contentBucketName}`;

        const adminRole = this.buildAdminRole(identityPool, datasetsBucketArn, contentBucketArn);
        const editorRole = this.buildEditorRole(identityPool, datasetsBucketArn, contentBucketArn);
        const publicRole = this.buildPublicRole(
            identityPool,
            props.authenticationRequired,
            datasetsBucketArn,
            contentBucketArn,
        );
        const noAccessRole = this.buildNoAccessRole(identityPool);

        const providerUrl = `cognito-idp.${stack.region}.amazonaws.com/${pool.userPoolId}:${client.userPoolClientId}`;
        new IdentityPoolRoleAttachment(this, "AuthRoleAttachment", {
            identityPool: identityPool,
            unauthenticatedRole: props.authenticationRequired ? noAccessRole : publicRole,
            roleMappings: [
                {
                    mappingKey: "cognito",
                    useToken: true,
                    providerUrl: IdentityPoolProviderUrl.userPool(providerUrl),
                    rules: [
                        {
                            claim: "custom:roles",
                            matchType: RoleMappingMatchType.CONTAINS,
                            claimValue: "Admin",
                            mappedRole: adminRole,
                        },
                        {
                            claim: "custom:roles",
                            matchType: RoleMappingMatchType.CONTAINS,
                            claimValue: "Editor",
                            mappedRole: editorRole,
                        },
                        {
                            claim: "custom:roles",
                            matchType: RoleMappingMatchType.CONTAINS,
                            claimValue: "Public",
                            mappedRole: publicRole,
                        },
                    ],
                },
            ],
        });

        const adminUser = new CfnUserPoolUser(this, "AdminUser", {
            userPoolId: pool.userPoolId,
            username: adminEmail.valueAsString,
            userAttributes: [
                {
                    name: "email",
                    value: adminEmail.valueAsString,
                },
                {
                    name: "custom:roles",
                    value: JSON.stringify(["Admin"]),
                },
            ],
        });

        /**
         * Outputs
         */
        this.userPoolArn = pool.userPoolArn;
        this.appClientId = client.userPoolClientId;
        this.userPoolId = pool.userPoolId;
        this.adminEmail = adminEmail.valueAsString;
        this.identityPoolId = identityPool.identityPoolId;

        new CfnOutput(this, "UserPoolArn", { value: this.userPoolArn });
        new CfnOutput(this, "AppClientId", { value: this.appClientId });
        new CfnOutput(this, "UserPoolId", { value: this.userPoolId });
        new CfnOutput(this, "AdminUsername", { value: adminUser.ref });
        new CfnOutput(this, "IdentityPoolId", {
            value: identityPool.identityPoolId,
        });

and the identity pool creation like

        const identityPool = new IdentityPool(this, "IdentityPool", {
            allowUnauthenticatedIdentities: !authenticationRequired,
            authenticationProviders: {
                userPools: [new UserPoolAuthenticationProvider({ userPool, userPoolClient })],
            },
        });

        return identityPool;

roles like

        const adminRole = this.buildIdentityPoolRole("CognitoAdminRole", true, identityPool);

        adminRole.addToPolicy(
            new PolicyStatement({
                effect: Effect.ALLOW,
                actions: ["s3:GetObject", "s3:PutObject"],
                resources: [
                    ...
                ],
            }),
        );

mabreuortega commented 1 year ago

the code above works fine for default authenticated and unauthenticated roles, but when I introduce the roleMapping the stack starts failing

revmischa commented 1 year ago

Running into precisely the same issue - I want to create a role that is constrained to that pool so need to create the role after the pool has been created. Getting the same error trying to add the role mappings. I tried specifying roleMappings: [] when creating the identity pool but still get the error.

wz2b commented 1 year ago

My workaround for this was to create an AwsCustomResource that called PutRolePolicy which does an add or replace. In my case I implemented onCreate, onUpdate, and onDelete. I think these custom resources must be created in the order they're declared in the stack but I'm not 100% sure of that - it might take some playing around to figure that out.

Thanks @mabreuortega for providing sample code and @revmischa for confirming that I'm not the only one who sees the issue. Unfortunately I'm not close enough to the internals of the CDK to be of much use proposing a solution, except to note that CreateIdentityPool doesn't require you to set the roles (you do that later). Personally I feel like if someone creates an identity pool and doesn't attach auth/unauth roles then it shouldn't use default roles. That might break backward compatibility but I think it's the right thing to do - I don't like the idea of default roles here, I think users should have to consider their own roles.

peterwoodworth commented 1 year ago

Thanks for reporting this issue, and for the discussion 🙂

The way CDK currently handles this is that an IdentityPool will create the two roles, and then create an IdentityPoolRoleAttachment using those roles:

https://github.com/aws/aws-cdk/blob/9b2573b32e8535d3db21f07647f099c9e01eb292/packages/%40aws-cdk/aws-cognito-identitypool/lib/identitypool.ts#L432-L439

The roles are created in this way:

https://github.com/aws/aws-cdk/blob/9b2573b32e8535d3db21f07647f099c9e01eb292/packages/%40aws-cdk/aws-cognito-identitypool/lib/identitypool.ts#L480-L499

Before I head into possible solutions, I'd like to first understand what exactly about the current role generation would cause someone to want to create their own roles instead. Does anyone have some insight on this? A problem described earlier is that you need to configure your trust policy to include the ID of the identity pool. However the default behavior does create a principal with the ID included by default. Additionally, you can use methods to modify roles in CDK if you need to depend on information that comes after a certain time. For example, the addToPrincipalPolicy() method will configure a trust policy on your role, which should help work around the issue described in the original post. However, a problem with this might be that you are required to specify a principal on role creation in CDK

As for solutions, there are a couple different directions we could head to address the issues you are encountering, depending on the exact problems with the current role creation are. Regardless of what we do however, we should clarify in the docs that we are creating an IdentityPoolRoleAttachment resource as part of the IdentityPool construct.

Keep the default behavior of IdentityPool to automatically configure roles and attachment, but provide option to opt our of this behavior
We can make the default behavior of IdentityPool to NOT create roles + attachment if none are provided, but provide an option to opt into this behavior
Provide options to modify the way the generated role is created (i.e. something with configureDefaultGrantPrincipal() or a new related method)

Depending on the clarification of what exactly the issue with the current role creation is, we can choose a direction to head in. I'm also open to alternate solutions if anyone has any suggestions

mabreuortega commented 1 year ago

In my case, we have multiple roles at the application levels (unauthenticated, public, editor, admin). I would like to map a role for each one that only have access to what they need to do in our s3 bucket (read, put in specific paths). We do have a custom cognito claim (custom:roles = "" | "Public" | "Editor" | "Admin") and if possible I would like to assign a role for each of those values

revmischa commented 1 year ago

Yes I as well want to grant different permissions to users depending on what cognito group they belong to. I assume that must be a somewhat common use case.

wz2b commented 1 year ago

Yes I as well want to grant different permissions to users depending on what cognito group they belong to. I assume that must be a somewhat common use case.

Groups are a little different. You specify groups and their associated roles as attributes of the group in the userpool. What we're talking about here is the default roles for authenticated and unauthenticated users.

Typically, what you'd do is you'd have groups defined - for example 'admins' or 'superusers' - then the 'default' authorized user role (the one we're talking about here) would have a default set of permissions that gets applied if you're not a member of a group.

Use of the unauthenticated role is probably a little less common - it's typically a "guest" role but at least in my case I generally just make resources that would use that public. I'm sure there's use cases for the unauthenticated role that I'm not familiar with.

But to answer @peterwoodworth's question, I think the most common use case is to assign base permissions if a user doesn't have a higher level set of permissions based on group.

Implementation

For security reasons I think I'd prefer option 2 - cdk doesn't generate default roles unless you specify something like generateDefaultRoles: true (defaults to false). The only reason I'd vote for option 1 (same thing but default to true) would be if someone had serious backward compatibility concerns.

I don't know how option 3 would work. The whole problem here is the circular reference that's required to make sure this role can only be assumed by the identity pool you're trying to create. I'm going to tailor this role for my own needs - maybe add IoT Core permissions, DynamoDB permissions, really anything. I want to be able to lock the trust policy not just to any identity pool but to this identity pool. That's really what started this thread.

revmischa commented 1 year ago

Yes I as well want to grant different permissions to users depending on what cognito group they belong to. I assume that must be a somewhat common use case.

Groups are a little different. You specify groups and their associated roles as attributes of the group in the userpool. What we're talking about here is the default roles for authenticated and unauthenticated users.

I want my users to use the default authenticated role, but to grant them additional privileges if in certain groups. I don't need the unauthenticated role at the moment for my use case but it's not an issue if it exists. The issue that I have is with trying to add role mappings. Something along the lines of:

    identityPool.addRoleMappings(
    {
      // map admin users to admin role
      mappingKey: 'cognito', // internal dumb CDK thing if you want to use a CDK token as a key
      providerUrl: IdentityPoolProviderUrl.userPool(`${userPoolProviderId}:${webClient.userPoolClientId}`), // allowed for clients using the web client
      resolveAmbiguousRoles: true, // does something
      rules: [
        // JWT claim -> IAM role mappings
        {
          // if group matches admin
          claim: 'cognito:groups',
          matchType: RoleMappingMatchType.CONTAINS,
          claimValue: ADMIN_ROLE,

          // use this role
          mappedRole: adminRole,
        },
      ],
    }
  );

Trying to interpret how to apply what's mentioned in the docs here in CDK

I get the same error mentioned above:

us-east-1:09cbad3f-99be-4caf-ab51-584475ea29d2 already exists in stack

I believe because it's a key in a mapping which has been already defined when the default auth/unauth roles are created automatically. There doesn't seem to be a way to add or replace the default role mappings, even if I pass in [] explicitly in order to create my own mapping.

mabreuortega commented 1 year ago

I think that all this role generation should be handled by the developer and not hidden inside the framework but anyway if we go that route this needs to be mentioned in the README somewhere.

wz2b commented 1 year ago

I think that all this role generation should be handled by the developer and not hidden inside the framework but anyway if we go that route this needs to be mentioned in the README somewhere.

I agree that it should all be handled by the developer. I'm going to go a step further and say it should be a requirement that it be handled by the developer. A README or documentation can provide the defaults as an example.

If we DO want to keep default roles, there should be a way to attach your own policies to them, inline or otherwise. But I think it's simpler to just say "create your own roles, then your identity pool referencing those roles."

mabreuortega commented 1 year ago

I tried to split the stack in AUTH vs AUTHZ trying to create the roles and the mapping later but there are other failures there too.

The method to initialized a pool reference don't work when the identityPoolId is a token

identityPool: IdentityPool.fromIdentityPoolId(
    this,
    "IdentityPool",
    props.identityPoolId,
),

this throws an error in this line https://github.com/aws/aws-cdk/blob/9b2573b32e8535d3db21f07647f099c9e01eb292/packages/%40aws-cdk/aws-cognito-identitypool/lib/identitypool.ts#L334

/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/@aws-cdk/aws-cognito-identitypool-alpha/lib/identitypool.ts:334
    if (!(idParts.length === 2)) throw new Error('Invalid Identity Pool Id: Identity Pool Ids must follow the format <region>:<id>');
                                       ^
Error: Invalid Identity Pool Id: Identity Pool Ids must follow the format <region>:<id>
    at Function.fromIdentityPoolArn (/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/@aws-cdk/aws-cognito-identitypool-alpha/lib/identitypool.ts:334:40)
    at new AuthorizationStack (/Volumes/workplace/performance-dashboard-on-aws/cdk/lib/authz-stack.ts:42:43)
    at Object.<anonymous> (/Volumes/workplace/performance-dashboard-on-aws/cdk/bin/main.ts:39:15)
    at Module._compile (node:internal/modules/cjs/loader:1155:14)
    at Module.m._compile (/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/ts-node/src/index.ts:1618:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1209:10)
    at Object.require.extensions.<computed> [as .ts] (/Volumes/workplace/performance-dashboard-on-aws/cdk/node_modules/ts-node/src/index.ts:1621:12)
    at Module.load (node:internal/modules/cjs/loader:1033:32)
    at Function.Module._load (node:internal/modules/cjs/loader:868:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)

rsmayda commented 1 year ago

Having this issue too

DanielleJOAMZN commented 1 year ago

Ran into this as well. I cannot create the roles without knowing the identityPoolId for the principal, so I have to do the role mapping after the identity pool is created

mabreuortega commented 1 year ago

I moved in the meantime to the use CfnIdentityPoolRoleAttachment and creating the IdentityPool in 2 different stacks:

Leo10Gama commented 2 weeks ago

I've found that the issue seems to be stemming from the way the service API is creating the ID for the IdentityPoolRoleAttachment. Currently they are investigating the issue on their end.

aws / aws-cdk