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

feat: create impersonated service credentials #499

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

viacheslav-rostovtsev
Copy link
Member

feat: create impersonated service credentials
feat: add duplication mechanism to various credentials to support "re-scoping" them for IAM requests

@viacheslav-rostovtsev viacheslav-rostovtsev requested a review from a team as a code owner November 2, 2024 03:36
@viacheslav-rostovtsev viacheslav-rostovtsev force-pushed the dev/virost/impersonated_creds branch 3 times, most recently from 743091e to 2ed3089 Compare November 11, 2024 23:05
Copy link
Member

@dazuma dazuma left a comment

Choose a reason for hiding this comment

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

General question: what kind(s) of credentials are acceptable base credentials for these impersonated credentials? Is it just service account creds, or does it include compute engine creds, or external creds?


describe "duplicates" do
before :example do
Google::Cloud.env.compute_smbios.override_product_name = "Google Compute Engine"
Copy link
Member

Choose a reason for hiding this comment

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

Should probably undo this in an after clause, so it doesn't affect other tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

# and then that claim is exchanged for a short-lived token at an IAMCredentials endpoint.
# The short-lived token and its expiration time are cached.
class ImpersonatedServiceAccountCredentials
ERROR_SUFFIX = <<~ERROR.freeze
Copy link
Member

Choose a reason for hiding this comment

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

Mark this # @private so it's not part of the public interface

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

from IAM Credentials endpoint using the credentials provided.
ERROR

IAM_SCOPE = ["https://www.googleapis.com/auth/iam".freeze].freeze
Copy link
Member

Choose a reason for hiding this comment

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

Should this be # @private also?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

include Google::Auth::BaseClient
include Helpers::Connection

attr_reader :base_credentials
Copy link
Member

Choose a reason for hiding this comment

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

Each of these should have YARD documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

new options
end

def initialize options = {}
Copy link
Member

Choose a reason for hiding this comment

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

Either document this or mark it # @private.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

# to fetch short-lived impersionation access token
# @param impersonation_url [String] the URL to use to impersonate the service account.
# This URL should be in the format:
# https://iamcredentials.{universe_domain}/v1/projects/-/serviceAccounts/{source_sa_email}:generateAccessToken
Copy link
Member

Choose a reason for hiding this comment

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

Put backticks around the URL format string, otherwise YARD will try to linkify it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

# This URL should be in the format:
# https://iamcredentials.{universe_domain}/v1/projects/-/serviceAccounts/{source_sa_email}:generateAccessToken
# where:
# * {universe_domain} is the domain of the IAMCredentials API endpoint (e.g. 'googleapis.com')
Copy link
Member

Choose a reason for hiding this comment

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

Put backticks around anything with literal curly braces, since it is a linkification syntax for YARD.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

# and request short-lived credentials for a service account
# that has the authorization that your use case requires.
#
# @param base_credentials [Object] the authenticated principal that will be used
Copy link
Member

Choose a reason for hiding this comment

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

Are these parameters required? (Seems like they are.) If so, can you note that in the documentation, and also put some checks in the constructor to fail fast if they are not provided?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

spec/googleauth/impersonated_service_account_spec.rb Outdated Show resolved Hide resolved
spec/googleauth/impersonated_service_account_spec.rb Outdated Show resolved Hide resolved
@viacheslav-rostovtsev viacheslav-rostovtsev force-pushed the dev/virost/impersonated_creds branch from ce48b35 to 29e3873 Compare December 24, 2024 00:14
@viacheslav-rostovtsev viacheslav-rostovtsev added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 26, 2024
@viacheslav-rostovtsev
Copy link
Member Author

Do not merge: the case where the source credentials are wrapped in the Google::Auth::Credentials is not covered

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants