Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion deploy/common.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
readonly KUSTOMIZE_PATH="${PKGDIR}/bin/kustomize"
readonly VERBOSITY="${GCE_PD_VERBOSITY:-2}"
readonly KUBECTL="${GCE_PD_KUBECTL:-kubectl}"
readonly SA_USER_ROLE="roles/iam.serviceAccountUser"

# Common functions

Expand All @@ -17,9 +18,21 @@ function ensure_var(){
fi
}

# Return true if scoping serviceAccountUser role to node service accounts
function use_scoped_sa_role()
{
[ -n "${NODE_SERVICE_ACCOUNTS:-}" ]
}

function get_needed_roles()
{
echo "roles/editor roles/compute.storageAdmin roles/iam.serviceAccountUser projects/${PROJECT}/roles/gcp_compute_persistent_disk_csi_driver_custom_role"
ROLES="roles/editor roles/compute.storageAdmin projects/${PROJECT}/roles/gcp_compute_persistent_disk_csi_driver_custom_role"
# Grant the role at the project level if no node service accounts are provided.
if ! use_scoped_sa_role;
then
ROLES+=" ${SA_USER_ROLE}"
fi
echo "${ROLES}"
}

# Installs kustomize in ${PKGDIR}/bin
Expand Down
1 change: 0 additions & 1 deletion deploy/kubernetes/base/controller/controller.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ spec:
- "--supports-dynamic-throughput-provisioning=hyperdisk-balanced,hyperdisk-throughput,hyperdisk-ml"
- --enable-data-cache
- --run-node-service=false
- --enable-multitenancy
command:
- /gce-pd-csi-driver
env:
Expand Down
1 change: 0 additions & 1 deletion deploy/kubernetes/base/node_linux/node.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ spec:
- "--endpoint=unix:/csi/csi.sock"
- "--run-controller-service=false"
- "--enable-data-cache"
- "--enable-multitenancy"
- "--node-name=$(KUBE_NODE_NAME)"
securityContext:
privileged: true
Expand Down
20 changes: 20 additions & 0 deletions deploy/kubernetes/deploy-driver.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
# by setup-project.sh). Ignored if GCE_PD_DRIVER_VERSION == noauth.
# GCE_PD_DRIVER_VERSION: The kustomize overlay to deploy. See
# `deploy/kubernetes/overlays` for your choices.
# NODE_SERVICE_ACCOUNTS: (Optional) Comma-separated list of service accounts
# that the CSI driver should be allowed to impersonate. If not specified,
# defaults to project-level serviceAccountUser role.

set -o nounset
set -o errexit
Expand Down Expand Up @@ -65,6 +68,23 @@ function check_service_account()
MISSING_ROLES=true
fi
done

# Check each node service account for serviceAccountUser role
if use_scoped_sa_role;
then
IFS=',' read -ra NODE_SA_ARRAY <<< "${NODE_SERVICE_ACCOUNTS}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do some input validation for NODE_SERVICE_ACCOUNTS? Like not empty etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

The if use_scoped_sa_role check already returns false at line 73 if NODE_SERVICE_ACCOUNTS is an empty string:

function use_scoped_sa_role()
{
	[ -n "${NODE_SERVICE_ACCOUNTS:-}" ]
}

for example:

$ unset NODE_SERVICE_ACCOUNTS
$ [ -n "$NODE_SERVICE_ACCOUNTS" ]
$ echo $?
1
$ export NODE_SERVICE_ACCOUNTS=""
$ [ -n "$NODE_SERVICE_ACCOUNTS" ]
$ echo $?
1

I considered checking for a comma in NODE_SERVICE_ACCOUNTS, but it is valid to pass in a single service account with no comma delimiter, and this line still handles that case:

$ export NODE_SERVICE_ACCOUNTS="sa1"
$ IFS=',' read -ra NODE_SA_ARRAY <<< "${NODE_SERVICE_ACCOUNTS}" && echo ${NODE_SA_ARRAY[@]}
sa1
$ export NODE_SERVICE_ACCOUNTS="sa1,sa2"
$ IFS=',' read -ra NODE_SA_ARRAY <<< "${NODE_SERVICE_ACCOUNTS}" && echo ${NODE_SA_ARRAY[@]}
sa1 sa2

The only other thing I can think to validate is whether it's a valid service account, which we won't know until calling the gcloud command, which gives a useful error message:

$ gcloud iam service-accounts get-iam-policy "invalid-sa1" --project="${PROJECT}" --flatten="bindings[].members" --format='table(bindings.role)' --filter="bindings.members:${IAM_NAME}"
ERROR: (gcloud.iam.service-accounts.get-iam-policy) argument SERVICE_ACCOUNT: Bad value [invalid-sa1]: Not a valid service account identifier. It should be either a numeric string representing the unique_id or an email of the form: my-iam-account@somedomain.com or my-iam-account@PROJECT_ID.iam.gserviceaccount.com

$ gcloud iam service-accounts get-iam-policy "invalid-sa1@somedomain.com" --project="${PROJECT}" --flatten="bindings[].members" --format='table(bindings.role)' --filter="bindings.members:${IAM_NAME}"
ERROR: (gcloud.iam.service-accounts.get-iam-policy) NOT_FOUND: Unknown service account. This command is authenticated as jdobson@redhat.com which is the active account specified by the [core/account] property

Copy link
Contributor

Choose a reason for hiding this comment

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

It is good enough with use_scoped_sa_role :)

for node_sa in "${NODE_SA_ARRAY[@]}";
do
node_sa=$(echo "${node_sa}" | xargs) # trim whitespace
SA_POLICY=$(gcloud iam service-accounts get-iam-policy "${node_sa}" --project="${PROJECT}" --flatten="bindings[].members" --format='table(bindings.role)' --filter="bindings.members:${IAM_NAME}")
if ! grep -q "${SA_USER_ROLE}" <<< "${SA_POLICY}";
then
echo "Missing ${SA_USER_ROLE} for: ${node_sa}"
MISSING_ROLES=true
fi
done
fi

if [ "${MISSING_ROLES}" = true ];
then
echo "Cannot deploy with missing roles in service account, please run setup-project.sh to setup Service Account"
Expand Down
23 changes: 20 additions & 3 deletions deploy/setup-project.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
# ENABLE_KMS_ADMIN: Add service account permissions to destroy Cloud KMS keys.
# CREATE_SA_KEY: (Optional) If true, creates a new service account key and
# exports it if creating a new service account
# NODE_SERVICE_ACCOUNTS: (Optional) Comma-separated list of service accounts
# that the CSI driver should be allowed to impersonate. If not specified,
# defaults to project-level serviceAccountUser role.

set -o nounset
set -o errexit
Expand Down Expand Up @@ -102,22 +105,36 @@ gcloud iam roles $action gcp_compute_persistent_disk_csi_driver_custom_role --qu
# Bind service account to roles
for role in ${BIND_ROLES}
do
gcloud projects add-iam-policy-binding "${PROJECT}" --member serviceAccount:"${IAM_NAME}" --role "${role}"
gcloud projects add-iam-policy-binding "${PROJECT}" --member serviceAccount:"${IAM_NAME}" --role "${role}" --condition=None
done

# Grant scoped serviceAccountUser role for node service accounts
if use_scoped_sa_role;
then
IFS=',' read -ra NODE_SA_ARRAY <<< "${NODE_SERVICE_ACCOUNTS}"
for node_sa in "${NODE_SA_ARRAY[@]}";
do
node_sa=$(echo "${node_sa}" | xargs) # trim whitespace
echo "Granting ${SA_USER_ROLE} for ${node_sa} to serviceAccount:${IAM_NAME}"
gcloud iam service-accounts add-iam-policy-binding "${node_sa}" \
--member="serviceAccount:${IAM_NAME}" --condition=None \
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need condition=None? What is the default of condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added --condition=None because without it I was getting prompted several times when running the script, for example:

$ gcloud projects add-iam-policy-binding "${PROJECT}" --member serviceAccount:"${IAM_NAME}" --role "${SA_USER_ROLE}"
...
 [5] TITLE=test-platform__ filter
 [6] None
 [7] Specify a new condition
The policy contains bindings with conditions, so specifying a condition is required when
adding a binding. Please specify a condition.:

The list of conditions might be specific to our project(?) but IMO we always want None for these bindings.

I did not see that prompt when running gcloud iam service-accounts add-iam-policy-binding to create a binding for a specific service account. The condition defaulted to None in that case, but I included it anyway to leave no ambiguity.

--role="${SA_USER_ROLE}" --project="${PROJECT}"
done
fi

# Authorize GCE to encrypt/decrypt using Cloud KMS encryption keys.
# https://cloud.google.com/compute/docs/disks/customer-managed-encryption#before_you_begin
if [ "${ENABLE_KMS}" = true ];
then
gcloud services enable cloudkms.googleapis.com --project="${PROJECT}"
gcloud projects add-iam-policy-binding "${PROJECT}" --member serviceAccount:"service-${PROJECT_NUMBER}@compute-system.iam.gserviceaccount.com" --role "roles/cloudkms.cryptoKeyEncrypterDecrypter"
gcloud projects add-iam-policy-binding "${PROJECT}" --member serviceAccount:"service-${PROJECT_NUMBER}@compute-system.iam.gserviceaccount.com" --role "roles/cloudkms.cryptoKeyEncrypterDecrypter" --condition=None
fi

# Authorize SA to destroy Cloud KMS encryption keys.
if [ "${ENABLE_KMS_ADMIN}" = true ];
then
gcloud services enable cloudkms.googleapis.com --project="${PROJECT}"
gcloud projects add-iam-policy-binding "${PROJECT}" --member serviceAccount:"${IAM_NAME}" --role "roles/cloudkms.admin"
gcloud projects add-iam-policy-binding "${PROJECT}" --member serviceAccount:"${IAM_NAME}" --role "roles/cloudkms.admin" --condition=None
fi

# Export key if needed
Expand Down
30 changes: 25 additions & 5 deletions docs/kubernetes/user-guides/driver-install.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

## Install Driver

1. Clone the driver to your local machine
### 1. Clone the driver to your local machine

```console
$ git clone https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver $GOPATH/src/sigs.k8s.io/gcp-compute-persistent-disk-csi-driver
```

2. [One-time per project] Set up or use an existing service account:
### 2. [One-time per project] Set up or use an existing service account:

The driver requires a service account that has the following permissions and
roles to function properly:
Expand All @@ -18,7 +18,7 @@ compute.instances.get
compute.instances.attachDisk
compute.instances.detachDisk
roles/compute.storageAdmin
roles/iam.serviceAccountUser
roles/iam.serviceAccountUser (see security note below)
```

If there is a pre-existing service account with these roles for use then the
Expand All @@ -33,7 +33,24 @@ $ GCE_PD_SA_DIR=/my/safe/credentials/directory
**Note**: The service account key *must* be named `cloud-sa.json` at driver deploy time

However, if there is no pre-existing service account for use the provided script
can be used to create a new service account with all the required permissions:
can be used to create a new service account with all the required permissions.

#### Security Note: Service Account Impersonation

The CSI driver requires the `roles/iam.serviceAccountUser` role to impersonate node service accounts when attaching and detaching disks. This role can be configured in two ways:

* **Recommended (Scoped)**: Grant the role only for specific node service accounts
* **Default (Project-wide)**: Allow project-wide service account impersonation (less secure)

For improved security, specify the node service accounts that the CSI driver needs to impersonate using the `NODE_SERVICE_ACCOUNTS` environment variable. This limits the role to only the specified accounts. Without `NODE_SERVICE_ACCOUNTS`, the CSI driver can impersonate any service account in the project.

```console
$ NODE_SERVICE_ACCOUNTS="master-sa@project.iam.gserviceaccount.com,worker-sa@project.iam.gserviceaccount.com" # Comma-separated list of node service accounts
```

For more details, see [How to remediate over privileged service account users](https://cloud.google.com/security-command-center/docs/how-to-remediate-security-health-analytics-findings#over_privileged_service_account_user).

#### Create service account for the CSI driver

```console
$ PROJECT=your-project-here # GCP project
Expand All @@ -46,9 +63,10 @@ $ ./deploy/setup-project.sh
deployment, all actions performed by the driver will be performed as the
specified service account

3. Deploy driver to Kubernetes Cluster
### 3. Deploy driver to Kubernetes Cluster

```console
$ NODE_SERVICE_ACCOUNTS="master-sa@project.iam.gserviceaccount.com,worker-sa@project.iam.gserviceaccount.com" # Same as the setup-project.sh step
$ GCE_PD_SA_DIR=/my/safe/credentials/directory # Directory to get the service account key
$ GCE_PD_DRIVER_VERSION=stable-master # Driver version to deploy
$ ./deploy/kubernetes/deploy-driver.sh
Expand All @@ -74,6 +92,8 @@ additional permissions are required in order to create the new service account:
```
resourcemanager.projects.getIamPolicy
resourcemanager.projects.setIamPolicy
iam.serviceAccounts.getIamPolicy
iam.serviceAccounts.setIamPolicy
iam.serviceAccounts.create
iam.serviceAccounts.delete
```
Expand Down