Added verification support for $FLEET_VAR_HOST_UUID - #31777
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change centralizes Fleet variable detection, parsing, and replacement logic into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant FleetServer
participant VariablesPkg
participant LicenseChecker
participant Host
Admin->>FleetServer: Uploads MDM profile with $FLEET_VAR_HOST_UUID
FleetServer->>VariablesPkg: Find variables in profile
VariablesPkg-->>FleetServer: Returns detected variables
FleetServer->>LicenseChecker: Check if premium license required
LicenseChecker-->>FleetServer: Returns license status
alt License valid
FleetServer->>VariablesPkg: Replace $FLEET_VAR_HOST_UUID with host UUID
VariablesPkg-->>FleetServer: Returns processed profile
FleetServer->>Host: Sends processed profile
else License invalid
FleetServer-->>Admin: Returns error (premium required)
end
Host->>FleetServer: Reports profile status
FleetServer->>Admin: Shows profile verification status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (17)
server/variables/variables.go (2)
47-61: Avoid recomputing named-group indices on every call
FindKeepDuplicatesrebuildsnameToIndexfor each invocation. Cache the subexp indices at package init to reduce per-call overhead.Apply this diff within the function and use precomputed indices:
- nameToIndex := make(map[string]int, 2) - for i, name := range fleetVariableRegex.SubexpNames() { - if name == "" { - continue - } - nameToIndex[name] = i - } - - for _, match := range matches { - for _, i := range nameToIndex { - if match[i] != "" { - result = append(result, match[i]) - } - } - } + for _, match := range matches { + if idxName1 >= 0 && match[idxName1] != "" { + result = append(result, match[idxName1]) + continue + } + if idxName2 >= 0 && match[idxName2] != "" { + result = append(result, match[idxName2]) + } + }Add these cached indices once near the regex declarations:
var ( idxName1 = fleetVariableRegex.SubexpIndex("name1") idxName2 = fleetVariableRegex.SubexpIndex("name2") )
87-90: Expose a bytes-based Contains to avoid allocations at call sitesMany callers have []byte. Offer
ContainsBytes([]byte)to skip converting to string.Add this next to
Contains:func Contains(contents string) bool { return fleetVariableRegex.MatchString(contents) } + +// ContainsBytes checks if the given content contains any Fleet variables (bytes version). +func ContainsBytes(contents []byte) bool { + return fleetVariableRegex.Match(contents) +}server/service/appconfig.go (1)
1411-1468: De-duplicate allowed-variable validation across CACN/SeatID/UPNsThe same allowed-set loop appears in three validators. Consider extracting a small helper to validate an input string against the allowed set to reduce duplication and future drift.
For example:
func validateAllowedFleetVars(input string, field string, allowed map[string]struct{}, invalid *fleet.InvalidArgumentError) bool { vars := variables.Find(input) for v := range vars { if _, ok := allowed[v]; !ok { invalid.Append(field, "FLEET_VAR_"+v+" is not allowed in "+field) return false } } return true }Then call it with the appropriate field and allowed names for each validator.
server/mdm/apple/mobileconfig/mobileconfig.go (2)
123-125: Avoid string conversion when checking for variables in signed profilesUse a bytes-based contains to skip allocation. If you add
variables.ContainsBytes, this becomes a cheapMatch.- if variables.Contains(string(mcBytes)) { + if variables.ContainsBytes(mcBytes) { return nil, errors.New("a signed profile cannot contain Fleet variables ($FLEET_VAR_*)") }
175-177: Repeat suggestion: use bytes-based containsSame as above; switch to
variables.ContainsBytes(mcBytes)if added.- if variables.Contains(string(mcBytes)) { + if variables.ContainsBytes(mcBytes) { return nil, errors.New("a signed profile cannot contain Fleet variables ($FLEET_VAR_*)") }server/service/mdm_test.go (1)
2251-2287: Make error assertion robust to wrappingUse
require.ErrorIsinstead ofrequire.Equalfor license checks to tolerate wrapped errors.- require.Error(t, err) - require.Equal(t, fleet.ErrMissingLicense, err) + require.ErrorIs(t, err, fleet.ErrMissingLicense)server/mdm/microsoft/profile_verifier_test.go (1)
824-893: Optional: also assert the processed XML remains well-formedAs a safety net, you could unmarshal the resulting XML into a minimal struct (or decode to a token stream) to ensure replacements never break XML structure.
server/service/apple_mdm_test.go (1)
5082-5138: Strengthen assertions and tolerate empty map; consider t.Parallel()
- Use ErrorIs for sentinel errors to be robust against wrapping.
- Prefer Empty over Nil for maps (implementation may return an empty map).
- Optional: add t.Parallel() at the test start for consistency with adjacent tests.
func TestValidateConfigProfileFleetVariablesLicense(t *testing.T) { + t.Parallel() t.Run("requires premium license", func(t *testing.T) { @@ - require.Equal(t, fleet.ErrMissingLicense, err) + require.ErrorIs(t, err, fleet.ErrMissingLicense) @@ - vars, err = validateConfigProfileFleetVariables(appConfig, profileNoVars, freeLic) + vars, err = validateConfigProfileFleetVariables(appConfig, profileNoVars, freeLic) require.NoError(t, err) - require.Nil(t, vars) + require.Empty(t, vars) }) }server/service/microsoft_mdm.go (1)
2306-2307: Optional: use Contains when only presence mattersSince you only branch on whether any Fleet variable exists, using Contains avoids building a map:
- fleetVars := variables.Find(profileStr) - if len(fleetVars) == 0 { + if !variables.Contains(profileStr) { // No Fleet variables...server/variables/variables_test.go (1)
9-80: Solid coverage for Find and Contains; add a false-positive guardConsider adding a case to ensure near-miss patterns don't match, e.g. "$FLEET_VARX_HOST_UUID" or "${FLEET_VAR_HOST_UUID" (missing brace), to guard against overmatching.
server/service/apple_mdm.go (2)
405-411: Consider simplifying license retrieval flowFetching license info here and passing it down is fine. If you end up needing license checks elsewhere for Apple profiles, consider centralizing inside the validator or accepting a context and using
license.IsPremium(ctx)for consistency across the codebase.
5293-5299: Scoped variable replacement in CA items uses new finderRefactor to
variables.Findis consistent. Consider renamingcaFleetVarstovarsorfoundVarsfor readability, but optional.server/service/integration_mdm_profiles_test.go (3)
7263-7278: Mark test helper as a helper and keep failure locations crispAdd t.Helper() at the start so failures point to the call site. The lookup-by-name is fine here, but if you expect future reuse, consider keying by profile UUID to avoid name collisions.
- checkHostProfileStatus := func(hostUUID string, profileName string, expectedStatus fleet.MDMDeliveryStatus) { + checkHostProfileStatus := func(hostUUID string, profileName string, expectedStatus fleet.MDMDeliveryStatus) { + t.Helper() profiles, err := s.ds.GetHostMDMWindowsProfiles(ctx, hostUUID) require.NoError(t, err)
7348-7401: Tighten verification signal in simulateOsqueryProfileReportCurrent matching relies on substring contains. To reduce false positives, assert LocURI matches and compare data for equality when possible.
- { - "fleet_detail_query_mdm_config_profiles_windows": { - {"raw_mdm_command_output": string(rawResponse)}, - }, - }, + { + "fleet_detail_query_mdm_config_profiles_windows": { + {"raw_mdm_command_output": string(rawResponse)}, + }, + },And when validating in verifyProfileSubstitution:
- if strings.Contains(item.Data.Content, expectedData) { + if item.Target != nil && *item.Target == locURI && item.Data.Content == expectedData {Note: You can plumb locURI into verifyProfileSubstitution to assert both dimensions.
7417-7425: Make the global profile swap explicit with an assertionYou rely on ProfileNoVars replacing GlobalProfileWithVar for global hosts. Add a short assertion to confirm the swap so future refactors don’t silently change this behavior.
// Note: GlobalProfileWithVar was replaced by ProfileNoVars for global hosts // since both are global profiles and Fleet only keeps one profile per host. // So we need to simulate osquery reporting ProfileNoVars for global hosts. + { + profs, err := s.ds.GetHostMDMWindowsProfiles(ctx, hostGlobal1.UUID) + require.NoError(t, err) + var names []string + for _, p := range profs { names = append(names, p.Name) } + require.Contains(t, names, "ProfileNoVars") + require.NotContains(t, names, "GlobalProfileWithVar") + }server/service/mdm.go (2)
1769-1773: Fleet vars validation in batch path wired correctly.Passing license through to both Apple and Windows variable validators unifies enforcement. Consider documenting that profilesVariablesByIdentifierMap includes Apple entries even when no variables are found, while Windows entries are added only when variables exist, to avoid confusion.
1865-1895: validateFleetVariables: consistent behavior and clarity.
- Consistency: For Apple, you add an entry even when no variables were found (profileVars may be nil). For Windows, you add an entry only if len(windowsVars) > 0. Consider standardizing this behavior (either only add when >0 for both or always add an entry) to make downstream handling predictable.
- Naming: Using p.Name for Windows as identifier is fine (no PayloadIdentifier equivalent). A short comment noting this choice and its implications would help future readers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
server/mdm/apple/mobileconfig/mobileconfig.go(3 hunks)server/mdm/mdm.go(0 hunks)server/mdm/microsoft/profile_verifier.go(3 hunks)server/mdm/microsoft/profile_verifier_test.go(1 hunks)server/service/appconfig.go(4 hunks)server/service/apple_mdm.go(9 hunks)server/service/apple_mdm_test.go(2 hunks)server/service/integration_mdm_profiles_test.go(1 hunks)server/service/mdm.go(5 hunks)server/service/mdm_test.go(2 hunks)server/service/microsoft_mdm.go(3 hunks)server/service/microsoft_mdm_test.go(0 hunks)server/variables/variables.go(1 hunks)server/variables/variables_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- server/mdm/mdm.go
- server/service/microsoft_mdm_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit Configuration File
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/mdm/microsoft/profile_verifier_test.goserver/mdm/apple/mobileconfig/mobileconfig.goserver/service/appconfig.goserver/mdm/microsoft/profile_verifier.goserver/service/mdm_test.goserver/variables/variables.goserver/variables/variables_test.goserver/service/microsoft_mdm.goserver/service/mdm.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/integration_mdm_profiles_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: getvictor
PR: fleetdm/fleet#31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.467Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
📚 Learning: 2025-08-08T08:32:31.467Z
Learnt from: getvictor
PR: fleetdm/fleet#31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.467Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/mdm/microsoft/profile_verifier_test.goserver/mdm/apple/mobileconfig/mobileconfig.goserver/mdm/microsoft/profile_verifier.goserver/service/mdm_test.goserver/service/microsoft_mdm.goserver/service/mdm.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/integration_mdm_profiles_test.go
🔇 Additional comments (25)
server/variables/variables.go (1)
17-17: Regex pattern looks solid and covers both braced and non-braced formsCovers
$FLEET_VAR_*and${FLEET_VAR_*}with named groups for clean extraction. Good centralization.server/service/appconfig.go (1)
1411-1420: Switched to variables.Find — behavior is consistent and safe
variables.Findreturns names without prefix andrangeover a nil map is safe. Allowed set check is clear.server/mdm/apple/mobileconfig/mobileconfig.go (2)
116-117: Good: strip out DigiCert data variables before parsingUsing the centralized
variables.ProfileDataVariableRegexkeeps logic consistent across the codebase.
168-169: Good duplication of DigiCert data variable stripping in payloadSummaryConsistent with ParseConfigProfile; avoids false positives due to DigiCert placeholders.
server/service/mdm_test.go (2)
2387-2390: Signature update covered in testsPassing a premium license explicitly to
validateWindowsProfileFleetVariableskeeps the test focused on variable validation, not licensing.
1248-1249: Coverage for unsupported variables in Windows profiles looks goodTests assert unsupported Fleet vars are rejected. Consider adding a positive test for
$FLEET_VAR_HOST_UUIDunder free license via the higher-level profile creation path to ensure enforcement is wired end-to-end (if not already elsewhere).Would you like me to add an integration-style test for NewMDMWindowsConfigProfile with
$FLEET_VAR_HOST_UUIDunder both free and premium to assert license gating?Also applies to: 1635-1644
server/mdm/microsoft/profile_verifier_test.go (1)
824-893: Thorough test coverage for $FLEET_VAR_HOST_UUID preprocessing
- Covers braced/unbraced, multiplicity, and XML escaping including quotes and apostrophes.
- Validates unsupported vars are left intact.
Nice work.
server/service/apple_mdm_test.go (1)
5433-5436: LGTM: passing license info to validationUpdating the call site to pass a premium license aligns with the new function signature and test intent.
server/mdm/microsoft/profile_verifier.go (3)
19-19: Centralized variables package import — good directionUsing the shared variables package helps keep detection/replacement consistent across the codebase.
47-51: Preprocess after secret expansion is correctExpanding embedded secrets before Fleet variable replacement mirrors deployment behavior and ensures verification compares the actual on-device content.
281-324: Preprocessor logic is sound and already covered by tests
PreprocessWindowsProfileContentsis exercised byTestPreprocessWindowsProfileContentsin
server/mdm/microsoft/profile_verifier_test.go:824–889.- To simplify and reduce allocations, consider replacing the loop with a short-circuit check and a direct
bytes.Buffer:// only HOST_UUID is supported today if _, ok := variables.Find(profileContents)[string(fleet.FleetVarHostUUID)]; ok { var buf bytes.Buffer _ = xml.EscapeText(&buf, []byte(hostUUID)) result = variables.Replace(result, string(fleet.FleetVarHostUUID), buf.String()) }- This removes the extra slice allocation (
make([]byte, 0, len(hostUUID))) and the loop overhead.server/service/microsoft_mdm.go (2)
32-32: Import of variables is appropriateAligns this layer with centralized variable handling.
2329-2331: Per-host preprocessing is correct for $FLEET_VAR_HOST_UUIDGenerating a unique command per host when variables are present ensures the device receives resolved content. This aligns with verification logic.
server/variables/variables_test.go (2)
81-116: LGTM: FindKeepDuplicates exercises ordering and duplicationCovers both unique and repeated variables as expected.
117-169: LGTM: Replace handles both syntaxes and empty valuesGood assertions across absence and multiple occurrences.
server/service/apple_mdm.go (7)
56-56: Centralized variables handling import looks goodImporting
server/variableshere aligns this file with the new, shared Fleet variables package.
607-613: Dedup result set: goodConverting the duplicate-preserving slice to a set for datastore storage is correct and efficient.
619-621: Correct regex source for base64 data handlingSwitching to
variables.ProfileDataVariableRegexkeeps behavior consistent across platforms. Good call.
759-759: Consistent data-field variable strippingUsing
variables.ProfileDataVariableRegexhere matches the earlier change and preserves unmarshal behavior.
972-975: Clear, centralized variable detection in DDM validationUsing
variables.Containskeeps the validation concise and aligned with the new package.
2616-2620: Deprecated endpoint correctly rejects Fleet variablesGood guardrail to prevent variables from slipping through older API paths with a helpful error message.
4883-4886: Efficient profile preprocessing: only branch when variables existEarly-exit pattern via
variables.Findkeeps preprocessing fast for profiles without variables.server/service/mdm.go (3)
44-44: Import looks correct and consistent with usage.The new variables package is used below for Fleet variable detection. No issues.
1480-1485: Good: fetch license early for precise validation.Retrieving the license once here before validating the Windows profile keeps error reporting tight. No changes requested.
1763-1768: Good: license retrieval added before batch Fleet-vars validation.This mirrors Apple handling and centralizes enforcement. No changes requested.
| { | ||
| name: "fleet variable with both formats in same profile", | ||
| hostUUID: "test-host-1234-uuid", | ||
| profileContents: `<Replace><Data>ID1: $FLEET_VAR_HOST_UUID, ID2: ${FLEET_VAR_HOST_UUID}</Data></Replace>`, | ||
| expectedContents: `<Replace><Data>ID1: test-host-1234-uuid, ID2: test-host-1234-uuid</Data></Replace>`, | ||
| }, |
There was a problem hiding this comment.
Nit: Same test as multiple fleet variables.
2 participants
Fixes #30879
Adds verification support for Windows profiles containing $FLEET_VAR_HOST_UUID, which was missing in previous PR.
Also added a license check since Fleet variables are a premium feature.
Also includes some refactoring.
Demo video: https://www.youtube.com/watch?v=HNWlu-uA20U
Checklist for submitter
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests