From 95b535a62250806c7ea03e3f29a451d84ce41b89 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:46:47 -0500 Subject: [PATCH] Reverting printableCharacters SCEP validation (#49758) **Related issue:** Resolves #49756 # Checklist for submitter - [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. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit ## Summary by CodeRabbit * **Bug Fixes** * Custom SCEP proxy challenges can again include characters such as underscores. * Apple device enrollment works again with these challenges. * Removed the overly strict printable-character validation from the Custom SCEP configuration form. * The Challenge field now only enforces the required-value rule and no longer shows printable-character validation errors. --- ...revert-scep-challenge-printable-validation | 1 + ee/server/service/certificate_authorities.go | 40 +------ .../service/certificate_authorities_test.go | 113 +++--------------- .../CustomSCEPForm/CustomSCEPForm.tests.tsx | 26 +--- .../CustomSCEPForm/CustomSCEPForm.tsx | 1 - .../components/CustomSCEPForm/helpers.ts | 19 +-- 6 files changed, 26 insertions(+), 174 deletions(-) create mode 100644 changes/49756-revert-scep-challenge-printable-validation diff --git a/changes/49756-revert-scep-challenge-printable-validation b/changes/49756-revert-scep-challenge-printable-validation new file mode 100644 index 0000000000..d933078497 --- /dev/null +++ b/changes/49756-revert-scep-challenge-printable-validation @@ -0,0 +1 @@ +- Removed the validation, added in Fleet 4.89.0, that rejected custom SCEP proxy certificate authority challenges containing characters outside the ASN.1 PrintableString set (for example, an underscore). Apple devices can enroll certificates using such challenges, so they are accepted again. A fix for Windows certificate enrollment failing with these challenges will ship separately. diff --git a/ee/server/service/certificate_authorities.go b/ee/server/service/certificate_authorities.go index 1c437ebfa9..be4bc00cb8 100644 --- a/ee/server/service/certificate_authorities.go +++ b/ee/server/service/certificate_authorities.go @@ -127,8 +127,7 @@ func (svc *Service) NewCertificateAuthority(ctx context.Context, p fleet.Certifi if p.CustomSCEPProxy != nil { p.CustomSCEPProxy.Preprocess() - // New CA: the challenge is always being set, so validate its characters. - if err := svc.validateCustomSCEPProxy(ctx, p.CustomSCEPProxy, true, errPrefix); err != nil { + if err := svc.validateCustomSCEPProxy(ctx, p.CustomSCEPProxy, errPrefix); err != nil { return nil, err } @@ -393,27 +392,7 @@ func (svc *Service) validateNDESSCEPProxy(ctx context.Context, ndesSCEP *fleet.N return nil } -// 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 { +func (svc *Service) validateCustomSCEPProxy(ctx context.Context, customSCEP *fleet.CustomSCEPProxyCA, errPrefix string) error { if err := validateCAName(customSCEP.Name, errPrefix); err != nil { return err } @@ -423,9 +402,6 @@ 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)} @@ -814,11 +790,7 @@ func (svc *Service) processCustomSCEPProxyCAs(ctx context.Context, batchOps *fle } for name, incoming := range incomingByName { - // 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 { + if err := svc.validateCustomSCEPProxy(ctx, incoming, "certificate_authorities.custom_scep_proxy: "); err != nil { return err } // create the payload to be added or updated @@ -1506,12 +1478,6 @@ 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 dd8ee431d7..abee5e4b99 100644 --- a/ee/server/service/certificate_authorities_test.go +++ b/ee/server/service/certificate_authorities_test.go @@ -412,20 +412,24 @@ 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) { + t.Run("Create Custom SCEP CA - challenge with non-PrintableString characters is accepted", func(t *testing.T) { + // Regression test for the reverted PrintableString challenge validation (#49756): characters outside the + // ASN.1 PrintableString set (such as "_" and "@") must be accepted. 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 + Challenge: "base64url_style@challenge", }, } _, err := svc.NewCertificateAuthority(ctx, createRequest) - require.ErrorContains(t, err, scepChallengePrintableErrMsg) - require.Empty(t, createdCAs) + require.EqualError(t, err, "mock error to avoid NewActivity panic") + require.Len(t, createdCAs, 1) + require.NotNil(t, createdCAs[0].Challenge) + assert.Equal(t, createRequest.CustomSCEPProxy.Challenge, *createdCAs[0].Challenge) }) t.Run("Create NDES SCEP CA - Happy path", func(t *testing.T) { @@ -1597,28 +1601,13 @@ 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) { + t.Run("Challenge with non-PrintableString characters is accepted", func(t *testing.T) { + // Regression test for the reverted PrintableString challenge validation (#49756). 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), + Challenge: new("updated_challenge@with_special_chars"), }, } @@ -2029,85 +2018,17 @@ func TestDeleteCertificateAuthority(t *testing.T) { }) } -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) { +// TestProcessCustomSCEPProxyCAsChallengeChars is a regression test for the reverted PrintableString challenge validation +// (#49756): the GitOps/batch path must accept challenges containing characters outside the ASN.1 PrintableString set. +func TestProcessCustomSCEPProxyCAsChallengeChars(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) - } - }) - } + incoming := []fleet.CustomSCEPProxyCA{{Name: "SCEP1", URL: "https://customscep.example.com", Challenge: "base64url_style@challenge"}} + err := svc.processCustomSCEPProxyCAs(t.Context(), &fleet.CertificateAuthoritiesBatchOperations{}, incoming, nil) + 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 2b9273a7fc..cf7acaf685 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,8 +3,6 @@ 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) => ({ @@ -97,32 +95,16 @@ 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", () => { + it("accepts a challenge with non-PrintableString characters", () => { + // Regression test for the reverted PrintableString challenge validation (#49756): characters + // such as "_" and "@" must not block submission. render( string; @@ -95,18 +90,6 @@ 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 ' ( ) + , - . / : = ?", - }, ], }, };