Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes issue verifying Windows CSP profiles that contain ADMX policies. #25528

Merged
merged 7 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/24790-admx-policies
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes issue verifying Windows CSP profiles that contain ADMX policies.
104 changes: 104 additions & 0 deletions server/mdm/microsoft/admx/admx.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// Package admx handles ADMX (Administrative Template File) policies for Microsoft MDM server.
// See: https://learn.microsoft.com/en-us/windows/client-management/understanding-admx-backed-policies
//
// ADMX policy payload example:
// <![CDATA[
//
// <enabled/>
// <data id="Publishing_Server2_Name_Prompt" value="Name"/>
// <data id="Publishing_Server_URL_Prompt" value="http://someuri"/>
// <data id="Global_Publishing_Refresh_Options" value="1"/>
//
// ]]>
package admx

import (
"encoding/xml"
"fmt"
"slices"
"strings"
)

func IsADMX(text string) bool {
// We try to unmarshal the string to see if it looks like a valid ADMX policy
policy, err := unmarshal(text)
if err != nil {
return false
}
return policy.Enabled.Local == "enabled" || policy.Disabled.Local == "disabled" || len(policy.Data) > 0
}

func Equal(a, b string) (bool, error) {
aPolicy, err := unmarshal(a)
if err != nil {
return false, fmt.Errorf("unmarshalling ADMX policy a: %w", err)
}
bPolicy, err := unmarshal(b)
if err != nil {
return false, fmt.Errorf("unmarshalling ADMX policy b: %w", err)
}
return aPolicy.Equal(bPolicy), nil
}

func unmarshal(a string) (admxPolicy, error) {
// We unmarshal into a string to get the CDATA content and decode XML escape characters.
// We wrap the policy in an <admx> tag to ensure it can be unmarshalled by the XML decoder.
var unescaped string
err := xml.Unmarshal([]byte(`<admx>`+a+`</admx>`), &unescaped)
if err != nil {
return admxPolicy{}, fmt.Errorf("unmarshalling ADMX policy to string: %w", err)
}
// ADMX policy elements are not case-sensitive. For example: <enabled/> and <Enabled/> are equivalent
// For simplicity, we compare everything in lowercase.
var policy admxPolicy
err = xml.Unmarshal([]byte(`<admx>`+strings.ToLower(unescaped)+`</admx>`), &policy)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't unescaped already include the <admx/> tags? Is it intentional to wrap in those <admx/> again here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, unescaped looks like: <enabled/><data id="Item" value="foo"/>

I don't know how to unmarshal it without adding a root XML element.

<admx> is just a random throw-away name. It could be <bozo>

if err != nil {
return admxPolicy{}, fmt.Errorf("unmarshalling ADMX policy: %w", err)
}
return policy, nil
}

type admxPolicy struct {
Enabled xml.Name `xml:"enabled,omitempty"`
Disabled xml.Name `xml:"disabled,omitempty"`
Data []admxPolicyItem `xml:"data"`
}

func (a admxPolicy) Equal(b admxPolicy) bool {
if a.Disabled.Local != b.Disabled.Local {
return false
}
if a.Disabled.Local == "disabled" {
// If the ADMX policy is disabled, the data is not relevant
return true
}
if a.Enabled.Local != b.Enabled.Local {
return false
}
if len(a.Data) != len(b.Data) {
return false
}
a.sortData()
b.sortData()
for i := range a.Data {
if !a.Data[i].Equal(b.Data[i]) {
return false
}
}
return true
}

func (a *admxPolicy) sortData() {
slices.SortFunc(a.Data, func(i, j admxPolicyItem) int {
return strings.Compare(i.ID, j.ID)
})
}

type admxPolicyItem struct {
ID string `xml:"id,attr"`
Value string `xml:"value,attr"`
}

func (a admxPolicyItem) Equal(b admxPolicyItem) bool {
return a.ID == b.ID && a.Value == b.Value
}
174 changes: 174 additions & 0 deletions server/mdm/microsoft/admx/admx_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
package admx

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestIsADMX(t *testing.T) {
t.Parallel()
assert.False(t, IsADMX(""))
assert.False(t, IsADMX("not an ADMX policy"))
assert.False(t, IsADMX(`<![CDATA[bozo]]>`))
assert.False(t, IsADMX(`<![CDATA[<bozo/>]]>`))
assert.False(t, IsADMX(`<![CDATA[<bozo]]>`))
assert.True(t, IsADMX(`<![CDATA[<enabled/>]]>`))
assert.True(t, IsADMX(`<![CDATA[<disabled/>]]>`))
assert.True(t, IsADMX(`<![CDATA[<data id="id" value="value"/>]]>`))
assert.True(t, IsADMX(
` <![CDATA[
<enabled/>
]]>`))
assert.True(t,
IsADMX("&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;"))
assert.True(t, IsADMX(
`&lt;Enabled/&gt;
<![CDATA[<data id="ExecutionPolicy" value="AllSigned"/>]]>
<![CDATA[<data id="Listbox_ModuleNames" value="*"/>
<data id="OutputDirectory" value="false"/>
<data id="EnableScriptBlockInvocationLogging" value="true"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`))
}

func TestEqual(t *testing.T) {
t.Parallel()
testCases := []struct {
name, a, b, errorContains string
equal bool
}{
{
name: "empty policies",
a: "",
b: "",
equal: true,
errorContains: "",
},
{
name: "enabled policies",
a: "<![CDATA[<enabled/>]]>",
b: "&lt;Enabled/&gt;",
equal: true,
errorContains: "",
},
{
name: "disabled policies",
a: "<![CDATA[<disabled/>]]>",
b: "&lt;Disabled/&gt;",
equal: true,
errorContains: "",
},
{
name: "unequal policies",
a: "<![CDATA[<disabled/>]]>",
b: "&lt;enabled/&gt;",
equal: false,
errorContains: "",
},
{
name: "enabled policies with data",
a: `<![CDATA[<enabled/>
<data id="ExecutionPolicy" value="AllSigned"/>
<data id="Listbox_ModuleNames" value="*"/>
<data id="OutputDirectory" value="false"/>
<data id="EnableScriptBlockInvocationLogging" value="true"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`,
b: "&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;",
equal: true,
errorContains: "",
},
{
name: "enabled policies with data and nonstandard format",
a: `&lt;Enabled/&gt;
<![CDATA[<data id="ExecutionPolicy" value="AllSigned"/>]]>
<![CDATA[<data id="Listbox_ModuleNames" value="*"/>
<data id="OutputDirectory" value="false"/>
<data id="EnableScriptBlockInvocationLogging" value="true"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`,
b: "&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;",
equal: true,
errorContains: "",
},
{
name: "disabled policies with data",
a: `<![CDATA[<disabled/>
<data id="ExecutionPolicy" value="AllSigned"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`,
b: "&lt;Disabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;",
equal: true,
errorContains: "",
},
{
name: "unparsable policy a 1",
a: "<bozo",
b: "",
equal: false,
errorContains: "unmarshalling ADMX policy",
},
{
name: "unparsable policy a 2",
a: "&lt;bozo",
b: "",
equal: false,
errorContains: "unmarshalling ADMX policy",
},
{
name: "unparsable policy b 1",
a: "",
b: "<bozo",
equal: false,
errorContains: "unmarshalling ADMX policy",
},
{
name: "unparsable policy b 2",
a: "",
b: "&lt;bozo",
equal: false,
errorContains: "unmarshalling ADMX policy",
},
{
name: "unequal policies with missing enable",
a: `<![CDATA[<Xenabled/>
<data id="ExecutionPolicy" value="AllSigned"/>
<data id="Listbox_ModuleNames" value="*"/>
<data id="OutputDirectory" value="false"/>
<data id="EnableScriptBlockInvocationLogging" value="true"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`,
b: "&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;",
equal: false,
errorContains: "",
},
{
name: "unequal policies with data 1",
a: `<![CDATA[<enabled/>
<data id="EnableScriptBlockInvocationLogging" value="true"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`,
b: "&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;",
equal: false,
errorContains: "",
},
{
name: "unequal policies with data 2",
a: `<![CDATA[<enabled/>
<data id="ExecutionPolicy" value="XXXX"/>
<data id="Listbox_ModuleNames" value="*"/>
<data id="OutputDirectory" value="false"/>
<data id="EnableScriptBlockInvocationLogging" value="true"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`,
b: "&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;",
equal: false,
errorContains: "",
},
}
for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
equal, err := Equal(tt.a, tt.b)
if tt.errorContains == "" {
assert.NoError(t, err)
} else {
assert.ErrorContains(t, err, tt.errorContains)
}
assert.Equal(t, tt.equal, equal)
})
}
}
32 changes: 25 additions & 7 deletions server/mdm/microsoft/profile_verifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
"github.com/fleetdm/fleet/v4/server/fleet"
"github.com/fleetdm/fleet/v4/server/mdm"
"github.com/fleetdm/fleet/v4/server/mdm/microsoft/admx"
)

