From 8de3e9f258c5d261b019dca364e83b1efbe77916 Mon Sep 17 00:00:00 2001 From: Lucas Manuel Rodriguez Date: Tue, 11 Oct 2022 20:11:01 -0300 Subject: [PATCH] Fix Orbit bug when setting empty command_line_flags in agent options (#8176) --- orbit/changes/bug-orbit-restart-empty-flags | 1 + orbit/pkg/update/flag_runner.go | 88 +++++++++++++-------- orbit/pkg/update/flag_runner_test.go | 57 +++++++++++++ 3 files changed, 115 insertions(+), 31 deletions(-) create mode 100644 orbit/changes/bug-orbit-restart-empty-flags diff --git a/orbit/changes/bug-orbit-restart-empty-flags b/orbit/changes/bug-orbit-restart-empty-flags new file mode 100644 index 0000000000..775727bf2d --- /dev/null +++ b/orbit/changes/bug-orbit-restart-empty-flags @@ -0,0 +1 @@ +- Fixed Orbit bug that caused it to restart repeatedly when Fleet agent options are configured with `command_line_flags: {}`. diff --git a/orbit/pkg/update/flag_runner.go b/orbit/pkg/update/flag_runner.go index 96376601d6..436118db7e 100644 --- a/orbit/pkg/update/flag_runner.go +++ b/orbit/pkg/update/flag_runner.go @@ -16,14 +16,20 @@ import ( "github.com/rs/zerolog/log" ) +// OrbitConfigFetcher allows fetching Orbit configuration. +type OrbitConfigFetcher interface { + // GetConfig returns the Orbit configuration. + GetConfig() (*service.OrbitConfig, error) +} + // FlagRunner is a specialized runner to periodically check and update flags from Fleet // It is designed with Execute and Interrupt functions to be compatible with oklog/run // // It uses an OrbitClient, along with FlagUpdateOptions to connect to Fleet type FlagRunner struct { - orbitClient *service.OrbitClient - opt FlagUpdateOptions - cancel chan struct{} + configFetcher OrbitConfigFetcher + opt FlagUpdateOptions + cancel chan struct{} } // FlagUpdateOptions is options provided for the flag update runner @@ -36,11 +42,11 @@ type FlagUpdateOptions struct { // NewFlagRunner creates a new runner with provided options // The runner must be started with Execute -func NewFlagRunner(orbitClient *service.OrbitClient, opt FlagUpdateOptions) *FlagRunner { +func NewFlagRunner(configFetcher OrbitConfigFetcher, opt FlagUpdateOptions) *FlagRunner { return &FlagRunner{ - orbitClient: orbitClient, - opt: opt, - cancel: make(chan struct{}), + configFetcher: configFetcher, + opt: opt, + cancel: make(chan struct{}), } } @@ -93,7 +99,7 @@ func (r *FlagRunner) DoFlagsUpdate() (bool, error) { } // next GetConfig from Fleet API - config, err := r.orbitClient.GetConfig() + config, err := r.configFetcher.GetConfig() if err != nil { return false, fmt.Errorf("error getting flags from fleet: %w", err) } @@ -120,20 +126,19 @@ func (r *FlagRunner) DoFlagsUpdate() (bool, error) { return true, nil } -// getFlagsFromJSON converts the json of the type below -// {"number": 5, "string": "str", "boolean": true} -// to a map[string]string -// this map will get compared and written to the filesystem and passed to osquery -// this only supports simple key:value pairs and not nested structures +// getFlagsFromJSON converts a json document of the form +// `{"number": 5, "string": "str", "boolean": true}` to a map[string]string. +// +// This only supports simple key:value pairs and not nested structures. +// +// Returns an empty map if flags is nil or an empty JSON `{}`. func getFlagsFromJSON(flags json.RawMessage) (map[string]string, error) { - result := make(map[string]string) - var data map[string]interface{} err := json.Unmarshal([]byte(flags), &data) if err != nil { return nil, err } - + result := make(map[string]string) for k, v := range data { switch t := v.(type) { case string: @@ -146,7 +151,6 @@ func getFlagsFromJSON(flags json.RawMessage) (map[string]string, error) { result["--"+k] = fmt.Sprintf("%v", v) } } - return result, nil } @@ -170,29 +174,51 @@ func writeFlagFile(rootDir string, data map[string]string) error { return nil } -// readFlagFile reads and parses the osquery.flags file on disk -// and returns a map[string]string, of the form: -// {"--foo":"bar","--value":"5"} -// this only supports simple key:value pairs and not nested structures +// readFlagFile reads and parses the osquery.flags file on disk of the form +// +// --foo="bar" +// --bar=5 +// --zoo=true +// --verbose +// +// and returns a map[string]string: +// +// {"--foo": "bar", "--bar": 5, "--zoo", "--verbose": ""} +// +// This only supports simple key:value pairs and not nested structures. +// +// Returns: +// - an error if the file does not exist. +// - an empty map if the file is empty. func readFlagFile(rootDir string) (map[string]string, error) { flagfile := filepath.Join(rootDir, "osquery.flags") bytes, err := os.ReadFile(flagfile) if err != nil { return nil, fmt.Errorf("reading flagfile %s failed: %w", flagfile, err) } + content := strings.TrimSpace(string(bytes)) result := make(map[string]string) - lines := strings.Split(strings.TrimSpace(string(bytes)), "\n") + if len(content) == 0 { + return result, nil + } + lines := strings.Split(content, "\n") for _, line := range lines { + line := strings.TrimSpace(line) + // skip any empty lines + if line == "" { + continue + } // skip line starting with "#" indicating that it's a comment - if !strings.HasPrefix(line, "#") { - // split each line by "=" - str := strings.Split(strings.TrimSpace(line), "=") - if len(str) == 2 { - result[str[0]] = str[1] - } - if len(str) == 1 { - result[str[0]] = "" - } + if strings.HasPrefix(line, "#") { + continue + } + // split each line by "=" + str := strings.Split(line, "=") + if len(str) == 2 { + result[str[0]] = str[1] + } + if len(str) == 1 { + result[str[0]] = "" } } return result, nil diff --git a/orbit/pkg/update/flag_runner_test.go b/orbit/pkg/update/flag_runner_test.go index 3b214acd15..1cf6b7d571 100644 --- a/orbit/pkg/update/flag_runner_test.go +++ b/orbit/pkg/update/flag_runner_test.go @@ -2,9 +2,12 @@ package update import ( "encoding/json" + "os" + "path/filepath" "reflect" "testing" + "github.com/fleetdm/fleet/v4/server/service" "github.com/stretchr/testify/require" ) @@ -65,3 +68,57 @@ func TestWriteFlagFile(t *testing.T) { t.Errorf("expected flags to be equal: %v, %v", flags, diskFlags) } } + +func touchFile(t *testing.T, name string) { + t.Helper() + + file, err := os.OpenFile(name, os.O_RDONLY|os.O_CREATE, 0o644) + require.NoError(t, err) + require.NoError(t, file.Close()) +} + +type dummyConfigFetcher struct { + cfg *service.OrbitConfig +} + +func (d *dummyConfigFetcher) GetConfig() (*service.OrbitConfig, error) { + return d.cfg, nil +} + +// TestDoFlagsUpdateWithEmptyFlags tests the scenario of Fleet flag `command_line_flags` +// being set to an empty JSON document `{}` and Orbit osquery.flags file being +// an empty file. Such scenario should trigger no update of flags. +func TestDoFlagsUpdateWithEmptyFlags(t *testing.T) { + rootDir := t.TempDir() + osqueryFlagsFile := filepath.Join(rootDir, "osquery.flags") + touchFile(t, osqueryFlagsFile) + + dcf := dummyConfigFetcher{cfg: &service.OrbitConfig{ + Flags: json.RawMessage("{}"), + }} + fr := NewFlagRunner(&dcf, FlagUpdateOptions{ + RootDir: rootDir, + }) + + needsUpdate, err := fr.DoFlagsUpdate() + require.NoError(t, err) + require.False(t, needsUpdate) + + // Non-empty fleet flags and osquery.flags has empty flags. + dcf.cfg = &service.OrbitConfig{ + Flags: json.RawMessage(`{"--verbose": true}`), + } + needsUpdate, err = fr.DoFlagsUpdate() + require.NoError(t, err) + require.True(t, needsUpdate) + + // Empty Fleet flags and osquery.flags has non-empty flags. + dcf.cfg = &service.OrbitConfig{ + Flags: json.RawMessage("{}"), + } + err = os.WriteFile(osqueryFlagsFile, []byte("--verbose=true\n"), 0o644) + require.NoError(t, err) + needsUpdate, err = fr.DoFlagsUpdate() + require.NoError(t, err) + require.True(t, needsUpdate) +}