Android certificate renewal #38006
Android certificate renewal #38006
Conversation
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #38006 +/- ##
==========================================
- Coverage 65.86% 65.80% -0.06%
==========================================
Files 2389 2384 -5
Lines 190544 189022 -1522
Branches 8326 8402 +76
==========================================
- Hits 125493 124390 -1103
+ Misses 53641 53290 -351
+ Partials 11410 11342 -68
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:
|
dantecatalfamo
had a problem deploying
to
Docker Hub
GitHub Actions
Error
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR extends Android certificate handling to capture and propagate certificate metadata (expiration dates and serial number) throughout the enrollment and status reporting pipeline. Metadata is extracted during SCEP enrollment, persisted in orchestrator state, and included when reporting certificate installation status to the server. UUID change handling is also added to detect and re-enroll certificates when server-assigned identifiers change. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt (1)
615-622: Missing error handling for parsing operations.If
notAfter,notBefore, orserialNumbercontain malformed data (e.g., corrupted storage),parseISO8601()throwsIllegalArgumentExceptionandBigInteger()throwsNumberFormatException. This would crash the entire retry loop, preventing status reporting for other certificates.🐛 Suggested fix with error handling
val result = apiClient.updateCertificateStatus( certificateId = certId, status = UpdateCertificateStatusStatus.VERIFIED, operationType = operationType, - notAfter = state.notAfter?.let { parseISO8601(it) }, - notBefore = state.notBefore?.let { parseISO8601(it) }, - serialNumber = state.serialNumber?.let { BigInteger(it) }, + notAfter = state.notAfter?.let { + runCatching { parseISO8601(it) }.getOrNull() + }, + notBefore = state.notBefore?.let { + runCatching { parseISO8601(it) }.getOrNull() + }, + serialNumber = state.serialNumber?.let { + runCatching { BigInteger(it) }.getOrNull() + }, )
🤖 Fix all issues with AI agents
In @android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt:
- Line 20: Remove the unused import kotlin.math.exp from ScepClientImpl.kt;
locate the import statement at the top of the file and delete the line importing
kotlin.math.exp so there are no unused imports in the ScepClientImpl class.
🧹 Nitpick comments (2)
android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt (2)
74-83: Potential parse exception handling issue.
SimpleDateFormat.parse()throwsParseExceptionon invalid input rather than returningnull. The null-coalescing operator won't catch parsing failures - the exception will propagate.Additionally, the format
"yyyy-MM-dd'T'HH:mm:ss'Z'"doesn't handle fractional seconds (e.g.,"2025-01-01T12:00:00.123Z"), which could cause parsing failures if the server sends dates with milliseconds.🛠️ Suggested fix with proper exception handling and flexible parsing
private fun parseISO8601(dateString: String): Date { - val dateFormat = SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'", Locale.US) - dateFormat.timeZone = TimeZone.getTimeZone("UTC") - return dateFormat.parse(dateString) ?: throw IllegalArgumentException("Invalid ISO8601 date: $dateString") + val formats = listOf( + "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'", + "yyyy-MM-dd'T'HH:mm:ss'Z'" + ) + for (pattern in formats) { + try { + val dateFormat = SimpleDateFormat(pattern, Locale.US) + dateFormat.timeZone = TimeZone.getTimeZone("UTC") + return dateFormat.parse(dateString)!! + } catch (e: Exception) { + // Try next format + } + } + throw IllegalArgumentException("Invalid ISO8601 date: $dateString") }
675-680: Consider documenting or handling placeholder values more explicitly.Returning
Date(0)(epoch: 1970-01-01) andBigInteger.ZEROas placeholders when skipping enrollment is functional but could lead to confusion. Callers receiving these values might not know they're placeholders rather than actual certificate metadata.Consider either:
- Documenting in the
EnrollmentResult.Successclass that these values may be placeholders- Using nullable types in
EnrollmentResult.Successfor optional metadata- Adding a boolean flag like
isPlaceholderto distinguish skip vs. actual enrollment
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
android/app/src/main/java/com/fleetdm/agent/ApiClient.ktandroid/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.ktandroid/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.ktandroid/app/src/main/java/com/fleetdm/agent/MainActivity.ktandroid/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.ktandroid/app/src/main/java/com/fleetdm/agent/scep/ScepResult.ktandroid/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.ktandroid/app/src/test/java/com/fleetdm/agent/scep/MockScepClient.ktandroid/app/src/test/java/com/fleetdm/agent/testutil/FakeCertificateApiClient.kt
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36139
File: android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt:75-76
Timestamp: 2025-11-26T18:58:18.865Z
Learning: In Fleet's Android MDM agent SCEP implementation (android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt), OptimisticCertificateVerifier is intentionally used because: (1) SCEP URL is provided by authenticated MDM server, (2) challenge password authenticates enrollment, (3) enterprise SCEP servers use internal CAs not in system trust stores, (4) enrolled certificate is validated when used.
Learnt from: getvictor
Repo: fleetdm/fleet PR: 37640
File: android/app/src/main/java/com/fleetdm/agent/ApiClient.kt:518-522
Timestamp: 2026-01-02T22:48:09.865Z
Learning: In the Fleet Android app's SCEP proxy URL format (android/app/src/main/java/com/fleetdm/agent/ApiClient.kt), the `g` prefix before the certificate ID (e.g., `g$id`) is intentional and stands for "Google Android" to differentiate from other platforms like Apple and Windows.
📚 Learning: 2025-11-26T18:58:18.865Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36139
File: android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt:75-76
Timestamp: 2025-11-26T18:58:18.865Z
Learning: In Fleet's Android MDM agent SCEP implementation (android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt), OptimisticCertificateVerifier is intentionally used because: (1) SCEP URL is provided by authenticated MDM server, (2) challenge password authenticates enrollment, (3) enterprise SCEP servers use internal CAs not in system trust stores, (4) enrolled certificate is validated when used.
Applied to files:
android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.ktandroid/app/src/main/java/com/fleetdm/agent/scep/ScepResult.ktandroid/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.ktandroid/app/src/test/java/com/fleetdm/agent/scep/MockScepClient.ktandroid/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.ktandroid/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt
📚 Learning: 2026-01-02T22:48:09.865Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 37640
File: android/app/src/main/java/com/fleetdm/agent/ApiClient.kt:518-522
Timestamp: 2026-01-02T22:48:09.865Z
Learning: In the Fleet Android app's SCEP proxy URL format (android/app/src/main/java/com/fleetdm/agent/ApiClient.kt), the `g` prefix before the certificate ID (e.g., `g$id`) is intentional and stands for "Google Android" to differentiate from other platforms like Apple and Windows.
Applied to files:
android/app/src/main/java/com/fleetdm/agent/scep/ScepResult.ktandroid/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.ktandroid/app/src/main/java/com/fleetdm/agent/ApiClient.ktandroid/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.ktandroid/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt
📚 Learning: 2026-01-02T23:15:16.518Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 37640
File: android/app/src/main/java/com/fleetdm/agent/AgentApplication.kt:76-84
Timestamp: 2026-01-02T23:15:16.518Z
Learning: In the Fleet Android agent (android/app/src/main/java/com/fleetdm/agent/AgentApplication.kt), the enrollSecret is designed as a one-time credential. If an API key becomes invalid, the intended workflow is for customers to manually unenroll and re-enroll the device rather than automatically re-enrolling when credentials change. The logic that skips enrollment when ApiClient.getApiKey() != null is intentional.
Applied to files:
android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.ktandroid/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt
🧬 Code graph analysis (1)
android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt (1)
server/datastore/mysql/migrations/tables/20251124140138_CreateTableCertifcatesTemplates_test.go (1)
CertificateTemplateResult(33-39)
🔇 Additional comments (18)
android/app/src/main/java/com/fleetdm/agent/ApiClient.kt (4)
31-40: LGTM! ISO8601 date formatting looks correct.The extension function correctly formats dates to UTC ISO8601 format. Creating a new
SimpleDateFormatinstance per call avoids thread-safety issues with the class.
55-63: Interface extension looks good.The new optional parameters with null defaults maintain backward compatibility for any existing callers.
270-288: Implementation correctly propagates certificate metadata.The null-safe conversions (
?.toISO8601String(),?.toString()) properly handle optional values before sending to the API.
470-476: Data class fields correctly mapped to API contract.The
@SerialNameannotations properly map Kotlin field names to the expected API field names (not_valid_after,not_valid_before,serial).android/app/src/main/java/com/fleetdm/agent/MainActivity.kt (1)
233-238: LGTM! Debug display addition is straightforward.The UUID field is correctly added to the debug certificate list view, providing visibility into the certificate identifier for debugging purposes.
android/app/src/test/java/com/fleetdm/agent/scep/MockScepClient.kt (1)
63-75: LGTM! Mock correctly mirrors production metadata extraction.The mock implementation properly extracts certificate metadata from the generated self-signed certificate, ensuring tests exercise the same data flow as production code.
android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt (1)
101-113: LGTM! Metadata extraction is well-structured.The cast to
X509Certificateis safe since SCEP servers return X.509 certificates, and the empty-list check on lines 97-99 ensurescertificates.first()won't throw.android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.kt (2)
31-35: LGTM! EnrollmentResult.Success properly extended.The sealed class properly captures certificate metadata for propagation to the orchestrator layer. The fields match the non-null ScepResult properties.
51-57: Clean metadata passthrough from SCEP result to enrollment result.The values are correctly extracted from the SCEP result and forwarded without transformation.
android/app/src/main/java/com/fleetdm/agent/scep/ScepResult.kt (1)
17-23: LGTM! Well-documented data class expansion.The ScepResult now captures essential certificate metadata with proper KDoc documentation. Non-null fields are appropriate since a successful SCEP enrollment always yields a complete certificate with all these attributes.
android/app/src/test/java/com/fleetdm/agent/testutil/FakeCertificateApiClient.kt (2)
13-21: LGTM! Test utility correctly updated.The
UpdateStatusCalldata class properly captures the new metadata fields for test assertions. Nullable types correctly match the production interface.
44-64: LGTM! Method signature and call recording correctly extended.The fake client properly records all parameters including the new metadata fields, enabling comprehensive test assertions.
android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt (2)
927-932: LGTM! Clear comment explaining the updated cleanup behavior.The comment accurately explains why 2 cleanup results are expected: certificate 14 has
operation="remove", and certificate 20 has a UUID mismatch (null != "new-uuid"). The assertions correctly verify both certificates are processed.
959-1073: LGTM! Comprehensive test for certificate renewal via UUID change.This test thoroughly validates the UUID-based reinstallation flow:
- Certificate is already installed with old UUID
- Cleanup detects UUID mismatch and removes the certificate
- Enrollment reinstalls with new UUID
- All API calls and state transitions are verified
Good coverage of the critical renewal scenario described in issue #37567.
android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt (4)
351-354: LGTM! Core logic for UUID-based certificate renewal.The filter correctly identifies certificates for removal when:
operation == "remove"(explicit removal request), OR- Stored UUID doesn't match requested UUID (triggers reinstallation)
This enables the renewal flow where a UUID change causes the certificate to be uninstalled during cleanup and reinstalled during enrollment.
528-551: LGTM! Clean extension to persist certificate metadata for retry.The optional parameters allow storing certificate metadata (expiration dates, serial number) alongside the unreported status. This enables
retryUnreportedStatusesto include the metadata when retrying the API call, fulfilling the PR objective to include certificate expiration date in status reports.
736-759: LGTM! Proper metadata propagation for successful enrollment.The code correctly:
- Converts certificate metadata to strings for DataStore persistence
- Stores metadata with the unreported status for retry capability
- Passes the original typed values (Date, BigInteger) to the API call
This fulfills the PR objective to include
notAfter,notBefore, andserialNumberwhen reporting certificate installation status.
889-894: LGTM! Clean data model extension for certificate metadata.The new optional fields properly extend
CertificateStatewhile maintaining backward compatibility with existing stored data (vianulldefaults andexplicitNulls = falsein the JSON configuration). UsingString?for date storage is appropriate given the ISO8601 serialization approach.
android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt
Outdated
Show resolved
Hide resolved
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good overall. My main concern is that we don't want to remove a cert before we fetch its replacement.
android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt
Outdated
Show resolved
Hide resolved
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
dantecatalfamo
had a problem deploying
to
Docker Hub
GitHub Actions
Error
dantecatalfamo
had a problem deploying
to
Docker Hub
GitHub Actions
Error
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
|
@getvictor Changes made 👍 |
Related issue: Resolves #37567
Testing
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.