-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: 3.1.0-eap
Are you sure you want to change the base?
Conversation
286dd71
to
cc70f73
Compare
cc70f73
to
13901e5
Compare
@@ -436,3 +430,5 @@ public data class CertificatePinner( | |||
) | |||
} | |||
} | |||
|
|||
public class CertificatePinnerException(message: String) : IOException(message) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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! 🙏
@@ -436,3 +430,5 @@ public data class CertificatePinner( | |||
) | |||
} | |||
} | |||
|
|||
public class CertificatePinnerException(message: String) : IOException(message) |
There was a problem hiding this comment.
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
@@ -436,3 +430,5 @@ public data class CertificatePinner( | |||
) | |||
} | |||
} | |||
|
|||
public class CertificatePinnerException(message: String) : IOException(message) |
There was a problem hiding this comment.
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! 🙏
Subsystem
Client, Darwin
Motivation
KTOR-6759 Darwin: Ambiguous DarwinHttpRequestException for SSL Pinning failure
The problem is that we call
completionHandler
withNSURLSessionAuthChallengePerformDefaultHandling
on SSL pinning failure and then third-party API throws an exception. There are no mechanisms to attach additional data to this exception (or at least I don't know any).The place where the actual error occurs and the place where
DarwinHttpRequestException
is constructed are two completely different places.Solution
Save actual failure thrown during challenge handling to a
DarwinTaskHandler
and throw it later instead of creating a generic one. Throw specific exceptionCertificatePinner
to make it possible to catch pinning failures specifically:Naming, comments, and other stylistic things aren’t final yet, but I need this draft to be reviewed.