Fixing Windows SCEP issues - #47255
Merged
getvictor merged 9 commits intoJun 15, 2026
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
Code Review by Qodo
1. Masked challenge overwrites secret
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two Windows SCEP-related issues in Fleet: (1) a server panic during Windows SCEP profile validation when non-SCEP <LocURI> elements appear before SCEP nodes, and (2) Windows enrollment failures caused by proxy-generated challenges containing base64url characters (_, -) that the Windows SCEP CSP rejects.
Changes:
- Prevent
windowsSCEPProfileValidatorfrom indexing into placeholder empty LocURI arrays by reinitializing the “valid/required” LocURI lists when the first SCEP LocURI is encountered (even if earlier non-SCEP LocURIs were seen). - Generate custom SCEP proxy dynamic challenges using base32 (32 chars,
[A-Z2-7]) to ensure the challenge is alphanumeric and compatible with Windows. - Add regression tests for both issues and add
changes/entries.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/fleet/windows_mdm.go | Fixes SCEP profile validation logic to avoid panic on mixed LocURI ordering and ensure clean validation errors. |
| server/fleet/windows_mdm_test.go | Adds regression tests covering the non-SCEP-first mixed LocURI panic scenarios. |
| server/fleet/datastore.go | Updates Datastore interface comment to reflect base32 challenge encoding. |
| server/datastore/mysql/challenges.go | Switches generated challenges from base64url to base32 to avoid Windows CSP “non-printable character” failures. |
| server/datastore/mysql/challenges_test.go | Adds tests to enforce the generated challenge alphabet and basic consume behavior. |
| changes/46990-windows-scep-proxy-challenge-alphabet | Release note entry for the Windows SCEP proxy challenge fix. |
| changes/46982-windows-scep-profile-validation-panic | Release note entry for the Windows SCEP validation panic fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
29
to
33
| // Base32 keeps the challenge strictly alphanumeric: the Windows ClientCertificateInstall/SCEP CSP rejects | ||
| // base64url's '_' and '-' as non-printable characters, failing certificate enrollment. 20 random bytes | ||
| // (160 bits) encode to exactly 32 base32 characters with no padding, fitting the CHAR(32) column. | ||
| challenge := base32.StdEncoding.EncodeToString(key) | ||
| _, err = exec.ExecContext(ctx, `INSERT INTO challenges (challenge) VALUES (?)`, challenge) |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR fixes two Windows SCEP issues. First, setLocURIArrays now reinitializes SCEP arrays when the first SCEP is encountered so mixed profiles with a non-SCEP LocURI first no longer cause an index-out-of-range panic and instead yield a validation error. Second, custom SCEP proxy CA challenges are validated against an ASN.1 PrintableString-compatible regex: the backend enforces this on creation and when a challenge is changed (including batch apply), the update path skips masked/unchanged challenges, and the frontend form shows the same validation message. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47255 +/- ##
==========================================
- Coverage 67.19% 67.19% -0.01%
==========================================
Files 3068 3489 +421
Lines 226815 228550 +1735
Branches 11721 11912 +191
==========================================
+ Hits 152418 153577 +1159
- Misses 60656 61148 +492
- Partials 13741 13825 +84
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:
|
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
|
Code review by qodo was updated up to the latest commit 9f8bf3c |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsx (1)
118-118: ⚡ Quick winUse the shared unchanged-password constant instead of a hardcoded mask literal.
"********"in this test can drift from runtime sentinel behavior. Import and useUNCHANGED_PASSWORD_API_RESPONSEto keep tests coupled to the real contract.🤖 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 `@frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsx` at line 118, Replace the hardcoded mask literal "********" with the shared sentinel constant by importing UNCHANGED_PASSWORD_API_RESPONSE and passing it to createTestFormData for the formData prop; update the test in CustomSCEPForm.tests.tsx to import UNCHANGED_PASSWORD_API_RESPONSE and use createTestFormData({ challenge: UNCHANGED_PASSWORD_API_RESPONSE }) so the test stays consistent with the runtime API contract.
🤖 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.
Nitpick comments:
In
`@frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsx`:
- Line 118: Replace the hardcoded mask literal "********" with the shared
sentinel constant by importing UNCHANGED_PASSWORD_API_RESPONSE and passing it to
createTestFormData for the formData prop; update the test in
CustomSCEPForm.tests.tsx to import UNCHANGED_PASSWORD_API_RESPONSE and use
createTestFormData({ challenge: UNCHANGED_PASSWORD_API_RESPONSE }) so the test
stays consistent with the runtime API contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80ad700d-aa95-4442-975c-6caf902db01f
📒 Files selected for processing (8)
changes/46982-windows-scep-profile-validation-panicee/server/service/certificate_authorities.goee/server/service/certificate_authorities_test.gofrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tests.tsxfrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/CustomSCEPForm.tsxfrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CustomSCEPForm/helpers.tsserver/fleet/windows_mdm.goserver/fleet/windows_mdm_test.go
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
ksykulev
approved these changes
Jun 12, 2026
| // 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 |
Contributor
There was a problem hiding this comment.
non-blocking but there is no code path where exists = true AND existing == nil, so we could probably shorten this to challengeChanged := !exists || incoming.Challenge != existing.Challenge
getvictor
deleted the
victor/fix-windows-scep-challenge-and-validator-panic
branch
June 15, 2026 07:52
Closed
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #47492 and Resolves #46982
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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
Summary by CodeRabbit
Bug Fixes
New Features
UI / Validation
Tests