Remove certificates from device when missing from managed config (#37198)

<!-- Add the related story/sub-task/bug number, like Resolves #123, or
remove if NA -->
**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))


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## 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

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com>
This commit is contained in:
Dante Catalfamo
2025-12-22 16:28:50 -06:00
committed by GitHub
co-authored by Victor Lyuboslavsky
parent fbe21a951e
commit b2391c80b7
5 changed files with 348 additions and 11 deletions
@@ -230,10 +230,19 @@ object ApiClient {
)
}
suspend fun updateCertificateStatus(certificateId: Int, status: String, detail: String? = null): Result<Unit> = makeRequest(
suspend fun updateCertificateStatus(
certificateId: Int,
status: UpdateCertificateStatusStatus,
operationType: UpdateCertificateStatusOperation,
detail: String? = null,
): Result<Unit> = 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")
@@ -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(
@@ -70,7 +70,6 @@ object CertificateOrchestrator {
fun installedCertsFlow(context: Context): Flow<CertStatusMap> = 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<CertStatusMap>(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<Int>): Map<Int, CleanupResult> {
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<Int, CleanupResult>()
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()
}
@@ -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<DebugDestination> {
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(),
@@ -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(