gokrazy / tools

this repository contains the gok CLI tool of gokrazy
https://gokrazy.org
BSD 3-Clause "New" or "Revised" License
50 stars 26 forks source link

parttable: pass HOME env var to sudo #44

Closed markdrayton closed 1 year ago

markdrayton commented 1 year ago

From the commit message:

Without passing $HOME into the `sudo` invocation the instance parent directory detection
(https://github.com/gokrazy/internal/blob/main/instanceflag/instanceflag.go#L19) fails:

  $ gok -i test overwrite --full /dev/sdc
  [..]
  2023/01/25 22:13:04 partitioning /dev/sdc (GPT + Hybrid MBR)
  2023/01/25 22:13:04 Using sudo to gain permission to format /dev/sdc
  2023/01/25 22:13:04 If you prefer, cancel and use: sudo setfacl -m u:${USER}:rw /dev/sdc
  2023/01/25 22:13:04 open os.UserHomeDir failed: $HOME is not defined/gokrazy/test/config.json: no such file or directory
  2023/01/25 22:13:04 exit status 1

This commit does the necessary passing.

While the attached commit fixes the immediate issue I'm not sure it's the right fix, so please do make suggestions if you have any.

(I wondered if we should be Completely Honest (tm) and only set $HOME if it's set in the parent's environment (using os.LookupEnv first) but that seems a) unnecessarily complicated and b) at any rate os.UserHomeDir() considers both a missing $HOME and one with an empty value as both signifying no home directory found.)

Also, something seems broken (or I don't understand it!) in instanceflag.parentDir:

homeDir, err := os.UserHomeDir()
if err != nil {
        homeDir = fmt.Sprintf("os.UserHomeDir failed: %v", err)
}
def = filepath.Join(homeDir, "gokrazy")

it's not clear to my why you'd want to treat the error as a path component. I'd guess the intended behaviour here is to either abort (panic?) if the home dir can't be determined, or maybe use the current working directory as parent directory? Ultimately the configuration location has be inferred somehow, if not passed explicitly, so I suppose failing if the home directory isn't found is fair.

stapelberg commented 1 year ago

While the attached commit fixes the immediate issue I'm not sure it's the right fix

It is the right fix, thanks!

I was wondering why I didn’t run into this problem during my own testing, but then I realized that on my main PC, my user is part of the disk group which gives it permission to write to e.g. /dev/sda without elevating privileges, thereby circumventing this code path entirely…

it's not clear to my why you'd want to treat the error as a path component.

Error handling is inconvenient in this place. The flag default needs to be initialized at init time, so plumbing an error return through is not that easy. I wanted to strike a balance between failing loudly and being pragmatic here :)

markdrayton commented 1 year ago

I wanted to strike a balance between failing loudly and being pragmatic here :)

Fair enough!