Repository navigation
Conversation
|
@coderabbitai full review |
|
/agentic_review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds delayed Windows SCEP status probes after proxied installation acknowledgments. The probes request certificate status and error details, and the service delays sending each probe for 30 seconds. Probe results can verify a profile, fail or retry an install, or keep it verifying and schedule another probe. The change also updates profile-status rollups when probe handling changes a profile. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The reviewed probe flow keeps unresolved certificate installs from releasing ESP and updates status when results arrive. No merge-blocking issue was identified in the inspected paths. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 48.48% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 11 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: Windows certificates reach verified status sooner to support timely ESP release. Description check The description includes the related issue, changes file, retry and timeout confirmation, automated test updates, and manual QA confirmation. It is sufficiently complete for this change. Linked Issues check The changes implement the coding requirements in #55239. Fleet queues delayed Windows SCEP status probes after proxied install acknowledgements. Each probe requestsStatusandErrorCodefor the co…Out of Scope Changes check The changes remain connected to #55239. Probe command identifiers, delayed delivery, reserved-command validation, probe result handling, profile status updates, and rollup updates support the Windows …
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Code Review by Qodo
1.
|
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.
🟡 Changes recommended
Probe scoping, duplicate acknowledgments, and failure handling can produce behavior inconsistent with the linked issue.
3 open findings
What changed in this PR
Adds Windows MDM SCEP status probes to verify certificates sooner, especially during Autopilot ESP.
Changes:
- Queues delayed SCEP status and error-code probes.
- Applies probe results to profile status and adds retry handling.
- Adds datastore and service tests.
| File | Description |
|---|---|
changes/55239-windows-scep-status-probe |
Changes entry; contents unavailable for review. |
server/fleet/microsoft_mdm.go |
Defines probe identifiers and delay. |
server/service/microsoft_mdm.go |
Delays recently queued probes. |
server/service/microsoft_mdm_test.go |
Tests delayed probe delivery. |
server/datastore/mysql/windows_scep_status_probe.go |
Builds, queues, and processes probes. |
server/datastore/mysql/windows_scep_status_probe_test.go |
Tests probe lifecycle and results. |
server/datastore/mysql/microsoft_mdm.go |
Integrates probes with response processing. |
server/datastore/mysql/microsoft_mdm_test.go |
Updates helper invocation. |
server/datastore/mysql/host_certificates_test.go |
Updates helper invocation. |
Files excluded by content exclusion policy (1)
- changes/55239-windows-scep-status-probe
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…erate duplicate probe insert - A repeated install ACK no longer sends a delivery the probe already verified back to verifying. - Queuing a probe that already exists is a no-op instead of failing the response save. - Parse the SCEP ErrorCode as a 32-bit value (CodeQL integer conversion).
…scep-status-probe
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55293 +/- ##
========================================
Coverage 77.19% 77.20%
========================================
Files 4299 4300 +1
Lines 266562 266785 +223
Branches 15682 15682
========================================
+ Hits 205779 205968 +189
- Misses 60597 60631 +34
Partials 186 186
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:
|
jbelbo
left a comment
There was a problem hiding this comment.
Looks good, left a few comments
| } | ||
|
|
||
| switch status { | ||
| case windowsSCEPStatusSucceeded: |
There was a problem hiding this comment.
If the issued cert doesn't carry the renewal ID Status=1 still marks this verified, and the 1h backstop only looks at verifying rows, so it never fires. Before, the admin got "Fleet did not detect the SCEP certificate". Now it will show verified while automatic renewal can't find the cert. Is that intended? Limiting the probe to the ESP, as Copilot suggested in one of the comments, would also keep osquery as the check everywhere else.
| if hp.OperationType == fleet.MDMOperationTypeInstall && payload.Status != nil && *payload.Status == fleet.MDMDeliveryVerified { | ||
| if _, ok := scepProxyProfileUUIDs[hp.ProfileUUID]; ok { | ||
| // A repeated ACK must not undo a verification this delivery already got. | ||
| alreadyVerified := hp.Status != nil && *hp.Status == fleet.MDMDeliveryVerified |
There was a problem hiding this comment.
Following up on the ACK fix: it covers verified, but the same late ACK after the probe failed the delivery (failed, or reset for a retry) still flips it back to verifying, and since the attempt-1 probe already exists no new one is queued, so it waits for the 1h backstop. I think only moving a row to verifying while it's still pending will cover this case too.
There was a problem hiding this comment.
I'll fix this one. It is unlikely (since it is a misconfiguration) so I'll widen the backstop to catch this case.
| scope = "./User" | ||
| } | ||
| // The SCEP node ID is the profile UUID. | ||
| node := fmt.Sprintf("%s/Vendor/MSFT/ClientCertificateInstall/SCEP/%s", scope, profileUUID) |
There was a problem hiding this comment.
Every delivery checks the same SCEP/<profile_uuid>/Status node, so I'm wondering what a probe sees after a retry or a renewal. If Windows doesn't reset Status when the new enrollment starts, could the probe pick up the previous attempt's 16 or 1? Did you get to try a redelivery on a device?
There was a problem hiding this comment.
The device runs the Exec that starts the new enrollment before it sends the ACK, so any probe, which always goes out after the ACK, sees the new attempt. Also, older answers are discarded anyway. A probe is tied to its install command's UUID, so an answer about an earlier delivery can't change a newer one.
Related issue: Resolves #55239
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.
Timeouts are implemented and retries are limited to avoid infinite loops
Testing
Summary by CodeRabbit