feat: Add AWS Workload Identity Federation Support #386

rbclark · 2022-06-21T20:27:49Z

This adds AWS Workload Identity Federation support for AWS EC2 -> Google Cloud.

I understand this PR is likely not in it's final stage, but I was hoping to get some feedback on it from the maintainers on what would still be necessary for it to be fully acceptable.

Outstanding Questions:

This currently has one failing rubocop finding, lib/googleauth/credentials_loader.rb:24:5: C: Metrics/ModuleLength: Module has too many lines. [113/110], considering the nature of this file, would it be acceptable to ignore this finding?
Are there any other tests that I am currently missing which would be required to make this acceptable?

Closes #354

google-cla · 2022-06-21T20:27:53Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

hanikesn · 2022-08-04T14:47:59Z

Any updates on this?

rbclark · 2022-08-04T14:56:28Z

Any updates on this?

I'm still tracking this thread and willing to make any changes required to get it merged. For now I've just been using it by overriding my Gemfile to point at my fork of the code, although I'd suggest you make your own fork with this copy of the code if you take that route since my branch could change at any time.

bajajneha27 · 2022-08-25T05:52:13Z

Hi @rbclark
Sorry that reviewing this PR is taking long. Please know that we're working on getting this reviewed. Thank you for your patience.

rbclark · 2022-08-25T21:09:20Z

@bajajneha27 Not a problem, thanks! Let me know if there's anything else that needs to be done on my end!

hanikesn · 2022-09-20T09:38:00Z

@bajajneha27 checking in whether the review is still in progress. Or if it would help giving this higher priority if we talk to our GCP reps. Thanks!

bajajneha27 · 2022-09-21T08:36:38Z

@bajajneha27 checking in whether the review is still in progress. Or if it would help giving this higher priority if we talk to our GCP reps. Thanks!

Hi @hanikesn
It's still in progress. Please be assured that we're taking it as a higher priority.

ScruffyProdigy · 2022-09-21T20:15:10Z

lib/googleauth/external_account.rb

+    # This module handles the retrieval of credentials from Google Cloud
+    # by utilizing the AWS EC2 metadata service and then exchanging the
+    # credentials for a short-lived Google Cloud access token.
+    class AwsCredentials


I think it would make sense to move the AWS specific code to a separate file. It's very likely that the ExternalAccount credentials will also be extended to handle File based external account credentials, URL based external account credentials, and executable external account credentials, and this file will quickly become unmaintable

ScruffyProdigy · 2022-09-21T20:26:07Z

lib/googleauth/external_account.rb

+        raise "a json file is required for external account credentials" unless json_key_io
+        user_creds = read_json_key json_key_io
+
+        AwsCredentials.new(


It seems like there's some tight coupling between this class and the AWS class. It might make sense to move some of the amazon specific logic into the AwsCredentials class

I've gone ahead and moved the AWS region information into the AWS class as described below. I think the rest of these are somewhat generic based on the reference code I used from the other google auth libraries but I am still open to suggestions (python example)

ScruffyProdigy · 2022-09-21T20:52:00Z

lib/googleauth/external_account.rb

+          token_url: user_creds["token_url"],
+          credential_source: user_creds["credential_source"],
+          service_account_impersonation_url: user_creds["service_account_impersonation_url"],
+          region: ENV[CredentialsLoader::AWS_REGION_VAR] || ENV[CredentialsLoader::AWS_DEFAULT_REGION_VAR]


Wouldn't this make more sense to move into the region method of AwsCredentials?

ScruffyProdigy · 2022-09-21T20:53:19Z

lib/googleauth/external_account.rb

+
+        @environment_id = @credential_source["environment_id"]
+        @region_url = @credential_source["region_url"]
+        @credential_source_url = @credential_source["url"]


I think the purpose of this variable is more clear as @cred_verification_url

ScruffyProdigy · 2022-09-21T20:55:18Z

lib/googleauth/external_account.rb

+      end
+
+      def get_impersonated_access_token token, options = {}
+        c = options[:connection] || Faraday.default_connection


You use options[:connection] || Faraday.default_connection a lot. Move this to a new connection method, and then you can just cay connection.post or connection.get

I did this to try to match the rest of the application in the hopes that in the future it could be changed all at once. I've gone ahead and moved this into a helper now and tried to keep it generic so that the other classes in this library which have calls to options[:connection] can reuse it as well. One thing that would likely make this easier in this codebase at least would be if the options were set at a class level instead of being passed into each individual method.

Hmmm, yeah, I agree. I'll have to talk with the rest of the team to see if we can refactor some of these

ScruffyProdigy · 2022-09-21T21:02:15Z

lib/googleauth/oauth2/sts_client.rb

+        # @param [String] token_exchange_endpoint
+        #  The token exchange endpoint.
+        def initialize options = {}
+          @token_exchange_endpoint = options[:token_exchange_endpoint]


Maybe we should raise an error if there's no token exchange endpoint provided, as the class won't work otherwise

ScruffyProdigy · 2022-09-21T21:13:15Z

lib/googleauth/oauth2/sts_client.rb

+
+          c = options[:connection] || Faraday.default_connection
+
+          headers = URLENCODED_HEADERS.dup.merge(options[:additional_headers] || {})


Could we either add the ability to add authentication to the headers, or a TODO reminder that this will need to be done?

ScruffyProdigy · 2022-09-21T23:05:51Z

lib/googleauth/external_account.rb

+        notify_refresh_listeners
+      end
+
+      def notify_refresh_listeners


Isn't this method the same as the one defined in the client?

ScruffyProdigy · 2022-09-21T23:07:31Z

lib/googleauth/base_client.rb

@@ -0,0 +1,76 @@
+# Copyright 2015 Google, Inc.


ScruffyProdigy · 2022-09-21T23:09:41Z

lib/googleauth/base_client.rb

+    # BaseClient is a class used to contain common methods that are required by any
+    # Credentials Client, including AwsCredentials, ServiceAccountCredentials,
+    # and UserRefreshCredentials.
+    module BaseClient


Do we need this module? Is there a reason that ExternalAccountCredentials can't extend from Signet::Oauth2::Client like the other credential classes in this library, instead of selecting a few of the methods from this module, and creating a superclass for it? It seems like it would just be easier to overwrite the methods from the client that don't work

My thinking here was that the ExternalAccountCredentials uses a completely different OAuth2 flow that is not supported by Signet. Since Signet doesn't support token exchange I figured it would just add confusion for other developers if I pulled in Signet and didn't use any of it's features. The current implementation is the most clear way i could think of to avoid confusion around what Signet does and doesn't do while still DRYing everything up. If you would prefer I just paste everything into the Signet file and include it everywhere I can go ahead and make that change and try it out, just figured I would explain my thinking and get your feedback before making the change.

I think this makes sense. Let's mention Signet::Oauth2::Client in the comments, and mention that this has been created as a superclass for Signet for Credentials that don't need all of Signet's features

Is this more reasonable? a89e880

rbclark

Thank you for reviewing this! I believe I have corrected everything you noted except for the moving of everything into signet.rb. If after my explanation of my thought process there you still want that moved over please let me know and I will do so!

rbclark · 2022-09-30T15:31:42Z

lib/googleauth/external_account.rb

+      end
+
+      def get_impersonated_access_token token, options = {}
+        c = options[:connection] || Faraday.default_connection


I did this to try to match the rest of the application in the hopes that in the future it could be changed all at once. I've gone ahead and moved this into a helper now and tried to keep it generic so that the other classes in this library which have calls to options[:connection] can reuse it as well. One thing that would likely make this easier in this codebase at least would be if the options were set at a class level instead of being passed into each individual method.

rbclark · 2022-09-30T17:14:17Z

lib/googleauth/external_account.rb

+        raise "a json file is required for external account credentials" unless json_key_io
+        user_creds = read_json_key json_key_io
+
+        AwsCredentials.new(


I've gone ahead and moved the AWS region information into the AWS class as described below. I think the rest of these are somewhat generic based on the reference code I used from the other google auth libraries but I am still open to suggestions (python example)

rbclark · 2022-09-30T18:06:19Z

lib/googleauth/base_client.rb

+    # BaseClient is a class used to contain common methods that are required by any
+    # Credentials Client, including AwsCredentials, ServiceAccountCredentials,
+    # and UserRefreshCredentials.
+    module BaseClient


My thinking here was that the ExternalAccountCredentials uses a completely different OAuth2 flow that is not supported by Signet. Since Signet doesn't support token exchange I figured it would just add confusion for other developers if I pulled in Signet and didn't use any of it's features. The current implementation is the most clear way i could think of to avoid confusion around what Signet does and doesn't do while still DRYing everything up. If you would prefer I just paste everything into the Signet file and include it everywhere I can go ahead and make that change and try it out, just figured I would explain my thinking and get your feedback before making the change.

DevNico · 2022-10-11T18:55:11Z

Any updates on when this will be available?

shrkw · 2022-10-17T08:27:14Z

hi @ScruffyProdigy would you review the updates?

…impersonated credential

…st compatible with apply_auth_examples. ExternalAccount was not fully compatible with the other providers, this brings it in line and moves some common methods between the other provides and ExternalAccount into a module which is now included by the AWSClient provider and Signet::Auth::Client

…external account, create dedicated Connection helper module, other misc PR cleanup

ScruffyProdigy

This is looking good!

ScruffyProdigy · 2022-10-28T20:53:02Z

lib/googleauth/external_account.rb

+        raise "a json file is required for external account credentials" unless json_key_io
+        user_creds = read_json_key json_key_io
+
+        Google::Auth::ExternalAccount::AwsCredentials.new(


There are a few different types of external account credential files that exist right now. Let's make sure the file is intended to be AWS credentials before we use that class to create them, and raise an "unknown credential" error until we've implemented them. Use either of these methods:

In GoLang, we make sure that the environment ID of the credential source is "AWS1" before we create AWS credentials (and could be AWS2, AWS3, etc for future versions of these credentials)

In Python, we make sure that the subject token type of the credential source is "urn:ietf:params:aws:token-type:aws4_request"

Fixed in 994456a

ScruffyProdigy · 2022-10-28T21:03:48Z

lib/googleauth/signet.rb

    # Signet::OAuth2::Client creates an OAuth2 client
    #
    # This reopens Client to add #apply and #apply! methods which update a
    # hash with the fetched authentication token.
    class Client
+      include Google::Auth::BaseClient
+
      def configure_connection options


Would something like this in the base_client make it easier to not need to pass around the options for the connection?

The biggest issue I see is that configure_connection returns self and not a Faraday::Connection. I think the core issue is that signet does this internally (https://github.com/googleapis/signet/blob/main/lib/signet/oauth_2/client.rb#L993) and so the code here seems to just be setting up for handing off to signet. The code for external accounts on the other hand needs to actually handle the default case as well and is doing everything instead of just handing off to signet.

…redentials

rbclark · 2022-10-31T13:37:14Z

@ScruffyProdigy Sorry I had to correct one line too long error, could you re-approve the workflow run?

lsirac

Thanks for adding this @rbclark 🙌

Note this is missing a lot of tests. See the test coverage in the AWS portion of the NodeJS WIF PR.

lsirac · 2022-11-09T22:00:26Z

lib/googleauth/external_account.rb

+        Google::Auth::ExternalAccount::AwsCredentials.new(
+          audience: user_creds["audience"],
+          scope: scope,
+          subject_token_type: user_creds["subject_token_type"],


The token_url and service_account_impersonation_url need to be validated.

See the java implementation: https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java#L219

lsirac · 2022-11-09T22:01:25Z

lib/googleauth/external_account/aws_credentials.rb

+          @credential_source = options[:credential_source] || {}
+          @service_account_impersonation_url = options[:service_account_impersonation_url]
+          @environment_id = @credential_source["environment_id"]
+          @region_url = @credential_source["region_url"]


AWS specific URLs should be validated, see googleapis/google-auth-library-java#1079

lsirac · 2022-11-09T22:13:21Z

lib/googleauth/external_account/aws_credentials.rb

+            }
+          end
+
+          role_name = fetch_metadata_role_name options


We need to support IMDSv2, see https://github.com/googleapis/google-auth-library-java/pull/850/files.

bajajneha27 · 2023-03-24T06:59:46Z

We've already covered this in PR#418.

hermanbanken · 2023-03-24T07:01:12Z

I wouldn't call it "already", but good news, thanks!

rbclark requested a review from a team as a code owner June 21, 2022 20:27

rbclark force-pushed the add-external-account-creds-pr-version branch 2 times, most recently from 9e712c5 to c294764 Compare June 30, 2022 21:09

ScruffyProdigy suggested changes Sep 21, 2022

View reviewed changes

DeanBrunt mentioned this pull request Sep 22, 2022

Add support for Workload Identity Federation in Stackdriver output plugin fluent/fluent-bit#6083

Closed

rbclark requested a review from a team as a code owner September 30, 2022 18:43

rbclark force-pushed the add-external-account-creds-pr-version branch 2 times, most recently from 00ca080 to 4695994 Compare September 30, 2022 18:49

rbclark changed the title ~~Add AWS Workload Identity Federation Support~~ feat: Add AWS Workload Identity Federation Support Sep 30, 2022

rbclark commented Sep 30, 2022

View reviewed changes

rbclark and others added 3 commits October 28, 2022 16:34

feat: Add External Account Credential support as well as support for …

5dbc165

…impersonated credential

chore: Move AWS Credentials to their own file, do not pass role from …

ce07c11

…external account, create dedicated Connection helper module, other misc PR cleanup

rbclark force-pushed the add-external-account-creds-pr-version branch from 4695994 to ce07c11 Compare October 28, 2022 20:35

chore: Update BaseClient documentation comment

a89e880

ScruffyProdigy reviewed Oct 28, 2022

View reviewed changes

ScruffyProdigy approved these changes Oct 29, 2022

View reviewed changes

chore: Ensure the subject token type matches AWS before building AwsC…

94e1935

…redentials

rbclark force-pushed the add-external-account-creds-pr-version branch from 994456a to 94e1935 Compare October 31, 2022 13:36

lsirac reviewed Nov 9, 2022

View reviewed changes

bajajneha27 closed this Mar 24, 2023


		c = options[:connection] \|\| Faraday.default_connection

		headers = URLENCODED_HEADERS.dup.merge(options[:additional_headers] \|\| {})

feat: Add AWS Workload Identity Federation Support #386

feat: Add AWS Workload Identity Federation Support #386

Conversation

rbclark commented Jun 21, 2022

google-cla bot commented Jun 21, 2022

hanikesn commented Aug 4, 2022

rbclark commented Aug 4, 2022

bajajneha27 commented Aug 25, 2022

rbclark commented Aug 25, 2022

hanikesn commented Sep 20, 2022

bajajneha27 commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DevNico commented Oct 11, 2022

shrkw commented Oct 17, 2022

ScruffyProdigy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbclark commented Oct 31, 2022

lsirac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajajneha27 commented Mar 24, 2023

hermanbanken commented Mar 24, 2023