From b2391c80b734387f91bc069d078b4e8deabb26b6 Mon Sep 17 00:00:00 2001 From: Dante Catalfamo <43040593+dantecatalfamo@users.noreply.github.com> Date: Mon, 22 Dec 2025 17:28:50 -0500 Subject: [PATCH] Remove certificates from device when missing from managed config (#37198) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Related issue:** Resolves #36690 # 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/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [ ] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [ ] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [ ] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [ ] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [ ] Confirmed that the fix is not expected to adversely impact load test results - [ ] Alerted the release DRI if additional load testing is needed ## Database migrations - [ ] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [ ] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [ ] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). ## New Fleet configuration settings - [ ] Setting(s) is/are explicitly excluded from GitOps If you didn't check the box above, follow this checklist for GitOps-enabled settings: - [ ] Verified that the setting is exported via `fleetctl generate-gitops` - [ ] Verified the setting is documented in a separate PR to [the GitOps documentation](https://github.com/fleetdm/fleet/blob/main/docs/Configuration/yaml-files.md#L485) - [ ] Verified that the setting is cleared on the server if it is not supplied in a YAML file (or that it is documented as being optional) - [ ] Verified that any relevant UI is disabled when GitOps mode is enabled ## fleetd/orbit/Fleet Desktop - [ ] Verified compatibility with the latest released version of Fleet (see [Must rule](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/workflows/fleetd-development-and-release-strategy.md)) - [ ] If the change applies to only one platform, confirmed that `runtime.GOOS` is used as needed to isolate changes - [ ] Verified that fleetd runs on macOS, Linux and Windows - [ ] Verified auto-update works from the released version of component to the new version (see [tools/tuf/test](../tools/tuf/test/README.md)) ## Summary by CodeRabbit * **New Features** * Automatic cleanup of certificates that have been removed from your system, now executed automatically before enrolling new certificates * Enhanced certificate operation tracking with improved status reporting for installation and removal operations, providing better visibility into certificate lifecycle events * **Bug Fixes** * Fixed back navigation behavior on the Debug screen, improving navigation flow for users ✏️ Tip: You can customize this high-level summary in your review settings. --------- Co-authored-by: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> --- .../main/java/com/fleetdm/agent/ApiClient.kt | 35 +++- .../agent/CertificateEnrollmentWorker.kt | 22 +++ .../fleetdm/agent/CertificateOrchestrator.kt | 177 +++++++++++++++++- .../java/com/fleetdm/agent/MainActivity.kt | 10 +- .../agent/CertificateOrchestratorTest.kt | 115 ++++++++++++ 5 files changed, 348 insertions(+), 11 deletions(-) diff --git a/android/app/src/main/java/com/fleetdm/agent/ApiClient.kt b/android/app/src/main/java/com/fleetdm/agent/ApiClient.kt index 80891b0e30..a5fa7b9773 100644 --- a/android/app/src/main/java/com/fleetdm/agent/ApiClient.kt +++ b/android/app/src/main/java/com/fleetdm/agent/ApiClient.kt @@ -230,10 +230,19 @@ object ApiClient { ) } - suspend fun updateCertificateStatus(certificateId: Int, status: String, detail: String? = null): Result = makeRequest( + suspend fun updateCertificateStatus( + certificateId: Int, + status: UpdateCertificateStatusStatus, + operationType: UpdateCertificateStatusOperation, + detail: String? = null, + ): Result = makeRequest( endpoint = "/api/fleetd/certificates/$certificateId/status", method = "PUT", - body = UpdateCertificateStatusRequest(status = status, detail = detail), + body = UpdateCertificateStatusRequest( + status = status, + operationType = operationType, + detail = detail, + ), bodySerializer = UpdateCertificateStatusRequest.serializer(), responseSerializer = UpdateCertificateStatusResponse.serializer(), ).fold( @@ -410,11 +419,31 @@ private data class GetCertificateTemplateRequest( @Serializable data class UpdateCertificateStatusRequest( @SerialName("status") - val status: String, + val status: UpdateCertificateStatusStatus, + @SerialName("operation_type") + val operationType: UpdateCertificateStatusOperation, @SerialName("detail") val detail: String? = null, ) +@Serializable +enum class UpdateCertificateStatusStatus { + @SerialName("verified") + VERIFIED, + + @SerialName("failed") + FAILED, +} + +@Serializable +enum class UpdateCertificateStatusOperation { + @SerialName("install") + INSTALL, + + @SerialName("remove") + REMOVE, +} + @Serializable private data class UpdateCertificateStatusResponse( @SerialName("error") diff --git a/android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentWorker.kt b/android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentWorker.kt index c751d1bf27..ea578aef2e 100644 --- a/android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentWorker.kt +++ b/android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentWorker.kt @@ -28,11 +28,33 @@ class CertificateEnrollmentWorker(context: Context, workerParams: WorkerParamete val certificateIds = CertificateOrchestrator.getCertificateIDs(applicationContext) + // STEP 1: Cleanup removed certificates BEFORE enrolling new ones + // This runs even if certificateIds is empty to clean up any orphaned certificates + val currentIds = certificateIds ?: emptyList() + val cleanupResults = CertificateOrchestrator.cleanupRemovedCertificates( + context = applicationContext, + currentCertificateIds = currentIds, + ) + + // Log cleanup results + cleanupResults.forEach { (certId, result) -> + when (result) { + is CleanupResult.Success -> + Log.i(TAG, "Cleaned up certificate $certId (alias: ${result.alias})") + is CleanupResult.AlreadyRemoved -> + Log.i(TAG, "Certificate $certId already removed (alias: ${result.alias})") + is CleanupResult.Failure -> + Log.e(TAG, "Failed to cleanup certificate $certId: ${result.reason}", result.exception) + } + } + + // STEP 2: If no certificates to enroll, we're done if (certificateIds.isNullOrEmpty()) { Log.d(TAG, "No certificates to enroll") return Result.success() } + // STEP 3: Enroll new/updated certificates Log.i(TAG, "Enrolling ${certificateIds.size} certificate(s)") val results = CertificateOrchestrator.enrollCertificates( diff --git a/android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt b/android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt index 5c3da8a2c4..195fbf7c0c 100644 --- a/android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt +++ b/android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt @@ -70,7 +70,6 @@ object CertificateOrchestrator { fun installedCertsFlow(context: Context): Flow = context.prefDataStore.data.map { preferences -> try { val jsonStr = preferences[INSTALLED_CERTIFICATES_KEY] - Log.d("installedCertsFlow", "json: $jsonStr") json.decodeFromString(jsonStr!!) } catch (e: Exception) { Log.d("installedCertsFlow", e.toString()) @@ -194,6 +193,46 @@ object CertificateOrchestrator { } } + /** + * Removes a certificate installation record from DataStore. + * + * @param context Android context + * @param certificateId Certificate template ID to remove + */ + internal suspend fun removeCertificateInstallInfo(context: Context, certificateId: Int) { + certificateStorageMutex.withLock { + try { + context.prefDataStore.edit { preferences -> + val existingJsonString = preferences[INSTALLED_CERTIFICATES_KEY] + val existingMap = if (existingJsonString != null) { + try { + json.decodeFromString(existingJsonString) + } catch (e: Exception) { + Log.w(TAG, "Failed to parse existing certificates JSON: ${e.message}") + emptyMap() + } + } else { + emptyMap() + } + + // Remove the entry + val updatedMap = existingMap.toMutableMap().apply { + remove(certificateId) + } + + // Serialize and store + val updatedJsonString = json.encodeToString(updatedMap) + preferences[INSTALLED_CERTIFICATES_KEY] = updatedJsonString + + Log.d(TAG, "Removed certificate mapping for ID $certificateId (remaining: ${updatedMap.size})") + } + } catch (e: Exception) { + Log.e(TAG, "Failed to remove certificate installation info: ${e.message}", e) + // Non-fatal error - cleanup was attempted + } + } + } + /** * Retrieves the certificate alias for a given certificate ID from DataStore. * @@ -225,6 +264,43 @@ object CertificateOrchestrator { false } + /** + * Removes a certificate keypair from the Android keystore. + * + * @param context Android context + * @param alias Certificate alias to remove + * @return True if removal was successful or certificate doesn't exist + */ + private fun removeKeyPair(context: Context, alias: String): Boolean { + return try { + val dpm = context.getSystemService(Context.DEVICE_POLICY_SERVICE) as DevicePolicyManager + + // First check if keypair exists + if (!dpm.hasKeyPair(alias)) { + Log.i(TAG, "Certificate '$alias' doesn't exist in keystore, considering removal successful") + return true + } + + // Attempt to remove the keypair + // admin component is null because we're using delegated certificate management + val removed = dpm.removeKeyPair(null, alias) + + if (removed) { + Log.i(TAG, "Successfully removed certificate keypair with alias: $alias") + } else { + Log.e(TAG, "Failed to remove certificate keypair '$alias'. Check MDM policy and delegation status.") + } + + removed + } catch (e: SecurityException) { + Log.e(TAG, "Security exception removing certificate '$alias': ${e.message}", e) + false + } catch (e: Exception) { + Log.e(TAG, "Error removing certificate '$alias': ${e.message}", e) + false + } + } + /** * Checks if a certificate ID has been successfully installed and still exists in keystore. * This is a fast check that doesn't require fetching the template from the API. @@ -250,6 +326,90 @@ object CertificateOrchestrator { return existsInKeystore } + /** + * Cleans up certificates that were removed from managed configuration. + * + * This function: + * 1. Identifies certificates in DataStore that are no longer in current config + * 2. Removes the corresponding keypairs from the device using DevicePolicyManager + * 3. Cleans up the DataStore tracking + * 4. Reports removal status to the server + * + * @param context Android context for certificate operations + * @param currentCertificateIds List of certificate IDs from current managed configuration + * @return Map of certificate ID to cleanup result + */ + suspend fun cleanupRemovedCertificates(context: Context, currentCertificateIds: List): Map { + Log.d(TAG, "Starting certificate cleanup. Current IDs: $currentCertificateIds") + + // Get all installed certificates from DataStore + val installedCerts = getCertificateInstallInfos(context) + Log.d(TAG, "Found ${installedCerts.size} certificate(s) in DataStore") + + // Identify certificates to remove (in DataStore but not in current config) + val certificatesToRemove = installedCerts.keys.filter { it !in currentCertificateIds } + + if (certificatesToRemove.isEmpty()) { + Log.d(TAG, "No certificates to remove") + return emptyMap() + } + + Log.i(TAG, "Removing ${certificatesToRemove.size} certificate(s): $certificatesToRemove") + + val results = mutableMapOf() + + for (certificateId in certificatesToRemove) { + val certInfo = installedCerts[certificateId] + if (certInfo == null) { + Log.w(TAG, "Certificate ID $certificateId not found in DataStore, skipping") + continue + } + + val alias = certInfo.alias + Log.d(TAG, "Removing certificate ID $certificateId with alias '$alias' (status: ${certInfo.status})") + + // Attempt to remove the keypair + val removed = removeKeyPair(context, alias) + + if (removed) { + // Report successful removal to server + ApiClient.updateCertificateStatus( + certificateId = certificateId, + status = UpdateCertificateStatusStatus.VERIFIED, + operationType = UpdateCertificateStatusOperation.REMOVE, + ).onFailure { error -> + Log.e(TAG, "Failed to report certificate removal status for ID $certificateId: ${error.message}", error) + } + + // Clean up DataStore + removeCertificateInstallInfo(context, certificateId) + + results[certificateId] = CleanupResult.Success(alias) + Log.i(TAG, "Successfully removed certificate ID $certificateId (alias: '$alias')") + } else { + // Report failure to server + val errorDetail = "Failed to remove certificate keypair from device" + ApiClient.updateCertificateStatus( + certificateId = certificateId, + status = UpdateCertificateStatusStatus.FAILED, + operationType = UpdateCertificateStatusOperation.REMOVE, + detail = errorDetail, + ).onFailure { error -> + Log.e(TAG, "Failed to report certificate removal failure for ID $certificateId: ${error.message}", error) + } + + results[certificateId] = CleanupResult.Failure( + reason = errorDetail, + exception = null, + shouldRetry = false, // Permission or configuration issue, don't retry + ) + Log.e(TAG, "Failed to remove certificate ID $certificateId (alias: '$alias')") + } + } + + return results + } + /** * Enrolls a single certificate by fetching its template from the API, * performing SCEP enrollment, and installing it on the device. @@ -320,7 +480,8 @@ object CertificateOrchestrator { Log.i(TAG, "Certificate enrollment successful for ID $certificateId with alias: ${result.alias}") ApiClient.updateCertificateStatus( certificateId = certificateId, - status = "verified", + status = UpdateCertificateStatusStatus.VERIFIED, + operationType = UpdateCertificateStatusOperation.INSTALL, ).onFailure { error -> Log.e(TAG, "Failed to update certificate status to verified for ID $certificateId: ${error.message}", error) } @@ -334,7 +495,8 @@ object CertificateOrchestrator { Log.e(TAG, "Certificate enrollment failed for ID $certificateId: ${result.reason}", result.exception) ApiClient.updateCertificateStatus( certificateId = certificateId, - status = "failed", + status = UpdateCertificateStatusStatus.FAILED, + operationType = UpdateCertificateStatusOperation.INSTALL, detail = result.reason, ).onFailure { error -> Log.e(TAG, "Failed to update certificate status to failed for ID $certificateId: ${error.message}", error) @@ -428,3 +590,12 @@ data class CertificateInstallInfo( ) { fun shouldRetry(): Boolean = status == CertificateInstallStatus.RETRY && retries < (MAX_CERT_INSTALL_RETRIES) } + +/** + * Result of certificate cleanup operation + */ +sealed class CleanupResult { + data class Success(val alias: String) : CleanupResult() + data class Failure(val reason: String, val exception: Exception?, val shouldRetry: Boolean) : CleanupResult() + data class AlreadyRemoved(val alias: String) : CleanupResult() +} diff --git a/android/app/src/main/java/com/fleetdm/agent/MainActivity.kt b/android/app/src/main/java/com/fleetdm/agent/MainActivity.kt index a4b8d380d1..b089214ce7 100644 --- a/android/app/src/main/java/com/fleetdm/agent/MainActivity.kt +++ b/android/app/src/main/java/com/fleetdm/agent/MainActivity.kt @@ -40,7 +40,6 @@ import androidx.compose.material3.Scaffold import androidx.compose.material3.Text import androidx.compose.material3.TopAppBar import androidx.compose.runtime.Composable -import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember @@ -57,6 +56,7 @@ import androidx.compose.ui.text.withStyle import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.core.net.toUri +import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.navigation.compose.NavHost import androidx.navigation.compose.composable import androidx.navigation.compose.rememberNavController @@ -106,7 +106,7 @@ fun AppNavigation() { composable { DebugScreen( - onNavigateBack = { navController.popBackStack() }, + onNavigateBack = { navController.navigateUp() }, ) } } @@ -117,7 +117,7 @@ fun MainScreen(onNavigateToDebug: () -> Unit) { val context = LocalContext.current var versionClicks by remember { mutableStateOf(0) } - val installedCerts by CertificateOrchestrator.installedCertsFlow(context).collectAsState(initial = emptyMap()) + val installedCerts by CertificateOrchestrator.installedCertsFlow(context).collectAsStateWithLifecycle(initialValue = emptyMap()) Scaffold( modifier = Modifier.fillMaxSize(), @@ -180,8 +180,8 @@ fun DebugScreen(onNavigateBack: () -> Unit) { grantedPermissions.toList() } val fleetBaseUrl = remember { appRestrictions.getString("server_url") } - val baseUrl by ApiClient.baseUrlFlow.collectAsState(initial = null) - val installedCerts by CertificateOrchestrator.installedCertsFlow(context).collectAsState(initial = emptyMap()) + val baseUrl by ApiClient.baseUrlFlow.collectAsStateWithLifecycle(initialValue = null) + val installedCerts by CertificateOrchestrator.installedCertsFlow(context).collectAsStateWithLifecycle(initialValue = emptyMap()) Scaffold( modifier = Modifier.fillMaxSize(), diff --git a/android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt b/android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt index ed35221c2e..efadd1ca2e 100644 --- a/android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt +++ b/android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt @@ -413,6 +413,121 @@ class CertificateOrchestratorTest { assertFalse(mockInstaller.wasInstallCalled) } + // ========== Test Category 4: Certificate Cleanup ========== + + @Test + fun `removeCertificateInstallInfo removes certificate from DataStore`() = runTest { + // Arrange: Store 3 certificates + storeTestCertificateInDataStore(1, "cert-1") + storeTestCertificateInDataStore(2, "cert-2") + storeTestCertificateInDataStore(3, "cert-3") + + // Act: Remove certificate 2 + CertificateOrchestrator.removeCertificateInstallInfo(context, 2) + + // Assert: Only 1 and 3 remain + val stored = getStoredCertificates() + assertEquals(2, stored.size) + assertTrue(stored.containsKey(1)) + assertFalse(stored.containsKey(2)) + assertTrue(stored.containsKey(3)) + assertEquals("cert-1", stored[1]?.alias) + assertEquals("cert-3", stored[3]?.alias) + } + + @Test + fun `removeCertificateInstallInfo handles non-existent certificate gracefully`() = runTest { + // Arrange: Store 2 certificates + storeTestCertificateInDataStore(1, "cert-1") + storeTestCertificateInDataStore(2, "cert-2") + + // Act: Try to remove non-existent certificate + CertificateOrchestrator.removeCertificateInstallInfo(context, 999) + + // Assert: No exception thrown, DataStore unchanged + val stored = getStoredCertificates() + assertEquals(2, stored.size) + assertTrue(stored.containsKey(1)) + assertTrue(stored.containsKey(2)) + } + + @Test + fun `removeCertificateInstallInfo handles corrupted DataStore gracefully`() = runTest { + // Arrange: Corrupt DataStore with invalid JSON + context.prefDataStore.edit { preferences -> + preferences[stringPreferencesKey("installed_certificates")] = "{ invalid json }" + } + + // Act: Try to remove certificate (should not throw) + CertificateOrchestrator.removeCertificateInstallInfo(context, 123) + + // Assert: No exception, operation succeeds + // DataStore should be cleared/reset + val stored = getStoredCertificates() + assertTrue(stored.isEmpty()) + } + + @Test + fun `concurrent removeCertificateInstallInfo operations are thread-safe`() = runTest { + // Arrange: Store 10 certificates + repeat(10) { id -> + storeTestCertificateInDataStore(id + 1, "cert-${id + 1}") + } + + // Act: Remove certificates 2, 4, 6, 8, 10 in parallel + val jobs = listOf(2, 4, 6, 8, 10).map { certId -> + launch { + CertificateOrchestrator.removeCertificateInstallInfo(context, certId) + } + } + jobs.forEach { it.join() } + + // Assert: Only odd-numbered certificates remain (1, 3, 5, 7, 9) + val stored = getStoredCertificates() + assertEquals(5, stored.size) + listOf(1, 3, 5, 7, 9).forEach { id -> + assertTrue("Certificate $id should exist", stored.containsKey(id)) + } + listOf(2, 4, 6, 8, 10).forEach { id -> + assertFalse("Certificate $id should not exist", stored.containsKey(id)) + } + } + + @Test + fun `cleanupRemovedCertificates returns empty map when DataStore is empty`() = runTest { + // Arrange: DataStore is empty (default state after clearDataStore in setup) + + // Act: Call cleanup with some certificate IDs + val results = CertificateOrchestrator.cleanupRemovedCertificates( + context = context, + currentCertificateIds = listOf(1, 2, 3), + ) + + // Assert: No cleanup performed + assertTrue(results.isEmpty()) + } + + @Test + fun `cleanupRemovedCertificates returns empty map when all certificates still in config`() = runTest { + // Arrange: Store 3 certificates + storeTestCertificateInDataStore(1, "cert-1") + storeTestCertificateInDataStore(2, "cert-2") + storeTestCertificateInDataStore(3, "cert-3") + + // Act: Call cleanup with the same certificate IDs + val results = CertificateOrchestrator.cleanupRemovedCertificates( + context = context, + currentCertificateIds = listOf(1, 2, 3), + ) + + // Assert: No cleanup performed + assertTrue(results.isEmpty()) + + // Verify certificates still in DataStore + val stored = getStoredCertificates() + assertEquals(3, stored.size) + } + // ========== Helper Methods for Tests ========== private fun createMockTemplate(