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(auth): add support for service account impersonation #665

Closed

Conversation

m1racoli
Copy link
Contributor

Summary

Added the arguments target_principal and delegates which optionally define an impersonation chain for the given target principal to the Token class and related clients.

The request to generate the impersonated access token is made via the IAM credentials API method projects.serviceAccounts.generateAccessToken.

Test skeletons have been added. Those tests are currently skipped as
they require additional setup of the testing environment with additional
service accounts which are to be impersonated:

  • sa_a to be impersonated by the base service account
  • sa_b to be impersonated by sa_a via delegation

closes #421

Added the arguments `target_principal` and `delegates` which optionally
define an impersonation chain for the given target principal to the
Token class and related clients.

The request to generate the impersonated access token is made via the
IAM credentials API method [projects.serviceAccounts.generateAccessToken](https://cloud.google.com/iam/docs/reference/credentials/rest/v1/projects.serviceAccounts/generateAccessToken).

Test skeletons have been added. Those tests are currently skipped as
they require additional setup of the testing environment with additional
service accounts which are to be impersonated:

- `sa_a` to be impersonated by the base service account
- `sa_b` to be impersonated by `sa_a` via delegation
@m1racoli m1racoli requested review from eddiedialpad and removed request for a team December 22, 2023 15:47
@m1racoli
Copy link
Contributor Author

I understand that I added changes beyond the auth package. Let me know if you'd like this PR to focus only on the auth package.

Copy link

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

LGTM, This PR is highly anticipated in the Apache Airflow community :)

@m1racoli
Copy link
Contributor Author

m1racoli commented Jan 2, 2024

Thanks! :) Do we have an ETA for this to get merged or is something particular blocking the PR?

@Lee-W
Copy link

Lee-W commented Jan 17, 2024

Hi @TheKevJames , we're wondering whether gcloud-aio is interested in this feature and if so, is there any blocker of suggestions in this PR? Thanks!

@@ -70,12 +71,16 @@ def __init__(
service_file: Optional[Union[str, IO[AnyStr]]] = None,
session: Optional[Session] = None, token: Optional[Token] = None,
api_root: Optional[str] = None,
target_principal: Optional[str] = None,

Choose a reason for hiding this comment

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

do we need a corresponding change in bigquery.Job (and the other BigQuery classes) to pass in these values to the BigqueryBase constructor?

def __init__(
self, job_id: Optional[str] = None, project: Optional[str] = None,
service_file: Optional[Union[str, IO[AnyStr]]] = None,
session: Optional[Session] = None, token: Optional[Token] = None,
api_root: Optional[str] = None,
) -> None:
self.job_id = job_id
super().__init__(
project=project, service_file=service_file,
session=session, token=token, api_root=api_root,
)

@kaxil
Copy link

kaxil commented Jan 26, 2024

Gentle nudge @TheKevJames @shaundialpad

@TheKevJames
Copy link
Member

Hey all, sorry for the radio silence on this one, taking a look now! Thanks for posting this.

@TheKevJames
Copy link
Member

Ok, I've merged this (here) and will be releasing shortly. Thanks for the contribution! Overall this looked great, just needed to tweak some linting things. Thank you especially for the integration test skeleton, that was very easy to work with.

@TheKevJames
Copy link
Member

Oh, just to add -- I needed to pull out the changes to other libraries to avoid order-of-operations, but will introduce them later. For now, you can build a Token instance with the delegation and pass it to any other library class, you just can't let it pass through those values.

Now that the Token class has gotten so many params, I'm starting to think it might be worth making it mandatory to have users create the Token, frankly -- especially with how many places we would need to plumb everything, as you pointed out for BQ Jobs. I'll look into this and see about pushing out a follow-up.

@m1racoli
Copy link
Contributor Author

Thank you

Oh, just to add -- I needed to pull out the changes to other libraries to avoid order-of-operations, but will introduce them later. For now, you can build a Token instance with the delegation and pass it to any other library class, you just can't let it pass through those values.

Yeah, that makes sense.

Now that the Token class has gotten so many params, I'm starting to think it might be worth making it mandatory to have users create the Token, frankly -- especially with how many places we would need to plumb everything, as you pointed out for BQ Jobs. I'll look into this and see about pushing out a follow-up.

Yes, I had similar thoughts. You either provide a token or assume the default credentials. That would align with the methodology in google libs where the credentials object is a container for every aspect of the credentials and needs to be configured as such. I also wonder if the project ID needs to be part of the token.

@m1racoli m1racoli deleted the service-account-impersonation branch February 16, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support for Impersonated Credentials
6 participants