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.

Sign up for GitHub

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

Jump to bottom

Add SSL Pinning support #110

Merged
merged 19 commits into from
Apr 18, 2016
Merged

Add SSL Pinning support #110

merged 19 commits into from
Apr 18, 2016

Conversation

colinmcardell
Copy link
Contributor

For reference: #109

@8W9aG
Copy link
Contributor

8W9aG commented Apr 8, 2016

👍 Looks really great, needs a couple of unit tests though. One thing I have been thinking about is restricting the certificate lookups to using NSBundle pathForResource, it might be advantageous to only allow certificates from a read-only directory on the device (however a minor point)

8W9aG
8W9aG reviewed Apr 8, 2016
View reviewed changes
SPTDataLoader/SPTDataLoaderServerTrustPolicy.m

@interface SPTDataLoaderServerTrustPolicy ()

@property (strong, nonatomic) NSDictionary<NSString *, NSArray<NSData *> *> *trustedHostsAndCertificates;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to switch around nonatomic/strong so it conforms to the style of the rest of the repository

@colinmcardell
Copy link
Contributor Author

Thanks for the comments @8W9aG. I will take a pass this weekend cleaning up and writing test coverage.

@colinmcardell
Basic documentation of the -validateChallenge: method for `SPTDataL…
3f62b34 
…oaderServerTrustPolicy`
@colinmcardell
Matching Spotify code style guide.
eed6d0f
@colinmcardell
Exposing server trust policy's internal store of hosts and certificat…
810bcba 
…es in a private category to allow for better testability.
@colinmcardell
Exposing additional internals of SPTDataLoaderServerTrustPolicy in …
8753bb8 
…the private category to allow better test coverage.
@colinmcardell
WIP Adds tests for SPTDataLoaderServerTrustPolicy
8507059
@colinmcardell
WIP Additional tests for SPTDataLoaderServerTrustPolicy
d672612 
- Includes mocks for `NSURLAuthenticationChallenge` & `NSURLProtectionSpace`
- Adds tests for public interface, validation of `NSURLAuthenticationChallenge`
@colinmcardell
Integration tests for SPTDataLoaderServerTrustPolicy with `SPTDataL…
b1ecd9f 
…oaderService`
@colinmcardell
Resolves compiler warnings as errors. -Weverything
1e1f4f5
@colinmcardell
Adds -Wno-partial-availability to test target. Resolves issue when …
b400a1f 
…attempting to run tests due to import of `XCTest`. `XCTest` introduces new stuff in iOS 9.0 which triggers the partial availability warning.
@colinmcardell
Adds Security.framework to library and test target.
27d7622
@colinmcardell
Adds SPTDataLoaderServerTrustPolicy to framework targets within pro…
bf50f5e 
…ject.
@colinmcardell
Linking Security.framework within demo and frameworks projects. Add…
eb30652 
…s `Security` framework within podspec.
@colinmcardell
Removing -Wno-partial-availability from test target `WARNING_CFLAGS…
b25861f 
…`. This seems to make Travis cranky.
@coveralls
Copy link

Coverage Status

Coverage decreased (-9.1%) to 89.415% when pulling b25861f on colinmcardell:feature/ssl_pinning into 71f216d on spotify:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-9.1%) to 89.415% when pulling b25861f on colinmcardell:feature/ssl_pinning into 71f216d on spotify:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-9.1%) to 89.415% when pulling b25861f on colinmcardell:feature/ssl_pinning into 71f216d on spotify:master.

@colinmcardell
Adds SPTDataLoaderServerTrustPolicyTest to test target.
72f9762
@colinmcardell
Adding test certificates back into project. Modifying tests for `SPTD…
dd3113e 
…ataLoaderServerTrustPolicy` to more gracefully fail when certificates / trusts are not properly loaded or built up.
@colinmcardell
Resolves type comparison & null to a callee that requires a non-null …
a42c2df 
…argument issues within `SPTDataLoaderServerTrustPolicyTest.m`
@colinmcardell
Doh, reversing commit turning off inhibit all warnings of test target…
c2b39e7 
…. My bad.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.15% when pulling dd3113e on colinmcardell:feature/ssl_pinning into 71f216d on spotify:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.15% when pulling c2b39e7 on colinmcardell:feature/ssl_pinning into 71f216d on spotify:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.15% when pulling c2b39e7 on colinmcardell:feature/ssl_pinning into 71f216d on spotify:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.15% when pulling c2b39e7 on colinmcardell:feature/ssl_pinning into 71f216d on spotify:master.

@8W9aG
Copy link
Contributor

8W9aG commented Apr 17, 2016

👍 This is looking great, i think the only thing it needs is a rebase and then we can merge it

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 98.15% when pulling 32408ce on colinmcardell:feature/ssl_pinning into d1f27fa on spotify:master.

@colinmcardell
Copy link
Contributor Author

@8W9aG Awesome!
Did you want me to squash and force push down to a single commit??

@8W9aG
Copy link
Contributor

8W9aG commented Apr 17, 2016

Nah I think we should leave the history as is, if you are fine with this PR I'll merge it

@8W9aG 8W9aG merged commit 7c5342e into spotify:master Apr 18, 2016
@colinmcardell
Copy link
Contributor Author

I'm into it. Merge away! Thanks!

@rastersize rastersize mentioned this pull request Apr 18, 2016
Closed
@JensAyton JensAyton modified the milestone: beme Aug 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@colinmcardell @8W9aG @coveralls @rastersize @JensAyton