feat: make group ID configurable #11924

nico151999 · 2024-01-12T19:04:33Z

Problem
Currently, the sidecar containers do not include a runAsGroup attribute. In some companies, there are security guidelines requiring the runAsGroup attribute to be set.

Solution
This PR causes the group ID to be set and adds the option to set it manually. It depends on this other PR.

Validation
I built the proxy init image with the code in my other pull request and loaded it into my k3d cluster in the dev container. I built the images and loaded them into the k3d cluster as well. Then, I installed the CRDs, Linkerd and Linkerd Viz. I deployed Emojivoto without the config.linkerd.io/proxy-gid annotation being set and then with it being set to check if the group ID changes accordingly in the respective containers' security context. Note that I had to tweak my setup a little bit in order to make it work in WSL2 behind an HTTP proxy, so my setup might differ from the reference setup. Still, I assume that my tweaks did not have an impact on the outcome of the tests.

Fixes #11773

Signed-off-by: Nico Feulner nico.feulner@gmail.com

nico151999 · 2024-01-15T10:12:30Z

As stated in the PR description the PR depends on this. I have just marked the PR as draft so that it won't be merged accidently at some point before the updated image tag for proxy-init can be used. We should focus on the proxy-init PR first and get it released. Then, we can focus on this PR.

stale · 2024-04-15T06:10:34Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

nico151999 · 2024-04-17T12:56:23Z

Now as the changes to proxy-init are merged I would like to give this PR attention again. I have just updated it so that the comits that have been made in the meantime are included. Therefore, I would like to ask the maintainers to review the changes now. @mateiidavid @kflynn I don't know how much you can do here (i.e. if changes to the linkerd2 repos are part of your responsibility) but maybe you can take a look. Thanks!
I would also like to point out that I would recommend not to make a new release before a new version of proxy-init is released. Running the changes in this PR without the updates to proxy-init does not work since the group ID flag is expected to be available. Thanks in advance for having a look!

mateiidavid

Thanks @nico151999 I put up a PR (#12462) to bump the proxy-init deps in the linkerd2 repo.

Most of the changes look good to me. I mentioned in a comment that my preference would be to have this as something users explicitly opt-in to for their components.

charts/linkerd-control-plane/templates/psp.yaml

mateiidavid · 2024-04-18T11:53:43Z

charts/linkerd-control-plane/values.yaml

@@ -182,6 +182,8 @@ proxy:
      request: ""
  # -- User id under which the proxy runs
  uid: 2102
+  # -- Group id under which the proxy runs
+  gid: 2102


Is there an alternative solution here where we introduce a gid in the manifests explicitly? Your proxy-init change treats gids as optional. Being a bit more risk adverse, I'd prefer it if gids were not implicitly used here.

The only problem I can see with treating gids as optional is that it results in a bigger configuration surface area. Is there anything we can do to keep the config scope narrow and allow people to explicitly use a gid? For example, setting it as 0 would work, but from a UX standpoint seeing gid: 0 in the values.yaml file is not something I'd be a big fan of. I'm sure Helm supports optional args though in some capacity.

We could use a special value like -1 for this purpose and add a comment explaining it. Alternatively, we could do something like

gid: customise: true # tells if the gid should be set to the custom value provided value: 2102

When I write charts I personally prefer the latter style. So unless you intervene I would implement it this way. What are your thoughts @mateiidavid?

I decided to quickly implement it as I suggested. Just take a look and let me know what you think. Thanks!

mateiidavid

@nico151999 I like the way you structured the config. There's a "but" coming :) I think we'd rather avoid having a separate customise: false (or enabled: true) option. I don't think our config is set-up well enough to support it at the moment; it's an unfortunate side effect for this PR. It's also hard to keep things consistent with other similar parts of our config because we want to introduce optionality here...

Here's an idea. Can we use KindIs to infer whether a value has been set for the respective field? We tried it out locally and it seemed to work. A more concrete example:

# values.yaml
proxy:
  gid:

# template
{{- if kindIs "int" .Values.proxy.gid }}
- --gid
- {{.Values.proxy.gid }}

cc @alpeb

mateiidavid · 2024-04-25T14:57:32Z

charts/linkerd2-cni/values.yaml

+proxyGID:
+  # -- Tells if the customisation should be applied or not
+  customise: false
+  # -- Group id under which the proxy shall be ran
+  value: 2102


Can we nest gid under each specific container/app?

proxy: gid: controller: gid: proxyInit: gid:

mateiidavid · 2024-04-25T14:58:51Z

cli/cmd/inject.go

+	if proxy.GID.Customise && proxy.GID.Value != baseProxy.GID.Value {
+		overrideAnnotations[k8s.ProxyGIDAnnotation] = strconv.FormatInt(proxy.GID.Value, 10)
+	}
+


We should probably have an explicit check against 0. Using group: 0 might be a bit more dangerous since packets can be generated by other processes that run under the same group, and they will end not being redirected. This will all be really hard to debug.

mateiidavid · 2024-04-25T15:01:34Z

charts/linkerd2-cni/values.yaml

@@ -20,6 +20,12 @@ logLevel:         info
 portsToRedirect:  ""
 # -- User id under which the proxy shall be ran
 proxyUID:         2102
+# -- Optional customisation of the group id under which the proxy shall be ran


I think we need to document this a bit more to prevent people from misusing the field.

We should note that proxy's GID should be unique. No other container should run under the same group as the proxy (including the control plane's containers). Once the group ID is shared, packets can be skipped for processes that are not the proxy and it will result in hard to diagnose issues.

