From 3f86421a427f2878b007a310dc9058553ac3a3c9 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Wed, 4 Dec 2024 15:16:32 -0800 Subject: [PATCH 01/26] rough draft --- cmd/launcher/launcher.go | 8 +- cmd/launcher/main.go | 2 - cmd/launcher/secure_enclave_darwin.go | 49 --- cmd/launcher/secure_enclave_other.go | 14 - cmd/launcher/secure_enclave_test.go | 154 --------- ee/agent/keys.go | 79 +---- ee/agent/keys_darwin.go | 11 +- ee/agent/keys_tpm.go | 46 +-- ee/debug/shipper/shipper_test.go | 2 +- ee/desktop/runner/runner.go | 15 + ee/desktop/user/client/client.go | 32 ++ ee/desktop/user/server/server.go | 1 + ee/desktop/user/server/server_darwin.go | 29 ++ ee/desktop/user/server/server_other.go | 10 + ee/localserver/krypto-ec-middleware.go | 20 +- ee/localserver/krypto-ec-middleware_test.go | 4 +- ee/localserver/server.go | 20 +- .../mocks/secureEnclaveClient.go | 58 ++++ ee/secureenclaverunner/secureenclaverunner.go | 281 ++++++++++++++++ .../secureenclaverunner_test.go | 93 ++++++ .../secureenclavesigner_darwin.go | 313 ------------------ .../secureenclavesigner_test.go | 171 ---------- .../embedded.provisionprofile | Bin 16825 -> 0 bytes .../test_app_resources/entitlements | 8 - .../test_app_resources/info.plist | 20 -- .../test_app_resources/readme.md | 31 -- ee/tpmrunner/mocks/tpmSignerCreator.go | 104 ++++++ ee/tpmrunner/tpmrunner.go | 215 ++++++++++++ ee/tpmrunner/tpmrunner_test.go | 80 +++++ 29 files changed, 965 insertions(+), 905 deletions(-) delete mode 100644 cmd/launcher/secure_enclave_darwin.go delete mode 100644 cmd/launcher/secure_enclave_other.go delete mode 100644 cmd/launcher/secure_enclave_test.go create mode 100644 ee/desktop/user/server/server_darwin.go create mode 100644 ee/desktop/user/server/server_other.go create mode 100644 ee/secureenclaverunner/mocks/secureEnclaveClient.go create mode 100644 ee/secureenclaverunner/secureenclaverunner.go create mode 100644 ee/secureenclaverunner/secureenclaverunner_test.go delete mode 100644 ee/secureenclavesigner/secureenclavesigner_darwin.go delete mode 100644 ee/secureenclavesigner/secureenclavesigner_test.go delete mode 100644 ee/secureenclavesigner/test_app_resources/embedded.provisionprofile delete mode 100644 ee/secureenclavesigner/test_app_resources/entitlements delete mode 100644 ee/secureenclavesigner/test_app_resources/info.plist delete mode 100644 ee/secureenclavesigner/test_app_resources/readme.md create mode 100644 ee/tpmrunner/mocks/tpmSignerCreator.go create mode 100644 ee/tpmrunner/tpmrunner.go create mode 100644 ee/tpmrunner/tpmrunner_test.go diff --git a/cmd/launcher/launcher.go b/cmd/launcher/launcher.go index 1a9bb8a43..613559fb4 100644 --- a/cmd/launcher/launcher.go +++ b/cmd/launcher/launcher.go @@ -357,7 +357,7 @@ func runLauncher(ctx context.Context, cancel func(), multiSlogger, systemMultiSl if err := osquery.SetupLauncherKeys(k.ConfigStore()); err != nil { return fmt.Errorf("setting up initial launcher keys: %w", err) } - if err := agent.SetupKeys(ctx, k.Slogger(), k.ConfigStore(), false); err != nil { + if err := agent.SetupKeys(ctx, k.Slogger(), k.ConfigStore()); err != nil { return fmt.Errorf("setting up agent keys: %w", err) } @@ -434,6 +434,12 @@ func runLauncher(ctx context.Context, cancel func(), multiSlogger, systemMultiSl return fmt.Errorf("failed to create desktop runner: %w", err) } + execute, interrupt, err := agent.SetupHardwareKeys(ctx, k.Slogger(), k.ConfigStore(), runner) + if err != nil { + return fmt.Errorf("setting up hardware keys: %w", err) + } + runGroup.Add("hardwareKeys", execute, interrupt) + runGroup.Add("desktopRunner", runner.Execute, runner.Interrupt) controlService.RegisterConsumer(desktopMenuSubsystemName, runner) diff --git a/cmd/launcher/main.go b/cmd/launcher/main.go index d4fb53c6a..ab1e85322 100644 --- a/cmd/launcher/main.go +++ b/cmd/launcher/main.go @@ -188,8 +188,6 @@ func runSubcommands(systemMultiSlogger *multislogger.MultiSlogger) error { run = runDownloadOsquery case "uninstall": run = runUninstall - case "secure-enclave": - run = runSecureEnclave case "watchdog": // note: this is currently only implemented for windows run = watchdog.RunWatchdogTask default: diff --git a/cmd/launcher/secure_enclave_darwin.go b/cmd/launcher/secure_enclave_darwin.go deleted file mode 100644 index 4c7d7bca4..000000000 --- a/cmd/launcher/secure_enclave_darwin.go +++ /dev/null @@ -1,49 +0,0 @@ -//go:build darwin -// +build darwin - -package main - -import ( - "errors" - "fmt" - "os" - - "github.com/kolide/krypto/pkg/echelper" - "github.com/kolide/krypto/pkg/secureenclave" - "github.com/kolide/launcher/ee/secureenclavesigner" - "github.com/kolide/launcher/pkg/log/multislogger" -) - -// runSecureEnclave performs either a create-key operation using the secure enclave. -// 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(_ *multislogger.MultiSlogger, args []string) error { - // currently we are just creating key, but plan to add sign command in future - if len(args) < 1 { - return errors.New("not enough arguments, expect create_key") - } - - switch args[0] { - case secureenclavesigner.CreateKeyCmd: - return createSecureEnclaveKey() - - default: - return fmt.Errorf("unknown command %s", args[0]) - } -} - -func createSecureEnclaveKey() error { - secureEnclavePubKey, err := secureenclave.CreateKey() - if err != nil { - return fmt.Errorf("creating secure enclave key: %w", err) - } - - secureEnclavePubDer, err := echelper.PublicEcdsaToB64Der(secureEnclavePubKey) - if err != nil { - return fmt.Errorf("marshalling public key to der: %w", err) - } - - os.Stdout.Write(secureEnclavePubDer) - return nil -} diff --git a/cmd/launcher/secure_enclave_other.go b/cmd/launcher/secure_enclave_other.go deleted file mode 100644 index 62c722476..000000000 --- a/cmd/launcher/secure_enclave_other.go +++ /dev/null @@ -1,14 +0,0 @@ -//go:build !darwin -// +build !darwin - -package main - -import ( - "errors" - - "github.com/kolide/launcher/pkg/log/multislogger" -) - -func runSecureEnclave(_ *multislogger.MultiSlogger, args []string) error { - return errors.New("not implemented on non darwin platforms") -} diff --git a/cmd/launcher/secure_enclave_test.go b/cmd/launcher/secure_enclave_test.go deleted file mode 100644 index 9dd3ba28f..000000000 --- a/cmd/launcher/secure_enclave_test.go +++ /dev/null @@ -1,154 +0,0 @@ -//go:build darwin -// +build darwin - -package main - -import ( - "bytes" - "context" - "fmt" - "os" - "os/exec" - "path/filepath" - "testing" - "time" - - "github.com/kolide/krypto/pkg/echelper" - "github.com/kolide/launcher/ee/secureenclavesigner" - "github.com/kolide/launcher/pkg/log/multislogger" - "github.com/stretchr/testify/require" -) - -const ( - testWrappedEnvVarKey = "SECURE_ENCLAVE_TEST_WRAPPED" - testSkipSecureEnclaveTestingEnvVarKey = "SKIP_SECURE_ENCLAVE_TESTS" - macOsAppResourceDir = "../../ee/secureenclavesigner/test_app_resources" -) - -// TestSecureEnclaveTestRunner creates a MacOS app with the binary of this packages tests, then signs the app with entitlements and runs the tests. -// This is done because in order to access secure enclave to run tests, we need MacOS entitlements. -// #nosec G306 -- Need readable files -func TestSecureEnclaveTestRunner(t *testing.T) { - t.Parallel() - - if os.Getenv("CI") != "" { - t.Skipf("\nskipping because %s env var was not empty, this is being run in a CI environment without access to secure enclave", testWrappedEnvVarKey) - } - - if os.Getenv(testWrappedEnvVarKey) != "" { - t.Skipf("\nskipping because %s env var was not empty, this is the execution of the codesigned app with entitlements", testWrappedEnvVarKey) - } - - if os.Getenv(testSkipSecureEnclaveTestingEnvVarKey) != "" { - t.Skipf("\nskipping because %s env var was set", testSkipSecureEnclaveTestingEnvVarKey) - } - - t.Log("\nexecuting wrapped tests with codesigned app and entitlements") - - // set up app bundle - rootDir := t.TempDir() - appRoot := filepath.Join(rootDir, "launcher_test.app") - - // make required dirs launcher_test.app/Contents/MacOS and add files - require.NoError(t, os.MkdirAll(filepath.Join(appRoot, "Contents", "MacOS"), 0700)) - copyFile(t, filepath.Join(macOsAppResourceDir, "Info.plist"), filepath.Join(appRoot, "Contents", "Info.plist")) - copyFile(t, filepath.Join(macOsAppResourceDir, "embedded.provisionprofile"), filepath.Join(appRoot, "Contents", "embedded.provisionprofile")) - - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - - // build an executable containing the tests into the app bundle - executablePath := filepath.Join(appRoot, "Contents", "MacOS", "launcher_test") - out, err := exec.CommandContext( //nolint:forbidigo // Only used in test, don't want as standard allowedcmd - ctx, - "go", - "test", - "-c", - "--cover", - "--race", - "./", - "-o", - executablePath, - ).CombinedOutput() - - require.NoError(t, ctx.Err()) - require.NoError(t, err, string(out)) - - // sign app bundle - signApp(t, appRoot) - - // run app bundle executable - cmd := exec.CommandContext(ctx, executablePath, "-test.v") //nolint:forbidigo // Only used in test, don't want as standard allowedcmd - cmd.Env = append(os.Environ(), fmt.Sprintf("%s=%s", testWrappedEnvVarKey, "true")) - out, err = cmd.CombinedOutput() - require.NoError(t, ctx.Err()) - require.NoError(t, err, string(out)) - - // ensure the test ran successfully - require.Contains(t, string(out), "PASS: TestSecureEnclaveCmd") - require.NotContains(t, string(out), "FAIL") -} - -func TestSecureEnclaveCmd(t *testing.T) { //nolint:paralleltest - if os.Getenv(testWrappedEnvVarKey) == "" { - t.Skipf("\nskipping because %s env var was empty, test not being run from codesigned app with entitlements", testWrappedEnvVarKey) - } - - t.Log("\nrunning wrapped tests with codesigned app and entitlements") - - oldStdout := os.Stdout - defer func() { - os.Stdout = oldStdout - }() - - // create a pipe to capture stdout - pipeReader, pipeWriter, err := os.Pipe() - require.NoError(t, err) - - os.Stdout = pipeWriter - - require.NoError(t, runSecureEnclave(multislogger.New(), []string{secureenclavesigner.CreateKeyCmd})) - require.NoError(t, pipeWriter.Close()) - - var buf bytes.Buffer - _, err = buf.ReadFrom(pipeReader) - require.NoError(t, err) - - // convert response to public key - createKeyResponse := buf.Bytes() - secureEnclavePubKey, err := echelper.PublicB64DerToEcdsaKey(createKeyResponse) - require.NoError(t, err) - require.NotNil(t, secureEnclavePubKey, "should be able to get public key") -} - -// #nosec G306 -- Need readable files -func copyFile(t *testing.T, source, destination string) { - bytes, err := os.ReadFile(source) - require.NoError(t, err) - require.NoError(t, os.WriteFile(destination, bytes, 0700)) -} - -// #nosec G204 -- This triggers due to using env var in cmd, making exception for test -func signApp(t *testing.T, appRootDir string) { - codeSignId := os.Getenv("MACOS_CODESIGN_IDENTITY") - require.NotEmpty(t, codeSignId, "need MACOS_CODESIGN_IDENTITY env var to sign app, such as [Mac Developer: Jane Doe (ABCD123456)]") - - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - - cmd := exec.CommandContext( //nolint:forbidigo // Only used in test, don't want as standard allowedcmd - ctx, - "codesign", - "--deep", - "--force", - "--options", "runtime", - "--entitlements", filepath.Join(macOsAppResourceDir, "entitlements"), - "--sign", codeSignId, - "--timestamp", - appRootDir, - ) - - out, err := cmd.CombinedOutput() - require.NoError(t, ctx.Err()) - require.NoError(t, err, string(out)) -} diff --git a/ee/agent/keys.go b/ee/agent/keys.go index b88914b66..fda5abcfb 100644 --- a/ee/agent/keys.go +++ b/ee/agent/keys.go @@ -3,13 +3,12 @@ package agent import ( "context" "crypto" + "crypto/ecdsa" "fmt" "log/slog" - "time" "github.com/kolide/launcher/ee/agent/keys" "github.com/kolide/launcher/ee/agent/types" - "github.com/kolide/launcher/pkg/backoff" "github.com/kolide/launcher/pkg/traces" ) @@ -29,7 +28,11 @@ func LocalDbKeys() keyInt { return localDbKeys } -func SetupKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, skipHardwareKeys bool) error { +type secureEnclaveClient interface { + CreateSecureEnclaveKey(uid string) (*ecdsa.PublicKey, error) +} + +func SetupKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter) error { ctx, span := traces.StartSpan(ctx) defer span.End() @@ -43,75 +46,5 @@ func SetupKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSett return fmt.Errorf("setting up local db keys: %w", err) } - if skipHardwareKeys { - return nil - } - - err = backoff.WaitFor(func() error { - hwKeys, err := setupHardwareKeys(ctx, slogger, store) - if err != nil { - return err - } - hardwareKeys = hwKeys - return nil - }, 1*time.Second, 250*time.Millisecond) - - if err != nil { - // Use of hardware keys is not fully implemented as of 2023-02-01, so log an error and move on - slogger.Log(context.TODO(), slog.LevelInfo, - "failed setting up hardware keys", - "err", err, - ) - } - return nil } - -// This duplicates some of pkg/osquery/extension.go but that feels like the wrong place. -// Really, we should have a simpler interface over a storage layer. -const ( - privateEccData = "privateEccData" // nolint:unused - publicEccData = "publicEccData" // nolint:unused -) - -// nolint:unused -func fetchKeyData(store types.Getter) ([]byte, []byte, error) { - pri, err := store.Get([]byte(privateEccData)) - if err != nil { - return nil, nil, err - } - - pub, err := store.Get([]byte(publicEccData)) - if err != nil { - return nil, nil, err - } - - return pri, pub, nil -} - -// nolint:unused -func storeKeyData(store types.Setter, pri, pub []byte) error { - if pri != nil { - if err := store.Set([]byte(privateEccData), pri); err != nil { - return err - } - } - - if pub != nil { - if err := store.Set([]byte(publicEccData), pub); err != nil { - return err - } - } - - return nil -} - -// clearKeyData is used to clear the keys as part of error handling around new keys. It is not intended to be called -// regularly, and since the path that calls it is around DB errors, it has no error handling. -// nolint:unused -func clearKeyData(slogger *slog.Logger, deleter types.Deleter) { - slogger.Log(context.TODO(), slog.LevelInfo, - "clearing keys", - ) - _ = deleter.Delete([]byte(privateEccData), []byte(publicEccData)) -} diff --git a/ee/agent/keys_darwin.go b/ee/agent/keys_darwin.go index 891969f21..6cc852d46 100644 --- a/ee/agent/keys_darwin.go +++ b/ee/agent/keys_darwin.go @@ -9,19 +9,20 @@ import ( "log/slog" "github.com/kolide/launcher/ee/agent/types" - "github.com/kolide/launcher/ee/secureenclavesigner" + "github.com/kolide/launcher/ee/secureenclaverunner" "github.com/kolide/launcher/pkg/traces" ) -func setupHardwareKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter) (keyInt, error) { +func SetupHardwareKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, secureEnclaveClient secureEnclaveClient) (execute func() error, interrupt func(error), err error) { ctx, span := traces.StartSpan(ctx) defer span.End() - ses, err := secureenclavesigner.New(ctx, slogger, store) + ser, err := secureenclaverunner.New(ctx, slogger, store, secureEnclaveClient) if err != nil { traces.SetError(span, fmt.Errorf("creating secureenclave signer: %w", err)) - return nil, fmt.Errorf("creating secureenclave signer: %w", err) + return nil, nil, fmt.Errorf("creating secureenclave signer: %w", err) } - return ses, nil + hardwareKeys = ser + return ser.Execute, ser.Interrupt, nil } diff --git a/ee/agent/keys_tpm.go b/ee/agent/keys_tpm.go index 82d057ca2..f5c579783 100644 --- a/ee/agent/keys_tpm.go +++ b/ee/agent/keys_tpm.go @@ -5,52 +5,18 @@ package agent import ( "context" - "fmt" "log/slog" - "github.com/kolide/krypto/pkg/tpm" "github.com/kolide/launcher/ee/agent/types" - "github.com/kolide/launcher/pkg/traces" + "github.com/kolide/launcher/ee/tpmrunner" ) -func setupHardwareKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter) (keyInt, error) { - _, span := traces.StartSpan(ctx) - defer span.End() - - priData, pubData, err := fetchKeyData(store) - if err != nil { - return nil, err - } - - if pubData == nil || priData == nil { - slogger.Log(context.TODO(), slog.LevelInfo, - "generating new keys", - ) - - var err error - priData, pubData, err = tpm.CreateKey() - if err != nil { - clearKeyData(slogger, store) - traces.SetError(span, fmt.Errorf("creating key: %w", err)) - return nil, fmt.Errorf("creating key: %w", err) - } - - span.AddEvent("new_key_created") - - if err := storeKeyData(store, priData, pubData); err != nil { - clearKeyData(slogger, store) - traces.SetError(span, fmt.Errorf("storing key: %w", err)) - return nil, fmt.Errorf("storing key: %w", err) - } - - span.AddEvent("new_key_stored") - } - - k, err := tpm.New(priData, pubData) +func SetupHardwareKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, _ secureEnclaveClient) (execute func() error, interrupt func(error), err error) { + tpmRunner, err := tpmrunner.New(ctx, slogger, store) if err != nil { - traces.SetError(span, fmt.Errorf("creating tpm signer: from new key: %w", err)) - return nil, fmt.Errorf("creating tpm signer: from new key: %w", err) + return nil, nil, err } - return k, nil + hardwareKeys = tpmRunner + return tpmRunner.Execute, tpmRunner.Interrupt, nil } diff --git a/ee/debug/shipper/shipper_test.go b/ee/debug/shipper/shipper_test.go index 158625c60..a2f23ae4f 100644 --- a/ee/debug/shipper/shipper_test.go +++ b/ee/debug/shipper/shipper_test.go @@ -59,7 +59,7 @@ func TestShip(t *testing.T) { //nolint:paralleltest name: "happy path with signing keys and enroll secret", mockKnapsack: func(t *testing.T) *typesMocks.Knapsack { configStore := inmemory.NewStore() - agent.SetupKeys(context.TODO(), multislogger.NewNopLogger(), configStore, true) + agent.SetupKeys(context.TODO(), multislogger.NewNopLogger(), configStore) k := typesMocks.NewKnapsack(t) k.On("EnrollSecret").Return("enroll_secret_value") diff --git a/ee/desktop/runner/runner.go b/ee/desktop/runner/runner.go index bf73a8405..4014b2ab8 100644 --- a/ee/desktop/runner/runner.go +++ b/ee/desktop/runner/runner.go @@ -4,6 +4,7 @@ package runner import ( "bufio" "context" + "crypto/ecdsa" "errors" "fmt" "io" @@ -306,6 +307,20 @@ func (r *DesktopUsersProcessesRunner) DetectPresence(reason string, interval tim return lastDurationSinceLastDetection, fmt.Errorf("no desktop processes detected presence, last error: %w", lastErr) } +func (r *DesktopUsersProcessesRunner) CreateSecureEnclaveKey(uid string) (*ecdsa.PublicKey, error) { + if r.uidProcs == nil || len(r.uidProcs) == 0 { + return nil, errors.New("no desktop processes running") + } + + proc, ok := r.uidProcs[uid] + if !ok { + return nil, fmt.Errorf("no desktop process for uid: %s", uid) + } + + client := client.New(r.userServerAuthToken, proc.socketPath) + return client.CreateSecureEnclaveKey() +} + // killDesktopProcesses kills any existing desktop processes func (r *DesktopUsersProcessesRunner) killDesktopProcesses(ctx context.Context) { wgDone := make(chan struct{}) diff --git a/ee/desktop/user/client/client.go b/ee/desktop/user/client/client.go index da90291dd..967d2352d 100644 --- a/ee/desktop/user/client/client.go +++ b/ee/desktop/user/client/client.go @@ -3,13 +3,16 @@ package client import ( "bytes" "context" + "crypto/ecdsa" "encoding/json" "errors" "fmt" + "io" "net/http" "net/url" "time" + "github.com/kolide/krypto/pkg/echelper" "github.com/kolide/launcher/ee/desktop/user/notify" "github.com/kolide/launcher/ee/desktop/user/server" "github.com/kolide/launcher/ee/presencedetection" @@ -95,6 +98,35 @@ func (c *client) DetectPresence(reason string, interval time.Duration) (time.Dur return durationSinceLastDetection, detectionErr } +func (c *client) CreateSecureEnclaveKey() (*ecdsa.PublicKey, error) { + resp, err := c.base.Get("http://unix/secure_enclave_key") + if err != nil { + return nil, fmt.Errorf("getting secure enclave key: %w", err) + } + + if resp.Body == nil { + return nil, errors.New("response body is nil") + } + + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("unexpected status code: %d", resp.StatusCode) + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("reading response body: %w", err) + } + + key, err := echelper.PublicB64DerToEcdsaKey(body) + if err != nil { + return nil, fmt.Errorf("converting key: %w", err) + } + + return key, nil +} + func (c *client) Notify(n notify.Notification) error { notificationToSend := notify.Notification{ Title: n.Title, diff --git a/ee/desktop/user/server/server.go b/ee/desktop/user/server/server.go index 654558f87..2dcbe71a7 100644 --- a/ee/desktop/user/server/server.go +++ b/ee/desktop/user/server/server.go @@ -67,6 +67,7 @@ func New(slogger *slog.Logger, authedMux.HandleFunc("/refresh", userServer.refreshHandler) authedMux.HandleFunc("/show", userServer.showDesktop) authedMux.HandleFunc("/detect_presence", userServer.detectPresence) + authedMux.HandleFunc("/create_secure_enclave_key", userServer.createSecureEnclaveKey) userServer.server = &http.Server{ Handler: userServer.authMiddleware(authedMux), diff --git a/ee/desktop/user/server/server_darwin.go b/ee/desktop/user/server/server_darwin.go new file mode 100644 index 000000000..34326bb91 --- /dev/null +++ b/ee/desktop/user/server/server_darwin.go @@ -0,0 +1,29 @@ +//go:build darwin +// +build darwin + +package server + +import ( + "fmt" + "net/http" + + "github.com/kolide/krypto/pkg/echelper" + "github.com/kolide/krypto/pkg/secureenclave" +) + +func (s *UserServer) createSecureEnclaveKey(w http.ResponseWriter, _ *http.Request) { + key, err := secureenclave.CreateKey() + if err != nil { + http.Error(w, fmt.Errorf("creating key: %w", err).Error(), http.StatusInternalServerError) + return + } + + keyBytes, err := echelper.PublicEcdsaToB64Der(key) + if err != nil { + http.Error(w, fmt.Errorf("serializing key: %w", err).Error(), http.StatusInternalServerError) + return + } + + w.Write(keyBytes) + w.WriteHeader(http.StatusOK) +} diff --git a/ee/desktop/user/server/server_other.go b/ee/desktop/user/server/server_other.go new file mode 100644 index 000000000..348636a68 --- /dev/null +++ b/ee/desktop/user/server/server_other.go @@ -0,0 +1,10 @@ +//go:build !darwin +// +build !darwin + +package server + +import "net/http" + +func (s *UserServer) createSecureEnclaveKey(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "not implemented on non darwin", http.StatusNotImplemented) +} diff --git a/ee/localserver/krypto-ec-middleware.go b/ee/localserver/krypto-ec-middleware.go index ff943cab2..4be353eb6 100644 --- a/ee/localserver/krypto-ec-middleware.go +++ b/ee/localserver/krypto-ec-middleware.go @@ -19,6 +19,7 @@ import ( "github.com/kolide/krypto" "github.com/kolide/krypto/pkg/challenge" + "github.com/kolide/launcher/ee/agent" "github.com/kolide/launcher/pkg/log/multislogger" "github.com/kolide/launcher/pkg/traces" "go.opentelemetry.io/otel/attribute" @@ -69,17 +70,16 @@ func (cmdReq v2CmdRequestType) CallbackReq() (*http.Request, error) { } type kryptoEcMiddleware struct { - localDbSigner, hardwareSigner crypto.Signer - counterParty ecdsa.PublicKey - slogger *slog.Logger + localDbSigner crypto.Signer + counterParty ecdsa.PublicKey + slogger *slog.Logger } -func newKryptoEcMiddleware(slogger *slog.Logger, localDbSigner, hardwareSigner crypto.Signer, counterParty ecdsa.PublicKey) *kryptoEcMiddleware { +func newKryptoEcMiddleware(slogger *slog.Logger, localDbSigner crypto.Signer, counterParty ecdsa.PublicKey) *kryptoEcMiddleware { return &kryptoEcMiddleware{ - localDbSigner: localDbSigner, - hardwareSigner: hardwareSigner, - counterParty: counterParty, - slogger: slogger.With("keytype", "ec"), + localDbSigner: localDbSigner, + counterParty: counterParty, + slogger: slogger.With("keytype", "ec"), } } @@ -356,8 +356,8 @@ func (e *kryptoEcMiddleware) Wrap(next http.Handler) http.Handler { // krypto library has a nil check for the object but not the funcs, so if are getting nil from the funcs, just // pass nil to krypto // hardware signing is not implemented for darwin - if runtime.GOOS != "darwin" && e.hardwareSigner != nil && e.hardwareSigner.Public() != nil { - response, err = challengeBox.Respond(e.localDbSigner, e.hardwareSigner, responseBytes) + if runtime.GOOS != "darwin" && agent.HardwareKeys() != nil && agent.HardwareKeys().Public() != nil { + response, err = challengeBox.Respond(e.localDbSigner, agent.HardwareKeys(), responseBytes) } else { response, err = challengeBox.Respond(e.localDbSigner, nil, responseBytes) } diff --git a/ee/localserver/krypto-ec-middleware_test.go b/ee/localserver/krypto-ec-middleware_test.go index 51b8977a9..546459c9a 100644 --- a/ee/localserver/krypto-ec-middleware_test.go +++ b/ee/localserver/krypto-ec-middleware_test.go @@ -189,7 +189,7 @@ func TestKryptoEcMiddleware(t *testing.T) { })).Logger // set up middlewares - kryptoEcMiddleware := newKryptoEcMiddleware(slogger, tt.localDbKey, tt.hardwareKey, counterpartyKey.PublicKey) + kryptoEcMiddleware := newKryptoEcMiddleware(slogger, tt.localDbKey, counterpartyKey.PublicKey) require.NoError(t, err) mockPresenceDetector := mocks.NewPresenceDetector(t) @@ -369,7 +369,7 @@ func Test_AllowedOrigin(t *testing.T) { })).Logger // set up middlewares - kryptoEcMiddleware := newKryptoEcMiddleware(slogger, ecdsaKey(t), nil, counterpartyKey.PublicKey) + kryptoEcMiddleware := newKryptoEcMiddleware(slogger, ecdsaKey(t), counterpartyKey.PublicKey) require.NoError(t, err) h := kryptoEcMiddleware.Wrap(testHandler) diff --git a/ee/localserver/server.go b/ee/localserver/server.go index a606e4df2..494a5f2bd 100644 --- a/ee/localserver/server.go +++ b/ee/localserver/server.go @@ -52,9 +52,8 @@ type localServer struct { kolideServer string cancel context.CancelFunc - myKey *rsa.PrivateKey - myLocalDbSigner crypto.Signer - myLocalHardwareSigner crypto.Signer + myKey *rsa.PrivateKey + myLocalDbSigner crypto.Signer serverKey *rsa.PublicKey serverEcKey *ecdsa.PublicKey @@ -76,13 +75,12 @@ func New(ctx context.Context, k types.Knapsack, presenceDetector presenceDetecto defer span.End() ls := &localServer{ - slogger: k.Slogger().With("component", "localserver"), - knapsack: k, - limiter: rate.NewLimiter(defaultRateLimit, defaultRateBurst), - kolideServer: k.KolideServerURL(), - myLocalDbSigner: agent.LocalDbKeys(), - myLocalHardwareSigner: agent.HardwareKeys(), - presenceDetector: presenceDetector, + slogger: k.Slogger().With("component", "localserver"), + knapsack: k, + limiter: rate.NewLimiter(defaultRateLimit, defaultRateBurst), + kolideServer: k.KolideServerURL(), + myLocalDbSigner: agent.LocalDbKeys(), + presenceDetector: presenceDetector, } // TODO: As there may be things that adjust the keys during runtime, we need to persist that across @@ -98,7 +96,7 @@ func New(ctx context.Context, k types.Knapsack, presenceDetector presenceDetecto } ls.myKey = privateKey - ecKryptoMiddleware := newKryptoEcMiddleware(k.Slogger(), ls.myLocalDbSigner, ls.myLocalHardwareSigner, *ls.serverEcKey) + ecKryptoMiddleware := newKryptoEcMiddleware(k.Slogger(), ls.myLocalDbSigner, *ls.serverEcKey) ecAuthedMux := http.NewServeMux() ecAuthedMux.HandleFunc("/", http.NotFound) ecAuthedMux.Handle("/acceleratecontrol", ls.requestAccelerateControlHandler()) diff --git a/ee/secureenclaverunner/mocks/secureEnclaveClient.go b/ee/secureenclaverunner/mocks/secureEnclaveClient.go new file mode 100644 index 000000000..910873d4b --- /dev/null +++ b/ee/secureenclaverunner/mocks/secureEnclaveClient.go @@ -0,0 +1,58 @@ +// Code generated by mockery v2.44.1. DO NOT EDIT. + +package mocks + +import ( + ecdsa "crypto/ecdsa" + + mock "github.com/stretchr/testify/mock" +) + +// SecureEnclaveClient is an autogenerated mock type for the secureEnclaveClient type +type SecureEnclaveClient struct { + mock.Mock +} + +// CreateSecureEnclaveKey provides a mock function with given fields: uid +func (_m *SecureEnclaveClient) CreateSecureEnclaveKey(uid string) (*ecdsa.PublicKey, error) { + ret := _m.Called(uid) + + if len(ret) == 0 { + panic("no return value specified for CreateSecureEnclaveKey") + } + + var r0 *ecdsa.PublicKey + var r1 error + if rf, ok := ret.Get(0).(func(string) (*ecdsa.PublicKey, error)); ok { + return rf(uid) + } + if rf, ok := ret.Get(0).(func(string) *ecdsa.PublicKey); ok { + r0 = rf(uid) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*ecdsa.PublicKey) + } + } + + if rf, ok := ret.Get(1).(func(string) error); ok { + r1 = rf(uid) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// NewSecureEnclaveClient creates a new instance of SecureEnclaveClient. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewSecureEnclaveClient(t interface { + mock.TestingT + Cleanup(func()) +}) *SecureEnclaveClient { + mock := &SecureEnclaveClient{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go new file mode 100644 index 000000000..7217f1c4c --- /dev/null +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -0,0 +1,281 @@ +//go:build darwin +// +build darwin + +package secureenclaverunner + +import ( + "context" + "crypto" + "crypto/ecdsa" + "crypto/x509" + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "io" + "log/slog" + "os/user" + "sync" + "time" + + "github.com/kolide/launcher/ee/agent/types" + "github.com/kolide/launcher/ee/consoleuser" + "github.com/kolide/launcher/pkg/traces" +) + +const ( + publicEccDataKey = "publicEccData" +) + +type secureEnclaveRunner struct { + uidPubKeyMap map[string]*ecdsa.PublicKey + secureEnclaveClient secureEnclaveClient + store types.GetterSetterDeleter + slogger *slog.Logger + mux *sync.Mutex + interrupt chan struct{} + interrupted bool +} + +type secureEnclaveClient interface { + CreateSecureEnclaveKey(uid string) (*ecdsa.PublicKey, error) +} + +func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, secureEnclaveClient secureEnclaveClient) (*secureEnclaveRunner, error) { + ctx, span := traces.StartSpan(ctx) + defer span.End() + + ser := &secureEnclaveRunner{ + uidPubKeyMap: make(map[string]*ecdsa.PublicKey), + store: store, + secureEnclaveClient: secureEnclaveClient, + slogger: slogger.With("component", "secureenclaverunner"), + mux: &sync.Mutex{}, + interrupt: make(chan struct{}), + } + + data, err := store.Get([]byte(publicEccDataKey)) + if err != nil { + traces.SetError(span, fmt.Errorf("getting public ecc data from store: %w", err)) + return nil, fmt.Errorf("getting public ecc data from store: %w", err) + } + + if data != nil { + if err := json.Unmarshal(data, ser); err != nil { + traces.SetError(span, fmt.Errorf("unmarshaling secure enclave signer: %w", err)) + ser.slogger.Log(ctx, slog.LevelError, + "unable to unmarshal secure enclave signer, data may be corrupt, wiping", + "err", err, + ) + + if err := store.Delete([]byte(publicEccDataKey)); err != nil { + traces.SetError(span, fmt.Errorf("deleting corrupt public ecc data: %w", err)) + ser.slogger.Log(ctx, slog.LevelError, + "unable to unmarshal secure enclave signer, data may be corrupt, wiping", + "err", err, + ) + } + } + } + + return ser, nil +} + +// Public returns the public key of the current console user +// creating and peristing a new one if needed +func (ser *secureEnclaveRunner) Execute() error { + currentRetryInterval, maxRetryInterval := 1*time.Second, 1*time.Minute + retryTicker := time.NewTicker(currentRetryInterval) + defer retryTicker.Stop() + + for { + if _, err := ser.currentConsoleUserKey(context.TODO()); err != nil { + ser.slogger.Log(context.TODO(), slog.LevelError, + "getting current console user key, will retry", + "err", err, + ) + + if currentRetryInterval < maxRetryInterval { + currentRetryInterval += time.Second + } + } else { + retryTicker.Stop() + } + + select { + case <-retryTicker.C: + continue + case <-ser.interrupt: + ser.slogger.Log(context.TODO(), slog.LevelDebug, + "interrupt received, exiting secure enclave signer execute loop", + ) + return nil + } + } +} + +func (ser *secureEnclaveRunner) Interrupt(_ error) { + // Only perform shutdown tasks on first call to interrupt -- no need to repeat on potential extra calls. + if ser.interrupted { + return + } + + ser.interrupted = true + + // Tell the execute loop to stop checking, and exit + ser.interrupt <- struct{}{} +} + +// Public returns the public key of the current console user +// creating and peristing a new one if needed +func (ser *secureEnclaveRunner) Public() crypto.PublicKey { + k, err := ser.currentConsoleUserKey(context.TODO()) + if err != nil { + ser.slogger.Log(context.TODO(), slog.LevelError, + "getting public key", + "err", err, + ) + return nil + } + + // currentConsoleUserKey may return no error and a nil pointer where the inability + // to get the key is expected (see logic around calling firstConsoleUser). In this case, + // k will be a "typed" nil, as an uninitialized pointer to a ecdsa.PublicKey. We're returning + // this typed nil assigned as the crypto.PublicKey interface. This means that the interface's value + // will be nil, but it's underlying type will not be - so it will pass nil checks but panic + // when typecasted later. Explicitly return an untyped nil in this case to prevent confusion and panics later + if k == nil { + return nil + } + + return k +} + +func (ser *secureEnclaveRunner) Type() string { + return "secure_enclave" +} + +func (ser *secureEnclaveRunner) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) { + return nil, fmt.Errorf("not implemented") +} + +func (ser *secureEnclaveRunner) currentConsoleUserKey(ctx context.Context) (*ecdsa.PublicKey, error) { + ctx, span := traces.StartSpan(ctx) + defer span.End() + + ser.mux.Lock() + defer ser.mux.Unlock() + + cu, err := firstConsoleUser(ctx) + if err != nil { + ser.slogger.Log(ctx, slog.LevelDebug, + "getting first console user, expected when root launcher running without a logged in console user", + "err", err, + ) + + traces.SetError(span, fmt.Errorf("getting first console user: %w", err)) + return nil, nil + } + + key, ok := ser.uidPubKeyMap[cu.Uid] + if ok { + span.AddEvent("found_existing_key_for_console_user") + return key, nil + } + + key, err = ser.secureEnclaveClient.CreateSecureEnclaveKey(cu.Uid) + if err != nil { + traces.SetError(span, fmt.Errorf("creating key: %w", err)) + return nil, fmt.Errorf("creating key: %w", err) + } + + span.AddEvent("created_new_key_for_console_user") + + ser.uidPubKeyMap[cu.Uid] = key + if err := ser.save(); err != nil { + delete(ser.uidPubKeyMap, cu.Uid) + traces.SetError(span, fmt.Errorf("saving secure enclave signer: %w", err)) + return nil, fmt.Errorf("saving secure enclave signer: %w", err) + } + + span.AddEvent("saved_key_for_console_user") + return key, nil +} + +func (ser *secureEnclaveRunner) MarshalJSON() ([]byte, error) { + keyMap := make(map[string]string) + + for uid, pubKey := range ser.uidPubKeyMap { + pubKeyBytes, err := x509.MarshalPKIXPublicKey(pubKey) + if err != nil { + return nil, fmt.Errorf("marshalling to PXIX public key: %w", err) + } + + keyMap[uid] = base64.StdEncoding.EncodeToString(pubKeyBytes) + } + + return json.Marshal(keyMap) +} + +func (ser *secureEnclaveRunner) UnmarshalJSON(data []byte) error { + if ser.uidPubKeyMap == nil { + ser.uidPubKeyMap = make(map[string]*ecdsa.PublicKey) + } + + var keyMap map[string]string + if err := json.Unmarshal(data, &keyMap); err != nil { + return fmt.Errorf("unmarshalling key data: %w", err) + } + + for k, v := range keyMap { + decoded, err := base64.StdEncoding.DecodeString(v) + if err != nil { + return fmt.Errorf("decoding base64: %w", err) + } + + pubKey, err := x509.ParsePKIXPublicKey(decoded) + if err != nil { + return fmt.Errorf("parsing PXIX public key: %w", err) + } + + ecdsaPubKey, ok := pubKey.(*ecdsa.PublicKey) + if !ok { + return fmt.Errorf("public key is not ecdsa") + } + + ser.uidPubKeyMap[k] = ecdsaPubKey + } + + return nil +} + +func firstConsoleUser(ctx context.Context) (*user.User, error) { + ctx, span := traces.StartSpan(ctx) + defer span.End() + + c, err := consoleuser.CurrentUsers(ctx) + if err != nil { + traces.SetError(span, fmt.Errorf("getting current users: %w", err)) + return nil, fmt.Errorf("getting current users: %w", err) + } + + if len(c) == 0 { + traces.SetError(span, errors.New("no console users found")) + return nil, errors.New("no console users found") + } + + return c[0], nil +} + +func (ser *secureEnclaveRunner) save() error { + json, err := json.Marshal(ser) + if err != nil { + return fmt.Errorf("marshaling secure enclave signer: %w", err) + } + + if err := ser.store.Set([]byte(publicEccDataKey), json); err != nil { + return fmt.Errorf("setting public ecc data: %w", err) + } + + return nil +} diff --git a/ee/secureenclaverunner/secureenclaverunner_test.go b/ee/secureenclaverunner/secureenclaverunner_test.go new file mode 100644 index 000000000..c30352dcd --- /dev/null +++ b/ee/secureenclaverunner/secureenclaverunner_test.go @@ -0,0 +1,93 @@ +//go:build darwin +// +build darwin + +package secureenclaverunner + +import ( + "context" + "crypto/ecdsa" + "encoding/json" + "errors" + "testing" + "time" + + "github.com/kolide/krypto/pkg/echelper" + "github.com/kolide/launcher/ee/agent/storage/inmemory" + "github.com/kolide/launcher/ee/secureenclaverunner/mocks" + "github.com/kolide/launcher/pkg/log/multislogger" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func Test_secureEnclaveRunner(t *testing.T) { + t.Parallel() + + privKey, err := echelper.GenerateEcdsaKey() + require.NoError(t, err) + + t.Run("creates key in new", func(t *testing.T) { + t.Parallel() + + secureEnclaveClientMock := mocks.NewSecureEnclaveClient(t) + + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(&privKey.PublicKey, nil).Once() + ser, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), secureEnclaveClientMock) + require.NoError(t, err) + require.NotNil(t, ser.Public()) + }) + + t.Run("creates key in execute", func(t *testing.T) { + t.Parallel() + + secureEnclaveClientMock := mocks.NewSecureEnclaveClient(t) + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(nil, errors.New("not available yet")).Once() + ser, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), secureEnclaveClientMock) + require.NoError(t, err) + + // iniital key should be nil since client wasn't ready + require.Nil(t, ser.Public()) + + // give error on first execute loop + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(nil, errors.New("not available yet")).Once() + + // give key on second execute loop + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(&privKey.PublicKey, nil).Once() + + go func() { + ser.Execute() + }() + + // sleep long enough to get through 2 cycles of exectue + time.Sleep(3 * time.Second) + + // one cycle of execute should have created key + require.NotNil(t, ser.Public()) + ser.Interrupt(errors.New("test")) + }) + + t.Run("loads existing key", func(t *testing.T) { + t.Parallel() + + // populate store with key + store := inmemory.NewStore() + firstConsoleUser, err := firstConsoleUser(context.TODO()) + require.NoError(t, err) + serToSerialize := &secureEnclaveRunner{ + uidPubKeyMap: map[string]*ecdsa.PublicKey{ + firstConsoleUser.Uid: &privKey.PublicKey, + }, + } + serJson, err := json.Marshal(serToSerialize) + require.NoError(t, err) + err = store.Set([]byte(publicEccDataKey), serJson) + require.NoError(t, err) + + // create new signer with store containting key + ser, err := New(context.TODO(), multislogger.NewNopLogger(), store, nil) + require.NoError(t, err) + + // should be able to fetch the key + require.NotNil(t, ser.Public()) + }) + +} diff --git a/ee/secureenclavesigner/secureenclavesigner_darwin.go b/ee/secureenclavesigner/secureenclavesigner_darwin.go deleted file mode 100644 index 130eef9b7..000000000 --- a/ee/secureenclavesigner/secureenclavesigner_darwin.go +++ /dev/null @@ -1,313 +0,0 @@ -//go:build darwin -// +build darwin - -package secureenclavesigner - -import ( - "context" - "crypto" - "crypto/ecdsa" - "crypto/x509" - "encoding/base64" - "encoding/json" - "errors" - "fmt" - "io" - "log/slog" - "os" - "os/user" - "strings" - "sync" - - "github.com/kolide/krypto/pkg/echelper" - "github.com/kolide/launcher/ee/agent/types" - "github.com/kolide/launcher/ee/allowedcmd" - "github.com/kolide/launcher/ee/consoleuser" - "github.com/kolide/launcher/pkg/traces" -) - -const ( - CreateKeyCmd = "create-key" - PublicEccDataKey = "publicEccData" -) - -type opt func(*secureEnclaveSigner) - -type secureEnclaveSigner struct { - uidPubKeyMap map[string]*ecdsa.PublicKey - pathToLauncherBinary string - store types.GetterSetterDeleter - slogger *slog.Logger - mux *sync.Mutex -} - -func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, opts ...opt) (*secureEnclaveSigner, error) { - ctx, span := traces.StartSpan(ctx) - defer span.End() - - ses := &secureEnclaveSigner{ - uidPubKeyMap: make(map[string]*ecdsa.PublicKey), - store: store, - slogger: slogger.With("component", "secureenclavesigner"), - mux: &sync.Mutex{}, - } - - data, err := store.Get([]byte(PublicEccDataKey)) - if err != nil { - traces.SetError(span, fmt.Errorf("getting public ecc data from store: %w", err)) - return nil, fmt.Errorf("getting public ecc data from store: %w", err) - } - - if data != nil { - if err := json.Unmarshal(data, ses); err != nil { - traces.SetError(span, fmt.Errorf("unmarshaling secure enclave signer: %w", err)) - ses.slogger.Log(ctx, slog.LevelError, - "unable to unmarshal secure enclave signer, data may be corrupt, wiping", - "err", err, - ) - - if err := store.Delete([]byte(PublicEccDataKey)); err != nil { - traces.SetError(span, fmt.Errorf("deleting corrupt public ecc data: %w", err)) - return nil, fmt.Errorf("deleting corrupt public ecc data: %w", err) - } - } - } - - for _, opt := range opts { - opt(ses) - } - - if ses.pathToLauncherBinary == "" { - p, err := os.Executable() - if err != nil { - traces.SetError(span, fmt.Errorf("getting path to launcher binary: %w", err)) - return nil, fmt.Errorf("getting path to launcher binary: %w", err) - } - - ses.pathToLauncherBinary = p - } - - // get current console user key to make sure it's available - if _, err := ses.currentConsoleUserKey(ctx); err != nil { - traces.SetError(span, fmt.Errorf("getting current console user key: %w", err)) - ses.slogger.Log(ctx, slog.LevelError, - "getting current console user key", - "err", err, - ) - - // intentionally not returning error here, because this runs on start up - // and maybe the console user or secure enclave is not available yet - } - - return ses, nil -} - -// Public returns the public key of the current console user -// creating and peristing a new one if needed -func (ses *secureEnclaveSigner) Public() crypto.PublicKey { - k, err := ses.currentConsoleUserKey(context.TODO()) - if err != nil { - ses.slogger.Log(context.TODO(), slog.LevelError, - "getting public key", - "err", err, - ) - return nil - } - - // currentConsoleUserKey may return no error and a nil pointer where the inability - // to get the key is expected (see logic around calling firstConsoleUser). In this case, - // k will be a "typed" nil, as an uninitialized pointer to a ecdsa.PublicKey. We're returning - // this typed nil assigned as the crypto.PublicKey interface. This means that the interface's value - // will be nil, but it's underlying type will not be - so it will pass nil checks but panic - // when typecasted later. Explicitly return an untyped nil in this case to prevent confusion and panics later - if k == nil { - return nil - } - - return k -} - -func (ses *secureEnclaveSigner) Type() string { - return "secure_enclave" -} - -func (ses *secureEnclaveSigner) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) { - return nil, fmt.Errorf("not implemented") -} - -func (ses *secureEnclaveSigner) currentConsoleUserKey(ctx context.Context) (*ecdsa.PublicKey, error) { - ctx, span := traces.StartSpan(ctx) - defer span.End() - - ses.mux.Lock() - defer ses.mux.Unlock() - - cu, err := firstConsoleUser(ctx) - if err != nil { - ses.slogger.Log(ctx, slog.LevelDebug, - "getting first console user, expected when root launcher running without a logged in console user", - "err", err, - ) - - traces.SetError(span, fmt.Errorf("getting first console user: %w", err)) - return nil, nil - } - - key, ok := ses.uidPubKeyMap[cu.Uid] - if ok { - span.AddEvent("found_existing_key_for_console_user") - return key, nil - } - - key, err = ses.createKey(ctx, cu) - if err != nil { - traces.SetError(span, fmt.Errorf("creating key: %w", err)) - return nil, fmt.Errorf("creating key: %w", err) - } - - span.AddEvent("created_new_key_for_console_user") - - ses.uidPubKeyMap[cu.Uid] = key - if err := ses.save(); err != nil { - delete(ses.uidPubKeyMap, cu.Uid) - traces.SetError(span, fmt.Errorf("saving secure enclave signer: %w", err)) - return nil, fmt.Errorf("saving secure enclave signer: %w", err) - } - - span.AddEvent("saved_key_for_console_user") - return key, nil -} - -func (ses *secureEnclaveSigner) MarshalJSON() ([]byte, error) { - keyMap := make(map[string]string) - - for uid, pubKey := range ses.uidPubKeyMap { - pubKeyBytes, err := x509.MarshalPKIXPublicKey(pubKey) - if err != nil { - return nil, fmt.Errorf("marshalling to PXIX public key: %w", err) - } - - keyMap[uid] = base64.StdEncoding.EncodeToString(pubKeyBytes) - } - - return json.Marshal(keyMap) -} - -func (ses *secureEnclaveSigner) UnmarshalJSON(data []byte) error { - if ses.uidPubKeyMap == nil { - ses.uidPubKeyMap = make(map[string]*ecdsa.PublicKey) - } - - var keyMap map[string]string - if err := json.Unmarshal(data, &keyMap); err != nil { - return fmt.Errorf("unmarshalling key data: %w", err) - } - - for k, v := range keyMap { - decoded, err := base64.StdEncoding.DecodeString(v) - if err != nil { - return fmt.Errorf("decoding base64: %w", err) - } - - pubKey, err := x509.ParsePKIXPublicKey(decoded) - if err != nil { - return fmt.Errorf("parsing PXIX public key: %w", err) - } - - ecdsaPubKey, ok := pubKey.(*ecdsa.PublicKey) - if !ok { - return fmt.Errorf("public key is not ecdsa") - } - - ses.uidPubKeyMap[k] = ecdsaPubKey - } - - return nil -} - -func (ses *secureEnclaveSigner) createKey(ctx context.Context, u *user.User) (*ecdsa.PublicKey, error) { - ctx, span := traces.StartSpan(ctx) - defer span.End() - - cmd, err := allowedcmd.Launchctl( - ctx, - "asuser", - u.Uid, - "sudo", - "--preserve-env", - "-u", - u.Username, - ses.pathToLauncherBinary, - "secure-enclave", - CreateKeyCmd, - ) - - if err != nil { - traces.SetError(span, fmt.Errorf("creating command to create key: %w", err)) - return nil, fmt.Errorf("creating command to create key: %w", err) - } - - // skip updates since we have full path of binary - cmd.Env = append(cmd.Environ(), fmt.Sprintf("%s=%s", "LAUNCHER_SKIP_UPDATES", "true")) - out, err := cmd.CombinedOutput() - if err != nil { - traces.SetError(span, fmt.Errorf("executing launcher binary to create key: %w: %s", err, string(out))) - return nil, fmt.Errorf("executing launcher binary to create key: %w: %s", err, string(out)) - } - - pubKey, err := echelper.PublicB64DerToEcdsaKey([]byte(lastLine(out))) - if err != nil { - traces.SetError(span, fmt.Errorf("converting public key to ecdsa: %w", err)) - return nil, fmt.Errorf("converting public key to ecdsa: %w", err) - } - - return pubKey, nil -} - -// lastLine returns the last line of the out. -// This is needed because laucher sets up a logger by default. -// The last line of the output is the public key or signature. -func lastLine(out []byte) string { - outStr := string(out) - - // get last line of outstr - lastLine := "" - for _, line := range strings.Split(outStr, "\n") { - if line != "" { - lastLine = line - } - } - - return lastLine -} - -func firstConsoleUser(ctx context.Context) (*user.User, error) { - ctx, span := traces.StartSpan(ctx) - defer span.End() - - c, err := consoleuser.CurrentUsers(ctx) - if err != nil { - traces.SetError(span, fmt.Errorf("getting current users: %w", err)) - return nil, fmt.Errorf("getting current users: %w", err) - } - - if len(c) == 0 { - traces.SetError(span, errors.New("no console users found")) - return nil, errors.New("no console users found") - } - - return c[0], nil -} - -func (ses *secureEnclaveSigner) save() error { - json, err := json.Marshal(ses) - if err != nil { - return fmt.Errorf("marshaling secure enclave signer: %w", err) - } - - if err := ses.store.Set([]byte(PublicEccDataKey), json); err != nil { - return fmt.Errorf("setting public ecc data: %w", err) - } - - return nil -} diff --git a/ee/secureenclavesigner/secureenclavesigner_test.go b/ee/secureenclavesigner/secureenclavesigner_test.go deleted file mode 100644 index cd0f91206..000000000 --- a/ee/secureenclavesigner/secureenclavesigner_test.go +++ /dev/null @@ -1,171 +0,0 @@ -//go:build darwin -// +build darwin - -package secureenclavesigner - -import ( - "context" - "crypto/ecdsa" - "fmt" - "os" - "os/exec" - "path/filepath" - "testing" - "time" - - "github.com/kolide/krypto/pkg/echelper" - "github.com/kolide/launcher/ee/agent/storage/inmemory" - "github.com/kolide/launcher/pkg/log/multislogger" - "github.com/stretchr/testify/require" -) - -const ( - testWrappedEnvVarKey = "SECURE_ENCLAVE_TEST_WRAPPED" - testSkipSecureEnclaveTestingEnvVarKey = "SKIP_SECURE_ENCLAVE_TESTS" - macOsAppResourceDir = "./test_app_resources" -) - -func WithBinaryPath(p string) opt { - return func(ses *secureEnclaveSigner) { - ses.pathToLauncherBinary = p - } -} - -// #nosec G306 -- Need readable files -func TestSecureEnclaveSigner(t *testing.T) { - t.Parallel() - - if os.Getenv("CI") != "" { - t.Skipf("\nskipping because %s env var was not empty, this is being run in a CI environment without access to secure enclave", testWrappedEnvVarKey) - } - - if os.Getenv(testSkipSecureEnclaveTestingEnvVarKey) != "" { - t.Skipf("\nskipping because %s env var was set", testSkipSecureEnclaveTestingEnvVarKey) - } - - // put the root dir somewhere else if you want to persist the signed macos app bundle - // should build this into make at some point - rootDir := "/tmp/secure_enclave_test" - - // rootDir := t.TempDir() - appRoot := filepath.Join(rootDir, "launcher_test.app") - - // make required dirs krypto_test.app/Contents/MacOS and add files - require.NoError(t, os.MkdirAll(filepath.Join(appRoot, "Contents", "MacOS"), 0777)) - copyFile(t, filepath.Join(macOsAppResourceDir, "Info.plist"), filepath.Join(appRoot, "Contents", "Info.plist")) - copyFile(t, filepath.Join(macOsAppResourceDir, "embedded.provisionprofile"), filepath.Join(appRoot, "Contents", "embedded.provisionprofile")) - - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - - serverPrivKey, err := echelper.GenerateEcdsaKey() - require.NoError(t, err) - - serverPubKeyDer, err := echelper.PublicEcdsaToB64Der(serverPrivKey.Public().(*ecdsa.PublicKey)) - require.NoError(t, err) - - // build the executable - executablePath := filepath.Join(appRoot, "Contents", "MacOS", "launcher_test") - out, err := exec.CommandContext( //nolint:forbidigo // Only used in test, don't want as standard allowedcmd - ctx, - "go", - "build", - "-ldflags", - fmt.Sprintf("-X github.com/kolide/launcher/ee/secureenclavesigner.TestServerPubKey=%s", string(serverPubKeyDer)), - "-tags", - "secure_enclave_test", - "-o", - executablePath, - "../../cmd/launcher", - ).CombinedOutput() - - require.NoError(t, ctx.Err()) - require.NoError(t, err, string(out)) - - // sign app bundle - signApp(t, appRoot) - - store := inmemory.NewStore() - - // create brand new signer without existing key - // ask for public first to trigger key generation - ses, err := New(context.TODO(), multislogger.NewNopLogger(), store, WithBinaryPath(executablePath)) - require.NoError(t, err, - "should be able to create secure enclave signer", - ) - - pubKey := ses.Public() - require.NotNil(t, pubKey, - "should be able to create brand new public key", - ) - - pubEcdsaKey := pubKey.(*ecdsa.PublicKey) - require.NotNil(t, pubEcdsaKey, - "public key should convert to ecdsa key", - ) - - pubKeySame := ses.Public() - require.NotNil(t, pubKeySame, - "should be able to get public key again", - ) - - pubEcdsaKeySame := pubKeySame.(*ecdsa.PublicKey) - require.NotNil(t, pubEcdsaKeySame, - "public key should convert to ecdsa key", - ) - - require.Equal(t, pubEcdsaKey, pubEcdsaKeySame, - "asking for the same public key should return the same key", - ) - - existingDataSes, err := New(context.TODO(), multislogger.NewNopLogger(), store, WithBinaryPath(executablePath)) - require.NoError(t, err, - "should be able to create secure enclave signer with existing key", - ) - - pubKeyUnmarshalled := existingDataSes.Public() - require.NotNil(t, pubKeyUnmarshalled, - "should be able to get public key from unmarshalled secure enclave signer", - ) - - pubEcdsaKeyUnmarshalled := pubKeyUnmarshalled.(*ecdsa.PublicKey) - require.NotNil(t, pubEcdsaKeyUnmarshalled, - "public key should convert to ecdsa key", - ) - - require.Equal(t, pubEcdsaKey, pubEcdsaKeyUnmarshalled, - "unmarshalled public key should be the same as original public key", - ) -} - -// #nosec G306 -- Need readable files -func copyFile(t *testing.T, source, destination string) { - bytes, err := os.ReadFile(source) - require.NoError(t, err) - require.NoError(t, os.WriteFile(destination, bytes, 0700)) -} - -// #nosec G204 -- This triggers due to using env var in cmd, making exception for test -func signApp(t *testing.T, appRootDir string) { - codeSignId := os.Getenv("MACOS_CODESIGN_IDENTITY") - require.NotEmpty(t, codeSignId, "need MACOS_CODESIGN_IDENTITY env var to sign app, such as [Mac Developer: Jane Doe (ABCD123456)]") - - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - - cmd := exec.CommandContext( //nolint:forbidigo // Only used in test, don't want as standard allowcmd - ctx, - "codesign", - "--deep", - "--force", - "--options", "runtime", - "--entitlements", filepath.Join(macOsAppResourceDir, "entitlements"), - "--sign", codeSignId, - "--timestamp", - appRootDir, - ) - - out, err := cmd.CombinedOutput() - require.NoError(t, ctx.Err()) - require.NoError(t, err, string(out)) -} diff --git a/ee/secureenclavesigner/test_app_resources/embedded.provisionprofile b/ee/secureenclavesigner/test_app_resources/embedded.provisionprofile deleted file mode 100644 index 37a067b99b1a426de90e8ebb0765042fae694f77..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 16825 zcmeHv3A7_+wJzuMKzBpajLjg>hXx!-cc-c{OE*$;DoIsRl~g8Vs8l7DREA1YNh*l~ zO{{RZ%-Z1U&m6OC z{b5v`Oyv^vM6*`0Iui+*h;ih^>a}h+-fY&2Za0;pCK53&nVNt$+-_P}J+Zpn=`=s> zcH6eSPH}3jQ_Z?l(Q+r+X0zDt3}VpOT6kcc*3ni&H*#lbPq z9Y+aDy|~`(TtcbUX~STTNtKHeQL|RjiW780YnqitX9CxY@Rus~*1M-_&9+jjs7ePW z&1q1lQmPc&iFUD6>Cd#QwA;$u4zi)3%t(PS?wQ+~d#0K*9I{;;HA*R+*#T;BaSVoF z-nEEtErz8~|EE#x(?~GC-aWcCHKs&O>6FZN{X+wonPNPd{phYaD+}9J%2{!*d+dNs%NG!F1f_2wdmHWYf-? zjIUz2y2h}Hc7_O9DHymw=+#!aT46&rLg1ps;1q%KvS^EXj!KInMNu6>4HH&ANn{J2 zh?Ng(%{(jX5}U^InSM`Y(*ur))Cw$vD49@K9S~GL8-efnW)7Py<7fue*m6&;*N|e0 zj`4Vv#Zj3iZJU&ae4ifT5}^xe0ylZG%GfH~uW@v(n;Urhv_D-8hDP-%}BMPM5=Di%HuRa;igYO7%E+>dIU;h z`IKqL@#*<8LN3G4n_s*_B=ea_Er$tZjz-H0%XEuLl!Ns^3Yb@qG8&PRefgA;=J+bt zB+T?Qg|m;!1~U5TBG?6XV-=(^JF^B+qzCy zcpPELWQ$EgkCaHmKFK)lPTggq8Xf25jqlg&XO64l%Vv%^G)VSpROJ%KoFx1Fq(y?}0X5}muZN`hVUqskS zF^Uvz1`oPZNq0Yq6mkUTt!2z^xzz9r^&C^}xode3)lX+*v7QoF2}2cPte45unKX&@ z{DY{=@YzT?PO;HQ-xE=CB_vT!x-^ksLft%?(Sp>_;}_G_N;%rqkY1jtv~vTDYLq~B zcml^+L)SS2|S>tRm#}f+9;0>2eWJH$|s*+7E*-hrm zoIS_}qQwCp##xfISlB~`uxWx|MLTEU9YWXJgibSrsM@%hD=T3MQ7N+*!-SgJkcP4P zq+G=6La&evc{EHKAL1CyjR-sEQbKeBzB8adaEW%#*Q}1fgeoCISt)4iDnHw&&5=CEVid;? z<$BsuGE6sz$+{yOAooqs3C3KV0J@}7OcLcnqpGVcGtk0SJ(m@RS_Z8_-7?5oO$|%s zLS3kFY^ctKB@>=O6gJ)E*wH#Tx}{P8J*0Y^dQw@fi_lkkP{{InF5|0mEUZ&qYHDn) zS7`7smYl9-g<6dK&{%9o26G-5!{l?$dN=d+nvsJe>dZk6N6I;m1bt5S2mN&N86Tn! zytZKHxTu{Y2vH8>I-QAPWkd@TLuc(ZkLKB7-{!AFW{zG=25n|?{cE@wb`v-#M}1D) zHgAU=ogyJ|g2>TKbZCTfmV}p3zg!4sq9TiQxO}UcOSgF3J35aPoG-U6(trVBox)R~ z2PbvCvyVE>Pxixs&ff_R;BgX%M+@b8HWnDD)U#r9CNpi;@sz-k7gSzl$S6qJ@C4EjhF z4XT7*7G$ZWF;ZF>ZOo^MR6e%@gj+ekj!#vSE)=SiUvoP+q7H9qc4h{%OWi@Y*i5-JBIs$bg>tu5OJT#E?A3oC%pULtjS49l;nOpiJzK^AX6N*e z!fbC!?z1U|5tB$H#iux%pT=ymwlmC*<|g%tsAPn8J}V(`a`-rnCV7J&WNHi*P0?Ua z(9^a^G(%OrX*r)1kTL4Syq5ZqS2(AQ6#9%x_qO$N)1uMxhWi^S&@k z71^xKmb1}B#E1vY1ZF3UKq&8{1}+a3C;D8gmkWBLXsgIt1{v+@T)^M+;uP928<9e& zj%UkWmu_nAteEB8R6*bi;aJ6&OVUOWWyH zB3PbKFRTp(#m!~pX1^KfmID;qO4zs>$z@{6VWBIM9>l83gFuJz>(PKWDEIVgBJI-5 zqCXz=4^62X^%SX6G?PI^3Q0Fxkw%cqx9!FtVRIBN;)LntNNxz&RSw#eb3P;_HKpfl z&~v2`b|-n7rSObSh!MAir>lv2FORp9bh}!QvOHgN4H}j~*mE!$>?^6tvz#3hXLT}< zM_Df8=zH8B8-zx>T^-EFTEKv+T2}$TrJ8Es{W)#gcrI6)#rcq-dhl*8OW1&GfS;jG z3>%f%`~aQCds{FZT%OeL9G-;lvV+l*bG|4s7m1pAuH4g%JT4PF7k0xhHK$J!H%OwR z4+sl&Z1&nT#&#WyRUGWZ(gVQEI><~}1ANT^MguPTxG+%$T=W6{4pk4}-w0;~m@|RE zeFN~hm#;V&EjyTBD^#F9_zg}wxjNYMVZo_a&1EB1=&PpI(IK?e%>y16Fke*}VXY6! z9K25t^GQ$zJB&qT9T^myF^ry@Yg0JB2aIu$%Lw3mfZgEqmz8RTOuDOu`7TeXbL|y^ zsdPp*k~#z%3vgQ7Vc+KWoi1xx$pm}93ARd)CvuKn&>~=q1HSS+Zq8|&r+5Mx-KTUh z1BO9AoS@yq0AabiE+vTRwOENM2o0u*8F+kj9>w6umLzbVB)qsy+qvlMKIXt*8TCUF zB5u#_u_^#Q$DoXY{U`94KvyNQ-Utu_vKJI;UV~1w2wkD1LJ9Q=i8el{WOU7+ECdo2 zHr{6{s^#@S~@E>p5jL2d4pycj{X=S*fZb5|yWusEqMo z*MiQREVrh{67n$>q1i?GfZO`O9>chB{^Eh z!X*?({he~HPYt4d#LlMrove)dJcUTBj4>fh?0EVem5ioRbSa6|s}VuUDmBn>eKN1o zpzqzjn7dlWyzw?lRM=i2>oU@*x~JssM7iJ)v`bRZe2tF8adMDQeYH^9h+u_W4;OMd zyPHiJoZSe<8JCOCwBvzBy&1|h^KnKGR?&f{)A6ggKV`WMOK<8;wv4#zlBeWvMDuRb zs#69{_3On-~VG^!%U zy9zPCry~9jM$hW+|H0`0IvAab6(WW}WJItfuW^p)XOgJ3FMsB9icxFhJdI$}anbVM4A(8@eZ+C{(lQwg(JK7B|zSa=Kn%vP~jkcRY-m4(fK8jw)I= z+m4_GxnK0D9i3AoIhC8m==Ia2{uDD$(b8Jl35pc8wGg=|RciCd{Si|x1ZS*f$D&e6 zlj8(0AUvgc9dE5qjXds}UasmyUgT+Dd4MqiE8K6-Wo5&-0%bmg*7bOv&v8^Oh4U1y zWxz@uI@T#XM*wHYlQm$KGHJPh^qX2;CUKqVfEUV>3}J!AS_dn$!{aiLzM)hGSQ`rW zMO3s5R!#3fG)_zbOJ&<}DOj?WopW@$n-3E` z1=u-=Iy4-Xj-#cTX5ej#n2cmhdN$6uYE|dH6RChb64iRTu4Hxa8tomJ0NjFgPN(G> zO_3rhP(nHHSVtsU7bqExrKEB`!*z0o+UF~XCs)r6gkj#`S*aXnWjk-faAA&Gy*4@r z`HV#8!8QdCW5_c!&!%7zDPUkJz8@QX7YDFD10Inv&^|UV^K&TMjFe(N?Q6mo$nf07U9`p=*+i_SMrwuq`HC4Au^H`q2h!O89)9g-t z^2|8i`smD%d51BdX&cV|OwCT zI^&FG7{fV(>Hux1^$@Ctd7IL0e*XG+AqK>82}j>mooG}zVuA|8MUEs%598*HWMA&4 zCp}<@Q1RXns5pve)jm7K^OKP~o~NgydQ7BO@QAtyYBH82!oafCc~W$|WhWY_IkN4r z`t#2v$ZCZk=6K-$4-rEeB8K7+ z2$m$?;b`!Rk~upv$v(z_e4UPv{=LZOEZeBt(do96z@Eg64@dss{E39F0%LE)6Cy_t zy4==95$ANFg5$}YKN$>(;b=9G=PRm7r@&X1AdFIm@Nz$HIVFT#Q`3` zVl^2{`}0kz2}}TMhz9KRJfvT48{)CKcCnxT{#PDmutOtnNet3ku9Zrvbn9Ayo``dr04m?Kt)! z(cmS^W~V)1A~Jnek_9XT*d!1fiRexgJYwJwm2slr79WS8txEL&&* zjG1pvq#2^g&FQushPum(mQ7Cf%hGlvEf4V`V0&K};()<`F9tW{jfete@Cjup5Pk=5 zC_>@G+??_7v*Td|A7FH?kOUSSbXeXqh4Gbar4P_yjBq1iOJ@w~7};f<~lZ zrlNGrh~13j+BKh^@1;C_#q@?fz@{QRJ{j$$C!=c&4^jY;V6b~eY_kD^YBMG-cLP+> zOzXo+q*bjn8^vDR({6{_u}IXirMZzkk*4DY4hB|kmPH&zahw0PC~ju*tbh*hX)M?# zL*9u>I?Q(os6!jhq-ByVJ<8?kvuv|4DC`gwo@AaOf(qvnqN@!auu-ks^P`U8X1Hrq zqD&GfifTdVnL#BIwh(;KY<`3+gASV;b>t)hA4ME@l<@}0com}QX)0Qe_gk)vlIb=Y z081z?5@jj2;wk7I*qxwQ3wk4MLkWb95*4N+o>s#!8=;m*C3%(}atXTWcGtsv z%iA2VVn_o{r73V^%%iw+WK;@jvR~5%h}X)sYP|qUYW-*`Bv*rpVH9E~ByOu9=h4m# z)-i)lq#~wRJOet*VS8t=j{L7IE8(&#@Q!_$;(UB6hk)!OnNbQAfht)9WX(qL>;I5xHn}7UEQFB_e#CAjG~qv!{C<|r6@$SWyh{$AOp!BG4xY1O!7GX zA^9yVN zL|ke`RT$apbP4Z1<{%w_)?QuSPJq3^5zd21JCw`^f_S%(D%owi`vLYM|Zk7o*+~qy`KH39q=BVE7}VAe22f znq{YUsyB3j1HHlE6~Y5sltY1LO@-5t(b7wbq?U{ZWTTix`YumDltydqhS5l3B3H)a zLAk^weR@@{DSdA_mn}6k%MS8nJR53pVI0Bz!*Z0274D6_5bg-F0Pl=b~0C``#RIx8E+9K34hATh?~v& zBSna2&#d)`0|B059=8!8QgmXcyZ{Nha5gh70&~=O!Wu=Xa$KInGcpDH20<{gJj!O7 zlyfH|#g5JEvr6;8v2#KgKhNt<-teH_16y^eYoN};s<{I`;C-G56ouk7?8Fk>#=nRypERnS0PPWX4@6+av zdY_Pz?D3eTMY}TYfR*2{j>?j>GjnLu(N! z;WLXCTdugngxA-WIchKIarXEyK{(DymwvthIXE>^A3&b0&ipmToHJcur6AhCf3x zI_vzm7rRqb>i@RM%#Is2**!OJn(j9%?T^k)_WkYLhRg#e1J4HQaPz^@{j5(N32pAes(d;?&nsp)ReIXzAvsP*nqn*K3YY}(^n z$fkudvT4DFD;A88jgRkp^eG#c{o(QPD_?o-*^8e3#f_i5?5)cA$nu%@4#swa7ZD1h z_YxM!3m5M*E+^4_ki8be#j<^tjou`1)?^*}apVZ6aJPL9nJk=5SBDcdfpba{i zwrH0Ho5ohayBTB4#y5?P?Q;3?&#c=1*B?C+UA_LFp8EJ-_uux%%MU$9+pBcR8z4e_N6tQd-m8aF=KipV*;e%!Etv9}Zv>Cp8%UOHz7o2?S=gxJHA1wSUclWQ%+v9ilfBww#w?26P z)1|$R&cC{pw$4aAA-qstbjmZ`%c5i7dFI83>mC02H3z}t!Mg&E&pcoX(n^-iTW4svFv5n6$;<~PIL2J z-NVngY4GP)u6g3UFMJ$ZcJvWnJ8t62%N})aJmAvbtl#Z-x4-w=#>|TSw|vHR`X}z% zHojnNm$9o)djmP`HDqsC*hBUi8~b445@a!ajF0Vtta8c@Uw8nra^t?PM=I?3y`}H} zv3mBpW3S!D@~1920ihNzaV;KOv}o6{v4ugC|2aMaUse{XG`0T)6LnXI*r5 zL__t5j|S;QHS9q?QHw(OS0OHL2}=GgBZ{^aXh*PQh7)@x2Z_|?n4@YMMy z#tNY`h41Wh_Q~X(&z*4N-3J`|_A4va9KX-k-}+qfx}TQA&wuc}3oqS$JLccAW%&gs z2A7?6;bVK(?l^0=bAENqYv$PX5ApO#&wjC0D!hDP5qa>;59s9f#`Z7U9QlLdp&aXT`zs?sGqI6V9Rj#YuK}n zdtzn4^T53;_=onbJbM3u?LQsA$CIa?c}_=5B7NagoB@Y zV8iX7zUhxE_SofXyWMiw%5|M5*Drszu=0khTVG?7@;%>mjV9S;*Ypr_b&JBTfEXf|Nga; zDr3{w5R_=|L!!3A5xtQTS%fSA37jLUJJlb5SHWx=bI|ILiG`cSmOzgS;dhrQcvH1S zW5{W5BS%f^obj=hhdPSG`p`RYBW0s1ZsfF|%~T#oPW%2rr`-maQ&AdfaRP9zQ?6JO z>g=0Z6NSNq(iluM6?k=S0_^2(y=YB1rf9P>`Ai9(DLTK638xP*gbTIegavmsrL9eL zitRc)(9kASv!Q{Z?qHi)ngDBl&Ag79U=OQKXHavde+PP-ZAI;XHRrV1ne-jj?I4;X z_4Qf*#j&;hJ6`|FrO&-%U;1|MF&l~Bv+7G<_|nHN-oOUl{`RHEp0nnOTh874*LVNX zy7Ui^X)7KW#J#`1>X>)?A5`z#^|g~jUpOl{@p$mwTYhp$d|CAa*T3ezwP@+*_dhnh z*P)%q?>Ou5quHyUG>>>;>!Wuxf4hSI-=Fx!TlN0yzq@_^{jYoOl-0gp9QowaE1s%8 zan%ur@3rm98^3CucGAy3cHC0;*KYj_6@K%i;FkUVwBh6W#F-vr+wx5hKfm>A_q$2s zIPD)*kG<{FyRJ!ZeP+dBdpz?)>%jQ!Py0{3=XCk$Z(aNYjM)C`CtF)ixl+30V?X{n zam3z7{nLs=zRzEFf_h(c2uHZ@~4xgAMe(g*zL%c5b<6dv(pdv8&f_Os`n}vBiJA>Au15e}|5*{OcaB ze;KSg@?W0&;q`}J{PVjTdmjJtC09LmRQddCKe+Ia;3X%&cWChaciY>(b6)$hMy>yk6=DTOv3L^h3kk=pt*$+Xy;SeQfiQf74u=d;+gw&xB-K6r=db<^`PZr6>-ikanJxZ6irlM609E=qiFzm0Do%YXU>%=7TF z2kt-W)w9Hnm+$F%<-xJj7bH)*eZx-{AN?%9~;KMSNQI|%KMVN=H`q3d9T#!XWsNbl>btC^U@36 zdMEVDceliHYjWfZjb)$s#_GhEKYPKQbo?fK-OYPm_k)drz3=ZFdh~@WpWJZ&hKD-W z51u&s#9i_*RDT*{n%fQJ{K+?w zPt8{Hq6Z@f&H3K|SI+T2z$Znd*{Um08X=KoiWho_zIyYkzkWA&Uo85`{XaPGOSfHHS)N(^ zokf|)pL+GybKY+q@{MzD?VkVadB_!SJaE2+D?i`<2fQIZqinwV>=)nr;x9@QOM=%e zzpwi&wfhsNf9}k4-n`uW!yP8lBn)?N=_}v-U)3-*+B(E_3-|_kMoS>8S_)*#6<=u6N&lBYEl_zg=?aPbNONdL?-z znqG9%MV~uian~s5FTRjH@y)>HOXCxVUZd1j$HHtu Cp$(A$ diff --git a/ee/secureenclavesigner/test_app_resources/entitlements b/ee/secureenclavesigner/test_app_resources/entitlements deleted file mode 100644 index 4debb196d..000000000 --- a/ee/secureenclavesigner/test_app_resources/entitlements +++ /dev/null @@ -1,8 +0,0 @@ - - - keychain-access-groups - - X98UFR7HA3.com.kolide.agent.dev - - - diff --git a/ee/secureenclavesigner/test_app_resources/info.plist b/ee/secureenclavesigner/test_app_resources/info.plist deleted file mode 100644 index fe801acec..000000000 --- a/ee/secureenclavesigner/test_app_resources/info.plist +++ /dev/null @@ -1,20 +0,0 @@ - - - - - CFBundleExecutable - launcher_test - CFBundleIdentifier - com.kolide.agent - CFBundleName - launcher_test - LSUIElement - - CFBundlePackageType - APPL - CFBundleShortVersionString - 0.1 - CFBundleVersion - 0.1 - - diff --git a/ee/secureenclavesigner/test_app_resources/readme.md b/ee/secureenclavesigner/test_app_resources/readme.md deleted file mode 100644 index 939c65060..000000000 --- a/ee/secureenclavesigner/test_app_resources/readme.md +++ /dev/null @@ -1,31 +0,0 @@ -# Running Tests - -The files in this directory are used only for testing. - -The secure enclave keyer requires apple entitlements in order to be able to access the secure enclave to generate keys and perform cryptographic operations. In order to do this we build the secure enclave go tests to a binary, sign that binary with the required MacOS entitlements, then execute the binary and inspect the output. This is all done via the `TestSecureEnclaveTestRunner` function. - -In order to add entitlements we first need to create a MacOS app with the following structure: - -```sh -launcher_test.app - └── Contents - ├── Info.plist - ├── MacOS - │ └── launcher_test # <- this is the go test binary mentioned above - └── embedded.provisionprofile -``` - -Then we pass the top level directory to the MacOS codsign utility. - -In order to succesfully sign the app with entitlements, there are a few steps that must be completed on the machine in order to run the tests. - -1. Follow instructions to download and install a certificate from the Apple Developer account of type "Mac Development" https://developer.apple.com/account/resources/certificates/list -1. You may need to download and install the "Apple Worldwide Developer Relations Certificate" from https://www.apple.com/certificateauthority/. Inspect the issuer cert added in the previous step to find the correct cert to add. -1. Add you device to the developer account using the "Provisioning UDID" found at Desktop Menu Applie Icon> About This Mac > More Info > System Report https://developer.apple.com/account/resources/devices/list -1. Create a provisioing profile that includes the device https://developer.apple.com/account/resources/profiles/list ... should probably include all devices on the team and be updated in the repo -1. Replace the `embedded.provisionprofile` file with the new profile - -## Skipping Tests - -- To skip these tests (e.g. while running tests on a machine which is not included in the provisioning profile), you can set the `SKIP_SECURE_ENCLAVE_TESTS` environment variable to any non-empty value - - `SKIP_SECURE_ENCLAVE_TESTS=y make test` diff --git a/ee/tpmrunner/mocks/tpmSignerCreator.go b/ee/tpmrunner/mocks/tpmSignerCreator.go new file mode 100644 index 000000000..727456cb0 --- /dev/null +++ b/ee/tpmrunner/mocks/tpmSignerCreator.go @@ -0,0 +1,104 @@ +// Code generated by mockery v2.44.1. DO NOT EDIT. + +package mocks + +import ( + crypto "crypto" + + tpm "github.com/kolide/krypto/pkg/tpm" + mock "github.com/stretchr/testify/mock" +) + +// TpmSignerCreator is an autogenerated mock type for the tpmSignerCreator type +type TpmSignerCreator struct { + mock.Mock +} + +// CreateKey provides a mock function with given fields: opts +func (_m *TpmSignerCreator) CreateKey(opts ...tpm.TpmSignerOption) ([]byte, []byte, error) { + _va := make([]interface{}, len(opts)) + for _i := range opts { + _va[_i] = opts[_i] + } + var _ca []interface{} + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) + + if len(ret) == 0 { + panic("no return value specified for CreateKey") + } + + var r0 []byte + var r1 []byte + var r2 error + if rf, ok := ret.Get(0).(func(...tpm.TpmSignerOption) ([]byte, []byte, error)); ok { + return rf(opts...) + } + if rf, ok := ret.Get(0).(func(...tpm.TpmSignerOption) []byte); ok { + r0 = rf(opts...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]byte) + } + } + + if rf, ok := ret.Get(1).(func(...tpm.TpmSignerOption) []byte); ok { + r1 = rf(opts...) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).([]byte) + } + } + + if rf, ok := ret.Get(2).(func(...tpm.TpmSignerOption) error); ok { + r2 = rf(opts...) + } else { + r2 = ret.Error(2) + } + + return r0, r1, r2 +} + +// New provides a mock function with given fields: private, public +func (_m *TpmSignerCreator) New(private []byte, public []byte) (crypto.Signer, error) { + ret := _m.Called(private, public) + + if len(ret) == 0 { + panic("no return value specified for New") + } + + var r0 crypto.Signer + var r1 error + if rf, ok := ret.Get(0).(func([]byte, []byte) (crypto.Signer, error)); ok { + return rf(private, public) + } + if rf, ok := ret.Get(0).(func([]byte, []byte) crypto.Signer); ok { + r0 = rf(private, public) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(crypto.Signer) + } + } + + if rf, ok := ret.Get(1).(func([]byte, []byte) error); ok { + r1 = rf(private, public) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// NewTpmSignerCreator creates a new instance of TpmSignerCreator. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewTpmSignerCreator(t interface { + mock.TestingT + Cleanup(func()) +}) *TpmSignerCreator { + mock := &TpmSignerCreator{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go new file mode 100644 index 000000000..45bfc5c3a --- /dev/null +++ b/ee/tpmrunner/tpmrunner.go @@ -0,0 +1,215 @@ +package tpmrunner + +import ( + "context" + "crypto" + "errors" + "fmt" + "io" + "log/slog" + "time" + + "github.com/kolide/krypto/pkg/tpm" + "github.com/kolide/launcher/ee/agent/types" + "github.com/kolide/launcher/pkg/traces" +) + +type tpmRunner struct { + signer crypto.Signer + signerCreator tpmSignerCreator + store types.GetterSetterDeleter + slogger *slog.Logger + interrupt chan struct{} + interrupted bool +} + +type tpmSignerCreator interface { + CreateKey(opts ...tpm.TpmSignerOption) (private []byte, public []byte, err error) + New(private, public []byte) (crypto.Signer, error) +} + +type defaultTpmSignerCreator struct{} + +func (d defaultTpmSignerCreator) CreateKey(opts ...tpm.TpmSignerOption) (private []byte, public []byte, err error) { + return tpm.CreateKey() +} + +func (d defaultTpmSignerCreator) New(private, public []byte) (crypto.Signer, error) { + return tpm.New(private, public) +} + +type tpmRunnerOption func(*tpmRunner) + +func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, opts ...tpmRunnerOption) (*tpmRunner, error) { + _, span := traces.StartSpan(ctx) + defer span.End() + + _, _, err := fetchKeyData(store) + if err != nil { + return nil, err + } + + tpmRunner := &tpmRunner{ + store: store, + slogger: slogger.With("component", "tpmrunner"), + interrupt: make(chan struct{}), + signerCreator: defaultTpmSignerCreator{}, + } + + for _, opt := range opts { + opt(tpmRunner) + } + + return tpmRunner, nil +} + +// Public returns the public key of the current console user +// creating and peristing a new one if needed +func (tr *tpmRunner) Execute() error { + currentRetryInterval, maxRetryInterval := 1*time.Second, 1*time.Minute + retryTicker := time.NewTicker(currentRetryInterval) + defer retryTicker.Stop() + + for { + + signer, err := tr.fetchCreateKeys(context.TODO()) + if err != nil { + tr.slogger.Log(context.TODO(), slog.LevelError, + "creating tpm signer, will retry", + "err", err, + ) + + if currentRetryInterval < maxRetryInterval { + currentRetryInterval += time.Second + } + } else { + tr.signer = signer + retryTicker.Stop() + } + + select { + case <-retryTicker.C: + continue + case <-tr.interrupt: + tr.slogger.Log(context.TODO(), slog.LevelDebug, + "interrupt received, exiting secure enclave signer execute loop", + ) + return nil + } + } +} + +func (tr *tpmRunner) Interrupt(_ error) { + // Only perform shutdown tasks on first call to interrupt -- no need to repeat on potential extra calls. + if tr.interrupted { + return + } + + tr.interrupted = true + + // Tell the execute loop to stop checking, and exit + tr.interrupt <- struct{}{} +} + +// Public returns the public key of the current console user +// creating and peristing a new one if needed +func (tr *tpmRunner) Public() crypto.PublicKey { + if tr.signer == nil { + return nil + } + + return tr.signer.Public() +} + +func (tr *tpmRunner) Type() string { + return "tpm" +} + +func (tr *tpmRunner) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) { + if tr.signer == nil { + return nil, errors.New("no signer available") + } + + return tr.signer.Sign(rand, digest, opts) +} + +// This duplicates some of pkg/osquery/extension.go but that feels like the wrong place. +// Really, we should have a simpler interface over a storage layer. +const ( + privateEccData = "privateEccData" + publicEccData = "publicEccData" +) + +// nolint:unused +func fetchKeyData(store types.Getter) ([]byte, []byte, error) { + pri, err := store.Get([]byte(privateEccData)) + if err != nil { + return nil, nil, err + } + + pub, err := store.Get([]byte(publicEccData)) + if err != nil { + return nil, nil, err + } + + return pri, pub, nil +} + +// nolint:unused +func storeKeyData(store types.Setter, pri, pub []byte) error { + if pri != nil { + if err := store.Set([]byte(privateEccData), pri); err != nil { + return err + } + } + + if pub != nil { + if err := store.Set([]byte(publicEccData), pub); err != nil { + return err + } + } + + return nil +} + +// clearKeyData is used to clear the keys as part of error handling around new keys. It is not intended to be called +// regularly, and since the path that calls it is around DB errors, it has no error handling. +// nolint:unused +func clearKeyData(slogger *slog.Logger, deleter types.Deleter) { + slogger.Log(context.TODO(), slog.LevelInfo, + "clearing keys", + ) + _ = deleter.Delete([]byte(privateEccData), []byte(publicEccData)) +} + +func (tr *tpmRunner) fetchCreateKeys(ctx context.Context) (crypto.Signer, error) { + priData, pubData, err := fetchKeyData(tr.store) + if err != nil { + return nil, fmt.Errorf("unabled to access key data store: %w", err) + } + + if pubData == nil || priData == nil { + tr.slogger.Log(ctx, slog.LevelInfo, + "generating new keys", + ) + + var err error + priData, pubData, err = tr.signerCreator.CreateKey() + if err != nil { + clearKeyData(tr.slogger, tr.store) + return nil, fmt.Errorf("creating key: %w", err) + } + + if err := storeKeyData(tr.store, priData, pubData); err != nil { + clearKeyData(tr.slogger, tr.store) + return nil, fmt.Errorf("storing key: %w", err) + } + } + + k, err := tr.signerCreator.New(priData, pubData) + if err != nil { + return nil, fmt.Errorf("creating tpm signer: %w", err) + } + + return k, nil +} diff --git a/ee/tpmrunner/tpmrunner_test.go b/ee/tpmrunner/tpmrunner_test.go new file mode 100644 index 000000000..5860a20b9 --- /dev/null +++ b/ee/tpmrunner/tpmrunner_test.go @@ -0,0 +1,80 @@ +package tpmrunner + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/kolide/krypto/pkg/echelper" + "github.com/kolide/launcher/ee/agent/storage/inmemory" + "github.com/kolide/launcher/ee/tpmrunner/mocks" + "github.com/kolide/launcher/pkg/log/multislogger" + "github.com/stretchr/testify/require" +) + +func withTpmSignerCreator(tpmSignerCreator tpmSignerCreator) tpmRunnerOption { + return func(t *tpmRunner) { + t.signerCreator = tpmSignerCreator + } +} + +func Test_secureEnclaveSigner(t *testing.T) { + t.Parallel() + + privKey, err := echelper.GenerateEcdsaKey() + require.NoError(t, err) + + fakePrivData, fakePubData := []byte("fake priv data"), []byte("fake pub data") + + t.Run("creates key in execute", func(t *testing.T) { + t.Parallel() + + tpmSignerCreatorMock := mocks.NewTpmSignerCreator(t) + tpmRunner, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), withTpmSignerCreator(tpmSignerCreatorMock)) + require.NoError(t, err) + + require.Nil(t, tpmRunner.Public()) + + tpmSignerCreatorMock.On("CreateKey").Return(nil, nil, errors.New("not available yet")).Once() + tpmSignerCreatorMock.On("CreateKey").Return(fakePrivData, fakePubData, nil).Once() + tpmSignerCreatorMock.On("New", fakePrivData, fakePubData).Return(privKey, nil).Once() + + go func() { + tpmRunner.Execute() + }() + + // // sleep long enough to get through 2 cycles of exectue + time.Sleep(3 * time.Second) + require.NotNil(t, tpmRunner.Public()) + tpmRunner.Interrupt(errors.New("test")) + }) + + t.Run("loads existing key", func(t *testing.T) { + t.Parallel() + + // populate store with key info + store := inmemory.NewStore() + store.Set([]byte(privateEccData), fakePrivData) + store.Set([]byte(publicEccData), fakePubData) + + tpmSignerCreatorMock := mocks.NewTpmSignerCreator(t) + tpmRunner, err := New(context.TODO(), multislogger.NewNopLogger(), store, withTpmSignerCreator(tpmSignerCreatorMock)) + require.NoError(t, err) + + // public will be nil until execute is called and key is loaded form tpm + require.Nil(t, tpmRunner.Public()) + + tpmSignerCreatorMock.On("New", fakePrivData, fakePubData).Return(privKey, nil).Once() + + go func() { + tpmRunner.Execute() + }() + + // sleep long enough to get through 2 cycles of exectue + time.Sleep(3 * time.Second) + require.NotNil(t, tpmRunner.Public()) + tpmRunner.Interrupt(errors.New("test")) + }) + +} From 4c99f301ddd8a3b2d7bff0a0de9f934e4bfb8c88 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Thu, 5 Dec 2024 09:32:47 -0800 Subject: [PATCH 02/26] fix race, reset runner ticker --- ee/secureenclaverunner/secureenclaverunner.go | 1 + .../secureenclaverunner_test.go | 10 +++++----- ee/tpmrunner/tpmrunner.go | 1 + ee/tpmrunner/tpmrunner_test.go | 18 +++++++++--------- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index 7217f1c4c..c2d1ad64b 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -97,6 +97,7 @@ func (ser *secureEnclaveRunner) Execute() error { if currentRetryInterval < maxRetryInterval { currentRetryInterval += time.Second + retryTicker.Reset(currentRetryInterval) } } else { retryTicker.Stop() diff --git a/ee/secureenclaverunner/secureenclaverunner_test.go b/ee/secureenclaverunner/secureenclaverunner_test.go index c30352dcd..e8ef70489 100644 --- a/ee/secureenclaverunner/secureenclaverunner_test.go +++ b/ee/secureenclaverunner/secureenclaverunner_test.go @@ -54,15 +54,15 @@ func Test_secureEnclaveRunner(t *testing.T) { secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(&privKey.PublicKey, nil).Once() go func() { - ser.Execute() + // sleep long enough to get through 2 cycles of exectue + time.Sleep(3 * time.Second) + ser.Interrupt(errors.New("test")) }() - // sleep long enough to get through 2 cycles of exectue - time.Sleep(3 * time.Second) - + require.NoError(t, ser.Execute()) // one cycle of execute should have created key require.NotNil(t, ser.Public()) - ser.Interrupt(errors.New("test")) + }) t.Run("loads existing key", func(t *testing.T) { diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index 45bfc5c3a..00aeaa9e1 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -81,6 +81,7 @@ func (tr *tpmRunner) Execute() error { if currentRetryInterval < maxRetryInterval { currentRetryInterval += time.Second + retryTicker.Reset(currentRetryInterval) } } else { tr.signer = signer diff --git a/ee/tpmrunner/tpmrunner_test.go b/ee/tpmrunner/tpmrunner_test.go index 5860a20b9..ef821a6fb 100644 --- a/ee/tpmrunner/tpmrunner_test.go +++ b/ee/tpmrunner/tpmrunner_test.go @@ -41,13 +41,14 @@ func Test_secureEnclaveSigner(t *testing.T) { tpmSignerCreatorMock.On("New", fakePrivData, fakePubData).Return(privKey, nil).Once() go func() { - tpmRunner.Execute() + // sleep long enough to get through 2 cycles of execute + time.Sleep(3 * time.Second) + tpmRunner.Interrupt(errors.New("test")) }() - // // sleep long enough to get through 2 cycles of exectue - time.Sleep(3 * time.Second) + require.NoError(t, tpmRunner.Execute()) require.NotNil(t, tpmRunner.Public()) - tpmRunner.Interrupt(errors.New("test")) + }) t.Run("loads existing key", func(t *testing.T) { @@ -68,13 +69,12 @@ func Test_secureEnclaveSigner(t *testing.T) { tpmSignerCreatorMock.On("New", fakePrivData, fakePubData).Return(privKey, nil).Once() go func() { - tpmRunner.Execute() + // sleep long enough to get through 2 cycles of exectue + time.Sleep(3 * time.Second) + tpmRunner.Interrupt(errors.New("test")) }() - // sleep long enough to get through 2 cycles of exectue - time.Sleep(3 * time.Second) + require.NoError(t, tpmRunner.Execute()) require.NotNil(t, tpmRunner.Public()) - tpmRunner.Interrupt(errors.New("test")) }) - } From 5bd7d17f4e9eefe6cbafb9a86fbb58c8308d6a69 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Thu, 5 Dec 2024 10:01:20 -0800 Subject: [PATCH 03/26] lint --- ee/agent/keys.go | 6 +----- ee/secureenclaverunner/secureenclaverunner_test.go | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/ee/agent/keys.go b/ee/agent/keys.go index fda5abcfb..c32a6fe7a 100644 --- a/ee/agent/keys.go +++ b/ee/agent/keys.go @@ -9,7 +9,6 @@ import ( "github.com/kolide/launcher/ee/agent/keys" "github.com/kolide/launcher/ee/agent/types" - "github.com/kolide/launcher/pkg/traces" ) type keyInt interface { @@ -32,10 +31,7 @@ type secureEnclaveClient interface { CreateSecureEnclaveKey(uid string) (*ecdsa.PublicKey, error) } -func SetupKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter) error { - ctx, span := traces.StartSpan(ctx) - defer span.End() - +func SetupKeys(_ context.Context, slogger *slog.Logger, store types.GetterSetterDeleter) error { slogger = slogger.With("component", "agentkeys") var err error diff --git a/ee/secureenclaverunner/secureenclaverunner_test.go b/ee/secureenclaverunner/secureenclaverunner_test.go index e8ef70489..0400f431d 100644 --- a/ee/secureenclaverunner/secureenclaverunner_test.go +++ b/ee/secureenclaverunner/secureenclaverunner_test.go @@ -82,7 +82,7 @@ func Test_secureEnclaveRunner(t *testing.T) { err = store.Set([]byte(publicEccDataKey), serJson) require.NoError(t, err) - // create new signer with store containting key + // create new signer with store containing key ser, err := New(context.TODO(), multislogger.NewNopLogger(), store, nil) require.NoError(t, err) From 207b4c9f9044c4c11504f2aef23208f977844251 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Thu, 5 Dec 2024 10:13:35 -0800 Subject: [PATCH 04/26] add tracing to tpm runner --- ee/tpmrunner/tpmrunner.go | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index 00aeaa9e1..8ea5f1428 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -71,10 +71,10 @@ func (tr *tpmRunner) Execute() error { defer retryTicker.Stop() for { - - signer, err := tr.fetchCreateKeys(context.TODO()) + ctx := context.Background() + signer, err := tr.loadOrCreateKeys(ctx) if err != nil { - tr.slogger.Log(context.TODO(), slog.LevelError, + tr.slogger.Log(ctx, slog.LevelError, "creating tpm signer, will retry", "err", err, ) @@ -92,7 +92,7 @@ func (tr *tpmRunner) Execute() error { case <-retryTicker.C: continue case <-tr.interrupt: - tr.slogger.Log(context.TODO(), slog.LevelDebug, + tr.slogger.Log(ctx, slog.LevelDebug, "interrupt received, exiting secure enclave signer execute loop", ) return nil @@ -183,34 +183,51 @@ func clearKeyData(slogger *slog.Logger, deleter types.Deleter) { _ = deleter.Delete([]byte(privateEccData), []byte(publicEccData)) } -func (tr *tpmRunner) fetchCreateKeys(ctx context.Context) (crypto.Signer, error) { +func (tr *tpmRunner) loadOrCreateKeys(ctx context.Context) (crypto.Signer, error) { + ctx, span := traces.StartSpan(ctx) + defer span.End() + priData, pubData, err := fetchKeyData(tr.store) if err != nil { - return nil, fmt.Errorf("unabled to access key data store: %w", err) + thisErr := fmt.Errorf("fetching key data for data store: %w", err) + traces.SetError(span, thisErr) + return nil, thisErr } if pubData == nil || priData == nil { tr.slogger.Log(ctx, slog.LevelInfo, - "generating new keys", + "generating new tpm keys", ) var err error priData, pubData, err = tr.signerCreator.CreateKey() if err != nil { + thisErr := fmt.Errorf("creating key: %w", err) + traces.SetError(span, thisErr) + clearKeyData(tr.slogger, tr.store) - return nil, fmt.Errorf("creating key: %w", err) + return nil, thisErr } if err := storeKeyData(tr.store, priData, pubData); err != nil { + thisErr := fmt.Errorf("storing key data: %w", err) + traces.SetError(span, thisErr) + clearKeyData(tr.slogger, tr.store) - return nil, fmt.Errorf("storing key: %w", err) + return nil, thisErr } + + span.AddEvent("generated_new_tpm_keys") } k, err := tr.signerCreator.New(priData, pubData) if err != nil { - return nil, fmt.Errorf("creating tpm signer: %w", err) + thisErr := fmt.Errorf("creating tpm signer: %w", err) + traces.SetError(span, thisErr) + return nil, thisErr } + span.AddEvent("created_tpm_signer") + return k, nil } From d35e9afcf205ed8ab2db7471c872ba7c5a985e89 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Thu, 5 Dec 2024 10:43:49 -0800 Subject: [PATCH 05/26] comments --- cmd/launcher/launcher.go | 2 +- ee/agent/keys.go | 1 + ee/agent/keys_darwin.go | 2 +- ee/agent/keys_tpm.go | 4 +++- ee/tpmrunner/tpmrunner.go | 8 ++++++++ 5 files changed, 14 insertions(+), 3 deletions(-) diff --git a/cmd/launcher/launcher.go b/cmd/launcher/launcher.go index 613559fb4..9f9cc8355 100644 --- a/cmd/launcher/launcher.go +++ b/cmd/launcher/launcher.go @@ -434,7 +434,7 @@ func runLauncher(ctx context.Context, cancel func(), multiSlogger, systemMultiSl return fmt.Errorf("failed to create desktop runner: %w", err) } - execute, interrupt, err := agent.SetupHardwareKeys(ctx, k.Slogger(), k.ConfigStore(), runner) + execute, interrupt, err := agent.SetHardwareKeysRunner(ctx, k.Slogger(), k.ConfigStore(), runner) if err != nil { return fmt.Errorf("setting up hardware keys: %w", err) } diff --git a/ee/agent/keys.go b/ee/agent/keys.go index c32a6fe7a..6087a238a 100644 --- a/ee/agent/keys.go +++ b/ee/agent/keys.go @@ -19,6 +19,7 @@ type keyInt interface { var hardwareKeys keyInt = keys.Noop var localDbKeys keyInt = keys.Noop +// HardwareKeys returns the hardware keys for the agent, it's critical to not cache this value as it may change during runtime. func HardwareKeys() keyInt { return hardwareKeys } diff --git a/ee/agent/keys_darwin.go b/ee/agent/keys_darwin.go index 6cc852d46..b8add7c63 100644 --- a/ee/agent/keys_darwin.go +++ b/ee/agent/keys_darwin.go @@ -13,7 +13,7 @@ import ( "github.com/kolide/launcher/pkg/traces" ) -func SetupHardwareKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, secureEnclaveClient secureEnclaveClient) (execute func() error, interrupt func(error), err error) { +func SetHardwareKeysRunner(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, secureEnclaveClient secureEnclaveClient) (execute func() error, interrupt func(error), err error) { ctx, span := traces.StartSpan(ctx) defer span.End() diff --git a/ee/agent/keys_tpm.go b/ee/agent/keys_tpm.go index f5c579783..0badeb9eb 100644 --- a/ee/agent/keys_tpm.go +++ b/ee/agent/keys_tpm.go @@ -11,7 +11,9 @@ import ( "github.com/kolide/launcher/ee/tpmrunner" ) -func SetupHardwareKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, _ secureEnclaveClient) (execute func() error, interrupt func(error), err error) { +// SetHardwareKeysRunner creates a tpm runner and sets it as the agent hardware key as it also implements the keyInt/cyrpto.Signer interface. +// The returned execute and interrupt functions can be used to start and stop the secure enclave runner, generally via a run group. +func SetHardwareKeysRunner(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, _ secureEnclaveClient) (execute func() error, interrupt func(error), err error) { tpmRunner, err := tpmrunner.New(ctx, slogger, store) if err != nil { return nil, nil, err diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index 8ea5f1428..e87a93b1a 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -23,21 +23,29 @@ type tpmRunner struct { interrupted bool } +// tpmSignerCreator is an interface for creating and loading TPM signers +// useful for mocking in tests type tpmSignerCreator interface { CreateKey(opts ...tpm.TpmSignerOption) (private []byte, public []byte, err error) New(private, public []byte) (crypto.Signer, error) } +// defaultTpmSignerCreator is the default implementation of tpmSignerCreator +// using the tpm package type defaultTpmSignerCreator struct{} +// CreateKey creates a new TPM key func (d defaultTpmSignerCreator) CreateKey(opts ...tpm.TpmSignerOption) (private []byte, public []byte, err error) { return tpm.CreateKey() } +// New creates a new TPM signer func (d defaultTpmSignerCreator) New(private, public []byte) (crypto.Signer, error) { return tpm.New(private, public) } +// tpmRunnerOption is a functional option for tpmRunner +// useful for setting dependencies in tests type tpmRunnerOption func(*tpmRunner) func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, opts ...tpmRunnerOption) (*tpmRunner, error) { From 111f0959709d51410b1ceed4f8f70458c1ca0ae6 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Thu, 5 Dec 2024 10:57:43 -0800 Subject: [PATCH 06/26] fix client path --- ee/desktop/user/client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/desktop/user/client/client.go b/ee/desktop/user/client/client.go index 967d2352d..3757c49ab 100644 --- a/ee/desktop/user/client/client.go +++ b/ee/desktop/user/client/client.go @@ -99,7 +99,7 @@ func (c *client) DetectPresence(reason string, interval time.Duration) (time.Dur } func (c *client) CreateSecureEnclaveKey() (*ecdsa.PublicKey, error) { - resp, err := c.base.Get("http://unix/secure_enclave_key") + resp, err := c.base.Get("http://unix/create_secure_enclave_key") if err != nil { return nil, fmt.Errorf("getting secure enclave key: %w", err) } From 165852ea91ecbbe06e4ef1ab8cbb7d2aa8d3cfe9 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Thu, 5 Dec 2024 14:14:52 -0800 Subject: [PATCH 07/26] more comments --- ee/agent/keys_darwin.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ee/agent/keys_darwin.go b/ee/agent/keys_darwin.go index b8add7c63..d30b0f996 100644 --- a/ee/agent/keys_darwin.go +++ b/ee/agent/keys_darwin.go @@ -13,6 +13,8 @@ import ( "github.com/kolide/launcher/pkg/traces" ) +// SetHardwareKeysRunner creates a secure enclave runner and sets it as the agent hardware key as it also implements the keyInt/cyrpto.Signer interface. +// The returned execute and interrupt functions can be used to start and stop the secure enclave runner, generally via a run group. func SetHardwareKeysRunner(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, secureEnclaveClient secureEnclaveClient) (execute func() error, interrupt func(error), err error) { ctx, span := traces.StartSpan(ctx) defer span.End() From 805019ce7f708d8a0575c7f3a92e2dd5698600ba Mon Sep 17 00:00:00 2001 From: James Pickett Date: Fri, 6 Dec 2024 09:45:09 -0800 Subject: [PATCH 08/26] invert secure enclave runner return --- ee/secureenclaverunner/secureenclaverunner.go | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index c2d1ad64b..7126e92a8 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -60,21 +60,23 @@ func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDele return nil, fmt.Errorf("getting public ecc data from store: %w", err) } - if data != nil { - if err := json.Unmarshal(data, ser); err != nil { - traces.SetError(span, fmt.Errorf("unmarshaling secure enclave signer: %w", err)) + if data == nil { + return ser, nil + } + + if err := json.Unmarshal(data, ser); err != nil { + traces.SetError(span, fmt.Errorf("unmarshaling secure enclave signer: %w", err)) + ser.slogger.Log(ctx, slog.LevelError, + "unable to unmarshal secure enclave signer, data may be corrupt, wiping", + "err", err, + ) + + if err := store.Delete([]byte(publicEccDataKey)); err != nil { + traces.SetError(span, fmt.Errorf("deleting corrupt public ecc data: %w", err)) ser.slogger.Log(ctx, slog.LevelError, "unable to unmarshal secure enclave signer, data may be corrupt, wiping", "err", err, ) - - if err := store.Delete([]byte(publicEccDataKey)); err != nil { - traces.SetError(span, fmt.Errorf("deleting corrupt public ecc data: %w", err)) - ser.slogger.Log(ctx, slog.LevelError, - "unable to unmarshal secure enclave signer, data may be corrupt, wiping", - "err", err, - ) - } } } From 124c2a4ae0551f7385b298905a01c92ac075b796 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Fri, 6 Dec 2024 10:00:34 -0800 Subject: [PATCH 09/26] remove unneeded no lint directives --- ee/tpmrunner/tpmrunner.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index e87a93b1a..3c03a06ce 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -149,7 +149,6 @@ const ( publicEccData = "publicEccData" ) -// nolint:unused func fetchKeyData(store types.Getter) ([]byte, []byte, error) { pri, err := store.Get([]byte(privateEccData)) if err != nil { @@ -164,7 +163,6 @@ func fetchKeyData(store types.Getter) ([]byte, []byte, error) { return pri, pub, nil } -// nolint:unused func storeKeyData(store types.Setter, pri, pub []byte) error { if pri != nil { if err := store.Set([]byte(privateEccData), pri); err != nil { From 1257fa0f0995de25d232bc3c9aeea74baaa3d15a Mon Sep 17 00:00:00 2001 From: James Pickett Date: Fri, 6 Dec 2024 10:08:35 -0800 Subject: [PATCH 10/26] fix comment --- ee/tpmrunner/tpmrunner.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index 3c03a06ce..586b84c09 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -120,8 +120,7 @@ func (tr *tpmRunner) Interrupt(_ error) { tr.interrupt <- struct{}{} } -// Public returns the public key of the current console user -// creating and peristing a new one if needed +// Public returns the public hardware key func (tr *tpmRunner) Public() crypto.PublicKey { if tr.signer == nil { return nil From 238b058f88b2dccc884298acb12c477faeb5d0bb Mon Sep 17 00:00:00 2001 From: James Pickett Date: Mon, 9 Dec 2024 10:42:11 -0800 Subject: [PATCH 11/26] move hardware key loading to execute loop --- ee/secureenclaverunner/secureenclaverunner.go | 50 ++++++++---------- .../secureenclaverunner_test.go | 8 +++ ee/tpmrunner/tpmrunner.go | 52 ++++++++----------- ee/tpmrunner/tpmrunner_test.go | 2 +- 4 files changed, 54 insertions(+), 58 deletions(-) diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index 7126e92a8..735d25c8a 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -45,54 +45,48 @@ func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDele ctx, span := traces.StartSpan(ctx) defer span.End() - ser := &secureEnclaveRunner{ + return &secureEnclaveRunner{ uidPubKeyMap: make(map[string]*ecdsa.PublicKey), store: store, secureEnclaveClient: secureEnclaveClient, slogger: slogger.With("component", "secureenclaverunner"), mux: &sync.Mutex{}, interrupt: make(chan struct{}), - } + }, nil +} - data, err := store.Get([]byte(publicEccDataKey)) +// Public returns the public key of the current console user +// creating and peristing a new one if needed +func (ser *secureEnclaveRunner) Execute() error { + data, err := ser.store.Get([]byte(publicEccDataKey)) if err != nil { - traces.SetError(span, fmt.Errorf("getting public ecc data from store: %w", err)) - return nil, fmt.Errorf("getting public ecc data from store: %w", err) + return fmt.Errorf("getting public ecc data from store: %w", err) } - if data == nil { - return ser, nil - } - - if err := json.Unmarshal(data, ser); err != nil { - traces.SetError(span, fmt.Errorf("unmarshaling secure enclave signer: %w", err)) - ser.slogger.Log(ctx, slog.LevelError, - "unable to unmarshal secure enclave signer, data may be corrupt, wiping", - "err", err, - ) - - if err := store.Delete([]byte(publicEccDataKey)); err != nil { - traces.SetError(span, fmt.Errorf("deleting corrupt public ecc data: %w", err)) - ser.slogger.Log(ctx, slog.LevelError, + if data != nil { + if err := json.Unmarshal(data, ser); err != nil { + ser.slogger.Log(context.TODO(), slog.LevelError, "unable to unmarshal secure enclave signer, data may be corrupt, wiping", "err", err, ) + + if err := ser.store.Delete([]byte(publicEccDataKey)); err != nil { + ser.slogger.Log(context.TODO(), slog.LevelError, + "unable to unmarshal secure enclave signer, data may be corrupt, wiping", + "err", err, + ) + } } } - return ser, nil -} - -// Public returns the public key of the current console user -// creating and peristing a new one if needed -func (ser *secureEnclaveRunner) Execute() error { currentRetryInterval, maxRetryInterval := 1*time.Second, 1*time.Minute retryTicker := time.NewTicker(currentRetryInterval) defer retryTicker.Stop() for { - if _, err := ser.currentConsoleUserKey(context.TODO()); err != nil { - ser.slogger.Log(context.TODO(), slog.LevelError, + ctx := context.Background() + if _, err := ser.currentConsoleUserKey(ctx); err != nil { + ser.slogger.Log(ctx, slog.LevelError, "getting current console user key, will retry", "err", err, ) @@ -109,7 +103,7 @@ func (ser *secureEnclaveRunner) Execute() error { case <-retryTicker.C: continue case <-ser.interrupt: - ser.slogger.Log(context.TODO(), slog.LevelDebug, + ser.slogger.Log(ctx, slog.LevelDebug, "interrupt received, exiting secure enclave signer execute loop", ) return nil diff --git a/ee/secureenclaverunner/secureenclaverunner_test.go b/ee/secureenclaverunner/secureenclaverunner_test.go index 0400f431d..4d6e543a3 100644 --- a/ee/secureenclaverunner/secureenclaverunner_test.go +++ b/ee/secureenclaverunner/secureenclaverunner_test.go @@ -86,6 +86,14 @@ func Test_secureEnclaveRunner(t *testing.T) { ser, err := New(context.TODO(), multislogger.NewNopLogger(), store, nil) require.NoError(t, err) + go func() { + // sleep long enough to get through 2 cycles of exectue + time.Sleep(3 * time.Second) + ser.Interrupt(errors.New("test")) + }() + + require.NoError(t, ser.Execute()) + // should be able to fetch the key require.NotNil(t, ser.Public()) }) diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index 586b84c09..5588ac81b 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -14,25 +14,31 @@ import ( "github.com/kolide/launcher/pkg/traces" ) -type tpmRunner struct { - signer crypto.Signer - signerCreator tpmSignerCreator - store types.GetterSetterDeleter - slogger *slog.Logger - interrupt chan struct{} - interrupted bool -} +type ( + tpmRunner struct { + signer crypto.Signer + signerCreator tpmSignerCreator + store types.GetterSetterDeleter + slogger *slog.Logger + interrupt chan struct{} + interrupted bool + } -// tpmSignerCreator is an interface for creating and loading TPM signers -// useful for mocking in tests -type tpmSignerCreator interface { - CreateKey(opts ...tpm.TpmSignerOption) (private []byte, public []byte, err error) - New(private, public []byte) (crypto.Signer, error) -} + // tpmSignerCreator is an interface for creating and loading TPM signers + // useful for mocking in tests + tpmSignerCreator interface { + CreateKey(opts ...tpm.TpmSignerOption) (private []byte, public []byte, err error) + New(private, public []byte) (crypto.Signer, error) + } + + // defaultTpmSignerCreator is the default implementation of tpmSignerCreator + // using the tpm package + defaultTpmSignerCreator struct{} -// defaultTpmSignerCreator is the default implementation of tpmSignerCreator -// using the tpm package -type defaultTpmSignerCreator struct{} + // tpmRunnerOption is a functional option for tpmRunner + // useful for setting dependencies in tests + tpmRunnerOption func(*tpmRunner) +) // CreateKey creates a new TPM key func (d defaultTpmSignerCreator) CreateKey(opts ...tpm.TpmSignerOption) (private []byte, public []byte, err error) { @@ -44,19 +50,7 @@ func (d defaultTpmSignerCreator) New(private, public []byte) (crypto.Signer, err return tpm.New(private, public) } -// tpmRunnerOption is a functional option for tpmRunner -// useful for setting dependencies in tests -type tpmRunnerOption func(*tpmRunner) - func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, opts ...tpmRunnerOption) (*tpmRunner, error) { - _, span := traces.StartSpan(ctx) - defer span.End() - - _, _, err := fetchKeyData(store) - if err != nil { - return nil, err - } - tpmRunner := &tpmRunner{ store: store, slogger: slogger.With("component", "tpmrunner"), diff --git a/ee/tpmrunner/tpmrunner_test.go b/ee/tpmrunner/tpmrunner_test.go index ef821a6fb..8a569aeb6 100644 --- a/ee/tpmrunner/tpmrunner_test.go +++ b/ee/tpmrunner/tpmrunner_test.go @@ -19,7 +19,7 @@ func withTpmSignerCreator(tpmSignerCreator tpmSignerCreator) tpmRunnerOption { } } -func Test_secureEnclaveSigner(t *testing.T) { +func Test_tpmRunner(t *testing.T) { t.Parallel() privKey, err := echelper.GenerateEcdsaKey() From ffc708bda435e7ab579b65864d3acc88353f8a06 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Mon, 9 Dec 2024 10:48:08 -0800 Subject: [PATCH 12/26] lint --- ee/secureenclaverunner/secureenclaverunner.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index 735d25c8a..c8a39af9c 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -41,10 +41,7 @@ type secureEnclaveClient interface { CreateSecureEnclaveKey(uid string) (*ecdsa.PublicKey, error) } -func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, secureEnclaveClient secureEnclaveClient) (*secureEnclaveRunner, error) { - ctx, span := traces.StartSpan(ctx) - defer span.End() - +func New(_ context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, secureEnclaveClient secureEnclaveClient) (*secureEnclaveRunner, error) { return &secureEnclaveRunner{ uidPubKeyMap: make(map[string]*ecdsa.PublicKey), store: store, From 5c2a107f681049604e9489e77a197c39fdc92702 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Mon, 9 Dec 2024 12:58:46 -0800 Subject: [PATCH 13/26] add multiplicative ticker, use in runners --- ee/secureenclaverunner/secureenclaverunner.go | 9 +- ee/tpmrunner/tpmrunner.go | 9 +- pkg/backoff/ticker.go | 77 +++++++++++++ pkg/backoff/ticker_test.go | 105 ++++++++++++++++++ 4 files changed, 186 insertions(+), 14 deletions(-) create mode 100644 pkg/backoff/ticker.go create mode 100644 pkg/backoff/ticker_test.go diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index c8a39af9c..fec41a49d 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -20,6 +20,7 @@ import ( "github.com/kolide/launcher/ee/agent/types" "github.com/kolide/launcher/ee/consoleuser" + "github.com/kolide/launcher/pkg/backoff" "github.com/kolide/launcher/pkg/traces" ) @@ -76,8 +77,7 @@ func (ser *secureEnclaveRunner) Execute() error { } } - currentRetryInterval, maxRetryInterval := 1*time.Second, 1*time.Minute - retryTicker := time.NewTicker(currentRetryInterval) + retryTicker := backoff.NewMultiplicativeTicker(time.Second, time.Minute) defer retryTicker.Stop() for { @@ -87,11 +87,6 @@ func (ser *secureEnclaveRunner) Execute() error { "getting current console user key, will retry", "err", err, ) - - if currentRetryInterval < maxRetryInterval { - currentRetryInterval += time.Second - retryTicker.Reset(currentRetryInterval) - } } else { retryTicker.Stop() } diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index 5588ac81b..d58c44e3f 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -11,6 +11,7 @@ import ( "github.com/kolide/krypto/pkg/tpm" "github.com/kolide/launcher/ee/agent/types" + "github.com/kolide/launcher/pkg/backoff" "github.com/kolide/launcher/pkg/traces" ) @@ -68,8 +69,7 @@ func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDele // Public returns the public key of the current console user // creating and peristing a new one if needed func (tr *tpmRunner) Execute() error { - currentRetryInterval, maxRetryInterval := 1*time.Second, 1*time.Minute - retryTicker := time.NewTicker(currentRetryInterval) + retryTicker := backoff.NewMultiplicativeTicker(time.Second, time.Minute) defer retryTicker.Stop() for { @@ -80,11 +80,6 @@ func (tr *tpmRunner) Execute() error { "creating tpm signer, will retry", "err", err, ) - - if currentRetryInterval < maxRetryInterval { - currentRetryInterval += time.Second - retryTicker.Reset(currentRetryInterval) - } } else { tr.signer = signer retryTicker.Stop() diff --git a/pkg/backoff/ticker.go b/pkg/backoff/ticker.go new file mode 100644 index 000000000..d0333e00e --- /dev/null +++ b/pkg/backoff/ticker.go @@ -0,0 +1,77 @@ +package backoff + +import ( + "time" +) + +// NewMultiplicativeTicker returns a ticker where each interval = baseDuration * ticks until maxDuration is reached. +func NewMultiplicativeTicker(baseDuration, maxDuration time.Duration) *ticker { + return newTicker(newMultiplicativeCounter(baseDuration, maxDuration)) +} + +type ticker struct { + C chan time.Time + baseTicker *time.Ticker + stoppedChan chan struct{} + stopped bool + durationCounter durationCounter +} + +func newTicker(durationCounter *durationCounter) *ticker { + thisTicker := &ticker{ + C: make(chan time.Time), + stoppedChan: make(chan struct{}), + durationCounter: *durationCounter, + } + + thisTicker.baseTicker = time.NewTicker(thisTicker.durationCounter.next()) + + go func() { + for { + select { + case t := <-thisTicker.baseTicker.C: + thisTicker.baseTicker.Reset(thisTicker.durationCounter.next()) + thisTicker.C <- t + case <-thisTicker.stoppedChan: + thisTicker.baseTicker.Stop() + return + } + } + }() + + return thisTicker +} + +func (t *ticker) Stop() { + if t.stopped { + return + } + + t.stopped = true + close(t.stoppedChan) +} + +type durationCounter struct { + count int + baseInterval, maxInterval time.Duration + calcNext func(count int, baseDuration time.Duration) time.Duration +} + +func (dc *durationCounter) next() time.Duration { + dc.count++ + interval := dc.calcNext(dc.count, dc.baseInterval) + if interval > dc.maxInterval { + return dc.maxInterval + } + return interval +} + +func newMultiplicativeCounter(baseDuration, maxDuration time.Duration) *durationCounter { + return &durationCounter{ + baseInterval: baseDuration, + maxInterval: maxDuration, + calcNext: func(count int, baseInterval time.Duration) time.Duration { + return baseInterval * time.Duration(count) + }, + } +} diff --git a/pkg/backoff/ticker_test.go b/pkg/backoff/ticker_test.go new file mode 100644 index 000000000..73eda66d9 --- /dev/null +++ b/pkg/backoff/ticker_test.go @@ -0,0 +1,105 @@ +package backoff + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestMultiplicativeCounter(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + baseInterval time.Duration + maxInterval time.Duration + expected []time.Duration + }{ + { + name: "seconds", + baseInterval: time.Second, + maxInterval: 5 * time.Second, + expected: []time.Duration{ + time.Second, // 1s + 2 * time.Second, // 2s + 3 * time.Second, // 3s + 4 * time.Second, // 4s + 5 * time.Second, // 5s (max interval) + 5 * time.Second, // capped at max interval + }, + }, + { + name: "minutes", + baseInterval: time.Minute, + maxInterval: 3 * time.Minute, + expected: []time.Duration{ + time.Minute, // 1m + 2 * time.Minute, // 2m + 3 * time.Minute, // 3m (max interval) + 3 * time.Minute, // capped at max interval + 3 * time.Minute, // capped at max interval + }, + }, + { + name: "combo", + baseInterval: (1 * time.Minute) + (30 * time.Second), + maxInterval: 5 * time.Minute, + expected: []time.Duration{ + (1 * time.Minute) + (30 * time.Second), // 1m30s + 2 * ((1 * time.Minute) + (30 * time.Second)), // 3m + 3 * ((1 * time.Minute) + (30 * time.Second)), // 4m30s + 5 * time.Minute, // 5m + 5 * time.Minute, // 5m + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ec := newMultiplicativeCounter(tt.baseInterval, tt.maxInterval) + for _, expected := range tt.expected { + require.Equal(t, expected, ec.next()) + } + }) + } +} + +// TestMultiplicativeTicker tests the NewMultiplicativeTicker and its behavior. +func TestMultiplicativeTicker(t *testing.T) { + baseTime := 100 * time.Millisecond + maxTime := 500 * time.Millisecond + + tk := NewMultiplicativeTicker(baseTime, maxTime) + defer tk.Stop() + + expectedDurations := []time.Duration{ + 100 * time.Millisecond, + 200 * time.Millisecond, + 300 * time.Millisecond, + 400 * time.Millisecond, + 500 * time.Millisecond, // maxTime limit + 500 * time.Millisecond, // maxTime limit + } + + buffer := 25 * time.Millisecond + + for _, expected := range expectedDurations { + start := time.Now() + + select { + case <-tk.C: + require.WithinDuration(t, start, time.Now(), expected+buffer) + case <-time.After(maxTime + buffer): + t.Fatalf("ticker did not send event in expected time: %v", expected) + } + } + + // stop the ticker + tk.Stop() + + // call stop again to make sure no panic (same as stdlib ticker) + tk.Stop() +} From cc011e5e4e87c5f7f952e0e76bf6b3595ceada6d Mon Sep 17 00:00:00 2001 From: James Pickett Date: Mon, 9 Dec 2024 13:07:29 -0800 Subject: [PATCH 14/26] lint --- pkg/backoff/ticker_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/backoff/ticker_test.go b/pkg/backoff/ticker_test.go index 73eda66d9..ef113aaee 100644 --- a/pkg/backoff/ticker_test.go +++ b/pkg/backoff/ticker_test.go @@ -69,6 +69,8 @@ func TestMultiplicativeCounter(t *testing.T) { // TestMultiplicativeTicker tests the NewMultiplicativeTicker and its behavior. func TestMultiplicativeTicker(t *testing.T) { + t.Parallel() + baseTime := 100 * time.Millisecond maxTime := 500 * time.Millisecond From 257ce9900813861d3d3083de57d2c29260ed2ddd Mon Sep 17 00:00:00 2001 From: James Pickett Date: Mon, 9 Dec 2024 13:21:58 -0800 Subject: [PATCH 15/26] increase ticker test buffer --- pkg/backoff/ticker_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/backoff/ticker_test.go b/pkg/backoff/ticker_test.go index ef113aaee..6c2aed950 100644 --- a/pkg/backoff/ticker_test.go +++ b/pkg/backoff/ticker_test.go @@ -86,7 +86,7 @@ func TestMultiplicativeTicker(t *testing.T) { 500 * time.Millisecond, // maxTime limit } - buffer := 25 * time.Millisecond + buffer := 50 * time.Millisecond for _, expected := range expectedDurations { start := time.Now() From f2192ad67ee207d9e9f373033255190d0e121e29 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Mon, 9 Dec 2024 14:40:11 -0800 Subject: [PATCH 16/26] use post to crate secure enclave key --- ee/desktop/user/client/client.go | 2 +- ee/desktop/user/server/server.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/desktop/user/client/client.go b/ee/desktop/user/client/client.go index 3757c49ab..c08452350 100644 --- a/ee/desktop/user/client/client.go +++ b/ee/desktop/user/client/client.go @@ -99,7 +99,7 @@ func (c *client) DetectPresence(reason string, interval time.Duration) (time.Dur } func (c *client) CreateSecureEnclaveKey() (*ecdsa.PublicKey, error) { - resp, err := c.base.Get("http://unix/create_secure_enclave_key") + resp, err := c.base.Post("http://unix/secure_enclave_key", "application/json", http.NoBody) if err != nil { return nil, fmt.Errorf("getting secure enclave key: %w", err) } diff --git a/ee/desktop/user/server/server.go b/ee/desktop/user/server/server.go index 2dcbe71a7..80dd5bb69 100644 --- a/ee/desktop/user/server/server.go +++ b/ee/desktop/user/server/server.go @@ -67,7 +67,7 @@ func New(slogger *slog.Logger, authedMux.HandleFunc("/refresh", userServer.refreshHandler) authedMux.HandleFunc("/show", userServer.showDesktop) authedMux.HandleFunc("/detect_presence", userServer.detectPresence) - authedMux.HandleFunc("/create_secure_enclave_key", userServer.createSecureEnclaveKey) + authedMux.HandleFunc("/secure_enclave_key", userServer.createSecureEnclaveKey) userServer.server = &http.Server{ Handler: userServer.authMiddleware(authedMux), From 53f1bf14317d108573515fa45e2b9ff3bf803689 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Mon, 9 Dec 2024 15:02:52 -0800 Subject: [PATCH 17/26] add multiple interrupt tests --- ee/secureenclaverunner/secureenclaverunner.go | 7 +-- .../secureenclaverunner_test.go | 42 ++++++++++++++++- ee/tpmrunner/tpmrunner.go | 7 +-- ee/tpmrunner/tpmrunner_test.go | 46 ++++++++++++++++++- 4 files changed, 94 insertions(+), 8 deletions(-) diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index fec41a49d..32cb44f60 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -16,6 +16,7 @@ import ( "log/slog" "os/user" "sync" + "sync/atomic" "time" "github.com/kolide/launcher/ee/agent/types" @@ -35,7 +36,7 @@ type secureEnclaveRunner struct { slogger *slog.Logger mux *sync.Mutex interrupt chan struct{} - interrupted bool + interrupted atomic.Bool } type secureEnclaveClient interface { @@ -105,11 +106,11 @@ func (ser *secureEnclaveRunner) Execute() error { func (ser *secureEnclaveRunner) Interrupt(_ error) { // Only perform shutdown tasks on first call to interrupt -- no need to repeat on potential extra calls. - if ser.interrupted { + if ser.interrupted.Load() { return } - ser.interrupted = true + ser.interrupted.Store(true) // Tell the execute loop to stop checking, and exit ser.interrupt <- struct{}{} diff --git a/ee/secureenclaverunner/secureenclaverunner_test.go b/ee/secureenclaverunner/secureenclaverunner_test.go index 4d6e543a3..d1cf46033 100644 --- a/ee/secureenclaverunner/secureenclaverunner_test.go +++ b/ee/secureenclaverunner/secureenclaverunner_test.go @@ -29,7 +29,6 @@ func Test_secureEnclaveRunner(t *testing.T) { t.Parallel() secureEnclaveClientMock := mocks.NewSecureEnclaveClient(t) - secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(&privKey.PublicKey, nil).Once() ser, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), secureEnclaveClientMock) require.NoError(t, err) @@ -98,4 +97,45 @@ func Test_secureEnclaveRunner(t *testing.T) { require.NotNil(t, ser.Public()) }) + t.Run("multiple interrupts", func(t *testing.T) { + t.Parallel() + + secureEnclaveClientMock := mocks.NewSecureEnclaveClient(t) + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(&privKey.PublicKey, nil).Once() + + ser, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), secureEnclaveClientMock) + require.NoError(t, err) + + go func() { + ser.Execute() + }() + + // Confirm we can call Interrupt multiple times without blocking + interruptComplete := make(chan struct{}) + expectedInterrupts := 3 + for i := 0; i < expectedInterrupts; i += 1 { + go func() { + ser.Interrupt(nil) + interruptComplete <- struct{}{} + }() + } + + receivedInterrupts := 0 + for { + if receivedInterrupts >= expectedInterrupts { + break + } + + select { + case <-interruptComplete: + receivedInterrupts += 1 + continue + case <-time.After(5 * time.Second): + t.Errorf("could not call interrupt multiple times and return within 5 seconds -- received %d interrupts before timeout", receivedInterrupts) + t.FailNow() + } + } + + require.Equal(t, expectedInterrupts, receivedInterrupts) + }) } diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index d58c44e3f..6a45733f1 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "log/slog" + "sync/atomic" "time" "github.com/kolide/krypto/pkg/tpm" @@ -22,7 +23,7 @@ type ( store types.GetterSetterDeleter slogger *slog.Logger interrupt chan struct{} - interrupted bool + interrupted atomic.Bool } // tpmSignerCreator is an interface for creating and loading TPM signers @@ -99,11 +100,11 @@ func (tr *tpmRunner) Execute() error { func (tr *tpmRunner) Interrupt(_ error) { // Only perform shutdown tasks on first call to interrupt -- no need to repeat on potential extra calls. - if tr.interrupted { + if tr.interrupted.Load() { return } - tr.interrupted = true + tr.interrupted.Store(true) // Tell the execute loop to stop checking, and exit tr.interrupt <- struct{}{} diff --git a/ee/tpmrunner/tpmrunner_test.go b/ee/tpmrunner/tpmrunner_test.go index 8a569aeb6..edc3a87b9 100644 --- a/ee/tpmrunner/tpmrunner_test.go +++ b/ee/tpmrunner/tpmrunner_test.go @@ -48,7 +48,6 @@ func Test_tpmRunner(t *testing.T) { require.NoError(t, tpmRunner.Execute()) require.NotNil(t, tpmRunner.Public()) - }) t.Run("loads existing key", func(t *testing.T) { @@ -77,4 +76,49 @@ func Test_tpmRunner(t *testing.T) { require.NoError(t, tpmRunner.Execute()) require.NotNil(t, tpmRunner.Public()) }) + + t.Run("test multiple interrupts", func(t *testing.T) { + t.Parallel() + + tpmSignerCreatorMock := mocks.NewTpmSignerCreator(t) + tpmRunner, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), withTpmSignerCreator(tpmSignerCreatorMock)) + require.NoError(t, err) + + require.Nil(t, tpmRunner.Public()) + + tpmSignerCreatorMock.On("CreateKey").Return(fakePrivData, fakePubData, nil).Once() + tpmSignerCreatorMock.On("New", fakePrivData, fakePubData).Return(privKey, nil).Once() + + go func() { + tpmRunner.Execute() + }() + + // Confirm we can call Interrupt multiple times without blocking + interruptComplete := make(chan struct{}) + expectedInterrupts := 3 + for i := 0; i < expectedInterrupts; i += 1 { + go func() { + tpmRunner.Interrupt(nil) + interruptComplete <- struct{}{} + }() + } + + receivedInterrupts := 0 + for { + if receivedInterrupts >= expectedInterrupts { + break + } + + select { + case <-interruptComplete: + receivedInterrupts += 1 + continue + case <-time.After(5 * time.Second): + t.Errorf("could not call interrupt multiple times and return within 5 seconds -- received %d interrupts before timeout", receivedInterrupts) + t.FailNow() + } + } + + require.Equal(t, expectedInterrupts, receivedInterrupts) + }) } From ed176a34d806aa02fabf92a0d83b2bdb052d30bd Mon Sep 17 00:00:00 2001 From: James Pickett Date: Wed, 11 Dec 2024 15:07:33 -0800 Subject: [PATCH 18/26] update tpm runner to also support on demand key creation --- ee/tpmrunner/tpmrunner.go | 53 +++++++++++++++++++++++----------- ee/tpmrunner/tpmrunner_test.go | 8 +++-- 2 files changed, 41 insertions(+), 20 deletions(-) diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index 6a45733f1..02190c105 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "log/slog" + "sync" "sync/atomic" "time" @@ -19,6 +20,7 @@ import ( type ( tpmRunner struct { signer crypto.Signer + mux sync.Mutex signerCreator tpmSignerCreator store types.GetterSetterDeleter slogger *slog.Logger @@ -74,15 +76,18 @@ func (tr *tpmRunner) Execute() error { defer retryTicker.Stop() for { - ctx := context.Background() - signer, err := tr.loadOrCreateKeys(ctx) - if err != nil { - tr.slogger.Log(ctx, slog.LevelError, - "creating tpm signer, will retry", - "err", err, - ) - } else { - tr.signer = signer + // try to create signer if we don't have one + if tr.signer == nil { + ctx := context.Background() + if err := tr.loadOrCreateKeys(ctx); err != nil { + tr.slogger.Log(ctx, slog.LevelError, + "loading or creating keys in execute loop", + "err", err, + ) + } + } + + if tr.signer != nil { retryTicker.Stop() } @@ -90,7 +95,7 @@ func (tr *tpmRunner) Execute() error { case <-retryTicker.C: continue case <-tr.interrupt: - tr.slogger.Log(ctx, slog.LevelDebug, + tr.slogger.Log(context.TODO(), slog.LevelDebug, "interrupt received, exiting secure enclave signer execute loop", ) return nil @@ -112,7 +117,16 @@ func (tr *tpmRunner) Interrupt(_ error) { // Public returns the public hardware key func (tr *tpmRunner) Public() crypto.PublicKey { - if tr.signer == nil { + if tr.signer != nil { + return tr.signer.Public() + } + + if err := tr.loadOrCreateKeys(context.Background()); err != nil { + tr.slogger.Log(context.Background(), slog.LevelError, + "loading or creating keys in public call", + "err", err, + ) + return nil } @@ -178,7 +192,10 @@ func clearKeyData(slogger *slog.Logger, deleter types.Deleter) { _ = deleter.Delete([]byte(privateEccData), []byte(publicEccData)) } -func (tr *tpmRunner) loadOrCreateKeys(ctx context.Context) (crypto.Signer, error) { +func (tr *tpmRunner) loadOrCreateKeys(ctx context.Context) error { + tr.mux.Lock() + defer tr.mux.Unlock() + ctx, span := traces.StartSpan(ctx) defer span.End() @@ -186,7 +203,7 @@ func (tr *tpmRunner) loadOrCreateKeys(ctx context.Context) (crypto.Signer, error if err != nil { thisErr := fmt.Errorf("fetching key data for data store: %w", err) traces.SetError(span, thisErr) - return nil, thisErr + return thisErr } if pubData == nil || priData == nil { @@ -201,7 +218,7 @@ func (tr *tpmRunner) loadOrCreateKeys(ctx context.Context) (crypto.Signer, error traces.SetError(span, thisErr) clearKeyData(tr.slogger, tr.store) - return nil, thisErr + return thisErr } if err := storeKeyData(tr.store, priData, pubData); err != nil { @@ -209,7 +226,7 @@ func (tr *tpmRunner) loadOrCreateKeys(ctx context.Context) (crypto.Signer, error traces.SetError(span, thisErr) clearKeyData(tr.slogger, tr.store) - return nil, thisErr + return thisErr } span.AddEvent("generated_new_tpm_keys") @@ -219,10 +236,12 @@ func (tr *tpmRunner) loadOrCreateKeys(ctx context.Context) (crypto.Signer, error if err != nil { thisErr := fmt.Errorf("creating tpm signer: %w", err) traces.SetError(span, thisErr) - return nil, thisErr + return thisErr } + tr.signer = k + span.AddEvent("created_tpm_signer") - return k, nil + return nil } diff --git a/ee/tpmrunner/tpmrunner_test.go b/ee/tpmrunner/tpmrunner_test.go index edc3a87b9..4f7257a34 100644 --- a/ee/tpmrunner/tpmrunner_test.go +++ b/ee/tpmrunner/tpmrunner_test.go @@ -34,6 +34,7 @@ func Test_tpmRunner(t *testing.T) { tpmRunner, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), withTpmSignerCreator(tpmSignerCreatorMock)) require.NoError(t, err) + tpmSignerCreatorMock.On("CreateKey").Return(nil, nil, errors.New("not available yet")).Once() require.Nil(t, tpmRunner.Public()) tpmSignerCreatorMock.On("CreateKey").Return(nil, nil, errors.New("not available yet")).Once() @@ -62,11 +63,11 @@ func Test_tpmRunner(t *testing.T) { tpmRunner, err := New(context.TODO(), multislogger.NewNopLogger(), store, withTpmSignerCreator(tpmSignerCreatorMock)) require.NoError(t, err) - // public will be nil until execute is called and key is loaded form tpm - require.Nil(t, tpmRunner.Public()) - tpmSignerCreatorMock.On("New", fakePrivData, fakePubData).Return(privKey, nil).Once() + // the call to public should load the key from the store and signer creator should not be called any more after + require.NotNil(t, tpmRunner.Public()) + go func() { // sleep long enough to get through 2 cycles of exectue time.Sleep(3 * time.Second) @@ -84,6 +85,7 @@ func Test_tpmRunner(t *testing.T) { tpmRunner, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), withTpmSignerCreator(tpmSignerCreatorMock)) require.NoError(t, err) + tpmSignerCreatorMock.On("CreateKey").Return(nil, nil, errors.New("not available yet")).Once() require.Nil(t, tpmRunner.Public()) tpmSignerCreatorMock.On("CreateKey").Return(fakePrivData, fakePubData, nil).Once() From 788500196ab794d665936c5108780686a94c5548 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Wed, 11 Dec 2024 15:09:02 -0800 Subject: [PATCH 19/26] start span before mutex lock --- ee/tpmrunner/tpmrunner.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index 02190c105..ec4befddc 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -193,12 +193,12 @@ func clearKeyData(slogger *slog.Logger, deleter types.Deleter) { } func (tr *tpmRunner) loadOrCreateKeys(ctx context.Context) error { - tr.mux.Lock() - defer tr.mux.Unlock() - ctx, span := traces.StartSpan(ctx) defer span.End() + tr.mux.Lock() + defer tr.mux.Unlock() + priData, pubData, err := fetchKeyData(tr.store) if err != nil { thisErr := fmt.Errorf("fetching key data for data store: %w", err) From 905bf826eaead56d3de294731ac6bf39db74d1b9 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Thu, 12 Dec 2024 09:27:49 -0800 Subject: [PATCH 20/26] handle case in execute where no console users, wait longer --- ee/secureenclaverunner/secureenclaverunner.go | 29 ++++++- .../secureenclaverunner_test.go | 84 +++++++++++++++++-- 2 files changed, 106 insertions(+), 7 deletions(-) diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index 32cb44f60..4573f93b7 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -37,6 +37,7 @@ type secureEnclaveRunner struct { mux *sync.Mutex interrupt chan struct{} interrupted atomic.Bool + noConsoleUsersDelay time.Duration } type secureEnclaveClient interface { @@ -51,6 +52,7 @@ func New(_ context.Context, slogger *slog.Logger, store types.GetterSetterDelete slogger: slogger.With("component", "secureenclaverunner"), mux: &sync.Mutex{}, interrupt: make(chan struct{}), + noConsoleUsersDelay: 15 * time.Second, }, nil } @@ -78,7 +80,8 @@ func (ser *secureEnclaveRunner) Execute() error { } } - retryTicker := backoff.NewMultiplicativeTicker(time.Second, time.Minute) + tickerBaseDuration, tickerMaxDuraiton := time.Second, time.Minute + retryTicker := backoff.NewMultiplicativeTicker(tickerBaseDuration, tickerMaxDuraiton) defer retryTicker.Stop() for { @@ -88,6 +91,22 @@ func (ser *secureEnclaveRunner) Execute() error { "getting current console user key, will retry", "err", err, ) + + // if we don't have a console user, wait a little longer then reset the ticker + // and start trying again + if errors.Is(err, noConsoleUsersError{}) { + retryTicker.Stop() + + select { + case <-time.After(ser.noConsoleUsersDelay): + retryTicker = backoff.NewMultiplicativeTicker(tickerBaseDuration, tickerMaxDuraiton) + case <-ser.interrupt: + ser.slogger.Log(context.TODO(), slog.LevelDebug, + "interrupt received, exiting secure enclave signer execute loop", + ) + return nil + } + } } else { retryTicker.Stop() } @@ -239,6 +258,12 @@ func (ser *secureEnclaveRunner) UnmarshalJSON(data []byte) error { return nil } +type noConsoleUsersError struct{} + +func (noConsoleUsersError) Error() string { + return "no console users found" +} + func firstConsoleUser(ctx context.Context) (*user.User, error) { ctx, span := traces.StartSpan(ctx) defer span.End() @@ -251,7 +276,7 @@ func firstConsoleUser(ctx context.Context) (*user.User, error) { if len(c) == 0 { traces.SetError(span, errors.New("no console users found")) - return nil, errors.New("no console users found") + return nil, noConsoleUsersError{} } return c[0], nil diff --git a/ee/secureenclaverunner/secureenclaverunner_test.go b/ee/secureenclaverunner/secureenclaverunner_test.go index d1cf46033..ee84d118e 100644 --- a/ee/secureenclaverunner/secureenclaverunner_test.go +++ b/ee/secureenclaverunner/secureenclaverunner_test.go @@ -25,7 +25,7 @@ func Test_secureEnclaveRunner(t *testing.T) { privKey, err := echelper.GenerateEcdsaKey() require.NoError(t, err) - t.Run("creates key in new", func(t *testing.T) { + t.Run("creates key in public call", func(t *testing.T) { t.Parallel() secureEnclaveClientMock := mocks.NewSecureEnclaveClient(t) @@ -33,6 +33,12 @@ func Test_secureEnclaveRunner(t *testing.T) { ser, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), secureEnclaveClientMock) require.NoError(t, err) require.NotNil(t, ser.Public()) + + // key should have been created in public call + require.Len(t, ser.uidPubKeyMap, 1) + for _, v := range ser.uidPubKeyMap { + require.Equal(t, &privKey.PublicKey, v) + } }) t.Run("creates key in execute", func(t *testing.T) { @@ -59,9 +65,12 @@ func Test_secureEnclaveRunner(t *testing.T) { }() require.NoError(t, ser.Execute()) - // one cycle of execute should have created key - require.NotNil(t, ser.Public()) + // key should have been created in execute + require.Len(t, ser.uidPubKeyMap, 1) + for _, v := range ser.uidPubKeyMap { + require.Equal(t, &privKey.PublicKey, v) + } }) t.Run("loads existing key", func(t *testing.T) { @@ -93,8 +102,11 @@ func Test_secureEnclaveRunner(t *testing.T) { require.NoError(t, ser.Execute()) - // should be able to fetch the key - require.NotNil(t, ser.Public()) + // key should have been loaded in execute + require.Len(t, ser.uidPubKeyMap, 1) + for _, v := range ser.uidPubKeyMap { + require.Equal(t, &privKey.PublicKey, v) + } }) t.Run("multiple interrupts", func(t *testing.T) { @@ -138,4 +150,66 @@ func Test_secureEnclaveRunner(t *testing.T) { require.Equal(t, expectedInterrupts, receivedInterrupts) }) + + t.Run("no console users, creates key", func(t *testing.T) { + t.Parallel() + + secureEnclaveClientMock := mocks.NewSecureEnclaveClient(t) + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(nil, errors.New("not available yet")).Once() + ser, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), secureEnclaveClientMock) + require.NoError(t, err) + + // iniital key should be nil since client wasn't ready + require.Nil(t, ser.Public()) + + // set delay to 100ms for testing + ser.noConsoleUsersDelay = 100 * time.Millisecond + + // give error on first execute loop + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(nil, noConsoleUsersError{}).Once() + + // give key on second execute loop + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(&privKey.PublicKey, nil).Once() + + go func() { + // sleep long enough to get through 2 cycles of exectue + time.Sleep(3 * time.Second) + ser.Interrupt(errors.New("test")) + }() + + require.NoError(t, ser.Execute()) + + // key should have been loaded in execute + require.Len(t, ser.uidPubKeyMap, 1) + for _, v := range ser.uidPubKeyMap { + require.Equal(t, &privKey.PublicKey, v) + } + }) + + t.Run("no console users, handles interrupt", func(t *testing.T) { + t.Parallel() + + secureEnclaveClientMock := mocks.NewSecureEnclaveClient(t) + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(nil, errors.New("not available yet")).Once() + ser, err := New(context.TODO(), multislogger.NewNopLogger(), inmemory.NewStore(), secureEnclaveClientMock) + require.NoError(t, err) + + // iniital key should be nil since client wasn't ready + require.Nil(t, ser.Public()) + + // give error on first execute loop + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(nil, noConsoleUsersError{}).Once() + + go func() { + // sleep long enough to get through 2 cycles of exectue + time.Sleep(3 * time.Second) + ser.Interrupt(errors.New("test")) + }() + + require.NoError(t, ser.Execute()) + + // no key should be created since loop didn't execute + // and public not called + require.Len(t, ser.uidPubKeyMap, 0) + }) } From 14c06718b76d56a44833a94075c19a0117131977 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Thu, 12 Dec 2024 09:29:25 -0800 Subject: [PATCH 21/26] spelling --- ee/agent/keys_darwin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/agent/keys_darwin.go b/ee/agent/keys_darwin.go index d30b0f996..9d5394795 100644 --- a/ee/agent/keys_darwin.go +++ b/ee/agent/keys_darwin.go @@ -13,7 +13,7 @@ import ( "github.com/kolide/launcher/pkg/traces" ) -// SetHardwareKeysRunner creates a secure enclave runner and sets it as the agent hardware key as it also implements the keyInt/cyrpto.Signer interface. +// SetHardwareKeysRunner creates a secure enclave runner and sets it as the agent hardware key as it also implements the keyInt/crypto.Signer interface. // The returned execute and interrupt functions can be used to start and stop the secure enclave runner, generally via a run group. func SetHardwareKeysRunner(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter, secureEnclaveClient secureEnclaveClient) (execute func() error, interrupt func(error), err error) { ctx, span := traces.StartSpan(ctx) From 84a3a2c6d3bf167d123ce22e189ebf5be152db10 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Fri, 13 Dec 2024 12:19:08 -0800 Subject: [PATCH 22/26] give up on custom ticker, reset ticker in execute to back off --- ee/secureenclaverunner/secureenclaverunner.go | 25 +++--- ee/tpmrunner/tpmrunner.go | 4 +- pkg/backoff/counter.go | 34 ++++++++ .../{ticker_test.go => counter_test.go} | 47 ++--------- pkg/backoff/ticker.go | 77 ------------------- 5 files changed, 57 insertions(+), 130 deletions(-) create mode 100644 pkg/backoff/counter.go rename pkg/backoff/{ticker_test.go => counter_test.go} (58%) delete mode 100644 pkg/backoff/ticker.go diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index 4573f93b7..3fb3cc646 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -80,10 +80,13 @@ func (ser *secureEnclaveRunner) Execute() error { } } - tickerBaseDuration, tickerMaxDuraiton := time.Second, time.Minute - retryTicker := backoff.NewMultiplicativeTicker(tickerBaseDuration, tickerMaxDuraiton) + durationCounter := backoff.NewMultiplicativeDurationCounter(time.Second, time.Minute) + retryTicker := time.NewTicker(durationCounter.Next()) defer retryTicker.Stop() + noConsoleUserTicker := time.NewTicker(ser.noConsoleUsersDelay) + defer noConsoleUserTicker.Stop() + for { ctx := context.Background() if _, err := ser.currentConsoleUserKey(ctx); err != nil { @@ -96,23 +99,21 @@ func (ser *secureEnclaveRunner) Execute() error { // and start trying again if errors.Is(err, noConsoleUsersError{}) { retryTicker.Stop() - - select { - case <-time.After(ser.noConsoleUsersDelay): - retryTicker = backoff.NewMultiplicativeTicker(tickerBaseDuration, tickerMaxDuraiton) - case <-ser.interrupt: - ser.slogger.Log(context.TODO(), slog.LevelDebug, - "interrupt received, exiting secure enclave signer execute loop", - ) - return nil - } + } else { + noConsoleUserTicker.Stop() + durationCounter.Reset() + retryTicker.Reset(durationCounter.Next()) } } else { retryTicker.Stop() + noConsoleUserTicker.Stop() } select { case <-retryTicker.C: + retryTicker.Reset(durationCounter.Next()) + continue + case <-noConsoleUserTicker.C: continue case <-ser.interrupt: ser.slogger.Log(ctx, slog.LevelDebug, diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index ec4befddc..755a0ec0a 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -72,7 +72,8 @@ func New(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDele // Public returns the public key of the current console user // creating and peristing a new one if needed func (tr *tpmRunner) Execute() error { - retryTicker := backoff.NewMultiplicativeTicker(time.Second, time.Minute) + durationCounter := backoff.NewMultiplicativeDurationCounter(time.Second, time.Minute) + retryTicker := time.NewTicker(durationCounter.Next()) defer retryTicker.Stop() for { @@ -93,6 +94,7 @@ func (tr *tpmRunner) Execute() error { select { case <-retryTicker.C: + retryTicker.Reset(durationCounter.Next()) continue case <-tr.interrupt: tr.slogger.Log(context.TODO(), slog.LevelDebug, diff --git a/pkg/backoff/counter.go b/pkg/backoff/counter.go new file mode 100644 index 000000000..497205654 --- /dev/null +++ b/pkg/backoff/counter.go @@ -0,0 +1,34 @@ +package backoff + +import ( + "time" +) + +type durationCounter struct { + count int + baseInterval, maxInterval time.Duration + calcNext func(count int, baseDuration time.Duration) time.Duration +} + +func (dc *durationCounter) Next() time.Duration { + dc.count++ + interval := dc.calcNext(dc.count, dc.baseInterval) + if interval > dc.maxInterval { + return dc.maxInterval + } + return interval +} + +func (dc *durationCounter) Reset() { + dc.count = 0 +} + +func NewMultiplicativeDurationCounter(baseDuration, maxDuration time.Duration) *durationCounter { + return &durationCounter{ + baseInterval: baseDuration, + maxInterval: maxDuration, + calcNext: func(count int, baseInterval time.Duration) time.Duration { + return baseInterval * time.Duration(count) + }, + } +} diff --git a/pkg/backoff/ticker_test.go b/pkg/backoff/counter_test.go similarity index 58% rename from pkg/backoff/ticker_test.go rename to pkg/backoff/counter_test.go index 6c2aed950..b64c5463f 100644 --- a/pkg/backoff/ticker_test.go +++ b/pkg/backoff/counter_test.go @@ -59,49 +59,16 @@ func TestMultiplicativeCounter(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - ec := newMultiplicativeCounter(tt.baseInterval, tt.maxInterval) + ec := NewMultiplicativeDurationCounter(tt.baseInterval, tt.maxInterval) for _, expected := range tt.expected { - require.Equal(t, expected, ec.next()) + require.Equal(t, expected, ec.Next()) } - }) - } -} - -// TestMultiplicativeTicker tests the NewMultiplicativeTicker and its behavior. -func TestMultiplicativeTicker(t *testing.T) { - t.Parallel() - - baseTime := 100 * time.Millisecond - maxTime := 500 * time.Millisecond - tk := NewMultiplicativeTicker(baseTime, maxTime) - defer tk.Stop() - - expectedDurations := []time.Duration{ - 100 * time.Millisecond, - 200 * time.Millisecond, - 300 * time.Millisecond, - 400 * time.Millisecond, - 500 * time.Millisecond, // maxTime limit - 500 * time.Millisecond, // maxTime limit - } + ec.Reset() - buffer := 50 * time.Millisecond - - for _, expected := range expectedDurations { - start := time.Now() - - select { - case <-tk.C: - require.WithinDuration(t, start, time.Now(), expected+buffer) - case <-time.After(maxTime + buffer): - t.Fatalf("ticker did not send event in expected time: %v", expected) - } + for _, expected := range tt.expected { + require.Equal(t, expected, ec.Next()) + } + }) } - - // stop the ticker - tk.Stop() - - // call stop again to make sure no panic (same as stdlib ticker) - tk.Stop() } diff --git a/pkg/backoff/ticker.go b/pkg/backoff/ticker.go deleted file mode 100644 index d0333e00e..000000000 --- a/pkg/backoff/ticker.go +++ /dev/null @@ -1,77 +0,0 @@ -package backoff - -import ( - "time" -) - -// NewMultiplicativeTicker returns a ticker where each interval = baseDuration * ticks until maxDuration is reached. -func NewMultiplicativeTicker(baseDuration, maxDuration time.Duration) *ticker { - return newTicker(newMultiplicativeCounter(baseDuration, maxDuration)) -} - -type ticker struct { - C chan time.Time - baseTicker *time.Ticker - stoppedChan chan struct{} - stopped bool - durationCounter durationCounter -} - -func newTicker(durationCounter *durationCounter) *ticker { - thisTicker := &ticker{ - C: make(chan time.Time), - stoppedChan: make(chan struct{}), - durationCounter: *durationCounter, - } - - thisTicker.baseTicker = time.NewTicker(thisTicker.durationCounter.next()) - - go func() { - for { - select { - case t := <-thisTicker.baseTicker.C: - thisTicker.baseTicker.Reset(thisTicker.durationCounter.next()) - thisTicker.C <- t - case <-thisTicker.stoppedChan: - thisTicker.baseTicker.Stop() - return - } - } - }() - - return thisTicker -} - -func (t *ticker) Stop() { - if t.stopped { - return - } - - t.stopped = true - close(t.stoppedChan) -} - -type durationCounter struct { - count int - baseInterval, maxInterval time.Duration - calcNext func(count int, baseDuration time.Duration) time.Duration -} - -func (dc *durationCounter) next() time.Duration { - dc.count++ - interval := dc.calcNext(dc.count, dc.baseInterval) - if interval > dc.maxInterval { - return dc.maxInterval - } - return interval -} - -func newMultiplicativeCounter(baseDuration, maxDuration time.Duration) *durationCounter { - return &durationCounter{ - baseInterval: baseDuration, - maxInterval: maxDuration, - calcNext: func(count int, baseInterval time.Duration) time.Duration { - return baseInterval * time.Duration(count) - }, - } -} From 6c819bcd91284f6a8140d50aac68c5a063595cff Mon Sep 17 00:00:00 2001 From: James Pickett Date: Fri, 13 Dec 2024 12:25:00 -0800 Subject: [PATCH 23/26] comments --- pkg/backoff/counter.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/backoff/counter.go b/pkg/backoff/counter.go index 497205654..6cb4915ca 100644 --- a/pkg/backoff/counter.go +++ b/pkg/backoff/counter.go @@ -10,6 +10,8 @@ type durationCounter struct { calcNext func(count int, baseDuration time.Duration) time.Duration } +// Next increments the count and returns the base interval multiplied by the count. +// If the result is greater than the maxDuration, maxDuration is returned. func (dc *durationCounter) Next() time.Duration { dc.count++ interval := dc.calcNext(dc.count, dc.baseInterval) @@ -19,10 +21,14 @@ func (dc *durationCounter) Next() time.Duration { return interval } +// Reset resets the count to 0. func (dc *durationCounter) Reset() { dc.count = 0 } +// NewMultiplicativeDurationCounter creates a new durationCounter that multiplies the base interval by the count. +// Count is incremented each time Next() is called and returns the base interval multiplied by the count. +// If the result is greater than the maxDuration, maxDuration is returned. func NewMultiplicativeDurationCounter(baseDuration, maxDuration time.Duration) *durationCounter { return &durationCounter{ baseInterval: baseDuration, From 9457974c663b4bc048fd9933a47c2c543becd048 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Fri, 13 Dec 2024 12:56:35 -0800 Subject: [PATCH 24/26] make secure enclave loop more readable --- ee/secureenclaverunner/secureenclaverunner.go | 44 +++++++++---------- .../secureenclaverunner_test.go | 5 ++- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index 3fb3cc646..9df321424 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -81,39 +81,39 @@ func (ser *secureEnclaveRunner) Execute() error { } durationCounter := backoff.NewMultiplicativeDurationCounter(time.Second, time.Minute) - retryTicker := time.NewTicker(durationCounter.Next()) + retryTicker := time.NewTicker(time.Second) defer retryTicker.Stop() - noConsoleUserTicker := time.NewTicker(ser.noConsoleUsersDelay) - defer noConsoleUserTicker.Stop() + inNoConsoleUsersState := false for { ctx := context.Background() - if _, err := ser.currentConsoleUserKey(ctx); err != nil { - ser.slogger.Log(ctx, slog.LevelError, - "getting current console user key, will retry", - "err", err, - ) + _, err := ser.currentConsoleUserKey(ctx) - // if we don't have a console user, wait a little longer then reset the ticker - // and start trying again - if errors.Is(err, noConsoleUsersError{}) { - retryTicker.Stop() - } else { - noConsoleUserTicker.Stop() - durationCounter.Reset() - retryTicker.Reset(durationCounter.Next()) - } - } else { + switch { + + // don't have console user, so wait longer to retry + case errors.Is(err, noConsoleUsersError{}): + inNoConsoleUsersState = true + retryTicker.Reset(ser.noConsoleUsersDelay) + + // now that we have a console user, restart the backoff + case err != nil && inNoConsoleUsersState: + durationCounter.Reset() + retryTicker.Reset(durationCounter.Next()) + inNoConsoleUsersState = false + + // we have console user, but failed to get key + case err != nil: + retryTicker.Reset(durationCounter.Next()) + + // success + default: retryTicker.Stop() - noConsoleUserTicker.Stop() } select { case <-retryTicker.C: - retryTicker.Reset(durationCounter.Next()) - continue - case <-noConsoleUserTicker.C: continue case <-ser.interrupt: ser.slogger.Log(ctx, slog.LevelDebug, diff --git a/ee/secureenclaverunner/secureenclaverunner_test.go b/ee/secureenclaverunner/secureenclaverunner_test.go index ee84d118e..6f5783158 100644 --- a/ee/secureenclaverunner/secureenclaverunner_test.go +++ b/ee/secureenclaverunner/secureenclaverunner_test.go @@ -151,7 +151,7 @@ func Test_secureEnclaveRunner(t *testing.T) { require.Equal(t, expectedInterrupts, receivedInterrupts) }) - t.Run("no console users, creates key", func(t *testing.T) { + t.Run("no console users then creates key", func(t *testing.T) { t.Parallel() secureEnclaveClientMock := mocks.NewSecureEnclaveClient(t) @@ -168,6 +168,9 @@ func Test_secureEnclaveRunner(t *testing.T) { // give error on first execute loop secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(nil, noConsoleUsersError{}).Once() + // give error on first execute loop + secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(nil, errors.New("some other error")).Once() + // give key on second execute loop secureEnclaveClientMock.On("CreateSecureEnclaveKey", mock.AnythingOfType("string")).Return(&privKey.PublicKey, nil).Once() From c8cceea85614e2d07403e9be2673587669b32ea9 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Fri, 13 Dec 2024 13:00:24 -0800 Subject: [PATCH 25/26] log errors --- ee/secureenclaverunner/secureenclaverunner.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index 9df321424..ee6670edf 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -112,6 +112,14 @@ func (ser *secureEnclaveRunner) Execute() error { retryTicker.Stop() } + // log any errors + if err != nil { + ser.slogger.Log(ctx, slog.LevelDebug, + "getting current console user key", + "err", err, + ) + } + select { case <-retryTicker.C: continue From d1f815897d696444ec0301a052feebe807f12dae Mon Sep 17 00:00:00 2001 From: James Pickett Date: Wed, 18 Dec 2024 15:22:56 -0800 Subject: [PATCH 26/26] feedback --- ee/desktop/runner/runner.go | 13 ++++++++++++- ee/desktop/user/client/client.go | 11 ++--------- ee/secureenclaverunner/secureenclaverunner.go | 8 ++++---- ee/tpmrunner/tpmrunner.go | 1 - 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/ee/desktop/runner/runner.go b/ee/desktop/runner/runner.go index 4014b2ab8..9b82fcb9f 100644 --- a/ee/desktop/runner/runner.go +++ b/ee/desktop/runner/runner.go @@ -22,6 +22,7 @@ import ( "github.com/kolide/kit/ulid" "github.com/kolide/kit/version" + "github.com/kolide/krypto/pkg/echelper" "github.com/kolide/launcher/ee/agent" "github.com/kolide/launcher/ee/agent/flags/keys" "github.com/kolide/launcher/ee/agent/types" @@ -318,7 +319,17 @@ func (r *DesktopUsersProcessesRunner) CreateSecureEnclaveKey(uid string) (*ecdsa } client := client.New(r.userServerAuthToken, proc.socketPath) - return client.CreateSecureEnclaveKey() + keyBytes, err := client.CreateSecureEnclaveKey() + if err != nil { + return nil, fmt.Errorf("creating secure enclave key: %w", err) + } + + key, err := echelper.PublicB64DerToEcdsaKey(keyBytes) + if err != nil { + return nil, fmt.Errorf("converting key bytes to ecdsa key: %w", err) + } + + return key, nil } // killDesktopProcesses kills any existing desktop processes diff --git a/ee/desktop/user/client/client.go b/ee/desktop/user/client/client.go index c08452350..17a33a715 100644 --- a/ee/desktop/user/client/client.go +++ b/ee/desktop/user/client/client.go @@ -3,7 +3,6 @@ package client import ( "bytes" "context" - "crypto/ecdsa" "encoding/json" "errors" "fmt" @@ -12,7 +11,6 @@ import ( "net/url" "time" - "github.com/kolide/krypto/pkg/echelper" "github.com/kolide/launcher/ee/desktop/user/notify" "github.com/kolide/launcher/ee/desktop/user/server" "github.com/kolide/launcher/ee/presencedetection" @@ -98,7 +96,7 @@ func (c *client) DetectPresence(reason string, interval time.Duration) (time.Dur return durationSinceLastDetection, detectionErr } -func (c *client) CreateSecureEnclaveKey() (*ecdsa.PublicKey, error) { +func (c *client) CreateSecureEnclaveKey() ([]byte, error) { resp, err := c.base.Post("http://unix/secure_enclave_key", "application/json", http.NoBody) if err != nil { return nil, fmt.Errorf("getting secure enclave key: %w", err) @@ -119,12 +117,7 @@ func (c *client) CreateSecureEnclaveKey() (*ecdsa.PublicKey, error) { return nil, fmt.Errorf("reading response body: %w", err) } - key, err := echelper.PublicB64DerToEcdsaKey(body) - if err != nil { - return nil, fmt.Errorf("converting key: %w", err) - } - - return key, nil + return body, nil } func (c *client) Notify(n notify.Notification) error { diff --git a/ee/secureenclaverunner/secureenclaverunner.go b/ee/secureenclaverunner/secureenclaverunner.go index ee6670edf..b928eb02c 100644 --- a/ee/secureenclaverunner/secureenclaverunner.go +++ b/ee/secureenclaverunner/secureenclaverunner.go @@ -31,10 +31,10 @@ const ( type secureEnclaveRunner struct { uidPubKeyMap map[string]*ecdsa.PublicKey + uidPubKeyMapMux *sync.Mutex secureEnclaveClient secureEnclaveClient store types.GetterSetterDeleter slogger *slog.Logger - mux *sync.Mutex interrupt chan struct{} interrupted atomic.Bool noConsoleUsersDelay time.Duration @@ -50,7 +50,7 @@ func New(_ context.Context, slogger *slog.Logger, store types.GetterSetterDelete store: store, secureEnclaveClient: secureEnclaveClient, slogger: slogger.With("component", "secureenclaverunner"), - mux: &sync.Mutex{}, + uidPubKeyMapMux: &sync.Mutex{}, interrupt: make(chan struct{}), noConsoleUsersDelay: 15 * time.Second, }, nil @@ -181,8 +181,8 @@ func (ser *secureEnclaveRunner) currentConsoleUserKey(ctx context.Context) (*ecd ctx, span := traces.StartSpan(ctx) defer span.End() - ser.mux.Lock() - defer ser.mux.Unlock() + ser.uidPubKeyMapMux.Lock() + defer ser.uidPubKeyMapMux.Unlock() cu, err := firstConsoleUser(ctx) if err != nil { diff --git a/ee/tpmrunner/tpmrunner.go b/ee/tpmrunner/tpmrunner.go index 755a0ec0a..0b509df44 100644 --- a/ee/tpmrunner/tpmrunner.go +++ b/ee/tpmrunner/tpmrunner.go @@ -186,7 +186,6 @@ func storeKeyData(store types.Setter, pri, pub []byte) error { // clearKeyData is used to clear the keys as part of error handling around new keys. It is not intended to be called // regularly, and since the path that calls it is around DB errors, it has no error handling. -// nolint:unused func clearKeyData(slogger *slog.Logger, deleter types.Deleter) { slogger.Log(context.TODO(), slog.LevelInfo, "clearing keys",