feat!: add safety check if not running as PID 1. #153
Conversation
| if env, found := os.LookupEnv("DANGEROUS_DISABLE_PID_CHECK"); found && env == "1" { | ||
| _, _ = fmt.Fprintln(os.Stderr, `Bypassing PID check as DANGEROUS_DISABLE_PID_CHECK=1.`) | ||
| } else { |
There was a problem hiding this comment.
review: will move this inside Options once Bruno's PR is in.
I prefer closing issues myself :-) |
There was a problem hiding this comment.
Nice work!
Could this be problematic for some container deployments as well? I don't have any concrete example in mind, just thinking there may be cases when there's an actual init and this would make it harder to use envbuilder.
There's also other ways to detect we're in a container and (allow?) execution even if not PID 1.
local -r cgroup_file='/proc/1/cgroup'
local -r nspawn_file='/run/host/container-manager'
[[ -r "$cgroup_file" && "$(< $cgroup_file)" = *(lxc|docker)* ]] \
|| [[ "$container" == "lxc" ]] \
|| [[ "$container" == "oci" ]] \
|| [[ "$container" == "podman" ]] \
|| [[ -r "$nspawn_file" ]]
I didn't think about this much so I'm not saying we want to do this, just putting it out there.
| > **Note:** Envbuilder performs destructive filesystem operations! To guard against accidental data loss, it | ||
| > will refuse to run if it detects it is not running as PID 1. | ||
| > If you need to bypass this behaviour for any reason, you can bypass this safety check by setting | ||
| > `DANGEROUS_BYPASS_PID_CHECK=1`. |
There was a problem hiding this comment.
I would suggest using the prefix, ENVBUILDER_DANGEROUS_DISABLE_PID_CHECK (also observe bypass/disable used inconsistently).
| if os.Getpid() != 1 { | ||
| // TODO: rebase once https://github.com/coder/envbuilder/pull/140 is in | ||
| if env, found := os.LookupEnv("DANGEROUS_DISABLE_PID_CHECK"); found && env == "1" { | ||
| _, _ = fmt.Fprintln(os.Stderr, `Bypassing PID check as DANGEROUS_DISABLE_PID_CHECK=1.`) |
There was a problem hiding this comment.
| _, _ = fmt.Fprintln(os.Stderr, `Bypassing PID check as DANGEROUS_DISABLE_PID_CHECK=1.`) | |
| _, _ = fmt.Fprintln(os.Stderr, `ENVBUILDER_DANGEROUS_DISABLE_PID_CHECK enabled, skipping PID check.`) |
I think we're better of not logging the value here. Also, how would you feel about changing it to something not so easily typed? Perhaps even "i_really_know_what_im_doing_please_let_me_brake_my_system". 😂
| if env, found := os.LookupEnv("DANGEROUS_DISABLE_PID_CHECK"); found && env == "1" { | ||
| _, _ = fmt.Fprintln(os.Stderr, `Bypassing PID check as DANGEROUS_DISABLE_PID_CHECK=1.`) | ||
| } else { | ||
| _, _ = fmt.Fprintln(os.Stderr, `WARNING: Not running as PID 1, so exiting IMMEDIATELY!`) |
There was a problem hiding this comment.
| _, _ = fmt.Fprintln(os.Stderr, `WARNING: Not running as PID 1, so exiting IMMEDIATELY!`) | |
| _, _ = fmt.Fprintln(os.Stderr, `WARNING: Envbuilder is not running as PID 1, exiting IMMEDIATELY!`) |
| } else { | ||
| _, _ = fmt.Fprintln(os.Stderr, `WARNING: Not running as PID 1, so exiting IMMEDIATELY!`) | ||
| _, _ = fmt.Fprintln(os.Stderr, `This is a safety check to guard against accidental data loss when run outside of a container.`) | ||
| _, _ = fmt.Fprintln(os.Stderr, `To bypass this check, set DANGEROUS_DISABLE_PID_CHECK=1.`) |
There was a problem hiding this comment.
| _, _ = fmt.Fprintln(os.Stderr, `To bypass this check, set DANGEROUS_DISABLE_PID_CHECK=1.`) | |
| _, _ = fmt.Fprintln(os.Stderr, `To skip this check, set ENVBUILDER_DANGEROUS_DISABLE_PID_CHECK=1.`) |
It is an interesting point. Do we intend to support running envbuilder in a different way than using the official container image? |
At present, we only distribute envbuilder via Docker image. We do not provide precompiled binaries outside of this. |
|
Looking further at the options, |
|
Opening a separate PR with the actual correct fix. |
Relates to #144
This PR adds a safety check to guard against users accidentally running envbuilder on their local system.
It can be bypassed by setting an environment variable.
This is a breaking change for use-cases outside of containers.