From db38ea45bba6810decc8af4244e4a4e29cc7850b Mon Sep 17 00:00:00 2001
From: Aaron Crickenberger <spiffxp@google.com>
Date: Wed, 28 Apr 2021 16:26:49 -0400
Subject: [PATCH] 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 | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/infra/gcp/lib_gcs.sh b/infra/gcp/lib_gcs.sh
index db8337e545ed..a26335f721e9 100644
--- a/infra/gcp/lib_gcs.sh
+++ b/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"
 }