Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

turn off automatic Cognito UserPool SMS role creation #6943

Closed
kinbald opened this issue Mar 23, 2020 · 16 comments · Fixed by #9513
Closed

turn off automatic Cognito UserPool SMS role creation #6943

kinbald opened this issue Mar 23, 2020 · 16 comments · Fixed by #9513
Assignees
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md

Comments

@kinbald
Copy link

kinbald commented Mar 23, 2020

❓ General Issue

The Question

Hey there,

Working on Cognito module, I noticed that when you create a UserPool, a role for the SMS service (policy: sns:Publish) is created by default even when you don't specify it.
This behavior is not present in the console nor CloudFormation template. Creating this role is a problem in our environment as we don't use it.
Could you make it optional ?

Thanking you in advance

Environment

  • CDK CLI Version: 1.30.0
  • Module Version: Cognito@1.30.0
  • OS: all
  • Language: Typescript

Other information

Cfn stack example :

{
   "AWSTemplateFormatVersion":"2010-09-09",
   "Resources":{
      "UserPooldev01E6BF40":{
         "Type":"AWS::Cognito::UserPool",
         "Properties":{
            "AdminCreateUserConfig":{
               "AllowAdminCreateUserOnly":false
            },
            "AutoVerifiedAttributes":[
               "email"
            ],
            "EmailVerificationMessage":"Your verification code is {####}",
            "EmailVerificationSubject":"Your verification code",
            "Policies":{
               "PasswordPolicy":{
                  "MinimumLength":8,
                  "RequireLowercase":false,
                  "RequireNumbers":false,
                  "RequireSymbols":false,
                  "RequireUppercase":false,
                  "TemporaryPasswordValidityDays":7
               }
            },
            "Schema":[
               {
                  "Name":"email",
                  "Required":true
               },
               {
                  "Name":"name",
                  "Required":true
               },
               {
                  "AttributeDataType":"String",
                  "Name":"organization",
                  "StringAttributeConstraints":{
                     "MaxLength":"256",
                     "MinLength":"1"
                  }
               }
            ],
            "UsernameAttributes":[
               "email"
            ],
            "UserPoolName":"UserPool-dev",
            "VerificationMessageTemplate":{
               "DefaultEmailOption":"CONFIRM_WITH_CODE",
               "EmailMessage":"Your verification code is {####}",
               "EmailSubject":"Your verification code",
               "SmsMessage":"The verification code to your new account is {####}"
            }
         }
      }
   }
}
@kinbald kinbald added the needs-triage This issue or PR still needs to be triaged. label Mar 23, 2020
@SomayaB SomayaB added the @aws-cdk/aws-cognito Related to Amazon Cognito label Mar 25, 2020
@SomayaB SomayaB added bug This issue is a bug. feature-request A feature should be added or improved. labels Mar 25, 2020
@nija-at nija-at added effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 25, 2020
@nija-at nija-at changed the title Cognito UserPool SMS role creation turn off automatic Cognito UserPool SMS role creation Apr 7, 2020
@ryaeng
Copy link

ryaeng commented Apr 9, 2020

@nija-at I would like to take a stab at this one. I would create the UserPool via the CDK and verify the role is not in use. I could then comment out the creation of that role in the code and try again. Thoughts?

@nija-at
Copy link
Contributor

nija-at commented Apr 9, 2020

@ryaeng - is this so you can check whether a user pool can be created without a role? If so, this sounds good.

@ryaeng
Copy link

ryaeng commented Apr 9, 2020

@ryaeng - is this so you can check whether a user pool can be created without a role? If so, this sounds good.

I’ll give it a shot. Can I reach out if I get stuck?

@ryaeng
Copy link

ryaeng commented Apr 13, 2020

@nija-at The policy is required in order to deploy the UserPool. Looks like this is expected behavior. Is there another way this should be addressed or should we close out the issue?

https://gist.github.com/ryaeng/5beb00102d699bc1867be32c09f33fb6

@brainstorm
Copy link

I'm pulling in @dabit3 and @mmatouk from aws-amplify/amplify-js#3495, since I believe this is another counterintuitive issue with the current Cognito deployment/backend in general.

