Replace direct use of su with sudo su

[nrontsis]

To avoid complications when running as non-root, as discussed in loft-sh/devpod-provider-kubernetes#12

[bkneis]

@nrontsis I'm not sure what the discussion in the k8s operator has to do with this? This PR only changes running life cycle hooks as a sudo user, unconditionally. I doubt this would be expected behaviour for most people. Could you describe what you are trying to achieve? Or what the current issue you are facing with devpod?

[nrontsis]

Apologies for the lack of context @bkneis!

Could you describe what you are trying to achieve? Or what the current issue you are facing with devpod?

I am setting up the security context of my pods to a non-root user due to security constraints in my organisation (as discussed here), via the STRICT_SECURITY devpod-provider-kubernetes option and this in the POD_MANIFEST_TEMPLATE:

[...]
      securityContext: 
        runAsGroup: 1000
        runAsUser: 1000
        runAsNonRoot: true

As a result the check:

if user != "root" {
	args = append(args, "su", user, "-c", command.Quote(c))
	args = append(args, "sudo", "su", user, "-c", command.Quote(c))
} else {
	args = append(args, "sh", "-c", command.Quote(c))
}

is not applicable for my case, as the user running the above is the one with ID 1000, not root.

I agree that the approach of the PR is incorrect and was only a hack for my particular setup (user with access to sudo, but not su).

How about if I open a PR that changes:

if user != "root"

to something like:

if user != user.Current()

?

[bkneis]

@nrontsis thanks for clarifying, I understand your use case much better now :) What you suggested seems better, but I still have concerns that it could escalate privilege to a user that should not be a sudo user. Anyone else expecting to run as non root might also not want to run as sudo. Perhaps something like this could help isolate your workflow? @pascalbreuninger any thoughts? By checking STRICT_SECURITY it could be safer to assume sudo is fine as this is a common workaround when using non root users. Also @nrontsis can you explain how you tested this PR and if it worked for your use case?

if user != user.Current() && provider.GetProviderOptions(...)["STRICT_SECURITY"]

[nrontsis]

Thanks for the feedback @bkneis 🙌

What you suggested seems better, but I still have concerns that it could escalate privilege to a user that should not be a sudo user.

Just to clarify, following your comments, what I am now proposing is this new PR. I can't imagine how privilege escalation could happen in #1317, as I am not using sudo there.

Also @nrontsis can you explain how you tested this PR and if it worked for your use case?

I was testing end-to-end in a private repo that I can't share, but I am happy to create a minimal example that demonstrates #1317 working, if you are conceptually happy with the appoach.

[sanmai-NL]

Please don't check for a username, check for a UID.

[bkneis]

@sanmai-NL he's comparing 2 User structs, which will contain UID and username :) @nrontsis I like the implementation of 1317. If this works for you, can you please post a summary of your use case / test / working example on that PR

[nrontsis]

great, closing this PR and continue there.

[nrontsis]

@bkneis I updated #1317 with a description of the use case and added a conceptual minimal example. I did not test the minimal example provided end to end, I was hoping that the change is small enough that this might not be necessary, but let me know if you think otherwise.