From 5a959113036f3e7b49570e594a0e1dad24ab9f5d Mon Sep 17 00:00:00 2001 From: Sarah Gillespie <73313222+gillespi314@users.noreply.github.com> Date: Fri, 14 Jun 2024 12:48:00 -0500 Subject: [PATCH] Improve error messages for parsing MDM config profiles via fleetctl (#19495) --- changes/17316-parse-config-profile-error | 1 + server/service/client.go | 137 +++++++++++++++------- server/service/client_test.go | 138 +++++++++++++++++------ server/service/mdm.go | 13 ++- 4 files changed, 208 insertions(+), 81 deletions(-) create mode 100644 changes/17316-parse-config-profile-error diff --git a/changes/17316-parse-config-profile-error b/changes/17316-parse-config-profile-error new file mode 100644 index 0000000000..246bbff280 --- /dev/null +++ b/changes/17316-parse-config-profile-error @@ -0,0 +1 @@ +- Fixed issue where Windows-specific error message was displayed when failing to parse macOS configuration profiles. \ No newline at end of file diff --git a/server/service/client.go b/server/service/client.go index 69a1537f0d..54c5defdde 100644 --- a/server/service/client.go +++ b/server/service/client.go @@ -303,45 +303,84 @@ func (c *Client) runAppConfigChecks(fn func(ac *fleet.EnrichedAppConfig) error) // getProfilesContents takes file paths and creates a slice of profile payloads // ready to batch-apply. -func getProfilesContents(baseDir string, profiles []fleet.MDMProfileSpec, expandEnv bool) ([]fleet.MDMProfileBatchPayload, error) { - // map to check for duplicate names - extByName := make(map[string]string, len(profiles)) - result := make([]fleet.MDMProfileBatchPayload, 0, len(profiles)) +func getProfilesContents(baseDir string, macProfiles []fleet.MDMProfileSpec, windowsProfiles []fleet.MDMProfileSpec, expandEnv bool) ([]fleet.MDMProfileBatchPayload, error) { + // map to check for duplicate names across all profiles + extByName := make(map[string]string, len(macProfiles)) + result := make([]fleet.MDMProfileBatchPayload, 0, len(macProfiles)) - for _, profile := range profiles { - filePath := resolveApplyRelativePath(baseDir, profile.Path) - fileContents, err := os.ReadFile(filePath) - if err != nil { - return nil, fmt.Errorf("applying fleet config: %w", err) - } - - if expandEnv { - fileContents, err = spec.ExpandEnvBytes(fileContents) + // iterate over the profiles for each platform + for platform, profiles := range map[string][]fleet.MDMProfileSpec{ + "macos": macProfiles, + "windows": windowsProfiles, + } { + for _, profile := range profiles { + filePath := resolveApplyRelativePath(baseDir, profile.Path) + fileContents, err := os.ReadFile(filePath) if err != nil { - return nil, fmt.Errorf("expanding environment on file %q: %w", profile.Path, err) + return nil, fmt.Errorf("applying custom settings: %w", err) } - } - // by default, use the file name. macOS profiles use their PayloadDisplayName - ext := filepath.Ext(filePath) - name := strings.TrimSuffix(filepath.Base(filePath), ext) - if mdm.GetRawProfilePlatform(fileContents) == "darwin" && ext == ".mobileconfig" { - mc, err := fleet.NewMDMAppleConfigProfile(fileContents, nil) - if err != nil { - return nil, fmt.Errorf("applying fleet config: %w", err) + if expandEnv { + fileContents, err = spec.ExpandEnvBytes(fileContents) + if err != nil { + return nil, fmt.Errorf("expanding environment on file %q: %w", profile.Path, err) + } } - name = strings.TrimSpace(mc.Name) - } - if e, isDuplicate := extByName[name]; isDuplicate { - return nil, errors.New(fmtDuplicateNameErrMsg(name, e, ext)) - } - extByName[name] = ext - result = append(result, fleet.MDMProfileBatchPayload{ - Name: name, - Contents: fileContents, - Labels: profile.Labels, - }) + ext := filepath.Ext(filePath) + // by default, use the file name (for macOS mobileconfig profiles, we'll switch to + // their PayloadDisplayName when we parse the profile below) + name := strings.TrimSuffix(filepath.Base(filePath), ext) + // for validation errors, we want to include the platform and file name in the error message + prefixErrMsg := fmt.Sprintf("Couldn't edit %s_settings.custom_settings (%s%s)", platform, name, ext) + + // validate macOS profiles + if platform == "macos" { + switch ext { + case ".mobileconfig", ".xml": // allowing .xml for backwards compatibility + mc, err := fleet.NewMDMAppleConfigProfile(fileContents, nil) + if err != nil { + errForMsg := errors.Unwrap(err) + if errForMsg == nil { + errForMsg = err + } + return nil, fmt.Errorf("%s: %w", prefixErrMsg, errForMsg) + } + name = strings.TrimSpace(mc.Name) + case ".json": + if mdm.GetRawProfilePlatform(fileContents) != "darwin" { + return nil, fmt.Errorf("%s: %s", prefixErrMsg, "Declaration profiles should include valid JSON.") + } + default: + return nil, fmt.Errorf("%s: %s", prefixErrMsg, "macOS configuration profiles must be .mobileconfig or .json files.") + } + } + + // validate windows profiles + if platform == "windows" { + switch ext { + case ".xml": + if mdm.GetRawProfilePlatform(fileContents) != "windows" { + return nil, fmt.Errorf("%s: %s", prefixErrMsg, "Windows configuration profiles can only have or top level elements") + } + default: + return nil, fmt.Errorf("%s: %s", prefixErrMsg, "Windows configuration profiles must be .xml files.") + } + } + + // check for duplicate names across all profiles + if e, isDuplicate := extByName[name]; isDuplicate { + return nil, errors.New(fmtDuplicateNameErrMsg(name, e, ext)) + } + extByName[name] = ext + + result = append(result, fleet.MDMProfileBatchPayload{ + Name: name, + Contents: fileContents, + Labels: profile.Labels, + }) + + } } return result, nil } @@ -421,7 +460,6 @@ func (c *Client) ApplyGroup( if specs.AppConfig != nil { windowsCustomSettings := extractAppCfgWindowsCustomSettings(specs.AppConfig) macosCustomSettings := extractAppCfgMacOSCustomSettings(specs.AppConfig) - allCustomSettings := append(macosCustomSettings, windowsCustomSettings...) // if there is no custom setting but the windows and mac settings are // non-nil, this means that we want to clear the existing custom settings, @@ -430,8 +468,8 @@ func (c *Client) ApplyGroup( // TODO(mna): shouldn't that be an || instead of && ? I.e. if there are no // custom settings but windows is present and empty (but mac is absent), // shouldn't that clear the windows ones? - if (windowsCustomSettings != nil && macosCustomSettings != nil) || len(allCustomSettings) > 0 { - fileContents, err := getProfilesContents(baseDir, allCustomSettings, opts.ExpandEnvConfigProfiles) + if (windowsCustomSettings != nil && macosCustomSettings != nil) || len(windowsCustomSettings)+len(macosCustomSettings) > 0 { + fileContents, err := getProfilesContents(baseDir, macosCustomSettings, windowsCustomSettings, opts.ExpandEnvConfigProfiles) if err != nil { return nil, err } @@ -520,10 +558,10 @@ func (c *Client) ApplyGroup( tmMDMSettings := extractTmSpecsMDMCustomSettings(specs.Teams) tmFileContents := make(map[string][]fleet.MDMProfileBatchPayload, len(tmMDMSettings)) - for k, paths := range tmMDMSettings { - fileContents, err := getProfilesContents(baseDir, paths, opts.ExpandEnvConfigProfiles) + for k, profileSpecs := range tmMDMSettings { + fileContents, err := getProfilesContents(baseDir, profileSpecs.macos, profileSpecs.windows, opts.ExpandEnvConfigProfiles) if err != nil { - return nil, err + return nil, fmt.Errorf("Team %s: %w", k, err) // TODO: consider adding team name to improve error messages generally for other parts of the config because multiple team configs can be processed at once } tmFileContents[k] = fileContents } @@ -837,9 +875,14 @@ func extractAppCfgScripts(appCfg interface{}) []string { return scriptsStrings } +type profileSpecsByPlatform struct { + macos []fleet.MDMProfileSpec + windows []fleet.MDMProfileSpec +} + // returns the custom macOS and Windows settings keyed by team name. -func extractTmSpecsMDMCustomSettings(tmSpecs []json.RawMessage) map[string][]fleet.MDMProfileSpec { - var m map[string][]fleet.MDMProfileSpec +func extractTmSpecsMDMCustomSettings(tmSpecs []json.RawMessage) map[string]profileSpecsByPlatform { + var m map[string]profileSpecsByPlatform for _, tm := range tmSpecs { var spec struct { Name string `json:"name"` @@ -866,7 +909,7 @@ func extractTmSpecsMDMCustomSettings(tmSpecs []json.RawMessage) map[string][]fle if len(spec.MDM.MacOSSettings.CustomSettings) > 0 || len(spec.MDM.WindowsSettings.CustomSettings) > 0 { if m == nil { - m = make(map[string][]fleet.MDMProfileSpec) + m = make(map[string]profileSpecsByPlatform) } } @@ -894,8 +937,16 @@ func extractTmSpecsMDMCustomSettings(tmSpecs []json.RawMessage) map[string][]fle } // TODO: validate equal names here and API? + var result profileSpecsByPlatform + if macOSSettings != nil { + result.macos = macOSSettings + } + if windowsSettings != nil { + result.windows = windowsSettings + } + if macOSSettings != nil || windowsSettings != nil { - m[spec.Name] = append(macOSSettings, windowsSettings...) + m[spec.Name] = result } } } diff --git a/server/service/client_test.go b/server/service/client_test.go index f84baca68e..7eb7314da1 100644 --- a/server/service/client_test.go +++ b/server/service/client_test.go @@ -282,7 +282,7 @@ func TestExtractTeamSpecsMDMCustomSettings(t *testing.T) { cases := []struct { desc string yaml string - want map[string][]fleet.MDMProfileSpec + want map[string]profileSpecsByPlatform }{ { "no settings", @@ -342,7 +342,7 @@ spec: windows_settings: custom_settings: `, - map[string][]fleet.MDMProfileSpec{"Fleet": {}, "Fleet2": {}}, + map[string]profileSpecsByPlatform{"Fleet": {windows: []fleet.MDMProfileSpec{}, macos: []fleet.MDMProfileSpec{}}, "Fleet2": {windows: []fleet.MDMProfileSpec{}, macos: []fleet.MDMProfileSpec{}}}, }, { "custom settings specified", @@ -368,11 +368,15 @@ spec: - "foo" - baz `, - map[string][]fleet.MDMProfileSpec{"Fleet": { - {Path: "a", Labels: []string{"foo", "bar"}}, - {Path: "b"}, - {Path: "c"}, - {Path: "d", Labels: []string{"foo", "baz"}}, + map[string]profileSpecsByPlatform{"Fleet": { + macos: []fleet.MDMProfileSpec{ + {Path: "a", Labels: []string{"foo", "bar"}}, + {Path: "b"}, + }, + windows: []fleet.MDMProfileSpec{ + {Path: "c"}, + {Path: "d", Labels: []string{"foo", "baz"}}, + }, }}, }, { @@ -393,7 +397,13 @@ spec: - "c" - "d" `, - map[string][]fleet.MDMProfileSpec{"Fleet": {{Path: "a"}, {Path: "b"}, {Path: "c"}, {Path: "d"}}}, + map[string]profileSpecsByPlatform{"Fleet": { + macos: []fleet.MDMProfileSpec{{Path: "a"}, {Path: "b"}}, + windows: []fleet.MDMProfileSpec{ + {Path: "c"}, + {Path: "d"}, + }, + }}, }, { "invalid custom settings", @@ -423,7 +433,7 @@ spec: - path: 24 - path: "y" `, - map[string][]fleet.MDMProfileSpec{}, + map[string]profileSpecsByPlatform{}, }, { "old invalid custom settings", @@ -447,7 +457,7 @@ spec: - 24 - "y" `, - map[string][]fleet.MDMProfileSpec{}, + map[string]profileSpecsByPlatform{}, }, } for _, c := range cases { @@ -455,8 +465,13 @@ spec: specs, err := spec.GroupFromBytes([]byte(c.yaml)) require.NoError(t, err) if len(specs.Teams) > 0 { - got := extractTmSpecsMDMCustomSettings(specs.Teams) - require.Equal(t, c.want, got) + gotSpecs := extractTmSpecsMDMCustomSettings(specs.Teams) + for k, wantProfs := range c.want { + gotProfs, ok := gotSpecs[k] + require.True(t, ok) + require.Equal(t, wantProfs.macos, gotProfs.macos) + require.Equal(t, wantProfs.windows, gotProfs.windows) + } } }) } @@ -502,19 +517,21 @@ func TestGetProfilesContents(t *testing.T) { ` tests := []struct { - name string - baseDir string - setupFiles [][2]string - labels []string - environment map[string]string - expandEnv bool - expectError bool - want []fleet.MDMProfileBatchPayload + name string + baseDir string + macSetupFiles [][2]string + winSetupFiles [][2]string + labels []string + environment map[string]string + expandEnv bool + expectError bool + want []fleet.MDMProfileBatchPayload + wantErr string }{ { name: "invalid darwin xml", baseDir: tempDir, - setupFiles: [][2]string{ + macSetupFiles: [][2]string{ {"foo.mobileconfig", ``}, }, expectError: true, @@ -523,10 +540,12 @@ func TestGetProfilesContents(t *testing.T) { { name: "windows and darwin files", baseDir: tempDir, - setupFiles: [][2]string{ - {"foo.xml", string(windowsProfile)}, + macSetupFiles: [][2]string{ {"bar.mobileconfig", string(darwinProfile)}, }, + winSetupFiles: [][2]string{ + {"foo.xml", string(windowsProfile)}, + }, expectError: false, want: []fleet.MDMProfileBatchPayload{ {Name: "foo", Contents: windowsProfile}, @@ -536,10 +555,12 @@ func TestGetProfilesContents(t *testing.T) { { name: "windows and darwin files with labels", baseDir: tempDir, - setupFiles: [][2]string{ - {"foo.xml", string(windowsProfile)}, + macSetupFiles: [][2]string{ {"bar.mobileconfig", string(darwinProfile)}, }, + winSetupFiles: [][2]string{ + {"foo.xml", string(windowsProfile)}, + }, labels: []string{"foo", "bar"}, expectError: false, want: []fleet.MDMProfileBatchPayload{ @@ -550,10 +571,12 @@ func TestGetProfilesContents(t *testing.T) { { name: "darwin files with file name != PayloadDisplayName", baseDir: tempDir, - setupFiles: [][2]string{ - {"foo.xml", string(windowsProfile)}, + macSetupFiles: [][2]string{ {"bar.mobileconfig", string(darwinProfile)}, }, + winSetupFiles: [][2]string{ + {"foo.xml", string(windowsProfile)}, + }, expectError: false, want: []fleet.MDMProfileBatchPayload{ {Name: "foo", Contents: windowsProfile}, @@ -563,16 +586,18 @@ func TestGetProfilesContents(t *testing.T) { { name: "duplicate names across windows and darwin", baseDir: tempDir, - setupFiles: [][2]string{ - {"baz.xml", string(windowsProfile)}, + macSetupFiles: [][2]string{ {"bar.mobileconfig", string(mobileconfigForTest("baz", "I"))}, }, + winSetupFiles: [][2]string{ + {"baz.xml", string(windowsProfile)}, + }, expectError: true, }, { name: "duplicate file names", baseDir: tempDir, - setupFiles: [][2]string{ + winSetupFiles: [][2]string{ {"baz.xml", string(windowsProfile)}, {"baz.xml", string(windowsProfile)}, }, @@ -581,8 +606,10 @@ func TestGetProfilesContents(t *testing.T) { { name: "with environment variables", baseDir: tempDir, - setupFiles: [][2]string{ + macSetupFiles: [][2]string{ {"bar.mobileconfig", darwinProfileWithFooEnv}, + }, + winSetupFiles: [][2]string{ {"foo.xml", windowsProfileWithBarEnv}, }, environment: map[string]string{"FOO": "42", "BAR": "24"}, @@ -634,14 +661,43 @@ func TestGetProfilesContents(t *testing.T) { { name: "with environment variables but not set", baseDir: tempDir, - setupFiles: [][2]string{ + macSetupFiles: [][2]string{ {"bar.mobileconfig", darwinProfileWithFooEnv}, + }, + winSetupFiles: [][2]string{ {"foo.xml", windowsProfileWithBarEnv}, }, environment: map[string]string{}, expandEnv: true, expectError: true, }, + { + name: "with unprocessable json", + baseDir: tempDir, + macSetupFiles: [][2]string{ + {"bar.json", string(windowsProfile)}, + }, + expectError: true, + wantErr: "Couldn't edit macos_settings.custom_settings (bar.json): Declaration profiles should include valid JSON", + }, + { + name: "with unprocessable xml", + baseDir: tempDir, + winSetupFiles: [][2]string{ + {"bar.xml", string(darwinProfile)}, + }, + expectError: true, + wantErr: "Couldn't edit windows_settings.custom_settings (bar.xml): Windows configuration profiles can only have or top level elements", + }, + { + name: "with unsupported extension", + baseDir: tempDir, + macSetupFiles: [][2]string{ + {"bar.cfg", string(darwinProfile)}, + }, + expectError: true, + wantErr: "Couldn't edit macos_settings.custom_settings (bar.cfg): macOS configuration profiles must be .mobileconfig or .json files", + }, } for _, tt := range tests { @@ -658,17 +714,27 @@ func TestGetProfilesContents(t *testing.T) { }) } } - paths := []fleet.MDMProfileSpec{} - for _, fileSpec := range tt.setupFiles { + macPaths := []fleet.MDMProfileSpec{} + for _, fileSpec := range tt.macSetupFiles { filePath := filepath.Join(tempDir, fileSpec[0]) require.NoError(t, os.WriteFile(filePath, []byte(fileSpec[1]), 0o644)) - paths = append(paths, fleet.MDMProfileSpec{Path: filePath, Labels: tt.labels}) + macPaths = append(macPaths, fleet.MDMProfileSpec{Path: filePath, Labels: tt.labels}) } - profileContents, err := getProfilesContents(tt.baseDir, paths, tt.expandEnv) + winPaths := []fleet.MDMProfileSpec{} + for _, fileSpec := range tt.winSetupFiles { + filePath := filepath.Join(tempDir, fileSpec[0]) + require.NoError(t, os.WriteFile(filePath, []byte(fileSpec[1]), 0o644)) + winPaths = append(winPaths, fleet.MDMProfileSpec{Path: filePath, Labels: tt.labels}) + } + + profileContents, err := getProfilesContents(tt.baseDir, macPaths, winPaths, tt.expandEnv) if tt.expectError { require.Error(t, err) + if tt.wantErr != "" { + require.Contains(t, err.Error(), tt.wantErr) + } } else { require.NoError(t, err) require.NotNil(t, profileContents) diff --git a/server/service/mdm.go b/server/service/mdm.go index 069a5ecf1d..11e0fd1c05 100644 --- a/server/service/mdm.go +++ b/server/service/mdm.go @@ -1869,8 +1869,17 @@ func validateProfiles(profiles []fleet.MDMProfileBatchPayload) error { for _, profile := range profiles { platform := mdm.GetRawProfilePlatform(profile.Contents) if platform != "darwin" && platform != "windows" { - // TODO(roberto): there's ongoing feedback with Marko about improving this message, as it's too windows specific - return fleet.NewInvalidArgumentError("mdm", "Windows configuration profiles can only have or top level elements.") + // We can only display a generic error message here because at this point + // we don't know the file extension or whether the profile is intended + // for macos_settings or windows_settings. We should expecte never see this + // in practice because the client should be validating the profiles + // before sending them to the server so the client can surface more helpful + // error messages to the user. However, we're validating again here just + // in case the client is not working as expected. + return fleet.NewInvalidArgumentError("mdm", fmt.Sprintf( + "%s is not a valid macOS or Windows configuration profile. ", profile.Name)+ + "macOS profiles must be valid .mobileconfig or .json files. "+ + "Windows configuration profiles can only have or top level elements.") } }