Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

AFHTTPSessionManager now throws exception if SSL pinning mode is set for non https sessions #3687

Merged
merged 1 commit into from Oct 6, 2016

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Sep 19, 2016

Before this commit

Setting a security policy configured with AFSSLPinningModeCertificate or AFSSLPinningModePublicKey on a AFHTTPSessionManager instance configured with an insecure http base URL was valid. Requests made with this manager would always succeed since the -[AFURLSessionManager URLSession:didReceiveChallenge:completionHandler:] would never be called and thus the security policy would never be evaluated.

After this commit

Setting a security policy configured with AFSSLPinningModeCertificate or AFSSLPinningModePublicKey on a AFHTTPSessionManager instance configured with an insecure http base URL will throw an exception. This will force the manager to be configured with a secure https URL.

Note that properly configuring App Transport Security (ATS) would also solve this issue since insecure connections would fail anyway, but this is a belt and suspenders solution.

@codecov-io
Copy link

codecov-io commented Sep 19, 2016

Current coverage is 86.75% (diff: 100%)

Merging #3687 into master will increase coverage by 0.12%

@@             master      #3687   diff @@
==========================================
  Files            44         44          
  Lines          6067       6110    +43   
  Methods        1079       1088     +9   
  Messages          0          0          
  Branches        407        408     +1   
==========================================
+ Hits           5256       5301    +45   
+ Misses          808        806     -2   
  Partials          3          3          

Powered by Codecov. Last update e337471...f9ed2d2

@@ -98,6 +98,22 @@ - (void)setResponseSerializer:(AFHTTPResponseSerializer <AFURLResponseSerializat
[super setResponseSerializer:responseSerializer];
}

- (void)setSecurityPolicy:(AFSecurityPolicy *)securityPolicy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a note to the documentation declaring this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be a good idea. The only issue is that this behaviour only applies at the AFHTTPSessionManager level (as AFURLSessionManager has no base URL). So, should we redefine the securityPolicy property on AFHTTPSessionManager for the sole purpose of augmenting its documentation? Or is it going to be more confusing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be a good idea, since that behavior only affects AFHTTPSessionManager

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I just rebased on master and force-pushed with documentation.

@kcharwood kcharwood added this to the 3.2.0 milestone Sep 19, 2016
@0xced 0xced force-pushed the invalid-security-policy branch 5 times, most recently from e4d3f1b to 181d633 Compare September 21, 2016 21:53
@0xced
Copy link
Contributor Author

0xced commented Sep 22, 2016

After much struggling, all tests finally pass!

@kcharwood
Copy link
Contributor

@0xced awesome!

I think it would make sense to cherry-pick the tests stability into its on PR, so we can have a direct reference to that and it doesn't get lost in this changeset. Would you be able to do that? If not, I can take a stab at it.

@0xced
Copy link
Contributor Author

0xced commented Sep 22, 2016

Yes, I will do it.

@0xced 0xced force-pushed the invalid-security-policy branch 3 times, most recently from 424fd01 to e698f45 Compare September 23, 2016 13:08
@kcharwood
Copy link
Contributor

Can you rebase this with master to get Xcode 8 coverage?

…URLs

### Before this commit
Setting a security policy configured with `AFSSLPinningModeCertificate` or `AFSSLPinningModePublicKey` on a AFHTTPSessionManager instance configured with an insecure `http` base URL was valid. Requests made with this manager would always succeed since the `-[AFURLSessionManager URLSession:didReceiveChallenge:completionHandler:]` would never be called and thus the security policy would never be evaluated.

### After this commit
Setting a security policy configured with `AFSSLPinningModeCertificate` or `AFSSLPinningModePublicKey` on a AFHTTPSessionManager instance configured with an insecure `http` base URL will throw an exception. This will force the manager to be configured with a secure `https` URL.

Note that properly configuring App Transport Security (ATS) would also solve this issue since insecure connections would fail anyway, but this is a *belt and suspenders* solution.
@0xced
Copy link
Contributor Author

0xced commented Oct 5, 2016

I rebased, but I think you should be able to rebase and merge from the GitHub interface now.

@kcharwood
Copy link
Contributor

I wanted to see the green check before merging!

@0xced
Copy link
Contributor Author

0xced commented Oct 5, 2016

I realized right after posting. 😄

@kcharwood kcharwood added the added label Oct 6, 2016
@kcharwood kcharwood changed the title Make it impossible to set a security policy with pinning on insecure URLs AFHTTPSessionManager now throws exception if SSL pinning mode is set for non https sessions Oct 6, 2016
@kcharwood kcharwood merged commit a26a500 into AFNetworking:master Oct 6, 2016
@kcharwood kcharwood changed the title AFHTTPSessionManager now throws exception if SSL pinning mode is set for non https sessions AFHTTPSessionManager now throws exception if SSL pinning mode is set for non https sessions Oct 11, 2016
@0xced 0xced deleted the invalid-security-policy branch October 11, 2016 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants