helm: ensure defaultMode=0400 for projected volumes containing secrets #17367

rolinh · 2021-09-10T19:49:43Z

It seems that these volumes were wrongly given permission 420 (420 decimal == 0644 octal) instead of 0400, meaning that they were world readable and user writable. Secrets should only be readable by the actual user, thus with permission 0400.

joestringer

Looks right to me, I assume that CI should complain if there's any real problem with these permission changes?

rolinh · 2021-09-10T19:55:54Z

Looks right to me, I assume that CI should complain if there's any real problem with these permission changes?

That's my assumption, yes.

rolinh · 2021-09-10T21:27:59Z

test-me-please

Job 'Cilium-PR-K8s-1.19-kernel-5.4' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig AutoDirectNodeRoutes Check connectivity with sockops and direct routing

Failure Output

FAIL: Error creating resource /home/jenkins/workspace/Cilium-PR-K8s-1.19-kernel-5.4/src/github.com/cilium/cilium/test/k8sT/manifests/l3-policy-demo.yaml: Cannot retrieve cilium pod cilium-nctq6 policy revision: cannot get revision from json output '': could not parse JSON from command "kubectl exec -n kube-system cilium-nctq6 -- cilium policy get -o json"

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.19-kernel-5.4 so I can create a new GitHub issue to track it.

rolinh · 2021-09-13T08:35:22Z

/mlh new-flake Cilium-PR-K8s-1.19-kernel-5.4

👍 created #17373

tklauser

Nice catch. I think we have the same issue in cilium-cli:

https://github.com/cilium/cilium-cli/blob/master/install/install.go#L40

https://github.com/cilium/cilium-cli/blob/master/clustermesh/clustermesh.go#L49

kaworu

Thanks for the patch!

kaworu · 2021-09-14T11:32:24Z

It seems that these volumes were wrongly given permission 420 instead of 0400, giving write permission to the group

Actually, 420 is in decimal so before this patch the defaultMode expressed in octal would be 0644.

Follow cilium/cilium#17367: It seems that these volumes were wrongly given permission 420 instead of 0400, giving write permission to the group. In all instances, the actual volumeMounts were set to readonly so this change should have no effect but better be more cautious about permissions for secrets. Reported-by: Robin Hahling <robin.hahling@gw-computing.net> Signed-off-by: Tobias Klauser <tobias@cilium.io>

tklauser · 2021-09-14T12:20:46Z

Nice catch. I think we have the same issue in cilium-cli:

https://github.com/cilium/cilium-cli/blob/master/install/install.go#L40

https://github.com/cilium/cilium-cli/blob/master/clustermesh/clustermesh.go#L49

cilium/cilium-cli#539 should address this

Follow cilium/cilium#17367: It seems that these volumes were wrongly given permission 420 instead of 0400, giving write permission to the group. In all instances, the actual volumeMounts were set to readonly so this change should have no effect but better be more cautious about permissions for secrets. Reported-by: Robin Hahling <robin.hahling@gw-computing.net> Signed-off-by: Tobias Klauser <tobias@cilium.io>

joestringer

Given Alex's report that these YAML values do not support Octal and the previous values were decimal (and correct), it sounds like we shouldn't push forward with this.

rolinh · 2021-09-15T08:40:49Z

Given Alex's report that these YAML values do not support Octal and the previous values were decimal (and correct), it sounds like we shouldn't push forward with this.

I'm not sure I follow. Alex's point is that these permissions were probably inadvertently decimal instead of octal because of the missing leading 0. The decimal value 420 happens to be 0644 octal which is the default value if not specified. However, I think that what we probably want is actually 0400, as per this PR change, as YAML supports octal notation.

Note that the JSON spec doesn't support octal notation, so use the value 256 for 0400 permissions. If you use YAML instead of JSON for the Pod, you can use octal notation to specify permissions in a more natural way.

I will however update the PR to add a note that the leading 0 is actually important.

EDIT: I also updated the commit message to take into account Alex's comment.

joestringer

OK yeah my misunderstanding then. 👍

kaworu · 2021-09-16T08:47:49Z

@joestringer Sorry, my comment was incomplete. My guess is that the decimal 420 was meant to be 0420, as (1) 420=0644 is the default (thanks @rolinh for pointing it out) and (2) we have an actual 0420 in install/kubernetes/cilium/templates/clustermesh-apiserver/deployment.yaml.

In any case, I still think the PR is correct and improving the situation. However, it's a "bigger change" than we originally thought it was.

@rolinh From the commit message (emphasis mine):

[…] It seems that these volumes were wrongly given permission 420 (420 decimal == 0644 octal) instead of 0400

Did you meant instead of 0420 instead?

rolinh · 2021-09-16T15:42:31Z

test-me-please

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Host firewall Check connectivity with IPv6 disabled

Failure Output

FAIL: Pods are not ready in time: timed out waiting for pods with filter  to be ready: 4m0s timeout expired

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.16-net-next' hit: #17176 (85.97% similarity)

It seems that these volumes were wrongly given permission 420 (420 decimal == 0644 octal) instead of 0400, meaning that they were world readable and user writable. Secrets should only be readable by the actual user, thus with permission 0400. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

rolinh · 2021-10-04T19:51:56Z

/test

Job 'Cilium-PR-K8s-GKE' has 1 failure but they might be new flake since it also hit 1 known flake: #17403 (80.36)

rolinh · 2021-10-05T08:32:47Z

ci-aks

Follow cilium/cilium#17367: It seems that these volumes were wrongly given permission 420 instead of 0400, giving write permission to the group. In all instances, the actual volumeMounts were set to readonly so this change should have no effect but better be more cautious about permissions for secrets. Reported-by: Robin Hahling <robin.hahling@gw-computing.net> Signed-off-by: Tobias Klauser <tobias@cilium.io>

Follow cilium#17367: It seems that these volumes were wrongly given permission 420 instead of 0400, giving write permission to the group. In all instances, the actual volumeMounts were set to readonly so this change should have no effect but better be more cautious about permissions for secrets. Reported-by: Robin Hahling <robin.hahling@gw-computing.net> Signed-off-by: Tobias Klauser <tobias@cilium.io>

rolinh added release-note/misc This PR makes changes that have no direct user impact. area/helm Impacts helm charts and user deployment experience labels Sep 10, 2021

rolinh requested a review from a team as a code owner September 10, 2021 19:49

rolinh requested review from a team, kaworu and nathanjsweet September 10, 2021 19:49

maintainer-s-little-helper bot assigned kaworu and nathanjsweet Sep 10, 2021

joestringer approved these changes Sep 10, 2021

View reviewed changes

tklauser approved these changes Sep 13, 2021

View reviewed changes

kaworu approved these changes Sep 14, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned kaworu Sep 14, 2021

sayboras approved these changes Sep 14, 2021

View reviewed changes

sayboras mentioned this pull request Sep 14, 2021

Align values.yaml with templates #17243

Merged

tklauser mentioned this pull request Sep 14, 2021

Ensure defaultMode=0400 for projected volumes containing secrets cilium/cilium-cli#539

Merged

joestringer requested changes Sep 15, 2021

View reviewed changes

rolinh force-pushed the pr/rolinh/helm-volumes-defaultmode branch 2 times, most recently from 49352a2 to 50e5ae1 Compare September 15, 2021 08:50

rolinh requested a review from joestringer September 15, 2021 08:51

maintainer-s-little-helper bot assigned joestringer Sep 15, 2021

joestringer approved these changes Sep 15, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned joestringer Sep 15, 2021

maintainer-s-little-helper bot mentioned this pull request Sep 16, 2021

CI: K8sServicesTest Checks service across nodes Tests NodePort BPF Tests L2-less with Wireguard provisioned via kube-wireguarder Tests NodePort BPF #17176

Closed

nathanjsweet approved these changes Sep 21, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned nathanjsweet Sep 21, 2021

rolinh force-pushed the pr/rolinh/helm-volumes-defaultmode branch from 50e5ae1 to dd28406 Compare October 4, 2021 13:02

maintainer-s-little-helper bot mentioned this pull request Oct 4, 2021

CI: K8sServicesTest Checks ClusterIP Connectivity Checks service.kubernetes.io/service-proxy-name label implementation #17403

Closed

aanm merged commit 6f0a866 into master Oct 11, 2021

aanm deleted the pr/rolinh/helm-volumes-defaultmode branch October 11, 2021 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helm: ensure defaultMode=0400 for projected volumes containing secrets #17367

helm: ensure defaultMode=0400 for projected volumes containing secrets #17367

rolinh commented Sep 10, 2021 •

edited

joestringer left a comment

rolinh commented Sep 10, 2021

rolinh commented Sep 10, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

rolinh commented Sep 13, 2021 •

edited by maintainer-s-little-helper bot

tklauser left a comment

kaworu left a comment

kaworu commented Sep 14, 2021

tklauser commented Sep 14, 2021 •

edited

joestringer left a comment

rolinh commented Sep 15, 2021 •

edited

joestringer left a comment

kaworu commented Sep 16, 2021 •

edited

rolinh commented Sep 16, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

rolinh commented Oct 4, 2021 •

edited by maintainer-s-little-helper bot

rolinh commented Oct 5, 2021

helm: ensure defaultMode=0400 for projected volumes containing secrets #17367

helm: ensure defaultMode=0400 for projected volumes containing secrets #17367

Conversation

rolinh commented Sep 10, 2021 • edited

joestringer left a comment

Choose a reason for hiding this comment

rolinh commented Sep 10, 2021

rolinh commented Sep 10, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

rolinh commented Sep 13, 2021 • edited by maintainer-s-little-helper bot

tklauser left a comment

Choose a reason for hiding this comment

kaworu left a comment

Choose a reason for hiding this comment

kaworu commented Sep 14, 2021

tklauser commented Sep 14, 2021 • edited

joestringer left a comment

Choose a reason for hiding this comment

rolinh commented Sep 15, 2021 • edited

joestringer left a comment

Choose a reason for hiding this comment

kaworu commented Sep 16, 2021 • edited

rolinh commented Sep 16, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

rolinh commented Oct 4, 2021 • edited by maintainer-s-little-helper bot

rolinh commented Oct 5, 2021

rolinh commented Sep 10, 2021 •

edited

rolinh commented Sep 10, 2021 •

edited by maintainer-s-little-helper bot

rolinh commented Sep 13, 2021 •

edited by maintainer-s-little-helper bot

tklauser commented Sep 14, 2021 •

edited

rolinh commented Sep 15, 2021 •

edited

kaworu commented Sep 16, 2021 •

edited

rolinh commented Sep 16, 2021 •

edited by maintainer-s-little-helper bot

rolinh commented Oct 4, 2021 •

edited by maintainer-s-little-helper bot