From 04e7a308d4a510eee52dab602550938ffe6899f9 Mon Sep 17 00:00:00 2001 From: Ian Littman Date: Mon, 17 Nov 2025 09:44:24 -0600 Subject: [PATCH] Clean up Windows profile preprocess parameter passing in advance of adding additional profile variables (#35809) Should be no functional changes, though in a few cases I was able to pull some dep declarations outside of loops. Setting this as a separate PR so refactors can be evaluated without worrying about functional changes, and vice versa. Functional changes on #34364 will be stacked on top of this. --- server/fleet/certificate_authorities.go | 9 ++ server/mdm/microsoft/profile_verifier.go | 125 +++++++++++------- server/mdm/microsoft/profile_verifier_test.go | 33 ++++- server/mdm/profiles/profile_variables.go | 1 + server/service/microsoft_mdm.go | 21 ++- 5 files changed, 131 insertions(+), 58 deletions(-) diff --git a/server/fleet/certificate_authorities.go b/server/fleet/certificate_authorities.go index 0a87e17c87..8eff69341b 100644 --- a/server/fleet/certificate_authorities.go +++ b/server/fleet/certificate_authorities.go @@ -498,6 +498,15 @@ type GroupedCertificateAuthorities struct { Smallstep []SmallstepSCEPProxyCA `json:"smallstep"` } +// ToCustomSCEPProxyCAMap converts the CustomScepProxy slice to a map keyed by CA name +func (g *GroupedCertificateAuthorities) ToCustomSCEPProxyCAMap() map[string]*CustomSCEPProxyCA { + customSCEPCAs := make(map[string]*CustomSCEPProxyCA, len(g.CustomScepProxy)) + for _, ca := range g.CustomScepProxy { + customSCEPCAs[ca.Name] = &ca + } + return customSCEPCAs +} + func GroupCertificateAuthoritiesByType(cas []*CertificateAuthority) (*GroupedCertificateAuthorities, error) { grouped := &GroupedCertificateAuthorities{ DigiCert: []DigiCertCA{}, diff --git a/server/mdm/microsoft/profile_verifier.go b/server/mdm/microsoft/profile_verifier.go index 30686585d1..ffcf165436 100644 --- a/server/mdm/microsoft/profile_verifier.go +++ b/server/mdm/microsoft/profile_verifier.go @@ -41,7 +41,10 @@ func LoopOverExpectedHostProfiles( return fmt.Errorf("getting host profiles for verification: %w", err) } - params := PreprocessingParameters{ + deps := ProfilePreprocessDependenciesForVerify{ + Context: ctx, + Logger: logger, + DataStore: ds, HostIDForUUIDCache: make(map[string]uint), } @@ -53,7 +56,10 @@ func LoopOverExpectedHostProfiles( // Process Fleet variables if present (similar to how it's done during profile deployment) // This ensures we compare what was actually sent to the device - processedContent := PreprocessWindowsProfileContentsForVerification(ctx, logger, ds, host.UUID, expectedProf.ProfileUUID, expanded, params) + processedContent := PreprocessWindowsProfileContentsForVerification(deps, ProfilePreprocessParams{ + HostUUID: host.UUID, + ProfileUUID: expectedProf.ProfileUUID, + }, expanded) expectedProf.RawProfile = []byte(processedContent) var prof fleet.SyncMLCmd @@ -292,43 +298,25 @@ func IsWin32OrDesktopBridgeADMXCSP(locURI string) bool { return false } -// PreprocessingParameters holds parameters needed for preprocessing Windows profiles, for both verification and deployment only. -// It should only contain helper stuff, and not core values such as hostUUID, profileUUID, etc. -type PreprocessingParameters struct { - // a lookup map to avoid repeated datastore calls for hostID from hostUUID - HostIDForUUIDCache map[string]uint -} - // PreprocessWindowsProfileContentsForVerification processes Windows configuration profiles to replace Fleet variables // with the given host UUID for verification purposes. // // This function is similar to PreprocessWindowsProfileContentsForDeployment, but it does not require // a datastore or logger since it only replaces certain fleet variables to avoid datastore unnecessary work. -func PreprocessWindowsProfileContentsForVerification(ctx context.Context, logger kitlog.Logger, ds fleet.Datastore, hostUUID string, profileUUID string, profileContents string, params PreprocessingParameters) string { - replacedContents, _ := preprocessWindowsProfileContents(ctx, logger, ds, nil, true, hostUUID, profileUUID, nil, profileContents, nil, params) - // ^ We ignore the error here, and rely on the fact that the function will return the original contents if no replacements were made. - // So verification fails on individual profile level, instead of entire verification failing. +func PreprocessWindowsProfileContentsForVerification(deps ProfilePreprocessDependenciesForVerify, params ProfilePreprocessParams, profileContents string) string { + replacedContents, _ := preprocessWindowsProfileContents(deps, params, profileContents) + // ^ We ignore the error here and rely on the fact that the function will return the original contents if no replacements were made. + // So verification fails on the individual profile level, instead of the entire verification failing. return replacedContents } // PreprocessWindowsProfileContentsForDeployment processes Windows configuration profiles to replace Fleet variables // with their actual values for each host during profile deployment. -func PreprocessWindowsProfileContentsForDeployment(ctx context.Context, logger kitlog.Logger, ds fleet.Datastore, - appConfig *fleet.AppConfig, hostCmdUUID string, profileUUID string, - groupedCAs *fleet.GroupedCertificateAuthorities, profileContents string, - managedCertificatePayloads *[]*fleet.MDMManagedCertificate, - params PreprocessingParameters, -) (string, error) { - // TODO: Should we avoid iterating this list for every profile? - customSCEPCAs := make(map[string]*fleet.CustomSCEPProxyCA, len(groupedCAs.CustomScepProxy)) - for _, ca := range groupedCAs.CustomScepProxy { - customSCEPCAs[ca.Name] = &ca - } - - return preprocessWindowsProfileContents(ctx, logger, ds, appConfig, false, hostCmdUUID, profileUUID, customSCEPCAs, profileContents, managedCertificatePayloads, params) +func PreprocessWindowsProfileContentsForDeployment(deps ProfilePreprocessDependenciesForDeploy, params ProfilePreprocessParams, profileContents string) (string, error) { + return preprocessWindowsProfileContents(deps, params, profileContents) } -// This error type is used to indicate errors during Microsoft profile processing, such as variable replacement failures. +// MicrosoftProfileProcessingError is used to indicate errors during Microsoft profile processing, such as variable replacement failures. // It should not break the entire deployment flow, but rather be handled gracefully at the profile level, setting it to failed and detail = Error() type MicrosoftProfileProcessingError struct { message string @@ -338,6 +326,48 @@ func (e *MicrosoftProfileProcessingError) Error() string { return e.message } +type ProfilePreprocessDependencies interface { + GetContext() context.Context + GetLogger() kitlog.Logger + GetDS() fleet.Datastore + GetHostIdForUUIDCache() map[string]uint +} + +type ProfilePreprocessDependenciesForVerify struct { + Context context.Context + Logger kitlog.Logger + DataStore fleet.Datastore + HostIDForUUIDCache map[string]uint +} + +func (p ProfilePreprocessDependenciesForVerify) GetContext() context.Context { + return p.Context +} + +func (p ProfilePreprocessDependenciesForVerify) GetLogger() kitlog.Logger { + return p.Logger +} + +func (p ProfilePreprocessDependenciesForVerify) GetDS() fleet.Datastore { + return p.DataStore +} + +func (p ProfilePreprocessDependenciesForVerify) GetHostIdForUUIDCache() map[string]uint { + return p.HostIDForUUIDCache +} + +type ProfilePreprocessDependenciesForDeploy struct { + ProfilePreprocessDependenciesForVerify + AppConfig *fleet.AppConfig + CustomSCEPCAs map[string]*fleet.CustomSCEPProxyCA + ManagedCertificatePayloads *[]*fleet.MDMManagedCertificate +} + +type ProfilePreprocessParams struct { + HostUUID string + ProfileUUID string +} + // preprocessWindowsProfileContents processes Windows configuration profiles to replace Fleet variables // with their actual values for each host. This function is used both during profile deployment // and during profile verification to ensure consistency. @@ -360,12 +390,11 @@ func (e *MicrosoftProfileProcessingError) Error() string { // thousands of host profiles. Direct string replacement is more efficient for our use case. // 4. XML escaping: We need XML-specific escaping for values, which is simpler to control with direct // string replacement rather than template functions. -func preprocessWindowsProfileContents(ctx context.Context, logger kitlog.Logger, ds fleet.Datastore, appConfig *fleet.AppConfig, - isVerifying bool, hostUUID string, profileUUID string, - customSCEPCAs map[string]*fleet.CustomSCEPProxyCA, profileContents string, - managedCertificatePayloads *[]*fleet.MDMManagedCertificate, - params PreprocessingParameters, -) (string, error) { +// +// If you need another dependency that should be reused across profiles, add it to a ProfilePreprocessDependencies +// implementation and to the interface if it's required for both verification and deployment. For new dependencies that +// vary profile-to-profile, add them to ProfilePreprocessParams. +func preprocessWindowsProfileContents(deps ProfilePreprocessDependencies, params ProfilePreprocessParams, profileContents string) (string, error) { // Check if Fleet variables are present fleetVars := variables.Find(profileContents) if len(fleetVars) == 0 { @@ -377,40 +406,42 @@ func preprocessWindowsProfileContents(ctx context.Context, logger kitlog.Logger, result := profileContents for _, fleetVar := range fleetVars { if fleetVar == string(fleet.FleetVarHostUUID) { - result = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostUUIDRegexp, result, hostUUID) + result = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostUUIDRegexp, result, params.HostUUID) } else if slices.Contains(fleet.IDPFleetVariables, fleet.FleetVarName(fleetVar)) { - replacedContents, replacedVariable, err := profiles.ReplaceHostEndUserIDPVariables(ctx, ds, fleetVar, result, hostUUID, params.HostIDForUUIDCache, func(errMsg string) error { + replacedContents, replacedVariable, err := profiles.ReplaceHostEndUserIDPVariables(deps.GetContext(), deps.GetDS(), fleetVar, result, params.HostUUID, deps.GetHostIdForUUIDCache(), func(errMsg string) error { return &MicrosoftProfileProcessingError{message: errMsg} }) if err != nil { return profileContents, err } if !replacedVariable { - return profileContents, ctxerr.Wrap(ctx, err, "host end user IDP variable replacement failed for variable") + return profileContents, ctxerr.Wrap(deps.GetContext(), err, "host end user IDP variable replacement failed for variable") } result = replacedContents } - // We skip some variables during verification, to avoid unnecessary datastore calls - // or processing that is not needed for verification. - if isVerifying { + // We skip some variables during verification to avoid unnecessary datastore calls + // or processing that is not needed for verification. We determine whether to process + // or verify based on which set of deps are passed in. + deps, forDeploy := deps.(ProfilePreprocessDependenciesForDeploy) + if !forDeploy { continue } switch { case fleetVar == string(fleet.FleetVarSCEPWindowsCertificateID): - result = profiles.ReplaceFleetVariableInXML(fleet.FleetVarSCEPWindowsCertificateIDRegexp, result, profileUUID) + result = profiles.ReplaceFleetVariableInXML(fleet.FleetVarSCEPWindowsCertificateIDRegexp, result, params.ProfileUUID) case strings.HasPrefix(fleetVar, string(fleet.FleetVarCustomSCEPChallengePrefix)): caName := strings.TrimPrefix(fleetVar, string(fleet.FleetVarCustomSCEPChallengePrefix)) - err := profiles.IsCustomSCEPConfigured(ctx, customSCEPCAs, caName, fleetVar, func(errMsg string) error { + err := profiles.IsCustomSCEPConfigured(deps.Context, deps.CustomSCEPCAs, caName, fleetVar, func(errMsg string) error { return &MicrosoftProfileProcessingError{message: errMsg} }) if err != nil { return profileContents, err } - replacedContents, replacedVariable, err := profiles.ReplaceCustomSCEPChallengeVariable(ctx, logger, fleetVar, customSCEPCAs, result) + replacedContents, replacedVariable, err := profiles.ReplaceCustomSCEPChallengeVariable(deps.Context, deps.Logger, fleetVar, deps.CustomSCEPCAs, result) if err != nil { - return profileContents, ctxerr.Wrap(ctx, err, "replacing custom SCEP challenge variable") + return profileContents, ctxerr.Wrap(deps.Context, err, "replacing custom SCEP challenge variable") } if !replacedVariable { return profileContents, &MicrosoftProfileProcessingError{message: fmt.Sprintf("Custom SCEP challenge variable replacement failed for variable %s", fleetVar)} @@ -418,22 +449,22 @@ func preprocessWindowsProfileContents(ctx context.Context, logger kitlog.Logger, result = replacedContents case strings.HasPrefix(fleetVar, string(fleet.FleetVarCustomSCEPProxyURLPrefix)): caName := strings.TrimPrefix(fleetVar, string(fleet.FleetVarCustomSCEPProxyURLPrefix)) - err := profiles.IsCustomSCEPConfigured(ctx, customSCEPCAs, caName, fleetVar, func(errMsg string) error { + err := profiles.IsCustomSCEPConfigured(deps.Context, deps.CustomSCEPCAs, caName, fleetVar, func(errMsg string) error { return &MicrosoftProfileProcessingError{message: errMsg} }) if err != nil { return profileContents, err } - replacedContents, managedCertificate, replacedVariable, err := profiles.ReplaceCustomSCEPProxyURLVariable(ctx, logger, ds, appConfig, fleetVar, customSCEPCAs, result, hostUUID, profileUUID) + replacedContents, managedCertificate, replacedVariable, err := profiles.ReplaceCustomSCEPProxyURLVariable(deps.Context, deps.Logger, deps.DataStore, deps.AppConfig, fleetVar, deps.CustomSCEPCAs, result, params.HostUUID, params.ProfileUUID) if err != nil { - return profileContents, ctxerr.Wrap(ctx, err, "replacing custom SCEP challenge variable") + return profileContents, ctxerr.Wrap(deps.Context, err, "replacing custom SCEP challenge variable") } if !replacedVariable { return profileContents, &MicrosoftProfileProcessingError{message: fmt.Sprintf("Custom SCEP challenge variable replacement failed for variable %s", fleetVar)} } result = replacedContents - *managedCertificatePayloads = append(*managedCertificatePayloads, managedCertificate) + *deps.ManagedCertificatePayloads = append(*deps.ManagedCertificatePayloads, managedCertificate) } // Add other Fleet variables here as they are implemented, identify if it can be skipped for verification. diff --git a/server/mdm/microsoft/profile_verifier_test.go b/server/mdm/microsoft/profile_verifier_test.go index 770a91fa35..7db8379642 100644 --- a/server/mdm/microsoft/profile_verifier_test.go +++ b/server/mdm/microsoft/profile_verifier_test.go @@ -928,13 +928,19 @@ func TestPreprocessWindowsProfileContentsForVerification(t *testing.T) { }, } - params := PreprocessingParameters{ + deps := ProfilePreprocessDependenciesForVerify{ + Context: t.Context(), + Logger: log.NewNopLogger(), + DataStore: ds, HostIDForUUIDCache: make(map[string]uint), } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := PreprocessWindowsProfileContentsForVerification(t.Context(), log.NewNopLogger(), ds, tt.hostUUID, uuid.NewString(), tt.profileContents, params) + result := PreprocessWindowsProfileContentsForVerification(deps, ProfilePreprocessParams{ + HostUUID: tt.hostUUID, + ProfileUUID: uuid.NewString(), + }, tt.profileContents) require.Equal(t, tt.expectedContents, result) }) } @@ -1143,9 +1149,7 @@ func TestPreprocessWindowsProfileContentsForDeployment(t *testing.T) { }, } - params := PreprocessingParameters{ - HostIDForUUIDCache: make(map[string]uint), - } + hostIDForUUIDCache := make(map[string]uint) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1172,9 +1176,26 @@ func TestPreprocessWindowsProfileContentsForDeployment(t *testing.T) { groupedCAs, err := ds.GetGroupedCertificateAuthorities(ctx, true) require.NoError(t, err) + customSCEPCAs := groupedCAs.ToCustomSCEPProxyCAMap() + managedCertificates := &[]*fleet.MDMManagedCertificate{} - result, err := PreprocessWindowsProfileContentsForDeployment(ctx, log.NewNopLogger(), ds, appConfig, tt.hostUUID, profileUUID, groupedCAs, tt.profileContents, managedCertificates, params) + deps := ProfilePreprocessDependenciesForDeploy{ + ProfilePreprocessDependenciesForVerify: ProfilePreprocessDependenciesForVerify{ + Context: ctx, + Logger: log.NewNopLogger(), + DataStore: ds, + HostIDForUUIDCache: hostIDForUUIDCache, + }, + AppConfig: appConfig, + CustomSCEPCAs: customSCEPCAs, + ManagedCertificatePayloads: managedCertificates, + } + + result, err := PreprocessWindowsProfileContentsForDeployment(deps, ProfilePreprocessParams{ + HostUUID: tt.hostUUID, + ProfileUUID: profileUUID, + }, tt.profileContents) if tt.expectError { require.Error(t, err) if tt.processingError != "" { diff --git a/server/mdm/profiles/profile_variables.go b/server/mdm/profiles/profile_variables.go index 3b931e9336..eba6d63623 100644 --- a/server/mdm/profiles/profile_variables.go +++ b/server/mdm/profiles/profile_variables.go @@ -135,6 +135,7 @@ func getHostEndUserIDPUser(ctx context.Context, ds fleet.Datastore, return nil, false, onError(fmt.Sprintf("Unexpected number of hosts (%d) for UUID %s. ", len(ids), hostUUID)) } hostID = ids[0] + // TODO figure out whether we should be passing a ref around here, as this looks like the cache doesn't persist hostIDForUUIDCache[hostUUID] = hostID } diff --git a/server/service/microsoft_mdm.go b/server/service/microsoft_mdm.go index a7f2991bb3..7af7528c75 100644 --- a/server/service/microsoft_mdm.go +++ b/server/service/microsoft_mdm.go @@ -2315,11 +2315,18 @@ func ReconcileWindowsProfiles(ctx context.Context, ds fleet.Datastore, logger ki return ctxerr.Wrap(ctx, err, "getting grouped certificate authorities") } - params := microsoft_mdm.PreprocessingParameters{ - HostIDForUUIDCache: make(map[string]uint), - } - managedCertificatePayloads := &[]*fleet.MDMManagedCertificate{} + deps := microsoft_mdm.ProfilePreprocessDependenciesForDeploy{ + ProfilePreprocessDependenciesForVerify: microsoft_mdm.ProfilePreprocessDependenciesForVerify{ + Context: ctx, + Logger: logger, + DataStore: ds, + HostIDForUUIDCache: make(map[string]uint), + }, + AppConfig: appConfig, + CustomSCEPCAs: groupedCAs.ToCustomSCEPProxyCAMap(), + ManagedCertificatePayloads: managedCertificatePayloads, + } for profUUID, target := range installTargets { p, ok := profileContents[profUUID] @@ -2350,7 +2357,11 @@ func ReconcileWindowsProfiles(ctx context.Context, ds fleet.Datastore, logger ki } // Preprocess the profile content for this specific host - processedContent, err := microsoft_mdm.PreprocessWindowsProfileContentsForDeployment(ctx, logger, ds, appConfig, hostUUID, profUUID, groupedCAs, string(p.SyncML), managedCertificatePayloads, params) + processedContent, err := microsoft_mdm.PreprocessWindowsProfileContentsForDeployment( + deps, + microsoft_mdm.ProfilePreprocessParams{HostUUID: hostUUID, ProfileUUID: profUUID}, + string(p.SyncML), + ) var profileProcessingError *microsoft_mdm.MicrosoftProfileProcessingError if err != nil && !errors.As(err, &profileProcessingError) { return ctxerr.Wrapf(ctx, err, "preprocessing profile contents for host %s and profile %s", hostUUID, profUUID)