From 7375b88e65a9335aae6ffe6088c9af15cd97ce93 Mon Sep 17 00:00:00 2001 From: Dante Catalfamo <43040593+dantecatalfamo@users.noreply.github.com> Date: Wed, 10 Dec 2025 18:50:38 -0500 Subject: [PATCH] Sync app with server vars, fix retry logic (#36923) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Related issue:** Resolves #36591 # 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 ## Release Notes * **New Features** * Added automatic retry mechanism for failed certificate installations with up to 3 retry attempts. * Enhanced certificate installation status tracking and visibility. * **Bug Fixes** * Improved error handling and detailed error reporting for certificate enrollment failures. * **Tests** * Added comprehensive test coverage for certificate enrollment and status tracking workflows. ✏️ 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 | 36 +- .../agent/CertificateEnrollmentHandler.kt | 74 ++- .../fleetdm/agent/CertificateOrchestrator.kt | 133 +++++- .../java/com/fleetdm/agent/MainActivity.kt | 11 +- .../com/fleetdm/agent/scep/ScepClientImpl.kt | 5 +- android/app/src/main/res/values/strings.xml | 12 +- .../app/src/main/res/xml/app_restrictions.xml | 18 +- .../agent/CertificateEnrollmentHandlerTest.kt | 2 +- .../agent/CertificateOrchestratorTest.kt | 438 ++++++++++++++++++ .../fleetdm/agent/scep/ScepClientImplTest.kt | 2 +- .../fleetdm/agent/scep/ScepIntegrationTest.kt | 4 +- server/mdm/android/android.go | 2 +- 12 files changed, 637 insertions(+), 100 deletions(-) create mode 100644 android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt 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 85867015f8..34fe245f46 100644 --- a/android/app/src/main/java/com/fleetdm/agent/ApiClient.kt +++ b/android/app/src/main/java/com/fleetdm/agent/ApiClient.kt @@ -19,6 +19,7 @@ import kotlinx.coroutines.withContext import kotlinx.serialization.KSerializer import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable +import kotlinx.serialization.Transient import kotlinx.serialization.json.Json import kotlinx.serialization.json.JsonElement @@ -146,6 +147,8 @@ object ApiClient { ?: "HTTP $responseCode" } + Log.d("ApiClient", "server response from $method $endpoint ($responseCode): $response") + if (responseCode in 200..299) { val parsed = json.decodeFromString(string = response, deserializer = responseSerializer) Result.success(parsed) @@ -220,14 +223,14 @@ object ApiClient { val credentials = getEnrollmentCredentials() ?: return Result.failure(Exception("enroll credentials not set")) return makeRequest( - endpoint = "/api/fleetd/orbit/certificates/$certificateId", - method = "POST", + endpoint = "/api/fleetd/certificates/$certificateId", + method = "GET", body = GetCertificateTemplateRequest(orbitNodeKey = orbitNodeKey), bodySerializer = GetCertificateTemplateRequest.serializer(), - responseSerializer = GetCertificateTemplateResponse.serializer(), - authorized = false, + responseSerializer = GetCertificateTemplateResponseWrapper.serializer(), ).fold( - onSuccess = { res -> + onSuccess = { wrapper -> + val res = wrapper.certificate Log.i("ApiClient", "successfully retrieved certificate template ${res.id}: ${res.name}") Result.success( res.apply { @@ -436,8 +439,15 @@ private data class UpdateCertificateStatusResponse( val error: String? = null, ) +@Serializable +data class GetCertificateTemplateResponseWrapper( + @SerialName("certificate") + val certificate: GetCertificateTemplateResponse, +) + @Serializable data class GetCertificateTemplateResponse( + // CertificateTemplateResponseSummary @SerialName("id") val id: Int, @@ -445,7 +455,7 @@ data class GetCertificateTemplateResponse( val name: String, @SerialName("certificate_authority_id") - val certificateAuthorityId: String, + val certificateAuthorityId: Int, @SerialName("certificate_authority_name") val certificateAuthorityName: String, @@ -453,6 +463,7 @@ data class GetCertificateTemplateResponse( @SerialName("created_at") val createdAt: String, + // CertificateTemplateResponseFull @SerialName("subject_name") val subjectName: String, @@ -463,20 +474,21 @@ data class GetCertificateTemplateResponse( val status: String, @SerialName("scep_challenge") - val scepChallenge: String, + val scepChallenge: String? = "", @SerialName("fleet_challenge") - val fleetChallenge: String?, + val fleetChallenge: String? = "", - @SerialName("key_length") + @Transient val keyLength: Int = 2048, - @SerialName("signature_algorithm") + @Transient val signatureAlgorithm: String = "SHA256withRSA", - var url: String?, + @Transient + var url: String? = null, ) { fun setUrl(serverUrl: String, hostUUID: String) { - url = "$serverUrl/mdm/scep/proxy/$hostUUID,g$id,$certificateAuthorityType,$fleetChallenge" + url = "$serverUrl/mdm/scep/proxy/$hostUUID,g$id,$certificateAuthorityType,${fleetChallenge ?: ""}" } } diff --git a/android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.kt b/android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.kt index ef1c713fe8..393a020ddd 100644 --- a/android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.kt +++ b/android/app/src/main/java/com/fleetdm/agent/CertificateEnrollmentHandler.kt @@ -1,9 +1,12 @@ package com.fleetdm.agent +import com.fleetdm.agent.scep.ScepCertificateException import com.fleetdm.agent.scep.ScepClient import com.fleetdm.agent.scep.ScepConfig +import com.fleetdm.agent.scep.ScepCsrException import com.fleetdm.agent.scep.ScepEnrollmentException -import com.fleetdm.agent.scep.ScepException +import com.fleetdm.agent.scep.ScepKeyGenerationException +import com.fleetdm.agent.scep.ScepNetworkException import com.fleetdm.agent.scep.ScepResult import org.json.JSONObject import java.security.PrivateKey @@ -34,47 +37,42 @@ class CertificateEnrollmentHandler(private val scepClient: ScepClient, private v /** * Main enrollment flow: parse config, enroll via SCEP, install certificate. */ - suspend fun handleEnrollment(config: GetCertificateTemplateResponse): EnrollmentResult { - return try { - // Step 2: Perform SCEP enrollment - val result = performEnrollment(config) ?: return EnrollmentResult.Failure( - reason = "SCEP enrollment failed or returned null", - exception = null, - ) + suspend fun handleEnrollment(config: GetCertificateTemplateResponse): EnrollmentResult = try { + // Perform SCEP enrollment + val result = scepClient.enroll(config) - // Step 3: Install certificate - val installed = certificateInstaller.installCertificate( - config.name, - result.privateKey, - result.certificateChain.toTypedArray(), - ) + // Install certificate + val installed = certificateInstaller.installCertificate( + config.name, + result.privateKey, + result.certificateChain.toTypedArray(), + ) - if (installed) { - EnrollmentResult.Success(config.name) - } else { - EnrollmentResult.Failure("Certificate installation failed") - } - } catch (e: IllegalArgumentException) { - EnrollmentResult.Failure("Invalid configuration: ${e.message}", e) - } catch (e: Exception) { - EnrollmentResult.Failure("Unexpected error: ${e.message}", e) + if (installed) { + EnrollmentResult.Success(config.name) + } else { + EnrollmentResult.Failure("Certificate installation failed") } - } - - /** - * Performs SCEP enrollment, returning result or null on failure. - */ - @Suppress("SwallowedException") - suspend fun performEnrollment(config: GetCertificateTemplateResponse): ScepResult? = try { - scepClient.enroll(config) } catch (e: ScepEnrollmentException) { - // Enrollment failure is expected in some scenarios (pending approval, invalid challenge) - null - } catch (e: ScepException) { - // SCEP protocol errors are expected in some scenarios - null + // SCEP server rejected enrollment (e.g., PENDING status, invalid challenge) + EnrollmentResult.Failure("SCEP enrollment failed: ${e.message}", e) + } catch (e: ScepNetworkException) { + // Network communication failure - likely transient, can retry + EnrollmentResult.Failure("Network error during SCEP enrollment: ${e.message}", e) + } catch (e: ScepCertificateException) { + // Certificate validation or processing failed + EnrollmentResult.Failure("Certificate validation failed: ${e.message}", e) + } catch (e: ScepKeyGenerationException) { + // Key generation failed - device cryptography issue + EnrollmentResult.Failure("Failed to generate key pair: ${e.message}", e) + } catch (e: ScepCsrException) { + // CSR creation failed - likely configuration issue + EnrollmentResult.Failure("Failed to create CSR: ${e.message}", e) + } catch (e: IllegalArgumentException) { + // Configuration validation failed + EnrollmentResult.Failure("Invalid configuration: ${e.message}", e) } catch (e: Exception) { - // Unexpected errors are logged by the SCEP client - null + // Unexpected errors + EnrollmentResult.Failure("Unexpected error during enrollment: ${e.message}", e) } } 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 4961c68be4..4d90b68bba 100644 --- a/android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt +++ b/android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt @@ -12,12 +12,17 @@ import java.security.PrivateKey import java.security.cert.Certificate import kotlinx.coroutines.async import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.map import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock -import kotlinx.serialization.encodeToString +import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable import kotlinx.serialization.json.Json +const val MAX_CERT_INSTALL_RETRIES = 3 + /** * Orchestrates certificate enrollment operations by coordinating API calls, * SCEP enrollment, and certificate installation. @@ -60,6 +65,17 @@ object CertificateOrchestrator { // Mutex to protect concurrent access to certificate storage private val certificateStorageMutex = Mutex() + 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()) + emptyMap() + } + } + /** * Reads certificate IDs from Android Managed Configuration. * @@ -70,8 +86,8 @@ object CertificateOrchestrator { val restrictionsManager = context.getSystemService(Context.RESTRICTIONS_SERVICE) as android.content.RestrictionsManager val appRestrictions = restrictionsManager.applicationRestrictions - val certRequestList = appRestrictions.getParcelableArray("certificates", Bundle::class.java)?.toList() - return certRequestList?.map { bundle -> bundle.getInt("certificate_id") } + val certRequestList = appRestrictions.getParcelableArray("certificate_templates", Bundle::class.java)?.toList() + return certRequestList?.map { bundle -> bundle.getInt("id") } } /** @@ -80,7 +96,7 @@ object CertificateOrchestrator { * @param context Android context * @return Map of certificate ID to alias, or empty map if none stored */ - internal suspend fun getInstalledCertificates(context: Context): Map { + internal suspend fun getCertificateInstallInfos(context: Context): CertStatusMap { certificateStorageMutex.withLock { return try { val prefs = context.prefDataStore.data.first() @@ -91,7 +107,7 @@ object CertificateOrchestrator { return emptyMap() } - val map = json.decodeFromString>(jsonString) + val map = json.decodeFromString(jsonString) Log.d(TAG, "Loaded ${map.size} installed certificate(s) from DataStore") map } catch (e: Exception) { @@ -101,6 +117,38 @@ object CertificateOrchestrator { } } + internal suspend fun getCertificateInstallInfo(context: Context, certificateId: Int): CertificateInstallInfo? { + val certs = getCertificateInstallInfos(context = context) + return certs[certificateId] + } + + internal suspend fun markCertificateInstalled(context: Context, certificateId: Int, alias: String) { + val existingInfo = getCertificateInstallInfo(context = context, certificateId = certificateId) + ?: CertificateInstallInfo(alias = alias, status = CertificateInstallStatus.INSTALLED, retries = 0) + + val newInfo = existingInfo.copy(alias = alias, status = CertificateInstallStatus.INSTALLED, retries = 0) + storeCertificateInstallationInfo(context = context, certificateId = certificateId, certInstallInfo = newInfo) + } + + internal suspend fun markCertificateFailure(context: Context, certificateId: Int, alias: String): CertificateInstallInfo { + val existingInfo = getCertificateInstallInfo(context = context, certificateId = certificateId) + ?: CertificateInstallInfo(alias = alias, status = CertificateInstallStatus.RETRY, retries = 0) + + if (existingInfo.status != CertificateInstallStatus.RETRY) { + return existingInfo + } + + var newInfo = existingInfo.copy(retries = existingInfo.retries + 1) + + if (newInfo.retries >= MAX_CERT_INSTALL_RETRIES) { + newInfo = newInfo.copy(status = CertificateInstallStatus.FAILED) + } + + storeCertificateInstallationInfo(context = context, certificateId = certificateId, newInfo) + + return newInfo + } + /** * Stores a certificate ID→alias mapping in DataStore after successful installation. * This performs a read-modify-write operation to update the map. @@ -109,7 +157,7 @@ object CertificateOrchestrator { * @param certificateId Certificate template ID * @param alias Certificate alias used during installation */ - internal suspend fun storeCertificateInstallation(context: Context, certificateId: Int, alias: String) { + internal suspend fun storeCertificateInstallationInfo(context: Context, certificateId: Int, certInstallInfo: CertificateInstallInfo) { certificateStorageMutex.withLock { try { context.prefDataStore.edit { preferences -> @@ -117,7 +165,7 @@ object CertificateOrchestrator { val existingJsonString = preferences[INSTALLED_CERTIFICATES_KEY] val existingMap = if (existingJsonString != null) { try { - json.decodeFromString>(existingJsonString) + json.decodeFromString(existingJsonString) } catch (e: Exception) { Log.w(TAG, "Failed to parse existing certificates JSON, starting fresh: ${e.message}") emptyMap() @@ -128,14 +176,14 @@ object CertificateOrchestrator { // Add new mapping val updatedMap = existingMap.toMutableMap().apply { - put(certificateId, alias) + put(certificateId, certInstallInfo) } // Serialize and store val updatedJsonString = json.encodeToString(updatedMap) preferences[INSTALLED_CERTIFICATES_KEY] = updatedJsonString - Log.d(TAG, "Stored certificate mapping: $certificateId → $alias (total: ${updatedMap.size})") + Log.d(TAG, "Stored certificate mapping: $certificateId → ${certInstallInfo.alias} (total: ${updatedMap.size})") } } catch (e: Exception) { Log.e(TAG, "Failed to store certificate installation: ${e.message}", e) @@ -152,10 +200,10 @@ object CertificateOrchestrator { * @return Certificate alias if previously installed, null otherwise */ internal suspend fun getCertificateAlias(context: Context, certificateId: Int): String? { - val installedCerts = getInstalledCertificates(context) - val alias = installedCerts[certificateId] - Log.d(TAG, "Certificate $certificateId alias lookup: ${alias ?: "not found"}") - return alias + val installedCerts = getCertificateInstallInfos(context) + val status = installedCerts[certificateId] + Log.d(TAG, "Certificate $certificateId alias lookup: ${status?.alias ?: "not found"}") + return status?.alias } /** @@ -218,14 +266,22 @@ object CertificateOrchestrator { ): CertificateEnrollmentHandler.EnrollmentResult { Log.d(TAG, "Starting certificate enrollment for certificate ID: $certificateId") - // Step 1: Check if certificate is already installed (BEFORE API call) + // Check if certificate is already installed (BEFORE API call) if (isCertificateIdInstalled(context, certificateId)) { val alias = getCertificateAlias(context, certificateId)!! Log.i(TAG, "Certificate ID $certificateId (alias: '$alias') is already installed, skipping enrollment") return CertificateEnrollmentHandler.EnrollmentResult.Success(alias) } - // Step 2: Fetch certificate template from API (only if not already installed) + // Skip enrollment if already marked as permanently failed (max retries exceeded). + // Returns Success to prevent retry loops - the failure has already been reported + // to the Fleet server via updateCertificateStatus(). + val storedInfo = getCertificateInstallInfo(context = context, certificateId = certificateId) + if (storedInfo?.status == CertificateInstallStatus.FAILED) { + return CertificateEnrollmentHandler.EnrollmentResult.Success(storedInfo.alias) + } + + // Fetch certificate template from API (only if not already installed) val templateResult = ApiClient.getCertificateTemplate(certificateId) val template = templateResult.getOrElse { error -> Log.e(TAG, "Failed to fetch certificate template for ID $certificateId: ${error.message}", error) @@ -247,7 +303,7 @@ object CertificateOrchestrator { ) // Step 5: Perform enrollment - Log.d(TAG, "Starting SCEP enrollment for certificate: ${template.name}") + Log.d(TAG, "Starting SCEP enrollment for certificate: ${template.name}: $template") val result = handler.handleEnrollment(template) when (result) { @@ -261,16 +317,19 @@ object CertificateOrchestrator { } // Store certificate installation in DataStore - storeCertificateInstallation(context, certificateId, result.alias) + markCertificateInstalled(context, certificateId = certificateId, alias = template.name) } is CertificateEnrollmentHandler.EnrollmentResult.Failure -> { - Log.e(TAG, "Certificate enrollment failed for ID $certificateId: ${result.reason}", result.exception) - ApiClient.updateCertificateStatus( - certificateId = certificateId, - status = "failed", - detail = result.reason, - ).onFailure { error -> - Log.e(TAG, "Failed to update certificate status to failed for ID $certificateId: ${error.message}", error) + val updatedInfo = markCertificateFailure(context = context, certificateId = certificateId, alias = template.name) + if (!updatedInfo.shouldRetry()) { + Log.e(TAG, "Certificate enrollment failed for ID $certificateId: ${result.reason}", result.exception) + ApiClient.updateCertificateStatus( + certificateId = certificateId, + status = "failed", + detail = result.reason, + ).onFailure { error -> + Log.e(TAG, "Failed to update certificate status to failed for ID $certificateId: ${error.message}", error) + } } } } @@ -334,3 +393,29 @@ object CertificateOrchestrator { } } } + +typealias CertStatusMap = Map + +@Serializable +enum class CertificateInstallStatus { + @SerialName("installed") + INSTALLED, + + @SerialName("failed") + FAILED, + + @SerialName("retry") + RETRY, +} + +@Serializable +data class CertificateInstallInfo( + @SerialName("alias") + val alias: String, + @SerialName("status") + val status: CertificateInstallStatus, + @SerialName("retries") + val retries: Int = 0, +) { + fun shouldRetry(): Boolean = status == CertificateInstallStatus.RETRY && retries < (MAX_CERT_INSTALL_RETRIES) +} 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 f2798bca9f..f3e12ae455 100644 --- a/android/app/src/main/java/com/fleetdm/agent/MainActivity.kt +++ b/android/app/src/main/java/com/fleetdm/agent/MainActivity.kt @@ -36,11 +36,13 @@ import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.text.withStyle import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp +import androidx.datastore.preferences.core.stringPreferencesKey import com.fleetdm.agent.ui.theme.MyApplicationTheme import java.security.KeyStore import java.security.cert.X509Certificate import java.util.Date import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -62,10 +64,7 @@ class MainActivity : ComponentActivity() { } val androidID by remember { mutableStateOf(Settings.Secure.getString(contentResolver, Settings.Secure.ANDROID_ID)) } val enrollmentSpecificID by remember { mutableStateOf(appRestrictions.getString("host_uuid")) } - val certRequestList by remember { - mutableStateOf(appRestrictions.getParcelableArray("certificates", Bundle::class.java)?.toList()) - } - val certIds by remember { mutableStateOf(certRequestList?.map { bundle -> bundle.getInt("certificate_id") }) } + val certIds by remember { mutableStateOf(CertificateOrchestrator.getCertificateIDs(this)) } val permissionsList by remember { val grantedPermissions = mutableListOf() val packageInfo: PackageInfo = packageManager.getPackageInfo(packageName, PackageManager.GET_PERMISSIONS) @@ -88,6 +87,7 @@ class MainActivity : ComponentActivity() { var installedCertificates: List by remember { mutableStateOf(listOf()) } val apiKey by ApiClient.apiKeyFlow.collectAsState(initial = null) val baseUrl by ApiClient.baseUrlFlow.collectAsState(initial = null) + val allegedInstalledCerts by CertificateOrchestrator.installedCertsFlow(this).collectAsState(initial = "") LaunchedEffect(Unit) { installedCertificates = listKeystoreCertificates() @@ -112,7 +112,8 @@ class MainActivity : ComponentActivity() { KeyValue("server_url (MC)", fleetBaseUrl) KeyValue("orbit_node_key (datastore)", apiKey) KeyValue("base_url (datastore)", baseUrl) - KeyValue("certificate_ids", certIds.toString()) + KeyValue("certificate_templates->id", certIds.toString()) + KeyValue("alleged_installed", allegedInstalledCerts.toString()) PermissionList( permissionsList = permissionsList, ) diff --git a/android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt b/android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt index 905d74df93..091d1cb17c 100644 --- a/android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt +++ b/android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt @@ -1,5 +1,6 @@ package com.fleetdm.agent.scep +import android.util.Log import com.fleetdm.agent.GetCertificateTemplateResponse import org.bouncycastle.asn1.DERPrintableString import org.bouncycastle.asn1.pkcs.PKCSObjectIdentifiers @@ -32,6 +33,8 @@ import kotlinx.coroutines.withContext */ class ScepClientImpl : ScepClient { + val TAG = "ScepClientImpl" + companion object { private const val SCEP_PROFILE = "NDESCA" // Network Device Enrollment Service CA private const val SELF_SIGNED_CERT_VALIDITY_DAYS = 100L @@ -81,7 +84,7 @@ class ScepClientImpl : ScepClient { val client = Client(server, verifier) // Step 5: Build Certificate Signing Request (CSR) - val csr = buildCsr(entity, keyPair, config.scepChallenge, config.signatureAlgorithm) + val csr = buildCsr(entity, keyPair, config.scepChallenge ?: "", config.signatureAlgorithm) // Step 6: Send enrollment request val response = try { diff --git a/android/app/src/main/res/values/strings.xml b/android/app/src/main/res/values/strings.xml index 7d22be31e1..8c53ba4661 100644 --- a/android/app/src/main/res/values/strings.xml +++ b/android/app/src/main/res/values/strings.xml @@ -5,12 +5,12 @@ Secret used to enroll in a fleet instance Fleet Base URL The base URL of the fleet server - Certificates - Array of bundles containing certificate information - Certificate - Certificate information - Certificate ID - Certificate ID to be requested + Certificates templates + Array of bundles containing certificate template information + Certificate template + Certificate template information + Certificate template ID + Certificate template ID to be requested Host UUID The host UUID to present to fleet during enrollment diff --git a/android/app/src/main/res/xml/app_restrictions.xml b/android/app/src/main/res/xml/app_restrictions.xml index 86f9acb1e9..22ec51783f 100644 --- a/android/app/src/main/res/xml/app_restrictions.xml +++ b/android/app/src/main/res/xml/app_restrictions.xml @@ -20,19 +20,19 @@ android:description="@string/host_uuid_description" /> diff --git a/android/app/src/test/java/com/fleetdm/agent/CertificateEnrollmentHandlerTest.kt b/android/app/src/test/java/com/fleetdm/agent/CertificateEnrollmentHandlerTest.kt index d43d60eb5c..93efa585eb 100644 --- a/android/app/src/test/java/com/fleetdm/agent/CertificateEnrollmentHandlerTest.kt +++ b/android/app/src/test/java/com/fleetdm/agent/CertificateEnrollmentHandlerTest.kt @@ -181,7 +181,7 @@ class CertificateEnrollmentHandlerTest { ): GetCertificateTemplateResponse = GetCertificateTemplateResponse( id = id, name = name, - certificateAuthorityId = "ca-123", + certificateAuthorityId = 123, certificateAuthorityName = "Test CA", createdAt = "2024-01-01T00:00:00Z", subjectName = subjectName, diff --git a/android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt b/android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt new file mode 100644 index 0000000000..ed35221c2e --- /dev/null +++ b/android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt @@ -0,0 +1,438 @@ +package com.fleetdm.agent + +import android.content.Context +import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.core.stringPreferencesKey +import com.fleetdm.agent.scep.MockScepClient +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Ignore +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.annotation.Config +import java.security.PrivateKey +import java.security.cert.Certificate +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runTest +import kotlinx.serialization.json.Json + +/** + * Unit tests for CertificateOrchestrator with DataStore-based certificate tracking. + * + * Tests: + * - DataStore certificate tracking (JSON storage) + * - Mutex protection for concurrent operations + * - Optimized API call avoidance + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [33]) // Target SDK 33 for testing +class CertificateOrchestratorTest { + + private lateinit var context: Context + private lateinit var mockScepClient: MockScepClient + private lateinit var mockInstaller: MockCertificateInstaller + + private val json = Json { + ignoreUnknownKeys = true + encodeDefaults = true + } + + @Before + fun setup() = runTest { + context = RuntimeEnvironment.getApplication() + mockScepClient = MockScepClient() + mockInstaller = MockCertificateInstaller() + + // Clear DataStore before each test + clearDataStore() + } + + @After + fun tearDown() = runTest { + clearDataStore() + mockScepClient.reset() + mockInstaller.reset() + } + + // ========== Helper Functions ========== + + private suspend fun clearDataStore() { + context.prefDataStore.edit { preferences -> + preferences.clear() + } + } + + private suspend fun getStoredCertificates(): CertStatusMap { + val prefs = context.prefDataStore.data.first() + val jsonString = prefs[stringPreferencesKey("installed_certificates")] ?: return emptyMap() + return json.decodeFromString(jsonString) + } + + private suspend fun storeTestCertificateInDataStore( + certificateId: Int, + alias: String, + status: CertificateInstallStatus = CertificateInstallStatus.INSTALLED, + retries: Int = 0, + ) { + context.prefDataStore.edit { preferences -> + val existing = preferences[stringPreferencesKey("installed_certificates")]?.let { + json.decodeFromString(it) + } ?: emptyMap() + + val certInfo = CertificateInstallInfo(alias, status, retries) + val updated = existing.toMutableMap().apply { + put(certificateId, certInfo) + } + + val jsonString = json.encodeToString(updated) + preferences[stringPreferencesKey("installed_certificates")] = jsonString + } + } + + // ========== Mock Certificate Installer ========== + + class MockCertificateInstaller : CertificateEnrollmentHandler.CertificateInstaller { + var shouldSucceed = true + var wasInstallCalled = false + var capturedAlias: String? = null + val installedCertificates = mutableSetOf() + + override fun installCertificate(alias: String, privateKey: PrivateKey, certificateChain: Array): Boolean { + wasInstallCalled = true + capturedAlias = alias + if (shouldSucceed) { + installedCertificates.add(alias) + } + return shouldSucceed + } + + fun hasKeyPair(alias: String): Boolean = installedCertificates.contains(alias) + + fun reset() { + shouldSucceed = true + wasInstallCalled = false + capturedAlias = null + installedCertificates.clear() + } + } + + // ========== Test Category 1: DataStore Certificate Tracking ========== + + @Test + fun `storeCertificateInstallation stores certificate in DataStore`() = runTest { + // Act + CertificateOrchestrator.markCertificateInstalled(context, 123, "test-cert-1") + + // Assert + val stored = getStoredCertificates() + assertEquals(1, stored.size) + assertEquals("test-cert-1", stored[123]?.alias) + } + + @Test + fun `storeCertificateInstallation handles multiple certificates`() = runTest { + // Act + CertificateOrchestrator.markCertificateInstalled(context, 123, "cert-1") + CertificateOrchestrator.markCertificateInstalled(context, 456, "cert-2") + CertificateOrchestrator.markCertificateInstalled(context, 789, "cert-3") + + // Assert + val stored = getStoredCertificates() + assertEquals(3, stored.size) + assertEquals("cert-1", stored[123]?.alias) + assertEquals("cert-2", stored[456]?.alias) + assertEquals("cert-3", stored[789]?.alias) + } + + @Test + fun `storeCertificateInstallation updates existing certificate`() = runTest { + // Arrange + CertificateOrchestrator.markCertificateInstalled(context, 123, "old-alias") + + // Act - Update the same certificate ID + CertificateOrchestrator.markCertificateInstalled(context, 123, "new-alias") + + // Assert + val stored = getStoredCertificates() + assertEquals(1, stored.size) // Should not duplicate + assertEquals("new-alias", stored[123]?.alias) // Should be updated + } + + @Test + fun `getCertificateAlias returns null for non-existent certificate`() = runTest { + // Act + val alias = CertificateOrchestrator.getCertificateAlias(context, 999) + + // Assert + assertNull(alias) + } + + @Test + fun `getCertificateAlias retrieves stored certificate`() = runTest { + // Arrange + CertificateOrchestrator.markCertificateInstalled(context, 456, "my-cert") + + // Act + val alias = CertificateOrchestrator.getCertificateAlias(context, 456) + + // Assert + assertEquals("my-cert", alias) + } + + @Test + fun `getInstalledCertificates returns empty map when DataStore is empty`() = runTest { + // Act + val certificates = CertificateOrchestrator.getCertificateInstallInfos(context) + + // Assert + assertTrue(certificates.isEmpty()) + } + + @Test + fun `getInstalledCertificates recovers from malformed JSON`() = runTest { + // Arrange: Manually corrupt DataStore with invalid JSON + context.prefDataStore.edit { preferences -> + preferences[stringPreferencesKey("installed_certificates")] = "{ invalid json }" + } + + // Act: Should not throw, returns empty map + val certificates = CertificateOrchestrator.getCertificateInstallInfos(context) + + // Assert + assertTrue(certificates.isEmpty()) + + // Verify we can still store new certificates after recovery + CertificateOrchestrator.markCertificateInstalled(context, 111, "recovered-cert") + val stored = getStoredCertificates() + assertEquals(1, stored.size) + assertEquals("recovered-cert", stored[111]?.alias) + } + + // ========== Test Category 2: Optimized API Call Avoidance ========== + + @Ignore("Requires DevicePolicyManager mocking - TODO: redesign test or add DI") + @Test + fun `isCertificateIdInstalled returns true when certificate tracked and in keystore`() = runTest { + // Arrange + val certificateId = 123 + val alias = "device-cert" + + storeTestCertificateInDataStore(certificateId, alias) + mockInstaller.installedCertificates.add(alias) + + // Act + val result = CertificateOrchestrator.isCertificateIdInstalled(context, certificateId) + + // Assert + assertTrue(result) + } + + @Test + fun `isCertificateIdInstalled returns false when certificate not in DataStore`() = runTest { + // Act + val result = CertificateOrchestrator.isCertificateIdInstalled(context, 999) + + // Assert + assertFalse(result) + } + + @Test + fun `isCertificateIdInstalled returns false when certificate tracked but missing from keystore`() = runTest { + // Arrange: Store in DataStore but not in keystore + val certificateId = 456 + val alias = "missing-cert" + + storeTestCertificateInDataStore(certificateId, alias) + // Don't add to mockInstaller.installedCertificates + + // Note: isCertificateInstalled() uses real DevicePolicyManager, not mockInstaller + // So this test verifies DataStore logic only. The keystore check will return false + // because the certificate doesn't actually exist in Robolectric's shadow DPM. + + // Act + val result = CertificateOrchestrator.isCertificateIdInstalled(context, certificateId) + + // Assert + assertFalse(result) + } + + // ========== Test Category 3: Mutex Protection (Concurrency) ========== + + @Test + fun `concurrent certificate storage does not lose data`() = runTest { + // Arrange: 10 different certificate IDs + val certificateIds = (1..10).toList() + + // Act: Store all in parallel + val jobs = certificateIds.map { certId -> + launch { + CertificateOrchestrator.markCertificateInstalled( + context, + certId, + "cert-$certId", + ) + } + } + jobs.forEach { it.join() } + + // Assert: All 10 certificates should be stored + val stored = getStoredCertificates() + assertEquals("All 10 certificates should be stored", 10, stored.size) + + // Verify each certificate is present + certificateIds.forEach { certId -> + assertEquals("cert-$certId", stored[certId]?.alias) + } + } + + @Test + fun `rapid sequential certificate storage preserves all data`() = runTest { + // Act: Store 5 certificates rapidly in sequence + repeat(5) { index -> + CertificateOrchestrator.markCertificateInstalled(context, index * 100, "cert-$index") + } + + // Assert: All 5 should be stored + val stored = getStoredCertificates() + assertEquals(5, stored.size) + + repeat(5) { index -> + assertEquals("cert-$index", stored[index * 100]?.alias) + } + } + + @Test + fun `concurrent reads during writes see consistent data`() = runTest { + // Arrange: Pre-populate with some certificates + CertificateOrchestrator.markCertificateInstalled(context, 1, "cert-1") + CertificateOrchestrator.markCertificateInstalled(context, 2, "cert-2") + + // Act: Concurrent write and read + val writeJob = launch { + CertificateOrchestrator.markCertificateInstalled(context, 3, "cert-3") + } + + val readJob = launch { + val certificates = CertificateOrchestrator.getCertificateInstallInfos(context) + // Should see either 2 or 3 certificates (before or after write), but data should be consistent + assertTrue(certificates.size >= 2) + } + + writeJob.join() + readJob.join() + + // Assert: Final state should have all 3 + val stored = getStoredCertificates() + assertEquals(3, stored.size) + } + + // ========== Test Category 4: Integration Tests ========== + + @Test + fun `full enrollment flow stores certificate in DataStore after success`() = runTest { + // Note: This test is limited because we can't easily mock ApiClient (it's an object) + // Instead, we verify that if enrollment succeeds, DataStore storage happens + + // We'll test this by verifying the storeCertificateInstallation call happens + // after a successful mock enrollment via the handler directly + + val template = createMockTemplate(123, "test-cert") + + // Create handler with mock client and installer + val handler = CertificateEnrollmentHandler( + scepClient = mockScepClient, + certificateInstaller = mockInstaller, + ) + + // Act: Perform enrollment + val result = handler.handleEnrollment(template) + + // Assert: Enrollment succeeded + assertTrue(result is CertificateEnrollmentHandler.EnrollmentResult.Success) + + // Manually verify the pattern - orchestrator would call storeCertificateInstallation + val alias = (result as CertificateEnrollmentHandler.EnrollmentResult.Success).alias + CertificateOrchestrator.markCertificateInstalled(context, 123, alias) + + // Verify it was stored + val storedAlias = CertificateOrchestrator.getCertificateAlias(context, 123) + assertNotNull(storedAlias) + assertEquals(alias, storedAlias) + } + + @Test + fun `failed enrollment does not store in DataStore`() = runTest { + // Arrange: Make SCEP enrollment fail + mockScepClient.shouldThrowEnrollmentException = true + + val template = createMockTemplate(456, "failing-cert") + + val handler = CertificateEnrollmentHandler( + scepClient = mockScepClient, + certificateInstaller = mockInstaller, + ) + + // Act + val result = handler.handleEnrollment(template) + + // Assert: Enrollment failed + assertTrue(result is CertificateEnrollmentHandler.EnrollmentResult.Failure) + + // Verify nothing was stored (orchestrator wouldn't call store on failure) + val stored = getStoredCertificates() + assertTrue(stored.isEmpty()) + } + + @Test + fun `enrollment with custom installer uses provided installer`() = runTest { + // This test verifies the dependency injection pattern works + val customInstaller = MockCertificateInstaller() + val template = createMockTemplate(789, "custom-cert") + + val handler = CertificateEnrollmentHandler( + scepClient = mockScepClient, + certificateInstaller = customInstaller, + ) + + // Act + handler.handleEnrollment(template) + + // Assert: Custom installer was used + assertTrue(customInstaller.wasInstallCalled) + assertEquals("custom-cert", customInstaller.capturedAlias) + + // Original installer was not used + assertFalse(mockInstaller.wasInstallCalled) + } + + // ========== Helper Methods for Tests ========== + + private fun createMockTemplate( + id: Int, + name: String, + url: String = "https://scep.example.com/scep", + challenge: String = "test-challenge", + ): GetCertificateTemplateResponse = GetCertificateTemplateResponse( + id = id, + name = name, + certificateAuthorityId = 123, + certificateAuthorityName = "Test CA", + createdAt = "2024-01-01T00:00:00Z", + subjectName = "CN=$name,O=FleetDM", + certificateAuthorityType = "SCEP", + status = "active", + scepChallenge = challenge, + fleetChallenge = "fleet-secret", + keyLength = 2048, + signatureAlgorithm = "SHA256withRSA", + url = url, + ) +} diff --git a/android/app/src/test/java/com/fleetdm/agent/scep/ScepClientImplTest.kt b/android/app/src/test/java/com/fleetdm/agent/scep/ScepClientImplTest.kt index 8d582f0f4a..605cbcced0 100644 --- a/android/app/src/test/java/com/fleetdm/agent/scep/ScepClientImplTest.kt +++ b/android/app/src/test/java/com/fleetdm/agent/scep/ScepClientImplTest.kt @@ -69,7 +69,7 @@ class ScepClientImplTest { ): GetCertificateTemplateResponse = GetCertificateTemplateResponse( id = 1, name = "test-cert", - certificateAuthorityId = "ca-123", + certificateAuthorityId = 123, certificateAuthorityName = "Test CA", createdAt = "2024-01-01T00:00:00Z", subjectName = subjectName, diff --git a/android/app/src/test/java/com/fleetdm/agent/scep/ScepIntegrationTest.kt b/android/app/src/test/java/com/fleetdm/agent/scep/ScepIntegrationTest.kt index fd2a8edfe3..fc58bba6c0 100644 --- a/android/app/src/test/java/com/fleetdm/agent/scep/ScepIntegrationTest.kt +++ b/android/app/src/test/java/com/fleetdm/agent/scep/ScepIntegrationTest.kt @@ -64,7 +64,7 @@ class ScepIntegrationTest { ): GetCertificateTemplateResponse = GetCertificateTemplateResponse( id = 1, name = name, - certificateAuthorityId = "ca-123", + certificateAuthorityId = 123, certificateAuthorityName = "Test CA", createdAt = "2024-01-01T00:00:00Z", subjectName = subject, @@ -122,7 +122,7 @@ class ScepIntegrationTest { val uniqueId = System.currentTimeMillis() val template = createTemplate( url = testTemplate.url ?: "https://scep.example.com/scep", - challenge = testTemplate.scepChallenge, + challenge = testTemplate.scepChallenge ?: "test-challenge", name = "test-cert-$keySize-$uniqueId", subject = "CN=IntegrationTestDevice-$keySize-$uniqueId,O=FleetDM,C=US", keyLength = keySize, diff --git a/server/mdm/android/android.go b/server/mdm/android/android.go index 3b6be09228..55eb9f91c3 100644 --- a/server/mdm/android/android.go +++ b/server/mdm/android/android.go @@ -56,7 +56,7 @@ type AgentManagedConfiguration struct { ServerURL string `json:"server_url"` HostUUID string `json:"host_uuid"` EnrollSecret string `json:"enroll_secret"` - CertificateTemplateIDs []AgentCertificateTemplate `json:"certificate_template_ids,omitempty"` + CertificateTemplateIDs []AgentCertificateTemplate `json:"certificate_templates,omitempty"` } type AgentCertificateTemplate struct {