macos presence detection #1867

James-Pickett · 2024-09-13T20:50:13Z

This PR adds a presence detection endpoint to the DesktopUserServer which will call the OS specific implementation of presence detection and return the result. The call to DesktopUserServer is made by middleware in the LocalServer. The LocalServer looks for values in cmdRequest (krypto) headers to determine when it needs to detect presence.

More docs here: https://github.com/kolide/monorepo/pull/240

relates to #1838

ee/localserver/server.go

… middleware

RebeccaMahany

Nice!!

RebeccaMahany · 2024-09-20T16:18:28Z

cmd/launcher/desktop.go

@@ -182,9 +184,10 @@ func runDesktop(_ *multislogger.MultiSlogger, args []string) error {
 		}
 	}()

+	// block until a send on showDesktopChan
+	<-showDesktopChan


Ooh, this is tidy!

I was torn between this route and just passing a flag when we spin up desktop. We are able to show the desktop on demand, but as far as I can tell, there is no way to hide the desktop on demand. When you tell systray to shutdown, it exits the program completely. So if we ever want to hide it, we have to kill the process and let a new one spin up hidden.

I chose this route because I think it's rare that we would turn off the desktop and I don't want to be starting / killing processes on a first install where a user is trying auth for the first time.

I think it's rare that we would turn off the desktop

I agree, I think that's a reasonable assumption -- this route makes sense to me

I think users will, but I think your reasoning is sound.

ee/desktop/user/server/server.go

ee/presencedetection/presencedetection_darwin.go

ee/presencedetection/presencedetection.go

ee/desktop/runner/runner.go

ee/localserver/server.go

ee/presencedetection/presencedetection.go

ee/presencedetection/presencedetection_darwin.go

ee/desktop/runner/runner.go

zackattack01

🔥

ee/desktop/user/server/server.go

…erver response header

directionless

love it

ee/localserver/server.go

directionless · 2024-09-26T21:16:13Z

cmd/launcher/desktop.go

@@ -182,9 +184,10 @@ func runDesktop(_ *multislogger.MultiSlogger, args []string) error {
 		}
 	}()

+	// block until a send on showDesktopChan
+	<-showDesktopChan


I think users will, but I think your reasoning is sound.

ee/desktop/user/client/client.go

ee/presencedetection/presencedetection.go

directionless · 2024-09-26T21:32:09Z

ee/presencedetection/presencedetection.go

+const DetectionFailedDurationValue = -1 * time.Second
+
+type PresenceDetector struct {
+	lastDetectionUTC          time.Time


if this is time.Time maybe drop the UTC?

will drop it, but we still want UTC instead of local time I'm assuming

Generally it's better to keep things in UTC, yes. And when we emit to k2, we should use one of the standard formats, or just integer unix time

cool, we don't emit the time for this to K2, just the go style duration since last detection like 0s or 1m30s

ee/presencedetection/presencedetection_darwin_test.go

James-Pickett · 2024-09-27T16:19:22Z

ee/localserver/krypto-ec-middleware.go

@@ -334,6 +344,14 @@ func (e *kryptoEcMiddleware) Wrap(next http.Handler) http.Handler {

 		w.Header().Add(kolideKryptoHeaderKey, kolideKryptoEccHeader20230130Value)

+		for k, v := range bhr.Header() {


These headers are outside the krypto box, is that okay?

It would be better if they were signed, but also not fatal. I thought if we put them into the CmdReq they'd be inside?

The headers of the request are inside the CmdReq (in krypto box). We don't have a mechanism currently to add headers from the response to the krypto box. Currently we just grab the body from the response and put it in the krypto box. We could come up with some kind of base response that we imbed in each of the local server responses.

I'm confused. We have too many layers

I think it's important for the response from launcher to k2, to be in the box. Otherwise it's a bit too trivial to slap whatever you want in flight.

But I'm not sure I'm concerned about what's happening between localserver and the desktop.

James-Pickett · 2024-09-27T16:19:39Z

ee/localserver/krypto-ec-middleware.go

@@ -334,6 +344,14 @@ func (e *kryptoEcMiddleware) Wrap(next http.Handler) http.Handler {

 		w.Header().Add(kolideKryptoHeaderKey, kolideKryptoEccHeader20230130Value)

+		for k, v := range bhr.Header() {


Do we want to blindly copy all headers or cherry pick?

directionless

I'm game to try! wooo!

ee/localserver/krypto-ec-middleware.go

RebeccaMahany

A couple very small comments -- feel free to ignore them or punt to another PR. LGTM!

ee/desktop/user/server/server.go

ee/localserver/server.go

ee/presencedetection/auth.m

James-Pickett added 3 commits September 13, 2024 13:50

first pass poc at macos presence detection

9d05605

naming

97f4287

rough exec implementation of presence detection

566d5bd

James-Pickett commented Sep 17, 2024

View reviewed changes

ee/localserver/server.go Show resolved Hide resolved

James-Pickett added 2 commits September 18, 2024 15:00

add headers to v2CmdRequestType, act on headers in presence detection…

ecd9d3e

… middleware

dont show desktop until requested by runner

d7efbab

RebeccaMahany reviewed Sep 20, 2024

View reviewed changes

do presence detection through desktop server

4b99368

James-Pickett commented Sep 20, 2024

View reviewed changes

ee/desktop/runner/runner.go Outdated Show resolved Hide resolved

ee/desktop/runner/runner.go Outdated Show resolved Hide resolved

ee/desktop/runner/runner.go Show resolved Hide resolved

ee/localserver/server.go Outdated Show resolved Hide resolved

clean up, lint

c1eb82d

James-Pickett commented Sep 20, 2024

View reviewed changes

ee/presencedetection/presencedetection.go Outdated Show resolved Hide resolved

James-Pickett commented Sep 20, 2024

View reviewed changes

ee/presencedetection/presencedetection_darwin.go Show resolved Hide resolved

James-Pickett added 5 commits September 23, 2024 10:14

look for reason in header with default, remove unneeded struct

635ec1b

put c in own file

c87a128

dont use desktop runner singleton

47fbb32

build tags in c files

1fd46c9

presence detection tests

888643f

James-Pickett changed the title ~~first pass poc at macos presence detection~~ macos presence detection Sep 23, 2024

test that headers are present after opening krypto challenge

ddc3ada

zackattack01 reviewed Sep 24, 2024

View reviewed changes

ee/desktop/runner/runner.go Outdated Show resolved Hide resolved

update log

9524d0d

zackattack01 reviewed Sep 24, 2024

View reviewed changes

James-Pickett added 2 commits September 24, 2024 11:32

dont display desktop in test, just manage process

cd65542

increase runner test start up time

6f3bb1a

James-Pickett marked this pull request as ready for review September 24, 2024 20:29

James-Pickett commented Sep 25, 2024

View reviewed changes

ee/desktop/user/server/server.go Outdated Show resolved Hide resolved

James-Pickett added 3 commits September 26, 2024 11:28

presence detector return durtion since last detection, add to local s…

15a981d

…erver response header

tweaks

aa824b1

skip presence test on non darwin

8747dd0

directionless reviewed Sep 26, 2024

View reviewed changes

James-Pickett added 2 commits September 26, 2024 14:51

simplify presence detector

e12a332

presence detection via ec middleware

dcaa01e

James-Pickett commented Sep 27, 2024

View reviewed changes

James-Pickett added 3 commits September 27, 2024 10:53

feedback

98c8c51

make presence detection part of test darwin only

4d952b2

add headers property to response in krypto middleware

4e75514

directionless previously approved these changes Oct 1, 2024

View reviewed changes

ee/localserver/krypto-ec-middleware.go Outdated Show resolved Hide resolved

ee/localserver/krypto-ec-middleware.go Show resolved Hide resolved

comments, feedback

c3691bf

James-Pickett dismissed directionless’s stale review via c3691bf October 1, 2024 14:57

clean tests

98a049b

directionless previously approved these changes Oct 1, 2024

View reviewed changes

RebeccaMahany previously approved these changes Oct 1, 2024

View reviewed changes

ee/desktop/user/server/server.go Outdated Show resolved Hide resolved

ee/localserver/server.go Outdated Show resolved Hide resolved

ee/presencedetection/auth.m Outdated Show resolved Hide resolved

logging tweaks, use same reason for password auth

f7c79bf

James-Pickett dismissed stale reviews from RebeccaMahany and directionless via f7c79bf October 1, 2024 18:42

RebeccaMahany approved these changes Oct 2, 2024

View reviewed changes

James-Pickett added this pull request to the merge queue Oct 2, 2024

Merged via the queue into kolide:main with commit a6ba352 Oct 2, 2024
29 checks passed

James-Pickett deleted the james/presence-detection-macos branch October 2, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

macos presence detection #1867

macos presence detection #1867

James-Pickett commented Sep 13, 2024 •

edited

Loading

RebeccaMahany left a comment

RebeccaMahany Sep 20, 2024

James-Pickett Sep 20, 2024

RebeccaMahany Sep 20, 2024

directionless Sep 26, 2024

zackattack01 left a comment

directionless left a comment

directionless Sep 26, 2024

directionless Sep 26, 2024

James-Pickett Sep 27, 2024

directionless Oct 1, 2024

James-Pickett Oct 1, 2024

James-Pickett Sep 27, 2024

directionless Sep 27, 2024

James-Pickett Sep 27, 2024

directionless Sep 27, 2024

directionless Sep 27, 2024

James-Pickett Sep 27, 2024

directionless left a comment

RebeccaMahany left a comment

		@@ -334,6 +344,14 @@ func (e *kryptoEcMiddleware) Wrap(next http.Handler) http.Handler {

		w.Header().Add(kolideKryptoHeaderKey, kolideKryptoEccHeader20230130Value)

		for k, v := range bhr.Header() {

macos presence detection #1867

macos presence detection #1867

Conversation

James-Pickett commented Sep 13, 2024 • edited Loading

RebeccaMahany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackattack01 left a comment

Choose a reason for hiding this comment

directionless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

directionless left a comment

Choose a reason for hiding this comment

RebeccaMahany left a comment

Choose a reason for hiding this comment

James-Pickett commented Sep 13, 2024 •

edited

Loading