Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSL support #427

Open
wants to merge 10 commits into
base: stable
Choose a base branch
from
Open

SSL support #427

wants to merge 10 commits into from

Conversation

viktorasl
Copy link

@viktorasl viktorasl commented Aug 2, 2019

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 to SwifterSampleOSX target.

Closes #111 #395

How did I test it?

Using mkcert tool:

mkcert -install
mkcert -pkcs12 localhost 127.0.0.1 ::1

and then imported generated .p12 file to System keychain.

@swifter-bot
Copy link

swifter-bot commented Aug 2, 2019

1 Error
🚫 Please include a CHANGELOG entry. You can find it at CHANGELOG.md.
2 Warnings
⚠️ It seems like you’ve added new tests to the library. If that’s the case, please update the XCTestManifests.swift file running in your terminal the command swift test --generate-linuxmain.
⚠️ If you ran the command swift test --generate-linuxmain in your terminal, please remove the line testCase(IOSafetyTests.__allTests__IOSafetyTests), from public func __allTests() -> [XCTestCaseEntry] in the bottom of the file. For more reference see #366.
1 Message
📖 Hey, @viktorasl 👋.

Generated by 🚫 Danger

@Vkt0r
Copy link
Member

Vkt0r commented Aug 5, 2019

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.

@Vkt0r Vkt0r requested review from Vkt0r, adamkaplan and pvzig August 5, 2019 00:54
/// - 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
Copy link
Member

Choose a reason for hiding this comment

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

SecPKCS12Import(_:_:_:) is expecting a CFData here, what would be the difference between passing an NSData object instead of a CFData here?

Copy link
Author

Choose a reason for hiding this comment

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

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

public static func loadP12Certificate(_ _data: Data, _ password: String) throws -> CFArray {
let data = _data as NSData
let options = [kSecImportExportPassphrase: password]
var items: CFArray!
Copy link
Member

Choose a reason for hiding this comment

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

Can we please make this CFArray optional in order to avoid the force-unwrapping in the next lines?

/// - 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]
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

Related to same discussion of String, NSString and CFString I believe they're all the same.

let data = _data as NSData
let options = [kSecImportExportPassphrase: password]
var items: CFArray!
try ensureNoErr(SecPKCS12Import(data, options as NSDictionary, &items))
Copy link
Member

Choose a reason for hiding this comment

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

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) })

Copy link
Author

Choose a reason for hiding this comment

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

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.

let options = [kSecImportExportPassphrase: password]
var items: CFArray!
try ensureNoErr(SecPKCS12Import(data, options as NSDictionary, &items))
let dictionary = (items! as [AnyObject])[0]
Copy link
Member

@Vkt0r Vkt0r Aug 22, 2019

Choose a reason for hiding this comment

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

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 }

var items: CFArray!
try ensureNoErr(SecPKCS12Import(data, options as NSDictionary, &items))
let dictionary = (items! as [AnyObject])[0]
let secIdentity = dictionary[kSecImportItemIdentity] as! SecIdentity
Copy link
Member

Choose a reason for hiding this comment

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

Same here we can please avoid the force-unwrapping even when Apple does in his examples?

Copy link
Author

Choose a reason for hiding this comment

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

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

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 }
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we are dropping the first SecCertificate ?

Copy link
Author

@viktorasl viktorasl Aug 23, 2019

Choose a reason for hiding this comment

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

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.

@Vkt0r
Copy link
Member

Vkt0r commented Aug 22, 2019

I'll be reviewing more soon and I'll try to test it locally with the certificates also.

@viktorasl
Copy link
Author

I'll probably fix everything after more thorough reviews.

@Vkt0r
Copy link
Member

Vkt0r commented Aug 29, 2019

Sure, please let me know to continue with the review as it's a little long.

@Vkt0r
Copy link
Member

Vkt0r commented Sep 26, 2019

@viktorasl Are you still interested in this PR? Please continue with the rest of the changes to be able to continue reviewing it 🙂.

@viktorasl
Copy link
Author

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.

@Vkt0r
Copy link
Member

Vkt0r commented Sep 27, 2019

Ups yes it was sorry about that ☹️. Sounds good please apply the changes in order to continue. Thanks again for your PR!

@MartyCatawiki
Copy link

Any news around this? It would be really nice when SSL support is added soon.

@Vkt0r
Copy link
Member

Vkt0r commented Oct 24, 2019

@MartyCatawiki @viktorasl I would be reviewing the latest changes ASAP. Sorry for the delay 😞.

@viktorasl
Copy link
Author

Sorry it took me so long. Fixed according to comments, please check it out

@Vkt0r
Copy link
Member

Vkt0r commented Oct 29, 2019

@viktorasl I'll during the week! Thanks for the heads up.

Comment on lines 17 to 24
#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
Copy link
Member

@Vkt0r Vkt0r Nov 5, 2019

Choose a reason for hiding this comment

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

SecCopyErrorMessageString is only available for iOS 11.3+ and we're supporting iOS 8+, not sure which is the counterpart here.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! Very weird that Xcode didn't notify me.
Could not find any counterpart so formed custom message.

@Vkt0r
Copy link
Member

Vkt0r commented Nov 5, 2019

@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.

@viktorasl
Copy link
Author

@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.

@ismetanin
Copy link

@viktorasl Hi! Could we use this functionality on iOS? Maybe you can add some examples and instructions to the SwifterSampleiOS?
Hope to see this pull request merged :)

@viktorasl
Copy link
Author

@viktorasl Hi! Could we use this functionality on iOS? Maybe you can add some examples and instructions to the SwifterSampleiOS?
Hope to see this pull request merged :)

I can look into that 👍 not sure if all APIs are supported on iOS.

@ismetanin
Copy link

ismetanin commented Jan 15, 2020

Hey @viktorasl
Have you had time to take a look at that? I tried to run it on iOS but I had some problems with certificates probably or not - anyway I couldn't make it work on iOS

@alinmuntean
Copy link

Hi guys, any update on this feature?

@linhaosunny
Copy link

hei, it's that any update for this?

@linhaosunny
Copy link

@viktorasl this fork commit use ssl can't send data over 16384 bytes,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSL Support
7 participants