Fixing Windows SCEP issues (#47255)

<!-- Add the related story/sub-task/bug number, like Resolves #123, or
remove if NA -->
**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

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## 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.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

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 `<LocURI>` elements with a non-SCEP element first. The profile is now rejected with a clear validation error.

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.

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
 }

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)
+			}
+		})
+	}
+}

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<ICustomSCEPFormData>) => ({
@@ -94,4 +96,40 @@ describe("CustomSCEPForm", () => {
 
     expect(screen.getByRole("button", { name: "Submit" })).toBeEnabled();
   });
+
+  it("rejects a challenge with non-printable characters", () => {
+    render(
+      <CustomSCEPForm
+        formData={createTestFormData({ challenge: "bad_challenge" })}
+        isSubmitting={false}
+        submitBtnText="Submit"
+        isDirty
+        onChange={noop}
+        onSubmit={noop}
+        onCancel={noop}
+      />
+    );
+
+    expect(screen.getByText(/Invalid characters/)).toBeVisible();
+    expect(screen.getByRole("button", { name: "Submit" })).toBeDisabled();
+  });
+
+  it("does not block an unchanged (masked) challenge when editing", () => {
+    render(
+      <CustomSCEPForm
+        formData={createTestFormData({
+          challenge: UNCHANGED_PASSWORD_API_RESPONSE,
+        })}
+        isSubmitting={false}
+        submitBtnText="Submit"
+        isEditing
+        isDirty
+        onChange={noop}
+        onSubmit={noop}
+        onCancel={noop}
+      />
+    );
+
+    expect(screen.getByRole("button", { name: "Submit" })).toBeEnabled();
+  });
 });

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."

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 ' ( ) + , - . / : = ?",
+        },
       ],
     },
   };

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

server/fleet/windows_mdm_test.go

@@ -574,6 +574,53 @@ func TestValidateUserProvided(t *testing.T) {
 			},
 			wantErr: "Only options that have <LocURI> 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(`
+				  <Replace>
+				    <Target>
+				      <LocURI>Custom/URI</LocURI>
+				    </Target>
+				  </Replace>
+				  <Replace>
+				    <Target>
+				      <LocURI>./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/$FLEET_VAR_SCEP_WINDOWS_CERTIFICATE_ID</LocURI>
+				    </Target>
+				  </Replace>
+				  <Exec>
+				    <Item>
+				      <Target>
+				        <LocURI>./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/$FLEET_VAR_SCEP_WINDOWS_CERTIFICATE_ID/Install/Enroll</LocURI>
+				      </Target>
+				    </Item>
+				  </Exec>
+				`),
+			},
+			wantErr: "Only options that have <LocURI> 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(`
+				  <Replace>
+				    <Target>
+				      <LocURI>./Device/Vendor/MSFT/RootCATrustedCertificates/CA/ABCDEF/EncodedCertificate</LocURI>
+				    </Target>
+				  </Replace>
+				  <Replace>
+				    <Target>
+				      <LocURI>./Device/Vendor/MSFT/ClientCertificateInstall/SCEP/$FLEET_VAR_SCEP_WINDOWS_CERTIFICATE_ID/Install/ServerURL</LocURI>
+				    </Target>
+				  </Replace>
+				`),
+			},
+			wantErr: "\"ClientCertificateInstall/SCEP/$FLEET_VAR_SCEP_WINDOWS_CERTIFICATE_ID/Install/Enroll\" must be included within <Exec>. Please add and try again.",
+		},
 		{
 			name: "SCEP profile without Exec block",
 			profile: MDMWindowsConfigProfile{