infra/gcp/gcs: tighten private gcs bucket iam

By default, GCS will create an IAM binding on new GCS buckets that gives
members of roles/editor for the project  roles/storage.legacyBucketOwner
for the bucket. roles/storage.legacyBucketOwner contains
storage.buckets.setIamPolicy, but not storage.objects.get.

This means someone with roles/editor on the project could grant themselves
access to read bucket contents that they aren't supposed to be able to read.

Given that roles/editor has no *.setIamPolicy permissions for other
service resources, this seems like a security gap that should be closed.

Ideally we would do this in _ensure_gcs_bucket. However, removing this
role means removing other (possibly needed) permissions that may be used
by GCP service agent service accounts (e.g. App Engine, GCR, GCE):
- storage.buckets.get
- storage.multipartUploads.(abort|create|list|listParts)
- storage.objects.(create|delete|list)

So for now we constrain to "private" gcs buckets that are currently only used
by humans to store terraform state containing potentially sensitive info

infra/gcp/lib_gcs.sh

@@ -68,7 +68,25 @@ function ensure_private_gcs_bucket() {
     local bucket="$2"
 
     _ensure_gcs_bucket "${project}" "${bucket}"
-    gsutil iam ch -d allUsers "${bucket}"
+    ensure_removed_gcs_role_binding "${bucket}" "allUsers" "objectViewer"
+    # roles/storage.legacyBucketOwner contains storage.buckets.setIamPolicy,
+    # but not storage.objects.get. This means someone with roles/editor on the
+    # project could grant themselves access to read bucket contents that they
+    # aren't supposed to be able to read.
+    #
+    # Given that roles/editor has no *.setIamPolicy permissions for other
+    # service resources, this seems like a security gap that should be closed.
+    #
+    # Ideally we would do this in _ensure_gcs_bucket. However, removing this
+    # role means removing other (possibly needed) permissions that may be used
+    # by GCP service agent service accounts (e.g. App Engine, GCR, GCE):
+    # - storage.buckets.get
+    # - storage.multipartUploads.(abort|create|list|listParts)
+    # - storage.objects.(create|delete|list)
+    #
+    # So for now we constrain to "private" gcs buckets that are currently only used
+    # by humans to store terraform state containing potentially sensitive info
+    ensure_removed_gcs_role_binding "${bucket}" "projectEditor" "roles/storage.legacyBucketOwner"
 }
 
 # Sets the web policy on the bucket, including a default index.html page
@@ -162,6 +180,9 @@ function _empower_principal_to_admin_gcs_bucket() {
     local principal="$1"
     local bucket="$2"
 
+    # TODO: replace with roles/storage.admin? this would additionally grant
+    #       storage.buckets.(create|delete|list) for this bucket, where
+    #       today a principal can't delete a bucket they admin
     ensure_gcs_role_binding "${bucket}" "${principal}" "objectAdmin"
     ensure_gcs_role_binding "${bucket}" "${principal}" "legacyBucketOwner"
 }