Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KTOR-6759 Darwin: Throw specific exception for SSL Pinning failure #4408

Draft
wants to merge 2 commits into
base: 3.1.0-eap
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
// - Show declarations: true

// Library unique name: <io.ktor:ktor-client-darwin>
final class io.ktor.client.engine.darwin.certificates/CertificatePinnerException : kotlinx.io/IOException { // io.ktor.client.engine.darwin.certificates/CertificatePinnerException|null[0]
constructor <init>(kotlin/String) // io.ktor.client.engine.darwin.certificates/CertificatePinnerException.<init>|<init>(kotlin.String){}[0]
}

final class io.ktor.client.engine.darwin.certificates/PinnedCertificate { // io.ktor.client.engine.darwin.certificates/PinnedCertificate|null[0]
constructor <init>(kotlin/String, kotlin/String, kotlin/String) // io.ktor.client.engine.darwin.certificates/PinnedCertificate.<init>|<init>(kotlin.String;kotlin.String;kotlin.String){}[0]

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014-2019 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
* Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

package io.ktor.client.engine.darwin
Expand All @@ -8,7 +8,6 @@ import io.ktor.client.call.*
import io.ktor.client.engine.*
import io.ktor.http.content.*
import io.ktor.utils.io.*
import io.ktor.utils.io.errors.*
import kotlinx.cinterop.*
import kotlinx.coroutines.*
import kotlinx.io.IOException
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014-2022 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
* Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

package io.ktor.client.engine.darwin
Expand Down Expand Up @@ -131,11 +131,16 @@ public class KtorNSURLSessionDelegate(
didReceiveChallenge: NSURLAuthenticationChallenge,
completionHandler: (NSURLSessionAuthChallengeDisposition, NSURLCredential?) -> Unit
) {
val handler = challengeHandler
if (handler != null) {
handler(session, task, didReceiveChallenge, completionHandler)
} else {
val handler = challengeHandler ?: run {
completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, didReceiveChallenge.proposedCredential)
return
}

try {
handler(session, task, didReceiveChallenge, completionHandler)
} catch (cause: Throwable) {
taskHandlers[task]?.saveFailure(cause)
completionHandler(NSURLSessionAuthChallengeCancelAuthenticationChallenge, null)
}
}
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
/*
* Copyright 2014-2021 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/
* Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

package io.ktor.client.engine.darwin.certificates

import io.ktor.client.engine.darwin.*
import io.ktor.util.logging.*
import kotlinx.cinterop.*
import kotlinx.io.*
import platform.CoreCrypto.*
import platform.CoreFoundation.*
import platform.Foundation.*
import platform.Security.*

private val LOG = KtorSimpleLogger("io.ktor.client.engine.darwin.certificates.CertificatePinner")

/**
* Constrains which certificates are trusted. Pinning certificates defends against attacks on
* certificate authorities. It also prevents connections through man-in-the-middle certificate
Expand Down Expand Up @@ -112,36 +116,36 @@ public data class CertificatePinner(
private val validateTrust: Boolean
) : ChallengeHandler {

@OptIn(ExperimentalForeignApi::class)
override fun invoke(
session: NSURLSession,
task: NSURLSessionTask,
challenge: NSURLAuthenticationChallenge,
completionHandler: (NSURLSessionAuthChallengeDisposition, NSURLCredential?) -> Unit
) {
if (applyPinning(challenge)) {
completionHandler(NSURLSessionAuthChallengeUseCredential, challenge.proposedCredential)
} else {
completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, null)
}
}

@OptIn(ExperimentalForeignApi::class)
private fun applyPinning(challenge: NSURLAuthenticationChallenge): Boolean {
val hostname = challenge.protectionSpace.host
val matchingPins = findMatchingPins(hostname)

if (matchingPins.isEmpty()) {
println("CertificatePinner: No pins found for host")
completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, null)
return
LOG.trace { "No pins found for host" }
return false
}

if (challenge.protectionSpace.authenticationMethod !=
NSURLAuthenticationMethodServerTrust
) {
println("CertificatePinner: Authentication method not suitable for pinning")
completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, null)
return
if (challenge.protectionSpace.authenticationMethod != NSURLAuthenticationMethodServerTrust) {
LOG.trace { "Authentication method not suitable for pinning" }
return false
}

val trust = challenge.protectionSpace.serverTrust
if (trust == null) {
println("CertificatePinner: Server trust is not available")
completionHandler(NSURLSessionAuthChallengeCancelAuthenticationChallenge, null)
return
}
?: throw CertificatePinnerException("Server trust is not available")

if (validateTrust) {
val hostCFString = CFStringCreateWithCString(null, hostname, kCFStringEncodingUTF8)
Expand All @@ -150,11 +154,7 @@ public data class CertificatePinner(
SecTrustSetPolicies(trust, policy)
}
}
if (!trust.trustIsValid()) {
println("CertificatePinner: Server trust is invalid")
completionHandler(NSURLSessionAuthChallengeCancelAuthenticationChallenge, null)
return
}
if (!trust.trustIsValid()) throw CertificatePinnerException("Server trust is invalid")
}

val certCount = SecTrustGetCertificateCount(trust)
Expand All @@ -163,19 +163,14 @@ public data class CertificatePinner(
}

if (certificates.size != certCount.toInt()) {
println("CertificatePinner: Unknown certificates")
completionHandler(NSURLSessionAuthChallengeCancelAuthenticationChallenge, null)
return
throw CertificatePinnerException("Unknown certificates")
}

val result = hasOnePinnedCertificate(certificates)
if (result) {
completionHandler(NSURLSessionAuthChallengeUseCredential, challenge.proposedCredential)
} else {
if (!hasOnePinnedCertificate(certificates)) {
val message = buildErrorMessage(certificates, hostname)
println(message)
completionHandler(NSURLSessionAuthChallengeCancelAuthenticationChallenge, null)
throw CertificatePinnerException(message)
}
return true
}

/**
Expand All @@ -199,17 +194,16 @@ public data class CertificatePinner(

pin.hash == sha256
}

CertificatesInfo.HASH_ALGORITHM_SHA_1 -> {
if (sha1 == null) {
sha1 = publicKey.toSha1String()
}

pin.hash == sha1
}
else -> {
println("CertificatePinner: Unsupported hashAlgorithm: ${pin.hashAlgorithm}")
false
}

else -> throw IllegalArgumentException("Unsupported hashAlgorithm: ${pin.hashAlgorithm}")
}
}
}
Expand Down Expand Up @@ -436,3 +430,5 @@ public data class CertificatePinner(
)
}
}

public class CertificatePinnerException(message: String) : IOException(message)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have concerns regarding this exception:

  • Previously DarwinHttpRequestException was thrown for such cases, so it is potentially breaking change
  • This exception is used for any error thrown by the pinner. Should we give a possibility to distinct between these errors?
  • Should we provide a multiplatform exception to be able to handle SSL pinning problems in a unified manner?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

  • I would still consider this as a bug fix
  • We can make this exception open, and wait if there are use cases for that.
  • We could introduce this exception in ktor-network common code.

Let's target to 3.1.0 and do it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osipxd Thanks for this PR! I have very little context on the Ktor code, so I don't have much feedback to share. I can see that the exception contains the output message which is exactly what I was hoping to be able to access. I also appreciate how you included some tests. Thanks for your help and @e5l thank you for your review as well! 🙏

Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
/*
* Copyright 2014-2022 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
* Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

package io.ktor.client.engine.darwin.internal

import io.ktor.client.engine.darwin.*
import io.ktor.client.plugins.sse.*
import io.ktor.client.request.*
import io.ktor.http.*
import io.ktor.util.*
import io.ktor.util.date.*
import io.ktor.utils.io.*
import io.ktor.utils.io.CancellationException
Expand All @@ -28,6 +26,9 @@ internal class DarwinTaskHandler(
private val requestTime: GMTDate = GMTDate()
private val bodyChunks = Channel<ByteArray>(Channel.UNLIMITED)

private var pendingFailure: Throwable? = null
get() = field?.also { field = null }

private val body: ByteReadChannel = GlobalScope.writer(callContext) {
try {
bodyChunks.consumeEach {
Expand All @@ -54,9 +55,13 @@ internal class DarwinTaskHandler(
}
}

fun saveFailure(cause: Throwable) {
pendingFailure = cause
}

fun complete(task: NSURLSessionTask, didCompleteWithError: NSError?) {
if (didCompleteWithError != null) {
val exception = handleNSError(requestData, didCompleteWithError)
val exception = pendingFailure ?: handleNSError(requestData, didCompleteWithError)
bodyChunks.close(exception)
response.completeExceptionally(exception)
return
Expand Down
14 changes: 14 additions & 0 deletions ktor-client/ktor-client-darwin/darwin/test/DarwinEngineTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,20 @@ class DarwinEngineTest {
}
}

@Test
fun testRethrowExceptionThrownDuringCustomChallenge() = runBlocking {
val challengeException = Exception("Challenge failed")

val client = HttpClient(Darwin) {
engine {
handleChallenge { _, _, _, _ -> throw challengeException }
}
}

val thrownException = assertFails { client.get(TEST_SERVER_TLS) }
assertTrue(thrownException === challengeException, "Expected exception to be rethrown")
}

private fun stringToNSUrlString(value: String): String {
return Url(value).toNSUrl().absoluteString!!
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014-2019 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
* Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

@file:Suppress("NO_EXPLICIT_RETURN_TYPE_IN_API_MODE_WARNING", "KDocMissingDocumentation")
Expand All @@ -18,6 +18,11 @@ import kotlin.time.Duration.Companion.milliseconds
*/
const val TEST_SERVER: String = "http://127.0.0.1:8080"

/**
* Web url with TLS for tests.
*/
const val TEST_SERVER_TLS: String = "https://127.0.0.1:8089"

/**
* Websocket server url for tests.
*/
Expand Down