Skip to content

init ee/currentprocess, centralize current process query APIs, fix unprivileged desktop runner - #2804

Open
brhoades wants to merge 7 commits into
kolide:mainfrom
brhoades:billy/fix-desktop-runner-windows
Open

init ee/currentprocess, centralize current process query APIs, fix unprivileged desktop runner#2804
brhoades wants to merge 7 commits into
kolide:mainfrom
brhoades:billy/fix-desktop-runner-windows

Conversation

@brhoades

@brhoades brhoades commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Why are you reading this:
I promised in #2800 to port and generalize process IsElevated logic. It's done. I used it to fix unprivileged Windows desktop runner detection and found a few weird bits to tweak.

Summary:

  • Add ee/currentprocess and which has cross-platform IsElevated() (bool, error) and Uid()
  • Consume new currentprocess API in flare, doctor, windows syslogger, desktop runner, and launcher options parsing.
  • desktopRunner is now aware of whether it's running privileged.
    • On POSIX platforms, this is minor. It does not run sudo/launchctl.
    • On Windows, it does not query the console users nor use the active user's token; both error.
    • Across all platforms this is passive to privileged execution.

I've left comments on the review for areas I'm least happy about or context.

Fixes:

  • Unprivileged Windows could not spawn a desktop process at all.
  • Unprivileged Linux from a user service or non-graphical shell launched, but never showed a tray icon.
  • Transient failures in Linux/MacOS on user.CurrentUser() are cached, so were permanently fatal to the runner.
  • Dead code: errors.Is(err, NoExplorerProcessError{}) was always false.

Known remaining bugs:

  • Failure to spawn for any console user can block spawning for others.

Testing

I ran the general launcher privileged and unprivileged on all three platforms. It detected permissions when it had them or when it didn't. Logs available, but I didn't attach them since I'm worried they may have credentials in them.

On Windows, that led me to discover that unprivileged users can't query console users at all which was fatal:

json failure log ```json { "time": "2026-08-26T20:36:33.257766Z", "level": "ERROR", "source": { "function": "github.com/kolide/launcher/v2/ee/desktop/runner.(*DesktopUsersProcessesRunner).Execute", "file": "/home/billy/work/launcher/ee/desktop/runner/runner.go", "line": 271 }, "msg": "could not run console user desktop process", "component": "desktop_runner", "err": "getting console users: getting logon session data for LUID: Access is denied." } ```

Also worth noting that unprivileged users have a complicated set of requirements to spawn a process to their own user's graphical session:

json failure log ```json { "time": "2026-09-11T15:47:16.7741552Z", "level": "ERROR", "source": { "function": "github.com/kolide/launcher/v2/ee/desktop/runner.(*DesktopUsersProcessesRunner).Execute", "fil e": "/home/billy/work/launcher/ee/desktop/runner/runner.go", "line": 300 }, "msg": "could not run console user desktop process", "component": "desktop_runner", "err": "spawning new desktop user process for DESKTOP-1P62ADB\\billy: running desktop command as user: fork/exec C:\\Users\\billy\\kolide\\launcher-edd16a0c-dirty-change-token-bind.exe: A required privilege is not held by the client." } ```

Questions

  1. I assumed we can add flare fields without any changes in k2. I also figured it'd be clearer to add an error key than to push it all into the old field. Is that right?
  2. Is ee/currentprocess the right folder/shape for a common API here? I didn't see clear distinction between pkg and ee.

Comment on lines +898 to +902
// Querying console users on Windows requires privilege, other platforms
// use session information to establish process configuration even when ran
// unprivileged
if runtime.GOOS == "windows" && !r.elevated {
consoleUsers = []string{r.currentUid}

@brhoades brhoades Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this, but an alternative distracts from my changeset: extract the loop's body below and call it for !r.elevated on all platforms. I can do it if preferred.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this -- since the launcher parent process being unprivileged is a new case (i.e. we always currently expect the parent process to be privileged), we can define whatever behavior we want here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root check was harmful in some cases. When ran as a systemd user unit (which seems plausible) or tmux (which is probably just me), the process won't have DISPLAY.

Also worth noting that userEnvVars below worked fine unprivileged on my box.

@brhoades
brhoades force-pushed the billy/fix-desktop-runner-windows branch from c9254dc to f92c99f Compare September 8, 2026 20:50
Comment on lines -255 to +257
if !windows.GetCurrentProcessToken().IsElevated() {
if elevated, err := currentprocess.IsElevated(); err != nil {
// historical behavior treats failure to check as not elevated
wc.slogger.Log(context.TODO(), slog.LevelError, "failed to check if process is elevated", "err", err)
return false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The historical behavior here is from IsElevated() which treats a failure to get token info as unprivileged:

// IsElevated returns whether the current token is elevated from a UAC perspective.
func (token Token) IsElevated() bool {
	var isElevated uint32
	var outLen uint32
	err := GetTokenInformation(token, TokenElevation, (*byte)(unsafe.Pointer(&isElevated)), uint32(unsafe.Sizeof(isElevated)), &outLen)
	if err != nil {
		return false
	}
	return outLen == uint32(unsafe.Sizeof(isElevated)) && isElevated != 0
}

If written today, I'd rather we fail to configure watchdog when unprivileged than not configure watchdog when privileged and a Windows API misbehaves.

Any opinions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me -- @zackattack01 ?

@brhoades
brhoades force-pushed the billy/fix-desktop-runner-windows branch 2 times, most recently from edd16a0 to 9e8bfeb Compare September 11, 2026 16:10
Signed-off-by: Billy J Rhoades II <billy.rhoades@agilebits.com>
…alculation

Signed-off-by: Billy J Rhoades II <billy.rhoades@agilebits.com>
Signed-off-by: Billy J Rhoades II <billy.rhoades@agilebits.com>
@brhoades
brhoades force-pushed the billy/fix-desktop-runner-windows branch from 9e8bfeb to 6b48b14 Compare September 11, 2026 16:30
A slight refactor lets us bail earlier and runAs only in specific cases.

Signed-off-by: Billy J Rhoades II <billy.rhoades@agilebits.com>
Signed-off-by: Billy J Rhoades II <billy.rhoades@agilebits.com>
Signed-off-by: Billy J Rhoades II <billy.rhoades@agilebits.com>
@brhoades
brhoades marked this pull request as ready for review September 11, 2026 17:25
@RebeccaMahany

Copy link
Copy Markdown
Contributor

I assumed we can add flare fields without any changes in k2. I also figured it'd be clearer to add an error key than to push it all into the old field. Is that right?

It should be fine -- you should be able to test by generating a flare and confirming k2 consumes it and displays it appropriately on the flare page.

Is ee/currentprocess the right folder/shape for a common API here? I didn't see clear distinction between pkg and ee.

Yes, this is a good spot for it. In general, new code lives in ee (which has a different license from pkg).

Comment thread ee/debug/checkups/flare-environment-platform-specifics_windows.go
RebeccaMahany
RebeccaMahany previously approved these changes Sep 11, 2026

@RebeccaMahany RebeccaMahany left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one small documentation request

Comment on lines +898 to +902
// Querying console users on Windows requires privilege, other platforms
// use session information to establish process configuration even when ran
// unprivileged
if runtime.GOOS == "windows" && !r.elevated {
consoleUsers = []string{r.currentUid}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this -- since the launcher parent process being unprivileged is a new case (i.e. we always currently expect the parent process to be privileged), we can define whatever behavior we want here.

Comment thread ee/desktop/runner/runner_darwin.go
Comment on lines -255 to +257
if !windows.GetCurrentProcessToken().IsElevated() {
if elevated, err := currentprocess.IsElevated(); err != nil {
// historical behavior treats failure to check as not elevated
wc.slogger.Log(context.TODO(), slog.LevelError, "failed to check if process is elevated", "err", err)
return false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me -- @zackattack01 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants