aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.47k stars 3.83k forks source link

[cognito] Unable to create user pool with MFA required #11478

Closed binoculars closed 2 years ago

binoculars commented 3 years ago

(See reproduction steps for code example)

Ideally, I would like to enable MFA required with OTP only (not SMS)

When mfaSecondFactor.sms is set to false, the stack is able to be synthesized, but CloudFormation gives this error:

SMS configuration and Auto verification for phone_number are required when MFA is required/optional (Service: AWSCognitoIdentityProviderService; Status Code: 400; Error Code: InvalidParameterException; Request ID: d5cbb83e-b38a-4f57-9993-65ff923ac325; Proxy: null)

When mfaSecondFactor.sms is set to `true, the stack is not able to be synthesized with the following error:

TypeError: Cannot read property 'uniqueId' of undefined
    at UserPool.smsConfiguration (/Users/barrett/Projects/github.com/tributumtax/monorepo/.yarn/$$virtual/@aws-cdk-aws-cognito-virtual-cef0628d0c/0/cache/@aws-cdk-aws-cognito-npm-1.73.0-8a062b6262-b3bdc35f1d.zip/node_modules/@aws-cdk/aws-cognito/lib/user-pool.ts:884:37)
    at new UserPool (/Users/barrett/Projects/github.com/tributumtax/monorepo/.yarn/$$virtual/@aws-cdk-aws-cognito-virtual-cef0628d0c/0/cache/@aws-cdk-aws-cognito-npm-1.73.0-8a062b6262-b3bdc35f1d.zip/node_modules/@aws-cdk/aws-cognito/lib/user-pool.ts:740:30)
    at new AuthStack (/Users/barrett/Projects/github.com/tributumtax/monorepo/packages/aws/lib/auth-stack.ts:23:22)
    at Object.<anonymous> (/Users/barrett/Projects/github.com/tributumtax/monorepo/packages/aws/bin/aws.ts:45:19)
    at Module._compile (internal/modules/cjs/loader.js:1176:30)
    at Module.m._compile (/Users/barrett/Projects/github.com/tributumtax/monorepo/.yarn/unplugged/ts-node-virtual-9b8834e7f3/node_modules/ts-node/src/index.ts:858:23)
    at Module._extensions..js (internal/modules/cjs/loader.js:1196:10)
    at Object.require.extensions.<computed> [as .ts] (/Users/barrett/Projects/github.com/tributumtax/monorepo/.yarn/unplugged/ts-node-virtual-9b8834e7f3/node_modules/ts-node/src/index.ts:861:12)
    at Module.load (internal/modules/cjs/loader.js:1040:32)
    at Function.external_module_.Module._load (/Users/barrett/Projects/github.com/tributumtax/monorepo/.pnp.js:26436:14)
Subprocess exited with error 1

Reproduction Steps

import { App, Stack, StackProps } from '@aws-cdk/core';
import * as cognito from '@aws-cdk/aws-cognito';

export class AuthStack extends Stack {
  constructor(scope: App, id: string, props: StackProps) {
    super(scope, id, props);

    const userPool = new cognito.UserPool(this, 'UserPool', {
      selfSignUpEnabled: true,
      signInAliases: {
        email: true,
      },
      autoVerify: {
        email: true,
      },
      mfa: cognito.Mfa.REQUIRED,
      mfaSecondFactor: {
        sms: true,
        otp: true,
      },
      passwordPolicy: {
        minLength: 16,
        requireLowercase: true,
        requireUppercase: true,
        requireDigits: true,
        requireSymbols: true,
      },
      accountRecovery: cognito.AccountRecovery.EMAIL_ONLY,
    });
  }
}

What did you expect to happen?

I should be able to deploy the stack

What actually happened?

The synth fails if sms is true and CloudFormation fails if it is set to false.

Environment

Other


This is :bug: Bug Report

nija-at commented 3 years ago

Ideally, I would like to enable MFA required with OTP only (not SMS)

I am able to set this up by taking the code you provided but setting mfaSecondFactor.sms to false and mfaSecondFactor.otp to true. The stack deployed successfully.

I was able to set up the stack again by setting mfaSecondFactor.sms to true.

Can you try creating the user pool afresh, rather than as an update to an existing user pool?

binoculars commented 3 years ago

@nija-at This is a new stack, so I'm not sure what you mean by creating the user pool afresh. Again, when mfaSecondFactor.sms is set to false, I get a the CloudFormation error

nija-at commented 3 years ago

I can't seem to figure out what the correct configuration for the user pool in this case is.

As you've noted, when MFA is required with OTP only (not SMS), the error message is -

SMS configuration and Auto verification for phone_number are required when MFA is required/optional (Service: AWSCognitoIdentityProviderService; Status Code: 400; Error Code: InvalidParameterException; Request ID: d5cbb83e-b38a-4f57-9993-65ff923ac325; Proxy: null)

However, if I do set the SMS configuration (even if it's just OTP only MFA), the error message I get is -

Can't turn off SMS_MFA when SMS configuration is set for the user pool with a required or optional MFAConfiguration. (Service: AWSCognitoIdentityProvider; Status Code: 400; Error Code: InvalidParameterException; Request ID: null; Proxy: null)

It's not clear what the correct configuration is for setting up a user pool for OTP only MFA. I've reached out to the Cognito team for guidance.

Internal ref: t.corp/P40850243

heyarnold1 commented 3 years ago

I think i created a duplicate ticket https://github.com/aws/aws-cdk/issues/12075

Does this issue impact just CDK or does it also impact Cloudformation?

I note Terraform had a similar issue that appears to now be closed https://github.com/hashicorp/terraform-provider-aws/issues/7261

Maybe also related to this Amplify issue https://github.com/aws-amplify/amplify-cli/issues/3007

velovix commented 3 years ago

I believe this is also an issue with CloudFormation. I see this problem when using Serverless with CloudFormation templates.

ravenscar commented 3 years ago

I ran into this problem and have discovered if I create the userpool first then add in

      mfa: cognito.Mfa.OPTIONAL,
      mfaSecondFactor: {
        sms: true,
        otp: true,
      },

and run it again, it gets set up correctly.

Obviously not a particularly useful workaround but might work in a pinch.

EDIT: whoops I was reading this incorrectly, I had been getting this error with OPTIONAL and used this two step process to turn on OPTIONAL MFA, obviously this won't work for REQUIRED :(

heikkis commented 3 years ago

I think CDK produce wrong Cloudformation template for MfaConfigration value. It is not Cloudformation problem in general.

      EnabledMfas:
        - SMS_MFA
        - SOFTWARE_TOKEN_MFA
      MfaConfiguration: "OFF"

I'm now able to bypass this problem with CFN modification:

            const cfnUserPool = userPool.node.defaultChild as CfnUserPool
            cfnUserPool.addPropertyOverride(
                'MfaConfiguration',
                "ON"
            )
BenassiJosef commented 3 years ago

@nija-at Hey guys, I was also getting this error: "SMS configuration and Auto verification for phone_number are required when MFA is required/optional..." When using the CDK (typescript) and setting mfa: cognito.Mfa.REQUIRED, mfaSecondFactor: { sms: false, otp: true } as you seem to be able to do this fine within the console easy enough. Ive seen other threads with people also being thrown this error using Cloud Formation... yeh I know CDK synths to a cloud formation template but you know... same same but different...

So i thought well ok .... what if I add the relevant Role in IAM and give this to the smsRole and add and external id ? Well that seemed to do the trick. My code looks like the following:

import * as cdk from '@aws-cdk/core';
import * as cognito from "@aws-cdk/aws-cognito";
import { Duration, RemovalPolicy } from '@aws-cdk/core';
import * as iam from "@aws-cdk/aws-iam"

export class InfrastructureIdamTypescriptStack extends cdk.Stack {
    constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
      super(scope, id, props);

      const POLICY = {
        "Version": "2012-10-17",
        "Statement": [
            {
                "Effect": "Allow",
                "Action": [
                    "sns:Publish"
                ],
                "Resource": "*"
            }
        ]
    }
      const snsPolicy = iam.PolicyDocument.fromJson(POLICY)
      const poolSmsRole = new iam.Role(this, 'Role', {
        assumedBy: new iam.ServicePrincipal('cognito-idp.amazonaws.com'),
        inlinePolicies:{['sns-test']:snsPolicy}
      });
      const userpool = new cognito.UserPool(this, 'testUserPool', {
        userPoolName: 'test-cdk',
        selfSignUpEnabled: true,
        userVerification: {
          emailSubject: 'Verify your email for our the test-cdk app!',
          emailBody: 'Thanks for signing up to our test-cdk app! Your verification code is {####}',
          emailStyle: cognito.VerificationEmailStyle.CODE,
        },
        userInvitation: {
          emailSubject: 'Invite to join our test app!',
          emailBody: 'Hello {username}, you have been invited to join our test app! Your temporary password is {####}',
          smsMessage: undefined
        },
        signInAliases: {
          email: true,
          username: false,
          phone: false
        },
        standardAttributes: {
          fullname: {
            required: true,
            mutable: false,
          },
          address: {
            required: false,
            mutable: false,
          },
          email: {
            required: true,
            mutable: false
          },
        },
        customAttributes: {
          'terms': new cognito.BooleanAttribute({ mutable: true })
        },
        accountRecovery: 2,
        autoVerify: {
          email: true,
          phone: false
        },
        signInCaseSensitive: false,
        passwordPolicy: {
          minLength: 8,
          requireLowercase: false,
          requireUppercase: false,
          requireDigits: false,
          requireSymbols: false,
          tempPasswordValidity: Duration.days(7),
        },
        removalPolicy: RemovalPolicy.DESTROY,
        mfa: cognito.Mfa.REQUIRED,
        mfaSecondFactor: {
          sms: true,
          otp: true
        },
        smsRole: poolSmsRole,
        smsRoleExternalId: 'c87467be-4f34-11ea-b77f-2e728ce88125'
      });    
      userpool.addClient('IDAM_BACKEND', {
          generateSecret: true,
          oAuth: {
            callbackUrls: ["http://localhost:3000/success"],
            flows: {
              authorizationCodeGrant: true,
              implicitCodeGrant: true,
              clientCredentials: false
            },

          },
          preventUserExistenceErrors: true

      });

    }
  }

Ok so this runs fine, cloud formation does its thing. And when I go to look at the user pools 'MFA and verifications" section In the console we get this ....

Screenshot 2021-04-03 at 04 07 33

So even although sms is ticked for MFA I must request a spending limit increase from Amazon SNS before being able to use this method for MFA. OTP is checked so happy days. We have OTP set up for MFA through the CDK under the required flag. Boom

Gslaughl commented 3 years ago

I just want to bump this issue. Fwiw I'm also seeing the same error with CloudFormation.

SMS configuration and Auto verification for phone_number are required when MFA is required/optional

ChrisSargent commented 3 years ago

I am also seeing this issue with the following stack:

const userPool = defaults.buildUserPool(this, {
      accountRecovery: AccountRecovery.EMAIL_ONLY,
      autoVerify: {
        email: true,
        phone: false,
      },
      enableSmsRole: false,
      mfa: cognito.Mfa.OPTIONAL,
      mfaSecondFactor: {
        otp: true,
        sms: false,
      },
      removalPolicy: RemovalPolicy.DESTROY,
      selfSignUpEnabled: true,
      signInAliases: {
        email: true,
        phone: false,
        preferredUsername: false,
        username: false,
      },
      signInCaseSensitive: false,
    });

UPDATE:

I was able to deploy the stack if I set mfa to off. And then I could switch to optional via the UI.... which is not ideal and also not that interesting. What is slightly interesting is that if I tried to redeploy the stack (still with mfa = off), it successfully deployed but did not actually change the user pool mfa setting back to off...

polarizeme commented 3 years ago

UPDATE:

I was able to deploy the stack if I set mfa to off. And then I could switch to optional via the UI.... which is not ideal and also not that interesting. What is slightly interesting is that if I tried to redeploy the stack (still with mfa = off), it successfully deployed but did not actually change the user pool mfa setting back to off...

Unless you have drift detection on, the stack has no way of knowing that you flipped MFA on, so re-running the stack with MFA set to "off" shouldn't do anything because the stack won't know there's anything to update for the MFA setting. Figuring this out when they launched Drift Detection was a massive help to solving the "people are manually changing stuff" problem over here haha.

Also, just to chime in on the topic at hand, as of today this is 100% still an issue in Cloudformation, so I'd imagine it's still plaguing the CDK as well since I think the CDK synths CFN for the resource generation. Hopefully this is fixed soon, because UGH this is dumb.

ChrisSargent commented 3 years ago

Unless you have drift detection on, the stack has no way of knowing that you flipped MFA on, so re-running the stack with MFA set to "off" shouldn't do anything because the stack won't know there's anything to update for the MFA setting. Figuring this out when they launched Drift Detection was a massive help to solving the "people are manually changing stuff" problem over here haha.

Right, yeah, makes sense.

drawde111 commented 3 years ago

Also experiencing this issue with cdk when mfa is required. Any update on this?

wmarcuse commented 3 years ago

The SMS configuration correlates to the auto verification service Cognito provides, if this verification method is set on phone_number then SMS configuration is required.

Default: - If signInAlias includes email and/or phone, they will be included in autoVerifiedAttributes by default. If absent, no attributes will be auto-verified.

So make sure you don't have auto verify enabled for phone number and MFA required with only otp enabled should work.

jumic commented 2 years ago

The CloudFormation issue SMS configuration and Auto verification for phone_number are required when MFA is required/optional was resolved, see https://github.com/aws-cloudformation/cloudformation-coverage-roadmap/issues/769.

I was able to deploy the examples from the original issue (sms set to true and false) and from the replies.

@corymhall Should we close this issue and open a new one, if there is another specific problem?

corymhall commented 2 years ago

@jumic thanks for letting me know! I'll close this issue for now since the issue looks to be resolved, but if anyone thinks the issue is not resolved let me know.

github-actions[bot] commented 2 years ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

welljj commented 1 year ago

@jumic thanks for letting me know! I'll close this issue for now since the issue looks to be resolved, but if anyone thinks the issue is not resolved let me know.

@corymhall I'm still having similar issues in CDK:

Here's my code:

Scenario 1

import aws_cdk.aws_cognito as cognito
from aws_cdk import Stack, RemovalPolicy, Duration
from constructs import Construct

class AuthStack(Stack):
    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        user_pool = cognito.UserPool(
            self,
            "userpool",
            account_recovery=cognito.AccountRecovery.EMAIL_AND_PHONE_WITHOUT_MFA,
            auto_verify=cognito.AutoVerifiedAttrs(email=False, phone=True),
            mfa=cognito.Mfa.REQUIRED,
            mfa_second_factor=cognito.MfaSecondFactor(otp=True, sms=True),
            removal_policy=RemovalPolicy.DESTROY,
            self_sign_up_enabled=True,
            sign_in_aliases=cognito.SignInAliases(
                email=False, phone=True, username=False, preferred_username=False
            ),
        )

But this gives me the error: Invalid account recovery setting parameter. Account Recovery Setting cannot use verified_phone_number as a recovery mechanism when SMS MFA is configured.

Furthermore, if I change this to

Scenario 2

mfa_second_factor=cognito.MfaSecondFactor(otp=True, sms=False),

I get the following error: Can't turn off SMS_MFA when SMS configuration is set for the user pool with a required or optional MFAConfiguration.

As I understand it, both of these scenarios are possible using the AWS Console, but CDK is throwing errors for them.

Scenario 1: My best guess here is that CloudFormation templates only allow setting a Priority for each Account Recovery Method, but no way to apply logic so that it won't use the Phone when SMS MFA is utilized, as both the CDK documentation and Console describe.

Scenario 2: I don't know why SMS_MFA cannot be disabled, since it's an option in the Console, and CDK provides the ability to disable it.

The easiest fix here to get this deploying without errors is using Scenario 1, except changing to

account_recovery=cognito.AccountRecovery.EMAIL_ONLY,

Since Scenario 2 with this change still gives the same error.

EDIT: It looks like setting

account_recovery=cognito.AccountRecovery.PHONE_AND_EMAIL,

will deploy successfully too, but in the Console it appears that "Email Only" is set instead of "SMS if available, otherwise email, and allow a user to reset their password via SMS if they are also using it for MFA"