From 4fdd4bd3b4412367b22ce88110dd48b6f8c30ca4 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> Date: Mon, 15 Jun 2026 08:52:25 +0100 Subject: [PATCH] Fixing Windows SCEP issues (#47255) **Related issue:** Resolves #47492 and Resolves #46982 - Fixed panic when uploading bad profile - Added validation for SCEP challenge to exclude underscore (and other non-printable characters). # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit * **Bug Fixes** * Prevented a server panic during Windows configuration profile validation when SCEP and non-SCEP elements are mixed; such profiles are now rejected with a clear validation error. * **New Features** * Enforced Windows-compatible printable characters for Custom SCEP proxy challenge values; rejects disallowed characters while preserving legacy values unless changed. * **UI / Validation** * Improved form validation feedback for the Custom SCEP challenge field, showing errors and disabling submit for invalid input while allowing masked/unchanged values. * **Tests** * Added regression and unit tests covering profile validation and challenge character validation. --- ...6982-windows-scep-profile-validation-panic | 1 + ...indows-scep-challenge-printable-characters | 1 + ee/server/service/certificate_authorities.go | 40 +++++- .../service/certificate_authorities_test.go | 128 ++++++++++++++++++ .../CustomSCEPForm/CustomSCEPForm.tests.tsx | 38 ++++++ .../CustomSCEPForm/CustomSCEPForm.tsx | 1 + .../components/CustomSCEPForm/helpers.ts | 19 ++- server/fleet/windows_mdm.go | 5 +- server/fleet/windows_mdm_test.go | 47 +++++++ 9 files changed, 275 insertions(+), 5 deletions(-) create mode 100644 changes/46982-windows-scep-profile-validation-panic create mode 100644 changes/47492-windows-scep-challenge-printable-characters diff --git a/changes/46982-windows-scep-profile-validation-panic b/changes/46982-windows-scep-profile-validation-panic new file mode 100644 index 0000000000..dfc1396721 --- /dev/null +++ b/changes/46982-windows-scep-profile-validation-panic @@ -0,0 +1 @@ +- Fixed a server panic when validating a Windows configuration profile that mixes SCEP and non-SCEP `` elements with a non-SCEP element first. The profile is now rejected with a clear validation error. diff --git a/changes/47492-windows-scep-challenge-printable-characters b/changes/47492-windows-scep-challenge-printable-characters new file mode 100644 index 0000000000..9059828065 --- /dev/null +++ b/changes/47492-windows-scep-challenge-printable-characters @@ -0,0 +1 @@ +- Validated that a custom SCEP proxy certificate authority challenge contains only printable characters, so Windows certificate enrollment no longer fails with "The string contains a non-printable character" (for example, when the challenge contains an underscore). Existing challenges are only re-validated when changed. diff --git a/ee/server/service/certificate_authorities.go b/ee/server/service/certificate_authorities.go index 0620511a69..8688c3eca1 100644 --- a/ee/server/service/certificate_authorities.go +++ b/ee/server/service/certificate_authorities.go @@ -127,7 +127,8 @@ func (svc *Service) NewCertificateAuthority(ctx context.Context, p fleet.Certifi if p.CustomSCEPProxy != nil { p.CustomSCEPProxy.Preprocess() - if err := svc.validateCustomSCEPProxy(ctx, p.CustomSCEPProxy, errPrefix); err != nil { + // New CA: the challenge is always being set, so validate its characters. + if err := svc.validateCustomSCEPProxy(ctx, p.CustomSCEPProxy, true, errPrefix); err != nil { return nil, err } @@ -392,7 +393,27 @@ func (svc *Service) validateNDESSCEPProxy(ctx context.Context, ndesSCEP *fleet.N return nil } -func (svc *Service) validateCustomSCEPProxy(ctx context.Context, customSCEP *fleet.CustomSCEPProxyCA, errPrefix string) error { +// printableStringChallengeRegexp matches challenges containing only characters that are valid in an ASN.1 PrintableString, minus +// the space. Windows encodes the SCEP challenge password as a PrintableString, so a challenge containing any other character +// (most commonly "_") makes Windows certificate enrollment fail with "The string contains a non-printable character." The space +// is a valid PrintableString character but is disallowed here because leading/trailing spaces are an invisible footgun. Keep in +// sync with PRINTABLE_STRING_REGEX in the CustomSCEPForm frontend helpers. +var printableStringChallengeRegexp = regexp.MustCompile(`^[A-Za-z0-9'()+,./:=?-]*$`) + +// scepChallengePrintableErrMsg is returned when a custom SCEP proxy challenge contains characters that Windows cannot use. +const scepChallengePrintableErrMsg = `Custom SCEP Proxy challenge can only contain letters, numbers, and the characters ' ( ) + , - . / : = ?. Certificate enrollment rejects other characters, such as "_".` + +// challengeHasAllowedChars reports whether the challenge contains only the characters Fleet allows in a SCEP challenge: the ASN.1 +// PrintableString set minus the space (see printableStringChallengeRegexp). +func challengeHasAllowedChars(challenge string) bool { + return printableStringChallengeRegexp.MatchString(challenge) +} + +// validateCustomSCEPProxy validates a custom SCEP proxy CA payload. validateChallengeChars controls whether +// the challenge is checked for Windows-incompatible (non-PrintableString) characters; callers should only +// set it when the challenge is being created or changed, so that challenges stored before this validation +// existed continue to work. +func (svc *Service) validateCustomSCEPProxy(ctx context.Context, customSCEP *fleet.CustomSCEPProxyCA, validateChallengeChars bool, errPrefix string) error { if err := validateCAName(customSCEP.Name, errPrefix); err != nil { return err } @@ -402,6 +423,9 @@ func (svc *Service) validateCustomSCEPProxy(ctx context.Context, customSCEP *fle if customSCEP.Challenge == "" || customSCEP.Challenge == fleet.MaskedPassword { return fleet.NewInvalidArgumentError("challenge", fmt.Sprintf("%sCustom SCEP Proxy challenge cannot be empty", errPrefix)) } + if validateChallengeChars && !challengeHasAllowedChars(customSCEP.Challenge) { + return fleet.NewInvalidArgumentError("challenge", fmt.Sprintf("%s%s", errPrefix, scepChallengePrintableErrMsg)) + } if err := svc.scepConfigService.ValidateSCEPURL(ctx, customSCEP.URL); err != nil { svc.logger.ErrorContext(ctx, "Failed to validate custom SCEP URL", "err", err) return &fleet.BadRequestError{Message: fmt.Sprintf("%sInvalid SCEP URL. Please correct and try again.", errPrefix)} @@ -786,7 +810,11 @@ func (svc *Service) processCustomSCEPProxyCAs(ctx context.Context, batchOps *fle } for name, incoming := range incomingByName { - if err := svc.validateCustomSCEPProxy(ctx, incoming, "certificate_authorities.custom_scep_proxy: "); err != nil { + // Only validate the challenge characters when the challenge is new or changed, so that challenges stored before this validation + // existed continue to work. + existing, exists := existingByName[name] + challengeChanged := !exists || existing == nil || incoming.Challenge != existing.Challenge + if err := svc.validateCustomSCEPProxy(ctx, incoming, challengeChanged, "certificate_authorities.custom_scep_proxy: "); err != nil { return err } // create the payload to be added or updated @@ -1474,6 +1502,12 @@ func (svc *Service) validateCustomSCEPProxyUpdate(ctx context.Context, customSCE Message: fmt.Sprintf("%sCustom SCEP Proxy challenge cannot be empty", errPrefix), } } + // Only validate the challenge characters when a new challenge value is provided. A nil or masked challenge means it is unchanged, + // so challenges stored before this validation existed keep working. + if customSCEP.Challenge != nil && *customSCEP.Challenge != fleet.MaskedPassword && + !challengeHasAllowedChars(*customSCEP.Challenge) { + return &fleet.BadRequestError{Message: fmt.Sprintf("%s%s", errPrefix, scepChallengePrintableErrMsg)} + } return nil } diff --git a/ee/server/service/certificate_authorities_test.go b/ee/server/service/certificate_authorities_test.go index 26eed78352..2100b1741c 100644 --- a/ee/server/service/certificate_authorities_test.go +++ b/ee/server/service/certificate_authorities_test.go @@ -396,6 +396,22 @@ func TestCreatingCertificateAuthorities(t *testing.T) { verifyNilFieldsForType(t, createdCA) }) + t.Run("Create Custom SCEP CA - challenge with non-printable character is rejected", func(t *testing.T) { + svc, ctx := baseSetupForCATests() + + createRequest := fleet.CertificateAuthorityPayload{ + CustomSCEPProxy: &fleet.CustomSCEPProxyCA{ + Name: "CustomSCEPWIFI", + URL: "https://customscep.example.com", + Challenge: "bad_challenge", // underscore is not a valid ASN.1 PrintableString character + }, + } + + _, err := svc.NewCertificateAuthority(ctx, createRequest) + require.ErrorContains(t, err, scepChallengePrintableErrMsg) + require.Empty(t, createdCAs) + }) + t.Run("Create NDES SCEP CA - Happy path", func(t *testing.T) { svc, ctx := baseSetupForCATests() @@ -1565,6 +1581,35 @@ func TestUpdatingCertificateAuthorities(t *testing.T) { require.EqualError(t, err, "mock error to avoid NewActivity panic") }) + t.Run("Challenge with non-printable character is rejected", func(t *testing.T) { + svc, ctx := baseSetupForCATests() + + payload := fleet.CertificateAuthorityUpdatePayload{ + CustomSCEPProxyCAUpdatePayload: &fleet.CustomSCEPProxyCAUpdatePayload{ + Challenge: new("bad_challenge"), // underscore is not a valid ASN.1 PrintableString character + }, + } + + err := svc.UpdateCertificateAuthority(ctx, scepID, payload) + require.ErrorContains(t, err, scepChallengePrintableErrMsg) + }) + + t.Run("Masked (unchanged) challenge skips character validation", func(t *testing.T) { + // Backward compatibility: an unchanged challenge is submitted as the masked placeholder, so it must + // not be re-validated. Otherwise editing a CA whose challenge predates this validation would break. + svc, ctx := baseSetupForCATests() + + payload := fleet.CertificateAuthorityUpdatePayload{ + CustomSCEPProxyCAUpdatePayload: &fleet.CustomSCEPProxyCAUpdatePayload{ + URL: new("https://customscep.example.com"), + Challenge: new(fleet.MaskedPassword), + }, + } + + err := svc.UpdateCertificateAuthority(ctx, scepID, payload) + require.EqualError(t, err, "mock error to avoid NewActivity panic") + }) + t.Run("Bad name", func(t *testing.T) { svc, ctx := baseSetupForCATests() @@ -1967,3 +2012,86 @@ func TestDeleteCertificateAuthority(t *testing.T) { require.Contains(t, err.Error(), "certificate authority was not found") }) } + +func TestChallengeHasAllowedChars(t *testing.T) { + tests := []struct { + name string + challenge string + want bool + }{ + {"alphanumeric", "FleetSCEPtest2026", true}, + {"empty", "", true}, + {"allowed punctuation", "Fleet-SCEP.2026(test)+,/:=?'", true}, + {"hyphen only", "abc-def-123", true}, + {"underscore rejected", "Fleet_SCEP", false}, + {"at sign rejected", "fleet@scep", false}, + {"asterisk rejected", "fleet*scep", false}, + {"base64url with underscore rejected", "JURAzXStYElNpVi63B_ps6D0WxF7b3Gv", false}, + {"base64url with hyphen only allowed", "i-8MPPQ85Ux3uqNptijN53Ru3KYIIgEI", true}, + {"hash rejected", "fleet#scep", false}, + {"tilde rejected", "fleet~scep", false}, + {"internal space rejected", "fleet scep", false}, + {"leading space rejected", " fleetscep", false}, + {"trailing space rejected", "fleetscep ", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.want, challengeHasAllowedChars(tt.challenge)) + }) + } +} + +// TestProcessCustomSCEPProxyCAsChallengeValidation covers the GitOps/batch path, which (unlike the UI +// update path) provides the challenge unmasked and detects "unchanged" by comparing the incoming +// challenge to the existing one. A pre-existing challenge with otherwise-disallowed characters must keep +// working when it is re-applied unchanged. +func TestProcessCustomSCEPProxyCAsChallengeValidation(t *testing.T) { + svc := &Service{ + logger: slog.New(slog.NewTextHandler(os.Stdout, nil)), + scepConfigService: &scep_mock.SCEPConfigService{ + ValidateSCEPURLFunc: func(_ context.Context, _ string) error { return nil }, + }, + } + const url = "https://customscep.example.com" + + tests := []struct { + name string + existing []fleet.CustomSCEPProxyCA + incoming []fleet.CustomSCEPProxyCA + wantErr bool + }{ + { + name: "new CA with disallowed challenge is rejected", + incoming: []fleet.CustomSCEPProxyCA{{Name: "SCEP1", URL: url, Challenge: "bad_challenge"}}, + wantErr: true, + }, + { + name: "unchanged disallowed challenge is skipped (backward compatible)", + existing: []fleet.CustomSCEPProxyCA{{Name: "SCEP1", URL: url, Challenge: "legacy_challenge"}}, + incoming: []fleet.CustomSCEPProxyCA{{Name: "SCEP1", URL: url, Challenge: "legacy_challenge"}}, + wantErr: false, + }, + { + name: "challenge changed to a disallowed value is rejected", + existing: []fleet.CustomSCEPProxyCA{{Name: "SCEP1", URL: url, Challenge: "goodchallenge"}}, + incoming: []fleet.CustomSCEPProxyCA{{Name: "SCEP1", URL: url, Challenge: "new_bad"}}, + wantErr: true, + }, + { + name: "challenge changed to an allowed value succeeds", + existing: []fleet.CustomSCEPProxyCA{{Name: "SCEP1", URL: url, Challenge: "goodchallenge"}}, + incoming: []fleet.CustomSCEPProxyCA{{Name: "SCEP1", URL: url, Challenge: "new-good.value"}}, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := svc.processCustomSCEPProxyCAs(t.Context(), &fleet.CertificateAuthoritiesBatchOperations{}, tt.incoming, tt.existing) + if tt.wantErr { + require.ErrorContains(t, err, scepChallengePrintableErrMsg) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsx b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsx index b9befc7214..2b9273a7fc 100644 --- a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsx +++ b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsx @@ -3,6 +3,8 @@ import { noop } from "lodash"; import { render, screen } from "@testing-library/react"; import { renderWithSetup } from "test/test-utils"; +import { UNCHANGED_PASSWORD_API_RESPONSE } from "utilities/constants"; + import CustomSCEPForm, { ICustomSCEPFormData } from "./CustomSCEPForm"; const createTestFormData = (overrides?: Partial) => ({ @@ -94,4 +96,40 @@ describe("CustomSCEPForm", () => { expect(screen.getByRole("button", { name: "Submit" })).toBeEnabled(); }); + + it("rejects a challenge with non-printable characters", () => { + render( + + ); + + expect(screen.getByText(/Invalid characters/)).toBeVisible(); + expect(screen.getByRole("button", { name: "Submit" })).toBeDisabled(); + }); + + it("does not block an unchanged (masked) challenge when editing", () => { + render( + + ); + + expect(screen.getByRole("button", { name: "Submit" })).toBeEnabled(); + }); }); diff --git a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tsx b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tsx index c7fdeeb3c5..56a1d6afd9 100644 --- a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tsx +++ b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tsx @@ -95,6 +95,7 @@ const CustomSCEPForm = ({ label="Challenge" name="challenge" value={challenge} + error={formValidation.challenge?.message} onChange={onInputChange} parseTarget helpText="Password to authenticate with a SCEP server." diff --git a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/helpers.ts b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/helpers.ts index b246d9f6e3..038a8e248b 100644 --- a/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/helpers.ts +++ b/frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/helpers.ts @@ -1,16 +1,21 @@ import { ICertificateAuthorityPartial } from "interfaces/certificates"; +import { UNCHANGED_PASSWORD_API_RESPONSE } from "utilities/constants"; import valid_url from "components/forms/validators/valid_url"; import { ICustomSCEPFormData } from "./CustomSCEPForm"; +// Windows encodes the SCEP challenge password as an ASN.1 PrintableString, so a challenge with any character outside that set (most +// commonly "_") fails. Keep in sync with printableStringChallengeRegexp in ee/server/service/certificate_authorities.go. +const PRINTABLE_STRING_REGEX = /^[A-Za-z0-9'()+,./:=?-]*$/; + // TODO: create a validator abstraction for this and the other form validation files export interface ICustomSCEPFormValidation { isValid: boolean; name?: { isValid: boolean; message?: string }; scepURL?: { isValid: boolean; message?: string }; - challenge?: { isValid: boolean }; + challenge?: { isValid: boolean; message?: string }; } type IMessageFunc = (formData: ICustomSCEPFormData) => string; @@ -90,6 +95,18 @@ export const generateFormValidations = ( return formData.challenge.length > 0; }, }, + { + name: "printableCharacters", + isValid: (formData: ICustomSCEPFormData) => { + // Skip an unchanged (masked) challenge, so editing a CA whose challenge predates this validation isn't blocked. + return ( + formData.challenge === UNCHANGED_PASSWORD_API_RESPONSE || + PRINTABLE_STRING_REGEX.test(formData.challenge) + ); + }, + message: + "Invalid characters. Certificate enrollment only supports letters, numbers, and ' ( ) + , - . / : = ?", + }, ], }, }; diff --git a/server/fleet/windows_mdm.go b/server/fleet/windows_mdm.go index d557701bf2..ca2645974f 100644 --- a/server/fleet/windows_mdm.go +++ b/server/fleet/windows_mdm.go @@ -450,7 +450,10 @@ func (v *windowsSCEPProfileValidator) validateExecLocURI(locURI string) error { func (v *windowsSCEPProfileValidator) setLocURIArrays(locURI string) error { switch { - case IsWindowsSCEPLocURI(locURI) && v.validExecSCEPProfileLocURIs == nil: + case IsWindowsSCEPLocURI(locURI) && (v.validExecSCEPProfileLocURIs == nil || len(*v.validExecSCEPProfileLocURIs) == 0): + // First SCEP LocURI seen. Earlier non-SCEP LocURIs may have set the empty placeholder arrays; replace them + // with the real ones so finalizeValidation rejects the mixed profile cleanly instead of indexing into an + // empty array below. if strings.HasPrefix(locURI, "./User") { v.requiredSCEPProfileLocURIs = &requiredUserSCEPProfileLocURIs v.validSCEPProfileLocURIs = &validUserSCEPProfileLocURIs diff --git a/server/fleet/windows_mdm_test.go b/server/fleet/windows_mdm_test.go index 36bc43d005..2a93b1c892 100644 --- a/server/fleet/windows_mdm_test.go +++ b/server/fleet/windows_mdm_test.go @@ -574,6 +574,53 @@ func TestValidateUserProvided(t *testing.T) { }, wantErr: "Only options that have starting with \"ClientCertificateInstall/SCEP/\" can be added to SCEP profile.", }, + { + // Regression test for #46982: a non-SCEP LocURI before the SCEP LocURIs used to panic with + // index out of range instead of returning a validation error. + name: "SCEP profile with other LocURIs, non-SCEP LocURI first", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + Custom/URI + + + + + ./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/$FLEET_VAR_SCEP_WINDOWS_CERTIFICATE_ID + + + + + + ./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/$FLEET_VAR_SCEP_WINDOWS_CERTIFICATE_ID/Install/Enroll + + + + `), + }, + wantErr: "Only options that have starting with \"ClientCertificateInstall/SCEP/\" can be added to SCEP profile.", + }, + { + // Regression test for #46982: same non-SCEP-first ordering without an Exec block (e.g. a root CA + // cert install combined with SCEP nodes) also used to panic. + name: "SCEP profile with non-SCEP LocURI first and no Exec block", + profile: MDMWindowsConfigProfile{ + SyncML: []byte(` + + + ./Device/Vendor/MSFT/RootCATrustedCertificates/CA/ABCDEF/EncodedCertificate + + + + + ./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/$FLEET_VAR_SCEP_WINDOWS_CERTIFICATE_ID/Install/ServerURL + + + `), + }, + wantErr: "\"ClientCertificateInstall/SCEP/$FLEET_VAR_SCEP_WINDOWS_CERTIFICATE_ID/Install/Enroll\" must be included within . Please add and try again.", + }, { name: "SCEP profile without Exec block", profile: MDMWindowsConfigProfile{