Run the etcd as non-root #100635

cindy52 · 2021-03-29T17:55:20Z

What type of PR is this?

/kind feature

Run etcd as non-root on GCE provider'

k8s-ci-robot · 2021-03-29T17:55:27Z

Welcome @cindy52!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-03-29T17:55:28Z

Hi @cindy52. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vinayakankugoyal · 2021-03-29T18:33:33Z

cluster/gce/util.sh

@@ -1254,6 +1254,8 @@ ${CUSTOM_CALICO_NODE_DAEMONSET_YAML//\'/\'\'}
 CUSTOM_TYPHA_DEPLOYMENT_YAML: |
 ${CUSTOM_TYPHA_DEPLOYMENT_YAML//\'/\'\'}
 CONCURRENT_SERVICE_SYNCS: $(yaml-quote "${CONCURRENT_SERVICE_SYNCS:-}")
+ETCD_RUNASUSER: $(yaml-quote "${ETCD_RUNASUSER:-2000}")


If this is not set then this should be defaulted to 0.

There is already an etcd user as 2000, I'm afraid of defaulting it to 0 will end up with running as root again.

Where is the etcd user 2000 created?

I can't find it anywhere in the code but when I ssh to the master node, I can see the etcd user by cat /etc/group.

vinayakankugoyal · 2021-03-29T18:33:57Z

cluster/gce/manifests/etcd.manifest

+    },
+    "runAsUser": {{runAsUser}},
+    "runAsGroup": {{runAsGroup}},
+    "runAsNonRoot": true


not really needed?

Setting it to true will prevent the pod run as user 0 which is root, not sure it's desired or not.

vinayakankugoyal · 2021-03-29T20:07:10Z

cluster/gce/gci/configure-helper.sh

+    sed -i -e "s@{{runAsUser}}@${ETCD_RUNASUSER}@g" "${temp_file}"
+    sed -i -e "s@{{runAsGroup}}@${ETCD_RUNASGROUP}@g" "${temp_file}"
+    chown -R ${ETCD_RUNASUSER}:${ETCD_RUNASGROUP} /var/etcd/
+    chown -R ${ETCD_RUNASUSER}:${ETCD_RUNASGROUP} /var/log/etcd.log


This is already done on line 1893 and 1894

vinayakankugoyal · 2021-03-29T20:09:05Z

cluster/gce/gci/configure-helper.sh

@@ -1878,10 +1889,15 @@ function start-etcd-servers {
  if [[ -e /etc/init.d/etcd ]]; then
    rm -f /etc/init.d/etcd
  fi
-  prepare-log-file /var/log/etcd.log
+  if [[ -n "${ETCD_RUNASUSER:-}" && -n "${ETCD_RUNASGROUP:-}" ]]; then
+    prepare-log-file /var/log/etcd.log  ${ETCD_RUNASUSER} ${ETCD_RUNASGROUP}


You only need to pass the user and not the group see:

kubernetes/cluster/gce/gci/configure-helper.sh

Line 2029 in 816bdd3

prepare-log-file /var/log/kube-controller-manager.log ${KUBE_CONTROLLER_MANAGER_RUNASUSER}

cluster/gce/manifests/etcd.manifest

cluster/gce/gci/configure-helper.sh

vinayakankugoyal · 2021-04-01T00:10:33Z

cluster/gce/gci/configure-helper.sh

+  if [[ -n "${ETCD_RUNASUSER:-}" && -n "${ETCD_RUNASGROUP:-}" ]]; then
+    container_security_context=$(echo "{\"allowPrivilegeEscalation\": false, \"capabilities\": {\"drop\": [\"all\"]}}" | base64 | tr -d '\r\n')
+  else
+    container_security_context=$(echo "{\"allowPrivilegeEscalation\": false}" | base64 | tr -d '\r\n')


lets just leave the securityContext alone if it is running as root?

vinayakankugoyal · 2021-04-01T00:11:42Z

cluster/gce/gci/configure-helper.sh

+  chown -R ${ETCD_RUNASUSER:-0}:${ETCD_RUNASGROUP:-0} /var/etcd/
+  # Replace capabilities
+  if [[ -n "${ETCD_RUNASUSER:-}" && -n "${ETCD_RUNASGROUP:-}" ]]; then
+    container_security_context=$(echo "{\"allowPrivilegeEscalation\": false, \"capabilities\": {\"drop\": [\"all\"]}}" | base64 | tr -d '\r\n')


do we really need the base64 and the tr?

vinayakankugoyal · 2021-04-01T00:16:18Z

cluster/gce/gci/configure-helper.sh

@@ -1858,6 +1858,18 @@ function prepare-etcd-manifest {
  fi
  # Replace the volume host path.
  sed -i -e "s@/mnt/master-pd/var/etcd@/mnt/disks/master-pd/var/etcd@g" "${temp_file}"
+  # Replace the runAsUser and runAsGroup
+  sed -i -e "s@{{runAsUser}}@${ETCD_RUNASUSER:-0}@g" "${temp_file}"


I think it would be safest to not edit the manifest if ETCD_RUNASUSER and ETCD_RUNASGROUP is not set. Basically if ETCD_RUNASUSER and ETCD_RUNASGROUP is not set then this change is a noop, WDYT?

We have to replace the {{runAsUser}} and {{runAsGroup}} in the manifest anyway isn't? I mean replace it to 0 if ETCD_RUNASUSER and ETCD_RUNASGROUP is not set.

Why do that? If we don't set it then it will anyways run as 0.

The manifest is written as:

"runAsUser": {{runAsUser}}, "runAsGroup": {{runAsGroup}}

The {{runAsUser}} and group need to be replace at least with some value, or are we talking about different thing?

No I am talking about this, but I think we can make the manifest to be:-

securityContext { ... {{runAsUser}} {{runAsGroup}} }

and replace {{runAsUser}} with "\"runAsUser\": ${ETCD_RUNASUSER}" and same for runAsGroup. That way we only insert it ETCD_RUNASUSER else we replace {{runAsUser}} with empty string.

Do we want to replace the entire securityContext as empty when it's root? Otherwise when it's root and runAsUser and runAsGroup is empty, the delimiter , at seccompProfile will be a problem.

"securityContext": { "seccompProfile": { "type": "RuntimeDefault" }, {{runAsUser}} {{runAsGroup}} }

Umm... you can add those before the seccompProfile 😃

vinayakankugoyal · 2021-04-01T20:00:02Z

cluster/gce/gci/configure-helper.sh

+  if [[ -n "${ETCD_RUNASUSER:-}" && -n "${ETCD_RUNASGROUP:-}" ]]; then
+    container_security_context=$(echo "{\"allowPrivilegeEscalation\": false, \"capabilities\": {\"drop\": [\"all\"]}}")
+  else
+    container_security_context=$(echo "{}")


I think instead of making the securityContext: {} when ETCD_RUNASUSER is not set, you can make it so that:-
instead of having

securityContext: {{security_context}}

You make it just:

{{containerSecurityContext}}

Then this would be

container_security_context="" if [[ -n "${ETCD_RUNASUSER:-}" && -n "${ETCD_RUNASGROUP:-}" ]]; then container_security_context=$(echo "{\"allowPrivilegeEscalation\": false, \"capabilities\": {\"drop\": [\"all\"]}}") fi sed -i -e "s@{{security_context}}@${container_security_context}@g" "${temp_file}"

wojtek-t · 2021-04-07T06:57:17Z

cluster/gce/gci/configure-helper.sh

+    pod_run_as_user="\"runAsUser\": ${ETCD_RUNASUSER},"
+    pod_run_as_group="\"runAsGroup\": ${{ETCD_RUNASGROUP},"
+    container_security_context="\"securityContext\": {\"allowPrivilegeEscalation\": false, \"capabilities\": {\"drop\": [\"all\"]}},"
+    chown -R ${ETCD_RUNASUSER}:${ETCD_RUNASGROUP} /var/etcd/


I would really like to avoid "chown" in the prepare manifests function.

Can we move this one to start-etcd-servers (also given that /var/etcd is actually shared between the two etcd's)?

wojtek-t · 2021-04-07T06:57:44Z

I'm mostly fine with this modulo the comment. But I would like @ptabor to also take a look at it.

ptabor · 2021-04-07T15:28:13Z

cluster/gce/manifests/etcd.manifest

@@ -26,7 +29,7 @@
    "command": [
              "/bin/sh",
              "-c",
-              "set -o errexit; if [ -e /usr/local/bin/migrate-if-needed.sh ]; then /usr/local/bin/migrate-if-needed.sh 1>>/var/log/etcd{{ suffix }}.log 2>&1; fi; exec /usr/local/bin/etcd --name etcd-{{ hostname }} --listen-peer-urls {{ etcd_protocol }}://{{ host_ip }}:{{ server_port }} --initial-advertise-peer-urls {{ etcd_protocol }}://{{ hostname }}:{{ server_port }} --advertise-client-urls {{ etcd_apiserver_protocol }}://127.0.0.1:{{ port }} --listen-client-urls {{ etcd_apiserver_protocol }}://{{ listen_client_ip }}:{{ port }} {{ quota_bytes }} --data-dir /var/etcd/data{{ suffix }} --initial-cluster-state {{ cluster_state }} --initial-cluster {{ etcd_cluster }} {{ etcd_creds }} {{ etcd_apiserver_creds }} {{ etcd_extra_args }} 1>>/var/log/etcd{{ suffix }}.log 2>&1"
+              "set -o errexit; if [ -e /usr/local/bin/migrate-if-needed.sh ]; then /usr/local/bin/migrate-if-needed.sh 1>>/var/log/etcd{{ suffix }}.log 2>&1; fi; exec /usr/local/bin/etcd-{{ pillar.get('etcd_version', '3.4.13') }} --name etcd-{{ hostname }} --listen-peer-urls {{ etcd_protocol }}://{{ host_ip }}:{{ server_port }} --initial-advertise-peer-urls {{ etcd_protocol }}://{{ hostname }}:{{ server_port }} --advertise-client-urls {{ etcd_apiserver_protocol }}://127.0.0.1:{{ port }} --listen-client-urls {{ etcd_apiserver_protocol }}://{{ listen_client_ip }}:{{ port }} {{ quota_bytes }} --data-dir /var/etcd/data{{ suffix }} --initial-cluster-state {{ cluster_state }} --initial-cluster {{ etcd_cluster }} {{ etcd_creds }} {{ etcd_apiserver_creds }} {{ etcd_extra_args }} 1>>/var/log/etcd{{ suffix }}.log 2>&1"


Do we need this? I assume that always a 'target' image is declared by the manifest,
and the .../bin/etcd in the image is the 'target' version within the image.

Long-term I wish we get rid of 'specific-version' binaries in the image... so we shouldn't add dependencies on this naming convention.

It's needed because in runMigrate, if the copy binary flag is true, it will copy the specific version of etcd and etcdclt to /etcd and /etcdclt https://github.com/kubernetes/kubernetes/blob/master/cluster/images/etcd/migrate/migrate.go#L77. As nonroot, we set the copy binary as false, thus we need to specify the version.

Please see bellow - I think ./etcd is already in the image and does not requires copying.

ptabor · 2021-04-07T15:31:12Z

cluster/gce/manifests/etcd.manifest

@@ -65,7 +72,7 @@
        "command": [
          "/bin/sh",
          "-c",
-          "set -x; exec /usr/local/bin/etcdctl --endpoints=127.0.0.1:{{ port }} {{ etcdctl_certs }} --command-timeout=15s endpoint health"
+          "set -x; exec /usr/local/bin/etcdctl-{{ pillar.get('etcd_version', '3.4.13') }} --endpoints=127.0.0.1:{{ port }} {{ etcdctl_certs }} --command-timeout=15s endpoint health"


ditto. E.g. listed binaries in the 3.4.9 (gcr.io/google-containers/etcd-amd64:3.4.9) image:

-r-xr-xr-x 1 ptab primarygroup 23827424 Jun 25 2020 etcd -r-xr-xr-x 1 ptab primarygroup 20186048 Jun 25 2020 etcd-3.0.17 -r-xr-xr-x 1 ptab primarygroup 16352288 Jun 25 2020 etcd-3.1.12 -r-xr-xr-x 1 ptab primarygroup 17899872 Jun 25 2020 etcd-3.2.24 -r-xr-xr-x 1 ptab primarygroup 22102784 Jun 25 2020 etcd-3.3.17 -r-xr-xr-x 1 ptab primarygroup 23827424 Jun 25 2020 etcd-3.4.9 -r-xr-xr-x 1 ptab primarygroup 17612384 Jun 25 2020 etcdctl -r-xr-xr-x 1 ptab primarygroup 18468640 Jun 25 2020 etcdctl-3.0.17 -r-xr-xr-x 1 ptab primarygroup 14319264 Jun 25 2020 etcdctl-3.1.12 -r-xr-xr-x 1 ptab primarygroup 15279616 Jun 25 2020 etcdctl-3.2.24 -r-xr-xr-x 1 ptab primarygroup 17770784 Jun 25 2020 etcdctl-3.3.17 -r-xr-xr-x 1 ptab primarygroup 17612384 Jun 25 2020 etcdctl-3.4.9

wojtek-t · 2021-04-08T18:48:36Z

I will wait for @ptabor to take another look, but I'm generally fine with it.

/ok-to-test

vinayakankugoyal · 2021-04-08T20:44:20Z

/ok-to-test

Instead of re-running everything consider re-running the only the ones that failed. You can do this by commenting /retest. Or by /test <test-name>

Co-authored-by: Vinayak Goyal <vinayakankugoyal@gmail.com>

cindy52 · 2021-04-08T20:53:37Z

/retest

cindy52 · 2021-04-08T22:25:14Z

/assign @dchen1107
/assign @tallclair

wojtek-t · 2021-04-09T07:21:30Z

/triage accepted
/priority important-soon

/lgtm
/approve

wojtek-t · 2021-04-09T07:21:53Z

/triage accepted
/priority important-soon

/lgtm
/approve

k8s-ci-robot · 2021-04-09T07:22:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cindy52, ptabor, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cluster/gce/gci/OWNERS~~ [wojtek-t]
~~cluster/gce/manifests/OWNERS~~ [wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ptabor · 2021-04-09T09:32:38Z

/lgtm

k8s-ci-robot · 2021-04-09T09:32:47Z

@ptabor: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 29, 2021

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/provider/gcp Issues or PRs related to gcp provider labels Mar 29, 2021

k8s-ci-robot requested review from MrHohn and mtaufen March 29, 2021 17:55

k8s-ci-robot added sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 29, 2021

vinayakankugoyal reviewed Mar 29, 2021

View reviewed changes

cindy52 force-pushed the etcd/rootless branch from a8bcf04 to 2e149c9 Compare March 29, 2021 22:13

vinayakankugoyal reviewed Mar 30, 2021

View reviewed changes

cluster/gce/manifests/etcd.manifest Show resolved Hide resolved

vinayakankugoyal reviewed Mar 30, 2021

View reviewed changes

cluster/gce/gci/configure-helper.sh Show resolved Hide resolved

cindy52 force-pushed the etcd/rootless branch from 2e149c9 to 8c8cb32 Compare March 30, 2021 22:54

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 30, 2021

cindy52 force-pushed the etcd/rootless branch from 8c8cb32 to acf6dfe Compare March 31, 2021 19:11

vinayakankugoyal reviewed Apr 1, 2021

View reviewed changes

cindy52 force-pushed the etcd/rootless branch from acf6dfe to 31f2188 Compare April 1, 2021 19:18

vinayakankugoyal reviewed Apr 1, 2021

View reviewed changes

cindy52 force-pushed the etcd/rootless branch from 31f2188 to e1f3c2e Compare April 1, 2021 22:45

wojtek-t reviewed Apr 7, 2021

View reviewed changes

ptabor reviewed Apr 7, 2021

View reviewed changes

cindy52 force-pushed the etcd/rootless branch from 8a12528 to d315c90 Compare April 7, 2021 21:03

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 8, 2021

run etcd as nonroot

9f05807

Co-authored-by: Vinayak Goyal <vinayakankugoyal@gmail.com>

cindy52 force-pushed the etcd/rootless branch from d315c90 to 9f05807 Compare April 8, 2021 20:52

ptabor approved these changes Apr 8, 2021

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 9, 2021

k8s-ci-robot assigned wojtek-t Apr 9, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 9, 2021

k8s-ci-robot merged commit 5b038e6 into kubernetes:master Apr 9, 2021

k8s-ci-robot added this to the v1.22 milestone Apr 9, 2021

Run the etcd as non-root #100635

Run the etcd as non-root #100635

Conversation

cindy52 commented Mar 29, 2021 • edited by wojtek-t

What type of PR is this?

k8s-ci-robot commented Mar 29, 2021

k8s-ci-robot commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cindy52 Apr 2, 2021 • edited

Choose a reason for hiding this comment

vinayakankugoyal Apr 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Apr 8, 2021

vinayakankugoyal commented Apr 8, 2021

cindy52 commented Apr 8, 2021

cindy52 commented Apr 8, 2021 • edited

wojtek-t commented Apr 9, 2021

wojtek-t commented Apr 9, 2021

k8s-ci-robot commented Apr 9, 2021

ptabor commented Apr 9, 2021

k8s-ci-robot commented Apr 9, 2021

cindy52 commented Mar 29, 2021 •

edited by wojtek-t

cindy52 Apr 2, 2021 •

edited

vinayakankugoyal Apr 2, 2021 •

edited

cindy52 commented Apr 8, 2021 •

edited