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

fix: remove instanceUser iam binding from the postgresql module #382

Merged

Conversation

ykyr
Copy link
Contributor

@ykyr ykyr commented Dec 2, 2022

It's a breaking change. This PR fixes #381

The recommended way for graceful migration:

  • Setup iam_binding outside the PostgreSQL module. Example:
locals {
  iam_user_emails = ["test-example-sa@proj-id.iam.gserviceaccount.com", "john.doe@gmail.com"]

  iam_users = [for iu in local.iam_user_emails : {
    email         = iu,
    is_account_sa = trimsuffix(iu, "gserviceaccount.com") == iu ? false : true
  }]
}

resource "google_project_iam_member" "iam_binding" {
  for_each = {
    for iu in local.iam_users :
    "${iu.email} ${iu.is_account_sa}" => iu
  }
  project = var.project_id
  role    = "roles/cloudsql.instanceUser"
  member = each.value.is_account_sa ? (
    "serviceAccount:${each.value.email}"
    ) : (
    "user:${each.value.email}"
  )
}
  • Move resources in the terraform state:
terraform state mv 'module.test_example.google_project_iam_member.iam_binding["test-example-sa@proj-id.iam.gserviceaccount.com true"]' 'google_project_iam_member.iam_binding["test-example-sa@proj-id.iam.gserviceaccount.com true"]'
terraform state mv 'module.test_example.google_project_iam_member.iam_binding["john.doe@gmail.com false"]' 'google_project_iam_member.iam_binding["john.doe@gmail.com false"]'

@ykyr ykyr requested a review from a team as a code owner December 2, 2022 14:40
@ykyr
Copy link
Contributor Author

ykyr commented Dec 2, 2022

Not sure why tests failed

@g-awmalik
Copy link
Contributor

Thanks for PR @ykyr . Although I understand your requirement, I have a concern with having the same service account accessing multiple databases. The way the module is written is that it assumes there will be a unique SA or a local user for each database to harden security. Consider a situation where a bad actor gets access to the SA, this will allow them to have access to multiple databases. On the flip side, if you want to shutdown a compromised SA, doing so will interrupt access to all databases potentially bringing down multiple applications.

Can you explain why you're not creating unique SAs per DB especially within one project?

@ykyr
Copy link
Contributor Author

ykyr commented Dec 13, 2022

Hi @g-awmalik,

On the flip side, if you want to shutdown a compromised SA, doing so will interrupt access to all databases potentially bringing down multiple applications.

Then SA follows a bad practice, doesn't it? Sharing a single service account across multiple applications can complicate the management of the service account

In our organization, the setup is very simple. Every application service has an identity that is represented by
a Google Service Account (GSA) in the GCP. Respectively, the application needs to access various GCP resources (buckets, databases, pub/sub, etc..) to function properly. All you need to do is to grant the required permission to the GSA. For the record, applications are running in the GKE, and we use WorkloadIdentity.

For example, an application named foo-bar will have GSA foo-bar@project-id.iam.gserviceaccount.com which represents foo-bar application in the GCP.

  • When my application needs to access 2 cloudsql databases, 3 buckets, 2 pub/subs... - I would just grant permission to those GCP resources for foo-bar@project-id.iam.gserviceaccount.com, without any additional modifications on the application side.
  • If the application is compromised - I can shutdown the foo-bar@project-id.iam.gserviceaccount.com GSA, and I would be sure that the bad actor won't have any additional access. I would also be sure that I didn't affect any other application, by shutting down the GSA.
  • At any point in time when I check buckets or Cloudsql instances - I can clearly see who has access there.

I provided the example with the application GSA, but let's take a look on user peter@gmail.com:

  • Regular users aren't generated per Cloudsql instance. peter@gmail.com represents Peter's identity in GCP.
  • Peter is an engineer that has access to multiple Cloudsql instances in the GCP project.
  • Once the DB owner revoked access for peter@gmail.com from one of the Cloudsql databases that use the current terraform-google-sql-db v13.0.1 module - Peter will lose IAM connection permission to all other Cloudsql instances too.

Few more thoughts:

  • I don't think that's a good practice to keep project-wide IAM permission grants in the module, since they can be easily overwritten outside of the module by mistake, and the access can be revoked the same way.
  • As was mentioned in the #381, unfortunately, it's not possible to configure IAM permission at the instance level itself.
  • Even if some companies are following the use-case model that you described (generate GSA per Cloudsql instance), the module implementation shouldn't limit the usage just to that.
  • If I would generate GSA per Cloudsql instance - it won't be just one SA. It will be multiple GSAs, because each will need different access permissions inside the database (admin, ro, rw, rw per schema, etc...). The amount of such SA will grow exponentially with the number of Cloudsql instances, which will become a nightmare to manage.

@g-awmalik
Copy link
Contributor

@ykyr - I agree with your sentiment around IAM users; it is in fact unreasonable to think that only one user can access one instance at time, that won't work.

However, I still think you should consider have a unique SA per application. Now if your application stack consists of multiple GCP resources, its fine to have that same SA access those resources because they are all part of the stack. Things get complicated, as suggested by the best practice link you quoted, when you start sharing that SA across applications which is not recommended.

To summarize, I'll accept this change since your concern is valid for IAM users. Do you mind creating a upgrade doc similar to this that upgrade process you have in your comments?

Thanks,

@ykyr
Copy link
Contributor Author

ykyr commented Dec 15, 2022

@g-awmalik I added the upgrade doc as you suggested, and named it as a 14.x release. Please let me know if that's sufficient.

@jawnsy
Copy link
Contributor

jawnsy commented Dec 16, 2022

Something else to consider, if you're using Google Groups to handle RBAC, to extend @ykyr's example:

  • Assuming that peter@gmail.com is part of an engineering@company.com Google Group

  • peter@gmail.com needs to be added to the database, because Cloud SQL does not support granting access to groups

  • The module, as it exists today, would add peter@gmail.com with cloudsql.instanceUser permissions to the project

    However, in reality, at the project level, we probably want to instead add cloudsql.instanceUser and cloudsql.client for the engineering@company.com group, to express a goal: engineers can use databases in this project, and connect to any database using the Cloud SQL Auth Proxy. engineers additionally must be a listed IAM user to use IAM authentication (if we also enable Connector Enforcement, then we prevent the use of password authentication and ensure all connections are encrypted, and also audit logged with Google Cloud principal information)

Copy link
Contributor

@g-awmalik g-awmalik left a comment

Choose a reason for hiding this comment

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

LGTM

@comment-bot-dev
Copy link

@ykyr
Thanks for the PR! 🚀
✅ Lint checks have passed.

@g-awmalik g-awmalik merged commit cc39074 into terraform-google-modules:master Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgresql module grants GCP Project permission (iam_binding) which can cause the outage on destroy
4 participants