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

Assume role and MFA support for Serverless CLI #5432

Merged
merged 3 commits into from
Nov 1, 2018

Conversation

kennu
Copy link
Contributor

@kennu kennu commented Oct 30, 2018

What did you implement:

With these changes Serverless Framework should be compatible with
AWS CLI, so that these instructions for assuming a role work
out-of-the-box:
https://docs.aws.amazon.com/cli/latest/userguide/cli-roles.html

Related to #5048

How did you implement it:

This sets the AWS_SDK_LOAD_CONFIG environment variable which allows
AWS SDK to read ~/.aws/config, and implements the tokenCodeFn
callback which asks the user to enter a MFA code. Credentials are
also cached to avoid creating the object multiple times, which
makes MFA work properly when multiple requests are made.

How can we verify it:

Deploy any project using an AWS profile that has role_arn, source_profile and mfa_serial configured in ~/.aws/config.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

This sets the AWS_SDK_LOAD_CONFIG environment variable which allows
AWS SDK to read ~/.aws/config, and implements the tokenCodeFn
callback which asks the user to enter a MFA code. Credentials are
also cached to avoid creating the object multiple times, which
makes MFA work properly when multiple requests are made.

With these changes Serverless Framework should be compatible with
AWS CLI, so that these instructions for assuming a role work
out-of-the-box:
https://docs.aws.amazon.com/cli/latest/userguide/cli-roles.html
Copy link
Contributor

@dschep dschep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @kennu! Just a small tweak, it'd be better if the cached creds were an instance variable rather than a global. I've tested that it still works that way, so feel free to just click enable "Allow edits from maintainers" for this PR and I'll push my change.


const constants = {
providerName: 'aws',
};

PromiseQueue.configure(BbPromise.Promise);

// Store credentials in this variable to avoid creating them several times (messes up MFA).
let cachedCredentials = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to this.cachedCredentials in the AwsProvider's constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think [x] Allow edits from maintainers is enabled, should I enable something more to make it work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh. You're right, I'm just not used to pushing to other peoples repos. Got it now.

dschep
dschep previously approved these changes Oct 31, 2018
Copy link
Contributor

@dschep dschep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to fail tests. I had some issues replicating the problem locally, and it turns out to only occur if you don't have ~/.aws already, so delete that before running tests and you should be able to replicate the errors locally.

@kennu
Copy link
Contributor Author

kennu commented Oct 31, 2018

I guess the tests are not compatible with environment variable AWS_SDK_LOAD_CONFIG=true. Should I just remove it and have users manually set it? I was hoping to eliminate that step, but I guess it could break backwards compatibility.

@dschep
Copy link
Contributor

dschep commented Nov 1, 2018

I think this solution is probably for the best anyway, don't want to set too many env vars around AWS without the user's input on it.

@kennu
Copy link
Contributor Author

kennu commented Nov 1, 2018

I guess this still needs some kind of test for the interactive MFA query..

@dschep
Copy link
Contributor

dschep commented Nov 1, 2018

Yeah, It'd be preferable if that function had coverage.

@dschep dschep merged commit ccfe5cb into serverless:master Nov 1, 2018
@kennu
Copy link
Contributor Author

kennu commented Nov 2, 2018

Thanks for the merge. I've been trying to figure a way to test readline() (stdin / stdout) and AWS MFA, but it's actually pretty complicated in any meaningful way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants