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

Added new server trust policy: Revocation #1822

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@WataruSuzuki
Contributor

WataruSuzuki commented Dec 4, 2016

Hello.
This request is added new policy at server trust evaluation that checking revoked server certificate.

@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Dec 9, 2016

Member

Hi @WataruSuzuki,

Could you provide a detailed writeup of this set of changes, the problems you were trying to solve and how the tests verify the behavior? The more detail the better. This will not only be for our own knowledge, but for those in the future to refer back to this ticket.

Thanks! 🍻

Member

cnoon commented Dec 9, 2016

Hi @WataruSuzuki,

Could you provide a detailed writeup of this set of changes, the problems you were trying to solve and how the tests verify the behavior? The more detail the better. This will not only be for our own knowledge, but for those in the future to refer back to this ticket.

Thanks! 🍻

@WataruSuzuki

This comment has been minimized.

Show comment
Hide comment
@WataruSuzuki

WataruSuzuki Dec 13, 2016

Contributor

Hi @cnoon

Here are information of this changes.

[1. About this change]
This change gives to new option about checking revoked server certificates.
iOS APIs(NSURLSession, WKWebView, etc) don't check revocation on default setting.
So people needs to create policy that specifically checks for certificate revocation (for example, via OCSP or a CRL).
https://developer.apple.com/library/content/technotes/tn2232/_index.html#//apple_ref/doc/uid/DTS40012884-CH1-SECSTRICTER

Alamofire also enable trust policy revocation if user wanna check server certificates is revoked.
But current trust policies of Alamofire provides pinning, not provide checking revocation.
Create manually about checking revocation at "customEvaluation" if I wanna check to revocation.
So I want to more option about more easy to enabling checking revocation.

[2. What solves this change]
This change makes more easier to checking revocation.

[3. How to test this change]
I created this sample app to testing this change.
https://github.com/WataruSuzuki/RevocationAlamofire

And also I added to unit test. It checks revoked server: https://revoked.badssl.com.

Thank you!

Contributor

WataruSuzuki commented Dec 13, 2016

Hi @cnoon

Here are information of this changes.

[1. About this change]
This change gives to new option about checking revoked server certificates.
iOS APIs(NSURLSession, WKWebView, etc) don't check revocation on default setting.
So people needs to create policy that specifically checks for certificate revocation (for example, via OCSP or a CRL).
https://developer.apple.com/library/content/technotes/tn2232/_index.html#//apple_ref/doc/uid/DTS40012884-CH1-SECSTRICTER

Alamofire also enable trust policy revocation if user wanna check server certificates is revoked.
But current trust policies of Alamofire provides pinning, not provide checking revocation.
Create manually about checking revocation at "customEvaluation" if I wanna check to revocation.
So I want to more option about more easy to enabling checking revocation.

[2. What solves this change]
This change makes more easier to checking revocation.

[3. How to test this change]
I created this sample app to testing this change.
https://github.com/WataruSuzuki/RevocationAlamofire

And also I added to unit test. It checks revoked server: https://revoked.badssl.com.

Thank you!

@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Jan 16, 2017

Member

Hi @WataruSuzuki,

First off let me apologize for taking so long to go through this. Your patience is appreciated. I can also say that I'm very glad I waited until I had enough time to go through this throughly. The tests took me forever to understand exactly what's happening across all the versions of all platforms.

With that said, thank you so much for your efforts here on this PR. It's greatly appreciated! I've picked this PR apart into a bunch of different commits to test all the functionality and for my own understanding. Here's a breakdown of what I've added and modified in this PR.

  • 22fec47 - Added TLS evaluation tests for revoked certs for no policy and default policy.
    • Contributed by me
  • 52fc15e - Added revoked evaluation server trust policy to check for revoked certificates.
    • Contributed by you (refactored a bit by me)
  • e8282c9 - Added server trust policy tests for revoked evaluation with varied host validation.
    • Contributed by you (refactored revocation flag by me)
  • 74f6473 - Added TLS evaluation tests for revoked evaluation against revoked certificate.
    • Contributed by you (tweaked by me slightly with revocation flag)
  • 2fd4997 - Updated TLS tests for tvOS 10.1 and added expiration test for revoked evaluation.
    • Contributed by me

What I was amazed to find is that it appears Apple has started enabling automatic revocation testing in the latest platforms (iOS 10.1+), but the test suite is so finicky that I had to disable the tests. Thankfully on all platforms, enabling revocation testing directly does result in consistent behavior.

Through most of my testing, I think you really want to weigh the tradeoffs of using kSecRevocationUseAnyAvailableMethod and kSecRevocationRequirePositiveResponse depending on what type of certificates you have deployed. If you have certs signed by a valid certificate authority, I think I'd recommend using the more secure kSecRevocationRequirePositiveResponse. You'll see in the ServerTrustPolicy tests that I switched over to the kSecRevocationUseAnyAvailableMethod flag since all the certs being tested were self signed which will always fail when trying to hit the server since they're not deployed.

Sadly the documentation here is terrible and isn't very helpful. Take what I said above with a grain of salt. That's my best guess interpretation of all these tests and the limited documentation I could find.

Overall, thank you so much for bringing the revocation testing to our attention and going the extra step to implement it. I think this is a great addition to the Alamofire core library and we really appreciate you taking the time to put this all together. Great work!

Just FYI...these changes will ship as part of Alamofire 4.3.0 here shortly.

Cheers. 🍻

Member

cnoon commented Jan 16, 2017

Hi @WataruSuzuki,

First off let me apologize for taking so long to go through this. Your patience is appreciated. I can also say that I'm very glad I waited until I had enough time to go through this throughly. The tests took me forever to understand exactly what's happening across all the versions of all platforms.

With that said, thank you so much for your efforts here on this PR. It's greatly appreciated! I've picked this PR apart into a bunch of different commits to test all the functionality and for my own understanding. Here's a breakdown of what I've added and modified in this PR.

  • 22fec47 - Added TLS evaluation tests for revoked certs for no policy and default policy.
    • Contributed by me
  • 52fc15e - Added revoked evaluation server trust policy to check for revoked certificates.
    • Contributed by you (refactored a bit by me)
  • e8282c9 - Added server trust policy tests for revoked evaluation with varied host validation.
    • Contributed by you (refactored revocation flag by me)
  • 74f6473 - Added TLS evaluation tests for revoked evaluation against revoked certificate.
    • Contributed by you (tweaked by me slightly with revocation flag)
  • 2fd4997 - Updated TLS tests for tvOS 10.1 and added expiration test for revoked evaluation.
    • Contributed by me

What I was amazed to find is that it appears Apple has started enabling automatic revocation testing in the latest platforms (iOS 10.1+), but the test suite is so finicky that I had to disable the tests. Thankfully on all platforms, enabling revocation testing directly does result in consistent behavior.

Through most of my testing, I think you really want to weigh the tradeoffs of using kSecRevocationUseAnyAvailableMethod and kSecRevocationRequirePositiveResponse depending on what type of certificates you have deployed. If you have certs signed by a valid certificate authority, I think I'd recommend using the more secure kSecRevocationRequirePositiveResponse. You'll see in the ServerTrustPolicy tests that I switched over to the kSecRevocationUseAnyAvailableMethod flag since all the certs being tested were self signed which will always fail when trying to hit the server since they're not deployed.

Sadly the documentation here is terrible and isn't very helpful. Take what I said above with a grain of salt. That's my best guess interpretation of all these tests and the limited documentation I could find.

Overall, thank you so much for bringing the revocation testing to our attention and going the extra step to implement it. I think this is a great addition to the Alamofire core library and we really appreciate you taking the time to put this all together. Great work!

Just FYI...these changes will ship as part of Alamofire 4.3.0 here shortly.

Cheers. 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment