launcher: log on Windows options parsing, better handle path overrides - #2800
Conversation
7604140 to
eae676d
Compare
brhoades
changed the title
0419a5e to
f94be89
Compare
…leged processes As it was written, unprivileged launcher is silently forced into a path it cannot write to if an existing installation exists.
a136163 to
98fd8e3
Compare
RebeccaMahany
left a comment
RebeccaMahany
left a comment
There was a problem hiding this comment.
LGTM, just had one suggestion for reusability. Worth getting a look over from @zackattack01 too since he worked on the root dir override.
| // Detects UAC elevation or when running as LocalSystem. | ||
| // Impl is copied from windows.Token.IsElevated, but exposes the error | ||
| // on a failure to check. | ||
| func runningElevated() (bool, error) { |
There was a problem hiding this comment.
I see a couple other places where we make this check (searching for GetCurrentProcessToken -- one in the watchdog, one with the syslogger, plus a cross-platform one in the flare for invoked_with_elevated_permissions metadata). Could we export this + implement for unix (looks like we use os.Geteuid() == 0 there?), and consolidate?
There was a problem hiding this comment.
Yes!
I plan to combine a few of these for sure. I didn't do it here because the main spot I want to swap, desktopRunner, has a couple of bugs to fix too. And there's a whole PR's worth of considerations about how to handle a failure to check if elevated 😓.
There was a problem hiding this comment.
I'm fine with punting to a follow-up PR!
|
|
||
| // Unreachable on non-Windows, included for compilation. | ||
| func runningElevated() (bool, error) { | ||
| return false, errors.New("OS does not support elevation check") |
There was a problem hiding this comment.
Mentioned elsewhere, might as well implement here too -- return os.Geteuid() == 0, nil
This puts us in line with how it used to work. That way, if for some reason launchers in the wild fail to check privileges they keep doing whatever they were doing.
|
Sneaking in d987400 so that we fail open if we cannot check if we're privileged. That matches the prior behavior. |
| } | ||
|
|
||
| // override paths are not usable for unprivileged users | ||
| if !opts.isPrivileged { |
There was a problem hiding this comment.
ohh hmm thinking about this more I'm wondering if this can cause issues with the desktop process, it would be rare but-
- Say launcher install is running for months from original root directory, database is there
- MDM installs newer launcher with the newer configured root directory in the flags file (this is the historical problem the override attempts to solve)
- root launcher process would know to do the override, but I think the unprivileged desktop process would get out of sync, deferring to the opts root directory
There was a problem hiding this comment.
I'm trying to think of a way around this but unsure of some context here- what is the motivation behind adding the check here? shouldn't all of the old and new directories have the same permissions now?
There was a problem hiding this comment.
root launcher process would know to do the override, but I think the unprivileged desktop process would get out of sync, deferring to the opts root directory
This tripped me up, but I think since we pass MENU_PATH, LOCALIZATION_PATH, ICON_PATH in to the desktop process, this isn't an issue? The desktop has its own flag parsing separate from options. (Even if not an issue, worth leaving a comment about it.)
There was a problem hiding this comment.
ahh yep you're totally right, should have looked harder there. okay perfect, then yeah I'd just add a comment (maybe somewhere in cmd/launcher/desktop.go) that if we ever use launcher.ParseOptions we would need to account for the potential split in root directory based on privileges to prevent the desktop process from thinking it has a different root directory than a root process running an older installer
There was a problem hiding this comment.
it would be rare but- [mdm install scenario over an existing installation]
💯 I don't love this override flow myself. It's tricky. It's surprising. It bit me.
On the desktop path: right, the parent process specifies the binary explicitly and turns off detection of newer binaries. Everything is explicitly passed by the parent in its own options parsing flow.
shouldn't all of the old and new directories have the same permissions now
I'm not familiar with the history referenced here. I saw remnants of some MSI pain or migrations, but I don't know where we landed.
I'm trying to think of a way around this but unsure of some context here- what is the motivation behind adding the check here?
The specific line highlighted by this thread is only for launcher proper, non-desktop, while unprivileged on Windows. It works, but it cannot work if it's using system-owned directories as a normal user. That could be clearer.
I'm not sure how to word advice on an unused dependency that doesn't really work in runDesktop 😅? But the meat of the thread is clear: it's confusing and requires knowing where this is invoked to know how it works. That led me to rewrite this PR three times in three different ways.
I'll add, edit, and clean up docs in a few spots that should help future souls.
There was a problem hiding this comment.
thanks that looks great to me. not for this PR but after reading some of your overview I'm wondering if you should consider some sort of config flag wired through knapsack for this kinda thing. especially if we're expecting this work to be for new installs only, it might give you more rope to opt-out of a lot of the legacy supporting logic a little easier
There was a problem hiding this comment.
Oh, for unprivileged specifically? Or for the Windows overrides?
I don't think unprivileged shouldn't be exciting, but the Windows override logic should be legacy if I understand right? But yeah, agreed! Let's keep this simple.
It was unclear without seeing the broader picture. Sprinkled more in a few spots to help.
3 participants
Plumbs and uses a logger in launcher options parsing. Previously on Windows, options parsing quietly overrode the configured root directory if a well-known database location existed. This is made more confusing by privileged Windows launchers not logging options parsing at all.
Changes on Windows:
I'm not very familiar with the history of why this is present, but I believe this is backwards compatible with solving the MSI issue? Please let me know.
Testing - Unprivileged
Command:
Fresh Run
{"time":"2026-08-24T18:44:29.5801767Z","level":"INFO","msg":"launcher running on windows without elevated permissions, using default stdout instead of eventlog"} {"time":"2026-08-24T18:44:29.5801767Z","level":"INFO","msg":"launcher starting up","version":"unknown","revision":"unknown"} {"time":"2026-08-24T18:44:29.5801767Z","level":"INFO","msg":"got new version of launcher to run","old_version":"unknown","new_binary_version":"","new_binary_path":"C:\\Users\\billy\\kolide-2\\launcher.exe"} {"time":"2026-08-24T18:44:29.5801767Z","level":"INFO","msg":"found newer version of launcher to run","new_binary":"C:\\Users\\billy\\kolide-2\\launcher.exe"} {"time":"2026-08-24T18:44:29.5801767Z","level":"INFO","msg":"preparing to run command","cmd":"C:\\Users\\billy\\kolide-2\\launcher.exe --config launcher.flags","component":"execwrapper"} {"time":"2026-08-24T18:44:33.4751211Z","level":"INFO","msg":"launcher running on windows without elevated permissions, using default stdout instead of eventlog"} {"time":"2026-08-24T18:44:33.4787187Z","level":"INFO","msg":"launcher starting up","version":"unknown","revision":"unknown"} {"time":"2026-08-24T18:44:33.4808552Z","level":"WARN","msg":"not running elevated, initializing launcher at configured root path and skipping well-known override locations","root_directory":"C:\\Users\\billy\\kolide-2\\"} {"time":"2026-08-24T18:44:33.7710365Z","level":"ERROR","msg":"{\"caller\":\"logshipper.go:97\",\"component\":\"logshipper\",\"err\":\"no token found\",\"msg\":\"updating auth token\",\"run_id\":\"01M0THDBAAVQ8JFM33AR3KY1GK\",\"session_pid\":8648,\"severity\":\"debug\",\"ts\":\"2026-08-24T18:44:33.7705801Z\"}","cmd":"C:\\Users\\billy\\kolide-2\\launcher.exe --config launcher.flags","component":"execwrapper","subcomponent":"cmd_stderr"} {"time":"2026-08-24T18:44:33.8957153Z","level":"INFO","msg":"started kolide launcher","version":"unknown","build":"unknown","span_id":"189273e3c87cc764","trace_id":"c1e598bd21ef6f6974a6435e2cf8d689","trace_sampled":true}Second Run
{"time":"2026-08-24T18:44:47.5153092Z","level":"INFO","msg":"launcher running on windows without elevated permissions, using default stdout instead of eventlog"} {"time":"2026-08-24T18:44:47.5153092Z","level":"INFO","msg":"launcher starting up","version":"unknown","revision":"unknown"} {"time":"2026-08-24T18:44:47.5153092Z","level":"INFO","msg":"got new version of launcher to run","old_version":"unknown","new_binary_version":"","new_binary_path":"C:\\Users\\billy\\kolide\\launcher.exe"} {"time":"2026-08-24T18:44:47.5153092Z","level":"INFO","msg":"found newer version of launcher to run","new_binary":"C:\\Users\\billy\\kolide\\launcher.exe"} {"time":"2026-08-24T18:44:47.5153092Z","level":"INFO","msg":"preparing to run command","cmd":"C:\\Users\\billy\\kolide\\launcher.exe --config launcher.flags","component":"execwrapper"} {"time":"2026-08-24T18:44:47.6925156Z","level":"INFO","msg":"launcher running on windows without elevated permissions, using default stdout instead of eventlog"} {"time":"2026-08-24T18:44:47.6925156Z","level":"INFO","msg":"launcher starting up","version":"unknown","revision":"unknown"}Flare
Doctor
Testing - Privileged
System Service
Privileged Shell - Fresh
Windows intentionally skips attaching stdout logging, so we get logs after options parsing in this case and only in debug.json/system log. They point to the right files being used.
Doctor