From edfa694ac220bcf93f8017fca84dc8f8144eb135 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> Date: Thu, 2 Apr 2026 10:44:50 -0500 Subject: [PATCH] Fixed certificate template fetch failing with DNS errors (and other issues) (#42625) **Related issue:** Resolves #42624 **Related issue:** Resolves #37546 - Fixed certificate template fetch failing with DNS errors (known Android issue) - stop polling certs that failed permanently - CertificateOrchestrator: When server returns template status "failed", mark the certificate as locally failed (markCertificateForceFailed) and stop polling - CertificateOrchestrator: Non-retryable SCEP failures (e.g. ScepEnrollmentException) now immediately mark as failed and report to server, skipping the 3-attempt retry logic - CertificateOrchestrator: recordEnrollmentAttemptFailure now stores the uuid, fixing a bug where the FAILED guard was bypassed because stored uuid was empty - CertificateOrchestrator: Renamed markCertificateFailure to recordEnrollmentAttemptFailure and added markCertificateForceFailed for clarity # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit * **Bug Fixes** * Fixed certificate template retrieval failures that displayed misleading DNS errors. Optimized HTTP request header handling for GET requests to prevent these errors during certificate enrollment operations. --- android/app/detekt.yml | 3 +- .../main/java/com/fleetdm/agent/ApiClient.kt | 63 ++++++---- .../fleetdm/agent/CertificateOrchestrator.kt | 47 ++++++-- .../com/fleetdm/agent/scep/ScepClientImpl.kt | 7 +- .../fleetdm/agent/ApiClientReenrollTest.kt | 10 ++ .../agent/CertificateOrchestratorTest.kt | 112 ++++++++++++++++++ android/changes/.keep | 0 ...-fix-certificate-template-get-content-type | 6 + 8 files changed, 214 insertions(+), 34 deletions(-) create mode 100644 android/changes/.keep create mode 100644 android/changes/42624-fix-certificate-template-get-content-type diff --git a/android/app/detekt.yml b/android/app/detekt.yml index a19ae50336..823a80aa9c 100644 --- a/android/app/detekt.yml +++ b/android/app/detekt.yml @@ -13,8 +13,7 @@ style: MaxLineLength: active: false # Handled by ktlint ReturnCount: - active: true - max: 5 + active: false complexity: CognitiveComplexMethod: 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 d85235f4a8..ec7524d3e8 100644 --- a/android/app/src/main/java/com/fleetdm/agent/ApiClient.kt +++ b/android/app/src/main/java/com/fleetdm/agent/ApiClient.kt @@ -1,6 +1,7 @@ package com.fleetdm.agent import android.content.Context +import android.net.ConnectivityManager import android.util.Log import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences @@ -10,10 +11,7 @@ import androidx.datastore.preferences.preferencesDataStore import java.math.BigInteger import java.net.HttpURLConnection import java.net.URL -import java.text.SimpleDateFormat import java.util.Date -import java.util.Locale -import java.util.TimeZone import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.first @@ -64,6 +62,7 @@ object ApiClient : CertificateApiClient { private val json = Json { ignoreUnknownKeys = true } private lateinit var dataStore: DataStore + private lateinit var appContext: Context private val API_KEY = stringPreferencesKey("api_key") private val SERVER_URL_KEY = stringPreferencesKey("server_url") private val ENROLL_SECRET = stringPreferencesKey("enroll_secret") @@ -74,8 +73,9 @@ object ApiClient : CertificateApiClient { fun initialize(context: Context) { Log.d(TAG, "initializing api client") + appContext = context.applicationContext if (!::dataStore.isInitialized) { - dataStore = context.applicationContext.prefDataStore + dataStore = appContext.prefDataStore } } @@ -116,6 +116,8 @@ object ApiClient : CertificateApiClient { responseSerializer: KSerializer, authorized: Boolean = true, ): Result = withContext(Dispatchers.IO) { + require(method != "GET" || body == null) { "GET requests must not include a body" } + var connection: HttpURLConnection? = null try { val baseUrl = getBaseUrl() ?: return@withContext Result.failure( @@ -137,13 +139,12 @@ object ApiClient : CertificateApiClient { } val url = URL("$baseUrl$endpoint") - connection = url.openConnection() as HttpURLConnection + connection = openConnectionOnActiveNetwork(url) connection.apply { requestMethod = method useCaches = false doInput = true - setRequestProperty("Content-Type", "application/json") if (authorized) { getNodeKeyOrEnroll().fold( onFailure = { throwable -> return@withContext Result.failure(throwable) }, @@ -155,8 +156,9 @@ object ApiClient : CertificateApiClient { connectTimeout = 15000 readTimeout = 15000 - if (body != null && method != "GET") { + if (body != null) { requireNotNull(bodySerializer) { "bodySerializer required when body is provided" } + setRequestProperty("Content-Type", "application/json") doOutput = true val bodyJson = json.encodeToString(value = body, serializer = bodySerializer) outputStream.use { it.write(bodyJson.toByteArray()) } @@ -178,6 +180,8 @@ object ApiClient : CertificateApiClient { Result.success(parsed) } else if (responseCode == 401) { Result.failure(UnauthorizedException(response)) + } else if (responseCode == 404) { + Result.failure(NotFoundException(response)) } else { Result.failure(Exception("HTTP $responseCode: $response")) } @@ -193,6 +197,26 @@ object ApiClient : CertificateApiClient { * This typically indicates the node key has been invalidated (e.g., host was deleted). */ class UnauthorizedException(message: String) : Exception("HTTP 401: $message") + class NotFoundException(message: String) : Exception("HTTP 404: $message") + + /** + * Opens an HTTP connection bound to the active network when available. This ensures DNS resolution uses + * the active network's DNS servers, avoiding failures when Android reports connectivity before DNS is ready. + * Falls back to a default connection if no active network is available. + */ + internal fun openConnectionOnActiveNetwork(url: URL): HttpURLConnection { + if (useActiveNetworkBinding) { + val connectivityManager = appContext.getSystemService(ConnectivityManager::class.java) + val activeNetwork = connectivityManager?.activeNetwork + if (activeNetwork != null) { + return activeNetwork.openConnection(url) as HttpURLConnection + } + } + return url.openConnection() as HttpURLConnection + } + + // Disabled in tests where Network.openConnection is not available (Robolectric) + internal var useActiveNetworkBinding = true /** * Executes a request block with automatic re-enrollment on 401 Unauthorized. @@ -262,19 +286,12 @@ object ApiClient : CertificateApiClient { } override suspend fun getCertificateTemplate(certificateId: Int): Result = withReenrollOnUnauthorized { - val nodeKeyResult = getNodeKeyOrEnroll() - val orbitNodeKey = nodeKeyResult.getOrElse { error -> - return@withReenrollOnUnauthorized Result.failure(error) - } - val credentials = getEnrollmentCredentials() ?: return@withReenrollOnUnauthorized Result.failure(Exception("enroll credentials not set")) - makeRequest( + makeRequest( endpoint = "/api/fleetd/certificates/$certificateId", method = "GET", - body = GetCertificateTemplateRequest(orbitNodeKey = orbitNodeKey), - bodySerializer = GetCertificateTemplateRequest.serializer(), responseSerializer = GetCertificateTemplateResponseWrapper.serializer(), ).fold( onSuccess = { wrapper -> @@ -326,8 +343,14 @@ object ApiClient : CertificateApiClient { } }, onFailure = { throwable -> - FleetLog.e(TAG, "failed to update certificate status $certificateId: ${throwable.message}") - Result.failure(throwable) + if (throwable is NotFoundException) { + // Certificate template was deleted from the server -- nothing to report to + Log.i(TAG, "certificate template $certificateId no longer exists on server, nothing to report") + Result.success(Unit) + } else { + FleetLog.e(TAG, "failed to update certificate status $certificateId: ${throwable.message}") + Result.failure(throwable) + } }, ) } @@ -481,12 +504,6 @@ data class OrbitUpdateChannels( val desktop: String = "", ) -@Serializable -private data class GetCertificateTemplateRequest( - @SerialName("orbit_node_key") - val orbitNodeKey: String, -) - @Serializable data class UpdateCertificateStatusRequest( @SerialName("status") 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 50ec99eb1f..18e7c8de88 100644 --- a/android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt +++ b/android/app/src/main/java/com/fleetdm/agent/CertificateOrchestrator.kt @@ -150,16 +150,29 @@ class CertificateOrchestrator( storeCertificateState(context = context, certificateId = certificateId, certInstallInfo = newInfo) } - internal suspend fun markCertificateFailure(context: Context, certificateId: Int, alias: String): CertificateState { + internal suspend fun markCertificateForceFailed(context: Context, certificateId: Int, alias: String, uuid: String) { val existingInfo = getCertificateState(context = context, certificateId = certificateId) - ?: CertificateState(alias = alias, status = CertificateStatus.RETRY, retries = 0) + ?: CertificateState(alias = alias, status = CertificateStatus.FAILED, retries = 0, uuid = uuid) + + val newInfo = existingInfo.copy(alias = alias, status = CertificateStatus.FAILED, uuid = uuid) + storeCertificateState(context = context, certificateId = certificateId, certInstallInfo = newInfo) + } + + internal suspend fun recordEnrollmentAttemptFailure( + context: Context, + certificateId: Int, + alias: String, + uuid: String, + ): CertificateState { + val existingInfo = getCertificateState(context = context, certificateId = certificateId) + ?: CertificateState(alias = alias, status = CertificateStatus.RETRY, retries = 0, uuid = uuid) if (existingInfo.status != CertificateStatus.RETRY) { - Log.d(TAG, "markCertificateFailure: skipping cert $certificateId, status is ${existingInfo.status}") + Log.d(TAG, "recordEnrollmentAttemptFailure: skipping cert $certificateId, status is ${existingInfo.status}") return existingInfo } - var newInfo = existingInfo.copy(retries = existingInfo.retries + 1) + var newInfo = existingInfo.copy(retries = existingInfo.retries + 1, uuid = uuid) if (newInfo.retries >= MAX_CERT_INSTALL_RETRIES) { newInfo = newInfo.copy(status = CertificateStatus.FAILED) @@ -701,9 +714,15 @@ class CertificateOrchestrator( Log.d(TAG, "Successfully fetched certificate template: ${template.name}") + if (template.status == "failed") { + // Server says this certificate is permanently failed. Mark it locally as failed so we stop polling + Log.i(TAG, "Certificate template ${template.name} has terminal status \"failed\", marking locally as failed") + markCertificateForceFailed(context, certificateId, template.name, uuid) + return CertificateEnrollmentHandler.EnrollmentResult.PermanentlyFailed(template.name) + } + if (template.status != "delivered") { - // The certificate template hasn't failed on the device, but isn't ready to be processed yet. - // Retry next time we fetch but don't mark as failed locally + // Not ready to be processed yet (e.g. pending, delivering). Retry next time. Log.i(TAG, "Certificate template ${template.name} does not have status \"delivered\": status \"${template.status}\"") return CertificateEnrollmentHandler.EnrollmentResult.Success( alias = template.name, @@ -769,8 +788,20 @@ class CertificateOrchestrator( } } is CertificateEnrollmentHandler.EnrollmentResult.Failure -> { - val updatedInfo = markCertificateFailure(context = context, certificateId = certificateId, alias = template.name) - if (!updatedInfo.shouldRetry()) { + val shouldReportFailure = if (result.isRetryable) { + val updatedInfo = recordEnrollmentAttemptFailure( + context = context, + certificateId = certificateId, + alias = template.name, + uuid = uuid, + ) + !updatedInfo.shouldRetry() + } else { + markCertificateForceFailed(context, certificateId, template.name, uuid) + true + } + + if (shouldReportFailure) { FleetLog.e(TAG, "Certificate enrollment failed for ID $certificateId: ${result.reason}", result.exception) apiClient.updateCertificateStatus( certificateId = certificateId, 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 515360b7de..5af744e943 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 @@ -116,9 +116,14 @@ class ScepClientImpl : ScepClient { "Enrollment is pending - requires CA administrator approval", ) } + response.isFailure -> { + throw ScepEnrollmentException( + "Enrollment failed with SCEP failInfo: ${response.failInfo}", + ) + } else -> { throw ScepEnrollmentException( - "Enrollment failed - certificate not issued by SCEP server", + "Enrollment failed - unexpected SCEP response", ) } } diff --git a/android/app/src/test/java/com/fleetdm/agent/ApiClientReenrollTest.kt b/android/app/src/test/java/com/fleetdm/agent/ApiClientReenrollTest.kt index 38a0d61581..97fed43533 100644 --- a/android/app/src/test/java/com/fleetdm/agent/ApiClientReenrollTest.kt +++ b/android/app/src/test/java/com/fleetdm/agent/ApiClientReenrollTest.kt @@ -7,6 +7,7 @@ import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.stringPreferencesKey import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -42,6 +43,7 @@ class ApiClientReenrollTest { mockWebServer.start() ApiClient.initialize(context) + ApiClient.useActiveNetworkBinding = false clearDataStore() // Set up enrollment credentials pointing to mock server @@ -187,6 +189,14 @@ class ApiClientReenrollTest { "Expected second-node-key in Authorization header", retryRequest.getHeader("Authorization")?.contains("second-node-key") == true, ) + + // GET requests should not send Content-Type since they have no body. + // A Content-Type header on a bodyless GET can cause intermediaries (proxies, CDNs) + // to reject the request, which surfaces as misleading DNS errors on Android. + assertNull( + "GET request should not have Content-Type header", + retryRequest.getHeader("Content-Type"), + ) } @Test 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 05f81cc2c4..0d99116ac5 100644 --- a/android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt +++ b/android/app/src/test/java/com/fleetdm/agent/CertificateOrchestratorTest.kt @@ -1291,4 +1291,116 @@ class CertificateOrchestratorTest { assertEquals(CertificateStatus.INSTALLED, getStoredCertificates()[1]?.status) assertEquals(CertificateStatus.REMOVED_UNREPORTED, getStoredCertificates()[2]?.status) } + + @Test + fun `enrollCertificate marks locally failed when server template status is failed`() = runTest { + val certificateId = 42 + val uuid = "test-uuid" + + // Arrange: configure API to return a template with status "failed" + fakeApiClient.getCertificateTemplateHandler = { certId -> + Result.success( + CertificateTemplateResult( + template = TestCertificateTemplateFactory.create( + id = certId, + name = "cert-failed", + status = "failed", + ), + scepUrl = TestCertificateTemplateFactory.DEFAULT_SCEP_URL, + ), + ) + } + + // Act + val result = orchestrator.enrollCertificate(context, certificateId, uuid, mockInstaller) + + // Assert: returns PermanentlyFailed + assertTrue( + "Expected PermanentlyFailed but got: $result", + result is CertificateEnrollmentHandler.EnrollmentResult.PermanentlyFailed, + ) + + // Assert: local state is FAILED so future runs skip this certificate + val stored = getStoredCertificates() + assertEquals(CertificateStatus.FAILED, stored[certificateId]?.status) + assertEquals(uuid, stored[certificateId]?.uuid) + + // Assert: no SCEP enrollment was attempted + assertNull("SCEP client should not have been called", mockScepClient.capturedConfig) + + // Assert: no status update sent to server (server already knows it failed) + assertTrue("No status update should be sent", fakeApiClient.updateStatusCalls.isEmpty()) + } + + @Test + fun `non-retryable SCEP failure immediately marks FAILED and reports to server`() = runTest { + val certificateId = 50 + val uuid = "test-uuid" + + // Arrange: SCEP enrollment will throw ScepEnrollmentException (non-retryable) + mockScepClient.shouldThrowEnrollmentException = true + fakeApiClient.getCertificateTemplateHandler = { certId -> + Result.success( + CertificateTemplateResult( + template = TestCertificateTemplateFactory.create(id = certId, name = "cert-nonretry", status = "delivered"), + scepUrl = TestCertificateTemplateFactory.DEFAULT_SCEP_URL, + ), + ) + } + + // Act + val result = orchestrator.enrollCertificate(context, certificateId, uuid, mockInstaller) + + // Assert: returns Failure (non-retryable) + assertTrue("Expected Failure but got: $result", result is CertificateEnrollmentHandler.EnrollmentResult.Failure) + assertFalse((result as CertificateEnrollmentHandler.EnrollmentResult.Failure).isRetryable) + + // Assert: immediately marked FAILED locally (no retry attempts) + val stored = getStoredCertificates() + assertEquals(CertificateStatus.FAILED, stored[certificateId]?.status) + assertEquals(uuid, stored[certificateId]?.uuid) + assertEquals(0, stored[certificateId]?.retries) + + // Assert: reported FAILED to server exactly once + assertEquals(1, fakeApiClient.updateStatusCalls.size) + assertEquals(UpdateCertificateStatusStatus.FAILED, fakeApiClient.updateStatusCalls[0].status) + } + + @Test + fun `retryable SCEP failure increments retries and does not report until max retries`() = runTest { + val certificateId = 51 + val uuid = "test-uuid" + + // Arrange: SCEP enrollment will throw ScepNetworkException (retryable) + mockScepClient.shouldThrowNetworkException = true + fakeApiClient.getCertificateTemplateHandler = { certId -> + Result.success( + CertificateTemplateResult( + template = TestCertificateTemplateFactory.create(id = certId, name = "cert-retry", status = "delivered"), + scepUrl = TestCertificateTemplateFactory.DEFAULT_SCEP_URL, + ), + ) + } + + // Act: first attempt + orchestrator.enrollCertificate(context, certificateId, uuid, mockInstaller) + + // Assert: status is RETRY, not FAILED + var stored = getStoredCertificates() + assertEquals(CertificateStatus.RETRY, stored[certificateId]?.status) + assertEquals(1, stored[certificateId]?.retries) + assertEquals(uuid, stored[certificateId]?.uuid) + assertTrue("Should not report FAILED yet", fakeApiClient.updateStatusCalls.isEmpty()) + + // Act: exhaust remaining retries + repeat(MAX_CERT_INSTALL_RETRIES - 1) { + orchestrator.enrollCertificate(context, certificateId, uuid, mockInstaller) + } + + // Assert: now FAILED and reported to server exactly once + stored = getStoredCertificates() + assertEquals(CertificateStatus.FAILED, stored[certificateId]?.status) + assertEquals(1, fakeApiClient.updateStatusCalls.size) + assertEquals(UpdateCertificateStatusStatus.FAILED, fakeApiClient.updateStatusCalls[0].status) + } } diff --git a/android/changes/.keep b/android/changes/.keep new file mode 100644 index 0000000000..e69de29bb2 diff --git a/android/changes/42624-fix-certificate-template-get-content-type b/android/changes/42624-fix-certificate-template-get-content-type new file mode 100644 index 0000000000..0525dc47cd --- /dev/null +++ b/android/changes/42624-fix-certificate-template-get-content-type @@ -0,0 +1,6 @@ +- Fixed background DNS resolution failures. +- Stop polling certificates when the server reports them as permanently failed. +- Non-retryable SCEP failures (e.g. server rejection) now immediately mark the certificate as failed instead of retrying 3 times. +- Fixed duplicate FAILED status reports. +- Treat HTTP 404 on certificate status updates as success when the template has been deleted server-side. +- Include SCEP `failInfo` details in enrollment failure messages instead of a generic error.