We should consider using the fail function to fail-fast if the proxy's gid == controller gid.

Open to any other suggestions if you have any. Our biggest concern here would be that users unknowingly set the same gid across all containers.

mateiidavid · 2024-04-25T15:03:09Z

pkg/charts/linkerd2/values.go

@@ -29,6 +29,7 @@ type (
 		ControllerImage              string                 `json:"controllerImage"`
 		ControllerReplicas           uint                   `json:"controllerReplicas"`
 		ControllerUID                int64                  `json:"controllerUID"`
+		ControllerGID                *Customisable[int64]   `json:"controllerGID"`


If we change gid to be nullable, we could probably just take a pointer to an int here instead of introducing a new type.

mateiidavid · 2024-05-02T15:38:18Z

@nico151999 thanks for making the changes! I'm afraid I might've led you down the wrong path here. When I tested the change I realised the documentation will look weird... on second thought it might be better to just do gid: -1 and call it a day.

@mateiidavid

As discussed with @mateiidavid I refactored the PR so that it now allows to omit the group ID with a negative value instead of null. Signed-off-by: Nico Feulner <nico.feulner@gmail.com>

alpeb · 2024-05-23T00:44:03Z

I took the liberty of rebasing with main and pushing some changes/fixes:

Reworded the comment for tap.GID (and the pre-existing tap.UID one that was also wrong)
Undid some changes to test/integration/install/inject/inject_test.go for the test to pass, given the control plane is installed with the default proxy.guid=-1 in that context.

I'll do some manual testing tomorrow; looking good!

nico151999 · 2024-05-23T08:00:49Z

Thanks a lot @alpeb

mateiidavid · 2024-05-23T09:56:39Z

viz/charts/linkerd-viz/values.yaml

@@ -125,6 +127,9 @@ metricsAPI:
  # -- UID for the metrics-api resource
  UID:

+  # -- GID for the metrics-api resource
+  gID:


Just a nitpick but we should go with gID or GID (below) for consistency.

Ah yea good catch, @nico151999 you wanna get that addressed?

I pushed the change so we can include this in today's edge 😉

mateiidavid

🚀 thanks @nico151999 and @alpeb

alpeb

🥳

nico151999 · 2024-05-23T19:29:28Z

Sorry for the late response. I'm on a short hiking trip and can't test the changes but they look good to my bare eyes and the tests seem to pass. So I would say we are good to go. Thanks for the effort today @alpeb

nico151999 requested a review from a team as a code owner January 12, 2024 19:04

nico151999 marked this pull request as draft January 15, 2024 10:09

stale bot added the wontfix label Apr 15, 2024

nico151999 force-pushed the nico151999/make-group-id-configurable branch from b6f5b36 to 60d6518 Compare April 17, 2024 12:33

stale bot removed the wontfix label Apr 17, 2024

nico151999 force-pushed the nico151999/make-group-id-configurable branch from 60d6518 to 4000715 Compare April 17, 2024 12:45

nico151999 marked this pull request as ready for review April 17, 2024 12:56

mateiidavid reviewed Apr 18, 2024

View reviewed changes

nico151999 force-pushed the nico151999/make-group-id-configurable branch 2 times, most recently from 552ffd8 to 59fd163 Compare April 24, 2024 07:54

mateiidavid reviewed Apr 25, 2024

View reviewed changes

nico151999 force-pushed the nico151999/make-group-id-configurable branch from 5f0885c to 6adbccb Compare April 26, 2024 15:29

nico151999 force-pushed the nico151999/make-group-id-configurable branch 4 times, most recently from 400d62d to 26cf299 Compare May 14, 2024 17:30

alpeb force-pushed the nico151999/make-group-id-configurable branch from b7732db to c765eb5 Compare May 22, 2024 20:15

refactor: treat negative group IDs as unset

572a6d1

As discussed with @mateiidavid I refactored the PR so that it now allows to omit the group ID with a negative value instead of null. Signed-off-by: Nico Feulner <nico.feulner@gmail.com>

alpeb force-pushed the nico151999/make-group-id-configurable branch from c765eb5 to 572a6d1 Compare May 22, 2024 22:57

alpeb added 3 commits May 22, 2024 19:02

reword tap UID/GID comments

63401a5

In inject_test.go, linkerd runs as default with gid=-1

78f973d

Helm docs

a2a50aa

mateiidavid reviewed May 23, 2024

View reviewed changes

s/gID/GID

7a98095

mateiidavid approved these changes May 23, 2024

View reviewed changes

alpeb approved these changes May 23, 2024

View reviewed changes

Merge branch 'main' into nico151999/make-group-id-configurable

5bf0ca9

alpeb merged commit 3d67459 into linkerd:main May 23, 2024
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make group ID configurable #11924

feat: make group ID configurable #11924

nico151999 commented Jan 12, 2024

nico151999 commented Jan 15, 2024

stale bot commented Apr 15, 2024

nico151999 commented Apr 17, 2024

mateiidavid left a comment

mateiidavid Apr 18, 2024

nico151999 Apr 19, 2024

nico151999 Apr 19, 2024

mateiidavid left a comment •

edited

Loading

mateiidavid Apr 25, 2024

mateiidavid Apr 25, 2024

mateiidavid Apr 25, 2024 •

edited

Loading

mateiidavid Apr 25, 2024

mateiidavid commented May 2, 2024

alpeb commented May 23, 2024

nico151999 commented May 23, 2024

mateiidavid May 23, 2024

alpeb May 23, 2024

alpeb May 23, 2024

mateiidavid left a comment

alpeb left a comment

nico151999 commented May 23, 2024

feat: make group ID configurable #11924

feat: make group ID configurable #11924

Conversation

nico151999 commented Jan 12, 2024

nico151999 commented Jan 15, 2024

stale bot commented Apr 15, 2024

nico151999 commented Apr 17, 2024

mateiidavid left a 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

mateiidavid left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateiidavid Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateiidavid commented May 2, 2024

alpeb commented May 23, 2024

nico151999 commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateiidavid left a comment

Choose a reason for hiding this comment

alpeb left a comment

Choose a reason for hiding this comment

nico151999 commented May 23, 2024

mateiidavid left a comment •

edited

Loading

mateiidavid Apr 25, 2024 •

edited

Loading