@dabit3 @mmatouk, please take a peek at #6765 if you need more general context.

@ryaeng
Copy link

ryaeng commented Apr 13, 2020

I'm pulling in @dabit3 and @mmatouk from aws-amplify/amplify-js#3495, since I believe this is another counterintuitive issue with the current Cognito deployment/backend in general.

@dabit3 @mmatouk, please take a peek at #6765 if you need more general context.

@brainstorm Is there anything I can do to assist?

@brainstorm
Copy link

@ryaeng I do think that the SMS role shouldn't be required for a successful deploy... if it does, it's a design issue that Cognito backend people can fix, imho? It really should be optional.

@nija-at
Copy link
Contributor

nija-at commented Apr 14, 2020

@ryaeng -

The policy is required in order to deploy the UserPool. Looks like this is expected behavior. Is there another way this should be addressed or should we close out the issue?

Can you take a look at the generated CloudFormation (and its defaults) to check if there are any attributes enabled that either enable phone verification or have an SMS message encoded?

Perhaps, unsetting them or turning them off would tell Cognito not to look for an SMS role?

@ryaeng
Copy link

ryaeng commented Apr 15, 2020

@nija-at What should I be looking for? I found the following comment under the smsConfiguration belonging to the user-pool library which may be of use.

       * The UserPool is very particular that it must contain an 'sns:Publish' action as an inline policy.
       * Ideally, a conditional that restricts this action to 'sms' protocol needs to be attached, but the UserPool deployment fails validation.
       * Seems like a case of being excessively strict.

@nija-at nija-at removed the good first issue Related to contributions. See CONTRIBUTING.md label May 4, 2020
@nija-at
Copy link
Contributor

nija-at commented May 4, 2020

@nija-at The policy is required in order to deploy the UserPool. Looks like this is expected behavior. Is there another way this should be addressed or should we close out the issue?

https://gist.github.com/ryaeng/5beb00102d699bc1867be32c09f33fb6

Can someone provide an example of a UserPool configuration that does not require the SMS role and successfully creates the UserPool with Cognito?

If this is not allowed by the Cognito backend, this option does not make sense for the CDK.

@nija-at nija-at added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 4, 2020
@ArteMisc
Copy link

ArteMisc commented May 4, 2020

I have verified through both the console and CloudFormation that Cognito does not require an SMS role in order to create a user pool, as long as Phone verification or SMS MFA are not enabled. When using the CDK's CfnUserPool construct, the same results can be achieved.

The CloudFormation template I used to test this is the one provided in the original issue report above. If needed, I can provide some screenshots / logs as proof.

Digging into the code, it seems the SMS role is always included in the CloudFormation template simply because it doesn't check whether one is actually needed.

private smsConfiguration(props: UserPoolProps): CfnUserPool.SmsConfigurationProperty {
if (props.smsRole) {
return {
snsCallerArn: props.smsRole.roleArn,
externalId: props.smsRoleExternalId,
};
} else {
const smsRoleExternalId = this.node.uniqueId.substr(0, 1223); // sts:ExternalId max length of 1224
const smsRole = props.smsRole ?? new Role(this, 'smsRole', {
assumedBy: new ServicePrincipal('cognito-idp.amazonaws.com', {
conditions: {
StringEquals: { 'sts:ExternalId': smsRoleExternalId },
},
}),
inlinePolicies: {
/*
* The UserPool is very particular that it must contain an 'sns:Publish' action as an inline policy.
* Ideally, a conditional that restricts this action to 'sms' protocol needs to be attached, but the UserPool deployment fails validation.
* Seems like a case of being excessively strict.
*/
'sns-publish': new PolicyDocument({
statements: [
new PolicyStatement({
actions: [ 'sns:Publish' ],
resources: [ '*' ],
}),
],
}),
},
});
return {
externalId: smsRoleExternalId,
snsCallerArn: smsRole.roleArn,
};
}
}

If there is a role, it is used in the returned configuration. If there is no role, a new one is created regardless of the other userpool parameters. The comment about the necessity of the sns:Publish action seems to only relate to Cognito needing the policy to be provided inline with the role as opposed to attached through a named policy.

It seems this issue may be fixed by simply adding a conditional, something along the lines of

if (props.smsRole) {
  // return unchanged
  return { ... };
}

const mfaEnabled = props.mfa === Mfa.OPTIONAL || props.mfa === Mfa.REQUIRED;
const mfaSms = !props.mfaSecondFactor || props.mfaSecondFactor.sms === true;
const phoneVerification = props.signInAliases?.phone === true;
// - maybe also needed if the schema contains the phone attribute?
const requireRole = (mfaEnabled && mfaSms) || phoneVerification;
if (!requireRole) {
  // no role needed or provided
  return undefined;
}

// generate the role
return { ... };

Any thoughts?

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 6, 2020
@nija-at
Copy link
Contributor

nija-at commented May 11, 2020

Thanks for the detailed response @ArteMisc. This seems reasonable.

Along with this, I would also recommend introducing a flag disableAutoSmsRole which defaults to false. An error is generated when this is set and, either SMS MFA is enabled or phone verification is enabled.

This flag makes sure that the user doesn't unintentionally create an SMS role. Especially, when they don't have these permissions, this flag and its error message will make it clear what is triggering the creation of the role.

@nija-at nija-at added the good first issue Related to contributions. See CONTRIBUTING.md label May 11, 2020
@ArteMisc
Copy link

So updating my snippet with your feedback, it would become

if (props.smsRole) {
  // return unchanged
  return { ... };
}

const mfaEnabled = props.mfa === Mfa.OPTIONAL || props.mfa === Mfa.REQUIRED;
const mfaSms = !props.mfaSecondFactor || props.mfaSecondFactor.sms === true;
const phoneVerification = props.signInAliases?.phone === true;
const requireRole = (mfaEnabled && mfaSms) || phoneVerification;
if (!requireRole) {
  // no role needed or provided
  return undefined;
}

// --- this check is added
if (!!props.disableAutoSmsRole) {
  const reasonSMS = (mfaEnabled && mfaSms) ? "SMS mfa enabled" : undefined;
  const reasonVerify = phoneVerification ? "Phone verification enabled" : undefined;
  throw new Error(`UserPool configuration requires an SMS role (reason: ${reasonSMS || reasonVerify}) but disableAutoSmsRole was set to true`);
}

// generate the role
return { ... };

If this seems alright, I could take a stab at implementing it over the weekend.

@nija-at
Copy link
Contributor

nija-at commented May 11, 2020

Yes, something like that seems reasonable.

@joraycorn
Copy link

Any news on that ?

@litsters
Copy link

litsters commented Aug 6, 2020

It's August now. Any progress on this? I'm in a situation like the original poster where we don't use SMS at all with our user pool. Also in a role-strict environment where I can't create IAM roles, so this is a blocker.

nija-at pushed a commit that referenced this issue Aug 7, 2020
- Introduce a property `enableSmsRole` that can be used to override CDK
logic and explicitly enable or disable automatic creation of an IAM role
for SMS.

- Instead of creating the SMS role by default, all of the time, be smart
about determining when the role is actually needed. Create the role only
if (a) SMS is configured as MFA second factor, (b) sign in via phone
number is enabled, or (c) phone verification is required.

BREAKING CHANGE: CDK may now remove a previously created IAM role for
SMS. The role will be removed only because it's not actually required by
the user pool based on its configuration, so this should have no impact.
This behaviour can be explicitly overridden by setting `enableSmsRole`
property.

closes #6943
@mergify mergify bot closed this as completed in #9513 Aug 12, 2020
mergify bot pushed a commit that referenced this issue Aug 12, 2020
- Introduce a property `enableSmsRole` that can be used to override CDK
logic and explicitly enable or disable automatic creation of an IAM role
for SMS.

- Instead of creating the SMS role by default, all of the time, be smart
about determining when the role is actually needed. Create the role only
if (a) SMS is configured as MFA second factor, (b) sign in via phone
number is enabled, or (c) phone verification is required.

closes #6943


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants