Apple ACME profile validation + accept CERTIFICATE_RENEWAL_ID variable (PR 6/9)#45043
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 40639-cert-renew #45043 +/- ##
===================================================
Coverage ? 66.83%
===================================================
Files ? 2726
Lines ? 219217
Branches ? 10717
===================================================
Hits ? 146514
Misses ? 59526
Partials ? 13177
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:
|
Collaborator
|
@mostlikelee , removing the PR from the board (The ticket is there) |
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
mostlikelee
changed the title
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughThis PR introduces 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/service/apple_mdm.go`:
- Around line 663-664: The validation errors incorrectly reference only the OU
while the code accepts the renewal marker in either the certificate Subject CN
or OU; update the error messages near the checks that use
fleet.FleetVarRenewalIDRegexp against scepPayloadContent.CommonName and
scepPayloadContent.OrganizationalUnit so they mention both CN and OU (e.g.,
"must be in the SCEP certificate's subject CN or organizational unit (OU)").
Make this change for the occurrences around the existing check using
fleet.FleetVarRenewalIDRegexp and returning &fleet.BadRequestError{Message: ...}
(the instances at the current locations and the other similar checks referenced
in the file).
- Around line 782-783: The ACME validation currently only checks
fleet.FleetVarCertificateRenewalIDRegexp against commonName and orgUnit,
rejecting the older $FLEET_VAR_SCEP_RENEWAL_ID; replace that single-regex check
with the unified renewal-ID regex used elsewhere (the combined FleetVar
renewal-ID regex) so both $FLEET_VAR_CERTIFICATE_RENEWAL_ID and
$FLEET_VAR_SCEP_RENEWAL_ID are accepted when validating commonName.String() and
orgUnit.String().
In `@server/service/mdm_profiles.go`:
- Around line 511-517: The check uses a brittle substring search
(strings.Contains(profileContents, mobileconfig.ACMEPayloadType)) to skip
renewal-only validation; replace this with a structured ACME payload detection
by parsing the mobileconfig/profile payloads and verifying any payload has Type
== mobileconfig.ACMEPayloadType (i.e., implement or call a helper like
ProfileHasPayloadType(profileContents, mobileconfig.ACMEPayloadType) that
unmarshals the plist/JSON profile and inspects payload dictionaries). Update the
conditional to use that helper (referencing profileContents and
mobileconfig.ACMEPayloadType) so the renewal-ID-only bypass only triggers when
an actual ACME payload is present.
🪄 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
Run ID: ec5a257b-f08c-4376-a2e3-b9379e1586df
📒 Files selected for processing (10)
server/fleet/errors.goserver/fleet/mdm.goserver/fleet/mdm_test.goserver/mdm/apple/profile_processor.goserver/mdm/microsoft/profile_variables.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/integration_mdm_profiles_test.goserver/service/mdm_profiles.goserver/service/mdm_profiles_test.go
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.
![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.
2 tasks
sgress454
approved these changes
May 12, 2026
Contributor
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
Makes sense, one nit question.
| @@ -118,9 +119,17 @@ var ( | |||
| FleetVarNDESSCEPProxyURLRegexp = regexp.MustCompile(fmt.Sprintf(`(\$FLEET_VAR_%s)|(\${FLEET_VAR_%[1]s})`, FleetVarNDESSCEPProxyURL)) | |||
| FleetVarHostEndUserIDPFullnameRegexp = regexp.MustCompile(fmt.Sprintf(`(\$FLEET_VAR_%s)|(\${FLEET_VAR_%[1]s})`, FleetVarHostEndUserIDPFullname)) | |||
| FleetVarSCEPRenewalIDRegexp = regexp.MustCompile(fmt.Sprintf(`(\$FLEET_VAR_%s)|(\${FLEET_VAR_%[1]s})`, FleetVarSCEPRenewalID)) | |||
Contributor
There was a problem hiding this comment.
do we still need this or is it completely superseded by FleetVarRenewalIDRegexp?
Contributor
Author
There was a problem hiding this comment.
good catch, it removed
…ple-validation-rename
Net-new validation surface, so no back-compat for CN placement (per Decision 2.7). Error message already said "must be in the OU"; this aligns behavior with the message. Also drops dead FleetVarSCEPRenewalIDRegexp — superseded by FleetVarRenewalIDRegexp (unified, for back-compat sites) and FleetVarCertificateRenewalIDRegexp (preferred-only, for net-new validators). Zero callers.
Contributor
Author
|
@sgress454 note I made one more change after your review. The guides all point customer to do OU=renewalID, so I assume all the existing CN support is for some undocumented backwards compatibility. Since Acme and Non-proxied SCEP renewals are both net new functionality, I removed support for CN=renewalID on those. |
sgress454
approved these changes
May 12, 2026
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
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.
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
3 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 #44346
What this PR does
Adds a top-level Apple ACME profile validator: any profile containing a
com.apple.security.acmepayload must carry the renewal-ID marker variable in the cert Subject's CN or OU, or the upload is rejected.Adds
\$FLEET_VAR_CERTIFICATE_RENEWAL_IDas the preferred name for the renewal-ID marker, alongside the legacy\$FLEET_VAR_SCEP_RENEWAL_ID. Both are accepted by validation and both substitute tofleet-<profile_uuid>. Validation error messages reference only the new name.Why this is needed
Phase 2 (#40639) auto-renews certs from non-proxied CAs. Renewal works by matching ingested certs against
fleet-<profile_uuid>markers in their Subject — so the marker variable has to actually be in the profile. The ACME validator forces customers to include it before the profile can be uploaded.The variable rename matches the customer-facing docs (PR #44069); back-compat with the legacy name avoids breaking already-deployed profiles.
Checklist for submitter
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Summary by CodeRabbit
New Features
CERTIFICATE_RENEWAL_IDas the preferred Fleet variable for certificate renewal configurationChores
CERTIFICATE_RENEWAL_IDvariable name; legacySCEP_RENEWAL_IDremains supported for backward compatibility