feat(cli): MFA support #6510

nikolauska · 2020-02-28T22:09:25Z

With these changes AWS CDK now supports mfa_serial field so when profile has mfa_serial set the user is asked for MFA token. If user then adds corrects short lived token they will get access to environment.

Example config for assume role with MFA that will be supported after these changes.

[profile mfa]
region=eu-west-1

[profile mfa-role]
source_profile=mfa
role_arn=arn:aws:iam::account:role/role
mfa_serial=arn:aws:iam::account:mfa/user

These changes currently only have one test as I don't have enough knowledge of the code base to write better tests. Current test only checks that user is asked for token by looking at the error message which should result in invalid token.

Fixes: #1248

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

nikolauska · 2020-03-11T15:02:03Z

Merged to latest master after there were lot of changes to authentication. Changes required are now much smaller than earlier as I only needed to add the tokenCodeFn function.

nikolauska · 2020-03-11T15:02:59Z

Seems like the build failed for 503 service unavailable error which should not be related to these changes

shivlaks · 2020-03-11T18:06:49Z

Seems like the build failed for 503 service unavailable error which should not be related to these changes

@nikolauska yeah it's unrelated, I'll see if i can trigger a re-run of the build for you

rix0rrr · 2020-03-11T19:34:48Z

packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts

    }

    if (await fs.pathExists(configFileName())) {
-      sources.push(() => new AWS.SharedIniFileCredentials({ profile, filename: credentialsFileName() }));
+      sources.push(() => new AWS.SharedIniFileCredentials({ profile, filename: credentialsFileName(), tokenCodeFn }));


Oh, this block slipped in accidentally during a refactor of mine.

Mind taking it out? <3

Sure thing :)

That line looked weird to me, but as it did not seem to cause issue when I was testing it I decided to keep it.

shivlaks

Add Tests
Update README
Add more motivation and approach details into the commit body. (i.e. why is it failing today, what are we missing, what are we adding, etc..)

packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts

shivlaks · 2020-04-03T05:03:21Z

packages/aws-cdk/lib/api/aws-auth/awscli-compatible.ts

+ */
+async function tokenCodeFn(serial: string, cb: (err?: Error, token?: string) => void): Promise<void> {
+  debug('Require MFA token for serial', serial);


is this the device serial or the serial ARN?

Pull request has been modified.

nikolauska · 2020-04-10T16:51:07Z

Squashed all the commits and added more info to commit body.

There is now also one test, but with limited knowledge of bigger code base it was the only one I got working. Also added section to README.md about the MFA support.

vastamaki · 2020-04-29T10:08:04Z

@shivlaks Any updates about this?

shivlaks · 2020-04-30T00:40:25Z

@nikolauska @vastamaki sorry about the delay, didn't realize this PR was updated. I'll take a look this week. stay tuned!

shivlaks

code looks good to me! had a question about the additional dependency we would be introducing here with inquirer

packages/aws-cdk/package.json

shivlaks

I believe you also have a conflict to resolve here.

Otherwise I'm okay with it. Added the do-not-merge label so @rix0rrr can also have a look

packages/aws-cdk/README.md

Pull request has been modified.

rix0rrr · 2020-05-13T09:56:12Z

packages/aws-cdk/test/api/sdk-provider.test.ts

+      await provider.withAssumedRole('arn:aws:iam::account:role/role', undefined, undefined);
+    } catch (e) {
+      // Mock token cannot work, but having this error means user was asked for MFA token
+      expect(e.message).toEqual('The security token included in the request is invalid.');


I sure hope this test doesn't do any actual calls to STS.

I don't think it will as it will ask token before calling STS, but I'm not 100% sure about that. If there is better way to test it I will change to use that as I just used that other role test as base and just changed it by adding mfa_serial to it

I've looked into the aws-sdk source code and according to the load function it will ask for token before continuing with assumeRole call. As my original test did return token it made STS call, but it failed on the next step as it was not a valid token. I've changed my test so that it now throws error so it won't call assumeRole.

Link to the code part in the aws-sdk I mentioned:
https://github.com/aws/aws-sdk-js/blob/75690eb8edb4f90dbcac1e8ad03847262f8cddf4/lib/credentials/shared_ini_file_credentials.js#L233

dnascimento · 2020-06-29T12:43:24Z

Hi @nikolauska , would you have a chance to review and have it merged? Your contribution will help large orgs to adopt CDK, thanks for making it

shivlaks

pending verification that unit tests don't make actual calls to STS

@nikolauska I also believe we can remove the yarn.lock changes in this PR as we are no longer adding a new dependency for prompting users

Pull request has been modified.

With this change AWS CDK supports MFA. Specifically it will support mfa_serial field in the profile config by asking user for MFA token for the mfa_serial field ARN. AWS SDK has support for this built in so only change is adding tokenCodeFn function to sharedIniCredentials options. Callback sent to that function is used to return token back to SDK. Inquirer package is used to create interactive prompt for user to type the MFA token.

nikolauska · 2020-07-16T09:19:20Z

@shivlaks I've now removed the yarn.lock changes and made one change to MFA test to make sure it won't call the assumeRole.

johan-lindqvist · 2020-07-16T12:19:31Z

This looks great! Hopefully this gets merged soon, thanks for the great work @nikolauska

nikolauska · 2020-08-06T13:21:57Z

@shivlaks Any news on getting this merged?

shivlaks

thanks for this contribution!

christophgysin · 2020-08-14T06:59:45Z

@shivlaks Now that inquirer has been replaced with promptly, is there still something that blocks this from being merged? I'm asking because it is still labelled pr/do-not-merge?

shivlaks · 2020-08-17T14:17:28Z

@shivlaks Now that inquirer has been replaced with promptly, is there still something that blocks this from being merged? I'm asking because it is still labelled pr/do-not-merge?

good catch, my miss on dropping the label

mergify · 2020-08-17T14:18:21Z

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-08-17T14:42:29Z

AWS CodeBuild CI Report

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

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

mergify · 2020-08-17T14:42:54Z

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).

nikolauska changed the title ~~feat(cli): Config file and MFA support~~ feat(cli): MFA support Feb 28, 2020

SomayaB assigned shivlaks Feb 29, 2020

nikolauska marked this pull request as ready for review March 2, 2020 18:42

rix0rrr reviewed Mar 11, 2020

View reviewed changes

shivlaks previously requested changes Apr 3, 2020

View reviewed changes

nikolauska force-pushed the add-mfa-support branch from 8f55243 to 2196d8f Compare April 10, 2020 16:41

shivlaks reviewed May 7, 2020

View reviewed changes

packages/aws-cdk/package.json Outdated Show resolved Hide resolved

shivlaks added the pr/do-not-merge This PR should not be merged at this time. label May 8, 2020

shivlaks previously approved these changes May 8, 2020

View reviewed changes

packages/aws-cdk/README.md Show resolved Hide resolved

shivlaks requested a review from rix0rrr May 9, 2020 03:24

nikolauska force-pushed the add-mfa-support branch from 2196d8f to 8b63e5a Compare May 10, 2020 11:28

nikolauska force-pushed the add-mfa-support branch from 8b63e5a to c244dd2 Compare May 10, 2020 11:38

rix0rrr reviewed May 13, 2020

View reviewed changes

nikolauska force-pushed the add-mfa-support branch from c244dd2 to ef171e1 Compare May 14, 2020 09:38

nikolauska force-pushed the add-mfa-support branch from ef171e1 to 5ea8701 Compare May 24, 2020 14:49

nikolauska force-pushed the add-mfa-support branch from 5ea8701 to ba2f9bc Compare June 12, 2020 15:43

nikolauska force-pushed the add-mfa-support branch from ba2f9bc to 9623f07 Compare June 29, 2020 13:14

nikolauska requested a review from rix0rrr June 29, 2020 13:16

nikolauska force-pushed the add-mfa-support branch from 9623f07 to d8755ce Compare June 30, 2020 06:33

nikolauska force-pushed the add-mfa-support branch from d8755ce to 494383e Compare July 15, 2020 20:04

shivlaks previously requested changes Jul 16, 2020

View reviewed changes

nikolauska force-pushed the add-mfa-support branch from 494383e to 80ee3e1 Compare July 16, 2020 09:01

nikolauska force-pushed the add-mfa-support branch from 80ee3e1 to ad978ea Compare July 16, 2020 09:14

shivlaks approved these changes Aug 7, 2020

View reviewed changes

shivlaks removed the pr/do-not-merge This PR should not be merged at this time. label Aug 17, 2020

Merge branch 'master' into add-mfa-support

a4c204e

mergify bot merged commit 611c48d into aws:master Aug 17, 2020

ghost mentioned this pull request Aug 20, 2020

[cli] Cache mfa credentials #9855

Closed

unleashed-jamest mentioned this pull request Aug 29, 2020

[cli] CDK_DEFAULT_ACCOUNT not set when using MFA and --profile specified #10048

Closed

johan-lindqvist mentioned this pull request Oct 14, 2020

[cli] Cache mfa credentials #10867

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): MFA support #6510

feat(cli): MFA support #6510

nikolauska commented Feb 28, 2020 •

edited by shivlaks

Loading

nikolauska commented Mar 11, 2020

nikolauska commented Mar 11, 2020

shivlaks commented Mar 11, 2020

rix0rrr Mar 11, 2020

nikolauska Mar 11, 2020

shivlaks left a comment

shivlaks Apr 3, 2020

nikolauska commented Apr 10, 2020

vastamaki commented Apr 29, 2020

shivlaks commented Apr 30, 2020

shivlaks left a comment

shivlaks left a comment

rix0rrr May 13, 2020

nikolauska May 13, 2020

nikolauska Jul 16, 2020

dnascimento commented Jun 29, 2020

shivlaks left a comment

nikolauska commented Jul 16, 2020

johan-lindqvist commented Jul 16, 2020

nikolauska commented Aug 6, 2020

shivlaks left a comment

christophgysin commented Aug 14, 2020

shivlaks commented Aug 17, 2020

mergify bot commented Aug 17, 2020

aws-cdk-automation commented Aug 17, 2020

mergify bot commented Aug 17, 2020

feat(cli): MFA support #6510

feat(cli): MFA support #6510

Conversation

nikolauska commented Feb 28, 2020 • edited by shivlaks Loading

nikolauska commented Mar 11, 2020

nikolauska commented Mar 11, 2020

shivlaks commented Mar 11, 2020

rix0rrr Mar 11, 2020

Choose a reason for hiding this comment

nikolauska Mar 11, 2020

Choose a reason for hiding this comment

shivlaks left a comment

Choose a reason for hiding this comment

shivlaks Apr 3, 2020

Choose a reason for hiding this comment

nikolauska commented Apr 10, 2020

vastamaki commented Apr 29, 2020

shivlaks commented Apr 30, 2020

shivlaks left a comment

Choose a reason for hiding this comment

shivlaks left a comment

Choose a reason for hiding this comment

rix0rrr May 13, 2020

Choose a reason for hiding this comment

nikolauska May 13, 2020

Choose a reason for hiding this comment

nikolauska Jul 16, 2020

Choose a reason for hiding this comment

dnascimento commented Jun 29, 2020

shivlaks left a comment

Choose a reason for hiding this comment

nikolauska commented Jul 16, 2020

johan-lindqvist commented Jul 16, 2020

nikolauska commented Aug 6, 2020

shivlaks left a comment

Choose a reason for hiding this comment

christophgysin commented Aug 14, 2020

shivlaks commented Aug 17, 2020

mergify bot commented Aug 17, 2020

aws-cdk-automation commented Aug 17, 2020

AWS CodeBuild CI Report

mergify bot commented Aug 17, 2020

nikolauska commented Feb 28, 2020 •

edited by shivlaks

Loading