add support for collecitng multiple user keys on via secure enclave #1644

James-Pickett · 2024-03-07T18:53:06Z

adds a launcher cmd for creating secure enclave key
adds secureenclavesigner that implements crypto.Signer
secureenclavesigner determines current console user and returns public secureenclave key for that user, execing cmd in user context

for now this just creates keys, follow on work will implement actual signing

cmd/launcher/secure_enclave_darwin.go

James-Pickett · 2024-03-07T19:01:02Z

ee/secureenclavesigner/secureenclavesigner_darwin.go

+		ses.pathToLauncherBinary = p
+	}
+
+	return ses, nil


wonder if we should do some kind of key verification here? we can at least query secure enclave to make sure it recognizes the users key.

ehh .. not sure it's worth the extra execs

we don't save a key unless secure enclave creates it
once we store the key server side, if the key is lost some how launcher would need a new install if were strict on validating SE keys

ee/secureenclavesigner/secureenclavesigner_darwin.go

ee/agent/keys.go

James-Pickett · 2024-03-07T21:42:42Z

pkg/osquery/table/launcher_info.go

+			if err != nil {
+				return nil, fmt.Errorf("marshalling hardware keys: %w", err)
+			}
+			results[0]["hardware_key"] = string(jsonBytes)


this produces a json array of objects with uid & keys, need to see how K2 handles this

This is changes the meaning of the column from prior and makes it different per platform. That's going to be hard to manage inside k2. Instead, I would recommend we move to using hardware_keys (and hardware_keys_source) and be consistent consistent across platforms.

addressed in #1647

ee/agent/keys.go

ee/agent/keys_darwin.go

ee/secureenclavesigner/secureenclavesigner_darwin.go

ee/agent/keys_tpm.go

RebeccaMahany

🔥 let's try it!

ee/secureenclavesigner/secureenclavesigner_darwin.go

RebeccaMahany

💯

directionless

Looks pretty good. Mostly nits and questions. I'm a bit ambivalent about the signature changes, but okay.

I think we do need to change how these are exposed to k2. The change in column meaning is hard to support.

directionless · 2024-03-08T00:20:06Z

cmd/launcher/secure_enclave_darwin.go

