Require TLS 1.2 & enable pinning.

[FredericJacobs]

• Enforce TLS 1.2 connections for secure connections. Prevents downgrade attacks
• I think a lot of applications are using this library along with AFNetworking or another HTTP library, therefore I abstracted out the pinning so that they can write their own pinning logic according to their needs. It can also help issues like Proxy support with SSL #265 where they might have to allow proxying certificates or others.
• if ([trustedCertData isEqualToData:certData]) is a very naïve way to do pinning. Certificates might be re-issued and the expiration dates or validation of the domain is done nowhere. It shouldn't matter too much if certificates are pinned but implementers would enjoy getting some more flexibility there.

[dfed]

Thanks for the PR @FredericJacobs . Before we can consider this PR for review, please make sure that CI succeeds (might just require rebasing master, which is green) and that you've signed our CLA.

[FredericJacobs]

Done

[dfed]

Your build is failing CI due to the addition of libobjc.tbd:

Check dependencies
warning: skipping file '/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator8.4.sdk/usr/lib/libobjc.tbd' (unexpected file type 'file' in Frameworks & Libraries build phase)
** BUILD FAILED **

[FredericJacobs]

Looks like the issue is related to running Xcode 7 => https://forums.developer.apple.com/thread/4572

That's all I have on this dev machine.

[dfed]

Our CI is running Xcode 6.4 on a 10.9 machine, so I don't think Xcode 7 is the issue.

[FredericJacobs]

Sorry, I wasn't clear. I added the file to the project with Xcode 7 and there are known issues with Xcode 7 adding .tbd files for libraries.

[dfed]

Got it. We can't merge something that will break old clients though, so this is an issue that will need to be resolved before we can address the rest of the PR.

[mikelikespie]

@FredericJacobs Thanks for the PR!

I'm going to ask some of our infosec people to look at this (since they're more familiar with this topic).

Anyways, in the meantime, I have a couple qs:

• Could we add a unit test or two for this behavior?
• Will the 1.2 requirements break backwards compatibility?
• Could we keep the SR_SSLPinnedCertificates property and mark it deprecated? I don't want to break backwards compatibility. In a future PR, we can pull this out after sufficient deprecation time.

Thanks!
Mike

[FredericJacobs]

1. Tests: Sure that's possible.
2. TLS 1.2: Depends on server configuration. But if you're going to have a server that does WebSocket, it should run TLS1.2 without any issues. I see no valid reason for not requiring it.
3. Sure, that's possible.

No time in the next two weeks but could consider writing tests and deprecation later.

[emonti]

Aforementioned infosec person chiming in here: I think the right choice here is definitely to support (edit: exclusive) TLS 1.2 for SocketRocket ASAP, but make it configurable since there are (unfortunately) a non-zero number of web-socket servers out in existence that don't yet support it as well.

On a quick pass, this PR looks like this would make TLS 1.2 mandatory. The best solution would be to bubble up some interface to set the socket security level to the user. I'd strongly encourage SR making TLS 1.2 the default, though.

[kwigginton]

Hi @FredericJacobs could you please take a look at @emonti 's requests?

We would love to take this PR, this is just a general ping!

[dfed]

Pinging again. We'd love to take this.

[FredericJacobs]

@dfed : What exactly are you interested in? Protection against fallback? Custom trust chain evaluation? It will be a breaking change, so I think it makes sense to bundle it with the next major version update.

[dfed]

Got it. Since we still haven't released a major version I guess we're okay to make this a breaking change. @mikelikespie, any strong opinions here?

[michaelkirk]

There's been some pretty substantial refactoring of upstream since you posed this PR @FredericJacobs.

I'm working on a rebase of your work now, but if you are so inclined to take a stab, let me know, otherwise I'll just tag you to review.

[nlutsenko]

Superseded by #429, continuing the discussion there. Thanks everyone for the comments and feedback on this.