SSL support #427
SSL support #427
Conversation
Generated by 🚫 Danger |
|
Hey, @viktorasl Thanks for taking the time to create this. I'll try to check your PR during this week, can you please in the meantime fix the issues with Danger? Thanks. |
XCode/Sources/TlsSession.swift
Outdated
| /// - Parameter _data: .p12 certificate file content | ||
| /// - Parameter password: password used when importing certificate | ||
| public static func loadP12Certificate(_ _data: Data, _ password: String) throws -> CFArray { | ||
| let data = _data as NSData |
There was a problem hiding this comment.
SecPKCS12Import(_:_:_:) is expecting a CFData here, what would be the difference between passing an NSData object instead of a CFData here?
There was a problem hiding this comment.
Most of Core Foundation types are toll-free bridged to their Cocoa Foundation counterparts and can be used interchangeably. But I agree, it's better to use actually expected type names here. Will fix that.
/cc https://developer.apple.com/documentation/corefoundation/cfdata-rv9
/cc https://developer.apple.com/library/archive/documentation/General/Conceptual/CocoaEncyclopedia/Toll-FreeBridgin/Toll-FreeBridgin.html
XCode/Sources/TlsSession.swift
Outdated
| public static func loadP12Certificate(_ _data: Data, _ password: String) throws -> CFArray { | ||
| let data = _data as NSData | ||
| let options = [kSecImportExportPassphrase: password] | ||
| var items: CFArray! |
There was a problem hiding this comment.
Can we please make this CFArray optional in order to avoid the force-unwrapping in the next lines?
XCode/Sources/TlsSession.swift
Outdated
| /// - Parameter password: password used when importing certificate | ||
| public static func loadP12Certificate(_ _data: Data, _ password: String) throws -> CFArray { | ||
| let data = _data as NSData | ||
| let options = [kSecImportExportPassphrase: password] |
There was a problem hiding this comment.
In the way Apple does it in the page you referenced they make a cast to String like this:
let options = [ kSecImportExportPassphrase as String: password ]Do we know what difference could generate?
There was a problem hiding this comment.
Related to same discussion of String, NSString and CFString I believe they're all the same.
XCode/Sources/TlsSession.swift
Outdated
| let data = _data as NSData | ||
| let options = [kSecImportExportPassphrase: password] | ||
| var items: CFArray! | ||
| try ensureNoErr(SecPKCS12Import(data, options as NSDictionary, &items)) |
There was a problem hiding this comment.
SecPKCS12Import(_:_:_:) is expecting a UnsafeMutablePointer<CFArray?>What are differences here between pass a UnsafeMutablePointer<CFArray?> and raw CFArray?. Wouldn't be more type-safe to pass a type like this:
try ensureNoErr(withUnsafeMutablePointer(to: &items) { SecPKCS12Import(data, options as CFDictionary, $0) })There was a problem hiding this comment.
Not fully sure what &items generates but I assume it's something like a pointer to CFArray. Used https://developer.apple.com/documentation/security/certificate_key_and_trust_services/identities/importing_an_identity as an example.
XCode/Sources/TlsSession.swift
Outdated
| let options = [kSecImportExportPassphrase: password] | ||
| var items: CFArray! | ||
| try ensureNoErr(SecPKCS12Import(data, options as NSDictionary, &items)) | ||
| let dictionary = (items! as [AnyObject])[0] |
There was a problem hiding this comment.
The items would be of [[String: Any]], can we please cast it as optional and use firsts instead of the first index directly? Something like this maybe:
guard let dictionary = (items as? [[String: Any]]).first else { throw SomeError }
XCode/Sources/TlsSession.swift
Outdated
| var items: CFArray! | ||
| try ensureNoErr(SecPKCS12Import(data, options as NSDictionary, &items)) | ||
| let dictionary = (items! as [AnyObject])[0] | ||
| let secIdentity = dictionary[kSecImportItemIdentity] as! SecIdentity |
There was a problem hiding this comment.
Same here we can please avoid the force-unwrapping even when Apple does in his examples?
There was a problem hiding this comment.
Well, this cannot quite be avoided, because when trying to make it optional compiler produces
Conditional downcast to CoreFoundation type 'SecIdentity' will always succeed error
XCode/Sources/TlsSession.swift
Outdated
| let dictionary = (items! as [AnyObject])[0] | ||
| let secIdentity = dictionary[kSecImportItemIdentity] as! SecIdentity | ||
| let chain = dictionary[kSecImportItemCertChain] as! [SecCertificate] | ||
| let certs = [secIdentity] + chain.dropFirst().map { $0 as Any } |
There was a problem hiding this comment.
Any reason we are dropping the first SecCertificate ?
There was a problem hiding this comment.
from SSLSetCertificate:
You must place in certRefs[0] a SecIdentityRef object that identifies the leaf certificate and its corresponding private key
so secIdentity already identifies leaf certificate so that leaf certificate should be dropped from chain otherwise chain becomes invalid due to duplicate certs.
|
I'll be reviewing more soon and I'll try to test it locally with the certificates also. |
|
I'll probably fix everything after more thorough reviews. |
|
Sure, please let me know to continue with the review as it's a little long. |
|
@viktorasl Are you still interested in this PR? Please continue with the rest of the changes to be able to continue reviewing it 🙂. |
|
Hm, miscommunication happened. I was about to apply changes after your full review. But as I understand that I should apply changes now. I'll do that. |
|
Ups yes it was sorry about that |
|
Any news around this? It would be really nice when SSL support is added soon. |
|
@MartyCatawiki @viktorasl I would be reviewing the latest changes ASAP. Sorry for the delay 😞. |
|
Sorry it took me so long. Fixed according to comments, please check it out |
|
@viktorasl I'll during the week! Thanks for the heads up. |
XCode/Sources/Errno.swift
Outdated
| #if !os(Linux) | ||
| public class func sslError(from status: OSStatus) -> Error { | ||
| guard let msg = SecCopyErrorMessageString(status, nil) else { | ||
| return SocketError.tlsSessionFailed("<\(status): message is not provided>") | ||
| } | ||
| return SocketError.tlsSessionFailed(msg as NSString as String) | ||
| } | ||
| #endif |
There was a problem hiding this comment.
SecCopyErrorMessageString is only available for iOS 11.3+ and we're supporting iOS 8+, not sure which is the counterpart here.
There was a problem hiding this comment.
Good catch! Very weird that Xcode didn't notify me.
Could not find any counterpart so formed custom message.
|
@viktorasl Can you please compile your PR with Xcode 11.1. I'm getting errors trying to compile it in order to continue the review. Thanks. |
Found the issue regarding force cast and added swiftlint rule ignore for that one particular line. Hope it solves the problem. |
|
@viktorasl Hi! Could we use this functionality on iOS? Maybe you can add some examples and instructions to the |
I can look into that 👍 not sure if all APIs are supported on iOS. |
|
Hey @viktorasl |
|
Hi guys, any update on this feature? |
|
hei, it's that any update for this? |
|
@viktorasl this fork commit use ssl can't send data over 16384 bytes, |
Motivation
To have SSL support and be able to use HTTPS. The use cases of swifter library will be much broader.
Our company use swifter to run mock API server in iOS UI tests (https://github.com/treatwell/twuitests), but currently arbitrary loads have to be enabled in iOS app because iOS enforces using SSL connections.
What's in this PR
This PR introduces SSL support using TLS session on Darwin OS (if everything will turn out fine I'll try adding support for Linux OS as well).
Demo optional
localhost(if appears at~/.swifter/localhost.p12) certificate loading is added toSwifterSampleOSXtarget.Closes #111 #395
How did I test it?
Using
mkcerttool:and then imported generated
.p12file to System keychain.