+// It's available as a separate command because launcher runs as root by default and since it's
+// not in a user security context, it can't use the secure enclave directly. However, this command
+// can be run in the user context using launchctl.
+func runSecureEnclave(args []string) error {


I cannot decide, but we could try to examine the parent process to see if launcher is in there. Though the parent is just going to be sudo, so this may not be feasible.

Early on we considered trying to do this, but decided not to since it was easy to spoof ppid. If we're feeling different now and we do go this route, we could even exec codesign and verify that the calling binary is signed as we expect.

cmd/launcher/secure_enclave_test.go

ee/secureenclavesigner/test_app_resources/info.plist

directionless · 2024-03-09T12:34:07Z

ee/localserver/krypto-ec-middleware.go

+		// hardware signing is not implemented for darwin
+		if runtime.GOOS != "darwin" && e.hardwareSigner != nil && e.hardwareSigner.Public() != nil {


Why change this?

In the current state, the signer will not be nil, but will error when sign function is called. So it didn't make sense to me to just let it error.

directionless · 2024-03-09T12:43:13Z

ee/agent/keys_tpm.go

 			return nil, fmt.Errorf("creating key: %w", err)
 		}

+		span.AddEvent("new_key_created")


do we need this? It's somewhat implied by the function name. I guess we're still learning the tools

I like seeing the events when I look at a span, but I can live without them.

directionless · 2024-03-09T12:47:19Z

pkg/osquery/extension.go

@@ -92,6 +92,9 @@ type ExtensionOpts struct {
 	// RunDifferentialQueriesImmediately allows the client to execute a new query the first time it sees it,
 	// bypassing the scheduler.
 	RunDifferentialQueriesImmediately bool
+	// skipHardwareKeysSetup is a flag to indicate if we should skip setting up hardware keys.
+	// This is useful for testing environments where we don't have required hardware.
+	skipHardwareKeysSetup bool


I like that this is explicit, but it does require changing a lot of test filed. Do you think it makes more sense than switching off the environment?

Ehh, I waffled on this one. It felt worse to have logic in the production code to try and figure out if we were under test.

ee/secureenclavesigner/secureenclavesigner_test.go

directionless · 2024-03-09T12:55:42Z

pkg/osquery/table/launcher_info.go

+			if err != nil {
+				return nil, fmt.Errorf("marshalling hardware keys: %w", err)
+			}
+			results[0]["hardware_key"] = string(jsonBytes)


This is changes the meaning of the column from prior and makes it different per platform. That's going to be hard to manage inside k2. Instead, I would recommend we move to using hardware_keys (and hardware_keys_source) and be consistent consistent across platforms.

directionless · 2024-03-09T13:02:35Z

ee/secureenclavesigner/secureenclavesigner_darwin.go

+		return nil, fmt.Errorf("getting public ecc data from store: %w", err)
+	}
+
+	if data != nil {


What happens if it's nil? That branch isn't populated. Maybe early return?

I don't we would gain anything with an early return. With the intention of having key available as soon as possible, were making a call to currentConsoleUserKey that will get/create keys near bottom of func. We would have to add another line calling this if we early returned.

Hrm. That's reasonable. But I think points to some lurking need to refactor. Maybe the quick path is to leave a comment, and call it good enough

directionless · 2024-03-09T13:03:30Z

ee/secureenclavesigner/secureenclavesigner_darwin.go

+				"err", err,
+			)
+
+			if err := store.Delete([]byte(PublicEccDataKey)); err != nil {


I think it's probably correct to delete the key if it's corrupt, but it feels a little odd to do it here in the secureenclavesigner.

Yea it feels a little odd. I though it might be better than switching on errors in the calling function, deciding what to do, then calling new again. Also, since, without some big changes to the way SetUpKeys works, we have to let secureEnclaveSIgner handle the saving of new keys. So it kinda made sense to just let it handle the whole storage layer.

directionless · 2024-03-09T13:04:07Z

ee/secureenclavesigner/secureenclavesigner_darwin.go

+		opt(ses)
+	}
+
+	if ses.pathToLauncherBinary == "" {


Feels a little odd to record this. Why not look at os.Executable each time?

This is for testing. If we look at os.Executable in a test, it will have a go test binary and will error out.

I will leave a comment about this

comment left in #1647

directionless · 2024-03-09T13:13:49Z

ee/secureenclavesigner/secureenclavesigner_darwin.go

+		"asuser",
+		u.Uid,
+		"sudo",
+		"--preserve-env",


Hrm. No idea what the risk is here. It's probably fine, but it's a bit of a yellow flag

I believe we need this to retain the os variable LAUNCHER_SKIP_UPDATES, though we could move the cmd to the top of main and bypass the update logic completely.

There are a couple of ways to approach this.

If that's the only one we care about, maybe we could --preserve-env= LAUNCHER_SKIP_UPDATES.

I think we could also invoke sudo with LAUNCHER_SKIP_UPDATES=true in the command line. from the man page:

Environment variables to be set for the command may also be passed as options to sudo in the form VAR=value, for example LD_LIBRARY_PATH=/usr/local/pkg/lib.

James-Pickett · 2024-03-11T17:18:41Z

follow up PR addressing post merge comments here #1647

James-Pickett added 2 commits March 7, 2024 10:52

add support for collecitng multiple user keys on via secure enclave

bb7805d

update root dir for secure enclave signer test

d41f914

James-Pickett commented Mar 7, 2024

View reviewed changes

James-Pickett and others added 2 commits March 7, 2024 13:00

updates secure enclave signer to fetch own data from store

b1cf533

Merge branch 'main' into james/secure-enclave-key-init

5adb031

James-Pickett commented Mar 7, 2024

View reviewed changes

ee/agent/keys.go Show resolved Hide resolved

remove unneeded code

78a0cd4

James-Pickett mentioned this pull request Mar 7, 2024

james/secure enclave cmd signer #1514

Closed

James-Pickett marked this pull request as ready for review March 7, 2024 21:31

remove unneeded mock

4ee293d

James-Pickett commented Mar 7, 2024

View reviewed changes

RebeccaMahany reviewed Mar 7, 2024

View reviewed changes

ee/agent/keys.go Show resolved Hide resolved

ee/agent/keys_darwin.go Outdated Show resolved Hide resolved

ee/secureenclavesigner/secureenclavesigner_darwin.go Outdated Show resolved Hide resolved

add tracing, create key in new

f1ebb6f

github-advanced-security bot found potential problems Mar 8, 2024

View reviewed changes

ee/agent/keys_tpm.go Fixed Show fixed Hide fixed

James-Pickett and others added 6 commits March 8, 2024 09:46

Merge branch 'main' into james/secure-enclave-key-init

6ba5fcb

lint

f730fb2

add bool for setting up hardware keys

2a6e787

skip hardware key setup in tests

9a0a3e9

more hw key set up skips

bc81209

more hardware key skipping in tests

5f22b6f

RebeccaMahany previously approved these changes Mar 8, 2024

View reviewed changes

ee/secureenclavesigner/secureenclavesigner_darwin.go Outdated Show resolved Hide resolved

ee/secureenclavesigner/secureenclavesigner_darwin.go Show resolved Hide resolved

use available contexts for logging

721794d

James-Pickett dismissed RebeccaMahany’s stale review via 721794d March 8, 2024 21:42

RebeccaMahany previously approved these changes Mar 8, 2024

View reviewed changes

dont use hardware keys for signing on darwin

022537e

James-Pickett dismissed RebeccaMahany’s stale review via 022537e March 8, 2024 22:23

RebeccaMahany approved these changes Mar 8, 2024

View reviewed changes

James-Pickett enabled auto-merge March 8, 2024 22:26

James-Pickett added this pull request to the merge queue Mar 8, 2024

Merged via the queue into kolide:main with commit 0abfa8b Mar 8, 2024
29 checks passed

James-Pickett deleted the james/secure-enclave-key-init branch March 8, 2024 22:43

directionless reviewed Mar 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for collecitng multiple user keys on via secure enclave #1644

add support for collecitng multiple user keys on via secure enclave #1644

James-Pickett commented Mar 7, 2024 •

edited

Loading

James-Pickett Mar 7, 2024

James-Pickett Mar 7, 2024 •

edited

Loading

James-Pickett Mar 7, 2024

directionless Mar 9, 2024

James-Pickett Mar 11, 2024

RebeccaMahany left a comment

RebeccaMahany left a comment

directionless left a comment

directionless Mar 8, 2024

James-Pickett Mar 11, 2024

directionless Mar 9, 2024

James-Pickett Mar 11, 2024

directionless Mar 9, 2024

James-Pickett Mar 11, 2024

directionless Mar 9, 2024

James-Pickett Mar 11, 2024

directionless Mar 9, 2024

directionless Mar 9, 2024

James-Pickett Mar 11, 2024

directionless Mar 13, 2024

directionless Mar 9, 2024

James-Pickett Mar 11, 2024

directionless Mar 9, 2024

James-Pickett Mar 11, 2024

James-Pickett Mar 11, 2024

James-Pickett Mar 11, 2024

directionless Mar 9, 2024

James-Pickett Mar 11, 2024

directionless Mar 13, 2024

James-Pickett commented Mar 11, 2024

		// hardware signing is not implemented for darwin
		if runtime.GOOS != "darwin" && e.hardwareSigner != nil && e.hardwareSigner.Public() != nil {

add support for collecitng multiple user keys on via secure enclave #1644

add support for collecitng multiple user keys on via secure enclave #1644

Conversation

James-Pickett commented Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

James-Pickett Mar 7, 2024 • edited Loading

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

RebeccaMahany left a comment

Choose a reason for hiding this comment

RebeccaMahany 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

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

James-Pickett commented Mar 11, 2024

James-Pickett commented Mar 7, 2024 •

edited

Loading

James-Pickett Mar 7, 2024 •

edited

Loading