Dynamically Supporting Invalid SSL Certificates #873

Merged
merged 12 commits into from Apr 17, 2013

Conversation

Projects
None yet
6 participants
@kcharwood
Contributor

kcharwood commented Mar 25, 2013

Pull request for #764

@kcharwood

This comment has been minimized.

Show comment Hide comment
@kcharwood

kcharwood Mar 25, 2013

Contributor

Updated the Pull Request to reference the new AFN 1.2.0 changes.

Only question I have is do you want the property to be named the same on both the AFHTTPClient and the AFURLConnectionOperation?

Contributor

kcharwood commented Mar 25, 2013

Updated the Pull Request to reference the new AFN 1.2.0 changes.

Only question I have is do you want the property to be named the same on both the AFHTTPClient and the AFURLConnectionOperation?

@rromanchuk

This comment has been minimized.

Show comment Hide comment
@rromanchuk

rromanchuk Mar 26, 2013

👍

👍

@rromanchuk

This comment has been minimized.

Show comment Hide comment
@rromanchuk

rromanchuk Mar 26, 2013

@kcharwood do you know of a somewhat clean way to inject the pod .pch file? Fork my own and redifine the podspec perhaps?

#ifdef DEBUG
#define _AFNETWORKING_ALLOW_INVALID_SSL_CERTIFICATES_
#endif

@kcharwood do you know of a somewhat clean way to inject the pod .pch file? Fork my own and redifine the podspec perhaps?

#ifdef DEBUG
#define _AFNETWORKING_ALLOW_INVALID_SSL_CERTIFICATES_
#endif
@rromanchuk

This comment has been minimized.

Show comment Hide comment
@rromanchuk

rromanchuk Mar 27, 2013

anyway you can merge this into your master, would like to use your fork until this gets merged into afnetworking

anyway you can merge this into your master, would like to use your fork until this gets merged into afnetworking

This comment has been minimized.

Show comment Hide comment
@kcharwood

kcharwood Mar 27, 2013

Owner

Hey @rromanchuk,

You can always just use my invalid-ssl branch on my fork. I would prefer to keep it in that branch until Matt accepts it.

Owner

kcharwood replied Mar 27, 2013

Hey @rromanchuk,

You can always just use my invalid-ssl branch on my fork. I would prefer to keep it in that branch until Matt accepts it.

This comment has been minimized.

Show comment Hide comment
@rromanchuk

rromanchuk Mar 27, 2013

this is a stupid question, but how can i specify a branch in my podfile?

this is a stupid question, but how can i specify a branch in my podfile?

This comment has been minimized.

Show comment Hide comment
@kcharwood

kcharwood Mar 27, 2013

Owner

I know you can create a custom podspec that points to a specific git repo and specific commit:

:git => 'https://github.com/kcharwood/AFNetworking.git', :commit => '8e5dac8d42638e8930054cbfd97f5d6f7a14f01b'
Owner

kcharwood replied Mar 27, 2013

I know you can create a custom podspec that points to a specific git repo and specific commit:

:git => 'https://github.com/kcharwood/AFNetworking.git', :commit => '8e5dac8d42638e8930054cbfd97f5d6f7a14f01b'

This comment has been minimized.

Show comment Hide comment
@rromanchuk

rromanchuk Mar 27, 2013

@kcharwood oh right, thx for the fix.

@kcharwood oh right, thx for the fix.

@kcharwood

This comment has been minimized.

Show comment Hide comment
@kcharwood

kcharwood Apr 7, 2013

Contributor

Updated the pull request with the latest code that has been push to master to resolve conflicts

Contributor

kcharwood commented Apr 7, 2013

Updated the pull request with the latest code that has been push to master to resolve conflicts

@kcharwood

This comment has been minimized.

Show comment Hide comment
@kcharwood

kcharwood Apr 8, 2013

Contributor

Continuing to try and keep this branch up to date and conflict free...

Contributor

kcharwood commented Apr 8, 2013

Continuing to try and keep this branch up to date and conflict free...

@kcharwood

This comment has been minimized.

Show comment Hide comment
@kcharwood

kcharwood Apr 9, 2013

Contributor

Another conflict merge.

Contributor

kcharwood commented Apr 9, 2013

Another conflict merge.

@carlosnavas

This comment has been minimized.

Show comment Hide comment
@carlosnavas

carlosnavas Apr 16, 2013

Hi @kcharwood & @rromanchuk:

I posted some time ago a question that is related with this pull request: http://stackoverflow.com/questions/15202652/set-unset-afnetworking-allow-invalid-ssl-certificates-programmatically-at-run .
What is the best solution to achieve this? I really appreciate any feedback. Thanks

Hi @kcharwood & @rromanchuk:

I posted some time ago a question that is related with this pull request: http://stackoverflow.com/questions/15202652/set-unset-afnetworking-allow-invalid-ssl-certificates-programmatically-at-run .
What is the best solution to achieve this? I really appreciate any feedback. Thanks

@kcharwood

This comment has been minimized.

Show comment Hide comment
@kcharwood

kcharwood Apr 16, 2013

Contributor

@carlosnavas This pull request solves that problem. I'm not sure what plans @mattt has for pulling this in... Hopefully soon :)

Contributor

kcharwood commented Apr 16, 2013

@carlosnavas This pull request solves that problem. I'm not sure what plans @mattt has for pulling this in... Hopefully soon :)

@carlosnavas

This comment has been minimized.

Show comment Hide comment
@carlosnavas

carlosnavas Apr 16, 2013

It would be great because I've seen several posts from people having the same concerning about this and nobody has posted a nice solution without changing the AFNetworking classes code.

It would be great because I've seen several posts from people having the same concerning about this and nobody has posted a nice solution without changing the AFNetworking classes code.

@mattt mattt merged commit 22d6747 into AFNetworking:master Apr 17, 2013

@mattt

This comment has been minimized.

Show comment Hide comment
@mattt

mattt Apr 17, 2013

Contributor

Just merged this in. Thanks for all of your great work on this, @kcharwood. This is a significant improvement over the compiler macro. Looking forward to rolling this into a new release later today.

Contributor

mattt commented Apr 17, 2013

Just merged this in. Thanks for all of your great work on this, @kcharwood. This is a significant improvement over the compiler macro. Looking forward to rolling this into a new release later today.

@kcharwood kcharwood deleted the kcharwood:invalid-ssl branch Apr 17, 2013

@kcharwood

This comment has been minimized.

Show comment Hide comment
@kcharwood

kcharwood Apr 17, 2013

Contributor

Thanks @mattt for getting this in. This definitely helps us address issues with Cocoapods. Looking forward to the release.

One thing I did want to bring up that may require some further investigation:

Is there any scenario where we should support SSL pinning AND an invalid certificate? Currently, the invalid SSL cert logic path assumes the pinning mode is set to none, which solves all of the use cases I've ever run into. Not sure its something we should worry about in the future, but at least wanted to ask the question.

AFN 1.2.1 coming out today?

cc/ @OliverLetterer since he did the original pinning implementation.

Contributor

kcharwood commented Apr 17, 2013

Thanks @mattt for getting this in. This definitely helps us address issues with Cocoapods. Looking forward to the release.

One thing I did want to bring up that may require some further investigation:

Is there any scenario where we should support SSL pinning AND an invalid certificate? Currently, the invalid SSL cert logic path assumes the pinning mode is set to none, which solves all of the use cases I've ever run into. Not sure its something we should worry about in the future, but at least wanted to ask the question.

AFN 1.2.1 coming out today?

cc/ @OliverLetterer since he did the original pinning implementation.

@mattt

This comment has been minimized.

Show comment Hide comment
@mattt

mattt Apr 18, 2013

Contributor

Just pushed version 1.2.1. A solid release if I've ever seen one. Thanks for your contributions!

Contributor

mattt commented Apr 18, 2013

Just pushed version 1.2.1. A solid release if I've ever seen one. Thanks for your contributions!

@tewha

This comment has been minimized.

Show comment Hide comment
@tewha

tewha Apr 18, 2013

Contributor

This looks like a dull release.

That's great! I love dull! Makes for an easy upgrade! :) Congrats!

Contributor

tewha commented Apr 18, 2013

This looks like a dull release.

That's great! I love dull! Makes for an easy upgrade! :) Congrats!

@OliverLetterer

This comment has been minimized.

Show comment Hide comment
@OliverLetterer

OliverLetterer Apr 18, 2013

Contributor

@kcharwood I thought the last day about if allowInvalidSSLCertificate should be respected in case SSLPinningMode is not equal to AFSSLPinningModeNone and I think it shouldn't. I wasn't sure at the beginning because with the pinning mode, a developer tells an url operation to trust a unique set of certificates. Now with allowInvalidSSLCertificate, he tells the url operation to basically trust every certificate. Therefore I was thinking about an early return in case allowInvalidSSLCertificate was set. But since we are taking about security here and the allowInvalidSSLCertificate might be set by accident (applied from an AFHTTPClient instance), the current behavior is perfect. In case the developer wants to allow invalid certificates, he should explicitly set the pinning mode to none.

Contributor

OliverLetterer commented Apr 18, 2013

@kcharwood I thought the last day about if allowInvalidSSLCertificate should be respected in case SSLPinningMode is not equal to AFSSLPinningModeNone and I think it shouldn't. I wasn't sure at the beginning because with the pinning mode, a developer tells an url operation to trust a unique set of certificates. Now with allowInvalidSSLCertificate, he tells the url operation to basically trust every certificate. Therefore I was thinking about an early return in case allowInvalidSSLCertificate was set. But since we are taking about security here and the allowInvalidSSLCertificate might be set by accident (applied from an AFHTTPClient instance), the current behavior is perfect. In case the developer wants to allow invalid certificates, he should explicitly set the pinning mode to none.

@kcharwood

This comment has been minimized.

Show comment Hide comment
@kcharwood

kcharwood Apr 18, 2013

Contributor

@OliverLetterer Sounds reasonable. I wonder if its worth adding a NSLog if you try to setup a conflict there.

Contributor

kcharwood commented Apr 18, 2013

@OliverLetterer Sounds reasonable. I wonder if its worth adding a NSLog if you try to setup a conflict there.

@OliverLetterer

This comment has been minimized.

Show comment Hide comment
@OliverLetterer

OliverLetterer Apr 18, 2013

Contributor

+1 on the NSLog statement

Sent from Mailbox for iPhone

On Thu, Apr 18, 2013 at 10:36 PM, Kevin Harwood notifications@github.com
wrote:

@OliverLetterer Sounds reasonable. I wonder if its worth adding a NSLog if you try to setup a conflict there.

Reply to this email directly or view it on GitHub:
#873 (comment)

Contributor

OliverLetterer commented Apr 18, 2013

+1 on the NSLog statement

Sent from Mailbox for iPhone

On Thu, Apr 18, 2013 at 10:36 PM, Kevin Harwood notifications@github.com
wrote:

@OliverLetterer Sounds reasonable. I wonder if its worth adding a NSLog if you try to setup a conflict there.

Reply to this email directly or view it on GitHub:
#873 (comment)

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