Certificate Pinning Verification #20
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
==========================================
- Coverage 53.12% 52.65% -0.47%
==========================================
Files 9 9
Lines 800 809 +9
==========================================
+ Hits 425 426 +1
- Misses 375 383 +8
Continue to review full report at Codecov.
|
ShihabMehboob
force-pushed
the
cert_pinning
branch
from
9fc6da3
to
4b255df
Compare
|
Hi there, any updates on this PR? |
|
@ianpartridge This PR is ready for review/merging (two Travis tests appear to fail but all pass locally so could just be an anomaly). |
| Log.warning(warning) | ||
| fallthrough | ||
| } | ||
| if let certificateData = NSData(contentsOfFile: Bundle.main.path(forResource: self.pinnedCertificateName, ofType: "der") ?? "") { |
There was a problem hiding this comment.
Why are we creating the NSData even if self.pinnedCertificateName is nil? Shouldn't we skip this whole section if it's nil?
There was a problem hiding this comment.
Added a guard to ensure it's not nil.
| completionHandler(.useCredential, URLCredential(trust: trust)) | ||
| return | ||
| } else { | ||
| completionHandler(.performDefaultHandling, nil) |
There was a problem hiding this comment.
Shouldn't we return after this? Otherwise we'll call the completion handler twice.
|
|
|
Does it cover the certificate pinning though? |
| @@ -140,6 +140,7 @@ class SwiftyRequestTests: XCTestCase { | |||
| let expectation = self.expectation(description: "Data Echoed Back") | |||
|
|
|||
| let request = RestRequest(method: .get, url: echoURLSecure, containsSelfSignedCert: true) | |||
| request.pinnedCertificateName = "" | |||
There was a problem hiding this comment.
Please can we leave this test alone, add a new testPinnedCertificate() and test with a real pinned certificate? Or is that not possible?
|
|
Verify server certificates the user is connecting to. In relation to this issue: #19