Apple non-proxied SCEP profile validation (PR 7/9)#45364
Merged
Conversation
Two related changes that prepare Apple profile uploads for the non-proxied renewal flow: 1. Add `$FLEET_VAR_CERTIFICATE_RENEWAL_ID` as the preferred name for the renewal-ID marker variable (per docs PR #44069). The legacy `$FLEET_VAR_SCEP_RENEWAL_ID` is still accepted by validation and substitution for back-compat with existing profiles. Both names substitute to the same value: "fleet-" + profile_uuid. 2. Add additionalACMEValidation: any profile containing a com.apple.security.acme payload must include either renewal-ID variable in the cert Subject's CN or OU, otherwise the upload is rejected. Without the marker the resulting cert can't be linked back to its profile and would never auto-renew. The shared SCEP-renewal-ID-without-URL/Challenge guard in mdm_profiles.go skips its check when an ACME payload is present — ACME profiles use the marker variable alone (no SCEP URL/Challenge companions). Validation error messages reference only the new CERTIFICATE_RENEWAL_ID name to steer new authoring toward it.
The validation error messages in mdm_profiles.go switched to reference the preferred CERTIFICATE_RENEWAL_ID name. The unit and integration test assertions still required the legacy SCEP_RENEWAL_ID text. Updating them to match.
Decision 2.7 requires profile validation to keep accepting the pre-rename variable name. The existing failure-case tests don't guard that contract because they all exercise error paths. Add a happy-path case asserting that a Custom SCEP profile authored with $FLEET_VAR_SCEP_RENEWAL_ID validates successfully when all other required variables are present.
…leetdm/fleet into pr-2.3-apple-validation-rename
The renewal-ID-only bypass in validateProfileCertificateAuthorityVariables checked the profile body with strings.Contains, which would false-positive on "com.apple.security.acme" appearing in a description, identifier, or custom string field. Switch to the existing Mobileconfig.HasPayloadType helper for a structured plist parse. Parse failure (e.g., Windows-path content that isn't a mobileconfig) is treated as no-ACME, so the SCEP-only error fires normally on that path.
…aw-scep-validation
…aw-scep-validation
…aw-scep-validation # Conflicts: # server/fleet/mdm.go # server/service/apple_mdm.go # server/service/apple_mdm_test.go
Reject raw-SCEP profiles (com.apple.security.scep payload, no Fleet proxy variables) whose Subject OU lacks $FLEET_VAR_CERTIFICATE_RENEWAL_ID. Profiles using Fleet proxy variables (NDES/Custom SCEP/Smallstep) keep their existing per-CA validators. Net-new validator: OU placement only, preferred variable name only — no back-compat owed on a surface that didn't previously enforce renewal-ID at all. Also broadens the validateConfigProfileFleetVariables bypass to recognize both ACME and SCEP payloads via mobileconfig.HasPayloadType.
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
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 (3)
WalkthroughThis PR implements validation for non-proxied SCEP profiles to require a certificate auto-renewal marker ( Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 40639-cert-renew #45364 +/- ##
===================================================
Coverage ? 66.74%
===================================================
Files ? 2730
Lines ? 218567
Branches ? 10703
===================================================
Hits ? 145879
Misses ? 59467
Partials ? 13221
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…aw-scep-validation
sgress454
approved these changes
May 13, 2026
Contributor
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
LGTM! It feels like when we're done here there's gonna be a fair amount of overlap/duplicated code between proxy-less SCEP and proxy-less ACME, that might be worth pulling into shared helpers at some point.
Comment on lines
+795
to
+798
| strings.HasPrefix(v, string(fleet.FleetVarCustomSCEPChallengePrefix)) || | ||
| strings.HasPrefix(v, string(fleet.FleetVarCustomSCEPProxyURLPrefix)) || | ||
| strings.HasPrefix(v, string(fleet.FleetVarSmallstepSCEPChallengePrefix)) || | ||
| strings.HasPrefix(v, string(fleet.FleetVarSmallstepSCEPProxyURLPrefix)) { |
Contributor
There was a problem hiding this comment.
This duck-typing is now repeated both here and in validateProfileCertificateAuthorityVariables, so they'll have to be kept in sync if any new kind of SCEP proxy is implemented. Not sure how likely that is.
Contributor
Author
There was a problem hiding this comment.
agree, i'll make a note
2 tasks
mostlikelee
added a commit
that referenced
this pull request
May 14, 2026
… 1.4) Status sweep: - PR 2.1 (#44344), PR 2.2 (#44345), PR 2.3 (#45043), PR 2.3b (#45364), PR 2.4 (#45237) all merged — task lists in §5/§6/§7/§7b/§8 now reflect completion. Items deferred to PR 2.5 (release notes) stay open. - §10.1 annotated with the 2026-05-14 local end-to-end validation result. Hydrant verification still required for customer readiness. New refinement — Decision 1.4 and §7c (PR 2.3c): - Surfaced during local validation: a user-installed Root CA ended up with origin=mdm because MDM CertificateList returned the entire keychain on InstallProfile ack, beating the next osquery sync. - Decision: osquery sync downgrades existing origin=mdm rows to origin=osquery when it also observes them. One-way (mdm → osquery, never reverse). - Rationale: osquery sees a strict superset of MDM CertificateList, so observation by both implies the cert is in the device keychain by some path — not evidence of MDM delivery. Origin should reflect "did Fleet deliver this?", not "who saw it first?" - Renewal correctness is unaffected; matcher keys on the fleet-<profile_uuid> marker, not on origin.
mostlikelee
added a commit
that referenced
this pull request
May 15, 2026
…ision 2.6) Removes the upload-time renewal-ID enforcement that PR 2.3 (#45043) and PR 2.3b (#45364) initially shipped. Marker becomes purely advisory: profiles that include it activate auto-renewal; profiles that don't continue to work as in 4.85, unchanged. Rationale (Decision 2.6 reversal): - GitOps trap: `fleetctl gitops apply` on existing customer profiles (Conditional Access, Okta Verify, ACME) would fail after upgrade because pre-4.86 profiles lack the marker. - UI-edit trap: opening an existing profile and saving an unrelated change would be rejected. - Conditional Access: Fleet's own generated profile lacks the marker (#45580), so customers following the published guide can't deploy. The cost of hard rejection — broken upgrades for customers who don't even use auto-renewal — exceeded the benefit of catching the misconfiguration that's only relevant to opt-in customers. Spec updates: - Decision 2.6: reversed entirely. Documents validator removal, GitOps-continuity rationale, and alternatives considered. - Decision 2.7: collapsed the "net-new surfaces accept preferred-only, OU-only" enforcement rules. Variable rename + substitution back-compat still documented. - Decision 1.2, 2.5: softened "customers must redeploy" to "customers may opt in by redeploying." - §7 / §7b task lists: struck through reverted tasks, kept history. - §7d added: revert tasks for removing the two Apple validators. - §9 (docs): framing shifted from "required migration" to "opt-in capability." Existing customers continue unchanged unless they want auto-renewal. - §10.6 / §10.7: validation-rejection QA replaced with opt-out path verification. Code revert work (§7d) will follow on a separate PR branch.
This was referenced May 15, 2026
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
2 participants
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 #45223
What this PR does
Adds
additionalNonProxiedSCEPValidation: any profile containing acom.apple.security.sceppayload AND no Fleet proxy variables must carry$FLEET_VAR_CERTIFICATE_RENEWAL_IDin the cert Subject OU, or the upload is rejected. Profiles using Fleet proxy variables (NDES / Custom SCEP / Smallstep) keep their existing per-CA validators.Why this is a follow-up to #45043
PR #45043 (2.3) added the ACME validator. Same renewal-ID requirement, same Decision 2.7 rationale, but it only fires on
com.apple.security.acmepayloads. Raw-SCEP profiles (literal Challenge/URL, no Fleet proxy vars — the Okta SCEP / Okta Verify-static-challenge flows named in #40639) still bypass all renewal-ID enforcement onmain. This PR closes that gap.What's new vs PR 2.3
$FLEET_VAR_CERTIFICATE_RENEWAL_IDis accepted. The legacy$FLEET_VAR_SCEP_RENEWAL_IDis rejected. Same net-new reasoning.validateConfigProfileFleetVariables's payload check now recognizes both ACME and SCEP viamobileconfig.HasPayloadTypeso the new validator runs before the fleetVars early-return.Checklist for submitter
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Summary by CodeRabbit
Bug Fixes
Tests