Update WLAN XML profile verification so they aren't resent #28296

JordanMontgomery · 2025-04-16T17:31:53Z

Fixes #24394 by adding new verification logic to detect and verify these profiles. We only verify a subset of the properties because there are certain settings such as the Authentication which Windows seems to upgrade in circumstances where it can(e.g. WPA2 specified but interface + router supports WPA3 results in WPA3 on the client and there are likely other similar scenarios). After discussion with design team we've decided the limited verification is better than what we had before and a good solution for now.

I know this is extremely heavy on comments but the behavior is strange and non obvious.

Also see latest comment on the issue for some testing discussion: #24394 (comment)

Checklist for submitter

If some of the following don't apply, delete the relevant line.

Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
See Changes files for more information.
Added/updated automated tests
A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it)
Manual QA for all new/changed functionality

codecov · 2025-04-16T17:45:41Z

Codecov Report

Attention: Patch coverage is 82.40000% with 22 lines in your changes missing coverage. Please review.

Project coverage is 63.98%. Comparing base (183d0d8) to head (84df57c).
Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
server/mdm/microsoft/wlanxml/wlanxml.go	83.01%	12 Missing and 6 partials ⚠️
server/mdm/microsoft/profile_verifier.go	73.33%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #28296   +/-   ##
=======================================
  Coverage   63.98%   63.98%           
=======================================
  Files        1783     1784    +1     
  Lines      171258   171327   +69     
  Branches     4945     4945           
=======================================
+ Hits       109576   109625   +49     
- Misses      53036    53054   +18     
- Partials     8646     8648    +2

Flag	Coverage Δ
backend	`65.03% <82.40%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mna

Looks great! I think the comments are very useful given that it's complementary to the code and provides insights into why that custom Equal stuff is even required. Just had some minor things as feedback, none are blockers to merge (which is why I also "Approve", meaning - up to you to apply the suggestions here, in a later PR or not at all).

orbit/pkg/table/mdm/mdm_windows.go

mna · 2025-04-16T18:39:33Z

server/mdm/microsoft/admx/admx_test.go

+
+	// This is a WLAN XML profile
+	assert.False(t, IsADMX(
+		`&lt;?xml version=&quot;1.0&quot;?&gt;


Just curious (I don't know what is ADMX?), why does the XML has to be escaped here? (i.e. why don't we send <?xml...?)

Oh I think it's because that's how we receive it from the host for verification, right?

Correct. WE get an XML file with a big XMLEncoded blob in it which is itself an XML file and likewise we send something similar.

mna · 2025-04-16T18:44:09Z

server/mdm/microsoft/wlanxml/wlanxml.go

+type wlanXmlProfileSSIDConfig struct {
+	SSID []wlanXmlProfileSSID `xml:"SSID"`
+	// Note that this field is optional so if we ever do more inspection of these policies
+	// we likely need to


Feels like there was more to come here?

Good catch. I wrote so many comments trying to capture all the strangeness I was trying to account for I missed this incomplete one.

Actually removed comment since it is no longer relevant

mna · 2025-04-16T18:48:41Z

server/mdm/microsoft/wlanxml/wlanxml_test.go

+		IsWLANXML("&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.False(t, IsWLANXML(admxPolicy))
+
+	assert.True(t, IsWLANXML(xmlEncodedProfile1))


Nit: this test would work nicely as table-driven (simple input -> expected output test cases).

Refactored this as a table based test

getvictor · 2025-04-16T21:18:54Z

I went over the PR with Jordan. I can re-review after the next set of changes.

getvictor · 2025-04-17T15:03:23Z

@JordanMontgomery When the diff fails, do we log the specific error (what is wrong) or print the error in the UI? This info would be useful if we have to debug this in production.

JordanMontgomery · 2025-04-17T17:48:16Z

@JordanMontgomery When the diff fails, do we log the specific error (what is wrong) or print the error in the UI? This info would be useful if we have to debug this in production.

We don't currently do this for any profile types as best I can tell except in the really bad cases where we completely fail to unmarshal(which shouldn't be common). Right now the error we show in the UI For these generally comes from us trying to re-Add the profile. I think as the code is currently written the easiest way to debug is probably to capture the fleetd logs which show the OSQuery output and allow comparing the profiles, but that is obviously not great.

We could for all failed profiles add a debug log where we log the profiles(expected + received. My only concern with that would be the possibility we leak secrets into the logs

getvictor

LGTM

For #9943 This will help us avoid issues like this where the log message never worked right: #28296 (comment) Most of the changes are no-op type changes like removing unneeded typecast or disabling gosec on reviewed lines of code # Checklist for submitter If some of the following don't apply, delete the relevant line.  - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated automated tests - [x] A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it) - [x] Manual QA for all new/changed functionality - For Orbit and Fleet Desktop changes: - [x] Make sure fleetd is compatible with the latest released version of Fleet (see [Must rule](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/fleetd-development-and-release-strategy.md)). - [x] Orbit runs on macOS, Linux and Windows. Check if the orbit feature/bugfix should only apply to one platform (`runtime.GOOS`). - [x] Manual QA must be performed in the three main OSs, macOS, Windows and Linux. - [x] Auto-update manual QA, from released version of component to new version (see [tools/tuf/test](../tools/tuf/test/README.md)).

JordanMontgomery added 3 commits April 16, 2025 11:44

Update Windows wireless profile verification

731373d

Remove debug code

bfb0aa3

Update SSID name verification logic

6fa6980

JordanMontgomery requested a review from a team as a code owner April 16, 2025 17:31

JordanMontgomery temporarily deployed to Docker Hub April 16, 2025 17:32 — with GitHub Actions Inactive

Fix lint issues

f22eff4

JordanMontgomery temporarily deployed to Docker Hub April 16, 2025 17:47 — with GitHub Actions Inactive

Update test descriptions

117ad5f

JordanMontgomery temporarily deployed to Docker Hub April 16, 2025 18:17 — with GitHub Actions Inactive

mna previously approved these changes Apr 16, 2025

View reviewed changes

JordanMontgomery added 2 commits April 16, 2025 15:11

Remove irrelevant comment

dab4cf3

Refactor as table based test

474eeef

JordanMontgomery dismissed mna’s stale review via 474eeef April 16, 2025 19:26

JordanMontgomery temporarily deployed to Docker Hub April 16, 2025 19:27 — with GitHub Actions Inactive

mna previously approved these changes Apr 16, 2025

View reviewed changes

Address review comments in wlanxml package

f8e1284

JordanMontgomery added 2 commits April 17, 2025 13:30

Extract profile generation function for use by other tests

81a4ca1

Update tests to use Add commands too

84df57c

JordanMontgomery dismissed mna’s stale review via 84df57c April 17, 2025 17:31

JordanMontgomery temporarily deployed to Docker Hub April 17, 2025 17:31 — with GitHub Actions Inactive

JordanMontgomery requested a review from getvictor April 17, 2025 17:48

getvictor approved these changes Apr 17, 2025

View reviewed changes

JordanMontgomery merged commit de7ce43 into main Apr 18, 2025
52 checks passed

JordanMontgomery deleted the JM-24394-wlan-xml-profile-verification branch April 18, 2025 12:45

JordanMontgomery mentioned this pull request Apr 30, 2025

Fix Windows lint issues and enable linting on Windows #28704

Merged

8 tasks

Update WLAN XML profile verification so they aren't resent #28296

Update WLAN XML profile verification so they aren't resent #28296

Uh oh!

Conversation

JordanMontgomery commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for submitter

Uh oh!

codecov bot commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

getvictor commented Apr 16, 2025

Uh oh!

getvictor commented Apr 17, 2025

Uh oh!

JordanMontgomery commented Apr 17, 2025

Uh oh!

getvictor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JordanMontgomery commented Apr 16, 2025 •

edited

Loading

codecov bot commented Apr 16, 2025 •

edited

Loading