// LoopOverExpectedHostProfiles loops all the <LocURI> values on all the profiles for a
Expand Down Expand Up @@ -72,20 +73,24 @@ func HashLocURI(profileName, locURI string) string {
func VerifyHostMDMProfiles(ctx context.Context, ds fleet.ProfileVerificationStore, host *fleet.Host, rawProfileResultsSyncML []byte) error {
profileResults, err := transformProfileResults(rawProfileResultsSyncML)
if err != nil {
return err
return ctxerr.Wrap(ctx, err, "transforming policy results")
}

verified, missing, err := compareResultsToExpectedProfiles(ctx, ds, host, profileResults)
if err != nil {
return err
return ctxerr.Wrap(ctx, err, "comparing results to expected profiles")
}

toFail, toRetry, err := splitMissingProfilesIntoFailAndRetryBuckets(ctx, ds, host, missing, verified)
if err != nil {
return err
return ctxerr.Wrap(ctx, err, "splitting missing profiles into fail and retry buckets")
}

return ds.UpdateHostMDMProfilesVerification(ctx, host, slices.Collect(maps.Keys(verified)), toFail, toRetry)
err = ds.UpdateHostMDMProfilesVerification(ctx, host, slices.Collect(maps.Keys(verified)), toFail, toRetry)
if err != nil {
return ctxerr.Wrap(ctx, err, "updating host mdm profiles during verification")
}
return nil
}

func splitMissingProfilesIntoFailAndRetryBuckets(ctx context.Context, ds fleet.ProfileVerificationStore, host *fleet.Host,
Expand Down Expand Up @@ -127,19 +132,32 @@ func compareResultsToExpectedProfiles(ctx context.Context, ds fleet.ProfileVerif
missing = map[string]struct{}{}
verified = map[string]struct{}{}
err = LoopOverExpectedHostProfiles(ctx, ds, host, func(profile *fleet.ExpectedMDMProfile, ref, locURI, wantData string) {
// if we didn't get a status for a LocURI, mark the profile as
// missing.
// if we didn't get a status for a LocURI, mark the profile as missing.
gotStatus, ok := profileResults.cmdRefToStatus[ref]
if !ok {
missing[profile.Name] = struct{}{}
return
}
// it's okay if we didn't get a result
gotResults := profileResults.cmdRefToResult[ref]
// non-200 status don't have results. Consider it failed
// TODO: should we be more granular instead? eg: special case
// `4xx` responses? I'm sure there are edge cases we're not
// accounting for here, but it's unclear at this moment.
if !strings.HasPrefix(gotStatus, "2") || wantData != gotResults {
var equal bool
switch {
case !strings.HasPrefix(gotStatus, "2"):
equal = false
case wantData == gotResults:
equal = true
case admx.IsADMX(wantData):
equal, err = admx.Equal(wantData, gotResults)
if err != nil {
err = fmt.Errorf("comparing ADMX policies: %w", err)
return
}
}
if !equal {
withinGracePeriod := profile.IsWithinGracePeriod(host.DetailUpdatedAt)
if !withinGracePeriod {
missing[profile.Name] = struct{}{}
Expand Down
35 changes: 35 additions & 0 deletions server/mdm/microsoft/profile_verifier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,41 @@ func TestVerifyHostMDMProfilesHappyPaths(t *testing.T) {
toFail: []string{"N2", "N4"},
toRetry: []string{"N1"},
},
{
name: "single profile with CDATA reported and verified",
hostProfiles: []hostProfile{
{"N1", syncml.ForTestWithData(map[string]string{
"L1": `
<![CDATA[<enabled/><data id="ExecutionPolicy" value="AllSigned"/>
<data id="Listbox_ModuleNames" value="*"/>
<data id="OutputDirectory" value="false"/>
<data id="EnableScriptBlockInvocationLogging" value="true"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`,
}), 0},
},
report: []osqueryReport{{"N1", "200", "L1",
"&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;",
}},
toVerify: []string{"N1"},
toFail: []string{},
toRetry: []string{},
},
{
name: "single profile with CDATA to retry",
hostProfiles: []hostProfile{
{"N1", syncml.ForTestWithData(map[string]string{
"L1": `
<![CDATA[<enabled/><data id="ExecutionPolicy" value="AllSigned"/>
<data id="SourcePathForUpdateHelp" value="false"/>]]>`,
}), 0},
},
report: []osqueryReport{{"N1", "200", "L1",
"&lt;Enabled/&gt;&lt;Data id=\"EnableScriptBlockInvocationLogging\" value=\"true\"/&gt;&lt;Data id=\"ExecutionPolicy\" value=\"AllSigned\"/&gt;&lt;Data id=\"Listbox_ModuleNames\" value=\"*\"/&gt;&lt;Data id=\"OutputDirectory\" value=\"false\"/&gt;&lt;Data id=\"SourcePathForUpdateHelp\" value=\"false\"/&gt;",
}},
toVerify: []string{},
toFail: []string{},
toRetry: []string{"N1"},
},
}

for _, tt := range cases {
Expand Down
Loading