diff --git a/changes/fix-undeletable-ddm-declarations.md b/changes/fix-undeletable-ddm-declarations.md new file mode 100644 index 0000000000..496d0b0b43 --- /dev/null +++ b/changes/fix-undeletable-ddm-declarations.md @@ -0,0 +1 @@ +- Fixed a bug where an Apple configuration profile (DDM declaration) could become undeletable if the set of allowed declaration types changed after the profile was added (for example, when a server configuration flag was toggled). Deleting a profile no longer re-runs upload-time validation. diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index a3a5bb6dea..e028ecf35b 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -1969,6 +1969,14 @@ func (svc *Service) DeleteMDMAppleDeclaration(ctx context.Context, declUUID stri // Check if the declaration contains a secret variable. If it does, this means that the declaration // has been provided by the user and can be deleted. We don't need to validate that it is a Fleet declaration. + // + // Whether a declaration is Fleet-managed (and therefore protected from + // deletion through this endpoint) is determined solely by its reserved name. + // We deliberately do NOT run the upload-time validator (ValidateUserProvided) + // here: any declaration already stored was accepted at upload time, so + // re-validating it on delete would trap user-uploaded declarations whenever + // the accepted set later shrinks (a config flag is toggled, or a type is + // added to ForbiddenDeclTypes). See https://github.com/fleetdm/fleet/issues/47535. hasSecretVariable := len(fleet.ContainsPrefixVars(string(decl.RawJSON), fleet.ServerSecretPrefix)) > 0 if !hasSecretVariable { if _, ok := mdm_types.FleetReservedProfileNames()[decl.Name]; ok { @@ -1977,21 +1985,6 @@ func (svc *Service) DeleteMDMAppleDeclaration(ctx context.Context, declUUID stri InternalErr: fmt.Errorf("deleting profile %s is not allowed because it's managed by Fleet", decl.Name), } } - - // TODO: refine our approach to deleting restricted/forbidden types of declarations so that we - // can check that Fleet-managed aren't being deleted; this can be addressed once we add support - // for more types of declarations - var d fleet.MDMAppleRawDeclaration - if err := json.Unmarshal(decl.RawJSON, &d); err != nil { - return ctxerr.Wrap(ctx, err, "unmarshalling declaration") - } - - // skip declaration validation if the allow all declarations flag is set. - if !svc.config.MDM.AllowAllDeclarations { - if err := d.ValidateUserProvided(); err != nil { - return ctxerr.Wrap(ctx, &fleet.BadRequestError{Message: err.Error()}) - } - } } teamID, teamName, err := svc.resolveProfileTeam(ctx, decl.TeamID) diff --git a/server/service/integration_mdm_profiles_test.go b/server/service/integration_mdm_profiles_test.go index a1400c3809..27fa1d23bf 100644 --- a/server/service/integration_mdm_profiles_test.go +++ b/server/service/integration_mdm_profiles_test.go @@ -3787,7 +3787,60 @@ func (s *integrationMDMTestSuite) TestMDMConfigProfileCRUD() { return err }) } - // TODO: Add tests for create/delete forbidden declaration types? + // A declaration whose type would now fail upload validation — e.g. it was + // accepted before its type was added to ForbiddenDeclTypes, or before a + // config flag was toggled — must still be deletable. Deletion does not + // re-run the upload-time validator; doing so would trap declarations the + // user already uploaded. Regression test for + // https://github.com/fleetdm/fleet/issues/47535. + { + forbiddenType := "com.apple.configuration.watch.enrollment" + require.Contains(t, fleet.ForbiddenDeclTypes, forbiddenType) // guard: type is genuinely forbidden on upload + + // The delete-time validation this fix removed only ran under strict + // validation (AllowAllDeclarations == false), so this test only guards + // against its reintroduction when the suite runs strict. Assert that + // precondition explicitly so the test can't silently become a no-op if + // the suite default ever changes. + require.False(t, s.fleetCfg.MDM.AllowAllDeclarations) + + declUUID := fleet.MDMAppleDeclarationUUIDPrefix + uuid.NewString() + rawJSON := fmt.Sprintf(`{"Type":%q,"Identifier":"com.fleet.forbidden-type","Payload":{}}`, forbiddenType) + mysqltest.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, + "INSERT INTO mdm_apple_declarations (declaration_uuid, identifier, name, raw_json, scope, uploaded_at, team_id) VALUES (?, ?, ?, ?, ?, NOW(6), 0)", + declUUID, "com.fleet.forbidden-type", "forbidden-type-decl", rawJSON, fleet.PayloadScopeSystem) + return err + }) + + var deleteResp deleteMDMConfigProfileResponse + s.DoJSON("DELETE", fmt.Sprintf("/api/latest/fleet/configuration_profiles/%s", declUUID), nil, http.StatusOK, &deleteResp) + } + + // A declaration that uses a Fleet-reserved name stays protected from deletion + // through this endpoint. The reserved-name check is the sole Fleet-managed + // gate on the delete path, so make sure removing the upload-time validation + // (above) didn't open a hole. + { + reservedName := servermdm.FleetMacOSUpdatesProfileName + declUUID := fleet.MDMAppleDeclarationUUIDPrefix + uuid.NewString() + rawJSON := `{"Type":"com.apple.configuration.softwareupdate.enforcement.specific","Identifier":"com.fleet.reserved","Payload":{}}` + mysqltest.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, + "INSERT INTO mdm_apple_declarations (declaration_uuid, identifier, name, raw_json, scope, uploaded_at, team_id) VALUES (?, ?, ?, ?, ?, NOW(6), 0)", + declUUID, "com.fleet.reserved", reservedName, rawJSON, fleet.PayloadScopeSystem) + return err + }) + + var deleteResp deleteMDMConfigProfileResponse + s.DoJSON("DELETE", fmt.Sprintf("/api/latest/fleet/configuration_profiles/%s", declUUID), nil, http.StatusBadRequest, &deleteResp) + + // the API refused to delete it, so remove the seeded row directly + mysqltest.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, "DELETE FROM mdm_apple_declarations WHERE declaration_uuid = ?", declUUID) + return err + }) + } // make fleet add a FileVault profile acResp := appConfigResponse{}