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

Rework Service Account Impersonation and remove credentials fields. #3999

Closed

Conversation

upodroid
Copy link
Contributor

@upodroid upodroid commented Sep 21, 2020

Fixes: hashicorp/terraform-provider-google#7332

@danawillow @rileykarson

This PR makes some backward incompatible changes to the provider:

  • Introduces impersonate_service_account and impersonate_service_account_delegates field to the provider. This provides refreshable access token of an impersonated service account and and a significant improvement over the current approach of declaring multiple providers and using access_token datasource.
  • Removes access_token from the provider. This has been replaced by impersonate_service_account.
  • Removes credentials from the provider. Service Account keys should be set via the GOOGLE_APPLICATION_CREDENTIALS environment variable.

If this approach is accepted, I also want to do the following:

If this PR is for Terraform, I acknowledge that I have:

  • Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

provider: removed `access_token` field from the provider. This has been replaced by `impersonate_service_account`. 
provider: removed `credentials` field from the provider. Service Account keys should be set via the `GOOGLE_APPLICATION_CREDENTIALS` environment variable
provider: introduced `impersonate_service_account` and `impersonate_service_account_delegates` fields to the provider. This provides refreshable access token of an impersonated service account and and a significant improvement over the current approach of declaring multiple providers and using access_token datasource.

@google-cla google-cla bot added the cla: yes label Sep 21, 2020
@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@rileykarson, please review this PR or find an appropriate assignee.

@upodroid
Copy link
Contributor Author

I tested this locally.

I see the correct email in the openid test and the audit logs of my project show the correct identities impersonating and carrying out operations.

export GOOGLE_IMPERSONATE_SERVICE_ACCOUNT=terraform@REDACTED.iam.gserviceaccount.com 
TF_LOG=debug make testacc TEST=./google-beta TESTARGS='-run=TestAccComputeRegions_basic' > debug.log
TF_LOG=debug make testacc TEST=./google-beta TESTARGS='-run=TestAccDataSourceGoogleClientOpenIDUserinfo_basic' > debug.log
TF_LOG=debug make testacc TEST=./google-beta TESTARGS='-run=TestAccDataSourceGoogleServiceAccountIdToken_impersonation' > debug.log
TF_LOG=debug make testacc TEST=./google-beta TESTARGS='-run=TestAccDataSourceGoogleServiceAccountAccessToken_basic' > debug.log

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 52 insertions(+), 181 deletions(-))
Terraform Beta: Diff ( 9 files changed, 52 insertions(+), 181 deletions(-))
TF Conversion: Diff ( 1 file changed, 16 insertions(+), 40 deletions(-))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 9 files changed, 55 insertions(+), 176 deletions(-))
Terraform Beta: Diff ( 9 files changed, 55 insertions(+), 176 deletions(-))
TF Conversion: Diff ( 1 file changed, 16 insertions(+), 40 deletions(-))

@danawillow
Copy link
Contributor

A general note around major breaking changes like this: there's a big difference between "does a change work" and "does a change solve the right problem", and a PR review is a good place for the first, but less of a good place for the second.

On our team, this would typically come in the form of a design doc (usually in google docs, but for a community member I think a GH issue would be fine) with the following sections: Objective, Background, Overview, Detailed Design, Alternatives Considered. The important part is being able to weigh the pros of making the change against the cons (in this case, having people who were successfully using one authentication method needing to migrate to a different one in order to get access to features that get added in version 4.0.0+). If that's work you want to take on then we can look at it afterward, but for full honesty, we probably won't get around to it until we start properly doing 4.0.0 planning.

My suggestion would be to do the service account impersonation as a standalone PR (which we should be able to review in our normal time frames) instead of having the breaking credentials change piggybacking along.

@upodroid
Copy link
Contributor Author

I'm happy to do the legwork to explain the change fully. The current approach works because I seldomly see plans or applies that run for more than 1 hour but I do want to simplify and improve the auth methods in the next major release.

I'll open an issue to explain this fully. One of the things I find frustrating is that is pretty much every tutorial for terraform on GCP instructs users download a service account key, specify the credentials field in the provider bloc and use that instead of using User ADCs generated by gcloud. Then I have to tell people that the tutorials that you were watching are wrong and that is not what you do for real. Best practice is to leave that out and load the credentials from the runtime environment as specified in the Google docs.

Perfect examples: https://github.com/rancher/terraform-modules/blob/master/example_ha/gce/management-cluster/main.tf and https://github.com/infracloudio/rancher-tf-gce/blob/master/gce.tf .

@upodroid
Copy link
Contributor Author

It is done. I'll leave this open and see how that issue goes. I'll open a separate PR to add the impersonation.

@joe-a-t
Copy link

joe-a-t commented Sep 24, 2020

If there is any way to get at least just the service account impersonation in, that would solve a big security vulnerability as documented in hashicorp/terraform-provider-google#7064 (comment) and shouldn't require waiting for a major release that's not coming any time soon

@upodroid
Copy link
Contributor Author

I'm planning on doing that tomorrow

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

Successfully merging this pull request may close these issues.

Proposal: Remove credentials and access_token fields from the terraform provider.
5 participants