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.")
}
}