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

SecTrust object now is conditionally unwrapped #349

Merged
merged 1 commit into from
Jun 29, 2017

Conversation

giullo
Copy link

@giullo giullo commented Jun 27, 2017

This PR fixes an issue introduced by dbeb119#diff-b9ab27dc605d36ce558013387d2ba2d5

Now the SecTrust object is conditionally unwrapped and if not found , the pinning will fail*. I was never able to reproduce the scenario where the SecTrust object is null within a wss/https connection, i think this could be related to a failed SSL handshake (maybe due to networking issues) but these are just speculations at this point.

  • I consider the pinning failed, because if you end up in that code branch it's because:
  1. the url is either wss or https
  2. a security object was provided to perform the certificate pinning
    the client is clearly signalling is will for the pinning to be performed, so in absence of a trust that contains the certificates to be checked, i think it's better to reject the connection

@daltoniam
Copy link
Owner

Great work and thanks for fixing that issue!

@daltoniam daltoniam merged commit 08edd54 into daltoniam:master Jun 29, 2017
@giullo
Copy link
Author

giullo commented Jul 3, 2017

Hey thanks for the quick approval. i see that this is included in 2.1.0, but somehow the spec hasn't been pushed to cocoa pods , because the latest available is still 2.0.4 (and if you try to target directly 2.1.0 you will get an error that the dependency can't be satisfied)

Could you have a look? ah and btw, many thanks for the great library :)

@daltoniam
Copy link
Owner

Finally got it done. Had a fair amount of trouble with CocoaPods & SPM working together. Anyway, 2.1.0 has been released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants