Certificate Pinning Verification#20
Conversation
ianpartridge
left a comment
ianpartridge
left a comment
There was a problem hiding this comment.
You seem to have changed all the spaces to tabs.
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). |
ianpartridge
left a comment
ianpartridge
left a comment
There was a problem hiding this comment.
A few comments, plus there's no test yet.
| 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? |
| 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?
|
|
5 participants
Verify server certificates the user is connecting to. In relation to this issue: #19