Discussion around Credential/SSL Pinning/Invalid SSL Handling.

[kcharwood]

I've been digging a bit deeper into the handling of credentials/ssl pinning, and invalid ssl handling, specifically looking at it from a testing perspective and what can be done to make that code more testable with the good work @blakewatters is doing in #985. I've found some interesting behavior here, and wanted to make a few suggestions as well as ask a question about unusual behavior I am seeing.

cc'ing @OliverLetterer, who originally implemented the pinning code.

Issue 1: Clean Up

The docs for connection:willSendRequestForAuthenticationChallenge state the following:

"This method allows the delegate to make an informed decision about connection authentication at once. If the delegate implements this method, it has no need to implement connection:canAuthenticateAgainstProtectionSpace:, connection:didReceiveAuthenticationChallenge:, connectionShouldUseCredentialStorage:. In fact, these other methods are not invoked."

The last comment is interesting, meaning that those methods are not even invoked if connection:willSendRequestForAuthenticationChallenge is implemented, which in AF's case, is true when _AFNETWORKING_PIN_SSL_CERTIFICATES_ is defined. Also note that when _AFNETWORKING_PIN_SSL_CERTIFICATES_ is defined, the other three methods are still implemented, even though they will never be called.

With that being said, I think that at a minimum those methods should only be implemented if _AFNETWORKING_PIN_SSL_CERTIFICATES_ is not defined, which would mean we need to add an #elseif to only implement those in that condition. (And potentially only expose the callback blocks in that same condition)

Additionally, I know that the reason we have that macro is because there was no way to detect if the Security Framework was added dynamically. I understand the original intent of this (as well as MobileCoreServices and SystemConfiguration) was to support easy library integration, and not require developers to have to link to other frameworks to get started. However, with the booming popularity of CocoaPods, I wonder if its time to leverage CocoaPods as the official means of integrating AFNetworking into your project, and drop support for dynamic framework linking. We could of course include manual install instructions, and require linking against the three frameworks there, but I think that will improve the readability of the codebase, and make it more maintainable going forward.

If we go down that route, then I think we can get rid of connection:canAuthenticateAgainstProtectionSpace:, connection:didReceiveAuthenticationChallenge:, connectionShouldUseCredentialStorage: entirely, as well as their callback blocks, since they are not invoked if connection:willSendRequestForAuthenticationChallenge is implemented.

Potential Outcome:

Remove the optional links to MobileCoreServices, SystemConfiguration, and Security. Move to CocoaPods as official install mechanism. Delete unneeded NSURLConnectionDelegate callback methods and blocks.

Issue 2: How do we Clear Credentials?

I've ran into interesting issue making this code more testable. Specifically, I can't find a way to remove a NSURLCredential for a NSURLProtectionSpace after it has been used once.

This actually has some subpar behavior. Specifically, if you set allowsInvalidSSLCertificate to YES once, that credential is set, it is automatically applied for every request with that host going forward, regardless of whether or not allowsInvalidSSLCertificate is set to NO. There is similar behavior for the pinning options as well. From a testing perspective, we'll need to be able to reset those states and ensure code is being handling correctly.

I've tried peeking into NSURLCredentialStorage, but there is no credential stored there, which seems wrong to me but I'm still feeling my way around trying to understand where that credential is actually being stored. Note that the method we are using to create the credential (credentialForTrust) is the only method that doesn't support passing a NSURLCredentialPersistence as a parameter, so I don't know how it's being persisted under the hood.

Additionally, I'm blasting away all cookies to ensure they aren't applied either, so that's not causing the problem.

Essentially, I'm trying to find a way to force connection:willSendRequestForAuthenticationChallenge: to be called again in the future for the same NSURLAuthenticationChallenge, but I can't get it to work. @OliverLetterer, any ideas?

[kcharwood]

More notes from NSURLConnectionDelegate:

connection:canAuthenticateAgainstProtectionSpace:
connection:didReciveAuthenticationChallenge:
connection:didCancelAuthenticationChallenge: are
deprected and new code should adopt
connection:willSendRequestForAuthenticationChallenge.
The older delegates will still be called for
compatability, but incur more latency in dealing
with the authentication challenge.

[kcharwood]

More notes I've dug up from The Eskimo on Apple Dev Forums:

Finally, be aware that NSURLCredentialPersistence is currently meaningful only for username and password credentials; client identity and server trust credentials are always treated as NSURLCredentialPersistenceNone.

[kcharwood]

More notes from The Eskimo.

[kcharwood]

Just found this Tech Q&A.

That's not encouraging. From a high level, TLS caches expire about every 10 minutes, meaning that once you set allowsSSLCertificate to YES for a specific host, that credential is cached for 10 minutes, regardless of if you set it to NO at some point in the future during that 10 minutes session. I have seen one use case here where someone was prompting a user to accept a cert, and giving them the ability to revoke it at anytime. He was then seeing the request continue to succeed because connection:willSendRequestForAuthenticationChallenge: was not being called again.

Would be similar outcome for the pinning values as well.

This concerns me, both from a testability perspective, and from a user expectation perspective. I'm going to need to sit on this for a bit and see what I can come up with.

[OliverLetterer]

Very very interesting about connection:willSendRequestForAuthenticationChallenge: and the 10 minute TLS cache. With regard to testability:

I tried to find the cache or session storing the credentials and manually invalidating them but as far as my reversing goes, NSURLConnectionInternalConnection is using CFURLConnection internally. I think CFURLConnection is using a C++ based URLConnection internally as well and I found some CredentialStorages and some CFURLStorageCopyCredentialStorages. So I think we would need to go very deep ddep down in CFNetworking and C++ if we would want to invalidate this cache on our own. I guess this is off the hook for us here.

For testability, we could simply run our own server on multiple ports?

[kcharwood]

@OliverLetterer it's actually the TLS cache that is catching us here. If you wait 10 minutes, those callbacks get called again.

All the public documentation states it is impossible to programmatically reset that cache... May need to find a private hook for testing, or hit a custom server with support for random subdomains.

[kcharwood]

If anyone is interested in following the Dev Forum Post I have opened up (or chiming in to try and get it more attention), you can find it here.

[0xced]

I’ve filed radar #12702749 (duplicate of #11975686 initially filed by @tjw) which describes a very similar problem, maybe even the exact same problem.

[kcharwood]

@0xced I think that issue filed by @tjw is very close to mine here, but based on the documentation I've compiled here I think it has more to do with the TLC cache than an HTTP Keep Alive.

[kcharwood]

Actually I stand corrected, that could be the root cause of this issue here.

[kcharwood]

The Eskimo replied here. Looks like there is no way to programmatically clear it.

At a minimum we are going to need to improve documentation would the SSL Pinning, Credential, and Invalid SSL certs to let devs know about the cached behavior under the hood.

[kcharwood]

If anyone is interested in filing more radars, we can dupe 8957312, which requests the ability to flush the TLS cache. That is doc'd in the Tech Q&A from Apple, so the more people that complain, the more likely we can get something done about it.

@mattt Any thoughts at dropping dynamic linking? If you're not interested in doing that just yet, I would recommend added the #elseif around the 3 delegate methods that are not called then _AFNETWORKING_PIN_SSL_CERTIFICATES_ is set, and wrap the corresponding blocks as well that are never called. Preferably, we just dump dynamic linking and delete some code.

[kcharwood]

@0xced The Eskimo confirmed that #11975686 is not related to this specific issue, so feel free to dupe #8957312.

[mattt]

Thanks again for your excellent insight on all of this @kcharwood.

After giving it more thought, it seems like 2.0 would be a good occasion to drop dynamic linking, since everything is on the table. The timing of 2.0 is within the next couple of months, so hopefully that is soon enough to not be a drag.

[Jagadeeshwar-Reddy]

connection will send request methos it is not call

[robmaceachern]

For anyone still running into TLS caching issues, Apple has updated the technical QA1727 on Aug 19 2015 with a note about NSURLSession:

Important: This solution can better be achieved by adopting the NSURLSession API. Using this framework, you can created two NSURLSessions, one for each desired connection. Since NSURLSession maintains it's own TLS session cache, the second connection will avoid consulting the first cache, and will return the authentication challenge.

[Koshub]

@robmaceachern it is not true. Credentials are cached in TLS session for 10 minutes for different NSURLSession instances

[robmaceachern]

@Koshub you're right. According to Quinn on the dev forums:

this feature got broken at some point (I think with iOS 8) and wasn’t fixed until iOS 9

Sigh

https://forums.developer.apple.com/message/49334

[Koshub]

@robmaceachern Thanks indeed.