Wait for CERT_INSTALL delegation to be available before attempting certificate enrollment - #43065
Conversation
…rtificate enrollment
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR addresses Android certificate enrollment failures that occur immediately after fresh MDM enrollment by waiting for the CERT_INSTALL delegated scope to be available before starting SCEP/certificate installation, and by improving installation failure diagnostics.
Changes:
- Gate
CertificateEnrollmentWorkerexecution onCERT_INSTALLdelegation availability (retry before any SCEP work). - Add a defensive delegation check in
AndroidCertificateInstallerand improve install failure logging/messages with alias/scopes context. - Add an Android changes entry documenting the fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| android/changes/42853-cert-install-delegation-gate | Release note for delegation gate + improved failure messages. |
| android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentWorker.kt | Adds worker-level delegation gate to retry before any enrollment work. |
| android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt | Adds installer-level delegation verification and improves install failure logging. |
| android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.kt | Improves failure messaging and adds handling for IllegalStateException from installer/system state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThe pull request implements a multi-layered gating mechanism for Android certificate enrollment based on CERT_INSTALL delegation scope availability. Changes include adding a pre-enrollment delegation check in CertificateEnrollmentWorker that retries if the scope is unavailable, a defensive delegation check in CertificateOrchestrator before certificate installation, enhanced failure messages that include the target alias and delegation status, and IllegalStateException handling in CertificateEnrollmentHandler for system state issues. The implementation defers enrollment until the required delegation becomes available, preventing permanent enrollment failure in scenarios where fresh MDM enrollment precedes delegation application. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.kt`:
- Around line 79-81: The catch block in CertificateEnrollmentHandler.kt that
handles IllegalStateException currently returns EnrollmentResult.Failure with
isRetryable = false; change it to treat delegation/state IllegalStateException
as retryable by returning EnrollmentResult.Failure("Certificate installation
failed: ${e.message}", e, isRetryable = true) in the catch for
IllegalStateException inside the method that performs certificate installation
(look for the catch handling IllegalStateException in
CertificateEnrollmentHandler).
In `@android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentWorker.kt`:
- Around line 35-42: The CERT_INSTALL delegation check in
CertificateEnrollmentWorker currently returns Result.retry() unconditionally and
bypasses your MAX_RETRY_ATTEMPTS limit; modify the early-return logic in the
CertificateEnrollmentWorker (where dpm/getDelegatedScopes and
DevicePolicyManager.DELEGATION_CERT_INSTALL are checked) to first examine the
worker's runAttemptCount (getRunAttemptCount()) and only return Result.retry()
if runAttemptCount < MAX_RETRY_ATTEMPTS, otherwise return Result.failure() (or a
non-retry terminal result) so the MAX_RETRY_ATTEMPTS cap is honored.
🪄 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: ecbd1513-ac73-474f-87a3-0ad5fd6d87d2
📒 Files selected for processing (4)
android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.ktandroid/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentWorker.ktandroid/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.ktandroid/changes/42853-cert-install-delegation-gate
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43065 +/- ##
==========================================
- Coverage 66.84% 66.83% -0.02%
==========================================
Files 2578 2578
Lines 206869 206883 +14
Branches 9283 9287 +4
==========================================
- Hits 138291 138277 -14
- Misses 56006 56037 +31
+ Partials 12572 12569 -3
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:
|
![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.
| } | ||
| Log.w(TAG, "CERT_INSTALL delegation not available yet (attempt $attempt/$MAX_DELEGATION_ATTEMPTS), will retry. Scopes: $scopes") | ||
| return Result.retry() | ||
| } |
There was a problem hiding this comment.
I am far from a Kotlin expert, but this method is called from doWork(). That method has a mutex, and the doEnrollment() worker will now hold the mutex while polling for delegation with 7 retries. Blocking any concurrent work (maybe?).
How many worker threads exist? Will it block other background tasks? Or just block the enrollment job.
All that is to say, should we move this tracking outside of the mutex into the doWork method?
override suspend fun doWork(): Result {
val attempt = runAttemptCount + 1
val dpm = applicationContext.getSystemService(DevicePolicyManager::class.java)
val scopes = dpm?.getDelegatedScopes(null, applicationContext.packageName) ?: emptyList()
if (!scopes.contains(DevicePolicyManager.DELEGATION_CERT_INSTALL)) {
if (attempt >= MAX_DELEGATION_ATTEMPTS) {
...
return Result.success()
}
...
return Result.retry()
}
if (!enrollmentMutex.tryLock()) { ... }
...
}
This also makes the separation of concerns cleaner, doWork() handles the WorkManager lifecycle. doEnrollment() does enrollment related tasks assuming all preconditions are met.
There was a problem hiding this comment.
@ksykulev The mutext is not held across retires. Separating concerns makes sense. I'll make the change.
…rtificate enrollment (#43065) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #43064 # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Certificate enrollment now verifies system delegation availability before attempting installation, preventing unnecessary failures. * **Bug Fixes** * Enhanced error messages to include specific certificate alias and delegation status information for better troubleshooting. * Improved handling of system state exceptions during the enrollment process. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 4457459)
Related issue: Resolves #43064
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes