From e1daf2e090d3a09eba6630652afc31175be5f85d Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Wed, 25 Dec 2024 14:38:50 -0600 Subject: [PATCH 1/5] Added ability to upload profiles with secret variables using the /configuration_profiles endpoint. --- .../20241209164540_AddSecretVariablesTable.go | 5 +- server/service/apple_mdm.go | 23 +-- .../service/integration_mdm_profiles_test.go | 153 ++++++++++++++++++ server/service/mdm.go | 4 +- 4 files changed, 171 insertions(+), 14 deletions(-) diff --git a/server/datastore/mysql/migrations/tables/20241209164540_AddSecretVariablesTable.go b/server/datastore/mysql/migrations/tables/20241209164540_AddSecretVariablesTable.go index d6252fbe448f..426bf35dc2f5 100644 --- a/server/datastore/mysql/migrations/tables/20241209164540_AddSecretVariablesTable.go +++ b/server/datastore/mysql/migrations/tables/20241209164540_AddSecretVariablesTable.go @@ -19,8 +19,9 @@ func Up_20241209164540(tx *sql.Tx) error { id INT UNSIGNED NOT NULL AUTO_INCREMENT, name VARCHAR(255) NOT NULL, value BLOB NOT NULL, -- 64KB max value size - created_at TIMESTAMP(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), - updated_at TIMESTAMP(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6), + -- Using DATETIME instead of TIMESTAMP to prevent future Y2K38 issues, since updated_at is used as a trigger to resend profiles + created_at DATETIME(6) NOT NULL DEFAULT NOW(6), + updated_at DATETIME(6) NOT NULL DEFAULT NOW(6) ON UPDATE NOW(6), PRIMARY KEY (id), CONSTRAINT idx_secret_variables_name UNIQUE (name) ) ENGINE = InnoDB DEFAULT CHARSET = utf8mb4 COLLATE = utf8mb4_unicode_ci`, diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index ddb9e94d97bc..51f8bc82b501 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -392,12 +392,17 @@ func (svc *Service) NewMDMAppleConfigProfile(ctx context.Context, teamID uint, r Message: fmt.Sprintf("failed to parse config profile: %s", err.Error()), }) } - // Save the original unexpanded profile - cp.Mobileconfig = b if err := cp.ValidateUserProvided(); err != nil { return nil, ctxerr.Wrap(ctx, &fleet.BadRequestError{Message: err.Error()}) } + err = validateConfigProfileFleetVariables(string(cp.Mobileconfig)) + if err != nil { + return nil, ctxerr.Wrap(ctx, err, "validating fleet variables") + } + + // Save the original unexpanded profile + cp.Mobileconfig = b labelMap, err := svc.validateProfileLabels(ctx, labels) if err != nil { @@ -414,11 +419,6 @@ func (svc *Service) NewMDMAppleConfigProfile(ctx context.Context, teamID uint, r // TODO what happens if mode is not set?s } - err = validateConfigProfileFleetVariables(string(cp.Mobileconfig)) - if err != nil { - return nil, ctxerr.Wrap(ctx, err, "validating fleet variables") - } - newCP, err := svc.ds.NewMDMAppleConfigProfile(ctx, *cp) if err != nil { var existsErr existsErrorInterface @@ -512,19 +512,22 @@ func (svc *Service) NewMDMAppleDeclaration(ctx context.Context, teamID uint, r i return nil, err } - if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{string(data)}); err != nil { + dataWithSecrets, err := svc.ds.ExpandEmbeddedSecrets(ctx, string(data)) + if err != nil { return nil, fleet.NewInvalidArgumentError("profile", err.Error()) } - if err := validateDeclarationFleetVariables(string(data)); err != nil { + if err := validateDeclarationFleetVariables(dataWithSecrets); err != nil { return nil, err } // TODO(roberto): Maybe GetRawDeclarationValues belongs inside NewMDMAppleDeclaration? We can refactor this in a follow up. - rawDecl, err := fleet.GetRawDeclarationValues(data) + rawDecl, err := fleet.GetRawDeclarationValues([]byte(dataWithSecrets)) if err != nil { return nil, err } + // After validation, we should no longer need to keep the secrets, so we explicitly clear the variable holding them + dataWithSecrets = "" if err := rawDecl.ValidateUserProvided(); err != nil { return nil, err diff --git a/server/service/integration_mdm_profiles_test.go b/server/service/integration_mdm_profiles_test.go index 7dc247133d9c..8c085521e2f6 100644 --- a/server/service/integration_mdm_profiles_test.go +++ b/server/service/integration_mdm_profiles_test.go @@ -5271,3 +5271,156 @@ func (s *integrationMDMTestSuite) TestOTAProfile() { require.Contains(t, string(b), fmt.Sprintf("%s/api/v1/fleet/ota_enrollment?enroll_secret=%s", cfg.ServerSettings.ServerURL, escSec)) require.Contains(t, string(b), cfg.OrgInfo.OrgName) } + +// TestAppleDDMSecretVariablesUpload tests uploading DDM profiles with secrets via the /configuration_profiles endpoint +func (s *integrationMDMTestSuite) TestAppleDDMSecretVariablesUpload() { + tmpl := ` +{ + "Type": "com.apple.configuration.decl%d", + "Identifier": "com.fleet.config%d", + "Payload": { + "ServiceType": "com.apple.bash%d", + "DataAssetReference": "com.fleet.asset.bash" + } +}` + + newProfileBytes := func(i int) []byte { + return []byte(fmt.Sprintf(tmpl, i, i, i)) + } + + s.testSecretVariablesUpload(newProfileBytes, "json", "darwin") +} + +func (s *integrationMDMTestSuite) testSecretVariablesUpload(newProfileBytes func(i int) []byte, fileExtension string, platform string) { + t := s.T() + var profiles [][]byte + for i := 0; i < 2; i++ { + profiles = append(profiles, newProfileBytes(i)) + } + // Use secrets + myBash := "com.apple.bash0" + profiles[0] = []byte(strings.ReplaceAll(string(profiles[0]), myBash, "$"+fleet.ServerSecretPrefix+"BASH")) + secretProfile := profiles[1] + profiles[1] = []byte("${" + fleet.ServerSecretPrefix + "PROFILE}") + + body, headers := generateNewProfileMultipartRequest( + t, "secret-config0."+fileExtension, profiles[0], s.token, nil, + ) + res := s.DoRawWithHeaders("POST", "/api/latest/fleet/configuration_profiles", body.Bytes(), http.StatusUnprocessableEntity, headers) + assertBodyContains(t, res, `Secret variable \"$FLEET_SECRET_BASH\" missing`) + + // Add secret(s) to server + req := secretVariablesRequest{ + SecretVariables: []fleet.SecretVariable{ + { + Name: "FLEET_SECRET_BASH", + Value: myBash, + }, + { + Name: "FLEET_SECRET_PROFILE", + Value: string(secretProfile), + }, + }, + } + secretResp := secretVariablesResponse{} + s.DoJSON("PUT", "/api/latest/fleet/spec/secret_variables", req, http.StatusOK, &secretResp) + res = s.DoRawWithHeaders("POST", "/api/latest/fleet/configuration_profiles", body.Bytes(), http.StatusOK, headers) + var resp newMDMConfigProfileResponse + err := json.NewDecoder(res.Body).Decode(&resp) + require.NoError(t, err) + assert.NotEmpty(t, resp.ProfileUUID) + + body, headers = generateNewProfileMultipartRequest( + t, "secret-config1."+fileExtension, profiles[1], s.token, nil, + ) + s.DoJSON("PUT", "/api/latest/fleet/spec/secret_variables", req, http.StatusOK, &secretResp) + res = s.DoRawWithHeaders("POST", "/api/latest/fleet/configuration_profiles", body.Bytes(), http.StatusOK, headers) + err = json.NewDecoder(res.Body).Decode(&resp) + require.NoError(t, err) + assert.NotEmpty(t, resp.ProfileUUID) + + var listResp listMDMConfigProfilesResponse + s.DoJSON("GET", "/api/latest/fleet/mdm/profiles", &listMDMConfigProfilesRequest{}, http.StatusOK, &listResp) + require.Len(t, listResp.Profiles, 2) + for _, p := range listResp.Profiles { + switch p.Name { + case "secret-config0": + assert.Equal(t, platform, p.Platform) + case "secret-config1": + assert.Equal(t, platform, p.Platform) + default: + t.Errorf("unexpected profile %s", p.Name) + } + } +} + +// TestAppleConfigSecretVariablesUpload tests uploading Apple config profiles with secrets via the /configuration_profiles endpoint +func (s *integrationMDMTestSuite) TestAppleConfigSecretVariablesUpload() { + tmpl := ` + + + + + PayloadDescription + For secret variables + PayloadDisplayName + secret-config%d + PayloadIdentifier + PI%d + PayloadType + Configuration + PayloadUUID + %d + PayloadVersion + 1 + PayloadContent + + + Bash + $FLEET_SECRET_BASH + PayloadDisplayName + secret payload + PayloadIdentifier + com.test.secret + PayloadType + com.test.secretd + PayloadUUID + 476F5334-D501-4768-9A31-1A18A4E1E808 + PayloadVersion + 1 + + + +` + + newProfileBytes := func(i int) []byte { + return []byte(fmt.Sprintf(tmpl, i, i, i)) + } + + s.testSecretVariablesUpload(newProfileBytes, "mobileconfig", "darwin") + +} + +// TestWindowsConfigSecretVariablesUpload tests uploading Windows profiles with secrets via the /configuration_profiles endpoint +func (s *integrationMDMTestSuite) TestWindowsConfigSecretVariablesUpload() { + tmpl := ` + + + + int + + + ./Device/Vendor/MSFT/Policy/Config/System/DisableOneDriveFileSync + + $FLEET_SECRET_BASH + + +` + + newProfileBytes := func(i int) []byte { + return []byte(fmt.Sprintf(tmpl, i, i, i)) + } + + s.testSecretVariablesUpload(newProfileBytes, "xml", "windows") + +} diff --git a/server/service/mdm.go b/server/service/mdm.go index 640f1dd6d7eb..8fdc0da52b3a 100644 --- a/server/service/mdm.go +++ b/server/service/mdm.go @@ -1426,12 +1426,12 @@ func (svc *Service) NewMDMWindowsConfigProfile(ctx context.Context, teamID uint, } if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{string(cp.SyncML)}); err != nil { - return nil, ctxerr.Wrap(ctx, err, "validating fleet secrets") + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("profile", err.Error())) } err = validateWindowsProfileFleetVariables(string(cp.SyncML)) if err != nil { - return nil, ctxerr.Wrap(ctx, err, "validating Windows profile") + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("profile", err.Error())) } newCP, err := svc.ds.NewMDMWindowsConfigProfile(ctx, cp) From 30c1a179da4c1f4f5e28bde113f6d6db2f0a9a9c Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Wed, 25 Dec 2024 14:40:31 -0600 Subject: [PATCH 2/5] Updated schema.sql --- server/datastore/mysql/schema.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/datastore/mysql/schema.sql b/server/datastore/mysql/schema.sql index aae0648b6fb9..89cd0c49c345 100644 --- a/server/datastore/mysql/schema.sql +++ b/server/datastore/mysql/schema.sql @@ -1660,8 +1660,8 @@ CREATE TABLE `secret_variables` ( `id` int unsigned NOT NULL AUTO_INCREMENT, `name` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL, `value` blob NOT NULL, - `created_at` timestamp(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), - `updated_at` timestamp(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6), + `created_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), + `updated_at` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6), PRIMARY KEY (`id`), UNIQUE KEY `idx_secret_variables_name` (`name`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; From 4025100e5f0935c7be3d0c244f48d5afdfc33d35 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Wed, 25 Dec 2024 14:51:19 -0600 Subject: [PATCH 3/5] Fix lint --- server/service/apple_mdm.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index 51f8bc82b501..aea85c0f9bd5 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -526,8 +526,7 @@ func (svc *Service) NewMDMAppleDeclaration(ctx context.Context, teamID uint, r i if err != nil { return nil, err } - // After validation, we should no longer need to keep the secrets, so we explicitly clear the variable holding them - dataWithSecrets = "" + // After validation, we should no longer need to keep the expanded secrets. if err := rawDecl.ValidateUserProvided(); err != nil { return nil, err From fcd99180aa70394b4cb53a0c824206119d8ca5fd Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Thu, 26 Dec 2024 07:35:49 -0600 Subject: [PATCH 4/5] Added profile deletion check. --- server/service/integration_mdm_profiles_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/server/service/integration_mdm_profiles_test.go b/server/service/integration_mdm_profiles_test.go index 8c085521e2f6..dfda495482e0 100644 --- a/server/service/integration_mdm_profiles_test.go +++ b/server/service/integration_mdm_profiles_test.go @@ -5342,7 +5342,9 @@ func (s *integrationMDMTestSuite) testSecretVariablesUpload(newProfileBytes func var listResp listMDMConfigProfilesResponse s.DoJSON("GET", "/api/latest/fleet/mdm/profiles", &listMDMConfigProfilesRequest{}, http.StatusOK, &listResp) require.Len(t, listResp.Profiles, 2) + profileUUIDs := make([]string, 0, 2) for _, p := range listResp.Profiles { + profileUUIDs = append(profileUUIDs, p.ProfileUUID) switch p.Name { case "secret-config0": assert.Equal(t, platform, p.Platform) @@ -5352,6 +5354,13 @@ func (s *integrationMDMTestSuite) testSecretVariablesUpload(newProfileBytes func t.Errorf("unexpected profile %s", p.Name) } } + require.Len(t, profileUUIDs, 2) + + s.Do("DELETE", "/api/latest/fleet/configuration_profiles/"+profileUUIDs[0], nil, http.StatusOK) + s.Do("DELETE", "/api/latest/fleet/configuration_profiles/"+profileUUIDs[1], nil, http.StatusOK) + s.DoJSON("GET", "/api/latest/fleet/mdm/profiles", &listMDMConfigProfilesRequest{}, http.StatusOK, &listResp) + require.Empty(t, listResp.Profiles) + } // TestAppleConfigSecretVariablesUpload tests uploading Apple config profiles with secrets via the /configuration_profiles endpoint From 4e298acbdba8ba62dfeb2e1c3c6237062a3aa5f9 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky Date: Thu, 26 Dec 2024 11:13:39 -0600 Subject: [PATCH 5/5] Added profile contents check. --- .../service/integration_mdm_profiles_test.go | 49 ++++++++++++++----- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/server/service/integration_mdm_profiles_test.go b/server/service/integration_mdm_profiles_test.go index dfda495482e0..9f626d930070 100644 --- a/server/service/integration_mdm_profiles_test.go +++ b/server/service/integration_mdm_profiles_test.go @@ -5288,13 +5288,21 @@ func (s *integrationMDMTestSuite) TestAppleDDMSecretVariablesUpload() { return []byte(fmt.Sprintf(tmpl, i, i, i)) } - s.testSecretVariablesUpload(newProfileBytes, "json", "darwin") + getProfileContents := func(profileUUID string) string { + profile, err := s.ds.GetMDMAppleDeclaration(context.Background(), profileUUID) + require.NoError(s.T(), err) + return string(profile.RawJSON) + } + + s.testSecretVariablesUpload(newProfileBytes, getProfileContents, "json", "darwin") } -func (s *integrationMDMTestSuite) testSecretVariablesUpload(newProfileBytes func(i int) []byte, fileExtension string, platform string) { +func (s *integrationMDMTestSuite) testSecretVariablesUpload(newProfileBytes func(i int) []byte, + getProfileContents func(profileUUID string) string, fileExtension string, platform string) { t := s.T() + const numProfiles = 2 var profiles [][]byte - for i := 0; i < 2; i++ { + for i := 0; i < numProfiles; i++ { profiles = append(profiles, newProfileBytes(i)) } // Use secrets @@ -5341,23 +5349,30 @@ func (s *integrationMDMTestSuite) testSecretVariablesUpload(newProfileBytes func var listResp listMDMConfigProfilesResponse s.DoJSON("GET", "/api/latest/fleet/mdm/profiles", &listMDMConfigProfilesRequest{}, http.StatusOK, &listResp) - require.Len(t, listResp.Profiles, 2) - profileUUIDs := make([]string, 0, 2) + require.Len(t, listResp.Profiles, numProfiles) + profileUUIDs := make([]string, numProfiles) for _, p := range listResp.Profiles { - profileUUIDs = append(profileUUIDs, p.ProfileUUID) switch p.Name { case "secret-config0": assert.Equal(t, platform, p.Platform) + profileUUIDs[0] = p.ProfileUUID case "secret-config1": assert.Equal(t, platform, p.Platform) + profileUUIDs[1] = p.ProfileUUID default: t.Errorf("unexpected profile %s", p.Name) } } - require.Len(t, profileUUIDs, 2) - s.Do("DELETE", "/api/latest/fleet/configuration_profiles/"+profileUUIDs[0], nil, http.StatusOK) - s.Do("DELETE", "/api/latest/fleet/configuration_profiles/"+profileUUIDs[1], nil, http.StatusOK) + // Check that contents are masking secret values + for i := 0; i < numProfiles; i++ { + assert.Equal(t, string(profiles[i]), getProfileContents(profileUUIDs[i])) + } + + // Delete profiles -- make sure there is no issue deleting profiles with secrets + for i := 0; i < numProfiles; i++ { + s.Do("DELETE", "/api/latest/fleet/configuration_profiles/"+profileUUIDs[i], nil, http.StatusOK) + } s.DoJSON("GET", "/api/latest/fleet/mdm/profiles", &listMDMConfigProfilesRequest{}, http.StatusOK, &listResp) require.Empty(t, listResp.Profiles) @@ -5406,7 +5421,13 @@ func (s *integrationMDMTestSuite) TestAppleConfigSecretVariablesUpload() { return []byte(fmt.Sprintf(tmpl, i, i, i)) } - s.testSecretVariablesUpload(newProfileBytes, "mobileconfig", "darwin") + getProfileContents := func(profileUUID string) string { + profile, err := s.ds.GetMDMAppleConfigProfile(context.Background(), profileUUID) + require.NoError(s.T(), err) + return string(profile.Mobileconfig) + } + + s.testSecretVariablesUpload(newProfileBytes, getProfileContents, "mobileconfig", "darwin") } @@ -5430,6 +5451,12 @@ func (s *integrationMDMTestSuite) TestWindowsConfigSecretVariablesUpload() { return []byte(fmt.Sprintf(tmpl, i, i, i)) } - s.testSecretVariablesUpload(newProfileBytes, "xml", "windows") + getProfileContents := func(profileUUID string) string { + profile, err := s.ds.GetMDMWindowsConfigProfile(context.Background(), profileUUID) + require.NoError(s.T(), err) + return string(profile.SyncML) + } + + s.testSecretVariablesUpload(newProfileBytes, getProfileContents, "xml", "windows") }