Windows SCEP profiles now fail with non-printable chars - #49887
Conversation
|
@coderabbitai full review |
|
/agentic_review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Adds Windows-specific validation for Custom SCEP Proxy challenge values so that profiles fail fast (with a clear host-facing detail) when the configured challenge contains characters that Windows can’t encode as an ASN.1 PrintableString (e.g., _), addressing the on-device enrollment failure described in #47492.
Changes:
- Introduces a Windows SCEP challenge
PrintableStringallowlist regexp and returns aMicrosoftProfileProcessingErrorwhen invalid characters are detected during Windows profile preprocessing. - Adds unit tests that lock down the exact allowed/rejected character set and exercises the new failure/success paths in Windows profile preprocessing.
- Includes a user-visible changes entry (content excluded by policy).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/mdm/microsoft/profile_variables.go | Adds PrintableString regexp and fails Windows profile preprocessing with a clear detail when the Custom SCEP challenge contains unsupported characters. |
| server/mdm/microsoft/profile_variables_test.go | Adds coverage for the regexp’s exact character class and new preprocessing behaviors (invalid challenge errors, space preservation). |
| changes/47492-windows-scep-challenge-failed-profile | User-visible change entry (content excluded; not reviewed). |
Files excluded by content exclusion policy (1)
- changes/47492-windows-scep-challenge-failed-profile
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughWindows SCEP preprocessing validates custom CA challenges against the ASN.1 PrintableString character set and returns a detailed processing error for invalid characters. Tests cover rejection of unsupported characters and preservation of surrounding spaces. Windows certificate installation errors are displayed verbatim in host profile status tooltips. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/mdm/microsoft/profile_variables.go`:
- Around line 151-156: Add write-path validation for custom SCEP CA creation and
updates, reusing windowsSCEPChallengeRegexp to reject unmasked challenge values
containing unsupported characters before persistence. Ensure unchanged masked
challenge updates remain valid by validating only newly supplied, unmasked
values while preserving the existing IsCustomSCEPConfigured/profile
preprocessing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0e1a339-a94c-4763-bcfe-70131908fbe6
📒 Files selected for processing (3)
changes/47492-windows-scep-challenge-failed-profileserver/mdm/microsoft/profile_variables.goserver/mdm/microsoft/profile_variables_test.go
Code Review by Qodo
1. Invalid new() usage
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #49887 +/- ##
========================================
Coverage 67.92% 67.92%
========================================
Files 3905 3906 +1
Lines 249676 249707 +31
Branches 13350 13191 -159
========================================
+ Hits 169580 169606 +26
Misses 64828 64828
- Partials 15268 15273 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/47492-windows-scep-challenge-failed-profile
Comments suppressed due to low confidence (2)
server/mdm/microsoft/profile_variables_test.go:304
- Same issue here:
new("...")is invalid Go (built-innewrequires a type). Useptr.String(...)to build*stringvalues so the test compiles.
Name: new("CERTIFICATE"),
Type: string(fleet.CATypeCustomSCEPProxy),
URL: new("https://scep.proxy.url/scep"),
Challenge: new(" super secret "),
server/mdm/microsoft/profile_variables.go:153
- Issue #47492’s expected behavior calls for rejecting an invalid Custom SCEP Proxy challenge at save-time (UI + backend) so admins get immediate feedback. This PR adds validation only at Windows profile preprocessing time, so an admin can still save an unsupported challenge and only discover it later when a Windows profile is processed.
if ca := deps.CustomSCEPCAs[caName]; ca != nil && !windowsSCEPChallengeRegexp.MatchString(ca.Challenge) {
return profileContents, &MicrosoftProfileProcessingError{
message: fmt.Sprintf(scepChallengeInvalidCharsDetail, caName),
}
}
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/47492-windows-scep-challenge-failed-profile
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/47492-windows-scep-challenge-failed-profile
Comments suppressed due to low confidence (1)
server/mdm/microsoft/profile_variables.go:153
- This change makes Windows profiles fail at deployment time if a Custom SCEP proxy challenge contains non-PrintableString characters, but it doesn’t implement the issue’s requested admin-side validation (reject invalid challenges on certificate authority create/update). As a result, admins can still save an invalid challenge and only discover it later when profiles are deployed.
if ca := deps.CustomSCEPCAs[caName]; ca != nil && !windowsSCEPChallengeRegexp.MatchString(ca.Challenge) {
return profileContents, &MicrosoftProfileProcessingError{
message: fmt.Sprintf(scepChallengeInvalidCharsDetail, caName),
}
}
|
@nulmete I'm assigning this one for you to review since you're familiar with this issue. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/mdm/microsoft/profile_variables.go (1)
149-153:⚠️ Potential issue | 🟠 MajorValidate custom SCEP challenges before persistence.
This only rejects invalid values during profile preprocessing, so
super_secret-style challenges can still be saved through CA create/update and fail later on Windows enrollment. Add the same validation at the write boundary, validating only newly supplied or changed unmasked values while allowing unchanged masked placeholders for backward compatibility. This remains the unresolved write-path requirement from the PR objective and prior review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mdm/microsoft/profile_variables.go` around lines 149 - 153, Extend the CA create/update write path to validate custom SCEP challenge values using windowsSCEPChallengeRegexp before persistence, reusing the existing invalid-character error detail where appropriate. Validate only newly supplied or changed unmasked challenges; accept unchanged masked placeholders so existing records remain compatible, while retaining the preprocessing validation in the profile flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/mdm/microsoft/profile_variables.go`:
- Around line 149-153: Extend the CA create/update write path to validate custom
SCEP challenge values using windowsSCEPChallengeRegexp before persistence,
reusing the existing invalid-character error detail where appropriate. Validate
only newly supplied or changed unmasked challenges; accept unchanged masked
placeholders so existing records remain compatible, while retaining the
preprocessing validation in the profile flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 80880243-78e5-4dc0-ac82-55c7094560f2
📒 Files selected for processing (4)
frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/errorTooltipHelpers.tests.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/errorTooltipHelpers.tsxserver/mdm/microsoft/profile_variables.goserver/mdm/microsoft/profile_variables_test.go
@getvictor Sounds good. Will look today. |
Related issue: Resolves #47492
Windows cert profile fails if challenge uses non-printable characters.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests