From af13166d3455946fe1d12e49a4fc293fcdcb6eb1 Mon Sep 17 00:00:00 2001 From: RebeccaMahany Date: Fri, 24 Jan 2025 14:58:26 -0500 Subject: [PATCH 1/3] Don't wait for hardware change detection to start up launcher --- cmd/launcher/launcher.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/launcher/launcher.go b/cmd/launcher/launcher.go index 9c81fde78..ac283abd0 100644 --- a/cmd/launcher/launcher.go +++ b/cmd/launcher/launcher.go @@ -327,8 +327,9 @@ func runLauncher(ctx context.Context, cancel func(), multiSlogger, systemMultiSl signalListener := newSignalListener(sigChannel, cancel, slogger) runGroup.Add("sigChannel", signalListener.Execute, signalListener.Interrupt) - // For now, remediation is not performed -- we only log the hardware change. - agent.DetectAndRemediateHardwareChange(ctx, k) + // For now, remediation is not performed -- we only log the hardware change. So we can + // perform this operation in the background to avoid slowing down launcher startup. + go agent.DetectAndRemediateHardwareChange(ctx, k) powerEventSubscriber := powereventwatcher.NewKnapsackSleepStateUpdater(slogger, k) powerEventWatcher, err := powereventwatcher.New(ctx, slogger, powerEventSubscriber) From 0da640e4086a82a10e0cf35fd3fabe607d1bf783 Mon Sep 17 00:00:00 2001 From: RebeccaMahany Date: Fri, 24 Jan 2025 15:02:33 -0500 Subject: [PATCH 2/3] Add additional spans for bbolt.MakeStores --- ee/agent/storage/bbolt/keyvalue_store_bbolt.go | 6 +++++- ee/agent/storage/bbolt/stores_bbolt.go | 4 ++-- ee/agent/storage/ci/keyvalue_store_ci.go | 3 ++- ee/agent/storage/ci/keyvalue_store_test.go | 3 ++- ee/agent/storage/ci/stores_ci.go | 3 ++- pkg/osquery/extension_test.go | 2 +- 6 files changed, 14 insertions(+), 7 deletions(-) diff --git a/ee/agent/storage/bbolt/keyvalue_store_bbolt.go b/ee/agent/storage/bbolt/keyvalue_store_bbolt.go index 7c8924ceb..467b7debf 100644 --- a/ee/agent/storage/bbolt/keyvalue_store_bbolt.go +++ b/ee/agent/storage/bbolt/keyvalue_store_bbolt.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" + "github.com/kolide/launcher/pkg/traces" "go.etcd.io/bbolt" ) @@ -34,7 +35,10 @@ type bboltKeyValueStore struct { bucketName string } -func NewStore(slogger *slog.Logger, db *bbolt.DB, bucketName string) (*bboltKeyValueStore, error) { +func NewStore(ctx context.Context, slogger *slog.Logger, db *bbolt.DB, bucketName string) (*bboltKeyValueStore, error) { + _, span := traces.StartSpan(ctx, "bucket_name", bucketName) + defer span.End() + err := db.Update(func(tx *bbolt.Tx) error { _, err := tx.CreateBucketIfNotExists([]byte(bucketName)) if err != nil { diff --git a/ee/agent/storage/bbolt/stores_bbolt.go b/ee/agent/storage/bbolt/stores_bbolt.go index d1e56caf1..917f0cfa4 100644 --- a/ee/agent/storage/bbolt/stores_bbolt.go +++ b/ee/agent/storage/bbolt/stores_bbolt.go @@ -13,7 +13,7 @@ import ( // MakeStores creates all the KVStores used by launcher func MakeStores(ctx context.Context, slogger *slog.Logger, db *bbolt.DB) (map[storage.Store]types.KVStore, error) { - _, span := traces.StartSpan(ctx) + ctx, span := traces.StartSpan(ctx) defer span.End() stores := make(map[storage.Store]types.KVStore) @@ -37,7 +37,7 @@ func MakeStores(ctx context.Context, slogger *slog.Logger, db *bbolt.DB) (map[st } for _, storeName := range storeNames { - store, err := NewStore(slogger, db, storeName.String()) + store, err := NewStore(ctx, slogger, db, storeName.String()) if err != nil { return nil, fmt.Errorf("failed to create '%s' KVStore: %w", storeName, err) } diff --git a/ee/agent/storage/ci/keyvalue_store_ci.go b/ee/agent/storage/ci/keyvalue_store_ci.go index cb1ede5f6..b37d84519 100644 --- a/ee/agent/storage/ci/keyvalue_store_ci.go +++ b/ee/agent/storage/ci/keyvalue_store_ci.go @@ -1,6 +1,7 @@ package storageci import ( + "context" "fmt" "log/slog" "os" @@ -23,7 +24,7 @@ func NewStore(t *testing.T, slogger *slog.Logger, bucketName string) (types.KVSt return inmemory.NewStore(), nil } - return agentbbolt.NewStore(slogger, SetupDB(t), bucketName) + return agentbbolt.NewStore(context.TODO(), slogger, SetupDB(t), bucketName) } // SetupDB is used for creating bbolt databases for testing diff --git a/ee/agent/storage/ci/keyvalue_store_test.go b/ee/agent/storage/ci/keyvalue_store_test.go index 2e614e010..b6ad89d23 100644 --- a/ee/agent/storage/ci/keyvalue_store_test.go +++ b/ee/agent/storage/ci/keyvalue_store_test.go @@ -1,6 +1,7 @@ package storageci import ( + "context" "errors" "fmt" "sync" @@ -16,7 +17,7 @@ import ( func getStores(t *testing.T) []types.KVStore { db := SetupDB(t) - bboltStore, err := agentbbolt.NewStore(multislogger.NewNopLogger(), db, "test_bucket") + bboltStore, err := agentbbolt.NewStore(context.TODO(), multislogger.NewNopLogger(), db, "test_bucket") require.NoError(t, err) stores := []types.KVStore{ diff --git a/ee/agent/storage/ci/stores_ci.go b/ee/agent/storage/ci/stores_ci.go index 8314472d0..e16d72e4a 100644 --- a/ee/agent/storage/ci/stores_ci.go +++ b/ee/agent/storage/ci/stores_ci.go @@ -1,6 +1,7 @@ package storageci import ( + "context" "fmt" "log/slog" "os" @@ -54,7 +55,7 @@ func makeBboltStores(t *testing.T, slogger *slog.Logger, db *bbolt.DB, storeName stores := make(map[storage.Store]types.KVStore) for _, storeName := range storeNames { - store, err := agentbbolt.NewStore(slogger, db, storeName.String()) + store, err := agentbbolt.NewStore(context.TODO(), slogger, db, storeName.String()) if err != nil { return nil, fmt.Errorf("failed to create '%s' KVStore: %w", storeName, err) } diff --git a/pkg/osquery/extension_test.go b/pkg/osquery/extension_test.go index af88bae07..cfcad3e68 100644 --- a/pkg/osquery/extension_test.go +++ b/pkg/osquery/extension_test.go @@ -99,7 +99,7 @@ func TestNewExtensionDatabaseError(t *testing.T) { }() m := mocks.NewKnapsack(t) - m.On("ConfigStore").Return(agentbbolt.NewStore(multislogger.NewNopLogger(), db, storage.ConfigStore.String())) + m.On("ConfigStore").Return(agentbbolt.NewStore(context.TODO(), multislogger.NewNopLogger(), db, storage.ConfigStore.String())) m.On("Slogger").Return(multislogger.NewNopLogger()).Maybe() e, err := NewExtension(context.TODO(), &mock.KolideService{}, m, ulid.New(), ExtensionOpts{}) From 5d448a3257d9781e054f9c56886b8aa74e45eb30 Mon Sep 17 00:00:00 2001 From: RebeccaMahany Date: Fri, 24 Jan 2025 15:11:04 -0500 Subject: [PATCH 3/3] Add more spans to CheckExecutable --- ee/tuf/util.go | 46 ++++++++++++++++++++++++++---------------- ee/tuf/util_darwin.go | 8 +++++++- ee/tuf/util_linux.go | 8 +++++++- ee/tuf/util_test.go | 24 +++++++++++----------- ee/tuf/util_windows.go | 8 +++++++- 5 files changed, 62 insertions(+), 32 deletions(-) diff --git a/ee/tuf/util.go b/ee/tuf/util.go index 9c51a4ee0..79e562e6c 100644 --- a/ee/tuf/util.go +++ b/ee/tuf/util.go @@ -16,10 +16,10 @@ import ( // CheckExecutable tests whether something is an executable. It // examines permissions, mode, and tries to exec it directly. func CheckExecutable(ctx context.Context, potentialBinary string, args ...string) error { - ctx, span := traces.StartSpan(ctx) + ctx, span := traces.StartSpan(ctx, "binary_path", potentialBinary) defer span.End() - if err := checkExecutablePermissions(potentialBinary); err != nil { + if err := checkExecutablePermissions(ctx, potentialBinary); err != nil { return err } @@ -36,34 +36,46 @@ func CheckExecutable(ctx context.Context, potentialBinary string, args ...string // in that circumstance. // See: https://github.com/golang/go/issues/22315 for i := 0; i < 3; i += 1 { - ctx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - - cmd := exec.CommandContext(ctx, potentialBinary, args...) //nolint:forbidigo // We trust the autoupdate library to find the correct location so we don't need allowedcmd - - // Set env, this should prevent launcher for fork-bombing - cmd.Env = append(cmd.Env, "LAUNCHER_SKIP_UPDATES=TRUE") - - out, execErr := cmd.CombinedOutput() + out, execErr := runExecutableCheck(ctx, potentialBinary, args...) if execErr != nil && errors.Is(execErr, syscall.ETXTBSY) { continue } - if ctx.Err() != nil { - return fmt.Errorf("timeout when checking executable: %w", ctx.Err()) - } - - return supressRoutineErrors(execErr, out) + return supressRoutineErrors(ctx, execErr, out) } return fmt.Errorf("could not exec %s despite retries due to text file busy", potentialBinary) } +// runExecutableCheck runs a single exec against the given binary and returns the result. +func runExecutableCheck(ctx context.Context, potentialBinary string, args ...string) ([]byte, error) { + ctx, span := traces.StartSpan(ctx) + defer span.End() + + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, potentialBinary, args...) //nolint:forbidigo // We trust the autoupdate library to find the correct location so we don't need allowedcmd + + // Set env, this should prevent launcher for fork-bombing + cmd.Env = append(cmd.Env, "LAUNCHER_SKIP_UPDATES=TRUE") + + out, err := cmd.CombinedOutput() + if ctx.Err() != nil { + return nil, fmt.Errorf("timeout when checking executable: %w", ctx.Err()) + } + + return out, err +} + // supressRoutineErrors attempts to tell whether the error was a // program that has executed, and then exited, vs one that's execution // was entirely unsuccessful. This differentiation allows us to // detect, and recover, from corrupt updates vs something in-app. -func supressRoutineErrors(err error, combinedOutput []byte) error { +func supressRoutineErrors(ctx context.Context, err error, combinedOutput []byte) error { + _, span := traces.StartSpan(ctx) + defer span.End() + if err == nil { return nil } diff --git a/ee/tuf/util_darwin.go b/ee/tuf/util_darwin.go index 3cd2b52e1..aa78ce5f2 100644 --- a/ee/tuf/util_darwin.go +++ b/ee/tuf/util_darwin.go @@ -4,10 +4,13 @@ package tuf import ( + "context" "errors" "fmt" "os" "path/filepath" + + "github.com/kolide/launcher/pkg/traces" ) // executableLocation returns the path to the executable in `updateDirectory`. @@ -33,7 +36,10 @@ func executableLocation(updateDirectory string, binary autoupdatableBinary) stri // checkExecutablePermissions checks whether a specific file looks // like it's executable. This is used in evaluating whether something // is an updated version. -func checkExecutablePermissions(potentialBinary string) error { +func checkExecutablePermissions(ctx context.Context, potentialBinary string) error { + _, span := traces.StartSpan(ctx) + defer span.End() + if potentialBinary == "" { return errors.New("empty string isn't executable") } diff --git a/ee/tuf/util_linux.go b/ee/tuf/util_linux.go index 9d7520266..5a8e55cb2 100644 --- a/ee/tuf/util_linux.go +++ b/ee/tuf/util_linux.go @@ -4,10 +4,13 @@ package tuf import ( + "context" "errors" "fmt" "os" "path/filepath" + + "github.com/kolide/launcher/pkg/traces" ) // executableLocation returns the path to the executable in `updateDirectory`. @@ -18,7 +21,10 @@ func executableLocation(updateDirectory string, binary autoupdatableBinary) stri // checkExecutablePermissions checks whether a specific file looks // like it's executable. This is used in evaluating whether something // is an updated version. -func checkExecutablePermissions(potentialBinary string) error { +func checkExecutablePermissions(ctx context.Context, potentialBinary string) error { + _, span := traces.StartSpan(ctx) + defer span.End() + if potentialBinary == "" { return errors.New("empty string isn't executable") } diff --git a/ee/tuf/util_test.go b/ee/tuf/util_test.go index b8b4519df..18f9faaf8 100644 --- a/ee/tuf/util_test.go +++ b/ee/tuf/util_test.go @@ -196,13 +196,13 @@ func windowsAddExe(in string) string { func Test_checkExecutablePermissions(t *testing.T) { t.Parallel() - require.Error(t, checkExecutablePermissions(""), "passing empty string") - require.Error(t, checkExecutablePermissions("/random/path/should/not/exist"), "passing non-existent file path") + require.Error(t, checkExecutablePermissions(context.TODO(), ""), "passing empty string") + require.Error(t, checkExecutablePermissions(context.TODO(), "/random/path/should/not/exist"), "passing non-existent file path") // Setup the tests tmpDir := t.TempDir() - require.Error(t, checkExecutablePermissions(tmpDir), "directory should not be executable") + require.Error(t, checkExecutablePermissions(context.TODO(), tmpDir), "directory should not be executable") dotExe := "" if runtime.GOOS == "windows" { @@ -222,17 +222,17 @@ func Test_checkExecutablePermissions(t *testing.T) { // windows doesn't have an executable bit if runtime.GOOS == "windows" { - require.NoError(t, checkExecutablePermissions(fileName), "plain file") - require.NoError(t, checkExecutablePermissions(hardLink), "hard link") - require.NoError(t, checkExecutablePermissions(symLink), "symlink") + require.NoError(t, checkExecutablePermissions(context.TODO(), fileName), "plain file") + require.NoError(t, checkExecutablePermissions(context.TODO(), hardLink), "hard link") + require.NoError(t, checkExecutablePermissions(context.TODO(), symLink), "symlink") } else { - require.Error(t, checkExecutablePermissions(fileName), "plain file") - require.Error(t, checkExecutablePermissions(hardLink), "hard link") - require.Error(t, checkExecutablePermissions(symLink), "symlink") + require.Error(t, checkExecutablePermissions(context.TODO(), fileName), "plain file") + require.Error(t, checkExecutablePermissions(context.TODO(), hardLink), "hard link") + require.Error(t, checkExecutablePermissions(context.TODO(), symLink), "symlink") require.NoError(t, os.Chmod(fileName, 0755)) - require.NoError(t, checkExecutablePermissions(fileName), "plain file") - require.NoError(t, checkExecutablePermissions(hardLink), "hard link") - require.NoError(t, checkExecutablePermissions(symLink), "symlink") + require.NoError(t, checkExecutablePermissions(context.TODO(), fileName), "plain file") + require.NoError(t, checkExecutablePermissions(context.TODO(), hardLink), "hard link") + require.NoError(t, checkExecutablePermissions(context.TODO(), symLink), "symlink") } } diff --git a/ee/tuf/util_windows.go b/ee/tuf/util_windows.go index 015cb9e4d..79f923055 100644 --- a/ee/tuf/util_windows.go +++ b/ee/tuf/util_windows.go @@ -4,11 +4,14 @@ package tuf import ( + "context" "errors" "fmt" "os" "path/filepath" "strings" + + "github.com/kolide/launcher/pkg/traces" ) // executableLocation returns the path to the executable in `updateDirectory`. @@ -22,7 +25,10 @@ func executableLocation(updateDirectory string, binary autoupdatableBinary) stri // // Windows does not have executable bits, so we omit those. And // instead check the file extension. -func checkExecutablePermissions(potentialBinary string) error { +func checkExecutablePermissions(ctx context.Context, potentialBinary string) error { + _, span := traces.StartSpan(ctx) + defer span.End() + if potentialBinary == "" { return errors.New("empty string isn't executable") }