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

Mechanism/Hook to Provide MFA Token Code For AssumeRole Authentication #2529

Closed
phene opened this issue Jun 3, 2021 · 9 comments · Fixed by #2663
Closed

Mechanism/Hook to Provide MFA Token Code For AssumeRole Authentication #2529

phene opened this issue Jun 3, 2021 · 9 comments · Fixed by #2663
Labels
feature-request A feature should be added or improved.

Comments

@phene
Copy link

phene commented Jun 3, 2021

Today, providing a token_code for assuming a role with an MFA requirement has to be done manually during the client creation. It would be great if a mechanism could be added to the Aws config to support a way to dynamically fetch the token. Perhaps the ability to provide a lambda that gets called?

This would allow users to either invoke a library, executable, or even prompt for STDIN in order to retrieve the appropriate token code.

@phene phene added the feature-request A feature should be added or improved. label Jun 3, 2021
@alextwoods
Copy link
Contributor

Can you provide an example of how your currently using the token_code and the resulting credentials?

I believe this is something we could support by providing some sort of callback on expiration w/ the RefreshingCredentials module potentially.

@phene
Copy link
Author

phene commented Jun 3, 2021

I've actually implemented my own Credentials class which performs this refreshing behavior. It currently just prompts for the MFA token via STDIN, but I could imagine extending it to support other mechanisms for retrieving it. I would like to just rip it all out in favor of leveraging CredentialProviderChain, but I can't without proper MFA token support.

@mullermp
Copy link
Contributor

mullermp commented Jun 3, 2021

Thanks for opening this! I think we could have a credential provider class that prompts for tokens (or from other sources). Did you want to contribute that back to the SDK? Unless we have a mechanism to fail fast, I don't think we can introduce prompting into the credential provider chain because it could be blocking for other applications.

@phene
Copy link
Author

phene commented Jun 3, 2021

I'll consider a sample implementation, since my existing implementation would not be suitable for integration into the core SDK. I don't think the core itself should include prompting, just the hook/callback to allow users to do so.

@phene
Copy link
Author

phene commented Jun 3, 2021

This is likely incomplete, but it's along the lines of what I'm thinking:

diff --git a/gems/aws-sdk-core/lib/aws-sdk-core/assume_role_credentials.rb b/gems/aws-sdk-core/lib/aws-sdk-core/assume_role_credentials.rb
index f7fa2355b..0d8cfceb1 100644
--- a/gems/aws-sdk-core/lib/aws-sdk-core/assume_role_credentials.rb
+++ b/gems/aws-sdk-core/lib/aws-sdk-core/assume_role_credentials.rb
@@ -26,12 +26,15 @@ class AssumeRoleCredentials
     # @option options [Integer] :duration_seconds
     # @option options [String] :external_id
     # @option options [STS::Client] :client
+    # @option options [Aws::TokenProvider] :token_provider
     def initialize(options = {})
       client_opts = {}
       @assume_role_params = {}
       options.each_pair do |key, value|
         if self.class.assume_role_options.include?(key)
           @assume_role_params[key] = value
+        elsif key == :token_provider && options['mfa_serial']
+          @assume_role_params['token_code'] = value.token_code
         else
           client_opts[key] = value
         end
diff --git a/gems/aws-sdk-core/lib/aws-sdk-core/shared_config.rb b/gems/aws-sdk-core/lib/aws-sdk-core/shared_config.rb
index 6f83856d0..59d78a2b1 100644
--- a/gems/aws-sdk-core/lib/aws-sdk-core/shared_config.rb
+++ b/gems/aws-sdk-core/lib/aws-sdk-core/shared_config.rb
@@ -10,6 +10,9 @@ class SharedConfig
     # @return [String]
     attr_reader :profile_name
 
+    # @return [Aws::TokenProvider]
+    attr_reader :token_provider
+
     # Constructs a new SharedConfig provider object. This will load the shared
     # credentials file, and optionally the shared configuration file, as ini
     # files which support profiles.
@@ -47,6 +50,7 @@ def initialize(options = {})
       @config_enabled = options[:config_enabled]
       @credentials_path = options[:credentials_path] ||
                           determine_credentials_path
+      @token_provider = options[:token_provider]
       @parsed_credentials = {}
       load_credentials_file if loadable?(@credentials_path)
       if @config_enabled
@@ -210,6 +214,7 @@ def assume_role_from_profile(cfg, profile, opts, chain_config)
             opts[:duration_seconds] ||= prof_cfg['duration_seconds']
             opts[:external_id] ||= prof_cfg['external_id']
             opts[:serial_number] ||= prof_cfg['mfa_serial']
+            opts[:token_provider] ||= chain_config[:token_provider]
             opts.delete(:source_profile) # Cleanup
             AssumeRoleCredentials.new(opts)
           else
diff --git a/gems/aws-sdk-core/lib/aws-sdk-core/token_provider.rb b/gems/aws-sdk-core/lib/aws-sdk-core/token_provider.rb
new file mode 100644
index 000000000..7ee91b718
--- /dev/null
+++ b/gems/aws-sdk-core/lib/aws-sdk-core/token_provider.rb
@@ -0,0 +1,16 @@
+module Aws
+  class TokenProvider
+    def token_code
+      raise NotImplemented
+    end
+  end
+end
+
+module Aws
+  require 'io/console'
+  class StdinTokenProvider < Aws::TokenProvider
+    def token_code
+      $stdin.getpass("\nEnter the MFA token code: ")
+    end
+  end
+end

@alextwoods
Copy link
Contributor

This looks pretty reasonable but a few general questions/comments.

Getting the token_provider through SharedConfig may not quite make sense - this class is primarily for getting configuration from the shared config files (ie ~/.aws/config), but I assume we'd only be setting the token_provider as code through Aws.config? Are there other ways we'd want to configure it?

I think it likely makes for the token_provider to accept a proc/callable rather than creating an interface and implementations.

Finally a question around caching/multiple clients. The credentials resolution chain is not cached in anyway, every time a new client is created, the credentials resolution is run and a new instance of the credentials are created. In this case, the token_provider would be called again, potentially prompting the user for another token. Does this make sense? Would we need to provide some level of caching for this to be useful for users? One solution could be to add the token_provider to AssumeRoleCredentials but not to any global/shared config and allow users to create/manage their own single/global credentials object that is passed to any clients they are creating. @phene - What do you think?

@phene
Copy link
Author

phene commented Dec 22, 2021

It might even make sense to just support this like the ProcessCredentials. Seems like something the whole CLI/SDK ecosystem could benefit from. Just allow people to roll their own Token Code retrieval.

@phene
Copy link
Author

phene commented Dec 23, 2021

A few use cases that could be supported with the above method:

  • External program prompts user for their MFA token code
  • External program reaches into a password manager to get the token code
  • External program calls a web service to trigger a push notification on a mobile device. Acceptance of the push notification allows the web service to generate a token and deliver it back to the external program.

@github-actions
Copy link

⚠️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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@phene @mullermp @alextwoods and others