Pluggable, more flexible, security policies. #429

michaelkirk · 2016-06-29T16:05:08Z

Motiviation

Extract @FredericJacobs' CertificateVerifier concept with @nlutsenko's
SRSecurityOptions into a pluggable SRSecurityPolicy protocol

This retains existing SSL configuration code paths, while allowing users
more flexibility to specify their own security policy. It's intended
that you're able to subclass an AFSecurityPolicy that conforms to the
<SRSecurityPolicy> protocol to share policy between websocket and
conventional requests. This approach was changed. SRSecurityPolicy is now it's own class.

Inspired by original "Require TLS 1.2 & enable pinning" pull request by
Frederic Jacobs (@FredericJacobs) at: https://github.com/facebook/SocketRocket/pull/274/files

TODO

There are a couple things in particular that were kind of quick hacks, but wanted to get some feedback on the approach:

Testing
I added a synchronized block in didConnect. Maybe we can refactor so this isn't reached at so many different points
Should SecurityPolicyBuilder be public?
Are you OK with defaulting to TLS 1.2 only?
~~Implement no-encryption debug~~

ghost · 2016-06-29T16:05:56Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

Pluggable policies are currently only in our fork, not upsstream, but pending PR: facebookincubator/SocketRocket#429 Also: * adapt to new upstream API for error handling (vs previous exception throwing) * renamed AFSecurityOWSPolicy.m -> OWSTransportSecurityPolicy.m since it's being sued outside of AFNetworking * Follow conventional singleton pattern with oneToken // FREEBIE

ghost · 2016-06-29T17:02:37Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Pluggable policies are currently only in our fork, not upsstream, but pending PR: facebookincubator/SocketRocket#429 Also: * adapt to new upstream API for error handling (vs previous exception throwing) * renamed AFSecurityOWSPolicy.m -> OWSTransportSecurityPolicy.m since it's being sued outside of AFNetworking * Follow conventional singleton pattern with oneToken // FREEBIE

nlutsenko · 2016-06-29T18:04:55Z

Love this PR, I think this is definitely the best of both worlds combined into one, with some polish required.

Putting few answers here the TODO questions you had:

Should SecurityPolicyBuilder be public?

Absolutely! I think we can structure it slightly differently, but creating different policies and using them should be public.

Are you OK with defaulting to TLS 1.2 only?

Yes, as long as we make sure we are no restricting only to it, meaning people can downgrade security to SSLv3 or TLS 1.1.

Implement no-encryption debug

I am less worried about this to be honest. When it comes to security - I think it's absolutely valid to test and develop with all security options turned on, otherwise - you are not testing the production flow.

nlutsenko · 2016-06-29T18:06:56Z

SocketRocket/SRSecurityPolicy.h

@@ -0,0 +1,25 @@
+//
+// Copyright 2012 Square Inc.


Nit: Since this is new code added and is not part of the original codebase - let's use the copyright statement from SRSecurityPolicyBuilder.

nlutsenko · 2016-06-29T18:27:08Z

Few initial comments and nits. The most crucial one is around requestRequiresSSL and changes around it.

Also, around SRSecurityPolicyBuilder I think it's closer to Cocoa standards to use class cluster pattern here instead of Factory one. A good example is NSURLSessionConfiguration, where it has class constructors for creating different configurations or you can subclass it or you can create one and set it up.

So here, we would do SRSecurityPolicy as a class with the same methods as the protocol, as well as class constructors for other policy subclasses that are internal, but control the behavior.

@michaelkirk, let me know your thoughts about all of this, and whether it makes sense.

michaelkirk · 2016-06-29T19:21:00Z

So here, we would do SRSecurityPolicy as a class with the same methods as the protocol, as well as class constructors for other policy subclasses that are internal, but control the behavior.

One concern we are trying to address is sharing a security policy from AFNetworking. It probably is more straight forward to implement a SRSecurityPolicy class like you've said, and then just delegate evaluateTrustForDomain:WithTrust to our AFSecurityPolicy.

Pluggable policies are currently only in our fork, but pending upstream PR: facebookincubator/SocketRocket#429 Also: * rebased SocketRocket against latest upstream to incorporate bug fixes. * adapt to new upstream API for error handling (vs previous exception throwing) * renamed AFSecurityOWSPolicy -> OWSHTTPSecurityPolicy to differentiate it from OSWWebSocketSecurityPolicy * Follow conventional singleton pattern with onceToken // FREEBIE

michaelkirk · 2016-06-30T01:15:03Z

OK, I've refactored the Factory/Protocol into a couple of concrete classes. Take a peak and let me know what you think.

I'm leaving my commits separate for the sake of a PR dialogue, but would suggest smashing them when merging.

michaelkirk · 2016-06-30T01:16:46Z

Here's an example of how we're using this to delegate domain verification to our existing AFNetworking security policy: https://github.com/WhisperSystems/SignalServiceKit/pull/15/files#diff-8aea6ac66bd50c6e854b63f64f7edeb9R1

nlutsenko · 2016-06-30T20:04:12Z

SocketRocket/Internal/Security/SRPinningSecurityPolicy.m

+
+@interface SRPinningSecurityPolicy ()
+
+@property (nonatomic, readonly) NSArray *pinnedCertificates;


nonatomic, copy, readonly

nlutsenko · 2016-06-30T21:48:30Z

This actually looks perfect now!
I am going to merge this in once Travis passes on it.

nlutsenko · 2016-06-30T21:48:41Z

Thank you so much for making this happen!

michaelkirk · 2016-06-30T23:28:26Z

Installing cocoapods-trunk 1.0.0
Using xcpretty 0.2.2
Installing activesupport 5.0.0
Gem::InstallError: activesupport requires Ruby version >= 2.2.2.
An error occurred while installing activesupport (5.0.0), and Bundler cannot
continue.

Ha. I wonder how many other people's CI failed due to today's Rails 5 release. :\

michaelkirk · 2016-06-30T23:54:31Z

Can you restart the build after #430 (or something similar) is merged?

nlutsenko · 2016-07-01T00:10:35Z

I believe this needs rebase actually, since it's going to test your specific branch and not the commit applied on top of master.

nlutsenko · 2016-07-01T00:44:38Z

This indeed fails tests, I am investigating right now.

nlutsenko · 2016-07-01T00:45:22Z

I bet it's due to the fact that we are still trying to validate the security policy, even though it's not required.

nlutsenko · 2016-07-01T00:47:33Z

SocketRocket/SRWebSocket.m

-
-    SRDebugLog(@"Allows connection any root cert: %d", _securityOptions.validatesCertificateChain);
-    SRDebugLog(@"Pinned cert count: %d", _securityOptions.pinnedCertificates.count);
+    [_securityPolicy updateSecurityOptionsInStream:_inputStream];


Failing tests and probably is going to fail to connect to http hosts with this.
Please wrap 474-475 in a if (_requestRequiresSSL) check.

@FredericJacobs

Extract @FredericJacobs' CertificateVerifier concept with @nlutsenko's SRSecurityOptions into a pluggable SRSecurityPolicy model This retains existing SSL configuration code paths, while allowing users more flexibility to specify their own security policy. If you are alread using AFNetworking and an `AFSecurityPolicy`, it's intended that you can share domain trust logic by delegating `SRSecurityPolicy evaluateTrust:ForDomain` to your AFSecurityPolicy instance. Inspired by original "Require TLS 1.2 & enable pinning" pull request by Frederic Jacobs (@FredericJacobs) at: https://github.com/facebook/SocketRocket/pull/274/files

nlutsenko · 2016-07-01T01:19:48Z

Should be all good now, let's wait for Travis 😁

Pluggable policies are currently only in our fork, but pending upstream PR: facebookincubator/SocketRocket#429 Also: * rebased SocketRocket against latest upstream to incorporate bug fixes. * adapt to new upstream API for error handling (vs previous exception throwing) * renamed AFSecurityOWSPolicy -> OWSHTTPSecurityPolicy to differentiate it from OSWWebSocketSecurityPolicy * Follow conventional singleton pattern with onceToken // FREEBIE

* Use SocketRocket pluggable policies Pluggable policies are currently only in our fork, but pending upstream PR: facebookincubator/SocketRocket#429 Also: * rebased SocketRocket against latest upstream to incorporate bug fixes. * adapt to new upstream API for error handling (vs previous exception throwing) * renamed AFSecurityOWSPolicy -> OWSHTTPSecurityPolicy to differentiate it from OSWWebSocketSecurityPolicy * Follow conventional singleton pattern with onceToken * bump xcode version to play nice with SWIFT_NAME in SocketRocket // FREEBIE

michaelkirk · 2016-07-01T03:59:51Z

Great. Thanks for your prompt attention @nlutsenko! 🙏

@FredericJacobs

Extract @FredericJacobs' CertificateVerifier concept with @nlutsenko's SRSecurityOptions into a pluggable SRSecurityPolicy model This retains existing SSL configuration code paths, while allowing users more flexibility to specify their own security policy. If you are alread using AFNetworking and an `AFSecurityPolicy`, it's intended that you can share domain trust logic by delegating `SRSecurityPolicy evaluateTrust:ForDomain` to your AFSecurityPolicy instance. Inspired by original "Require TLS 1.2 & enable pinning" pull request by Frederic Jacobs (@FredericJacobs) at: https://github.com/facebook/SocketRocket/pull/274/files

ghost added the GH Review: review-needed label Jun 29, 2016

michaelkirk force-pushed the pluggable-security-policies branch from 1af96bf to caa2765 Compare June 29, 2016 17:02

ghost added the CLA Signed label Jun 29, 2016

michaelkirk mentioned this pull request Jun 29, 2016

Use SocketRocket pluggable policies signalapp/SignalServiceKit#15

Merged

nlutsenko mentioned this pull request Jun 29, 2016

Require TLS 1.2 & enable pinning. #274

Closed

nlutsenko reviewed Jun 29, 2016
View reviewed changes

nlutsenko added GH Review: needs-revision and removed GH Review: review-needed labels Jun 29, 2016

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jun 29, 2016

nlutsenko reviewed Jun 30, 2016
View reviewed changes

nlutsenko added the GH Review: accepted label Jun 30, 2016

nlutsenko removed the GH Review: review-needed label Jul 1, 2016

michaelkirk force-pushed the pluggable-security-policies branch from 6f13c5a to 01ed1be Compare July 1, 2016 00:12

nlutsenko reviewed Jul 1, 2016
View reviewed changes

nlutsenko added GH Review: needs-revision and removed GH Review: accepted labels Jul 1, 2016

michaelkirk force-pushed the pluggable-security-policies branch from 01ed1be to cb2cf16 Compare July 1, 2016 01:18

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jul 1, 2016

nlutsenko added GH Review: accepted and removed GH Review: review-needed labels Jul 1, 2016

nlutsenko merged commit 8096fef into facebookincubator:master Jul 1, 2016

michaelkirk mentioned this pull request Jul 1, 2016

Get back on upstream SocketRocket signalapp/Signal-iOS#1241

Merged

nlutsenko mentioned this pull request Jul 12, 2016

Evalute ssl cert chain with hostname policy for leaf cert. #328

Closed

nlutsenko added this to the 0.6.0 milestone Oct 31, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pluggable, more flexible, security policies. #429

Pluggable, more flexible, security policies. #429

michaelkirk commented Jun 29, 2016 •

edited

Loading

ghost commented Jun 29, 2016

ghost commented Jun 29, 2016

nlutsenko commented Jun 29, 2016 •

edited

Loading

nlutsenko Jun 29, 2016

nlutsenko commented Jun 29, 2016

michaelkirk commented Jun 29, 2016

michaelkirk commented Jun 30, 2016

michaelkirk commented Jun 30, 2016 •

edited

Loading

nlutsenko Jun 30, 2016

nlutsenko commented Jun 30, 2016

nlutsenko commented Jun 30, 2016

michaelkirk commented Jun 30, 2016 •

edited

Loading

michaelkirk commented Jun 30, 2016

nlutsenko commented Jul 1, 2016

nlutsenko commented Jul 1, 2016

nlutsenko commented Jul 1, 2016

nlutsenko Jul 1, 2016

nlutsenko commented Jul 1, 2016

michaelkirk commented Jul 1, 2016


		@interface SRPinningSecurityPolicy ()

		@property (nonatomic, readonly) NSArray *pinnedCertificates;

Pluggable, more flexible, security policies. #429

Pluggable, more flexible, security policies. #429

Conversation

michaelkirk commented Jun 29, 2016 • edited Loading

Motiviation

TODO

ghost commented Jun 29, 2016

ghost commented Jun 29, 2016

nlutsenko commented Jun 29, 2016 • edited Loading

nlutsenko Jun 29, 2016

Choose a reason for hiding this comment

nlutsenko commented Jun 29, 2016

michaelkirk commented Jun 29, 2016

michaelkirk commented Jun 30, 2016

michaelkirk commented Jun 30, 2016 • edited Loading

nlutsenko Jun 30, 2016

Choose a reason for hiding this comment

nlutsenko commented Jun 30, 2016

nlutsenko commented Jun 30, 2016

michaelkirk commented Jun 30, 2016 • edited Loading

michaelkirk commented Jun 30, 2016

nlutsenko commented Jul 1, 2016

nlutsenko commented Jul 1, 2016

nlutsenko commented Jul 1, 2016

nlutsenko Jul 1, 2016

Choose a reason for hiding this comment

nlutsenko commented Jul 1, 2016

michaelkirk commented Jul 1, 2016

michaelkirk commented Jun 29, 2016 •

edited

Loading

nlutsenko commented Jun 29, 2016 •

edited

Loading

michaelkirk commented Jun 30, 2016 •

edited

Loading

michaelkirk commented Jun 30, 2016 •

edited

Loading